diff --git a/Makefile b/Makefile index c2ba222d..1fe2ea02 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build build-ui build-go test bench bench-baseline bench-compare lint fmt vet fix install-hooks clean ko-build build-wfctl +.PHONY: build build-ui build-go test bench bench-baseline bench-compare lint fmt vet fix install-hooks clean ko-build build-wfctl build-iac-codemod migrate-providers # Common benchmark flags BENCH_FLAGS = -bench=. -benchmem -run=^$$ -timeout=30m @@ -83,8 +83,51 @@ run-admin: build ko-build: KO_DOCKER_REPO=ko.local ko build ./cmd/server --bare --platform=linux/$(shell go env GOARCH) +# Build the iac-codemod CLI (W-8 / cmd/iac-codemod). GOWORK=off keeps +# the build self-contained: contributors with a workspace go.work file +# that doesn't include this module shouldn't have to amend their +# environment to run `make migrate-providers`. +build-iac-codemod: + GOWORK=off go build -o iac-codemod ./cmd/iac-codemod + +# Workspace-wide IaC migration runner (W-8 / T8.6). +# +# Runs `iac-codemod lint -dry-run` against the AWS, GCP, and Azure plugin +# repos as advisory-only checks. The plugins themselves stay un-migrated +# at v1 (per plan §W-8: "AWS/GCP/Azure plugins are run advisory-only (no +# `-fix`); their reports are filed as GitHub issues against the +# respective plugin repos for activation-time triage"). For DO, run the +# refactor-* modes manually with `-fix` against the workspace's DO +# checkout — that migration is the subject of P-DO and is intentionally +# excluded from this target's mechanical sweep. +# +# Provider paths are sibling-repo defaults; override on the command line: +# +# make migrate-providers AWS=/path/to/workflow-plugin-aws \ +# GCP=/path/to/workflow-plugin-gcp \ +# AZURE=/path/to/workflow-plugin-azure +AWS ?= ../workflow-plugin-aws +GCP ?= ../workflow-plugin-gcp +AZURE ?= ../workflow-plugin-azure + +migrate-providers: build-iac-codemod + @# iac-codemod lint exit-code semantics (review round-5 finding #7): + @# 0 = clean / 1 = advisory findings (continue) / 2 = parse errors (fail). + @# Naive `|| true` would swallow real execution failures alongside the + @# expected advisory findings; gate on exit code 1 specifically so a + @# parse-error or unknown-flag (>=2) still fails the target. + @echo "==> Running iac-codemod lint (advisory) against AWS plugin: $(AWS)" + @if [ -d "$(AWS)" ]; then ./iac-codemod lint -dry-run "$(AWS)"; ec=$$?; if [ $$ec -ne 0 ] && [ $$ec -ne 1 ]; then echo " iac-codemod lint failed (exit=$$ec)"; exit $$ec; fi; else echo " (skipping: $(AWS) not found)"; fi + @echo "==> Running iac-codemod lint (advisory) against GCP plugin: $(GCP)" + @if [ -d "$(GCP)" ]; then ./iac-codemod lint -dry-run "$(GCP)"; ec=$$?; if [ $$ec -ne 0 ] && [ $$ec -ne 1 ]; then echo " iac-codemod lint failed (exit=$$ec)"; exit $$ec; fi; else echo " (skipping: $(GCP) not found)"; fi + @echo "==> Running iac-codemod lint (advisory) against Azure plugin: $(AZURE)" + @if [ -d "$(AZURE)" ]; then ./iac-codemod lint -dry-run "$(AZURE)"; ec=$$?; if [ $$ec -ne 0 ] && [ $$ec -ne 1 ]; then echo " iac-codemod lint failed (exit=$$ec)"; exit $$ec; fi; else echo " (skipping: $(AZURE) not found)"; fi + @echo "==> migrate-providers complete (advisory-only; no files mutated)" + # Clean build artifacts clean: rm -f server + rm -f wfctl + rm -f iac-codemod rm -f example/workflow-example rm -rf module/ui_dist/assets module/ui_dist/index.html module/ui_dist/vite.svg diff --git a/cmd/iac-codemod/add_validate_plan.go b/cmd/iac-codemod/add_validate_plan.go new file mode 100644 index 00000000..3626d9c5 --- /dev/null +++ b/cmd/iac-codemod/add_validate_plan.go @@ -0,0 +1,980 @@ +// Copyright (c) 2026 Jon Langevin +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "fmt" + "go/ast" + "go/format" + "go/parser" + "go/token" + "io" + "io/fs" + "os" + "path/filepath" + "sort" + "strings" +) + +func init() { + modes["add-validate-plan"] = runAddValidatePlan +} + +// validatePlanClassification labels the disposition of a single +// provider receiver type with respect to the ValidatePlan stub +// injection. Drives both the report grouping and the mutation gate. +type validatePlanClassification int + +const ( + // validatePlanMissing: provider has Plan + Apply but no + // ValidatePlan; the stub will be injected on -fix. + validatePlanMissing validatePlanClassification = iota + // validatePlanAlreadyImplemented: provider already has + // ValidatePlan; idempotent no-op. + validatePlanAlreadyImplemented + // validatePlanSkipped: marker on the type decl or on Plan/Apply. + validatePlanSkipped +) + +func (c validatePlanClassification) String() string { + switch c { + case validatePlanMissing: + return "missing-validate-plan" + case validatePlanAlreadyImplemented: + return "already-implemented" + case validatePlanSkipped: + return "skipped" + default: + return "unknown" + } +} + +// validatePlanSite captures one provider-type site in the report. +type validatePlanSite struct { + Path string + Line int + Receiver string + Class validatePlanClassification + Inserted bool // set when -fix actually injected a stub +} + +type validatePlanReport struct { + sites []validatePlanSite + errors []string +} + +// runAddValidatePlan is the entry point for the add-validate-plan +// subcommand. It walks the supplied paths, classifies each provider +// receiver, and (under -fix) injects a no-op ValidatePlan stub on +// missing sites. +func runAddValidatePlan(args []string, opts *Options, stdout, stderr io.Writer) int { + if len(args) == 0 { + fmt.Fprintln(stderr, "iac-codemod add-validate-plan: at least one path is required") + usage(stderr) + return 2 + } + report := &validatePlanReport{} + for _, path := range args { + if err := addValidatePlanPath(path, opts, report); err != nil { + fmt.Fprintf(stderr, "iac-codemod add-validate-plan: %s: %v\n", path, err) + return 1 + } + } + report.print(stdout, opts) + if len(report.errors) > 0 { + return 1 + } + return 0 +} + +func addValidatePlanPath(path string, opts *Options, report *validatePlanReport) error { + info, err := stat(path) + if err != nil { + return err + } + if !info.IsDir() { + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return fmt.Errorf("not a Go source file (or is a _test.go): %s", path) + } + if err := addValidatePlanFile(path, opts, report); err != nil { + report.errors = append(report.errors, fmt.Sprintf("%s: %v", path, err)) + } + return nil + } + return filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + base := d.Name() + if shouldSkipDir(base) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { + return nil + } + if err := addValidatePlanFile(p, opts, report); err != nil { + report.errors = append(report.errors, fmt.Sprintf("%s: %v", p, err)) + } + return nil + }) +} + +// addValidatePlanFile parses `path`, identifies provider-shaped +// receiver types, and (under -fix) appends a no-op ValidatePlan stub +// for each provider missing one. The stub uses an unqualified +// `*IaCPlan` and `[]PlanDiagnostic` so the substituted code compiles +// against whichever package alias the rest of the file uses. +// +// Insertion strategy: rather than synthesising the FuncDecl via +// AST nodes (which is brittle when the package's IaCPlan type is +// imported under an alias), we append the stub as raw text after +// printing the file. This keeps the rest of the file byte-identical +// for files that only need a stub appended, and avoids any risk of +// printer-induced reformatting elsewhere in the source. +func addValidatePlanFile(path string, opts *Options, report *validatePlanReport) error { + src, err := readFile(path) + if err != nil { + return err + } + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, src, parser.ParseComments) + if err != nil { + return err + } + + // Round-12 #4: skip files in a non-dominant package (same + // rationale as refactor-plan #2 / refactor-apply #3). + dominant := dominantPackageForDir(filepath.Dir(path)) + if dominant != "" && file.Name.Name != dominant { + return nil + } + provs, methodsByRecv, typeDecls := providerReceiversWithMethods(file) + // Widen `provs` AND `methodsByRecv` to the directory-wide method + // set so all per-receiver decisions (skip-marker check, + // hasValidatePlanMethod, receiver-kind inference) consult ALL + // methods of the type, not only the ones declared in this file. + // Review round-2 finding #9: rev1 only widened `provs`, leaving + // methodsByRecv file-local. A provider whose ValidatePlan was + // already implemented in a sibling file would still receive a + // duplicate stub here. Now methodsByRecv carries the package-wide + // view; stub injection still only fires when typeDecls[recv] is + // non-nil so we never APPEND to a sibling file. + if dirProvs, dirMethods := planLikeProviderMethodsInDir(filepath.Dir(path)); dirProvs != nil { + for recv := range dirProvs { + if _, ok := provs[recv]; !ok && typeDecls[recv] != nil { + provs[recv] = typeDecls[recv] + } + } + // Merge sibling methods into methodsByRecv. Per-recv slice is + // append-merged so any sibling ValidatePlan declaration is + // visible to hasValidatePlanMethod, and any sibling Plan/Apply + // is visible to providerReceiverConvention. + // + // Round-7 #10: rev3 deduped by method NAME only ("avoid + // double-counting" was the rationale, since the directory + // re-parser produces fresh *ast.FuncDecl values for the local + // file too). But name-dedupe drops a sibling-correct + // ValidatePlan when the local file has a wrong-signature + // shadow, leading to a duplicate stub injection. The fix: + // dedupe by (name, file-path) using fset.Position. A method + // from a sibling file always has a different file path than + // methods from `file`, so adding it never duplicates. + for recv, sibMethods := range dirMethods { + if _, ok := provs[recv]; !ok { + continue + } + for _, m := range sibMethods { + // Position uses the *separate* FileSet from + // planLikeProviderMethodsInDir. We can't compare + // directly to the primary fset's positions. The + // safest signal: is the FuncDecl's own *ast.FuncDecl + // pointer present in methodsByRecv[recv] (the local + // methods)? Pointer comparison handles the dedupe + // without name shadowing. + present := false + for _, lm := range methodsByRecv[recv] { + if lm == m { + present = true + break + } + } + if present { + continue + } + // Distinct *ast.FuncDecl: name+signature dedupe so a + // sibling Plan/Apply with identical signature to a + // local one (re-parsed) doesn't duplicate. ValidatePlan + // is INTENTIONALLY not deduped by name alone; if the + // local has wrong-signature ValidatePlan and sibling + // has correct, both are added so hasValidatePlanMethod + // can find the correct one (it ignores wrong shapes). + if isLocalDuplicate(m, methodsByRecv[recv]) { + continue + } + methodsByRecv[recv] = append(methodsByRecv[recv], m) + } + } + } + // Determine the qualifier for *IaCPlan / []PlanDiagnostic so the + // stub's signature matches whatever import-naming convention the + // file already uses (review round-1 finding #7). + // + // Round-4 #1: when the type declaration lives in a sibling file + // (no interfaces import in THIS file), fall back to the qualifier + // the package uses AND inject the import. + // + // Round-7 #4: rev3 fell back to "interfaces" if ANY sibling imports + // interfaces. That's wrong if the provider itself uses LOCAL + // IaCPlan types (e.g., a unit-test fixture in package `p` with + // local types, where an unrelated sibling imports interfaces for + // other reasons). The correct signal is per-receiver: inspect THIS + // PROVIDER's existing Plan/Apply parameter types (now visible via + // the directory-wide methodsByRecv merge from round-3 #1) to see + // what qualifier they use. Only fall back if the provider's own + // methods reference the qualified shape. + qualifier := interfacesQualifier(file) + needsInterfacesImport := false + // Captures the per-receiver qualifier set in the loop below, so the + // post-loop import injection (round-9 #4) can match the alias name + // the stub will reference. + injectedQualifier := "" + // Deterministic order for the report and for mutation: sort by + // declaration line. Round-7 finding #7 + #8: provs[recv] can be + // nil when the type declaration lives in a sibling file (round-3's + // directory-wide method-set scan supports this layout). Calling + // .Pos() on a nil *ast.TypeSpec panics. Default position to NoPos + // for nil specs; sort still works (NoPos sorts equal-to-zero). + type recvOrder struct { + Name string + Pos token.Pos + } + var ordered []recvOrder + for recv := range provs { + var pos token.Pos + if ts := provs[recv]; ts != nil { + pos = ts.Pos() + } + ordered = append(ordered, recvOrder{Name: recv, Pos: pos}) + } + sort.Slice(ordered, func(i, j int) bool { + if ordered[i].Pos != ordered[j].Pos { + return ordered[i].Pos < ordered[j].Pos + } + return ordered[i].Name < ordered[j].Name + }) + + // Directory-wide type-doc lookup so a skip-marker on a sibling + // file's type declaration is honored (round-7 #5). + siblingTypeDocs := receiverTypeDocsInDir(filepath.Dir(path), file) + + mutated := false + var pendingStubs []string + for _, rec := range ordered { + recv := rec.Name + methods := methodsByRecv[recv] + // Skip-marker check: the type decl (in this file OR a sibling + // file via the directory-wide doc lookup) OR any of the + // existing Plan/Apply methods (across files) carrying the + // marker suppresses the classification. + // + // Round-7 #5: rev3 only consulted typeDecls (this file's TypeSpec). + // When Plan/Apply are here but the provider type with + // `// wfctl:skip-iac-codemod` lives in a SIBLING file, the + // skip got ignored. siblingTypeDocs now provides the + // directory-wide view (matching the round-6 fix in refactor-*). + ts := typeDecls[recv] + skipped := false + if ts != nil && hasSkipMarkerOn(ts.Doc) { + skipped = true + } + if !skipped { + if doc, ok := siblingTypeDocs[recv]; ok && doc.carriesMarker() { + skipped = true + } + } + if !skipped { + // Round-8 #2: rev2 checked the marker on EVERY method, so + // a marker on Destroy/Status/etc. accidentally suppressed + // add-validate-plan for the whole provider. Restrict to + // Plan and Apply (the provider-defining methods that + // actually opt the type out of the migration). + for _, m := range methods { + if m.Name.Name != "Plan" && m.Name.Name != "Apply" { + continue + } + if hasSkipMarkerOn(m.Doc) { + skipped = true + break + } + } + } + // Also honor the parent GenDecl's doc for a `type Foo struct{}` + // declared in a single-spec block (current file only — + // receiverTypeDocsInDir's GenDeclDoc already covers siblings). + if !skipped { + if gd := genDeclFor(file, ts); gd != nil && hasSkipMarkerOn(gd.Doc) { + skipped = true + } + } + + var class validatePlanClassification + switch { + case skipped: + class = validatePlanSkipped + case hasValidatePlanMethod(methods): + class = validatePlanAlreadyImplemented + default: + // Round-11 #3 reverts round-10 #2's broad-suppress: ANY + // embedded field would suppress the missing diagnostic, + // but `sync.Mutex`, loggers, config mixins, etc. don't + // promote a `ValidatePlan` method, so real migration + // targets were silently missed. Without full type info we + // can't resolve promotion, so report missing + // unconditionally; maintainers whose providers actually + // satisfy ProviderValidator via promotion can suppress + // with the explicit `// wfctl:skip-iac-codemod` marker. + class = validatePlanMissing + } + + line := 0 + if ts != nil { + line = fset.Position(ts.Pos()).Line + } else if len(methods) > 0 { + line = fset.Position(methods[0].Pos()).Line + } + site := validatePlanSite{ + Path: path, + Line: line, + Receiver: recv, + Class: class, + } + 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; + // the sibling's own pass will inject the stub. Without + // this guard, both files write a `ValidatePlan` stub for + // the same receiver, producing duplicate method + // declarations in the package. + if ts == nil { + report.sites = append(report.sites, site) + continue + } + pointerRecv := providerReceiverConvention(methods) + // Per-receiver qualifier resolution. If THIS file has its + // own interfaces import, qualifier already reflects that + // (set above). Otherwise inspect this provider's existing + // Plan/Apply parameter types for the qualifier they use — + // round-7 #4: an unrelated sibling importing interfaces is + // not a reliable signal that THIS provider uses qualified + // types. + recvQualifier := qualifier + if recvQualifier == "" { + recvQualifier = qualifierFromProviderMethods(methods) + if recvQualifier != "" { + needsInterfacesImport = true + // Round-9 #4: capture for post-loop import-alias + // matching. If multiple receivers in the same file + // derive different aliases, the LAST one wins — + // rare in practice (a single file usually has one + // interfaces alias). + injectedQualifier = recvQualifier + } + } + pendingStubs = append(pendingStubs, validatePlanStubText(recv, recvQualifier, pointerRecv)) + site.Inserted = true + mutated = true + } + report.sites = append(report.sites, site) + } + + if mutated && opts != nil && opts.Fix { + baseSrc := src + // Round-4 finding #1: when the stub uses a qualified type but + // the file doesn't import interfaces, add the import via AST + // printing first so the qualified type resolves. + // + // Round-9 finding #4: if the qualifier we derived from a + // sibling method's signature is NOT "interfaces" (e.g. the + // sibling uses an alias like `iface "github.com/.../interfaces"`), + // the injected import must also use that alias so the stub's + // `iface.IaCPlan` resolves to the imported package. + if needsInterfacesImport { + injectedAlias := "" + if injectedQualifier != "" && injectedQualifier != "interfaces" { + injectedAlias = injectedQualifier + } + ensureImportAs(file, "github.com/GoCodeAlone/workflow/interfaces", injectedAlias) + var buf bytes.Buffer + if err := format.Node(&buf, fset, file); err != nil { + return fmt.Errorf("format %s: %w", path, err) + } + baseSrc = buf.Bytes() + } + // Append stubs as raw text. baseSrc is either the unmodified + // original (no interfaces import needed) or the AST-reprinted + // form with the interfaces import injected. + appended := append([]byte{}, baseSrc...) + if len(appended) == 0 || appended[len(appended)-1] != '\n' { + appended = append(appended, '\n') + } + for _, stub := range pendingStubs { + appended = append(appended, '\n') + appended = append(appended, stub...) + if !strings.HasSuffix(stub, "\n") { + appended = append(appended, '\n') + } + } + if err := writeFileAtomicBytes(path, appended); err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + } + return nil +} + +// validatePlanStubText returns the source text for a no-op ValidatePlan +// stub on the named receiver type. `qualifier` is the package alias +// the source file uses for github.com/GoCodeAlone/workflow/interfaces +// (typically "interfaces", or "" if the file is itself in that package +// and uses unqualified names). `pointerReceiver` controls whether the +// stub uses `*T` or `T` as its receiver — set to match the existing +// receiver convention of the type's other methods. +// +// Review history: +// - rev0 (round 0): always emitted unqualified `*IaCPlan` / +// `[]PlanDiagnostic`, breaking compile in files importing +// interfaces. Fixed in round-1 (qualifier param added). +// - rev1 (round 2 finding #5): always used `(p *T)` even when the +// type's existing methods used value receivers. Method-set +// mismatch left the type failing the ProviderValidator type +// assertion. Fixed by threading pointerReceiver through the +// caller, which inspects the type's existing Plan/Apply +// receivers. +func validatePlanStubText(recv, qualifier string, pointerReceiver bool) string { + planType := "*IaCPlan" + diagsType := "[]PlanDiagnostic" + if qualifier != "" { + planType = "*" + qualifier + ".IaCPlan" + diagsType = "[]" + qualifier + ".PlanDiagnostic" + } + receiver := recv + if pointerReceiver { + receiver = "*" + recv + } + return fmt.Sprintf(`// ValidatePlan reports diagnostics for any plan-time concerns. The +// stub generated by iac-codemod returns no diagnostics; replace with +// real provider-specific checks (region constraints, quota limits, +// resource-type conflicts, etc.) before relying on it. +func (p %s) ValidatePlan(plan %s) %s { + return nil +} +`, receiver, planType, diagsType) +} + +// receiverIsPointer returns true if the receiver of fn is `*T` (i.e. +// a pointer receiver). Helps determine the convention to use when +// inserting a new ValidatePlan stub on the same type so the method-set +// matches the existing Plan/Apply (review round-2 #5). +func receiverIsPointer(fn *ast.FuncDecl) bool { + if fn == nil || fn.Recv == nil || len(fn.Recv.List) == 0 { + return false + } + _, ok := fn.Recv.List[0].Type.(*ast.StarExpr) + return ok +} + +// providerReceiverConvention reports whether the receiver type's +// Plan/Apply methods use a pointer receiver. The convention used by +// the existing Plan method takes precedence; if Plan is missing the +// Apply convention is used. Defaults to true (pointer receiver) when +// no Plan/Apply pair exists, matching the dominant Go style. +func providerReceiverConvention(methods []*ast.FuncDecl) bool { + for _, m := range methods { + if m.Name.Name == "Plan" { + return receiverIsPointer(m) + } + } + for _, m := range methods { + if m.Name.Name == "Apply" { + return receiverIsPointer(m) + } + } + return true +} + +// isLocalDuplicate returns true if `m` appears to be a re-parse of a +// FuncDecl already in `existing`. Round-8 #1: arity-only dedupe (rev2) +// still mistreated a correct ValidatePlan(plan *IaCPlan) +// []PlanDiagnostic as a duplicate of a wrong-signature +// ValidatePlan(name string) []PlanDiagnostic — same arity, different +// types. Now compares parameter and return TYPES via a structural +// fingerprint (typeFingerprint) so signatures with matching names but +// different types are correctly distinguished. +func isLocalDuplicate(m *ast.FuncDecl, existing []*ast.FuncDecl) bool { + mSig := signatureFingerprint(m.Type) + for _, lm := range existing { + if lm == m { + continue + } + if lm.Name.Name != m.Name.Name { + continue + } + if signatureFingerprint(lm.Type) == mSig { + return true + } + } + return false +} + +// signatureFingerprint returns a string fingerprint of a FuncType +// that's stable across distinct *ast.FuncDecl values (as produced by +// re-parsing the same file in planLikeProviderMethodsInDir). The +// fingerprint includes BOTH parameter and return type strings so +// same-name same-arity DIFFERENT-type methods (the wrong-signature +// shadow scenario) get distinct fingerprints (round-8 #1). +func signatureFingerprint(ft *ast.FuncType) string { + if ft == nil { + return "" + } + var b strings.Builder + b.WriteString("(") + if ft.Params != nil { + for i, p := range ft.Params.List { + if i > 0 { + b.WriteString(",") + } + b.WriteString(typeFingerprint(p.Type)) + } + } + b.WriteString(")") + if ft.Results != nil { + b.WriteString("(") + for i, r := range ft.Results.List { + if i > 0 { + b.WriteString(",") + } + b.WriteString(typeFingerprint(r.Type)) + } + b.WriteString(")") + } + return b.String() +} + +// typeFingerprint returns a structural string for an ast.Expr type. +// Conservative: covers the type shapes used by IaC provider methods +// (Ident, SelectorExpr, StarExpr, ArrayType, MapType, InterfaceType, +// FuncType, Ellipsis). Anything else returns "?", which still +// participates in fingerprint comparison correctly. +func typeFingerprint(expr ast.Expr) string { + switch e := expr.(type) { + case *ast.Ident: + return e.Name + case *ast.SelectorExpr: + return typeFingerprint(e.X) + "." + e.Sel.Name + case *ast.StarExpr: + return "*" + typeFingerprint(e.X) + case *ast.ArrayType: + return "[]" + typeFingerprint(e.Elt) + case *ast.MapType: + return "map[" + typeFingerprint(e.Key) + "]" + typeFingerprint(e.Value) + case *ast.InterfaceType: + return "interface{}" + case *ast.Ellipsis: + return "..." + typeFingerprint(e.Elt) + case *ast.FuncType: + return "func" + signatureFingerprint(e) + } + return "?" +} + +// qualifierFromProviderMethods inspects the parameter types of the +// supplied methods (the receiver's directory-wide method set per +// round-3 #1) and returns the qualifier used for the IaCPlan type if +// any method's signature references it qualified (e.g. *interfaces.IaCPlan). +// Returns "" if no method's signature uses a qualified IaCPlan. +// +// Round-7 #4: rev3 of add_validate_plan fell back to qualifier="interfaces" +// based on whether ANY sibling file in the directory imported +// interfaces. That signal is unreliable: if the provider itself uses +// LOCAL IaCPlan types (test fixtures, etc.) but an unrelated sibling +// imports interfaces for some other reason, the stub got a wrongly- +// qualified signature and broke compilation. Per-receiver inspection +// of the actual signatures the provider already uses is the +// trustworthy signal. +func qualifierFromProviderMethods(methods []*ast.FuncDecl) string { + for _, m := range methods { + switch m.Name.Name { + case "Plan", "Apply": + // continue + default: + continue + } + if m.Type == nil || m.Type.Params == nil { + continue + } + for _, p := range m.Type.Params.List { + // Look for *.IaCPlan or *IaCPlan. + star, ok := p.Type.(*ast.StarExpr) + if !ok { + // Slice form `[].ResourceSpec` etc. also + // indicates qualified usage; check. + if arr, ok := p.Type.(*ast.ArrayType); ok && arr.Len == nil { + if sel, ok := arr.Elt.(*ast.SelectorExpr); ok { + if id, ok := sel.X.(*ast.Ident); ok { + return id.Name + } + } + } + continue + } + if sel, ok := star.X.(*ast.SelectorExpr); ok && sel.Sel.Name == "IaCPlan" { + if id, ok := sel.X.(*ast.Ident); ok { + return id.Name + } + } + } + } + return "" +} + +// siblingUsesInterfacesImport returns true if any non-test .go file +// in dir (other than excludePath) imports +// github.com/GoCodeAlone/workflow/interfaces. Used to decide whether +// to inject an interfaces import into a file that doesn't have one +// when emitting a qualified ValidatePlan stub (review round-4 #1). +func siblingUsesInterfacesImport(dir, excludePath string) bool { + const wantPath = "github.com/GoCodeAlone/workflow/interfaces" + entries, err := os.ReadDir(dir) + if err != nil { + return false + } + for _, e := range entries { + if e.IsDir() { + continue + } + name := e.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + fpath := filepath.Join(dir, name) + if fpath == excludePath { + continue + } + src, err := readFile(fpath) + if err != nil { + continue + } + fs := token.NewFileSet() + sib, err := parser.ParseFile(fs, fpath, src, parser.ImportsOnly) + if err != nil { + continue + } + for _, imp := range sib.Imports { + if imp.Path == nil { + continue + } + if strings.Trim(imp.Path.Value, `"`) == wantPath { + return true + } + } + } + return false +} + +// interfacesQualifier returns the package alias `file` uses for +// github.com/GoCodeAlone/workflow/interfaces. If the import is +// renamed (`alias "github.com/.../interfaces"`), the alias name is +// returned. If the file does not import interfaces at all, returns +// "" (the rare case of a file declaring providers entirely in +// terms of locally-defined types, e.g. unit-test fixtures). +func interfacesQualifier(file *ast.File) string { + const wantPath = "github.com/GoCodeAlone/workflow/interfaces" + for _, imp := range file.Imports { + if imp.Path == nil { + continue + } + v := strings.Trim(imp.Path.Value, `"`) + if v != wantPath { + continue + } + if imp.Name != nil { + if imp.Name.Name == "_" || imp.Name.Name == "." { + // Blank/dot imports — the latter would let the user + // reference IaCPlan unqualified. We can't safely + // disambiguate so we err on the side of qualifying + // (the file would not compile with a blank import + // of the types anyway). + continue + } + return imp.Name.Name + } + // Default-named import — the package's actual name is + // "interfaces" (verified by reading the package clause). + return "interfaces" + } + return "" +} + +// providerReceiversWithMethods returns three views of the file's +// receiver-type structure: +// - the set of receiver type names whose method set (in this file +// alone) looks like an IaCProvider (has Plan + Apply); +// - methodsByRecv: every method's *ast.FuncDecl indexed by receiver; +// - typeDecls: the *ast.TypeSpec for each struct receiver, used so +// the report can point at the type's declaration line and the +// skip-marker can be looked up on the type doc. +// +// Note: cross-file method sets are not supported in this single-file +// pass. A provider whose Plan and Apply live in different files will +// be missed; the codemod's spec scope is single-file (the four +// per-plugin Apply/Plan files in the workspace today are each +// self-contained). +func providerReceiversWithMethods(file *ast.File) ( + map[string]*ast.TypeSpec, // provs (key = recv name; value = its TypeSpec or nil) + map[string][]*ast.FuncDecl, // methodsByRecv + map[string]*ast.TypeSpec, // typeDecls +) { + methodsByRecv := make(map[string][]*ast.FuncDecl) + typeDecls := make(map[string]*ast.TypeSpec) + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + if d.Recv == nil || len(d.Recv.List) == 0 { + continue + } + recv := receiverTypeName(d) + if recv == "" { + continue + } + methodsByRecv[recv] = append(methodsByRecv[recv], d) + case *ast.GenDecl: + if d.Tok != token.TYPE { + continue + } + for _, spec := range d.Specs { + ts, ok := spec.(*ast.TypeSpec) + if !ok { + continue + } + if _, isStruct := ts.Type.(*ast.StructType); !isStruct { + continue + } + typeDecls[ts.Name.Name] = ts + } + } + } + provs := make(map[string]*ast.TypeSpec) + for recv, methods := range methodsByRecv { + if !looksLikeProvider(methods) { + continue + } + provs[recv] = typeDecls[recv] + } + return provs, methodsByRecv, typeDecls +} + +// hasValidatePlanMethod returns true if the method list contains a +// ValidatePlan method whose signature matches +// `ValidatePlan(*IaCPlan) []PlanDiagnostic` AND whose receiver kind +// matches the dominant receiver kind of the type's existing +// Plan/Apply methods. +// +// Review history: +// - round-1 #8: rev0 only checked the method name; a ValidatePlan +// with the wrong parameter or result type passed silently. Fixed +// by adding validatePlanSignatureMatches. +// - round-5 #3: rev1 ignored receiver kind; a value-receiver +// provider (Plan/Apply on `T`) with a pointer-receiver +// ValidatePlan on `*T` still failed the +// interfaces.ProviderValidator type assertion (method set on `T` +// does not include `*T` methods). hasValidatePlanMethod now +// accepts ValidatePlan only if its receiver kind matches the +// existing convention; otherwise the type is reported as missing. +func hasValidatePlanMethod(methods []*ast.FuncDecl) bool { + // Round-5 #3 added receiver-kind enforcement; round-9 #3 corrects + // the asymmetry: per Go spec, *T's method set includes both + // pointer- and value-receiver methods of T. So: + // + // - value-receiver provider (Plan/Apply on T): ValidatePlan + // MUST also be value-receiver, because T's method set excludes + // pointer methods. + // - pointer-receiver provider (Plan/Apply on *T): ValidatePlan + // can be EITHER value- or pointer-receiver; *T's method set + // includes both. + // + // Only the value-receiver provider case requires strict matching; + // pointer-receiver providers accept either kind. + providerWantsPointer := providerReceiverConvention(methods) + for _, m := range methods { + if m.Name.Name != "ValidatePlan" { + continue + } + if !validatePlanSignatureMatches(m.Type) { + continue + } + if !providerWantsPointer && receiverIsPointer(m) { + // Value-receiver provider can't satisfy ProviderValidator + // via a pointer-receiver ValidatePlan (T's method set + // excludes *T methods). + continue + } + return true + } + return false +} + +// validatePlanSignatureMatches returns true if ft has the canonical +// `func(*IaCPlan) []PlanDiagnostic` signature shape (qualified or +// unqualified). See hasValidatePlanMethod for the rationale. +func validatePlanSignatureMatches(ft *ast.FuncType) bool { + if ft == nil { + return false + } + if ft.Params == nil || len(ft.Params.List) != 1 { + return false + } + if ft.Results == nil || len(ft.Results.List) != 1 { + return false + } + // Param must be a pointer to a type whose name ends in "IaCPlan". + star, ok := ft.Params.List[0].Type.(*ast.StarExpr) + if !ok { + return false + } + if !typeNameTailMatches(star.X, "IaCPlan") { + return false + } + // Result must be a slice whose element name ends in "PlanDiagnostic". + arr, ok := ft.Results.List[0].Type.(*ast.ArrayType) + if !ok { + return false + } + if arr.Len != nil { + // Fixed-size array (e.g. [3]PlanDiagnostic) is not a slice. + return false + } + return typeNameTailMatches(arr.Elt, "PlanDiagnostic") +} + +// typeNameTailMatches returns true if expr is `.` or just +// `` (i.e. matches an unqualified or any-qualifier-qualified +// type name). +func typeNameTailMatches(expr ast.Expr, want string) bool { + switch e := expr.(type) { + case *ast.Ident: + return e.Name == want + case *ast.SelectorExpr: + return e.Sel.Name == want + } + return false +} + +// (typeHasEmbeddedFields was added in round-10 #2/#3 to suppress the +// missing-ValidatePlan diagnostic on providers with ANY embedded +// field, on the assumption embedding might promote ValidatePlan. +// Round-11 #3/#4 reverted that broad suppression because most +// embeddings — sync.Mutex, loggers, config mixins — don't promote +// ValidatePlan, so real targets were silently missed. The function +// is removed; maintainers whose providers ACTUALLY satisfy +// ProviderValidator via promotion suppress with the explicit +// `// wfctl:skip-iac-codemod` marker.) + +// genDeclFor returns the *ast.GenDecl wrapper for the given TypeSpec, +// which is where a doc comment placed before the `type` keyword +// (rather than between `type` and the type name) lives. AST attaches +// such comments to the GenDecl rather than the inner TypeSpec. +func genDeclFor(file *ast.File, ts *ast.TypeSpec) *ast.GenDecl { + if ts == nil { + return nil + } + for _, decl := range file.Decls { + gd, ok := decl.(*ast.GenDecl) + if !ok || gd.Tok != token.TYPE { + continue + } + for _, spec := range gd.Specs { + if spec == ts { + return gd + } + } + } + return nil +} + +// writeFileAtomicBytes is the bytes-input twin of writeFileAtomic. +// Round-11 #5: rev1 left the temp file at os.CreateTemp's default +// 0600 mode, so the rename clobbered the source's original +// permissions. Now delegates to writeFileAtomicBytesPreserveMode +// (defined in refactor_plan.go) which captures the original mode +// and chmods the temp file before rename. +func writeFileAtomicBytes(path string, data []byte) error { + return writeFileAtomicBytesPreserveMode(path, data) +} + +// ============================================================ +// Report rendering +// ============================================================ + +func (r *validatePlanReport) print(w io.Writer, opts *Options) { + sort.Slice(r.sites, func(i, j int) bool { + if r.sites[i].Path != r.sites[j].Path { + return r.sites[i].Path < r.sites[j].Path + } + return r.sites[i].Line < r.sites[j].Line + }) + fmt.Fprintln(w, "# iac-codemod add-validate-plan report") + fmt.Fprintln(w) + mode := "dry-run" + if opts != nil && opts.Fix { + mode = "fix" + } + fmt.Fprintf(w, "Mode: %s\n", mode) + fmt.Fprintf(w, "Sites: %d\n", len(r.sites)) + fmt.Fprintf(w, "Errors: %d\n", len(r.errors)) + fmt.Fprintln(w) + + groups := map[validatePlanClassification][]validatePlanSite{} + for _, s := range r.sites { + groups[s.Class] = append(groups[s.Class], s) + } + order := []validatePlanClassification{ + validatePlanMissing, + validatePlanAlreadyImplemented, + validatePlanSkipped, + } + headers := map[validatePlanClassification]string{ + validatePlanMissing: "Missing ValidatePlan (stub injection candidate)", + validatePlanAlreadyImplemented: "Already-implemented (no-op)", + validatePlanSkipped: "Skipped (// wfctl:skip-iac-codemod)", + } + for _, c := range order { + sites := groups[c] + if len(sites) == 0 { + continue + } + fmt.Fprintf(w, "## %s\n\n", headers[c]) + for _, s := range sites { + suffix := "" + if c == validatePlanMissing && s.Inserted { + suffix = " (stub inserted)" + } + fmt.Fprintf(w, "- %s:%d %s %s%s\n", s.Path, s.Line, s.Receiver, s.Class, suffix) + } + fmt.Fprintln(w) + } + + if len(r.errors) > 0 { + fmt.Fprintln(w, "## Errors") + fmt.Fprintln(w) + for _, e := range r.errors { + fmt.Fprintf(w, "- %s\n", e) + } + fmt.Fprintln(w) + } +} diff --git a/cmd/iac-codemod/add_validate_plan_test.go b/cmd/iac-codemod/add_validate_plan_test.go new file mode 100644 index 00000000..309be7ac --- /dev/null +++ b/cmd/iac-codemod/add_validate_plan_test.go @@ -0,0 +1,391 @@ +// Copyright (c) 2026 Jon Langevin +// SPDX-License-Identifier: Apache-2.0 + +// Tests in this file MUST NOT call t.Parallel(). Same global-state +// constraint as main_test.go / lint_test.go / refactor_*_test.go. + +package main + +import ( + "bytes" + "os" + "strings" + "testing" + "time" +) + +// ============================================================ +// Source fixtures +// ============================================================ + +// avpProviderMissingValidatePlanSrc is a provider with both Plan and Apply +// but no ValidatePlan method. The codemod must insert a no-op stub. +const avpProviderMissingValidatePlanSrc = `package p + +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type FooProvider struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return &IaCPlan{}, nil +} + +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return &ApplyResult{}, nil +} +` + +// avpProviderWithValidatePlanSrc is the no-op idempotent case: ValidatePlan +// already exists; the codemod must NOT add another stub. +const avpProviderWithValidatePlanSrc = `package p + +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type FooProvider struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return &IaCPlan{}, nil +} + +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return &ApplyResult{}, nil +} + +func (p *FooProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +// avpProviderSkippedValidatePlanSrc carries the marker on the type decl — +// the codemod must NOT inject ValidatePlan and must list the site as +// skipped. (Plan rev2 line 2400: marker honored at type-doc level.) +const avpProviderSkippedValidatePlanSrc = `package p + +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type PlanDiagnostic struct{} + +// FooProvider is intentionally without ValidatePlan; the constraint +// surface lives in a sibling type. +// +// wfctl:skip-iac-codemod sibling-validator pattern, see ADR-042 +type FooProvider struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return &IaCPlan{}, nil +} + +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return &ApplyResult{}, nil +} +` + +// avpNonProviderSrc — has methods named Plan/Apply but on a non-provider +// type (insufficient signature shape). Must NOT be touched. +const avpNonProviderSrc = `package p + +import "context" + +type Settings struct{} + +func (s Settings) Plan(x int) error { return nil } +func (s Settings) Apply(y int) error { return nil } +` + +// ============================================================ +// Detection (dry-run) +// ============================================================ + +func TestAddValidatePlan_DryRun_DetectsMissing(t *testing.T) { + path := writeFixture(t, "provider.go", avpProviderMissingValidatePlanSrc) + var stdout, stderr bytes.Buffer + code := runAddValidatePlan([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "FooProvider") { + t.Errorf("report should name FooProvider; got:\n%s", out) + } + if !strings.Contains(out, "missing-validate-plan") { + t.Errorf("report should classify as missing-validate-plan; got:\n%s", out) + } + got, _ := os.ReadFile(path) + if string(got) != avpProviderMissingValidatePlanSrc { + t.Errorf("dry-run modified the file; expected no mutation") + } +} + +func TestAddValidatePlan_DryRun_AlreadyImplemented(t *testing.T) { + path := writeFixture(t, "provider.go", avpProviderWithValidatePlanSrc) + var stdout, stderr bytes.Buffer + code := runAddValidatePlan([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "already-implemented") { + t.Errorf("report should classify provider as already-implemented; got:\n%s", out) + } +} + +func TestAddValidatePlan_DryRun_HonorsSkipMarker(t *testing.T) { + path := writeFixture(t, "provider.go", avpProviderSkippedValidatePlanSrc) + var stdout, stderr bytes.Buffer + code := runAddValidatePlan([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "Skipped") { + t.Errorf("report should have a Skipped section; got:\n%s", out) + } + if !strings.Contains(out, "FooProvider") { + t.Errorf("Skipped section should list FooProvider; got:\n%s", out) + } +} + +func TestAddValidatePlan_DryRun_IgnoresNonProviders(t *testing.T) { + path := writeFixture(t, "settings.go", avpNonProviderSrc) + var stdout, stderr bytes.Buffer + code := runAddValidatePlan([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if strings.Contains(out, "Settings") { + t.Errorf("non-provider type Settings should NOT be reported; got:\n%s", out) + } +} + +// ============================================================ +// Mutation (-fix) +// ============================================================ + +func TestAddValidatePlan_Fix_InsertsStub(t *testing.T) { + path := writeFixture(t, "provider.go", avpProviderMissingValidatePlanSrc) + var stdout, stderr bytes.Buffer + code := runAddValidatePlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + gotStr := string(got) + if !strings.Contains(gotStr, "ValidatePlan(plan *IaCPlan) []PlanDiagnostic") { + t.Errorf("inserted stub must be `ValidatePlan(plan *IaCPlan) []PlanDiagnostic`; got:\n%s", gotStr) + } + // Stub returns nil (no-op). + if !strings.Contains(gotStr, "return nil") { + t.Errorf("inserted stub must return nil; got:\n%s", gotStr) + } +} + +func TestAddValidatePlan_Fix_IdempotentOnImplemented(t *testing.T) { + path := writeFixture(t, "provider.go", avpProviderWithValidatePlanSrc) + var stdout, stderr bytes.Buffer + if code := runAddValidatePlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr); code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != avpProviderWithValidatePlanSrc { + t.Errorf("provider with ValidatePlan must be byte-identical after fix (idempotent); got:\n%s", string(got)) + } +} + +func TestAddValidatePlan_Fix_HonorsSkipMarker(t *testing.T) { + path := writeFixture(t, "provider.go", avpProviderSkippedValidatePlanSrc) + var stdout, stderr bytes.Buffer + code := runAddValidatePlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != avpProviderSkippedValidatePlanSrc { + t.Errorf("skip-marker'd provider must NOT receive ValidatePlan stub; file changed:\n%s", string(got)) + } +} + +func TestAddValidatePlan_Fix_DoesNotTouchNonProvider(t *testing.T) { + path := writeFixture(t, "settings.go", avpNonProviderSrc) + var stdout, stderr bytes.Buffer + if code := runAddValidatePlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr); code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != avpNonProviderSrc { + t.Errorf("non-provider file must NOT be modified") + } +} + +// ============================================================ +// Review round-1 regression tests +// ============================================================ + +// avpProviderInterfacesQualifierSrc — review round-1 finding #7. A +// provider whose package imports interfaces and references the +// canonical types as `*interfaces.IaCPlan` etc. must receive a stub +// whose signature uses the same qualifier. rev0 always emitted +// unqualified types and broke compilation. +const avpProviderInterfacesQualifierSrc = `package p + +import ( + "context" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +type FooProvider struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []interfaces.ResourceSpec, current []interfaces.ResourceState) (*interfaces.IaCPlan, error) { + return &interfaces.IaCPlan{}, nil +} + +func (p *FooProvider) Apply(ctx context.Context, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + return &interfaces.ApplyResult{}, nil +} +` + +func TestAddValidatePlan_Fix_QualifiedSignature(t *testing.T) { + path := writeFixture(t, "provider.go", avpProviderInterfacesQualifierSrc) + var stdout, stderr bytes.Buffer + if code := runAddValidatePlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr); code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + gotStr := string(got) + if !strings.Contains(gotStr, "ValidatePlan(plan *interfaces.IaCPlan) []interfaces.PlanDiagnostic") { + t.Errorf("stub must use the same qualifier as the file's existing imports (interfaces.IaCPlan); got:\n%s", gotStr) + } +} + +// avpProviderWrongSignatureSrc — review round-1 finding #8. A provider +// with a `ValidatePlan` method whose signature is wrong (takes a string +// instead of *IaCPlan) must NOT be classified as already-implemented; +// the codemod would then leave the type failing to satisfy +// interfaces.ProviderValidator. +const avpProviderWrongSignatureSrc = `package p + +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type FooProvider struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return &IaCPlan{}, nil +} + +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return &ApplyResult{}, nil +} + +// Wrong signature: parameter is a string, not *IaCPlan. +func (p *FooProvider) ValidatePlan(name string) []PlanDiagnostic { return nil } +` + +func TestAddValidatePlan_DryRun_FlagsWrongSignature(t *testing.T) { + path := writeFixture(t, "provider.go", avpProviderWrongSignatureSrc) + var stdout, stderr bytes.Buffer + code := runAddValidatePlan([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if strings.Contains(out, "already-implemented") { + t.Errorf("ValidatePlan with wrong signature must NOT be classified as already-implemented; got:\n%s", out) + } + if !strings.Contains(out, "missing-validate-plan") { + t.Errorf("ValidatePlan with wrong signature should be classified as missing (signature mismatch); got:\n%s", out) + } +} + +// ============================================================ +// Review round-2 regression tests +// ============================================================ + +// avpProviderValueReceiverSrc — review round-2 finding #5. A provider +// whose existing Plan/Apply use VALUE receivers (`(p FooProvider)`) +// must get a ValidatePlan stub with a value receiver too. rev1 always +// emitted `(p *T)`, mismatching method-sets and breaking the +// ProviderValidator type assertion. +const avpProviderValueReceiverSrc = `package p + +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type ValueProvider struct{} + +func (p ValueProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return &IaCPlan{}, nil +} + +func (p ValueProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return &ApplyResult{}, nil +} +` + +func TestAddValidatePlan_Fix_ValueReceiverConvention(t *testing.T) { + path := writeFixture(t, "provider.go", avpProviderValueReceiverSrc) + var stdout, stderr bytes.Buffer + if code := runAddValidatePlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr); code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + gotStr := string(got) + // Stub MUST use value receiver to match Plan/Apply. + if !strings.Contains(gotStr, "func (p ValueProvider) ValidatePlan(") { + t.Errorf("stub must use value receiver to match Plan/Apply convention; got:\n%s", gotStr) + } + // And NOT pointer receiver. + if strings.Contains(gotStr, "func (p *ValueProvider) ValidatePlan(") { + t.Errorf("stub must NOT use pointer receiver when Plan/Apply use value; got:\n%s", gotStr) + } +} + +// ============================================================ +// Mutation-gate negative test +// ============================================================ + +func TestAddValidatePlan_DryRunFalseWithoutFix_DoesNotMutate(t *testing.T) { + path := writeFixture(t, "provider.go", avpProviderMissingValidatePlanSrc) + stat0, _ := os.Stat(path) + mtime0 := stat0.ModTime() + time.Sleep(10 * time.Millisecond) + + var stdout, stderr bytes.Buffer + code := run([]string{"add-validate-plan", "-dry-run=false", path}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != avpProviderMissingValidatePlanSrc { + t.Errorf("file must NOT be mutated when -dry-run=false alone; content changed") + } + stat1, _ := os.Stat(path) + if !stat1.ModTime().Equal(mtime0) { + t.Errorf("file mtime should be unchanged; before=%v after=%v", mtime0, stat1.ModTime()) + } +} diff --git a/cmd/iac-codemod/lint.go b/cmd/iac-codemod/lint.go new file mode 100644 index 00000000..9ffb6d88 --- /dev/null +++ b/cmd/iac-codemod/lint.go @@ -0,0 +1,1245 @@ +// Copyright (c) 2026 Jon Langevin +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "go/types" + "io" + "io/fs" + "os" + "path/filepath" + "sort" + "strings" + "unicode" + "unicode/utf8" + + "golang.org/x/tools/go/analysis" +) + +// osStat / osReadFile are direct stdlib bindings that the var indirections +// `stat` and `readFile` point at by default. The indirection is in place +// so future tests could substitute in-memory filesystems without +// touching disk. +func osStat(p string) (os.FileInfo, error) { return os.Stat(p) } +func osReadFile(p string) ([]byte, error) { return os.ReadFile(p) } + +func init() { + modes["lint"] = runLint +} + +// AssertPlanDelegatesToHelper flags any provider type's Plan method +// whose body does NOT call platform.ComputePlan (the canonical Plan +// helper at platform/differ.go:72). Legacy `wfctlhelpers.Plan(...)` calls +// are also accepted as delegated, for forward-compatibility with rev0 +// of this analyzer when the rewrite target was still misnamed. See +// refactor_plan.go's planHelperImportPath docstring for the rev1 +// review-correction history (Copilot review finding #1). +// +// The check is syntactic — it matches the SelectorExpr regardless of +// whether the call resolves at type-check time, so it works on plugins +// that have not yet vendored the helper. +var AssertPlanDelegatesToHelper = &analysis.Analyzer{ + Name: "AssertPlanDelegatesToHelper", + Doc: "Provider Plan() must delegate to platform.ComputePlan.", + Run: runAssertPlanDelegatesToHelper, +} + +// AssertApplyDelegatesToHelper flags any provider type's Apply method whose +// body does NOT call wfctlhelpers.ApplyPlan. The canonical migration target +// is `return wfctlhelpers.ApplyPlan(ctx, p, plan)`. Same syntactic-match +// approach as AssertPlanDelegatesToHelper. +var AssertApplyDelegatesToHelper = &analysis.Analyzer{ + Name: "AssertApplyDelegatesToHelper", + Doc: "Provider Apply() must delegate to wfctlhelpers.ApplyPlan.", + Run: runAssertApplyDelegatesToHelper, +} + +// AssertDiffSetsNeedsReplaceForForceNew flags any driver Diff method that +// references a ForceNew field (typically FieldChange.ForceNew) but never +// assigns NeedsReplace = true (typically DiffResult.NeedsReplace). This is +// the W-3 contract: when a force-new field changes, the diff must signal +// replacement so platform.ComputePlan classifies the action correctly. +var AssertDiffSetsNeedsReplaceForForceNew = &analysis.Analyzer{ + Name: "AssertDiffSetsNeedsReplaceForForceNew", + Doc: "Driver Diff() that observes ForceNew fields must set DiffResult.NeedsReplace=true.", + Run: runAssertDiffSetsNeedsReplaceForForceNew, +} + +// AssertProviderImplementsValidatePlan flags any provider-shaped type +// (a type with Plan + Apply methods matching the IaCProvider signature) +// that does NOT also have a ValidatePlan method satisfying the +// ProviderValidator interface (`ValidatePlan(plan *IaCPlan) []PlanDiagnostic`). +// The check uses pass.TypesInfo to verify method-set membership rather +// than raw AST string-match per team-lead's W-8 brief. +var AssertProviderImplementsValidatePlan = &analysis.Analyzer{ + Name: "AssertProviderImplementsValidatePlan", + Doc: "Provider type must implement ProviderValidator (ValidatePlan method).", + Run: runAssertProviderImplementsValidatePlan, +} + +// lintAnalyzers is the canonical ordered list of T8.2 analyzers. Order +// is preserved in the report so output is deterministic across runs. +var lintAnalyzers = []*analysis.Analyzer{ + AssertPlanDelegatesToHelper, + AssertApplyDelegatesToHelper, + AssertDiffSetsNeedsReplaceForForceNew, + AssertProviderImplementsValidatePlan, +} + +// lintFinding captures one analyzer diagnostic for the report. +type lintFinding struct { + Path string + Line int + Analyzer string + Message string +} + +// skippedSite captures one declaration suppressed by SkipMarker. +type skippedSite struct { + Path string + Line int + Analyzer string + Decl string // function or type name +} + +// lintReport aggregates findings, skipped sites, and per-file errors +// across an entire lint run. +type lintReport struct { + findings []lintFinding + skipped []skippedSite + errors []string +} + +// runLint is the entry point for the lint subcommand. It is read-only +// by definition: the -fix flag is meaningless and a warning is surfaced +// so the user knows the flag did nothing. Mutation regardless of flag +// combination is pinned by TestRunLint_DoesNotMutateFilesEvenWithFixFlag. +func runLint(args []string, opts *Options, stdout, stderr io.Writer) int { + if len(args) == 0 { + fmt.Fprintln(stderr, "iac-codemod lint: at least one path is required") + usage(stderr) + return 2 + } + if opts != nil && opts.Fix { + // Lint never mutates. Surface a warning so the user knows -fix + // did not change behavior; preserves predictable advisory-only + // semantics from plan §W-8 line 397. + fmt.Fprintln(stderr, "iac-codemod lint: warning: -fix has no effect (lint is read-only)") + } + + report := &lintReport{} + for _, path := range args { + if err := lintPath(path, report); err != nil { + fmt.Fprintf(stderr, "iac-codemod lint: %s: %v\n", path, err) + return 1 + } + } + report.print(stdout) + // Exit code semantics: + // 0 = clean (no findings, no errors) + // 1 = advisory findings present (no per-file errors) + // 2 = per-file parse/type-check errors (findings count + // irrelevant; the analyzer never got a chance to run on + // at least one file) + // + // Round-1 #10 conflated findings and errors at exit 1, which let + // `make migrate-providers || true` swallow real failures. Round-5 + // #7 splits the codes so callers can `|| [ $? -eq 1 ]` to accept + // findings as advisory while still failing on unparseable input. + if len(report.errors) > 0 { + return 2 + } + if len(report.findings) > 0 { + return 1 + } + return 0 +} + +// lintPath walks path for *.go files (excluding _test.go, vendor, +// testdata, hidden dirs) and invokes lintFile on each. Per-file errors +// are recorded in the report rather than aborting the whole run so a +// single broken file in a multi-package plugin does not lose findings +// from the rest. +// +// Round-10 #5: rev2 of this walker called lintFile per file, and +// lintFile re-parsed every sibling per-call → O(n²) on packages with +// many files. Now lintFile takes an optional pre-parsed sibling +// cache (lintDirCache) so per-directory parses are reused across the +// directory's files. +func lintPath(path string, report *lintReport) error { + info, err := stat(path) + if err != nil { + return err + } + if !info.IsDir() { + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return fmt.Errorf("not a Go source file (or is a _test.go): %s", path) + } + if err := lintFile(path, nil, report); err != nil { + report.errors = append(report.errors, fmt.Sprintf("%s: %v", path, err)) + } + return nil + } + // Group files by directory so we can build a per-directory sibling + // parse cache once and reuse it across the directory's files. + dirFiles := make(map[string][]string) + if err := filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + base := d.Name() + if shouldSkipDir(base) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { + return nil + } + dir := filepath.Dir(p) + dirFiles[dir] = append(dirFiles[dir], p) + return nil + }); err != nil { + return err + } + // Process each directory with a fresh sibling cache. Errors per + // file are recorded in the report; we never abort the walk. + for dir, paths := range dirFiles { + cache := newLintDirCache(dir) + for _, p := range paths { + if err := lintFile(p, cache, report); err != nil { + report.errors = append(report.errors, fmt.Sprintf("%s: %v", p, err)) + } + } + } + return nil +} + +// lintDirCache caches parsed sibling files for a single directory so +// lintFile doesn't re-parse them per-target. Round-10 #5: closes the +// O(n²) perf gap. +type lintDirCache struct { + dir string + files map[string]*ast.File // path → parsed file (re-used across siblings) + fset *token.FileSet +} + +// newLintDirCache constructs a cache and pre-parses every non-test +// .go file in dir. Errors during pre-parse are silently dropped (the +// per-file pass will surface them via its own parse). +func newLintDirCache(dir string) *lintDirCache { + c := &lintDirCache{ + dir: dir, + files: make(map[string]*ast.File), + fset: token.NewFileSet(), + } + entries, err := os.ReadDir(dir) + if err != nil { + return c + } + for _, e := range entries { + if e.IsDir() { + continue + } + name := e.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + fpath := filepath.Join(dir, name) + src, err := readFile(fpath) + if err != nil { + continue + } + f, err := parser.ParseFile(c.fset, fpath, src, parser.ParseComments) + if err != nil { + continue + } + c.files[fpath] = f + } + return c +} + +// lintFile parses path, loads its sibling .go files (same directory, +// non-test) so cross-file method sets are visible to the analyzers, +// type-checks tolerantly, and runs every analyzer in lintAnalyzers. +// +// Review round-2 finding #9: rev0/rev1 of this function passed only +// the target file in pass.Files, so providerLikeReceivers / +// driverLikeReceivers / AssertProviderImplementsValidatePlan saw +// only methods declared in that file. Providers with Plan/Apply (or +// drivers with Diff + companion methods) split across sibling files +// were silently skipped — same blind spot the refactor-* modes had +// in round 1. Now lintFile loads every non-test .go file in the same +// directory and feeds the full slice to each analyzer. +// +// Diagnostics for files OTHER than `path` are silently dropped: each +// invocation of lintFile only owns the report for `path`, and the +// outer walker visits each file in turn. This avoids duplicate +// findings without requiring a higher-level dedup. Sibling files +// serve only as method-set context. +func lintFile(path string, cache *lintDirCache, report *lintReport) error { + // Round-10 #5: prefer the per-directory cache (built once per dir + // in lintPath) so sibling parses are reused across the directory's + // files. Falls back to per-call parsing when no cache is supplied + // (single-file invocation). + var primary *ast.File + var fset *token.FileSet + files := []*ast.File{} + if cache != nil && cache.files[path] != nil { + primary = cache.files[path] + fset = cache.fset + } else { + src, err := readFile(path) + if err != nil { + return err + } + fset = token.NewFileSet() + primary, err = parser.ParseFile(fset, path, src, parser.ParseComments) + if err != nil { + return err + } + } + files = append(files, primary) + // Sibling files from the cache (or per-call fallback walk). + if cache != nil { + for sibPath, sib := range cache.files { + if sibPath == path || sib == nil { + continue + } + if sib.Name.Name != primary.Name.Name { + continue + } + files = append(files, sib) + } + } else if entries, err := os.ReadDir(filepath.Dir(path)); err == nil { + for _, e := range entries { + if e.IsDir() { + continue + } + name := e.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + sibPath := filepath.Join(filepath.Dir(path), name) + if sibPath == path { + continue + } + sibSrc, err := readFile(sibPath) + if err != nil { + continue + } + sib, err := parser.ParseFile(fset, sibPath, sibSrc, parser.ParseComments) + if err != nil { + continue + } + if sib.Name.Name != primary.Name.Name { + continue + } + files = append(files, sib) + } + } + + conf := &types.Config{ + Importer: stubImporterRuntime{}, + Error: func(error) {}, // tolerate type errors; lint is best-effort + } + info := &types.Info{ + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + } + pkg, _ := conf.Check(primary.Name.Name, fset, files, info) + + for _, analyzer := range lintAnalyzers { + pass := &analysis.Pass{ + Analyzer: analyzer, + Fset: fset, + Files: files, + Pkg: pkg, + TypesInfo: info, + Report: func(d analysis.Diagnostic) { + // Drop diagnostics targeting other files: the outer + // walker will visit them in their own turn. + diagPath := fset.Position(d.Pos).Filename + if diagPath != "" && diagPath != path { + return + } + report.findings = append(report.findings, lintFinding{ + Path: path, + Line: fset.Position(d.Pos).Line, + Analyzer: analyzer.Name, + Message: d.Message, + }) + }, + } + if _, err := analyzer.Run(pass); err != nil { + return fmt.Errorf("%s: %w", analyzer.Name, err) + } + } + return nil +} + +// stubImporterRuntime is the importer used by the runtime lintFile path. +// It mirrors stubImporter in lint_test.go so test and runtime behavior +// stay aligned. +type stubImporterRuntime struct{} + +func (stubImporterRuntime) Import(path string) (*types.Package, error) { + return types.NewPackage(path, filepath.Base(path)), nil +} + +// stat / readFile are split out so tests could override them in future +// if needed. Today they are thin wrappers over os.Stat / os.ReadFile. +var ( + stat = osStat + readFile = osReadFile +) + +// ============================================================ +// Skip-marker helpers +// ============================================================ + +// hasSkipMarkerOn reports whether the given doc CommentGroup contains +// the canonical SkipMarker from main.go. Used by every analyzer that +// flags a function or type declaration. +// +// Accepted shapes: +// +// // wfctl:skip-iac-codemod +// // wfctl:skip-iac-codemod legacy upsert recovery, see ADR-042 +// // wfctl:skip-iac-codemod\tlegacy upsert recovery, see ADR-042 +// +// The marker followed by ANY whitespace separator + arbitrary +// justification text is honored (review round-2 follow-up A). Go +// maintainers may use spaces or tabs to align justifications; silently +// ignoring tab-delimited reasons would replicate the silent-no-op +// surface plan rev2 line 2400 unifies the marker to prevent. +// +// Rejected shapes (a non-whitespace suffix means a different marker): +// +// // wfctl:skip-iac-codemod-extension +// // wfctl:skip-iac-codemodSOMETHING +// // wfctl:skip-codemod (legacy, design rev1) +// +// The whitespace-separator discipline keeps the match tight enough +// that no substring shadow can bypass marker discipline. +func hasSkipMarkerOn(doc *ast.CommentGroup) bool { + if doc == nil { + return false + } + for _, c := range doc.List { + // Comment text includes the leading `//` per ast.Comment convention. + text := strings.TrimSpace(c.Text) + if text == SkipMarker { + return true + } + if strings.HasPrefix(text, SkipMarker) && len(text) > len(SkipMarker) { + next, _ := utf8.DecodeRuneInString(text[len(SkipMarker):]) + if unicode.IsSpace(next) { + return true + } + } + } + return false +} + +// Skipped sites are surfaced through the same pass.Report channel as +// real findings, distinguished by a message prefix. The driver +// (lintReport.unpackSkippedFromFindings) splits them out before +// rendering so skip records do NOT contribute to the finding count or +// the non-zero exit code. The indirection keeps each analyzer's API +// surface to a single Reportf-style channel rather than threading a +// second sink through every Run signature, and lets unit tests use a +// vanilla analysis.Pass without any custom rigging. +// +// IMPORTANT: lintFile invocation is currently sequential per path. If a +// future maintainer parallelises it, the skip-prefix encoding stays +// safe (each pass owns its own diagnostic slice via its Report closure) +// — but introducing concurrent map access via the package-level +// `modes` var or shared *lintReport pointer would not. See main_test.go +// header for the t.Parallel prohibition that applies here too. + +const skipDiagnosticPrefix = "[skipped] " + +// reportSkip emits a synthetic diagnostic that the driver decodes as a +// skipped-site record rather than a finding. This keeps the analyzer +// API surface minimal (one channel, not two). +func reportSkip(pass *analysis.Pass, pos token.Pos, declName string) { + pass.Report(analysis.Diagnostic{ + Pos: pos, + Message: skipDiagnosticPrefix + declName, + }) +} + +// ============================================================ +// Analyzer #1: AssertPlanDelegatesToHelper +// ============================================================ + +func runAssertPlanDelegatesToHelper(pass *analysis.Pass) (any, error) { + provs := providerLikeReceivers(pass) + typeDocsByFile := receiverTypeDocsForPass(pass) + for _, file := range pass.Files { + typeDocs := typeDocsByFile[file] + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + if !isProviderMethod(fn, "Plan", 3, 2) { + continue + } + recv := receiverTypeName(fn) + if !provs[recv] { + // Method named Plan on a non-provider type (e.g., a + // deploy target). Skip to keep precision high. + continue + } + // Honor SkipMarker on fn.Doc OR receiver-type docs (review + // round-2 finding #6). + if hasSkipMarkerOn(fn.Doc) || typeDocs[recv].carriesMarker() { + routeSkip(pass, fn) + continue + } + // Round-10 #7: rev3 accepted ANY platform.ComputePlan or + // wfctlhelpers.Plan call anywhere in the body, so a Plan + // method that called the helper as an intermediate step + // (then added bespoke logic, returned a wrapped value, + // etc.) was reported clean despite NOT actually delegating. + // Now we require the canonical SHAPE: either the + // 2-statement delegation form (matching + // isAlreadyDelegatedPlanBody) OR a single-statement legacy + // `return .Plan(...)`. Anything else flags the + // diagnostic so the maintainer reviews the bespoke wrapper. + if !planBodyDelegatesCanonically(fn.Body, file) { + pass.Reportf(fn.Pos(), "%s.%s does not delegate to platform.ComputePlan; non-canonical Plan() body", receiverTypeName(fn), fn.Name.Name) + } + } + } + return nil, nil +} + +// planBodyDelegatesCanonically returns true if body matches the +// canonical Plan-delegation shape (round-10 #7). Accepts EITHER: +// +// - 2-statement rev2 form: `plan, err := .ComputePlan(...); +// return &plan, err` (matches isAlreadyDelegatedPlanBody) +// - single-statement legacy form: `return .Plan(...)` +// OR `return .ComputePlan(...)` (planned-but-not-shipped +// and broken-rev1 fixtures, accepted as advisory-clean here even +// though the rewriter would repair them) +// +// Anything else (including bodies that CALL the helper anywhere but +// don't return its value verbatim) is rejected as non-canonical. +func planBodyDelegatesCanonically(body *ast.BlockStmt, file *ast.File) bool { + if body == nil { + return false + } + // Shape 1: 2-statement form (matches the rewriter's idempotency). + if isAlreadyDelegatedPlanBody(body, file) { + return true + } + // Shape 2: single-statement legacy `return .Plan(...)`. + // The planned-but-not-shipped wfctlhelpers.Plan target was speculative; + // any code using it is fictional and the type-check will fail anyway, + // but we accept it as advisory-clean so a maintainer who hand-applied + // rev0 of this codemod isn't re-flagged. + // + // Round-11 #1: the BROKEN `return platform.ComputePlan(...)` + // single-statement form (rev1 ill-formed rewrite — uncompilable + // due to value/pointer mismatch) is REJECTED here. Lint should + // surface this as still-needs-fixup so `migrate-providers` + // catches partially-migrated providers. + if len(body.List) == 1 { + if ret, ok := body.List[0].(*ast.ReturnStmt); ok && len(ret.Results) == 1 { + if call, ok := ret.Results[0].(*ast.CallExpr); ok { + if sel, ok := call.Fun.(*ast.SelectorExpr); ok { + if x, ok := sel.X.(*ast.Ident); ok { + wfhAlias := pkgAliasFor(file, helperImportPath, "wfctlhelpers") + if (x.Name == wfhAlias || x.Name == "wfctlhelpers") && sel.Sel.Name == "Plan" { + return true + } + } + } + } + } + } + return false +} + +// receiverTypeDocsForPass builds a SINGLE merged receiverDoc map +// across every file in pass.Files. The same map is returned per-file +// (callers do `typeDocs := typeDocsByFile[file]`) — they get the +// directory-wide view so a skip-marker on a sibling file's type +// declaration is honored even when the function being analyzed lives +// in a different file. Round-6 finding #1: rev2 returned per-file +// maps, so `typeDocs[recv]` missed sibling-file TypeSpec docs and +// providers split across files were rewritten despite type-doc skip +// markers. +// +// First-occurrence wins: if multiple files declare the same receiver +// type name (an unusual layout but possible), the first iteration +// order wins. The lint analyzers prefer the in-file declaration over +// shadows since they iterate pass.Files in stable order. +func receiverTypeDocsForPass(pass *analysis.Pass) map[*ast.File]map[string]receiverDoc { + merged := make(map[string]receiverDoc) + for _, file := range pass.Files { + for recv, doc := range receiverTypeDocs(file) { + if _, ok := merged[recv]; ok { + continue + } + merged[recv] = doc + } + } + out := make(map[*ast.File]map[string]receiverDoc, len(pass.Files)) + for _, file := range pass.Files { + out[file] = merged + } + return out +} + +// ============================================================ +// Analyzer #2: AssertApplyDelegatesToHelper +// ============================================================ + +func runAssertApplyDelegatesToHelper(pass *analysis.Pass) (any, error) { + provs := providerLikeReceivers(pass) + typeDocsByFile := receiverTypeDocsForPass(pass) + for _, file := range pass.Files { + typeDocs := typeDocsByFile[file] + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + if !isProviderMethod(fn, "Apply", 2, 2) { + continue + } + recv := receiverTypeName(fn) + if !provs[recv] { + // Method named Apply on a non-provider type. Skip. + continue + } + // Honor SkipMarker on fn.Doc OR receiver-type docs (review + // round-2 finding #7). + if hasSkipMarkerOn(fn.Doc) || typeDocs[recv].carriesMarker() { + routeSkip(pass, fn) + continue + } + // Round-10 #8: rev3 accepted ANY wfctlhelpers.ApplyPlan + // call anywhere in the body, so an Apply that referenced + // the helper incidentally (with extra work before/after) + // was reported clean despite NOT actually delegating. Now + // we require the canonical single-statement + // `return .ApplyPlan(...)` form (the same shape the + // rewriter checks for idempotency). + if !isAlreadyDelegatedApplyBody(fn.Body, file) { + pass.Reportf(fn.Pos(), "%s.%s does not delegate to wfctlhelpers.ApplyPlan; non-canonical Apply() body", receiverTypeName(fn), fn.Name.Name) + } + } + } + return nil, nil +} + +// ============================================================ +// Analyzer #3: AssertDiffSetsNeedsReplaceForForceNew +// ============================================================ + +func runAssertDiffSetsNeedsReplaceForForceNew(pass *analysis.Pass) (any, error) { + drivers := driverLikeReceivers(pass) + typeDocsByFile := receiverTypeDocsForPass(pass) + for _, file := range pass.Files { + typeDocs := typeDocsByFile[file] + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + if !isProviderMethod(fn, "Diff", 3, 2) { + continue + } + recv := receiverTypeName(fn) + if !drivers[recv] { + // Method named Diff on a non-driver type (e.g., a + // settings struct or config differ). Skip to keep + // precision high — review finding #3. + continue + } + // Honor SkipMarker on fn.Doc OR receiver-type docs (review + // round-2 finding #8). + if hasSkipMarkerOn(fn.Doc) || typeDocs[recv].carriesMarker() { + routeSkip(pass, fn) + continue + } + refsForceNew := bodyReferencesField(fn.Body, "ForceNew") + assignsNeedsReplace := bodyAssignsField(fn.Body, "NeedsReplace") + if refsForceNew && !assignsNeedsReplace { + pass.Reportf(fn.Pos(), "%s.%s references ForceNew but never assigns NeedsReplace; W-3 force-new contract violated", receiverTypeName(fn), fn.Name.Name) + } + } + } + return nil, nil +} + +// ============================================================ +// Analyzer #4: AssertProviderImplementsValidatePlan +// ============================================================ + +func runAssertProviderImplementsValidatePlan(pass *analysis.Pass) (any, error) { + if pass.Pkg == nil { + return nil, nil + } + scope := pass.Pkg.Scope() + if scope == nil { + return nil, nil + } + // Group method sets by receiver type name, walking AST so we can + // surface the original ast.FuncDecl for skip-marker handling. + // typeDocsByName captures both TypeSpec.Doc and the wrapping + // GenDecl.Doc so the skip-marker check can consult both — review + // round-3 finding #7: rev2 only checked ts.Doc, missing markers + // placed before the `type` keyword (the wrapping GenDecl). + methodsByRecv := make(map[string][]*ast.FuncDecl) + typeDecls := make(map[string]*ast.TypeSpec) + typeDocsByName := make(map[string]receiverDoc) + for _, file := range pass.Files { + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + if d.Recv == nil || len(d.Recv.List) == 0 { + continue + } + recv := receiverTypeName(d) + if recv == "" { + continue + } + methodsByRecv[recv] = append(methodsByRecv[recv], d) + case *ast.GenDecl: + if d.Tok != token.TYPE { + continue + } + for _, spec := range d.Specs { + ts, ok := spec.(*ast.TypeSpec) + if !ok { + continue + } + if _, isStruct := ts.Type.(*ast.StructType); !isStruct { + continue + } + typeDecls[ts.Name.Name] = ts + typeDocsByName[ts.Name.Name] = receiverDoc{ + TypeSpecDoc: ts.Doc, + GenDeclDoc: d.Doc, + } + } + } + } + } + for recv, methods := range methodsByRecv { + if !looksLikeProvider(methods) { + continue + } + // Skip if the type's TypeSpec.Doc OR wrapping GenDecl.Doc + // carries the marker, or any of the provider's signature + // methods (Plan/Apply) carry it. ValidatePlan being absent is + // the whole point of this analyzer, so checking only + // Plan/Apply is sufficient. + if typeDocsByName[recv].carriesMarker() { + ts := typeDecls[recv] + pos := token.NoPos + if ts != nil { + pos = ts.Pos() + } else if len(methods) > 0 { + pos = methods[0].Pos() + } + routeSkipName(pass, pos, recv) + continue + } + // Round-8 #3: rev2 checked the marker on EVERY method, so a + // marker on Destroy/Status/etc. accidentally suppressed the + // whole provider's analysis. Restrict to Plan and Apply (the + // provider-defining methods that actually opt the type out). + anyMarker := false + for _, m := range methods { + if m.Name.Name != "Plan" && m.Name.Name != "Apply" { + continue + } + if hasSkipMarkerOn(m.Doc) { + anyMarker = true + break + } + } + if anyMarker { + routeSkipName(pass, methods[0].Pos(), recv) + continue + } + // Signature + receiver-kind match. Round-1 #11 added the + // signature check; round-5 #4 added the receiver-kind check + // (a value-receiver provider with a pointer-receiver + // ValidatePlan still fails the ProviderValidator type + // assertion because the method set on `T` does not include + // `*T` methods). hasValidatePlanMethod centralises the logic. + if hasValidatePlanMethod(methods) { + continue + } + // Round-11 #4 reverts round-10 #3's broad-suppress on + // embedded fields: many embeddings (sync.Mutex, loggers, + // config mixins) don't promote ValidatePlan, so real targets + // were silently missed. Maintainers whose providers actually + // promote ValidatePlan can suppress with the explicit + // `// wfctl:skip-iac-codemod` marker (the universal opt-out). + // Report at the type decl if available, else at the first method. + var pos token.Pos + if ts, ok := typeDecls[recv]; ok { + pos = ts.Pos() + } else { + pos = methods[0].Pos() + } + pass.Reportf(pos, "provider type %s does not implement ValidatePlan; ProviderValidator (R-A10) cannot run on plans involving this provider", recv) + } + return nil, nil +} + +// driverLikeReceivers returns the set of receiver type names whose +// method set in pass.Files contains a Diff method AND at least one +// canonical companion driver method (Read, Create, Update, Delete). +// Used by AssertDiffSetsNeedsReplaceForForceNew to keep precision high +// — review finding #3: a type with only Diff (e.g. a config differ) +// is not a resource driver and should not be analysed for force-new +// contract compliance. +func driverLikeReceivers(pass *analysis.Pass) map[string]bool { + methodsByRecv := make(map[string][]*ast.FuncDecl) + for _, file := range pass.Files { + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + recv := receiverTypeName(fn) + if recv == "" { + continue + } + methodsByRecv[recv] = append(methodsByRecv[recv], fn) + } + } + out := make(map[string]bool) + for recv, methods := range methodsByRecv { + hasDiff, hasCompanion := false, false + for _, m := range methods { + switch m.Name.Name { + case "Diff": + if m.Type.Params != nil && len(m.Type.Params.List) >= 2 && m.Type.Results != nil && len(m.Type.Results.List) == 2 { + hasDiff = true + } + case "Read", "Create", "Update", "Delete": + hasCompanion = true + } + } + if hasDiff && hasCompanion { + out[recv] = true + } + } + return out +} + +// providerLikeReceivers returns the set of receiver type names whose +// method set in pass.Files contains both Plan and Apply with shapes +// matching IaCProvider. Used by every analyzer that should fire only +// on IaC providers (not on deploy targets or other Apply-shaped types). +func providerLikeReceivers(pass *analysis.Pass) map[string]bool { + methodsByRecv := make(map[string][]*ast.FuncDecl) + for _, file := range pass.Files { + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + recv := receiverTypeName(fn) + if recv == "" { + continue + } + methodsByRecv[recv] = append(methodsByRecv[recv], fn) + } + } + out := make(map[string]bool) + for recv, methods := range methodsByRecv { + if looksLikeProvider(methods) { + out[recv] = true + } + } + return out +} + +// looksLikeProvider returns true if the method list contains both Plan +// and Apply with shapes matching IaCProvider: +// +// Plan(context.Context, []ResourceSpec, []ResourceState) (*IaCPlan, error) +// Apply(context.Context, *IaCPlan) (*ApplyResult, error) +// +// Round-12 #8: rev1 only checked method NAMES + rough arity, so any +// unrelated type with `Plan(...)` and `Apply(...)` (e.g., a deploy +// strategy or a UI handler) was treated as a provider. Tightened to +// match the signature shape via type-name suffix checks (qualified or +// unqualified): IaCPlan / ResourceSpec / ResourceState / ApplyResult / +// context.Context. +func looksLikeProvider(methods []*ast.FuncDecl) bool { + hasPlan, hasApply := false, false + for _, m := range methods { + switch m.Name.Name { + case "Plan": + if planSignatureMatches(m.Type) { + hasPlan = true + } + case "Apply": + if applySignatureMatches(m.Type) { + hasApply = true + } + } + } + return hasPlan && hasApply +} + +// planSignatureMatches verifies the function type matches +// `Plan(ctx, []ResourceSpec, []ResourceState) (*IaCPlan, error)`. +// Returns true if the parameter and result types match by name suffix +// (qualified or unqualified). Used by looksLikeProvider to filter +// false positives on unrelated `Plan` methods (round-12 #8). +func planSignatureMatches(ft *ast.FuncType) bool { + if ft == nil || ft.Params == nil || ft.Results == nil { + return false + } + paramTypes := flattenFieldTypes(ft.Params.List) + if len(paramTypes) != 3 { + return false + } + resultTypes := flattenFieldTypes(ft.Results.List) + if len(resultTypes) != 2 { + return false + } + // Param 1: context.Context (selector .Context) + if !typeNameTailMatches(paramTypes[0], "Context") { + return false + } + // Param 2: []ResourceSpec + arr, ok := paramTypes[1].(*ast.ArrayType) + if !ok || arr.Len != nil || !typeNameTailMatches(arr.Elt, "ResourceSpec") { + return false + } + // Param 3: []ResourceState + arr2, ok := paramTypes[2].(*ast.ArrayType) + if !ok || arr2.Len != nil || !typeNameTailMatches(arr2.Elt, "ResourceState") { + return false + } + // Result 1: *IaCPlan + star, ok := resultTypes[0].(*ast.StarExpr) + if !ok || !typeNameTailMatches(star.X, "IaCPlan") { + return false + } + // Result 2: error + return typeNameTailMatches(resultTypes[1], "error") +} + +// applySignatureMatches verifies the function type matches +// `Apply(ctx, *IaCPlan) (*ApplyResult, error)`. +func applySignatureMatches(ft *ast.FuncType) bool { + if ft == nil || ft.Params == nil || ft.Results == nil { + return false + } + paramTypes := flattenFieldTypes(ft.Params.List) + if len(paramTypes) != 2 { + return false + } + resultTypes := flattenFieldTypes(ft.Results.List) + if len(resultTypes) != 2 { + return false + } + if !typeNameTailMatches(paramTypes[0], "Context") { + return false + } + star, ok := paramTypes[1].(*ast.StarExpr) + if !ok || !typeNameTailMatches(star.X, "IaCPlan") { + return false + } + starR, ok := resultTypes[0].(*ast.StarExpr) + if !ok || !typeNameTailMatches(starR.X, "ApplyResult") { + return false + } + return typeNameTailMatches(resultTypes[1], "error") +} + +// flattenFieldTypes expands a Go FieldList (where `a, b T` is one +// field with two names) into a flat slice of types — one per +// parameter or return value. +func flattenFieldTypes(list []*ast.Field) []ast.Expr { + var out []ast.Expr + for _, f := range list { + count := 1 + if len(f.Names) > 1 { + count = len(f.Names) + } + for i := 0; i < count; i++ { + out = append(out, f.Type) + } + } + return out +} + +// ============================================================ +// Shared AST helpers +// ============================================================ + +// isProviderMethod returns true if fn is a method (has receiver) named +// methodName, with at least minParams parameter fields and exactly +// expectedResults result fields. Parameter and result counts are +// approximate (Go FieldList groups multi-param fields like `a, b T` +// into one field), so the actual call-site arity may differ — but the +// shape filter is sufficient for distinguishing IaCProvider/Driver +// methods from unrelated lookalikes. +func isProviderMethod(fn *ast.FuncDecl, methodName string, minParams, expectedResults int) bool { + if fn.Recv == nil || len(fn.Recv.List) == 0 { + return false + } + if fn.Name.Name != methodName { + return false + } + if fn.Type.Params == nil || len(fn.Type.Params.List) < minParams { + return false + } + if fn.Type.Results == nil || len(fn.Type.Results.List) != expectedResults { + return false + } + if fn.Body == nil { + return false + } + return true +} + +// receiverTypeName extracts the receiver type identifier from a method +// declaration, stripping any pointer indirection. Returns "" for +// unrecognised receiver shapes. +func receiverTypeName(fn *ast.FuncDecl) string { + if fn.Recv == nil || len(fn.Recv.List) == 0 { + return "" + } + expr := fn.Recv.List[0].Type + if star, ok := expr.(*ast.StarExpr); ok { + expr = star.X + } + id, ok := expr.(*ast.Ident) + if !ok { + return "" + } + return id.Name +} + +// bodyCallsSelector reports whether the function body contains a +// CallExpr whose callee is a SelectorExpr with the given X.Name and +// Sel.Name, e.g. `wfctlhelpers.Plan(...)`. +func bodyCallsSelector(body *ast.BlockStmt, pkgIdent, selName string) bool { + if body == nil { + return false + } + found := false + ast.Inspect(body, func(n ast.Node) bool { + if found { + return false + } + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + x, ok := sel.X.(*ast.Ident) + if !ok { + return true + } + if x.Name == pkgIdent && sel.Sel.Name == selName { + found = true + return false + } + return true + }) + return found +} + +// bodyReferencesField reports whether the function body references any +// SelectorExpr with the given Sel.Name, e.g. any `.ForceNew`. +func bodyReferencesField(body *ast.BlockStmt, fieldName string) bool { + if body == nil { + return false + } + found := false + ast.Inspect(body, func(n ast.Node) bool { + if found { + return false + } + sel, ok := n.(*ast.SelectorExpr) + if !ok { + return true + } + if sel.Sel.Name == fieldName { + found = true + return false + } + return true + }) + return found +} + +// bodyAssignsField reports whether the function body contains any +// assignment `.fieldName = ` regardless of RHS shape, EXCEPT +// for an explicit literal `false` (which is treated as no-assignment). +// Both `r.NeedsReplace = true` (inside an `if c.ForceNew` guard) and +// the terser `r.NeedsReplace = c.ForceNew` are valid expressions of +// the W-3 force-new contract — review round-1 finding #4 widened the +// matcher so the second pattern doesn't trigger a false positive. +// Review round-2 follow-up B then narrowed the widening so the +// copy-paste bug `r.NeedsReplace = false` (assigning the wrong +// constant inside a ForceNew branch) is still flagged. Maintainers +// who genuinely don't propagate ForceNew leave NeedsReplace untouched +// entirely, which the analyzer also catches. +func bodyAssignsField(body *ast.BlockStmt, fieldName string) bool { + if body == nil { + return false + } + found := false + ast.Inspect(body, func(n ast.Node) bool { + if found { + return false + } + assign, ok := n.(*ast.AssignStmt) + if !ok { + return true + } + for i, lhs := range assign.Lhs { + sel, ok := lhs.(*ast.SelectorExpr) + if !ok { + continue + } + if sel.Sel.Name != fieldName { + continue + } + if i < len(assign.Rhs) { + if id, ok := assign.Rhs[i].(*ast.Ident); ok && id.Name == "false" { + // Literal-false assignment is treated as + // no-assignment so a copy-paste typo inside the + // ForceNew branch is still flagged. + continue + } + } + found = true + return false + } + return true + }) + return found +} + +// routeSkip records a skipped FuncDecl through the pass.Report channel +// using the skipDiagnosticPrefix encoding. +func routeSkip(pass *analysis.Pass, fn *ast.FuncDecl) { + declName := fmt.Sprintf("%s.%s", receiverTypeName(fn), fn.Name.Name) + reportSkip(pass, fn.Pos(), declName) +} + +// routeSkipName records a skipped declaration by its name (used for +// type-level skips). +func routeSkipName(pass *analysis.Pass, pos token.Pos, name string) { + reportSkip(pass, pos, name) +} + +// ============================================================ +// Report rendering +// ============================================================ + +// print renders the report to w in Markdown-ish format. Findings come +// first (sorted by file, line, analyzer); then skipped sites; then +// per-file errors. Skipped diagnostics encoded with skipDiagnosticPrefix +// are extracted from findings into the skipped section first so the +// finding count reflects only real issues. +func (r *lintReport) print(w io.Writer) { + r.unpackSkippedFromFindings() + + sort.Slice(r.findings, func(i, j int) bool { + if r.findings[i].Path != r.findings[j].Path { + return r.findings[i].Path < r.findings[j].Path + } + if r.findings[i].Line != r.findings[j].Line { + return r.findings[i].Line < r.findings[j].Line + } + return r.findings[i].Analyzer < r.findings[j].Analyzer + }) + sort.Slice(r.skipped, func(i, j int) bool { + if r.skipped[i].Path != r.skipped[j].Path { + return r.skipped[i].Path < r.skipped[j].Path + } + return r.skipped[i].Line < r.skipped[j].Line + }) + + fmt.Fprintln(w, "# iac-codemod lint report") + fmt.Fprintln(w) + fmt.Fprintf(w, "Findings: %d\n", len(r.findings)) + fmt.Fprintf(w, "Skipped: %d\n", len(r.skipped)) + fmt.Fprintf(w, "Errors: %d\n", len(r.errors)) + fmt.Fprintln(w) + + if len(r.findings) > 0 { + fmt.Fprintln(w, "## Findings") + fmt.Fprintln(w) + for _, f := range r.findings { + fmt.Fprintf(w, "- %s:%d [%s] %s\n", f.Path, f.Line, f.Analyzer, f.Message) + } + fmt.Fprintln(w) + } + + if len(r.skipped) > 0 { + fmt.Fprintln(w, "## Skipped (// wfctl:skip-iac-codemod)") + fmt.Fprintln(w) + for _, s := range r.skipped { + fmt.Fprintf(w, "- %s:%d [%s] %s\n", s.Path, s.Line, s.Analyzer, s.Decl) + } + fmt.Fprintln(w) + } + + if len(r.errors) > 0 { + fmt.Fprintln(w, "## Errors") + fmt.Fprintln(w) + for _, e := range r.errors { + fmt.Fprintf(w, "- %s\n", e) + } + fmt.Fprintln(w) + } +} + +// unpackSkippedFromFindings moves skip-prefixed diagnostics from the +// findings list into the skipped list, restoring the canonical exit-code +// semantics (skipped sites do not count as findings). +func (r *lintReport) unpackSkippedFromFindings() { + if len(r.findings) == 0 { + return + } + kept := r.findings[:0] + for _, f := range r.findings { + if decl, ok := strings.CutPrefix(f.Message, skipDiagnosticPrefix); ok { + r.skipped = append(r.skipped, skippedSite{ + Path: f.Path, + Line: f.Line, + Analyzer: f.Analyzer, + Decl: decl, + }) + continue + } + kept = append(kept, f) + } + r.findings = kept +} diff --git a/cmd/iac-codemod/lint_test.go b/cmd/iac-codemod/lint_test.go new file mode 100644 index 00000000..754df0a7 --- /dev/null +++ b/cmd/iac-codemod/lint_test.go @@ -0,0 +1,751 @@ +// Copyright (c) 2026 Jon Langevin +// SPDX-License-Identifier: Apache-2.0 + +// See main_test.go for the t.Parallel() prohibition (this file follows +// the same constraint — modes map is mutated transitively via the lint +// init() call and cross-test analyzer state). + +package main + +import ( + "bytes" + "fmt" + "go/ast" + "go/parser" + "go/token" + "go/types" + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/tools/go/analysis" +) + +// runAnalyzerOnSource parses a single Go source string, type-checks it +// tolerantly, runs the supplied analyzer, and returns the REAL diagnostics +// (skip-encoded synthetic diagnostics from skip-marker handling are +// filtered out here, matching the driver's post-processing). Use +// runAnalyzerOnSourceRaw if you need to inspect skip records directly. +func runAnalyzerOnSource(t *testing.T, src string, analyzer *analysis.Analyzer) []analysis.Diagnostic { + t.Helper() + all := runAnalyzerOnSourceRaw(t, src, analyzer) + out := all[:0] + for _, d := range all { + if strings.HasPrefix(d.Message, skipDiagnosticPrefix) { + continue + } + out = append(out, d) + } + return out +} + +// runAnalyzerOnSourceRaw is like runAnalyzerOnSource but returns ALL +// diagnostics (including skip-encoded ones). Used by skip-marker tests +// that need to verify the synthetic record was emitted at all. +func runAnalyzerOnSourceRaw(t *testing.T, src string, analyzer *analysis.Analyzer) []analysis.Diagnostic { + t.Helper() + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "src.go", src, parser.ParseComments) + if err != nil { + t.Fatalf("parse: %v\nsrc:\n%s", err, src) + } + conf := &types.Config{ + Importer: stubImporter{}, + Error: func(err error) {}, // tolerate unresolved-import / undeclared-name errors + } + info := &types.Info{ + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + } + pkg, _ := conf.Check(file.Name.Name, fset, []*ast.File{file}, info) + var diags []analysis.Diagnostic + pass := &analysis.Pass{ + Analyzer: analyzer, + Fset: fset, + Files: []*ast.File{file}, + Pkg: pkg, + TypesInfo: info, + Report: func(d analysis.Diagnostic) { diags = append(diags, d) }, + } + if _, err := analyzer.Run(pass); err != nil { + t.Fatalf("analyzer %s: %v", analyzer.Name, err) + } + return diags +} + +// stubImporter is a tolerant importer that returns an empty package for +// any import path. It lets type-check proceed past unresolved imports +// like "wfctlhelpers" or "interfaces" without bailing. +type stubImporter struct{} + +func (stubImporter) Import(path string) (*types.Package, error) { + return types.NewPackage(path, filepath.Base(path)), nil +} + +// ============================================================ +// AssertPlanDelegatesToHelper +// ============================================================ + +// providerScaffold is the boilerplate every Plan/Apply test source +// includes so its receiver type satisfies the precision filter +// (providerLikeReceivers — must have BOTH Plan and Apply matching +// IaCProvider shape) and the integration-test "no findings" cases pass +// every analyzer. Apply is canonical and ValidatePlan is present so +// only the method under test (Plan) drives the analyzer behaviour. +const providerScaffold = `package p +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type FooProvider struct{} + +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return wfctlhelpers.ApplyPlan(ctx, p, plan) +} +func (p *FooProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +// planCanonicalSrc uses the canonical 2-statement form per round-2 +// finding #1 (the value/pointer bridge for platform.ComputePlan). +const planCanonicalSrc = providerScaffold + ` +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + plan, err := platform.ComputePlan(ctx, p, desired, current) + return &plan, err +} +` + +// planLegacyDelegatedSrc preserves the rev0 codemod's planned-but-not-shipped +// `wfctlhelpers.Plan` target as also-accepted. Pinned regression: a maintainer +// who hand-applied an early version of the codemod must NOT be re-flagged. +const planLegacyDelegatedSrc = providerScaffold + ` +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return wfctlhelpers.Plan(ctx, p, desired, current) +} +` + +const planNonCanonicalSrc = providerScaffold + ` +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + // Custom planning logic, not delegating to platform.ComputePlan. + return &IaCPlan{}, nil +} +` + +const planSkippedSrc = providerScaffold + ` +// wfctl:skip-iac-codemod +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return &IaCPlan{}, nil +} +` + +func TestAssertPlanDelegatesToHelper_Canonical_NoDiagnostic(t *testing.T) { + diags := runAnalyzerOnSource(t, planCanonicalSrc, AssertPlanDelegatesToHelper) + if len(diags) != 0 { + t.Errorf("canonical Plan should produce no diagnostic; got %d:\n%s", len(diags), diagSummary(diags)) + } +} + +func TestAssertPlanDelegatesToHelper_NonCanonical_Diagnoses(t *testing.T) { + diags := runAnalyzerOnSource(t, planNonCanonicalSrc, AssertPlanDelegatesToHelper) + if len(diags) != 1 { + t.Fatalf("non-canonical Plan should produce 1 diagnostic; got %d:\n%s", len(diags), diagSummary(diags)) + } + if !strings.Contains(diags[0].Message, "platform.ComputePlan") { + t.Errorf("diagnostic should reference platform.ComputePlan (canonical target); got %q", diags[0].Message) + } +} + +// TestAssertPlanDelegatesToHelper_LegacyTargetAccepted pins review round-1 +// finding #1: the analyzer accepts the legacy `wfctlhelpers.Plan` target as +// already-delegated so a maintainer who hand-applied the rev0 codemod isn't +// re-flagged on the next run. +func TestAssertPlanDelegatesToHelper_LegacyTargetAccepted(t *testing.T) { + diags := runAnalyzerOnSource(t, planLegacyDelegatedSrc, AssertPlanDelegatesToHelper) + if len(diags) != 0 { + t.Errorf("legacy wfctlhelpers.Plan target must be accepted as delegated; got %d:\n%s", len(diags), diagSummary(diags)) + } +} + +func TestAssertPlanDelegatesToHelper_SkipMarker_Honored(t *testing.T) { + // Real findings should be empty (the marker suppresses the + // non-canonical-Plan diagnostic). + diags := runAnalyzerOnSource(t, planSkippedSrc, AssertPlanDelegatesToHelper) + if len(diags) != 0 { + t.Errorf("skip-marker should suppress real diagnostic; got %d:\n%s", len(diags), diagSummary(diags)) + } + // And a skip-encoded synthetic diagnostic should be present so the + // driver can surface the skipped site in its report (plan rev2 line + // 2400: "Each mode also surfaces a list of skipped sites in its + // report"). + all := runAnalyzerOnSourceRaw(t, planSkippedSrc, AssertPlanDelegatesToHelper) + gotSkip := false + for _, d := range all { + if strings.HasPrefix(d.Message, skipDiagnosticPrefix) { + gotSkip = true + break + } + } + if !gotSkip { + t.Errorf("skip-marker should produce a skip record for the driver to surface; got:\n%s", diagSummary(all)) + } +} + +// TestSkipMarker_AcceptsTrailingJustification pins review-round-2 finding +// #2: a trailing space + justification context after SkipMarker is a +// natural Go idiom (`// wfctl:skip-iac-codemod legacy upsert recovery, +// see ADR-042`) and must NOT silently turn the marker into a no-op. +// Plan rev2 line 2400 unifies the marker specifically to prevent +// silent-no-op surfaces; permissive trailing-context is in that family. +const planSkippedWithJustificationSrc = providerScaffold + ` +// wfctl:skip-iac-codemod legacy upsert recovery, see ADR-042 +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return &IaCPlan{}, nil +} +` + +func TestSkipMarker_AcceptsTrailingJustification(t *testing.T) { + diags := runAnalyzerOnSource(t, planSkippedWithJustificationSrc, AssertPlanDelegatesToHelper) + if len(diags) != 0 { + t.Errorf("trailing justification text after marker must NOT silently break suppression; got %d diagnostics:\n%s", len(diags), diagSummary(diags)) + } +} + +// TestSkipMarker_RejectsCloseButWrongMarker confirms we only accept the +// canonical marker — a different prefix (e.g. legacy +// `// wfctl:skip-codemod` from the design rev1 era) should still +// flag the diagnostic. Guards against accidentally-too-loose matching. +const planSkippedWithWrongMarkerSrc = providerScaffold + ` +// wfctl:skip-codemod +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return &IaCPlan{}, nil +} +` + +func TestSkipMarker_RejectsCloseButWrongMarker(t *testing.T) { + diags := runAnalyzerOnSource(t, planSkippedWithWrongMarkerSrc, AssertPlanDelegatesToHelper) + if len(diags) != 1 { + t.Errorf("non-canonical marker `// wfctl:skip-codemod` must NOT suppress the diagnostic (plan rev2 unifies on // wfctl:skip-iac-codemod ONLY); got %d:\n%s", len(diags), diagSummary(diags)) + } +} + +// TestSkipMarker_AcceptsTabDelimitedJustification — review round-2 +// follow-up A. Maintainers who tab-align justifications must NOT see a +// silent no-op; the marker logic widens to accept any whitespace +// separator. +const planSkippedTabJustifiedSrc = providerScaffold + "\n// wfctl:skip-iac-codemod\tlegacy upsert recovery, see ADR-042\nfunc (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) {\n\treturn &IaCPlan{}, nil\n}\n" + +func TestSkipMarker_AcceptsTabDelimitedJustification(t *testing.T) { + diags := runAnalyzerOnSource(t, planSkippedTabJustifiedSrc, AssertPlanDelegatesToHelper) + if len(diags) != 0 { + t.Errorf("tab-delimited justification must NOT silently break the marker; got %d:\n%s", len(diags), diagSummary(diags)) + } +} + +// TestSkipMarker_RejectsAdjacentNonWhitespace — review round-2 follow-up +// C. Pin that strings sharing the marker prefix but extending without a +// whitespace separator (dash/letter/digit suffix) are NOT accepted as +// the marker, so future loosening of hasSkipMarkerOn fails this test. +func TestSkipMarker_RejectsAdjacentNonWhitespace(t *testing.T) { + cases := []struct { + name, comment string + }{ + {"dash-suffix", "// wfctl:skip-iac-codemod-extension"}, + {"letters-suffix", "// wfctl:skip-iac-codemodSOMETHING"}, + {"digit-suffix", "// wfctl:skip-iac-codemod1"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + src := providerScaffold + "\n" + tc.comment + "\nfunc (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) {\n\treturn &IaCPlan{}, nil\n}\n" + diags := runAnalyzerOnSource(t, src, AssertPlanDelegatesToHelper) + if len(diags) != 1 { + t.Errorf("comment %q without whitespace separator must NOT match the marker; got %d:\n%s", tc.comment, len(diags), diagSummary(diags)) + } + }) + } +} + +// ============================================================ +// AssertApplyDelegatesToHelper +// ============================================================ + +// applyTestScaffold mirrors providerScaffold but with a canonical Plan +// (so the receiver passes the provider-like filter without the Apply +// analyzer under test being affected). ValidatePlan is included so +// integration-test "no findings" cases stay clean across all analyzers. +const applyTestScaffold = `package p +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type FooProvider struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + plan, err := platform.ComputePlan(ctx, p, desired, current) + return &plan, err +} +func (p *FooProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +const applyCanonicalSrc = applyTestScaffold + ` +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return wfctlhelpers.ApplyPlan(ctx, p, plan) +} +` + +const applyNonCanonicalSrc = applyTestScaffold + ` +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return &ApplyResult{}, nil +} +` + +func TestAssertApplyDelegatesToHelper_Canonical_NoDiagnostic(t *testing.T) { + diags := runAnalyzerOnSource(t, applyCanonicalSrc, AssertApplyDelegatesToHelper) + if len(diags) != 0 { + t.Errorf("canonical Apply should produce no diagnostic; got %d:\n%s", len(diags), diagSummary(diags)) + } +} + +func TestAssertApplyDelegatesToHelper_NonCanonical_Diagnoses(t *testing.T) { + diags := runAnalyzerOnSource(t, applyNonCanonicalSrc, AssertApplyDelegatesToHelper) + if len(diags) != 1 { + t.Fatalf("non-canonical Apply should produce 1 diagnostic; got %d:\n%s", len(diags), diagSummary(diags)) + } + if !strings.Contains(diags[0].Message, "wfctlhelpers.ApplyPlan") { + t.Errorf("diagnostic should reference wfctlhelpers.ApplyPlan; got %q", diags[0].Message) + } +} + +// ============================================================ +// AssertDiffSetsNeedsReplaceForForceNew +// ============================================================ + +// driverScaffold provides the Read companion method that +// driverLikeReceivers requires before AssertDiffSetsNeedsReplaceForForceNew +// will fire. Drivers conventionally have Read in addition to Diff. +const driverScaffold = `package p +import "context" + +type ResourceSpec struct{} +type ResourceOutput struct{} +type ResourceState struct{} +type FieldChange struct { + ForceNew bool +} +type DiffResult struct { + NeedsReplace bool + Changes []FieldChange +} + +type FooDriver struct{} + +func (d *FooDriver) Read(ctx context.Context, s ResourceState) (*ResourceOutput, error) { + return nil, nil +} +` + +const diffCanonicalSrc = driverScaffold + ` +func (d *FooDriver) Diff(ctx context.Context, desired ResourceSpec, current *ResourceOutput) (*DiffResult, error) { + r := &DiffResult{} + for _, c := range r.Changes { + if c.ForceNew { + r.NeedsReplace = true + } + } + return r, nil +} +` + +const diffMissingNeedsReplaceSrc = driverScaffold + ` +func (d *FooDriver) Diff(ctx context.Context, desired ResourceSpec, current *ResourceOutput) (*DiffResult, error) { + r := &DiffResult{} + for _, c := range r.Changes { + if c.ForceNew { + // Forgot to set NeedsReplace=true — this is the bug the analyzer flags. + _ = c + } + } + return r, nil +} +` + +func TestAssertDiffSetsNeedsReplaceForForceNew_Canonical_NoDiagnostic(t *testing.T) { + diags := runAnalyzerOnSource(t, diffCanonicalSrc, AssertDiffSetsNeedsReplaceForForceNew) + if len(diags) != 0 { + t.Errorf("canonical Diff should produce no diagnostic; got %d:\n%s", len(diags), diagSummary(diags)) + } +} + +func TestAssertDiffSetsNeedsReplaceForForceNew_MissingAssign_Diagnoses(t *testing.T) { + diags := runAnalyzerOnSource(t, diffMissingNeedsReplaceSrc, AssertDiffSetsNeedsReplaceForForceNew) + if len(diags) != 1 { + t.Fatalf("Diff that references ForceNew but never assigns NeedsReplace should produce 1 diagnostic; got %d:\n%s", len(diags), diagSummary(diags)) + } + if !strings.Contains(diags[0].Message, "NeedsReplace") { + t.Errorf("diagnostic should reference NeedsReplace; got %q", diags[0].Message) + } +} + +// TestAssertDiffSetsNeedsReplaceForForceNew_AcceptsDirectAssign pins +// review finding #4: the alternate canonical pattern `r.NeedsReplace = +// c.ForceNew` (instead of `if c.ForceNew { r.NeedsReplace = true }`) +// also satisfies the W-3 force-new contract and must NOT trigger a +// false-positive diagnostic. +const diffDirectAssignSrc = `package p +import "context" + +type ResourceSpec struct{} +type ResourceOutput struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type FieldChange struct { + ForceNew bool +} +type DiffResult struct { + NeedsReplace bool + Changes []FieldChange +} + +type FooDriver struct{} + +func (d *FooDriver) Read(ctx context.Context, s ResourceState) (*ResourceOutput, error) { + return nil, nil +} + +func (d *FooDriver) Diff(ctx context.Context, desired ResourceSpec, current *ResourceOutput) (*DiffResult, error) { + r := &DiffResult{} + for _, c := range r.Changes { + r.NeedsReplace = c.ForceNew + } + return r, nil +} +` + +func TestAssertDiffSetsNeedsReplaceForForceNew_AcceptsDirectAssign(t *testing.T) { + diags := runAnalyzerOnSource(t, diffDirectAssignSrc, AssertDiffSetsNeedsReplaceForForceNew) + if len(diags) != 0 { + t.Errorf("`r.NeedsReplace = c.ForceNew` is a valid alternate canonical; should NOT flag; got %d:\n%s", len(diags), diagSummary(diags)) + } +} + +// TestAssertDiffSetsNeedsReplaceForForceNew_RejectsLiteralFalseAssign +// — review round-2 follow-up B. The widened bodyAssignsField (any RHS) +// would silently accept `r.NeedsReplace = false` inside a ForceNew +// branch — a real copy-paste bug pattern. The matcher must specifically +// treat literal-`false` RHS as no-assignment so this typo is still +// flagged. +const diffLiteralFalseSrc = driverScaffold + ` +func (d *FooDriver) Diff(ctx context.Context, desired ResourceSpec, current *ResourceOutput) (*DiffResult, error) { + r := &DiffResult{} + for _, c := range r.Changes { + if c.ForceNew { + r.NeedsReplace = false + } + } + return r, nil +} +` + +func TestAssertDiffSetsNeedsReplaceForForceNew_RejectsLiteralFalseAssign(t *testing.T) { + diags := runAnalyzerOnSource(t, diffLiteralFalseSrc, AssertDiffSetsNeedsReplaceForForceNew) + if len(diags) != 1 { + t.Errorf("`r.NeedsReplace = false` is a copy-paste bug — analyzer must flag; got %d:\n%s", len(diags), diagSummary(diags)) + } +} + +// TestAssertDiffSetsNeedsReplaceForForceNew_NonDriverNotFlagged pins +// review finding #3: the analyzer must NOT fire on types that have a +// method named Diff but are not resource drivers (no Read / Create / +// Update / Delete companion methods). Adversarially: a `func (s +// *Settings) Diff(...)` that happens to match the arity should be +// invisible to this analyzer. +const diffNonDriverSrc = `package p +import "context" + +type ResourceSpec struct{} +type ResourceOutput struct{} +type FieldChange struct { + ForceNew bool +} +type DiffResult struct { + NeedsReplace bool + Changes []FieldChange +} + +// Not a driver — no Read/Create/Update/Delete. Just a settings struct +// that exposes a "Diff" method for unrelated reasons (e.g. config diff). +type SettingsDiff struct{} + +func (s *SettingsDiff) Diff(ctx context.Context, desired ResourceSpec, current *ResourceOutput) (*DiffResult, error) { + r := &DiffResult{} + for _, c := range r.Changes { + if c.ForceNew { + // No NeedsReplace assign — but this isn't a driver, so we + // should not flag. + _ = c + } + } + return r, nil +} +` + +func TestAssertDiffSetsNeedsReplaceForForceNew_NonDriverNotFlagged(t *testing.T) { + diags := runAnalyzerOnSource(t, diffNonDriverSrc, AssertDiffSetsNeedsReplaceForForceNew) + if len(diags) != 0 { + t.Errorf("type with Diff() but no driver-companion method (Read/Create/Update/Delete) should NOT be flagged; got %d:\n%s", len(diags), diagSummary(diags)) + } +} + +// Refresh diffCanonicalSrc to include a Read companion method so it +// passes the new precision filter (provider/driver heuristic). +// (The constant is replaced via Edit after the analyzer is updated; +// this comment is an intent marker only — see lint_test.go body.) + +// ============================================================ +// AssertProviderImplementsValidatePlan +// ============================================================ + +const providerWithValidatePlanSrc = `package p +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type FooProvider struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return nil, nil +} +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return nil, nil +} +func (p *FooProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { + return nil +} +` + +const providerWithoutValidatePlanSrc = `package p +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} + +type FooProvider struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return nil, nil +} +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return nil, nil +} +` + +func TestAssertProviderImplementsValidatePlan_HasValidatePlan_NoDiagnostic(t *testing.T) { + diags := runAnalyzerOnSource(t, providerWithValidatePlanSrc, AssertProviderImplementsValidatePlan) + if len(diags) != 0 { + t.Errorf("provider with ValidatePlan should produce no diagnostic; got %d:\n%s", len(diags), diagSummary(diags)) + } +} + +func TestAssertProviderImplementsValidatePlan_Missing_Diagnoses(t *testing.T) { + diags := runAnalyzerOnSource(t, providerWithoutValidatePlanSrc, AssertProviderImplementsValidatePlan) + if len(diags) != 1 { + t.Fatalf("provider without ValidatePlan should produce 1 diagnostic; got %d:\n%s", len(diags), diagSummary(diags)) + } + if !strings.Contains(diags[0].Message, "ValidatePlan") { + t.Errorf("diagnostic should reference ValidatePlan; got %q", diags[0].Message) + } +} + +// ============================================================ +// runLint dispatcher (integration) +// ============================================================ + +// writeTempPackage writes a single-package set of files to a tempdir +// and returns the dir. +func writeTempPackage(t *testing.T, files map[string]string) string { + t.Helper() + dir := t.TempDir() + for name, content := range files { + full := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", filepath.Dir(full), err) + } + if err := os.WriteFile(full, []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + } + return dir +} + +// runLintInDir invokes runLint against dir with the given Options and +// returns stdout, stderr, exit code. +func runLintInDir(t *testing.T, dir string, opts Options) (string, string, int) { + t.Helper() + var stdout, stderr bytes.Buffer + code := runLint([]string{dir}, &opts, &stdout, &stderr) + return stdout.String(), stderr.String(), code +} + +func TestRunLint_NoArgs_Exits2(t *testing.T) { + var stdout, stderr bytes.Buffer + code := runLint(nil, &Options{DryRun: true}, &stdout, &stderr) + if code != 2 { + t.Errorf("exit = %d, want 2", code) + } +} + +func TestRunLint_CanonicalSource_NoFindings(t *testing.T) { + dir := writeTempPackage(t, map[string]string{ + "provider.go": planCanonicalSrc, + }) + stdout, _, code := runLintInDir(t, dir, Options{DryRun: true}) + if code != 0 { + t.Errorf("exit = %d, want 0; stdout=%s", code, stdout) + } + if strings.Contains(stdout, "AssertPlanDelegatesToHelper") { + t.Errorf("canonical source should not be flagged; stdout:\n%s", stdout) + } +} + +func TestRunLint_NonCanonical_FindingsPresent(t *testing.T) { + dir := writeTempPackage(t, map[string]string{ + "provider.go": planNonCanonicalSrc, + }) + stdout, _, code := runLintInDir(t, dir, Options{DryRun: true}) + if code != 1 { + t.Errorf("exit = %d, want 1 (findings present); stdout=%s", code, stdout) + } + if !strings.Contains(stdout, "AssertPlanDelegatesToHelper") { + t.Errorf("expected analyzer name in report; stdout:\n%s", stdout) + } + if !strings.Contains(stdout, "provider.go") { + t.Errorf("expected file path in report; stdout:\n%s", stdout) + } +} + +func TestRunLint_SkipMarker_SurfacedInReport(t *testing.T) { + dir := writeTempPackage(t, map[string]string{ + "provider.go": planSkippedSrc, + }) + stdout, _, code := runLintInDir(t, dir, Options{DryRun: true}) + if code != 0 { + t.Errorf("exit = %d, want 0 (skipped, no findings); stdout=%s", code, stdout) + } + if !strings.Contains(stdout, "Skipped") { + t.Errorf("report must surface skipped sites; stdout:\n%s", stdout) + } + if !strings.Contains(stdout, "provider.go") { + t.Errorf("skipped section must include file path; stdout:\n%s", stdout) + } +} + +// TestRunLint_DoesNotMutateFilesEvenWithFixFlag pins the contract from +// carry-forward #2: lint is read-only by definition. Even with -fix and +// -dry-run=false, file mtimes and contents must be unchanged across the +// run. (Fix=true cannot reach this code path through the dispatcher +// because run() in main.go normalizes the gate, but the in-mode contract +// is also pinned for defense-in-depth.) +func TestRunLint_DoesNotMutateFilesEvenWithFixFlag(t *testing.T) { + dir := writeTempPackage(t, map[string]string{ + "provider.go": planNonCanonicalSrc, + }) + target := filepath.Join(dir, "provider.go") + + beforeStat, err := os.Stat(target) + if err != nil { + t.Fatalf("stat before: %v", err) + } + beforeContent, err := os.ReadFile(target) + if err != nil { + t.Fatalf("read before: %v", err) + } + + // Hostile flags: simulate a caller bypassing the dispatcher's gate. + _, _, _ = runLintInDir(t, dir, Options{Fix: true, DryRun: false}) + + afterStat, err := os.Stat(target) + if err != nil { + t.Fatalf("stat after: %v", err) + } + afterContent, err := os.ReadFile(target) + if err != nil { + t.Fatalf("read after: %v", err) + } + + if !beforeStat.ModTime().Equal(afterStat.ModTime()) { + t.Errorf("mtime changed: before=%v, after=%v — lint must never mutate", beforeStat.ModTime(), afterStat.ModTime()) + } + if !bytes.Equal(beforeContent, afterContent) { + t.Errorf("content changed — lint must never mutate") + } +} + +func TestRunLint_FixFlag_WarnsItHasNoEffect(t *testing.T) { + dir := writeTempPackage(t, map[string]string{ + "provider.go": planCanonicalSrc, + }) + _, stderr, _ := runLintInDir(t, dir, Options{Fix: true, DryRun: false}) + if !strings.Contains(stderr, "no effect") { + t.Errorf("stderr should warn that -fix has no effect on lint; got:\n%s", stderr) + } +} + +func TestRunLint_AnalyzerCount_FourRegistered(t *testing.T) { + if len(lintAnalyzers) != 4 { + t.Errorf("plan §T8.2 mandates 4 analyzers; got %d", len(lintAnalyzers)) + } + want := []string{ + "AssertPlanDelegatesToHelper", + "AssertApplyDelegatesToHelper", + "AssertDiffSetsNeedsReplaceForForceNew", + "AssertProviderImplementsValidatePlan", + } + got := make(map[string]bool) + for _, a := range lintAnalyzers { + got[a.Name] = true + } + for _, name := range want { + if !got[name] { + t.Errorf("plan-literal analyzer %q is missing from lintAnalyzers", name) + } + } +} + +func TestRunLint_RegistersIntoModesMap(t *testing.T) { + fn, ok := modes["lint"] + if !ok { + t.Fatalf("lint init() must register runLint into modes map") + } + if fn == nil { + t.Fatalf("modes[\"lint\"] is nil") + } +} + +// diagSummary formats a slice of diagnostics for test failure messages. +func diagSummary(diags []analysis.Diagnostic) string { + if len(diags) == 0 { + return " (none)" + } + var sb strings.Builder + for i, d := range diags { + fmt.Fprintf(&sb, " [%d] pos=%d: %s\n", i, d.Pos, d.Message) + } + return sb.String() +} diff --git a/cmd/iac-codemod/main.go b/cmd/iac-codemod/main.go new file mode 100644 index 00000000..b7fc9f7f --- /dev/null +++ b/cmd/iac-codemod/main.go @@ -0,0 +1,207 @@ +// Copyright (c) 2026 Jon Langevin +// SPDX-License-Identifier: Apache-2.0 + +// Command iac-codemod is an AST-based migration tool for IaC plugin providers. +// +// Modes: +// +// refactor-plan — rewrite Plan() bodies to delegate to platform.ComputePlan +// refactor-apply — rewrite Apply() bodies to delegate to wfctlhelpers.ApplyPlan +// (with informative reports for non-canonical idioms) +// add-validate-plan — inject a no-op ValidatePlan stub on providers missing it +// lint — static checks (no rewrite); advisory-only mode +// +// All modes default to dry-run. Pass -fix to opt into mutation. +// +// All modes honor the `// wfctl:skip-iac-codemod` marker on functions and +// types; skipped sites are surfaced in each mode's report. +package main + +import ( + "fmt" + "io" + "os" + "strings" +) + +// SkipMarker is the single canonical comment that opts a function or type +// declaration out of every iac-codemod mode (refactor-plan, refactor-apply, +// add-validate-plan, lint). Plan rev2 (line 2400) unifies the four modes +// on this marker specifically to prevent mismatched-marker silent-no-op +// surfaces (e.g. // wfctl:skip-codemod or // wfctl:skip-plan-codemod). All +// downstream parsers (T8.3-T8.5) MUST reference this constant rather than +// the literal string, and each mode surfaces a list of skipped sites in +// its report. +const SkipMarker = "// wfctl:skip-iac-codemod" + +// Options carries flags shared by every codemod mode. +// +// Mode implementations MUST treat Fix as the sole authority for mutation. +// DryRun is mirrored as `!Fix` purely for ergonomic reading of report +// preambles and is normalized by run() at the dispatcher boundary so a +// user-supplied -dry-run=false cannot bypass the explicit -fix gate +// (plan §W-8 line 2347: "-dry-run flag default true; -fix opts into +// mutation"). Predicates like `if !opts.DryRun { mutate() }` are safe +// because the dispatcher guarantees DryRun==true whenever Fix==false. +type Options struct { + // DryRun reports findings without mutating files. Forced true when + // Fix is false; forced false when Fix is true. The user's + // -dry-run= value is informational once dispatcher normalization + // runs. + DryRun bool + // Fix opts into mutation. Sole authority for mutation gating. + Fix bool +} + +// modeFunc is the entry point for one of the codemod's subcommand modes. +// args is the residual positional argument list (target paths, etc.) after +// shared flags have been parsed off. Returns a process exit code. +type modeFunc func(args []string, opts *Options, stdout, stderr io.Writer) int + +// modes registers every supported subcommand. Tests swap entries in this +// map to capture the parsed Options without spawning a subprocess. +var modes = map[string]modeFunc{ + "refactor-plan": stubMode("refactor-plan"), + "refactor-apply": stubMode("refactor-apply"), + "add-validate-plan": stubMode("add-validate-plan"), + "lint": stubMode("lint"), +} + +// stubMode returns a placeholder modeFunc used by the T8.1 skeleton. +// Subsequent tasks (T8.2 lint, T8.3 refactor-plan, T8.4 refactor-apply, +// T8.5 add-validate-plan) replace these entries with real implementations +// in the package's init() inside their own files. +func stubMode(name string) modeFunc { + return func(_ []string, _ *Options, stdout, _ io.Writer) int { + fmt.Fprintf(stdout, "iac-codemod %s: not yet implemented (skeleton stub)\n", name) + return 0 + } +} + +func main() { + os.Exit(run(os.Args[1:], os.Stdout, os.Stderr)) +} + +// run is the testable entry point. Returns the desired process exit code. +func run(args []string, stdout, stderr io.Writer) int { + if len(args) == 0 { + usage(stderr) + return 2 + } + switch args[0] { + case "-h", "--help", "help": + usage(stdout) + return 0 + } + + mode := args[0] + rest := args[1:] + fn, ok := modes[mode] + if !ok { + fmt.Fprintf(stderr, "iac-codemod: unknown mode: %s\n\n", mode) + usage(stderr) + return 2 + } + + // Round-12 #1: rev1 used a single FlagSet with only `-dry-run` + // and `-fix` registered, so any mode-specific flag (e.g. + // `-report-file` for refactor-apply) failed with + // "flag provided but not defined" BEFORE the mode could parse + // it. Now we manually extract the two shared flags from the + // argument list, leaving the rest (including unknown-to-dispatcher + // flags) intact for the mode's own FlagSet. Manual extraction is + // preferred over flag.NewFlagSet's `flag.ContinueOnError` because + // stdlib's parser stops at the first unknown flag and consumes + // nothing further — manual lets us preserve EVERYTHING the mode + // needs. + opts := &Options{} + residual := []string{} + for i := 0; i < len(rest); i++ { + arg := rest[i] + switch arg { + case "-h", "--help": + usage(stdout) + return 0 + case "-dry-run", "--dry-run": + opts.DryRun = true + case "-dry-run=true", "--dry-run=true": + opts.DryRun = true + case "-dry-run=false", "--dry-run=false": + opts.DryRun = false + case "-fix", "--fix": + opts.Fix = true + case "-fix=true", "--fix=true": + opts.Fix = true + case "-fix=false", "--fix=false": + opts.Fix = false + default: + residual = append(residual, arg) + } + } + // Normalize the mutation gate at the dispatcher boundary: Fix is the + // sole authority for "may I mutate?". A user-supplied -dry-run=false + // without -fix must NOT bypass the gate (plan §W-8 line 2347), and + // -fix must override an explicit -dry-run=true. + if opts.Fix { + opts.DryRun = false + } else { + opts.DryRun = true + } + return fn(residual, opts, stdout, stderr) +} + +// shouldSkipDir is the canonical directory-walk filter shared by every +// mode's filepath.WalkDir callback. It excludes: +// +// - "vendor" — the standard Go vendor tree; mirrors `go build`'s +// behavior of treating vendor/ as a private dependency island. +// - "testdata" — by convention not real source. +// - hidden directories (prefix ".", except the literal "."): .git, +// .idea, .vscode, etc. +// - underscore-prefix directories (prefix "_", except the literal +// "_"): Go tooling itself ignores these (cmd/go skips package paths +// starting with underscore). The DigitalOcean plugin uses +// `_worktrees/` for parallel feature branches; without this filter +// a single lint run reports the same site dozens of times across +// stale checkouts. +func shouldSkipDir(base string) bool { + switch base { + case "vendor", "testdata": + return true + } + if len(base) > 1 && (strings.HasPrefix(base, ".") || strings.HasPrefix(base, "_")) { + return true + } + return false +} + +func usage(w io.Writer) { + fmt.Fprintf(w, `usage: iac-codemod [flags] [paths...] + +Modes: + refactor-plan Rewrite Plan() bodies to delegate to platform.ComputePlan. + refactor-apply Rewrite Apply() bodies to delegate to wfctlhelpers.ApplyPlan + (with informative reports for non-canonical idioms). + add-validate-plan Insert a no-op ValidatePlan stub on providers missing it. + lint Run static checks; no rewrite. Advisory-only. + +Flags (all modes): + -dry-run Report findings without mutating files (default true). + -fix Opt into mutation; overrides -dry-run. + +Mode-specific flags: + refactor-apply: + -report-file Also write the Markdown report to . Default + is stdout-only. + + Flags may appear anywhere on the command line (round-12 #1: the + dispatcher uses a manual flag scan instead of stdlib flag, so + positional-then-flag ordering is supported). Mode-specific flags + (e.g. -report-file) are passed through to the mode's own parser. + +Marker: + Functions and type declarations annotated with the comment + %s + are skipped by every mode and surfaced in each mode's report. +`, SkipMarker) +} diff --git a/cmd/iac-codemod/main_test.go b/cmd/iac-codemod/main_test.go new file mode 100644 index 00000000..7b42db16 --- /dev/null +++ b/cmd/iac-codemod/main_test.go @@ -0,0 +1,331 @@ +// Copyright (c) 2026 Jon Langevin +// SPDX-License-Identifier: Apache-2.0 + +// Tests in this file MUST NOT call t.Parallel(). The package-global +// `modes` map is mutated and restored under defer (see captureMode, +// TestRun_FlagAfterPath_SilentlyTreatedAsPositional, +// TestRun_PositionalArgsForwardedToMode); concurrent test goroutines +// would race on the same map and -race would catch it only at high +// concurrency. If a future T8.x test needs parallelism, refactor `run` +// to take the mode map as a parameter (dependency injection) so each +// test can build a local map per-test. + +package main + +import ( + "bytes" + "io" + "os" + "strings" + "testing" +) + +// captureMode swaps modes[name] for a recorder that captures the Options +// it was invoked with. Returns a teardown func and a pointer to the +// captured Options (nil until the mode actually runs). +func captureMode(t *testing.T, name string) (*Options, func()) { + t.Helper() + orig, ok := modes[name] + if !ok { + t.Fatalf("captureMode: unknown mode %q", name) + } + captured := &Options{} + called := false + modes[name] = func(args []string, opts *Options, stdout, stderr io.Writer) int { + *captured = *opts + called = true + _ = args + _ = stdout + _ = stderr + return 0 + } + return captured, func() { + modes[name] = orig + if !called { + t.Errorf("captureMode(%q): mode never invoked", name) + } + } +} + +func TestRun_NoArgs_ExitsWithUsage(t *testing.T) { + var stdout, stderr bytes.Buffer + code := run(nil, &stdout, &stderr) + if code != 2 { + t.Errorf("exit code = %d, want 2", code) + } + combined := stdout.String() + stderr.String() + if !strings.Contains(combined, "usage:") { + t.Errorf("expected usage in output; got stdout=%q stderr=%q", stdout.String(), stderr.String()) + } + for _, mode := range []string{"refactor-plan", "refactor-apply", "add-validate-plan", "lint"} { + if !strings.Contains(combined, mode) { + t.Errorf("usage should list mode %q; got %q", mode, combined) + } + } +} + +func TestRun_HelpFlag_ExitsZero(t *testing.T) { + for _, flag := range []string{"-h", "--help", "help"} { + t.Run(flag, func(t *testing.T) { + var stdout, stderr bytes.Buffer + code := run([]string{flag}, &stdout, &stderr) + if code != 0 { + t.Errorf("exit code = %d, want 0", code) + } + if !strings.Contains(stdout.String()+stderr.String(), "usage:") { + t.Errorf("expected usage in output; got stdout=%q stderr=%q", stdout.String(), stderr.String()) + } + }) + } +} + +func TestRun_UnknownMode_Exits2(t *testing.T) { + var stdout, stderr bytes.Buffer + code := run([]string{"frobnicate"}, &stdout, &stderr) + if code != 2 { + t.Errorf("exit code = %d, want 2", code) + } + if !strings.Contains(stderr.String(), "unknown mode") { + t.Errorf("expected 'unknown mode' in stderr; got %q", stderr.String()) + } + if !strings.Contains(stderr.String(), "frobnicate") { + t.Errorf("expected unknown mode name in stderr; got %q", stderr.String()) + } +} + +func TestRun_KnownModes_DispatchToHandlers(t *testing.T) { + for _, mode := range []string{"refactor-plan", "refactor-apply", "add-validate-plan", "lint"} { + t.Run(mode, func(t *testing.T) { + opts, teardown := captureMode(t, mode) + defer teardown() + var stdout, stderr bytes.Buffer + code := run([]string{mode}, &stdout, &stderr) + if code != 0 { + t.Errorf("exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + if !opts.DryRun { + t.Errorf("DryRun should default to true") + } + if opts.Fix { + t.Errorf("Fix should default to false") + } + }) + } +} + +func TestRun_DryRunDefaultsTrue(t *testing.T) { + opts, teardown := captureMode(t, "lint") + defer teardown() + var stdout, stderr bytes.Buffer + if code := run([]string{"lint"}, &stdout, &stderr); code != 0 { + t.Fatalf("exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + if !opts.DryRun { + t.Errorf("DryRun should default to true; got false") + } +} + +func TestRun_FixOptsIntoMutation(t *testing.T) { + opts, teardown := captureMode(t, "refactor-plan") + defer teardown() + var stdout, stderr bytes.Buffer + if code := run([]string{"refactor-plan", "-fix"}, &stdout, &stderr); code != 0 { + t.Fatalf("exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + if !opts.Fix { + t.Errorf("Fix should be true when -fix passed") + } + if opts.DryRun { + t.Errorf("DryRun should be false when -fix passed (mutation opt-in)") + } +} + +// TestRun_HelpAfterMode_PrintsGlobalUsageToStdout pins T8.2 carry-forward +// #1 (and review round-2 finding #1): `iac-codemod -h` must +// produce the same structured output as `iac-codemod -h` — including +// the destination stream. Per kubectl / git / gh convention, -h on +// success goes to stdout; the test asserts stream specifically so a +// regression to stderr cannot pass via a string-union check. +func TestRun_HelpAfterMode_PrintsGlobalUsageToStdout(t *testing.T) { + for _, mode := range []string{"refactor-plan", "refactor-apply", "add-validate-plan", "lint"} { + t.Run(mode, func(t *testing.T) { + var stdout, stderr bytes.Buffer + code := run([]string{mode, "-h"}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + // Global usage on -h must land on STDOUT, not stderr. + for _, modeName := range []string{"refactor-plan", "refactor-apply", "add-validate-plan", "lint"} { + if !strings.Contains(stdout.String(), modeName) { + t.Errorf("global usage on -h must go to stdout (matching `iac-codemod -h`); mode %q missing from stdout=%q (stderr=%q)", modeName, stdout.String(), stderr.String()) + } + } + }) + } +} + +// TestRun_DryRunFalseWithoutFix_StillForcesDryRun pins the mutation-gate +// contract from plan §W-8 line 2347: "-dry-run flag default true; -fix opts +// into mutation". Fix must be the SINGLE source of truth for "may I +// mutate?" — if a user passes -dry-run=false without -fix, the dispatcher +// must reassert DryRun=true so T8.2-T8.5 modes that naturally check +// !opts.DryRun cannot be tricked into a silent rewrite. +func TestRun_DryRunFalseWithoutFix_StillForcesDryRun(t *testing.T) { + opts, teardown := captureMode(t, "lint") + defer teardown() + var stdout, stderr bytes.Buffer + if code := run([]string{"lint", "-dry-run=false"}, &stdout, &stderr); code != 0 { + t.Fatalf("exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + if !opts.DryRun { + t.Errorf("DryRun must remain true without -fix; -dry-run=false alone must NOT bypass the mutation gate (plan §W-8 line 2347)") + } + if opts.Fix { + t.Errorf("Fix should remain false; got true") + } +} + +// TestRun_FixWithDryRunFalse_MutationStillAuthorized covers the redundant +// but legal case: -fix wins regardless of -dry-run's user-supplied value. +func TestRun_FixWithDryRunFalse_MutationStillAuthorized(t *testing.T) { + opts, teardown := captureMode(t, "refactor-plan") + defer teardown() + var stdout, stderr bytes.Buffer + if code := run([]string{"refactor-plan", "-fix", "-dry-run=false"}, &stdout, &stderr); code != 0 { + t.Fatalf("exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + if !opts.Fix { + t.Errorf("Fix should be true") + } + if opts.DryRun { + t.Errorf("DryRun should be false when -fix is set") + } +} + +// TestRun_FixWithExplicitDryRunTrue_FixWins covers the inverse: -fix wins +// over a user-supplied -dry-run=true. -fix is the single mutation gate; +// -dry-run is informational once -fix is set. +func TestRun_FixWithExplicitDryRunTrue_FixWins(t *testing.T) { + opts, teardown := captureMode(t, "refactor-apply") + defer teardown() + var stdout, stderr bytes.Buffer + if code := run([]string{"refactor-apply", "-dry-run=true", "-fix"}, &stdout, &stderr); code != 0 { + t.Fatalf("exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + if !opts.Fix { + t.Errorf("Fix should be true") + } + if opts.DryRun { + t.Errorf("DryRun should be false; -fix overrides explicit -dry-run=true") + } +} + +// TestPackageDoc_MentionsSkipMarker is documentation-only insurance that +// the package doc comment in main.go does not silently desync from the +// SkipMarker const. godoc is human-read, not parser-read, so this is +// belt-and-suspenders against a future rename. +func TestPackageDoc_MentionsSkipMarker(t *testing.T) { + src, err := os.ReadFile("main.go") + if err != nil { + t.Fatalf("read main.go: %v", err) + } + if !strings.Contains(string(src), SkipMarker) { + t.Errorf("main.go must reference SkipMarker literal %q somewhere; package doc may have desynced", SkipMarker) + } +} + +// TestSkipMarker_LiteralPinned guards against drift in the canonical marker +// string. Plan rev2 (line 2400) unifies all four modes on a single marker +// specifically to prevent mismatched-marker silent-no-op surfaces. T8.3-T8.5 +// will import SkipMarker for actual parsing; pinning the literal here means +// any rename or typo trips this test before it reaches a mode parser. +func TestSkipMarker_LiteralPinned(t *testing.T) { + const want = "// wfctl:skip-iac-codemod" + if SkipMarker != want { + t.Errorf("canonical marker drift: SkipMarker = %q, want %q", SkipMarker, want) + } +} + +func TestUsage_MentionsSkipMarker(t *testing.T) { + var buf bytes.Buffer + usage(&buf) + if !strings.Contains(buf.String(), SkipMarker) { + t.Errorf("usage must mention canonical marker %q; got:\n%s", SkipMarker, buf.String()) + } +} + +// TestUsage_DocumentsFlagOrderingFlexibility — round-12 #1: usage now +// documents the position-independent flag handling (was: documented a +// stdlib limitation that the manual-scan dispatcher no longer has). +func TestUsage_DocumentsFlagOrderingFlexibility(t *testing.T) { + var buf bytes.Buffer + usage(&buf) + if !strings.Contains(buf.String(), "Flags may appear anywhere") { + t.Errorf("usage must document flag-position flexibility; got:\n%s", buf.String()) + } +} + +// TestRun_FlagAfterPath_RecognizedByDispatcher pins the round-12 #1 +// fix: the manual scan in run() now recognises -dry-run/-fix anywhere +// in the argument list, including after positional args. Previous +// behavior (stdlib flag-pkg stopping at the first non-flag) was +// surprising and made the documented `-report-file` mode flag +// unusable from the CLI entrypoint. The post-round-12 behavior is +// position-independent for the dispatcher's two flags. +func TestRun_FlagAfterPath_RecognizedByDispatcher(t *testing.T) { + var capturedOpts Options + var capturedArgs []string + orig := modes["refactor-plan"] + modes["refactor-plan"] = func(args []string, opts *Options, stdout, stderr io.Writer) int { + capturedOpts = *opts + capturedArgs = append([]string{}, args...) + return 0 + } + defer func() { modes["refactor-plan"] = orig }() + + var stdout, stderr bytes.Buffer + if code := run([]string{"refactor-plan", "/path", "-fix"}, &stdout, &stderr); code != 0 { + t.Fatalf("exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + if !capturedOpts.Fix { + t.Errorf("Fix should be true when -fix appears AFTER positional (manual-scan dispatcher recognises flags anywhere); got Fix=false") + } + if capturedOpts.DryRun { + t.Errorf("DryRun should be false when -fix is set; got true") + } + // Mode receives only the positional path; -fix was consumed by + // the dispatcher. + wantArgs := []string{"/path"} + if len(capturedArgs) != len(wantArgs) { + t.Fatalf("got args %v, want %v", capturedArgs, wantArgs) + } + for i := range wantArgs { + if capturedArgs[i] != wantArgs[i] { + t.Errorf("arg[%d] = %q, want %q", i, capturedArgs[i], wantArgs[i]) + } + } +} + +func TestRun_PositionalArgsForwardedToMode(t *testing.T) { + var gotArgs []string + orig := modes["lint"] + modes["lint"] = func(args []string, opts *Options, stdout, stderr io.Writer) int { + gotArgs = append([]string{}, args...) + return 0 + } + defer func() { modes["lint"] = orig }() + + var stdout, stderr bytes.Buffer + if code := run([]string{"lint", "-dry-run", "/path/to/plugin", "/another/path"}, &stdout, &stderr); code != 0 { + t.Fatalf("exit code = %d, want 0; stderr=%q", code, stderr.String()) + } + wantArgs := []string{"/path/to/plugin", "/another/path"} + if len(gotArgs) != len(wantArgs) { + t.Fatalf("got args %v, want %v", gotArgs, wantArgs) + } + for i := range wantArgs { + if gotArgs[i] != wantArgs[i] { + t.Errorf("arg[%d] = %q, want %q", i, gotArgs[i], wantArgs[i]) + } + } +} diff --git a/cmd/iac-codemod/refactor_apply.go b/cmd/iac-codemod/refactor_apply.go new file mode 100644 index 00000000..3a1d9301 --- /dev/null +++ b/cmd/iac-codemod/refactor_apply.go @@ -0,0 +1,1352 @@ +// Copyright (c) 2026 Jon Langevin +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "flag" + "fmt" + "go/ast" + "go/parser" + "go/token" + "io" + "io/fs" + "os" + "path/filepath" + "sort" + "strings" +) + +func init() { + modes["refactor-apply"] = runRefactorApply +} + +// applyCanonicalCallExpr is the canonical replacement-body expression +// emitted by refactor-apply. +const applyCanonicalCallExpr = "wfctlhelpers.ApplyPlan(ctx, p, plan)" + +// applyClassification labels the disposition of a single Apply() +// method site. The non-canonical idioms are surfaced as distinct +// classes so the report can suggest the right hand-port handling. +type applyClassification int + +const ( + applyCanonical applyClassification = iota + applyAlreadyDelegated + applySkipped + // Non-canonical idioms (each with its own suggested handling): + applyUpsertRecovery // DO upsert-on-create-conflict — emit upsertSupporter hook patch + applyUpdateReplaceCollapse // AWS `case "update", "replace":` — emit "manual port required" + applyCustomErrorWrapping // custom fmt.Errorf wrapping — emit extension-point hook + sample + applyNonCanonicalOther // some other shape we don't recognise + applyMissingSwitch // no switch-on-action; cannot mechanically rewrite +) + +func (c applyClassification) String() string { + switch c { + case applyCanonical: + return "canonical" + case applyAlreadyDelegated: + return "already-delegated" + case applySkipped: + return "skipped" + case applyUpsertRecovery: + return "upsert-recovery" + case applyUpdateReplaceCollapse: + return "update+replace-collapse" + case applyCustomErrorWrapping: + return "custom-error-wrapping" + case applyNonCanonicalOther: + return "non-canonical" + case applyMissingSwitch: + return "missing-action-switch" + default: + return "unknown" + } +} + +// applySite captures one Apply-method site in the report. +type applySite struct { + Path string + Line int + Receiver string + Class applyClassification + OffenderPos string // path:line of the offending construct (for collapse/wrap idioms) + Suggestion string // hand-port suggestion text + Rewrote bool +} + +// applyReport aggregates per-file results across an entire refactor-apply +// run. +type applyReport struct { + sites []applySite + errors []string +} + +// runRefactorApply is the entry point for the refactor-apply subcommand. +// Mode-local flags (currently `-report-file`) are parsed off `args` +// before path walking begins. +func runRefactorApply(args []string, opts *Options, stdout, stderr io.Writer) int { + fs := flag.NewFlagSet("iac-codemod refactor-apply", flag.ContinueOnError) + fs.SetOutput(stderr) + // Steer per-mode -h to stdout for symmetry with the top-level + // `iac-codemod -h` (T8.2 carry-forward #1). The dispatcher in main.go + // intercepts `-h` before it reaches this FlagSet, so this closure + // only fires on parse errors. Mode-specific flags (-report-file) + // are documented in main.go's global usage() text — that's the + // fix surface for review round-1 finding #11. + fs.Usage = func() { usage(stdout) } + reportFile := fs.String("report-file", "", "if set, also write the report (Markdown) to this path; default is stdout-only") + if err := fs.Parse(args); err != nil { + // flag.ContinueOnError already wrote a parse-error message via + // SetOutput(stderr); a -h returns ErrHelp which we surface as 0. + if err == flag.ErrHelp { + return 0 + } + return 2 + } + rest := fs.Args() + if len(rest) == 0 { + fmt.Fprintln(stderr, "iac-codemod refactor-apply: at least one path is required") + usage(stderr) + return 2 + } + report := &applyReport{} + for _, path := range rest { + if err := refactorApplyPath(path, opts, report); err != nil { + fmt.Fprintf(stderr, "iac-codemod refactor-apply: %s: %v\n", path, err) + return 1 + } + } + report.print(stdout, opts) + if *reportFile != "" { + var buf bytes.Buffer + report.print(&buf, opts) + if err := os.WriteFile(*reportFile, buf.Bytes(), 0o644); err != nil { + fmt.Fprintf(stderr, "iac-codemod refactor-apply: write report-file %s: %v\n", *reportFile, err) + return 1 + } + } + if len(report.errors) > 0 { + return 1 + } + return 0 +} + +// refactorApplyPath walks `path` for *.go files and processes each. +func refactorApplyPath(path string, opts *Options, report *applyReport) error { + info, err := stat(path) + if err != nil { + return err + } + if !info.IsDir() { + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return fmt.Errorf("not a Go source file (or is a _test.go): %s", path) + } + if err := refactorApplyFile(path, opts, report); err != nil { + report.errors = append(report.errors, fmt.Sprintf("%s: %v", path, err)) + } + return nil + } + return filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + base := d.Name() + if shouldSkipDir(base) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { + return nil + } + if err := refactorApplyFile(p, opts, report); err != nil { + report.errors = append(report.errors, fmt.Sprintf("%s: %v", p, err)) + } + return nil + }) +} + +// refactorApplyFile parses `path`, classifies every Apply method, and +// (in -fix mode) mutates canonical bodies in place. +func refactorApplyFile(path string, opts *Options, report *applyReport) error { + src, err := readFile(path) + if err != nil { + return err + } + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, src, parser.ParseComments) + if err != nil { + return err + } + + // Round-12 #3: skip files in a non-dominant package (same + // rationale as refactor-plan #2). + dominant := dominantPackageForDir(filepath.Dir(path)) + if dominant != "" && file.Name.Name != dominant { + return nil + } + // Directory-wide method set (review round-1 finding #9). + provs := planLikeReceiversInDir(filepath.Dir(path)) + if len(provs) == 0 { + provs = planLikeReceivers(file) + } + // Directory-wide type-doc lookup (review round-6 finding #1) so + // skip-marker on a sibling file's type declaration is honored. + typeDocs := receiverTypeDocsInDir(filepath.Dir(path), file) + + mutated := false + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + if !isProviderMethod(fn, "Apply", 2, 2) { + continue + } + recv := receiverTypeName(fn) + if !provs[recv] { + continue + } + // Honor SkipMarker on fn.Doc OR receiver-type docs (review + // round-1 finding #4). + if hasSkipMarkerOn(fn.Doc) || typeDocs[recv].carriesMarker() { + report.sites = append(report.sites, applySite{ + Path: path, + Line: fset.Position(fn.Pos()).Line, + Receiver: recv, + Class: applySkipped, + }) + continue + } + class, offenderPos, suggestion := classifyApplyBody(fn, file, fset, path) + site := applySite{ + Path: path, + Line: fset.Position(fn.Pos()).Line, + Receiver: recv, + Class: class, + OffenderPos: offenderPos, + Suggestion: suggestion, + } + if class == applyCanonical && opts != nil && opts.Fix { + rewriteApplyBody(fn, file) + mutated = true + site.Rewrote = true + } + report.sites = append(report.sites, site) + } + + if mutated && opts != nil && opts.Fix { + ensureWfctlhelpersImport(file) // refactor-apply emits wfctlhelpers.ApplyPlan + if err := writeFileAtomic(path, fset, file); err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + } + return nil +} + +// classifyApplyBody returns the disposition of fn's Apply body. If the +// body has any of the recognised non-canonical idioms, the offender's +// path:line and a hand-port suggestion are returned alongside the class +// label. The order of detection is intentional: the most-disruptive +// idiom (collapse) is reported first since it cannot be mechanically +// migrated, then upsert (which has a clean wfctlhelpers hook), then +// custom-error-wrapping. Multiple idioms in one body produce a single +// label; the report points at the first detected. +func classifyApplyBody(fn *ast.FuncDecl, file *ast.File, fset *token.FileSet, path string) (applyClassification, string, string) { + if fn.Body == nil { + return applyNonCanonicalOther, "", "" + } + if isAlreadyDelegatedApplyBody(fn.Body, file) { + return applyAlreadyDelegated, "", "" + } + sw := findActionSwitch(fn.Body) + if sw == nil { + return applyMissingSwitch, "", "Apply body has no `switch action.Action` dispatch — wfctlhelpers.ApplyPlan expects this loop+switch shape; hand-port required." + } + // AWS update+replace collapse: any case clause with both "update" + // and "replace" string literals. + if pos := findUpdateReplaceCollapseCase(sw); pos.IsValid() { + offender := fset.Position(pos) + return applyUpdateReplaceCollapse, fmtPosShort(path, offender.Line), "manual port required: split `case \"update\", \"replace\":` into separate `update` and `replace` clauses (or rely on wfctlhelpers.ApplyPlan's doReplace semantic). The collapsed shape silently treats Replace as Update which loses the delete+create semantic for force-new fields." + } + // DO upsert recovery: errors.Is(err, ErrResourceAlreadyExists). + if pos := findUpsertRecovery(sw); pos.IsValid() { + offender := fset.Position(pos) + return applyUpsertRecovery, fmtPosShort(path, offender.Line), "preserve via wfctlhelpers.ApplyPlan's upsertSupporter hook: drivers that support name-based discovery should implement `SupportsUpsert() bool` returning true; the helper handles ErrResourceAlreadyExists → Read+Update internally. Sample patch: keep the existing `upsertSupporter` interface declaration on the driver type, then delete the manual upsert branch from Apply." + } + // Custom error wrapping: a `case` body where err is reassigned via + // fmt.Errorf with %w wrapping after a driver call. + if pos := findCustomErrorWrap(sw); pos.IsValid() { + offender := fset.Position(pos) + return applyCustomErrorWrapping, fmtPosShort(path, offender.Line), "manual port required: wfctlhelpers.ApplyPlan does NOT expose a per-action error-wrap hook today (review round-1 finding #6: rev0 of this report named a fictional ApplyResultErrorHook / WrapActionError API). Two honest options: (a) preserve the domain-context wrap by adding `// wfctl:skip-iac-codemod` to the Apply method and keeping the manual switch; (b) move the wrap into the driver itself (Create/Update/Delete return the already-wrapped error) so wfctlhelpers' generic dispatcher records it verbatim. Option (b) is preferred because it survives any future migration." + } + // Heuristic: if the switch has the canonical create/update[/delete] + // triple (plus optional separate replace), no non-canonical idiom + // inside the switch, AND the surrounding Apply body matches the + // canonical scaffold (result-init + range-loop + return), treat as + // canonical. Round-5 finding #2: rev3 only verified the switch + // shape — setup/teardown/custom result aggregation OUTSIDE the + // switch was silently dropped on -fix. + // Extract receiver + plan parameter identifier names so the + // outer-shape and loop-body validators don't hardcode `p` / + // `result` / `plan` (round-8 #5 + #9: providers using `res` / + // `pl` / etc. were misclassified as non-canonical even though + // rewriteApplyBody preserves custom names). + recvName := "" + if fn.Recv != nil && len(fn.Recv.List) > 0 && len(fn.Recv.List[0].Names) > 0 { + recvName = fn.Recv.List[0].Names[0].Name + } + planName := "" + if fn.Type.Params != nil && len(fn.Type.Params.List) >= 2 && len(fn.Type.Params.List[1].Names) >= 1 { + planName = fn.Type.Params.List[1].Names[0].Name + } + if hasCanonicalCases(sw, recvName) && isCanonicalApplyOuterShape(fn.Body, recvName, planName) { + return applyCanonical, "", "" + } + return applyNonCanonicalOther, "", "Apply outer shape (result-init + range-loop + return) or switch has unrecognised statements; review manually." +} + +// isCanonicalApplyOuterShape returns true if fn.Body matches the +// canonical 3-statement scaffold around the action switch: +// +// 1. ` := &ApplyResult{...}` +// 2. `for _, action := range .Actions { ... }` +// 3. `return , nil` +// +// `recvName` is the receiver identifier (used by isCanonicalApplyLoopBody +// to validate the driver-lookup receiver is the provider). +// `planName` is the actual `plan` parameter name from the signature +// (round-8 #9: providers using `pl` etc. were misclassified). +// +// The accumulator-variable name is recovered from statement 1 and +// then required to match in statement 3, so any local convention +// (`result`, `res`, `out`) survives as long as it's consistent. +// +// Reject any deviation (extra setup, teardown, custom aggregation, +// trailing helper calls) so bespoke logic outside the switch is +// preserved as non-canonical (review round-5 #2). +func isCanonicalApplyOuterShape(body *ast.BlockStmt, recvName, planName string) bool { + if body == nil || len(body.List) != 3 { + return false + } + if planName == "" { + planName = "plan" + } + // 1. := &ApplyResult{...} — recover the local + // accumulator name so the canonical detector doesn't hardcode + // "result" (round-8 #9). + a, ok := body.List[0].(*ast.AssignStmt) + if !ok || a.Tok != token.DEFINE || len(a.Lhs) != 1 || len(a.Rhs) != 1 { + return false + } + resultIdent, ok := a.Lhs[0].(*ast.Ident) + if !ok { + return false + } + resultName := resultIdent.Name + if resultName == "" || resultName == "_" { + return false + } + un, ok := a.Rhs[0].(*ast.UnaryExpr) + if !ok || un.Op != token.AND { + return false + } + cl, ok := un.X.(*ast.CompositeLit) + if !ok { + return false + } + if !typeNameTailMatches(cl.Type, "ApplyResult") { + return false + } + // 2. for _, action := range .Actions { ... } + rng, ok := body.List[1].(*ast.RangeStmt) + if !ok { + return false + } + xSel, ok := rng.X.(*ast.SelectorExpr) + if !ok || xSel.Sel.Name != "Actions" { + return false + } + if planId, ok := xSel.X.(*ast.Ident); !ok || planId.Name != planName { + return false + } + // Round-7 #3 + #9: validate the loop body is one of the recognised + // canonical scaffolds. Pass the provider receiver name so the + // driver-lookup check (round-8 #5) verifies .ResourceDriver + // rather than accepting any selector. + if !isCanonicalApplyLoopBody(rng.Body, recvName, resultName) { + return false + } + // 3. return , nil + ret, ok := body.List[2].(*ast.ReturnStmt) + if !ok || len(ret.Results) != 2 { + return false + } + if id, ok := ret.Results[0].(*ast.Ident); !ok || id.Name != resultName { + return false + } + if id, ok := ret.Results[1].(*ast.Ident); !ok || id.Name != "nil" { + return false + } + return true +} + +// isCanonicalApplyLoopBody returns true if the for-loop body matches +// one of the canonical scaffolds. Round-7 #3 + #9: rev5 of +// isCanonicalApplyOuterShape only verified the outer 3 statements; +// any per-action logging/metrics/accumulators inside the for loop +// was silently dropped on -fix. +// +// Whitelist (every loop-body statement must match one of these): +// +// - SwitchStmt with tag `.Action` (the action dispatch). Exactly 1 +// such switch is required across the loop body. +// - DeclStmt: `var out *ResourceOutput` (or qualified equivalent). +// - AssignStmt: `, err := .ResourceDriver(...)` (driver lookup). +// - IfStmt: `if err != nil { result.Errors = append(...); continue }` +// OR `if out != nil { result.Resources = append(*out) }` +// +// Anything else (bare logging calls, metric increments, helper-call +// statements, alternate-driver lookup) rejects the canonical +// classification. +func isCanonicalApplyLoopBody(body *ast.BlockStmt, recvName, resultName string) bool { + if body == nil { + return false + } + switchCount := 0 + for _, stmt := range body.List { + switch s := stmt.(type) { + case *ast.SwitchStmt: + switchCount++ + // (the switch body itself is validated by hasCanonicalCases + // in classifyApplyBody before this function fires). + case *ast.DeclStmt: + if !isLocalOutPointerDecl(s) { + return false + } + case *ast.AssignStmt: + if !isCanonicalApplyLoopAssign(s, recvName) { + return false + } + case *ast.IfStmt: + if !isCanonicalApplyLoopIf(s, resultName) { + return false + } + default: + return false + } + } + return switchCount == 1 +} + +// isCanonicalApplyLoopAssign returns true for the canonical loop-body +// AssignStmt shapes: `, err := .ResourceDriver(...)`. The +// receiver MUST be the provider's own receiver identifier (round-8 #5: +// rev3 accepted any `.ResourceDriver(...)`, so `helper.ResourceDriver(...)` +// or `plan.ResourceDriver(...)` falsely classified as canonical). +func isCanonicalApplyLoopAssign(a *ast.AssignStmt, recvName string) bool { + if len(a.Lhs) != 2 || len(a.Rhs) != 1 { + return false + } + if id, ok := a.Lhs[1].(*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 + } + // Round-9 #2: only ResourceDriver is canonical. wfctlhelpers.ApplyPlan + // dispatches through IaCProvider.ResourceDriver specifically — a + // provider that wraps lookup in `Driver(...)` or `DriverFor(...)` + // would have its wrapper bypassed on rewrite, which can change the + // driver returned (caching, instrumentation, etc.). + if sel.Sel.Name != "ResourceDriver" { + return false + } + // Receiver must be the provider's own identifier. + x, ok := sel.X.(*ast.Ident) + if !ok { + return false + } + if !((recvName != "" && x.Name == recvName) || (recvName == "" && x.Name == "p")) { + return false + } + // Round-12 #7: also verify the lookup KEY is `action.Resource.Type`. + // wfctlhelpers.ApplyPlan always dispatches with `action.Resource.Type`, + // so a provider that picks drivers by some other key (e.g. + // `action.Tag` or a computed value) would see different driver + // behavior on rewrite. Require the canonical key shape. + if len(call.Args) != 1 { + return false + } + keySel, ok := call.Args[0].(*ast.SelectorExpr) + if !ok || keySel.Sel.Name != "Type" { + return false + } + innerSel, ok := keySel.X.(*ast.SelectorExpr) + if !ok || innerSel.Sel.Name != "Resource" { + return false + } + innerId, ok := innerSel.X.(*ast.Ident) + if !ok || innerId.Name != "action" { + return false + } + return true +} + +// isCanonicalApplyLoopIf returns true for the canonical loop-body +// IfStmt shapes: +// +// - `if err != nil { .Errors = append(...); continue }` +// - `if out != nil { .Resources = append(...) }` +// +// `resultName` is the local accumulator identifier (recovered from the +// outer scaffold). +// +// Round-8 #8: rev6 of isCanonicalApplyLoopIfBodyStmt accepted a bare +// `continue`/`break` statement, but wfctlhelpers ALWAYS records an +// ActionError before continuing past a failure. So a guard like +// `if err != nil { continue }` (no append) would silently change +// behavior on rewrite. Now we require: when the guard body contains a +// continue/break, it MUST also contain an append-to-result statement. +func isCanonicalApplyLoopIf(ifs *ast.IfStmt, resultName string) bool { + if ifs == nil { + return false + } + be, ok := ifs.Cond.(*ast.BinaryExpr) + if !ok || be.Op != token.NEQ { + return false + } + id, ok := be.X.(*ast.Ident) + if !ok || (id.Name != "err" && id.Name != "out") { + return false + } + if rhs, ok := be.Y.(*ast.Ident); !ok || rhs.Name != "nil" { + return false + } + if ifs.Else != nil { + return false + } + hasAppend := false + hasBranch := false + for _, s := range ifs.Body.List { + switch ss := s.(type) { + case *ast.AssignStmt: + if !isCanonicalAppendToResult(ss, resultName) { + return false + } + hasAppend = true + case *ast.BranchStmt: + // Round-9 #1: only `continue` is canonical; `break` + // silently aborts the loop on first error, but + // wfctlhelpers.ApplyPlan records the error and KEEPS + // processing later actions, so accepting `break` would + // silently change behavior on rewrite. + if ss.Tok != token.CONTINUE { + return false + } + hasBranch = true + default: + return false + } + } + // A bare continue/break (no append) is rejected — wfctlhelpers + // always records the ActionError before continuing. + if hasBranch && !hasAppend { + return false + } + return true +} + +// isCanonicalAppendToResult returns true if stmt is +// `. = append(...)`. Used inside loop-body if-guards +// (round-8 #8: tightened to require this shape, not just "any +// append"). +func isCanonicalAppendToResult(s *ast.AssignStmt, resultName string) bool { + if len(s.Lhs) != 1 || len(s.Rhs) != 1 || s.Tok != token.ASSIGN { + return false + } + sel, ok := s.Lhs[0].(*ast.SelectorExpr) + if !ok { + return false + } + if id, ok := sel.X.(*ast.Ident); !ok || id.Name != resultName { + return false + } + call, ok := s.Rhs[0].(*ast.CallExpr) + if !ok { + return false + } + idFn, ok := call.Fun.(*ast.Ident) + if !ok || idFn.Name != "append" { + return false + } + return true +} + +// fmtPosShort renders a path:line short form for offender positions. +// Path is left as-supplied (caller provides the path the user gave). +func fmtPosShort(path string, line int) string { + return fmt.Sprintf("%s:%d", path, line) +} + +// isAlreadyDelegatedApplyBody returns true if fn.Body is a single +// `return .ApplyPlan(...)`. Review round-4 finding +// #4: rev3 hardcoded the package identifier as `wfctlhelpers`. A +// provider that already delegates through an aliased import (e.g. +// `wf "github.com/.../wfctlhelpers"; return wf.ApplyPlan(...)`) was +// misreported as non-canonical. Resolves the import alias via +// pkgAliasFor so any aliased delegation is recognised. +func isAlreadyDelegatedApplyBody(body *ast.BlockStmt, file *ast.File) bool { + if len(body.List) != 1 { + return false + } + ret, ok := body.List[0].(*ast.ReturnStmt) + if !ok || len(ret.Results) != 1 { + return false + } + call, ok := ret.Results[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 + } + if sel.Sel.Name != "ApplyPlan" { + return false + } + // Accept the literal default name OR the file's local alias for + // the wfctlhelpers import path. Falls back to the literal name + // when file is nil (test paths that don't pass it). + wantAlias := pkgAliasFor(file, helperImportPath, "wfctlhelpers") + return x.Name == wantAlias || x.Name == "wfctlhelpers" +} + +// findActionSwitch returns the first switch statement whose tag is a +// SelectorExpr `.Action` (canonical: `action.Action`). Only the +// outermost RangeStmt's body is searched: nested switches inside if +// branches are still matched by ast.Inspect, which is fine — the +// dispatch must be on `something.Action`. +func findActionSwitch(body *ast.BlockStmt) *ast.SwitchStmt { + var found *ast.SwitchStmt + ast.Inspect(body, func(n ast.Node) bool { + if found != nil { + return false + } + sw, ok := n.(*ast.SwitchStmt) + if !ok { + return true + } + sel, ok := sw.Tag.(*ast.SelectorExpr) + if !ok { + return true + } + if sel.Sel.Name == "Action" { + found = sw + return false + } + return true + }) + return found +} + +// findUpdateReplaceCollapseCase returns the position of the first case +// clause whose case-list literals include both "update" and "replace". +// Returns token.NoPos if no such collapse exists. +func findUpdateReplaceCollapseCase(sw *ast.SwitchStmt) token.Pos { + for _, stmt := range sw.Body.List { + cc, ok := stmt.(*ast.CaseClause) + if !ok { + continue + } + hasUpdate, hasReplace := false, false + for _, expr := range cc.List { + s, ok := stringLiteral(expr) + if !ok { + continue + } + switch s { + case "update": + hasUpdate = true + case "replace": + hasReplace = true + } + } + if hasUpdate && hasReplace { + return cc.Pos() + } + } + return token.NoPos +} + +// findUpsertRecovery returns the position of an `errors.Is(err, X)` +// call inside a case clause where X has the suffix `AlreadyExists`. +// Match is conservative: the receiver is `errors`, the selector is +// `Is`, and the second arg's name (or its selector tail) ends in +// "AlreadyExists". This catches both `ErrResourceAlreadyExists` and +// `interfaces.ErrResourceAlreadyExists`. +func findUpsertRecovery(sw *ast.SwitchStmt) token.Pos { + var found token.Pos + ast.Inspect(sw, func(n ast.Node) bool { + if found.IsValid() { + return false + } + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + x, ok := sel.X.(*ast.Ident) + if !ok || x.Name != "errors" || sel.Sel.Name != "Is" { + return true + } + if len(call.Args) < 2 { + return true + } + if name := tailIdent(call.Args[1]); strings.HasSuffix(name, "AlreadyExists") { + found = call.Pos() + return false + } + return true + }) + return found +} + +// tailIdent returns the trailing identifier name of a SelectorExpr +// chain (or the bare ident name), or "" for unrecognised shapes. +func tailIdent(expr ast.Expr) string { + switch e := expr.(type) { + case *ast.Ident: + return e.Name + case *ast.SelectorExpr: + return e.Sel.Name + } + return "" +} + +// findCustomErrorWrap returns the position of an `err = fmt.Errorf(..., +// %w, err)` reassignment that wraps an existing error — i.e., the RHS +// fmt.Errorf call references the local `err` variable as one of its +// arguments. This is the bespoke domain-context wrapping pattern. +// +// The narrower-than-just-`err = fmt.Errorf(...)` shape is intentional: +// a `default:` case in the action switch often has `err = fmt.Errorf("unknown action %q", ...)`, +// which is a FRESH error for an unknown action, not a wrap of a driver +// error. wfctlhelpers' generic dispatcher already errors on unknown +// actions, so the codemod must NOT flag that benign case. +// +// Match shape: assignment whose LHS is `err` and whose RHS is a +// fmt.Errorf call where at least one arg is the identifier `err`. +func findCustomErrorWrap(sw *ast.SwitchStmt) token.Pos { + var found token.Pos + ast.Inspect(sw, func(n ast.Node) bool { + if found.IsValid() { + return false + } + assign, ok := n.(*ast.AssignStmt) + if !ok { + return true + } + if len(assign.Lhs) != 1 || len(assign.Rhs) != 1 { + return true + } + id, ok := assign.Lhs[0].(*ast.Ident) + if !ok || id.Name != "err" { + return true + } + call, ok := assign.Rhs[0].(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + x, ok := sel.X.(*ast.Ident) + if !ok { + return true + } + if x.Name != "fmt" || sel.Sel.Name != "Errorf" { + return true + } + // Must reference `err` somewhere in the args (the wrap target). + // A fmt.Errorf for a fresh error doesn't pass `err`, so this + // keeps the unknown-action default case clean. + for _, a := range call.Args { + if argId, ok := a.(*ast.Ident); ok && argId.Name == "err" { + found = assign.Pos() + return false + } + } + return true + }) + return found +} + +// hasCanonicalCases returns true if the switch has at least the +// "create" + "update" cases (delete is conventional but optional in +// providers that don't support delete via Apply) AND every case body +// has only the canonical-shape statements (driver method calls, +// ResourceRef construction, simple if-guards on action.Current). The +// body-shape validation closes review round-1 finding #5: rev0 of this +// function only checked case labels, so extra bookkeeping or metrics +// inside a case body would still classify as canonical and get silently +// dropped during rewrite. +// +// Recognised case-body statement kinds (each maps to a known shape +// in wfctlhelpers' generic dispatcher): +// +// - AssignStmt: `out, err = drv.Create(ctx, action.Resource)` / +// `err = drv.Delete(ctx, ref)` / `ref := ResourceRef{...}` / +// `ref.ProviderID = action.Current.ProviderID` +// - IfStmt: only the `if action.Current != nil` ProviderID-set +// guard pattern (cond is BinaryExpr NEQ on action.Current and nil) +// - DeclStmt: `var out *ResourceOutput` (rare but legal; wfctlhelpers +// handles its own out variable) +// +// Round-8 #4: rev3 of this function accepted `default:` clauses +// without inspecting their body. Logging/metrics/etc. in default +// silently dropped. Now default bodies are validated against the +// same shape: only AssignStmt of `err = fmt.Errorf(...)` (the +// canonical unknown-action error pattern) is allowed. Everything +// else (including bare logging) rejects. +// +// `recvName` is the provider receiver identifier — passed through to +// caseBodyIsCanonical → isCanonicalCaseAssign → isDriverMethodCall to +// validate driver-receiver names per the round-4 #3 fix. +func hasCanonicalCases(sw *ast.SwitchStmt, recvName string) bool { + hasCreate, hasUpdate := false, false + for _, stmt := range sw.Body.List { + cc, ok := stmt.(*ast.CaseClause) + if !ok { + continue + } + labels := caseLabels(cc) + isCanonicalLabel := false + for _, l := range labels { + switch l { + case "create": + hasCreate = true + isCanonicalLabel = true + case "update": + hasUpdate = true + isCanonicalLabel = true + case "delete", "replace": + isCanonicalLabel = true + } + } + // `default:` (no labels) — round-8 #4: validate body matches + // the canonical unknown-action error shape. Anything else + // (logging, metrics, alternate side-effect) rejects. + if len(labels) == 0 { + if !isCanonicalDefaultBody(cc.Body) { + return false + } + continue + } + if !isCanonicalLabel { + return false + } + if !caseBodyIsCanonical(cc.Body) { + return false + } + // Round-12 #6: verify the case body's driver call matches the + // case label. A `case "create"` body that actually calls + // `.Update(...)` or `.Delete(...)` would be silently rewritten + // away because wfctlhelpers.ApplyPlan dispatches "create" to + // Driver.Create. Mismatch means the rewrite changes semantics. + if !caseBodyMatchesLabel(cc.Body, labels) { + return false + } + } + return hasCreate && hasUpdate +} + +// caseBodyMatchesLabel returns true if the driver-method calls inside +// body match the case labels. The mapping is: +// +// "create" → .Create +// "update" → .Update +// "replace" → either .Update OR .Delete+.Create (helpers may use either) +// "delete" → .Delete +// +// A case body with no driver call still passes (helpers like ref-init +// don't have a method call). A case body whose ONLY driver call has +// the wrong method-name for ANY of the labels rejects. +// +// Round-12 #6: rev1 of hasCanonicalCases didn't link labels to body +// operations; mismatched implementations were silently rewritten. +func caseBodyMatchesLabel(body []ast.Stmt, labels []string) bool { + calledMethods := make(map[string]bool) + for _, stmt := range body { + ast.Inspect(stmt, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + switch sel.Sel.Name { + case "Create", "Read", "Update", "Delete": + calledMethods[sel.Sel.Name] = true + } + return true + }) + } + if len(calledMethods) == 0 { + // No driver call (e.g., body just inits a ref var). Passes — + // the canonical detector elsewhere handles ref-init shapes. + return true + } + for _, label := range labels { + 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 + } + } + return true +} + +// caseLabels returns the unquoted string-literal values of the case +// clause's case-list. A `default:` clause returns an empty slice. +// isCanonicalDefaultBody returns true if body matches the canonical +// `default:` clause shape: a single `err = fmt.Errorf("unknown action +// %q", ...)` assignment. Anything else (logging, metrics, alternate +// side-effects) rejects (round-8 #4). +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" +} + +func caseLabels(cc *ast.CaseClause) []string { + var out []string + for _, expr := range cc.List { + if s, ok := stringLiteral(expr); ok { + out = append(out, s) + } + } + return out +} + +// caseBodyIsCanonical returns true if every statement in body is in +// the recognised whitelist (driver call, ResourceRef construction, +// ProviderID guard). The whitelist is intentionally narrow so that +// bespoke statements (logging, metrics, alternate construction) cause +// rejection — the codemod errs on the side of NOT rewriting in +// ambiguous shapes. +func caseBodyIsCanonical(body []ast.Stmt) bool { + for _, stmt := range body { + if !canonicalCaseStmt(stmt) { + return false + } + } + return true +} + +// canonicalCaseStmt returns true if stmt fits one of the canonical +// shapes inside an action-switch case body. The whitelist is +// intentionally narrow: any statement outside the recognised set +// (bookkeeping counters, map updates, accumulators, alternate calls) +// causes rejection. Review round-3 finding #5: rev2 of this function +// accepted ANY AssignStmt, so `createsTotal++` / `metrics[action.Action]++` +// / `result.Stats.Updates++` all passed and the bespoke logic was +// silently dropped during -fix. +// +// Recognised AssignStmt shapes: +// +// - Multi-target call: `out, err = .(...)` with X a Driver +// identifier and METHOD in {Create, Read, Update, Delete} +// - Single-target call: `err = .(...)` (delete-style) +// - Composite literal: `ref := {...}` where T is ResourceRef-shaped +// - Selector assignment: `. = .` where the LHS is a known +// ProviderID-style field (ProviderID, Name, Type) +// +// Recognised non-Assign shapes: +// +// - if-guard: `if action.Current != nil { ... }` containing only +// canonical shapes (recursion via isProviderIDGuard) +// - var-decl: `var out *ResourceOutput` +func canonicalCaseStmt(stmt ast.Stmt) bool { + switch s := stmt.(type) { + case *ast.AssignStmt: + return isCanonicalCaseAssign(s) + case *ast.IfStmt: + return isProviderIDGuard(s) + case *ast.DeclStmt: + // Only `var out *ResourceOutput` (or qualified equivalent). + // Review round-4 finding #6: rev3 accepted ALL DeclStmts, so + // `var x SomeBookkeepingType` declarations passed as canonical + // and the bespoke local variable was silently dropped. + return isLocalOutPointerDecl(s) + } + return false +} + +// isLocalOutPointerDecl returns true if stmt is a single +// `var *` declaration. The name is not +// constrained (the standard convention is `out` but `o` / `result` +// are valid) but the type tail must be ResourceOutput. +func isLocalOutPointerDecl(s *ast.DeclStmt) bool { + gd, ok := s.Decl.(*ast.GenDecl) + if !ok || gd.Tok != token.VAR || len(gd.Specs) != 1 { + return false + } + vs, ok := gd.Specs[0].(*ast.ValueSpec) + if !ok || vs.Type == nil || len(vs.Names) != 1 { + return false + } + star, ok := vs.Type.(*ast.StarExpr) + if !ok { + return false + } + return typeNameTailMatches(star.X, "ResourceOutput") +} + +// isCanonicalCaseAssign tightens the AssignStmt acceptance whitelist +// to known canonical shapes (round-3 #5). +func isCanonicalCaseAssign(a *ast.AssignStmt) bool { + // Multi-target driver call: `out, err = .(...)`. + // Two LHS, one RHS that is a CallExpr on a SelectorExpr. + if len(a.Lhs) == 2 && len(a.Rhs) == 1 { + if isDriverMethodCall(a.Rhs[0]) { + return true + } + } + // Single-target driver call: `err = .(...)`. + if len(a.Lhs) == 1 && len(a.Rhs) == 1 { + if isDriverMethodCall(a.Rhs[0]) { + // LHS must be `err` — a different LHS would mean + // custom variable bookkeeping. + if id, ok := a.Lhs[0].(*ast.Ident); ok && id.Name == "err" { + return true + } + return false + } + // Composite-literal `ref := ResourceRef{...}` ONLY. Review + // round-4 finding #2: rev3 of this branch accepted any + // composite literal, so a bookkeeping struct construction + // (`payload := AuditPayload{...}`) was misclassified as + // canonical and silently dropped. Now the literal type's + // name (qualified or unqualified) must be ResourceRef. + if a.Tok == token.DEFINE { + if cl, ok := a.Rhs[0].(*ast.CompositeLit); ok && typeNameTailMatches(cl.Type, "ResourceRef") { + return true + } + } + // Selector assignment `ref. = ` to a ResourceRef-style + // field (ProviderID, Name, Type). Round-10 #4: rev3 accepted + // any LHS selector with this field name, so unrelated bookkeeping + // like `audit.Type = ...` or `result.ProviderID = ...` was + // misclassified as canonical and dropped on rewrite. The LHS + // receiver must be `ref` (the canonical ResourceRef + // construction site name). + if sel, ok := a.Lhs[0].(*ast.SelectorExpr); ok && a.Tok == token.ASSIGN { + if id, ok := sel.X.(*ast.Ident); !ok || id.Name != "ref" { + return false + } + switch sel.Sel.Name { + case "ProviderID", "Name", "Type": + return true + } + return false + } + } + return false +} + +// isDriverMethodCall reports whether expr is a call to a Driver method +// (Create/Read/Update/Delete) where the receiver is a known +// driver-bound identifier. Review round-4 finding #3: rev3 of this +// function only checked the selector NAME, so any call like +// `helper.Update(...)` or `metrics.Delete(...)` was misclassified as +// canonical driver dispatch and the case body was rewritten away. +// +// The receiver allowlist is intentionally narrow: `d`, `drv`, +// `driver` are the canonical names produced by the standard +// `d, err := p.ResourceDriver(action.Resource.Type)` pattern (DO, +// AWS, GCP, Azure). Anything else falls outside the rewrite-safe +// shape and the case body is reported as non-canonical. +func isDriverMethodCall(expr ast.Expr) bool { + call, ok := expr.(*ast.CallExpr) + if !ok { + return false + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + switch sel.Sel.Name { + case "Create", "Read", "Update", "Delete": + // fall through to receiver check + default: + return false + } + x, ok := sel.X.(*ast.Ident) + if !ok { + return false + } + // Conservative driver-receiver allowlist. Round-5 finding #9: rev3 + // allowlist {d, drv, driver} missed `dr`, `rd`, `rdrv`, etc. Widen + // to a slightly larger set of common single-/short-identifier names + // while still rejecting bookkeeping-style receivers like `metrics`, + // `audit`, `helper` (per round-4 #3 — that's the whole point of + // the receiver check). + switch x.Name { + case "d", "dr", "drv", "rd", "rdrv", "driver", "resourceDriver": + return true + } + return false +} + +// isProviderIDGuard checks for the canonical +// `if action.Current != nil { ... }` guard. Permissive on the body +// since the inner statement is itself a canonical AssignStmt +// (`ref.ProviderID = action.Current.ProviderID`). +func isProviderIDGuard(ifs *ast.IfStmt) bool { + be, ok := ifs.Cond.(*ast.BinaryExpr) + if !ok || be.Op != token.NEQ { + return false + } + xIsCurrent := false + if sel, ok := be.X.(*ast.SelectorExpr); ok && sel.Sel.Name == "Current" { + xIsCurrent = true + } + yIsNil := false + if id, ok := be.Y.(*ast.Ident); ok && id.Name == "nil" { + yIsNil = true + } + if !(xIsCurrent && yIsNil) { + // Allow the reverse order too (`nil != action.Current`), + // though it's not idiomatic Go. + yIsCurrent := false + if sel, ok := be.Y.(*ast.SelectorExpr); ok && sel.Sel.Name == "Current" { + yIsCurrent = true + } + xIsNil := false + if id, ok := be.X.(*ast.Ident); ok && id.Name == "nil" { + xIsNil = true + } + if !(yIsCurrent && xIsNil) { + return false + } + } + if ifs.Else != nil { + return false + } + for _, s := range ifs.Body.List { + if !canonicalCaseStmt(s) { + return false + } + } + return true +} + +// stringLiteral returns the unquoted value of a BasicLit STRING +// expression, or ("", false) for any other shape. +func stringLiteral(expr ast.Expr) (string, bool) { + bl, ok := expr.(*ast.BasicLit) + if !ok || bl.Kind != token.STRING { + return "", false + } + if len(bl.Value) < 2 { + return "", false + } + // Strip surrounding quotes (single-line strings only). + return bl.Value[1 : len(bl.Value)-1], true +} + +// rewriteApplyBody replaces fn.Body with +// `return wfctlhelpers.ApplyPlan(, , )`. +// +// Identifier recovery + injection (review round-1 #2, round-2 #4): +// +// - Receiver: ensureReceiverName injects "p" if the receiver is +// unnamed (`func (*Provider) Apply(...)`). rev1 fell back to a +// hardcoded "p" without updating the receiver decl, so the +// rewritten call referenced an undefined identifier. +// - ctx: ensureCtxParamName renames `_` → `ctx`; preserves any other +// non-blank name. +// - plan: same shape as ctx, applied to the second parameter slot. +func rewriteApplyBody(fn *ast.FuncDecl, file *ast.File) { + recvName := ensureReceiverName(fn, "p") + ctxName := ensureCtxParamName(fn) + planName := ensureNthParamName(fn, 1, "plan") + // Resolve the wfctlhelpers package alias (review round-3 finding #6: + // rev2 hardcoded "wfctlhelpers" but a file using + // `wf "github.com/.../wfctlhelpers"` wouldn't compile). + pkgAlias := pkgAliasFor(file, helperImportPath, "wfctlhelpers") + + call := &ast.CallExpr{ + Fun: &ast.SelectorExpr{ + X: ast.NewIdent(pkgAlias), + Sel: ast.NewIdent("ApplyPlan"), + }, + Args: []ast.Expr{ + ast.NewIdent(ctxName), + ast.NewIdent(recvName), + ast.NewIdent(planName), + }, + } + fn.Body = &ast.BlockStmt{ + List: []ast.Stmt{ + &ast.ReturnStmt{Results: []ast.Expr{call}}, + }, + } +} + +// ensureNthParamName returns the name of fn's `idx`-th parameter, +// injecting `defaultName` (and renaming `_`) the same way +// ensureCtxParamName does for the first parameter. Used by +// rewriteApplyBody for the `plan` argument slot. +func ensureNthParamName(fn *ast.FuncDecl, idx int, defaultName string) string { + if fn.Type.Params == nil || len(fn.Type.Params.List) <= idx { + return defaultName + } + field := fn.Type.Params.List[idx] + if len(field.Names) == 0 { + field.Names = []*ast.Ident{ast.NewIdent(defaultName)} + return defaultName + } + if len(field.Names) == 1 { + n := field.Names[0].Name + if n == "_" || n == "" { + field.Names[0] = ast.NewIdent(defaultName) + return defaultName + } + return n + } + if field.Names[0].Name != "" && field.Names[0].Name != "_" { + return field.Names[0].Name + } + field.Names[0] = ast.NewIdent(defaultName) + return defaultName +} + +// (writeFileAtomic + ensureImport live in refactor_plan.go; +// refactor-apply reuses them via ensureWfctlhelpersImport.) + +// ============================================================ +// Report rendering +// ============================================================ + +func (r *applyReport) print(w io.Writer, opts *Options) { + sort.Slice(r.sites, func(i, j int) bool { + if r.sites[i].Path != r.sites[j].Path { + return r.sites[i].Path < r.sites[j].Path + } + return r.sites[i].Line < r.sites[j].Line + }) + fmt.Fprintln(w, "# iac-codemod refactor-apply report") + fmt.Fprintln(w) + mode := "dry-run" + if opts != nil && opts.Fix { + mode = "fix" + } + fmt.Fprintf(w, "Mode: %s\n", mode) + fmt.Fprintf(w, "Sites: %d\n", len(r.sites)) + fmt.Fprintf(w, "Errors: %d\n", len(r.errors)) + fmt.Fprintln(w) + + groups := map[applyClassification][]applySite{} + order := []applyClassification{ + applyCanonical, + applyUpsertRecovery, + applyUpdateReplaceCollapse, + applyCustomErrorWrapping, + applyNonCanonicalOther, + applyMissingSwitch, + applyAlreadyDelegated, + applySkipped, + } + for _, s := range r.sites { + groups[s.Class] = append(groups[s.Class], s) + } + headers := map[applyClassification]string{ + applyCanonical: "Canonical (rewrite candidate)", + applyUpsertRecovery: "Upsert recovery — DO-style ErrResourceAlreadyExists path", + applyUpdateReplaceCollapse: "Update+replace collapse — manual port required", + applyCustomErrorWrapping: "Custom error wrapping — extension-point hook required", + applyNonCanonicalOther: "Non-canonical (manual review required)", + applyMissingSwitch: "Missing action-switch — hand-port required", + applyAlreadyDelegated: "Already-delegated (no-op)", + applySkipped: "Skipped (// wfctl:skip-iac-codemod)", + } + for _, c := range order { + sites := groups[c] + if len(sites) == 0 { + continue + } + fmt.Fprintf(w, "## %s\n\n", headers[c]) + for _, s := range sites { + suffix := "" + if c == applyCanonical && s.Rewrote { + suffix = " (rewritten)" + } + line := fmt.Sprintf("- %s:%d %s.Apply %s%s", s.Path, s.Line, s.Receiver, s.Class, suffix) + if s.OffenderPos != "" { + line += fmt.Sprintf(" (offender at %s)", s.OffenderPos) + } + fmt.Fprintln(w, line) + if s.Suggestion != "" { + fmt.Fprintf(w, " - suggestion: %s\n", s.Suggestion) + } + } + fmt.Fprintln(w) + } + + if len(r.errors) > 0 { + fmt.Fprintln(w, "## Errors") + fmt.Fprintln(w) + for _, e := range r.errors { + fmt.Fprintf(w, "- %s\n", e) + } + fmt.Fprintln(w) + } +} diff --git a/cmd/iac-codemod/refactor_apply_test.go b/cmd/iac-codemod/refactor_apply_test.go new file mode 100644 index 00000000..61364a9e --- /dev/null +++ b/cmd/iac-codemod/refactor_apply_test.go @@ -0,0 +1,717 @@ +// Copyright (c) 2026 Jon Langevin +// SPDX-License-Identifier: Apache-2.0 + +// Tests in this file MUST NOT call t.Parallel(). Same global-state +// constraint as main_test.go / lint_test.go / refactor_plan_test.go. + +package main + +import ( + "bytes" + "os" + "strings" + "testing" + "time" +) + +// ============================================================ +// Source fixtures +// ============================================================ + +// applyWithExtraBookkeepingSrc — review round-1 finding #5. An Apply +// body with bespoke bookkeeping inside a case body (here, a metrics +// counter / println) must NOT be classified as canonical: silently +// rewriting would drop the bookkeeping. Test fixture defined below the +// canonicalApplySrc anchor; lookup test follows it. +const applyWithExtraBookkeepingSrc = `package p + +import ( + "context" + "fmt" +) + +type ResourceSpec struct{ Name, Type string } +type ResourceState struct{ Name string; ProviderID string } +type IaCPlan struct{ ID string; Actions []PlanAction } +type PlanAction struct{ Action string; Resource ResourceSpec; Current *ResourceState } +type ApplyResult struct{ PlanID string; Errors []ActionError; Resources []ResourceOutput } +type ActionError struct{ Resource, Action, Error string } +type ResourceRef struct{ Name, Type, ProviderID string } +type ResourceOutput struct{ ProviderID string } +type PlanDiagnostic struct{} + +type Driver interface { + Create(ctx context.Context, r ResourceSpec) (*ResourceOutput, error) + Update(ctx context.Context, ref ResourceRef, r ResourceSpec) (*ResourceOutput, error) + Delete(ctx context.Context, ref ResourceRef) error +} + +type BookkeepingProvider struct{} + +func (p *BookkeepingProvider) ResourceDriver(string) (Driver, error) { return nil, nil } + +func (p *BookkeepingProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return platform.ComputePlan(ctx, p, desired, current) +} + +func (p *BookkeepingProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + result := &ApplyResult{PlanID: plan.ID} + for _, action := range plan.Actions { + d, err := p.ResourceDriver(action.Resource.Type) + if err != nil { + result.Errors = append(result.Errors, ActionError{Resource: action.Resource.Name, Action: action.Action, Error: err.Error()}) + continue + } + var out *ResourceOutput + switch action.Action { + case "create": + fmt.Println("creating") + out, err = d.Create(ctx, action.Resource) + case "update": + ref := ResourceRef{Name: action.Resource.Name, Type: action.Resource.Type} + out, err = d.Update(ctx, ref, action.Resource) + } + if err != nil { + result.Errors = append(result.Errors, ActionError{Resource: action.Resource.Name, Action: action.Action, Error: err.Error()}) + continue + } + _ = out + } + return result, nil +} + +func (p *BookkeepingProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +func TestRefactorApply_ExtraBookkeepingNotCanonical(t *testing.T) { + path := writeFixture(t, "provider.go", applyWithExtraBookkeepingSrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if strings.Contains(out, "BookkeepingProvider.Apply canonical") && !strings.Contains(out, "non-canonical") { + t.Errorf("Apply with extra bookkeeping inside a case body must NOT be canonical; got:\n%s", out) + } + if !strings.Contains(out, "non-canonical") { + t.Errorf("Apply with extra bookkeeping should be reported non-canonical; got:\n%s", out) + } +} + +// Round-2 finding #4 also applied to refactor-apply but the +// unnamed-receiver path can't have a canonical-shape body in real Go: +// without a receiver identifier in scope, the body can't call +// `.ResourceDriver(...)`, which the round-7+round-8-tightened +// canonical detector now requires. The receiver-injection helper +// ensureReceiverName is shared between refactor-plan and refactor-apply; +// coverage is in TestRefactorPlan_Fix_UnnamedReceiverGetsName. + +// canonicalApplySrc is a minimal Apply body the codemod will rewrite. +// Loop+switch on action.Action with create/update/delete branches that +// dispatch directly to the driver. Modeled on the simplest pattern +// expected by wfctlhelpers.ApplyPlan. +const canonicalApplySrc = `package p + +import ( + "context" + "fmt" + "time" +) + +type ResourceSpec struct{ Name, Type string } +type ResourceState struct{ Name string; ProviderID string } +type IaCPlan struct{ ID string; CreatedAt time.Time; Actions []PlanAction } +type PlanAction struct{ Action string; Resource ResourceSpec; Current *ResourceState } +type ApplyResult struct{ PlanID string; Errors []ActionError; Resources []ResourceOutput } +type ActionError struct{ Resource, Action, Error string } +type ResourceRef struct{ Name, Type, ProviderID string } +type ResourceOutput struct{ ProviderID string } +type PlanDiagnostic struct{} + +type Driver interface { + Create(ctx context.Context, r ResourceSpec) (*ResourceOutput, error) + Update(ctx context.Context, ref ResourceRef, r ResourceSpec) (*ResourceOutput, error) + Delete(ctx context.Context, ref ResourceRef) error +} + +type FooProvider struct{} + +func (p *FooProvider) ResourceDriver(string) (Driver, error) { return nil, nil } + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return wfctlhelpers.Plan(ctx, p, desired, current) +} + +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + result := &ApplyResult{PlanID: plan.ID} + for _, action := range plan.Actions { + d, err := p.ResourceDriver(action.Resource.Type) + if err != nil { + result.Errors = append(result.Errors, ActionError{Resource: action.Resource.Name, Action: action.Action, Error: err.Error()}) + continue + } + var out *ResourceOutput + switch action.Action { + case "create": + out, err = d.Create(ctx, action.Resource) + case "update": + ref := ResourceRef{Name: action.Resource.Name, Type: action.Resource.Type} + if action.Current != nil { + ref.ProviderID = action.Current.ProviderID + } + out, err = d.Update(ctx, ref, action.Resource) + case "delete": + ref := ResourceRef{Name: action.Resource.Name, Type: action.Resource.Type} + if action.Current != nil { + ref.ProviderID = action.Current.ProviderID + } + err = d.Delete(ctx, ref) + default: + err = fmt.Errorf("unknown action %q", action.Action) + } + if err != nil { + result.Errors = append(result.Errors, ActionError{Resource: action.Resource.Name, Action: action.Action, Error: err.Error()}) + continue + } + if out != nil { + result.Resources = append(result.Resources, *out) + } + } + return result, nil +} + +func (p *FooProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +// doUpsertApplySrc replicates the DigitalOcean upsert-on-create-conflict +// pattern. The "create" case branches on errors.Is(err, +// ErrResourceAlreadyExists) and routes through Read+Update to recover. +// The codemod must DETECT this and refuse to rewrite, emitting a +// suggested upsertSupporter hook patch. +const doUpsertApplySrc = `package p + +import ( + "context" + "errors" + "fmt" +) + +type ResourceSpec struct{ Name, Type string } +type ResourceState struct{ Name, ProviderID string } +type IaCPlan struct{ Actions []PlanAction } +type PlanAction struct{ Action string; Resource ResourceSpec; Current *ResourceState } +type ApplyResult struct{ Errors []ActionError; Resources []ResourceOutput } +type ActionError struct{ Resource, Action, Error string } +type ResourceRef struct{ Name, Type, ProviderID string } +type ResourceOutput struct{ ProviderID string } +type PlanDiagnostic struct{} + +var ErrResourceAlreadyExists = errors.New("already exists") + +type upsertSupporter interface{ SupportsUpsert() bool } + +type Driver interface { + Create(ctx context.Context, r ResourceSpec) (*ResourceOutput, error) + Update(ctx context.Context, ref ResourceRef, r ResourceSpec) (*ResourceOutput, error) + Read(ctx context.Context, ref ResourceRef) (*ResourceOutput, error) + Delete(ctx context.Context, ref ResourceRef) error +} + +type DOProvider struct{} + +func (p *DOProvider) ResourceDriver(string) (Driver, error) { return nil, nil } + +func (p *DOProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return wfctlhelpers.Plan(ctx, p, desired, current) +} + +func (p *DOProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + result := &ApplyResult{} + for _, action := range plan.Actions { + d, err := p.ResourceDriver(action.Resource.Type) + if err != nil { + result.Errors = append(result.Errors, ActionError{Resource: action.Resource.Name, Action: action.Action, Error: err.Error()}) + continue + } + var out *ResourceOutput + switch action.Action { + case "create": + out, err = d.Create(ctx, action.Resource) + if errors.Is(err, ErrResourceAlreadyExists) { + us, ok := d.(upsertSupporter) + if !ok || !us.SupportsUpsert() { + break + } + createErr := err + ref := ResourceRef{Name: action.Resource.Name, Type: action.Resource.Type} + existing, readErr := d.Read(ctx, ref) + if readErr != nil { + err = fmt.Errorf("upsert: read after conflict: %w", errors.Join(createErr, readErr)) + break + } + ref.ProviderID = existing.ProviderID + out, err = d.Update(ctx, ref, action.Resource) + } + case "update": + ref := ResourceRef{Name: action.Resource.Name, Type: action.Resource.Type, ProviderID: action.Current.ProviderID} + out, err = d.Update(ctx, ref, action.Resource) + default: + err = fmt.Errorf("unknown action %q", action.Action) + } + if err != nil { + result.Errors = append(result.Errors, ActionError{Resource: action.Resource.Name, Action: action.Action, Error: err.Error()}) + continue + } + if out != nil { + result.Resources = append(result.Resources, *out) + } + } + return result, nil +} + +func (p *DOProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +// awsUpdateReplaceCollapseSrc replicates AWSProvider.Apply: the +// "update" and "replace" actions share a single case clause. The +// codemod must DETECT this and emit "manual port required" with line +// numbers because wfctlhelpers' doReplace path is meaningfully +// different from doUpdate (delete+create vs in-place modify) and +// silent collapse would lose semantic distinction. +const awsUpdateReplaceCollapseSrc = `package p + +import ( + "context" + "fmt" +) + +type ResourceSpec struct{ Name, Type string } +type ResourceState struct{ Name, ProviderID string } +type IaCPlan struct{ ID string; Actions []PlanAction } +type PlanAction struct{ Action string; Resource ResourceSpec; Current *ResourceState } +type ApplyResult struct{ PlanID string; Errors []ActionError; Resources []ResourceOutput } +type ActionError struct{ Resource, Action, Error string } +type ResourceRef struct{ Name, Type, ProviderID string } +type ResourceOutput struct{ ProviderID string } +type PlanDiagnostic struct{} + +type Driver interface { + Create(ctx context.Context, r ResourceSpec) (*ResourceOutput, error) + Update(ctx context.Context, ref ResourceRef, r ResourceSpec) (*ResourceOutput, error) + Delete(ctx context.Context, ref ResourceRef) error +} + +type AWSProvider struct{} + +func (p *AWSProvider) resourceDriver(string) (Driver, error) { return nil, nil } + +func (p *AWSProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return wfctlhelpers.Plan(ctx, p, desired, current) +} + +func (p *AWSProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + result := &ApplyResult{PlanID: plan.ID} + for _, action := range plan.Actions { + drv, err := p.resourceDriver(action.Resource.Type) + if err != nil { + result.Errors = append(result.Errors, ActionError{Resource: action.Resource.Name, Action: action.Action, Error: err.Error()}) + continue + } + var out *ResourceOutput + switch action.Action { + case "create": + out, err = drv.Create(ctx, action.Resource) + case "update", "replace": + ref := ResourceRef{Name: action.Resource.Name, Type: action.Resource.Type} + if action.Current != nil { + ref.ProviderID = action.Current.ProviderID + } + out, err = drv.Update(ctx, ref, action.Resource) + case "delete": + ref := ResourceRef{Name: action.Resource.Name, Type: action.Resource.Type} + if action.Current != nil { + ref.ProviderID = action.Current.ProviderID + } + err = drv.Delete(ctx, ref) + } + if err != nil { + result.Errors = append(result.Errors, ActionError{Resource: action.Resource.Name, Action: action.Action, Error: err.Error()}) + continue + } + if out != nil { + result.Resources = append(result.Resources, *out) + } + } + _ = fmt.Sprintf("anchor") + return result, nil +} + +func (p *AWSProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +// customErrorWrapApplySrc replicates a custom-error-wrapping idiom: +// errors returned from the driver are wrapped with bespoke domain text +// before being recorded. wfctlhelpers' default error path doesn't +// preserve this wrapping, so the codemod must DETECT and emit an +// extension-point hook + sample patch (post-hook on ApplyResult.Errors). +const customErrorWrapApplySrc = `package p + +import ( + "context" + "fmt" +) + +type ResourceSpec struct{ Name, Type string } +type ResourceState struct{ Name, ProviderID string } +type IaCPlan struct{ Actions []PlanAction } +type PlanAction struct{ Action string; Resource ResourceSpec } +type ApplyResult struct{ Errors []ActionError; Resources []ResourceOutput } +type ActionError struct{ Resource, Action, Error string } +type ResourceRef struct{ Name, Type string } +type ResourceOutput struct{} +type PlanDiagnostic struct{} + +type Driver interface { + Create(ctx context.Context, r ResourceSpec) (*ResourceOutput, error) + Update(ctx context.Context, ref ResourceRef, r ResourceSpec) (*ResourceOutput, error) + Delete(ctx context.Context, ref ResourceRef) error +} + +type WrapProvider struct{} + +func (p *WrapProvider) resourceDriver(string) (Driver, error) { return nil, nil } + +func (p *WrapProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return wfctlhelpers.Plan(ctx, p, desired, current) +} + +func (p *WrapProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + result := &ApplyResult{} + for _, action := range plan.Actions { + d, err := p.resourceDriver(action.Resource.Type) + if err != nil { + result.Errors = append(result.Errors, ActionError{Resource: action.Resource.Name, Action: action.Action, Error: err.Error()}) + continue + } + var out *ResourceOutput + switch action.Action { + case "create": + out, err = d.Create(ctx, action.Resource) + if err != nil { + err = fmt.Errorf("wrap: %s create %s failed: %w", "wrap-provider", action.Resource.Name, err) + } + case "update": + out, err = d.Update(ctx, ResourceRef{Name: action.Resource.Name}, action.Resource) + if err != nil { + err = fmt.Errorf("wrap: %s update %s failed: %w", "wrap-provider", action.Resource.Name, err) + } + } + if err != nil { + result.Errors = append(result.Errors, ActionError{Resource: action.Resource.Name, Action: action.Action, Error: err.Error()}) + continue + } + _ = out + } + return result, nil +} + +func (p *WrapProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +// skippedApplySrc carries the canonical marker on the function doc. +// Apply must not be rewritten regardless of body shape; site listed in +// the report. +const skippedApplySrc = `package p + +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type FooProvider struct{} +type PlanDiagnostic struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { return wfctlhelpers.Plan(ctx, p, desired, current) } + +// wfctl:skip-iac-codemod custom orchestration, see ADR-042 +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return &ApplyResult{}, nil +} + +func (p *FooProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +// alreadyDelegatedApplySrc has Apply already calling wfctlhelpers.ApplyPlan. +// The mode must NOT report it as non-canonical and must NOT mutate it. +const alreadyDelegatedApplySrc = `package p + +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type FooProvider struct{} +type PlanDiagnostic struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { return wfctlhelpers.Plan(ctx, p, desired, current) } +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return wfctlhelpers.ApplyPlan(ctx, p, plan) +} +func (p *FooProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +// ============================================================ +// Detection (dry-run) +// ============================================================ + +func TestRefactorApply_DryRun_DetectsCanonical(t *testing.T) { + path := writeFixture(t, "provider.go", canonicalApplySrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "FooProvider.Apply") { + t.Errorf("report should name FooProvider.Apply; got:\n%s", out) + } + if !strings.Contains(out, "canonical") { + t.Errorf("report should classify as canonical; got:\n%s", out) + } + got, _ := os.ReadFile(path) + if string(got) != canonicalApplySrc { + t.Errorf("dry-run modified the file; expected no mutation") + } +} + +func TestRefactorApply_DryRun_DetectsDOUpsertRecovery(t *testing.T) { + path := writeFixture(t, "provider.go", doUpsertApplySrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "DOProvider.Apply") { + t.Errorf("report should name DOProvider.Apply; got:\n%s", out) + } + if !strings.Contains(out, "upsert-recovery") { + t.Errorf("report should classify as upsert-recovery; got:\n%s", out) + } + if !strings.Contains(out, "upsertSupporter") { + t.Errorf("report should suggest upsertSupporter hook patch; got:\n%s", out) + } +} + +func TestRefactorApply_DryRun_DetectsUpdateReplaceCollapse(t *testing.T) { + path := writeFixture(t, "provider.go", awsUpdateReplaceCollapseSrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "AWSProvider.Apply") { + t.Errorf("report should name AWSProvider.Apply; got:\n%s", out) + } + if !strings.Contains(out, "update+replace-collapse") { + t.Errorf("report should classify as update+replace-collapse; got:\n%s", out) + } + if !strings.Contains(out, "manual port required") { + t.Errorf("report should advise manual port; got:\n%s", out) + } + // Must include line numbers for the offending case clause. + if !strings.Contains(out, ":") { + t.Errorf("report should include path:line for the offending case; got:\n%s", out) + } +} + +func TestRefactorApply_DryRun_DetectsCustomErrorWrapping(t *testing.T) { + path := writeFixture(t, "provider.go", customErrorWrapApplySrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "WrapProvider.Apply") { + t.Errorf("report should name WrapProvider.Apply; got:\n%s", out) + } + if !strings.Contains(out, "custom-error-wrapping") { + t.Errorf("report should classify as custom-error-wrapping; got:\n%s", out) + } + if !strings.Contains(out, "extension-point") { + t.Errorf("report should mention extension-point hook; got:\n%s", out) + } +} + +func TestRefactorApply_DryRun_HonorsSkipMarker(t *testing.T) { + path := writeFixture(t, "provider.go", skippedApplySrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "Skipped") { + t.Errorf("report should have a Skipped section; got:\n%s", out) + } + if !strings.Contains(out, "FooProvider.Apply") { + t.Errorf("Skipped section should list FooProvider.Apply; got:\n%s", out) + } +} + +func TestRefactorApply_DryRun_AlreadyDelegated(t *testing.T) { + path := writeFixture(t, "provider.go", alreadyDelegatedApplySrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "already-delegated") { + t.Errorf("already-delegated Apply should be classified explicitly; got:\n%s", out) + } +} + +// ============================================================ +// Mutation (-fix) +// ============================================================ + +func TestRefactorApply_Fix_RewritesCanonical(t *testing.T) { + path := writeFixture(t, "provider.go", canonicalApplySrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + gotStr := string(got) + if !strings.Contains(gotStr, "return wfctlhelpers.ApplyPlan(ctx, p, plan)") { + t.Errorf("rewritten Apply must call wfctlhelpers.ApplyPlan; got:\n%s", gotStr) + } + if strings.Contains(gotStr, "switch action.Action {") { + t.Errorf("canonical switch should be removed by rewrite; got:\n%s", gotStr) + } + if !strings.Contains(gotStr, `"github.com/GoCodeAlone/workflow/iac/wfctlhelpers"`) { + t.Errorf("rewrite should add wfctlhelpers import; got:\n%s", gotStr) + } +} + +func TestRefactorApply_Fix_DoesNotRewriteUpsertRecovery(t *testing.T) { + path := writeFixture(t, "provider.go", doUpsertApplySrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != doUpsertApplySrc { + t.Errorf("upsert-recovery must NOT be rewritten; file changed:\n%s", string(got)) + } +} + +func TestRefactorApply_Fix_DoesNotRewriteUpdateReplaceCollapse(t *testing.T) { + path := writeFixture(t, "provider.go", awsUpdateReplaceCollapseSrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != awsUpdateReplaceCollapseSrc { + t.Errorf("update+replace-collapse must NOT be rewritten; file changed:\n%s", string(got)) + } +} + +func TestRefactorApply_Fix_DoesNotRewriteCustomErrorWrapping(t *testing.T) { + path := writeFixture(t, "provider.go", customErrorWrapApplySrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != customErrorWrapApplySrc { + t.Errorf("custom-error-wrapping must NOT be rewritten; file changed:\n%s", string(got)) + } +} + +func TestRefactorApply_Fix_HonorsSkipMarker(t *testing.T) { + path := writeFixture(t, "provider.go", skippedApplySrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != skippedApplySrc { + t.Errorf("skip-marker'd Apply must NOT be rewritten; file changed:\n%s", string(got)) + } +} + +func TestRefactorApply_Fix_IdempotentOnAlreadyDelegated(t *testing.T) { + path := writeFixture(t, "provider.go", alreadyDelegatedApplySrc) + var stdout, stderr bytes.Buffer + if code := runRefactorApply([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr); code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != alreadyDelegatedApplySrc { + t.Errorf("already-delegated source must be byte-identical after fix (idempotent)") + } +} + +// ============================================================ +// codemod-report.md output (per spec line 2388) +// ============================================================ + +func TestRefactorApply_DryRun_WritesReportFile(t *testing.T) { + // Per plan §T8.4 line 2388: "Output `codemod-report.md` with per-file + // findings + suggested handling." When -report-file is supplied the + // mode writes the report there as well as stdout. Default report + // filename matches the spec literally. + dir := t.TempDir() + reportPath := dir + "/codemod-report.md" + path := writeFixture(t, "provider.go", doUpsertApplySrc) + var stdout, stderr bytes.Buffer + code := runRefactorApply([]string{"-report-file", reportPath, path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + body, err := os.ReadFile(reportPath) + if err != nil { + t.Fatalf("report file not written: %v", err) + } + if !strings.Contains(string(body), "upsert-recovery") { + t.Errorf("report file must include classification; got:\n%s", string(body)) + } +} + +// ============================================================ +// Mutation-gate negative tests +// ============================================================ + +func TestRefactorApply_DryRunFalseWithoutFix_DoesNotMutate(t *testing.T) { + path := writeFixture(t, "provider.go", canonicalApplySrc) + stat0, _ := os.Stat(path) + mtime0 := stat0.ModTime() + time.Sleep(10 * time.Millisecond) + + var stdout, stderr bytes.Buffer + code := run([]string{"refactor-apply", "-dry-run=false", path}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != canonicalApplySrc { + t.Errorf("file must NOT be mutated; content changed") + } + stat1, _ := os.Stat(path) + if !stat1.ModTime().Equal(mtime0) { + t.Errorf("file mtime should be unchanged; before=%v after=%v", mtime0, stat1.ModTime()) + } +} diff --git a/cmd/iac-codemod/refactor_plan.go b/cmd/iac-codemod/refactor_plan.go new file mode 100644 index 00000000..2640ee64 --- /dev/null +++ b/cmd/iac-codemod/refactor_plan.go @@ -0,0 +1,1402 @@ +// Copyright (c) 2026 Jon Langevin +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "fmt" + "go/ast" + "go/format" + "go/parser" + "go/token" + "io" + "io/fs" + "os" + "path/filepath" + "sort" + "strings" +) + +func init() { + modes["refactor-plan"] = runRefactorPlan +} + +// helperImportPath is the canonical Go import path for the wfctlhelpers +// package (used by refactor-apply for ApplyPlan delegation). Any source +// file that gains a `wfctlhelpers.ApplyPlan` call must also import this +// package. +const helperImportPath = "github.com/GoCodeAlone/workflow/iac/wfctlhelpers" + +// planHelperImportPath is the import path for platform.ComputePlan, the +// canonical Plan helper provider Plan() bodies delegate to. This is a +// rev1-review correction: the plan-doc named `wfctlhelpers.Plan` as the +// rewrite target, but no such API exists today in the repo. The actual +// Plan-equivalent helper is `platform.ComputePlan(ctx, p, desired, current)` +// at platform/differ.go:72. Switching the codemod target to the real API +// closes Copilot review finding #1 (lint.go:45 + refactor_plan.go:36): +// "the rewrite target does not exist in the repository today; rewritten +// files would fail to compile". +const planHelperImportPath = "github.com/GoCodeAlone/workflow/platform" + +// planCanonicalCallExpr is the canonical replacement-body expression +// emitted by refactor-plan. Calls platform.ComputePlan (the real helper); +// see planHelperImportPath above for the review-correction rationale. +const planCanonicalCallExpr = "platform.ComputePlan(ctx, p, desired, current)" + +// planClassification labels the disposition of a single Plan() method +// site. Each report entry carries one classification; the rewriter +// honors only `planCanonical`. +type planClassification int + +const ( + // planCanonical: body matches the configHash-compare template; safe + // to rewrite to wfctlhelpers.Plan. + planCanonical planClassification = iota + // planNonCanonical: body has out-of-template logic; report only, + // never rewrite. + planNonCanonical + // planAlreadyDelegated: body is the canonical 2-statement + // `plan, err := platform.ComputePlan(...); return &plan, err` + // form (or the legacy `return wfctlhelpers.Plan(...)` shape); + // report as no-op (idempotent), do NOT rewrite. Round-11 #6: + // rev1 of this comment still referenced the old `wfctlhelpers.Plan` + // target; the actual recognised shape is platform.ComputePlan + // per planHelperImportPath above. + planAlreadyDelegated + // planSkipped: function carries the SkipMarker; report into the + // Skipped section. (Distinct from the lint-mode skip path because + // refactor-plan tracks skips per-site for the report.) + planSkipped +) + +// String renders the classification for the report. Lower-case so +// "non-canonical" / "canonical" read naturally inline. +func (c planClassification) String() string { + switch c { + case planCanonical: + return "canonical" + case planNonCanonical: + return "non-canonical" + case planAlreadyDelegated: + return "already-delegated" + case planSkipped: + return "skipped" + default: + return "unknown" + } +} + +// planSite captures one Plan-method site in the report. +type planSite struct { + Path string + Line int + Receiver string // type name, e.g. "DOProvider" + Class planClassification // canonical / non-canonical / already-delegated / skipped + Reason string // for non-canonical: why detection rejected the body + Rewrote bool // set true if this site was rewritten on -fix +} + +// planReport aggregates per-file results across an entire refactor-plan run. +type planReport struct { + sites []planSite + errors []string +} + +// runRefactorPlan is the entry point for the refactor-plan subcommand. +// It walks the supplied paths, classifies each Plan method site, and +// (when -fix is set) rewrites canonical bodies in place via atomic +// temp-file + rename. +func runRefactorPlan(args []string, opts *Options, stdout, stderr io.Writer) int { + if len(args) == 0 { + fmt.Fprintln(stderr, "iac-codemod refactor-plan: at least one path is required") + usage(stderr) + return 2 + } + report := &planReport{} + for _, path := range args { + if err := refactorPlanPath(path, opts, report); err != nil { + fmt.Fprintf(stderr, "iac-codemod refactor-plan: %s: %v\n", path, err) + return 1 + } + } + report.print(stdout, opts) + if len(report.errors) > 0 { + return 1 + } + return 0 +} + +// refactorPlanPath walks `path` for *.go files (excluding _test.go, +// vendor, testdata, hidden) and processes each. Per-file errors are +// recorded in the report so a single broken file does not abort the run. +func refactorPlanPath(path string, opts *Options, report *planReport) error { + info, err := stat(path) + if err != nil { + return err + } + if !info.IsDir() { + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return fmt.Errorf("not a Go source file (or is a _test.go): %s", path) + } + if err := refactorPlanFile(path, opts, report); err != nil { + report.errors = append(report.errors, fmt.Sprintf("%s: %v", path, err)) + } + return nil + } + return filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + base := d.Name() + if shouldSkipDir(base) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { + return nil + } + if err := refactorPlanFile(p, opts, report); err != nil { + report.errors = append(report.errors, fmt.Sprintf("%s: %v", p, err)) + } + return nil + }) +} + +// refactorPlanFile parses `path`, classifies every Plan method, and (in +// -fix mode) mutates the AST and writes the result atomically. +func refactorPlanFile(path string, opts *Options, report *planReport) error { + src, err := readFile(path) + if err != nil { + return err + } + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, src, parser.ParseComments) + if err != nil { + return err + } + + // Build the receiver-shape filter using the directory-wide + // method set so providers whose Plan/Apply live in sibling files + // are still recognised (review round-1 finding #9). Per-file + // fallback when the directory walk fails — keeps the rev0 + // behavior on isolated single-file targets. + // Round-12 #2: skip files in a non-dominant package. The + // directory-wide provs/typeDocs are built from the dominant + // package only; processing a non-dominant file against another + // package's method set could rewrite the wrong file when + // receiver names overlap. + dominant := dominantPackageForDir(filepath.Dir(path)) + if dominant != "" && file.Name.Name != dominant { + return nil + } + provs := planLikeReceiversInDir(filepath.Dir(path)) + if len(provs) == 0 { + provs = planLikeReceivers(file) + } + // Directory-wide type-doc lookup so a `// wfctl:skip-iac-codemod` + // marker on a sibling file's type declaration is honored even when + // the Plan/Apply methods we're walking live in a separate file + // (review round-6 finding #1). + typeDocs := receiverTypeDocsInDir(filepath.Dir(path), file) + + mutated := false + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + if !isProviderMethod(fn, "Plan", 3, 2) { + continue + } + recv := receiverTypeName(fn) + if !provs[recv] { + // Method named Plan on a non-provider type — skip with no + // report entry (lint already reports those if relevant; the + // codemod focuses on rewriting providers). + continue + } + // Honor the marker on the function doc, the receiver type's + // TypeSpec doc, AND the wrapping GenDecl doc. Review round-1 + // finding #4: PR description promises type-doc + GenDecl-doc + // honoring; rev0 only checked fn.Doc. + if hasSkipMarkerOn(fn.Doc) || typeDocs[recv].carriesMarker() { + report.sites = append(report.sites, planSite{ + Path: path, + Line: fset.Position(fn.Pos()).Line, + Receiver: recv, + Class: planSkipped, + }) + continue + } + class, reason := classifyPlanBody(fn, file) + site := planSite{ + Path: path, + Line: fset.Position(fn.Pos()).Line, + Receiver: recv, + Class: class, + Reason: reason, + } + if class == planCanonical && opts != nil && opts.Fix { + rewritePlanBody(fn, file) + mutated = true + site.Rewrote = true + } + report.sites = append(report.sites, site) + } + + if mutated && opts != nil && opts.Fix { + // Ensure the platform import is present (refactor-plan emits + // platform.ComputePlan). The function is idempotent. + ensurePlatformImport(file) + if err := writeFileAtomic(path, fset, file); err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + } + return nil +} + +// planLikeReceivers returns the set of receiver type names whose method +// set in `file` includes both Plan and Apply with shapes matching +// IaCProvider. Used as a fallback path when no package context is +// available; production callers should prefer planLikeReceiversInDir +// (review round-1 finding #9: rev0 of this function only consulted +// the current file, missing providers whose Plan and Apply live in +// sibling files). +func planLikeReceivers(file *ast.File) map[string]bool { + methodsByRecv := make(map[string][]*ast.FuncDecl) + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + recv := receiverTypeName(fn) + if recv == "" { + continue + } + methodsByRecv[recv] = append(methodsByRecv[recv], fn) + } + out := make(map[string]bool) + for recv, methods := range methodsByRecv { + if looksLikeProvider(methods) { + out[recv] = true + } + } + return out +} + +// planLikeReceiversInDir returns the set of receiver type names whose +// method set across ALL non-test .go files in dir includes both Plan +// and Apply (canonical IaCProvider shape). Closes review round-1 +// finding #9: a provider whose Plan() and Apply() live in sibling +// files (e.g. provider_plan.go + provider_apply.go) is invisible to +// the per-file planLikeReceivers helper. Per-directory aggregation +// matches Go's package-scoped method-set semantics. +// +// Errors are tolerated: any file whose parser.ParseFile call fails is +// silently dropped from the aggregation. The intent is to widen the +// detection net, not to enforce package-correctness (which is the +// linter's job). +func planLikeReceiversInDir(dir string) map[string]bool { + out, _ := planLikeProviderMethodsInDir(dir) + return out +} + +// dominantPackageForDir returns the most-common `package P` clause +// across non-test .go files in dir (lex-first wins on tie). Used by +// refactor-* and add-validate-plan to skip files in non-dominant +// packages — round-12 #2/#3/#4: rev2 walked every file but built +// provs/typeDocs from only the dominant package, so a non-dominant +// file with overlapping receiver names could be rewritten against +// the dominant package's method set and produce a wrong-file +// migration. Returns "" when dir cannot be read. +func dominantPackageForDir(dir string) string { + entries, err := os.ReadDir(dir) + if err != nil { + return "" + } + pkgCounts := make(map[string]int) + for _, e := range entries { + if e.IsDir() { + continue + } + name := e.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + fpath := filepath.Join(dir, name) + src, err := readFile(fpath) + if err != nil { + continue + } + fs := token.NewFileSet() + f, err := parser.ParseFile(fs, fpath, src, parser.PackageClauseOnly) + if err != nil { + continue + } + pkgCounts[f.Name.Name]++ + } + if len(pkgCounts) == 0 { + return "" + } + pkgNames := make([]string, 0, len(pkgCounts)) + for pkg := range pkgCounts { + pkgNames = append(pkgNames, pkg) + } + sort.Strings(pkgNames) + dominant := "" + dominantCount := 0 + for _, pkg := range pkgNames { + if pkgCounts[pkg] > dominantCount { + dominant = pkg + dominantCount = pkgCounts[pkg] + } + } + return dominant +} + +// planLikeProviderMethodsInDir is like planLikeReceiversInDir but also +// returns the per-receiver method slice (across all files in dir) so +// callers can inspect ValidatePlan presence + receiver-kind for +// providers split across sibling files (round-2 #5 + round-3 #1). +// +// Files are filtered by package name: only files whose `package P` +// clause matches the dominant (most-common) package in dir are +// aggregated. Review round-5 finding #6: rev2 merged methods from +// EVERY non-test .go file regardless of package, so a build-tagged +// or mixed-package directory could fold methods from unrelated +// packages into a synthetic provider and drive incorrect rewrites / +// stub insertion. +// +// The returned slice contains *ast.FuncDecl values from a SEPARATE +// parser.ParseFile call than any caller's primary file parse, so +// caller code that relies on AST-pointer identity must dedupe (see +// add_validate_plan.go's name-based merge). +func planLikeProviderMethodsInDir(dir string) (map[string]bool, map[string][]*ast.FuncDecl) { + entries, err := os.ReadDir(dir) + if err != nil { + return nil, nil + } + // Pass 1: parse every candidate file's package clause to find the + // dominant package. + type parsedFile struct { + pkg string + file *ast.File + } + var files []parsedFile + pkgCounts := make(map[string]int) + for _, e := range entries { + if e.IsDir() { + continue + } + name := e.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + fpath := filepath.Join(dir, name) + src, err := readFile(fpath) + if err != nil { + continue + } + fs := token.NewFileSet() + file, err := parser.ParseFile(fs, fpath, src, parser.ParseComments) + if err != nil { + continue + } + pkgCounts[file.Name.Name]++ + files = append(files, parsedFile{pkg: file.Name.Name, file: file}) + } + if len(files) == 0 { + return nil, nil + } + // Round-10 #6: rev3 used range-over-map which has random iteration + // order, so on a tie the dominant package selection was + // nondeterministic. Sort the package names so the tie-break is + // stable (lexicographic-first wins). + pkgNames := make([]string, 0, len(pkgCounts)) + for pkg := range pkgCounts { + pkgNames = append(pkgNames, pkg) + } + sort.Strings(pkgNames) + dominant := "" + dominantCount := 0 + for _, pkg := range pkgNames { + if pkgCounts[pkg] > dominantCount { + dominant = pkg + dominantCount = pkgCounts[pkg] + } + } + // Pass 2: aggregate methods only from the dominant package. + methodsByRecv := make(map[string][]*ast.FuncDecl) + for _, p := range files { + if p.pkg != dominant { + continue + } + for _, decl := range p.file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + recv := receiverTypeName(fn) + if recv == "" { + continue + } + methodsByRecv[recv] = append(methodsByRecv[recv], fn) + } + } + out := make(map[string]bool) + for recv, methods := range methodsByRecv { + if looksLikeProvider(methods) { + out[recv] = true + } + } + return out, methodsByRecv +} + +// receiverDoc captures the documentation positions where a skip marker +// could be placed for a given receiver type: the inner TypeSpec.Doc +// (between `type` and the type name) and the wrapping GenDecl.Doc +// (before the `type` keyword). hasSkipMarkerOn handles nil so the +// call site can pass either field unconditionally. +type receiverDoc struct { + TypeSpecDoc *ast.CommentGroup + GenDeclDoc *ast.CommentGroup +} + +func (d receiverDoc) carriesMarker() bool { + return hasSkipMarkerOn(d.TypeSpecDoc) || hasSkipMarkerOn(d.GenDeclDoc) +} + +// receiverTypeDocs returns a map from receiver type name to its +// associated documentation positions. Used by refactor-plan and +// refactor-apply to check the SkipMarker at type-doc and GenDecl-doc +// levels in addition to the function-doc level (review round-1 +// finding #4). +// +// Single-file scope only — for cross-file scenarios (provider type +// declared in a sibling file from its Plan/Apply methods), use +// receiverTypeDocsInDir which merges across the directory's dominant +// package (review round-6 finding #1). +func receiverTypeDocs(file *ast.File) map[string]receiverDoc { + out := make(map[string]receiverDoc) + for _, decl := range file.Decls { + gd, ok := decl.(*ast.GenDecl) + if !ok || gd.Tok != token.TYPE { + continue + } + for _, spec := range gd.Specs { + ts, ok := spec.(*ast.TypeSpec) + if !ok { + continue + } + out[ts.Name.Name] = receiverDoc{ + TypeSpecDoc: ts.Doc, + GenDeclDoc: gd.Doc, + } + } + } + return out +} + +// receiverTypeDocsInDir returns the receiver-type doc map merged across +// every non-test .go file in dir whose `package P` clause matches the +// dominant package. Closes review round-6 finding #1: rev3 of refactor-* +// ran receiverTypeDocs on the per-file AST only, so a provider whose +// type declaration lived in a SIBLING file (round-3's directory-wide +// method-set scan made this layout possible) had its `// wfctl:skip-iac-codemod` +// type-doc marker silently ignored, and the methods in the current +// file would still be rewritten. +// +// File parses are reused (not deduped) — each file gets its own +// FileSet/parse — but all yielded receiverDocs share the same +// dominant-package filter as planLikeProviderMethodsInDir to keep the +// build-tagged / mixed-package case safe. +// +// Falls back to the per-file map if the directory walk fails (e.g. +// path is a single file, not a directory). +func receiverTypeDocsInDir(dir string, primary *ast.File) map[string]receiverDoc { + out := receiverTypeDocs(primary) + entries, err := os.ReadDir(dir) + if err != nil { + return out + } + // Determine dominant package from the directory. + pkgCounts := make(map[string]int) + type parsedDoc struct { + pkg string + file *ast.File + } + var files []parsedDoc + for _, e := range entries { + if e.IsDir() { + continue + } + name := e.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + fpath := filepath.Join(dir, name) + src, err := readFile(fpath) + if err != nil { + continue + } + fs := token.NewFileSet() + f, err := parser.ParseFile(fs, fpath, src, parser.ParseComments) + if err != nil { + continue + } + pkgCounts[f.Name.Name]++ + files = append(files, parsedDoc{pkg: f.Name.Name, file: f}) + } + dominant := primary.Name.Name + if dominantCount, ok := pkgCounts[dominant]; !ok || dominantCount == 0 { + // Primary's package isn't in the directory walk (rare — + // happens when `path` is outside the dominant package). Just + // return the per-file map unchanged. + return out + } + for pkg, c := range pkgCounts { + if c > pkgCounts[dominant] { + dominant = pkg + } + } + // Merge sibling docs into out. The primary file's TypeSpec docs + // take precedence (they're already in `out` from receiverTypeDocs); + // sibling-file docs are added only for receivers not yet in `out`. + for _, p := range files { + if p.pkg != dominant { + continue + } + if p.file == primary { + continue // already merged via receiverTypeDocs(primary) + } + sib := receiverTypeDocs(p.file) + for recv, doc := range sib { + if _, ok := out[recv]; ok { + continue + } + out[recv] = doc + } + } + return out +} + +// classifyPlanBody inspects the body of a Plan method and returns its +// classification + (when non-canonical) a short reason. Detection is +// purely structural and conservative: only bodies that match the +// configHash-compare template are returned as canonical; anything else +// — including bodies that are MOSTLY canonical but have an extra +// statement — is reported as non-canonical. The conservative bias is +// intentional: a false-canonical risks silently dropping bespoke logic +// during rewrite, whereas a false-non-canonical merely surfaces a +// finding the maintainer can review and either skip-mark or hand-port. +func classifyPlanBody(fn *ast.FuncDecl, file *ast.File) (planClassification, string) { + if fn.Body == nil { + return planNonCanonical, "missing body" + } + // Already-delegated: single statement `return wfctlhelpers.Plan(...)`. + if isAlreadyDelegatedPlanBody(fn.Body, file) { + return planAlreadyDelegated, "" + } + // Canonical: body matches the configHash-compare template. + // Extract the actual `desired` and `current` parameter names from + // the signature so the canonical detector can validate the body + // against THIS provider's naming convention (round-8 #6: rev3 + // hardcoded "current"/"desired", missing providers using `state`/ + // `specs`). + desiredParam := nthParamName(fn, 1, "desired") + currentParam := nthParamName(fn, 2, "current") + if isCanonicalPlanBody(fn.Body, desiredParam, currentParam) { + return planCanonical, "" + } + return planNonCanonical, "Plan body does not match configHash-compare template" +} + +// nthParamName returns the name of fn's `idx`-th parameter (0-based) +// or `defaultName` if the parameter is unnamed/blank. Used by the +// canonical detector and rewriter to honor whatever names the original +// author used. +func nthParamName(fn *ast.FuncDecl, idx int, defaultName string) string { + if fn.Type.Params == nil || len(fn.Type.Params.List) <= idx { + return defaultName + } + field := fn.Type.Params.List[idx] + if len(field.Names) == 0 { + return defaultName + } + n := field.Names[0].Name + if n == "" || n == "_" { + return defaultName + } + return n +} + +// isAlreadyDelegatedPlanBody returns true ONLY for the canonical +// 2-statement rev2 form (with package alias resolution per round-4 #4): +// +// plan, err := .ComputePlan(ctx, p, desired, current) +// return &plan, err +// +// Round-5 finding #5: the legacy single-statement forms (broken rev1 +// `return platform.ComputePlan(...)` and rev0 `return wfctlhelpers.Plan(...)`) +// are NOT accepted as already-delegated. They're uncompilable broken +// output. Treating them as no-op meant rerunning the fixed codemod +// would never repair them. They now classify as non-canonical (the +// classifyPlanBody fallthrough) so a fresh -fix produces the correct +// 2-statement form. +// +// (The lint analyzer's "delegated" check still accepts the legacy +// forms as delegated for advisory purposes, since the marker mismatch +// is benign there. Only the rewriter distinguishes "broken output +// needing repair" from "true no-op idempotent".) +func isAlreadyDelegatedPlanBody(body *ast.BlockStmt, file *ast.File) bool { + platformAlias := pkgAliasFor(file, planHelperImportPath, "platform") + if len(body.List) != 2 { + return false + } + if !isPlatformComputePlanAssign(body.List[0], platformAlias) { + return false + } + return isAddrPlanReturn(body.List[1]) +} + +// isPlatformComputePlanAssign returns true if stmt is +// `plan, err := .ComputePlan(...)`. pkgAlias is the local +// name the file uses for the platform import (resolved by caller). +func isPlatformComputePlanAssign(stmt ast.Stmt, pkgAlias string) bool { + a, ok := stmt.(*ast.AssignStmt) + if !ok || a.Tok != token.DEFINE || len(a.Lhs) != 2 || len(a.Rhs) != 1 { + 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 == pkgAlias || x.Name == "platform") && sel.Sel.Name == "ComputePlan" +} + +// isAddrPlanReturn returns true if stmt is `return &, ` for +// some idents X, Y. Conservative match for the canonical 2-statement +// rewrite output. +func isAddrPlanReturn(stmt ast.Stmt) bool { + ret, ok := stmt.(*ast.ReturnStmt) + if !ok || len(ret.Results) != 2 { + return false + } + un, ok := ret.Results[0].(*ast.UnaryExpr) + if !ok || un.Op != token.AND { + return false + } + if _, ok := un.X.(*ast.Ident); !ok { + return false + } + if _, ok := ret.Results[1].(*ast.Ident); !ok { + return false + } + return true +} + +// isCanonicalPlanBody recognizes the configHash-compare template. The +// shape we accept (fuzzy on whitespace + identifier choice but tight on +// semantic structure): +// +// 1. An assignment building a name->state map: ` := make(map[string], len(current))`. +// 2. A range over `current` populating that map. +// 3. A composite-literal assignment building a `*{...}` plan +// (any IaCPlan-shaped struct). +// 4. A range over `desired` whose body appends `Action: "create"` or +// `Action: "update"` to plan.Actions, with the update branch gated +// on `configHash(...) != configHash(...)`. +// 5. A final `return plan, nil`. +// +// This is intentionally tighter than "first-pass heuristic" — review +// round 0 finding (anticipated): a too-loose canonical detector silently +// rewrites bespoke planners that happen to share keywords. +func isCanonicalPlanBody(body *ast.BlockStmt, desiredParam, currentParam string) bool { + stmts := body.List + + // 1. currentByName := make(map[string]...) + idx := 0 + if idx >= len(stmts) { + return false + } + if !isMapMakeAssign(stmts[idx]) { + return false + } + idx++ + + // 2. range over the `current` parameter (whatever the actual name). + if idx >= len(stmts) { + return false + } + if !isRangeOverIdent(stmts[idx], currentParam) { + return false + } + idx++ + + // 3. plan composite literal assignment + if idx >= len(stmts) { + return false + } + if !isPlanCompositeAssign(stmts[idx]) { + return false + } + idx++ + + // 4. range over the `desired` parameter (whatever the actual name) + // whose body has create + configHash-gated update. + if idx >= len(stmts) { + return false + } + rng, ok := stmts[idx].(*ast.RangeStmt) + if !ok { + return false + } + xIdent, ok := rng.X.(*ast.Ident) + if !ok || xIdent.Name != desiredParam { + return false + } + if !rangeBodyMatchesCanonicalDesired(rng.Body) { + return false + } + idx++ + + // 5. return plan, nil — must be EXACTLY this shape. Review round-3 + // finding #2: rev2 accepted any 2-result return, so a planner with + // the canonical scaffold but a bespoke final return (returning a + // cloned plan, propagating an error value, etc.) would still + // classify as canonical and the bespoke return logic would be + // silently dropped during rewrite. + if idx >= len(stmts) { + return false + } + ret, ok := stmts[idx].(*ast.ReturnStmt) + if !ok || len(ret.Results) != 2 { + return false + } + if id, ok := ret.Results[0].(*ast.Ident); !ok || id.Name != "plan" { + return false + } + if id, ok := ret.Results[1].(*ast.Ident); !ok || id.Name != "nil" { + return false + } + idx++ + + // Trailing junk → reject. + return idx == len(stmts) +} + +// isMapMakeAssign matches ` := make(map[string], ...)`. +func isMapMakeAssign(stmt ast.Stmt) bool { + a, ok := stmt.(*ast.AssignStmt) + if !ok || a.Tok != token.DEFINE || len(a.Rhs) != 1 { + return false + } + call, ok := a.Rhs[0].(*ast.CallExpr) + if !ok { + return false + } + id, ok := call.Fun.(*ast.Ident) + if !ok || id.Name != "make" { + return false + } + if len(call.Args) < 1 { + return false + } + _, ok = call.Args[0].(*ast.MapType) + return ok +} + +// isRangeOverIdent matches `for ..., ... := range { ... }`. +func isRangeOverIdent(stmt ast.Stmt, name string) bool { + rng, ok := stmt.(*ast.RangeStmt) + if !ok { + return false + } + id, ok := rng.X.(*ast.Ident) + if !ok { + return false + } + return id.Name == name +} + +// isPlanCompositeAssign matches `plan := &{...}`. +func isPlanCompositeAssign(stmt ast.Stmt) bool { + a, ok := stmt.(*ast.AssignStmt) + if !ok || a.Tok != token.DEFINE || len(a.Lhs) != 1 || len(a.Rhs) != 1 { + return false + } + if id, ok := a.Lhs[0].(*ast.Ident); !ok || id.Name != "plan" { + return false + } + un, ok := a.Rhs[0].(*ast.UnaryExpr) + if !ok || un.Op != token.AND { + return false + } + cl, ok := un.X.(*ast.CompositeLit) + if !ok { + return false + } + _ = cl + return true +} + +// rangeBodyMatchesCanonicalDesired verifies the body of the +// range-over-desired loop is EXACTLY the configHash-compare template: +// +// 1. lookup statement (`cur, exists := []`) +// 2. `if !exists { plan.Actions = append(plan.Actions, ...); continue }` +// — body MUST be exactly: one append-to-plan.Actions + one continue. +// 3. `if configHash(...) != configHash(...) { plan.Actions = append(plan.Actions, ...) }` +// — body MUST be exactly: one append-to-plan.Actions. +// +// Reject any statement that doesn't fit these three slots — bespoke +// telemetry, metrics, alternate construction, etc. — to keep the +// canonical detector tight. Round-5 finding #1: rev3 only checked the +// guard expressions and statement count; it never inspected what the +// branch bodies did, so extra logic inside `!exists` (or different +// create/update behavior) classified as canonical and was silently +// dropped during -fix. +// +// Both branch bodies are validated by isCanonicalPlanActionsAppendOnly +// (append + optional continue) so a planner with extra side-effects +// inside either branch is rejected. +// +// Top-level statement count must be exactly 3. The lookup statement +// may be assignment-style (`:=`) or simple-assign (`=`) — both are +// valid Go. +func rangeBodyMatchesCanonicalDesired(body *ast.BlockStmt) bool { + stmts := body.List + if len(stmts) != 3 { + return false + } + // 1. lookup `, := []` or single-target equivalent. + a, ok := stmts[0].(*ast.AssignStmt) + if !ok || (a.Tok != token.DEFINE && a.Tok != token.ASSIGN) { + return false + } + if len(a.Lhs) != 2 || len(a.Rhs) != 1 { + return false + } + if _, isIndex := a.Rhs[0].(*ast.IndexExpr); !isIndex { + return false + } + // 2. !exists guard with append+continue body. + notExists, ok := stmts[1].(*ast.IfStmt) + if !ok { + return false + } + u, ok := notExists.Cond.(*ast.UnaryExpr) + if !ok || u.Op != token.NOT { + return false + } + // Accept both `exists` (DO convention) and `ok` (idiomatic Go). + // Round-5 finding #8: rev3 hardcoded "exists", missing the + // semantically-identical `cur, ok := currentByName[...]` form. + id, ok := u.X.(*ast.Ident) + if !ok || (id.Name != "exists" && id.Name != "ok") { + return false + } + if notExists.Else != nil { + return false + } + if !isCanonicalCreateBranchBody(notExists.Body) { + return false + } + // 3. configHash != configHash guard with append-only body. + hashGuard, ok := stmts[2].(*ast.IfStmt) + if !ok { + return false + } + be, ok := hashGuard.Cond.(*ast.BinaryExpr) + if !ok || be.Op != token.NEQ { + return false + } + if !isConfigHashCall(be.X) || !isConfigHashCall(be.Y) { + return false + } + if hashGuard.Else != nil { + return false + } + if !isCanonicalUpdateBranchBody(hashGuard.Body) { + return false + } + return true +} + +// isCanonicalCreateBranchBody returns true if body is exactly: +// +// plan.Actions = append(plan.Actions, PlanAction{Action: "create", ...}) +// continue +// +// Round-12 #5: requires the appended action's `Action: "create"` field +// so a planner that builds different actions (e.g., "queue", "noop") +// from the canonical scaffold is rejected, preventing silent drop of +// custom action types. +func isCanonicalCreateBranchBody(body *ast.BlockStmt) bool { + if body == nil || len(body.List) != 2 { + return false + } + if !isPlanActionsAppendAssign(body.List[0], "create") { + return false + } + br, ok := body.List[1].(*ast.BranchStmt) + if !ok || br.Tok != token.CONTINUE { + return false + } + return true +} + +// isCanonicalUpdateBranchBody returns true if body is exactly: +// +// plan.Actions = append(plan.Actions, PlanAction{Action: "update", ...}) +// +// Round-12 #5: requires the appended action's `Action: "update"` field. +func isCanonicalUpdateBranchBody(body *ast.BlockStmt) bool { + if body == nil || len(body.List) != 1 { + return false + } + return isPlanActionsAppendAssign(body.List[0], "update") +} + +// isPlanActionsAppendAssign returns true if stmt is +// `plan.Actions = append(plan.Actions, )`. Both LHS AND +// the append's first argument must reference plan.Actions; the +// payload (second arg) is unconstrained (composite literal is fine). +// +// Round-7 finding #1: rev5 only verified the LHS, so a bespoke +// `plan.Actions = append(otherSlice, ...)` (e.g., a planner that +// builds actions from an alternate slice) was misclassified as +// canonical and the alternate-slice logic silently dropped during +// rewrite. +func isPlanActionsAppendAssign(stmt ast.Stmt, expectedAction string) bool { + a, ok := stmt.(*ast.AssignStmt) + if !ok || a.Tok != token.ASSIGN || len(a.Lhs) != 1 || len(a.Rhs) != 1 { + return false + } + sel, ok := a.Lhs[0].(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Actions" { + return false + } + if id, ok := sel.X.(*ast.Ident); !ok || id.Name != "plan" { + return false + } + call, ok := a.Rhs[0].(*ast.CallExpr) + if !ok { + return false + } + idFn, ok := call.Fun.(*ast.Ident) + if !ok || idFn.Name != "append" || len(call.Args) < 2 { + return false + } + // Verify append's first argument is also `plan.Actions`. + firstSel, ok := call.Args[0].(*ast.SelectorExpr) + if !ok || firstSel.Sel.Name != "Actions" { + return false + } + if id, ok := firstSel.X.(*ast.Ident); !ok || id.Name != "plan" { + return false + } + // Round-12 #5: verify the appended payload is a PlanAction + // composite literal whose Action field matches expectedAction. + // This rejects bespoke planners that use the canonical scaffold + // but build different actions (e.g., a planner that emits "noop" + // or "queue" instead of "create"/"update"); silent rewrite would + // drop those custom action types. + if expectedAction != "" { + cl, ok := call.Args[1].(*ast.CompositeLit) + if !ok { + return false + } + actionLit := "" + for _, elt := range cl.Elts { + kv, ok := elt.(*ast.KeyValueExpr) + if !ok { + continue + } + if k, ok := kv.Key.(*ast.Ident); !ok || k.Name != "Action" { + continue + } + if bl, ok := kv.Value.(*ast.BasicLit); ok && bl.Kind == token.STRING { + actionLit = strings.Trim(bl.Value, `"`) + } + break + } + if actionLit != expectedAction { + return false + } + } + return true +} + +// isConfigHashCall reports whether expr is a call to the unexported +// `configHash` function: `configHash()`. Used to recognise the +// configHash-compare guard inside the canonical Plan template. +func isConfigHashCall(expr ast.Expr) bool { + call, ok := expr.(*ast.CallExpr) + if !ok { + return false + } + id, ok := call.Fun.(*ast.Ident) + if !ok { + return false + } + return id.Name == "configHash" +} + +// rewritePlanBody replaces fn.Body with the canonical 2-statement +// delegation to platform.ComputePlan: +// +// plan, err := platform.ComputePlan(, , desired, current) +// return &plan, err +// +// platform.ComputePlan returns `(interfaces.IaCPlan, error)` BY VALUE, +// but provider Plan methods return `(*interfaces.IaCPlan, error)`. +// Review round-2 finding #1: a single-statement +// `return platform.ComputePlan(...)` rewrite produces uncompilable code +// because the value/pointer mismatch can't be implicitly bridged. The +// 2-statement form takes the address of the local return value before +// returning it, matching the provider interface. +// +// Receiver and ctx identifiers are recovered from the signature; rules +// (review round-1 #2, round-2 #3): +// +// - If the receiver is unnamed (`func (*Provider) Plan(...)`), give +// it a name (`p`) so the substituted call has a referent. rev1 +// fell back to a hardcoded "p" but didn't update the receiver +// decl, so the rewritten call referenced an undefined identifier. +// - Blank `_` ctx parameters are renamed to `ctx` (standard idiom); +// non-blank ctx names are preserved. +func rewritePlanBody(fn *ast.FuncDecl, file *ast.File) { + recvName := ensureReceiverName(fn, "p") + ctxName := ensureCtxParamName(fn) + // Review round-3 finding #3: rev2 hardcoded "desired" and "current" + // as the 2nd/3rd argument names. A canonical Plan declared as + // `Plan(ctx, specs, state)` rewrites to references to undefined + // identifiers `desired` / `current`. Extract the actual parameter + // names from the signature so the substituted call always + // references real identifiers. + desiredName := ensureNthParamName(fn, 1, "desired") + currentName := ensureNthParamName(fn, 2, "current") + + // Resolve the package alias for github.com/GoCodeAlone/workflow/platform + // so the call uses whatever name the file already imports under + // (review round-3 finding #4: rev2 hardcoded "platform" but a file + // using `pf "github.com/.../platform"` wouldn't compile because + // `platform` is undefined). + pkgAlias := pkgAliasFor(file, planHelperImportPath, "platform") + + call := &ast.CallExpr{ + Fun: &ast.SelectorExpr{ + X: ast.NewIdent(pkgAlias), + Sel: ast.NewIdent("ComputePlan"), + }, + Args: []ast.Expr{ + ast.NewIdent(ctxName), + ast.NewIdent(recvName), + ast.NewIdent(desiredName), + ast.NewIdent(currentName), + }, + } + // plan, err := platform.ComputePlan(ctx, p, desired, current) + planAssign := &ast.AssignStmt{ + Lhs: []ast.Expr{ast.NewIdent("plan"), ast.NewIdent("err")}, + Tok: token.DEFINE, + Rhs: []ast.Expr{call}, + } + // return &plan, err + returnStmt := &ast.ReturnStmt{ + Results: []ast.Expr{ + &ast.UnaryExpr{Op: token.AND, X: ast.NewIdent("plan")}, + ast.NewIdent("err"), + }, + } + fn.Body = &ast.BlockStmt{List: []ast.Stmt{planAssign, returnStmt}} +} + +// ensureReceiverName returns the receiver identifier of fn, mutating +// the AST to add `defaultName` if the receiver is unnamed (e.g. +// `func (*Provider) Plan(...)`). Used by rewritePlanBody and +// rewriteApplyBody to make the substituted call site valid even on +// previously-anonymous receivers (review round-2 #3 + #4). +func ensureReceiverName(fn *ast.FuncDecl, defaultName string) string { + if fn.Recv == nil || len(fn.Recv.List) == 0 { + return defaultName + } + first := fn.Recv.List[0] + if len(first.Names) > 0 && first.Names[0].Name != "" && first.Names[0].Name != "_" { + return first.Names[0].Name + } + // Receiver is unnamed (or `_`). Inject `defaultName` so the + // rewritten call has a referent. + first.Names = []*ast.Ident{ast.NewIdent(defaultName)} + return defaultName +} + +// ensureCtxParamName returns the name of the first parameter, renaming +// blank `_` to `ctx` so the substituted call has a referent. If the +// parameter already has a non-blank name, that name is preserved and +// returned (review round-1 #2). +func ensureCtxParamName(fn *ast.FuncDecl) string { + if fn.Type.Params == nil || len(fn.Type.Params.List) < 1 { + return "ctx" + } + first := fn.Type.Params.List[0] + if len(first.Names) == 0 { + first.Names = []*ast.Ident{ast.NewIdent("ctx")} + return "ctx" + } + if len(first.Names) == 1 { + n := first.Names[0].Name + if n == "_" || n == "" { + first.Names[0] = ast.NewIdent("ctx") + return "ctx" + } + return n + } + if first.Names[0].Name != "" && first.Names[0].Name != "_" { + return first.Names[0].Name + } + first.Names[0] = ast.NewIdent("ctx") + return "ctx" +} + +// ensureImport adds an unaliased ImportSpec for `path` if one is not +// already present. Returns true if an import was added. +func ensureImport(file *ast.File, path string) bool { + return ensureImportAs(file, path, "") +} + +// ensureImportAs adds an ImportSpec for `path` with optional `alias` +// if one is not already present. If `alias` is non-empty, the spec is +// emitted as `alias "path"` so call sites referencing `alias.X` +// resolve. Round-9 #4: round-4's ensureImport injected the unaliased +// import even when the stub used a sibling-derived alias (e.g. +// `iface.IaCPlan`), leaving the rewritten file referring to undefined +// `iface`. ensureImportAs propagates the alias through. +func ensureImportAs(file *ast.File, path, alias string) bool { + for _, imp := range file.Imports { + if imp.Path == nil { + continue + } + v := strings.Trim(imp.Path.Value, `"`) + if v == path { + return false + } + } + newImport := &ast.ImportSpec{ + Path: &ast.BasicLit{Kind: token.STRING, Value: `"` + path + `"`}, + } + if alias != "" { + newImport.Name = ast.NewIdent(alias) + } + for _, decl := range file.Decls { + gd, ok := decl.(*ast.GenDecl) + if !ok || gd.Tok != token.IMPORT { + continue + } + gd.Specs = append(gd.Specs, newImport) + if !gd.Lparen.IsValid() { + gd.Lparen = gd.Pos() + gd.Rparen = gd.End() + } + return true + } + gd := &ast.GenDecl{ + Tok: token.IMPORT, + Lparen: token.NoPos, + Specs: []ast.Spec{newImport}, + } + file.Decls = append([]ast.Decl{gd}, file.Decls...) + return true +} + +// ensurePlatformImport adds a `platform` import to file if absent; +// idempotent. Used by refactor-plan rewrites which substitute +// platform.ComputePlan calls. +func ensurePlatformImport(file *ast.File) bool { + return ensureImport(file, planHelperImportPath) +} + +// ensureWfctlhelpersImport adds a `wfctlhelpers` import to file if +// absent; idempotent. Used by refactor-apply rewrites which substitute +// wfctlhelpers.ApplyPlan calls. +func ensureWfctlhelpersImport(file *ast.File) bool { + return ensureImport(file, helperImportPath) +} + +// pkgAliasFor returns the local package name used by `file` for +// `importPath`. If the file imports the path under an explicit alias +// (`pf "github.com/.../platform"`), the alias is returned; otherwise +// the package's default name is `defaultName`. If the file does not +// import the path at all, returns `defaultName` (the caller is +// expected to call ensureImport before relying on this name). +// +// Review round-3 findings #4 + #6: rev2 of refactor-plan / refactor-apply +// hardcoded "platform" / "wfctlhelpers" as the call-site selector even +// when the file already used an aliased import. ensureImport saw the +// aliased import as satisfying the path check and skipped adding a +// fresh one, leaving the rewritten code referring to an undefined +// identifier. pkgAliasFor closes that gap by selecting the right name +// at rewrite time. +func pkgAliasFor(file *ast.File, importPath, defaultName string) string { + if file == nil { + return defaultName + } + for _, imp := range file.Imports { + if imp.Path == nil { + continue + } + if strings.Trim(imp.Path.Value, `"`) != importPath { + continue + } + if imp.Name != nil { + n := imp.Name.Name + if n == "" || n == "_" || n == "." { + return defaultName + } + return n + } + return defaultName + } + return defaultName +} + +// writeFileAtomic prints `file` to a temp sibling and renames it over +// `path`. The two-step write protects against partial writes on crash: +// either the destination contains the full new contents or it remains +// unchanged. +// +// Round-11 #2: rev1 left the temp file at os.CreateTemp's default +// 0600 mode, so the rename clobbered the source's original permissions +// (e.g., 0644 → 0600). Now captures the original mode via os.Stat +// and chmods the temp file to match before the rename. +func writeFileAtomic(path string, fset *token.FileSet, file *ast.File) error { + var buf bytes.Buffer + // format.Node produces gofmt-canonical output (the same algorithm + // `go fmt` uses), which keeps the rewrite indistinguishable from a + // hand-formatted file. Plain printer.Fprint produces tab-aligned + // columns that drift from gofmt output and would look like + // codemod-touched files in code review. + if err := format.Node(&buf, fset, file); err != nil { + return err + } + return writeFileAtomicBytesPreserveMode(path, buf.Bytes()) +} + +// writeFileAtomicBytesPreserveMode is the underlying atomic-write +// helper that captures the source file's mode and applies it to the +// temp file before rename. Used by both writeFileAtomic and +// writeFileAtomicBytes so both AST-printing and raw-bytes writers +// preserve permissions (round-11 #2 + #5). +func writeFileAtomicBytesPreserveMode(path string, data []byte) error { + // Capture the source's original mode so the rename doesn't + // clobber it with CreateTemp's default 0600. + var origMode os.FileMode = 0o644 + if info, err := os.Stat(path); err == nil { + origMode = info.Mode().Perm() + } + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, "."+filepath.Base(path)+".codemod-") + if err != nil { + return err + } + tmpPath := tmp.Name() + defer func() { + // Best-effort cleanup if rename fails. + _ = os.Remove(tmpPath) + }() + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Chmod(tmpPath, origMode); err != nil { + return err + } + return os.Rename(tmpPath, path) +} + +// print renders the report. Findings/sites are sorted by file, line so +// output is deterministic across runs. +func (r *planReport) print(w io.Writer, opts *Options) { + sort.Slice(r.sites, func(i, j int) bool { + if r.sites[i].Path != r.sites[j].Path { + return r.sites[i].Path < r.sites[j].Path + } + return r.sites[i].Line < r.sites[j].Line + }) + + fmt.Fprintln(w, "# iac-codemod refactor-plan report") + fmt.Fprintln(w) + mode := "dry-run" + if opts != nil && opts.Fix { + mode = "fix" + } + fmt.Fprintf(w, "Mode: %s\n", mode) + fmt.Fprintf(w, "Sites: %d\n", len(r.sites)) + fmt.Fprintf(w, "Errors: %d\n", len(r.errors)) + fmt.Fprintln(w) + + if len(r.sites) > 0 { + // Group by classification for readability. + var canonical, nonCanonical, alreadyDelegated, skipped []planSite + for _, s := range r.sites { + switch s.Class { + case planCanonical: + canonical = append(canonical, s) + case planNonCanonical: + nonCanonical = append(nonCanonical, s) + case planAlreadyDelegated: + alreadyDelegated = append(alreadyDelegated, s) + case planSkipped: + skipped = append(skipped, s) + } + } + printSitesSection(w, "Canonical (rewrite candidate)", canonical, true) + printSitesSection(w, "Non-canonical (manual review required)", nonCanonical, false) + printSitesSection(w, "Already-delegated (no-op)", alreadyDelegated, false) + printSitesSection(w, "Skipped (// wfctl:skip-iac-codemod)", skipped, false) + } + + if len(r.errors) > 0 { + fmt.Fprintln(w, "## Errors") + fmt.Fprintln(w) + for _, e := range r.errors { + fmt.Fprintf(w, "- %s\n", e) + } + fmt.Fprintln(w) + } +} + +// printSitesSection renders one classification group. +func printSitesSection(w io.Writer, header string, sites []planSite, showRewrite bool) { + if len(sites) == 0 { + return + } + fmt.Fprintf(w, "## %s\n\n", header) + for _, s := range sites { + suffix := "" + if showRewrite && s.Rewrote { + suffix = " (rewritten)" + } + if s.Reason != "" { + fmt.Fprintf(w, "- %s:%d %s.Plan %s — %s%s\n", s.Path, s.Line, s.Receiver, s.Class, s.Reason, suffix) + } else { + fmt.Fprintf(w, "- %s:%d %s.Plan %s%s\n", s.Path, s.Line, s.Receiver, s.Class, suffix) + } + } + fmt.Fprintln(w) +} diff --git a/cmd/iac-codemod/refactor_plan_test.go b/cmd/iac-codemod/refactor_plan_test.go new file mode 100644 index 00000000..b7923b6d --- /dev/null +++ b/cmd/iac-codemod/refactor_plan_test.go @@ -0,0 +1,575 @@ +// Copyright (c) 2026 Jon Langevin +// SPDX-License-Identifier: Apache-2.0 + +// Tests in this file MUST NOT call t.Parallel(). Same global-state +// constraint as main_test.go and lint_test.go (the package-level `modes` +// map is mutated transitively through init()). + +package main + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// ============================================================ +// Golden-file source fixtures +// ============================================================ + +// canonicalPlanSrc is the configHash-compare canonical pattern T8.3 +// targets for rewrite. Modeled on the DigitalOcean DOProvider.Plan body +// at workflow-plugin-digitalocean/internal/provider.go:141 (rev1 of the +// codemod). Mutation must replace the entire body with a single +// `return wfctlhelpers.Plan(ctx, p, desired, current)` and add an import +// for the helper package if it is not already present. +const canonicalPlanSrc = `package p + +import ( + "context" + "fmt" + "time" +) + +type ResourceSpec struct{ Name string; Config map[string]any } +type ResourceState struct{ Name string; AppliedConfig map[string]any } +type IaCPlan struct{ ID string; CreatedAt time.Time; Actions []PlanAction } +type PlanAction struct{ Action string; Resource ResourceSpec; Current *ResourceState } +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type DOProvider struct{} + +func configHash(m map[string]any) string { return "" } + +// Plan computes the set of actions needed to reach the desired state. +func (p *DOProvider) Plan(_ context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + currentByName := make(map[string]ResourceState, len(current)) + for _, r := range current { + currentByName[r.Name] = r + } + + plan := &IaCPlan{ + ID: fmt.Sprintf("plan-%d", time.Now().UnixNano()), + CreatedAt: time.Now(), + } + + for _, spec := range desired { + cur, exists := currentByName[spec.Name] + if !exists { + plan.Actions = append(plan.Actions, PlanAction{ + Action: "create", + Resource: spec, + }) + continue + } + if configHash(cur.AppliedConfig) != configHash(spec.Config) { + plan.Actions = append(plan.Actions, PlanAction{ + Action: "update", + Resource: spec, + Current: &cur, + }) + } + } + return plan, nil +} + +func (p *DOProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return wfctlhelpers.ApplyPlan(ctx, p, plan) +} +func (p *DOProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +// nonCanonicalPlanSrc has out-of-template logic (an extra log call and a +// custom return shape) that T8.3 must REFUSE to rewrite. The mode emits +// a finding instead. +const nonCanonicalPlanSrc = `package p + +import ( + "context" + "fmt" +) + +type ResourceSpec struct{ Name string; Config map[string]any } +type ResourceState struct{ Name string; AppliedConfig map[string]any } +type IaCPlan struct{ Actions []PlanAction } +type PlanAction struct{ Action string; Resource ResourceSpec } +type ApplyResult struct{} + +type FooProvider struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + // Out-of-template: telemetry call + bespoke ordering logic. + fmt.Println("planning custom flow") + plan := &IaCPlan{} + for _, spec := range desired { + _ = spec + plan.Actions = append(plan.Actions, PlanAction{Action: "noop"}) + } + return plan, nil +} + +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { return nil, nil } +` + +// skippedPlanSrc carries the canonical marker on the function doc and +// must NOT be rewritten regardless of body shape. The skipped site is +// listed in the report. +const skippedPlanSrc = `package p + +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type FooProvider struct{} + +// wfctl:skip-iac-codemod legacy custom planning, see ADR-042 +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + return &IaCPlan{}, nil +} + +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { return nil, nil } +` + +// canonicalPlanUnnamedReceiverSrc — review round-2 finding #3. A +// canonical Plan method whose receiver is unnamed +// (`func (*DOProvider) Plan(...)`) must produce a rewrite that compiles: +// the rewriter must inject a receiver name (`p`) AND update the receiver +// decl so the substituted call has a real referent. +const canonicalPlanUnnamedReceiverSrc = `package p + +import ( + "context" + "fmt" + "time" +) + +type ResourceSpec struct{ Name string; Config map[string]any } +type ResourceState struct{ Name string; AppliedConfig map[string]any } +type IaCPlan struct{ ID string; CreatedAt time.Time; Actions []PlanAction } +type PlanAction struct{ Action string; Resource ResourceSpec; Current *ResourceState } +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type DOProvider struct{} + +func configHash(m map[string]any) string { return "" } + +// Unnamed receiver: ` + "`func (*DOProvider) Plan(...)`" + ` style. +func (*DOProvider) Plan(_ context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + currentByName := make(map[string]ResourceState, len(current)) + for _, r := range current { + currentByName[r.Name] = r + } + plan := &IaCPlan{ID: fmt.Sprintf("plan-%d", time.Now().UnixNano()), CreatedAt: time.Now()} + for _, spec := range desired { + cur, exists := currentByName[spec.Name] + if !exists { + plan.Actions = append(plan.Actions, PlanAction{Action: "create", Resource: spec}) + continue + } + if configHash(cur.AppliedConfig) != configHash(spec.Config) { + plan.Actions = append(plan.Actions, PlanAction{Action: "update", Resource: spec, Current: &cur}) + } + } + return plan, nil +} + +func (p *DOProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return wfctlhelpers.ApplyPlan(ctx, p, plan) +} +func (p *DOProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +func TestRefactorPlan_Fix_UnnamedReceiverGetsName(t *testing.T) { + path := writeFixture(t, "provider.go", canonicalPlanUnnamedReceiverSrc) + var stdout, stderr bytes.Buffer + if code := runRefactorPlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr); code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + gotStr := string(got) + // The receiver must have been given a name (`p`) AND the call must + // reference it. Bare ast manipulation of an unnamed receiver into + // the body would refer to a non-existent `p` — test pins the fix. + if !strings.Contains(gotStr, "func (p *DOProvider) Plan(") { + t.Errorf("rewrite must inject a receiver name on previously-anonymous receivers; got:\n%s", gotStr) + } + if !strings.Contains(gotStr, "platform.ComputePlan(ctx, p,") { + t.Errorf("rewrite must reference the injected receiver name; got:\n%s", gotStr) + } +} + +// canonicalPlanCustomCtxNameSrc — review round-1 finding #2 regression +// test (non-blank ctx-param name preserved). The rewriter must NOT +// rename `c` to `ctx`; the substituted call must reference `c`. +const canonicalPlanCustomCtxNameSrc = `package p + +import ( + "context" + "fmt" + "time" +) + +type ResourceSpec struct{ Name string; Config map[string]any } +type ResourceState struct{ Name string; AppliedConfig map[string]any } +type IaCPlan struct{ ID string; CreatedAt time.Time; Actions []PlanAction } +type PlanAction struct{ Action string; Resource ResourceSpec; Current *ResourceState } +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type DOProvider struct{} + +func configHash(m map[string]any) string { return "" } + +func (p *DOProvider) Plan(c context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + currentByName := make(map[string]ResourceState, len(current)) + for _, r := range current { + currentByName[r.Name] = r + } + plan := &IaCPlan{ID: fmt.Sprintf("plan-%d", time.Now().UnixNano()), CreatedAt: time.Now()} + for _, spec := range desired { + cur, exists := currentByName[spec.Name] + if !exists { + plan.Actions = append(plan.Actions, PlanAction{Action: "create", Resource: spec}) + continue + } + if configHash(cur.AppliedConfig) != configHash(spec.Config) { + plan.Actions = append(plan.Actions, PlanAction{Action: "update", Resource: spec, Current: &cur}) + } + } + return plan, nil +} + +func (p *DOProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return wfctlhelpers.ApplyPlan(ctx, p, plan) +} +func (p *DOProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +func TestRefactorPlan_Fix_PreservesCustomCtxName(t *testing.T) { + path := writeFixture(t, "provider.go", canonicalPlanCustomCtxNameSrc) + var stdout, stderr bytes.Buffer + if code := runRefactorPlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr); code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + gotStr := string(got) + if !strings.Contains(gotStr, "Plan(c context.Context") { + t.Errorf("rewrite should preserve the custom ctx-param name `c`; got:\n%s", gotStr) + } + if !strings.Contains(gotStr, "platform.ComputePlan(c, p,") { + t.Errorf("substituted call must reference the original ctx name `c`, not `ctx`; got:\n%s", gotStr) + } +} + +// canonicalPlanWithExtraLoggingSrc — review round-1 finding #3. A Plan +// body whose desired-loop has an additional logging call (a real-world +// bespoke planner) must NOT be classified as canonical: silently +// rewriting it would drop the log line. +const canonicalPlanWithExtraLoggingSrc = `package p + +import ( + "context" + "fmt" + "time" +) + +type ResourceSpec struct{ Name string; Config map[string]any } +type ResourceState struct{ Name string; AppliedConfig map[string]any } +type IaCPlan struct{ ID string; CreatedAt time.Time; Actions []PlanAction } +type PlanAction struct{ Action string; Resource ResourceSpec; Current *ResourceState } +type ApplyResult struct{} +type PlanDiagnostic struct{} + +type DOProvider struct{} + +func configHash(m map[string]any) string { return "" } + +func (p *DOProvider) Plan(_ context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + currentByName := make(map[string]ResourceState, len(current)) + for _, r := range current { + currentByName[r.Name] = r + } + + plan := &IaCPlan{ + ID: fmt.Sprintf("plan-%d", time.Now().UnixNano()), + CreatedAt: time.Now(), + } + + for _, spec := range desired { + fmt.Println("planning:", spec.Name) // BESPOKE TELEMETRY — must not be silently dropped + cur, exists := currentByName[spec.Name] + if !exists { + plan.Actions = append(plan.Actions, PlanAction{Action: "create", Resource: spec}) + continue + } + if configHash(cur.AppliedConfig) != configHash(spec.Config) { + plan.Actions = append(plan.Actions, PlanAction{Action: "update", Resource: spec, Current: &cur}) + } + } + return plan, nil +} + +func (p *DOProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { + return wfctlhelpers.ApplyPlan(ctx, p, plan) +} +func (p *DOProvider) ValidatePlan(plan *IaCPlan) []PlanDiagnostic { return nil } +` + +func TestRefactorPlan_ExtraLoggingNotCanonical(t *testing.T) { + path := writeFixture(t, "provider.go", canonicalPlanWithExtraLoggingSrc) + var stdout, stderr bytes.Buffer + code := runRefactorPlan([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if strings.Contains(out, "DOProvider.Plan canonical") { + t.Errorf("Plan body with extra side-effect (telemetry) must NOT be classified canonical; got:\n%s", out) + } + if !strings.Contains(out, "non-canonical") { + t.Errorf("Plan body with extra side-effect should be reported non-canonical; got:\n%s", out) + } +} + +// alreadyDelegatedPlanSrc has a Plan body that is the canonical +// 2-statement delegation to platform.ComputePlan (the rev2 form which +// bridges the value/pointer mismatch from review round-2 finding #1). +// The mode must NOT report it as non-canonical and must NOT mutate it +// (idempotent). +const alreadyDelegatedPlanSrc = `package p + +import "context" + +type ResourceSpec struct{} +type ResourceState struct{} +type IaCPlan struct{} +type ApplyResult struct{} +type FooProvider struct{} + +func (p *FooProvider) Plan(ctx context.Context, desired []ResourceSpec, current []ResourceState) (*IaCPlan, error) { + plan, err := platform.ComputePlan(ctx, p, desired, current) + return &plan, err +} + +func (p *FooProvider) Apply(ctx context.Context, plan *IaCPlan) (*ApplyResult, error) { return nil, nil } +` + +// ============================================================ +// Helpers +// ============================================================ + +// writeFixture writes src to a fresh tempdir, returning the path. +func writeFixture(t *testing.T, name, src string) string { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte(src), 0o644); err != nil { + t.Fatalf("write fixture %s: %v", path, err) + } + return path +} + +// ============================================================ +// Detection / reporting (dry-run) +// ============================================================ + +func TestRefactorPlan_DryRun_DetectsCanonical(t *testing.T) { + path := writeFixture(t, "provider.go", canonicalPlanSrc) + var stdout, stderr bytes.Buffer + code := runRefactorPlan([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "DOProvider.Plan") { + t.Errorf("report should name DOProvider.Plan; got:\n%s", out) + } + if !strings.Contains(out, "canonical") { + t.Errorf("report should mark site as canonical (rewrite candidate); got:\n%s", out) + } + // Dry-run must not mutate. + got, _ := os.ReadFile(path) + if string(got) != canonicalPlanSrc { + t.Errorf("dry-run modified the file; expected no mutation") + } +} + +func TestRefactorPlan_DryRun_ReportsNonCanonical(t *testing.T) { + path := writeFixture(t, "provider.go", nonCanonicalPlanSrc) + var stdout, stderr bytes.Buffer + code := runRefactorPlan([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "FooProvider.Plan") { + t.Errorf("report should name FooProvider.Plan; got:\n%s", out) + } + if !strings.Contains(out, "non-canonical") { + t.Errorf("report should mark site as non-canonical; got:\n%s", out) + } +} + +func TestRefactorPlan_DryRun_HonorsSkipMarker(t *testing.T) { + path := writeFixture(t, "provider.go", skippedPlanSrc) + var stdout, stderr bytes.Buffer + code := runRefactorPlan([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "Skipped") { + t.Errorf("report should have a Skipped section; got:\n%s", out) + } + if !strings.Contains(out, "FooProvider.Plan") { + t.Errorf("Skipped section should list FooProvider.Plan; got:\n%s", out) + } +} + +func TestRefactorPlan_DryRun_AlreadyDelegatedReportedAsNoop(t *testing.T) { + path := writeFixture(t, "provider.go", alreadyDelegatedPlanSrc) + var stdout, stderr bytes.Buffer + code := runRefactorPlan([]string{path}, &Options{DryRun: true, Fix: false}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + out := stdout.String() + if strings.Contains(out, "non-canonical") { + t.Errorf("already-delegated Plan should NOT be reported non-canonical; got:\n%s", out) + } + if !strings.Contains(out, "already-delegated") { + t.Errorf("already-delegated should be classified explicitly; got:\n%s", out) + } +} + +// ============================================================ +// Mutation (-fix) +// ============================================================ + +func TestRefactorPlan_Fix_RewritesCanonical(t *testing.T) { + path := writeFixture(t, "provider.go", canonicalPlanSrc) + var stdout, stderr bytes.Buffer + code := runRefactorPlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read after fix: %v", err) + } + gotStr := string(got) + // Round-2 finding #1: platform.ComputePlan returns IaCPlan by value; + // provider Plan returns *IaCPlan. The rewrite uses the 2-statement + // form (`plan, err := platform.ComputePlan(...); return &plan, err`) + // to bridge the value/pointer mismatch. + if !strings.Contains(gotStr, "plan, err := platform.ComputePlan(ctx, p, desired, current)") { + t.Errorf("rewritten body should call platform.ComputePlan via 2-statement form; got:\n%s", gotStr) + } + if !strings.Contains(gotStr, "return &plan, err") { + t.Errorf("rewritten body should return &plan, err to bridge value→pointer; got:\n%s", gotStr) + } + if strings.Contains(gotStr, "currentByName := make(") { + t.Errorf("canonical body should be removed by rewrite; got:\n%s", gotStr) + } + // Helper import must be present after rewrite. + if !strings.Contains(gotStr, `"github.com/GoCodeAlone/workflow/platform"`) { + t.Errorf("rewrite should add platform import; got:\n%s", gotStr) + } +} + +func TestRefactorPlan_Fix_RenamesBlankReceiverParamSoCtxResolves(t *testing.T) { + // The DO provider declares Plan(_ context.Context, ...) and after + // rewrite the body must reference the ctx parameter. The codemod + // renames the blank `_` parameter to `ctx` so the substituted call + // compiles. Pinned regression: if the renamer is dropped, the + // rewritten file fails to type-check. + path := writeFixture(t, "provider.go", canonicalPlanSrc) + var stdout, stderr bytes.Buffer + if code := runRefactorPlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr); code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if !strings.Contains(string(got), "Plan(ctx context.Context") { + t.Errorf("blank ctx param should be renamed to ctx so the rewritten body compiles; got:\n%s", string(got)) + } +} + +func TestRefactorPlan_Fix_DoesNotRewriteNonCanonical(t *testing.T) { + path := writeFixture(t, "provider.go", nonCanonicalPlanSrc) + var stdout, stderr bytes.Buffer + code := runRefactorPlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != nonCanonicalPlanSrc { + t.Errorf("non-canonical body must NOT be rewritten; file changed:\n%s", string(got)) + } +} + +func TestRefactorPlan_Fix_HonorsSkipMarker(t *testing.T) { + path := writeFixture(t, "provider.go", skippedPlanSrc) + var stdout, stderr bytes.Buffer + code := runRefactorPlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != skippedPlanSrc { + t.Errorf("skip-marker'd body must NOT be rewritten; file changed:\n%s", string(got)) + } +} + +func TestRefactorPlan_Fix_IdempotentOnAlreadyDelegated(t *testing.T) { + path := writeFixture(t, "provider.go", alreadyDelegatedPlanSrc) + var stdout, stderr bytes.Buffer + if code := runRefactorPlan([]string{path}, &Options{DryRun: false, Fix: true}, &stdout, &stderr); code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != alreadyDelegatedPlanSrc { + t.Errorf("already-delegated source must be byte-identical after fix (idempotent); diff:\nbefore:\n%s\nafter:\n%s", alreadyDelegatedPlanSrc, string(got)) + } +} + +// ============================================================ +// Mutation-gate negative tests (T8.1 review pattern) +// ============================================================ + +// TestRefactorPlan_DryRunFalseWithoutFix_DoesNotMutate pins the dispatcher +// gate from main_test.go: a user-supplied -dry-run=false without -fix must +// NOT bypass mutation. The mode is invoked via run() so dispatcher +// normalization runs; we then verify file mtime and content unchanged. +func TestRefactorPlan_DryRunFalseWithoutFix_DoesNotMutate(t *testing.T) { + path := writeFixture(t, "provider.go", canonicalPlanSrc) + stat0, _ := os.Stat(path) + mtime0 := stat0.ModTime() + + // Sleep 1 nanosecond worth of mtime resolution? We use file mtime AND + // content equality; either being unchanged is sufficient. For + // portability across filesystems, we don't require sub-second mtime + // granularity — we assert content unchanged AND the dispatcher + // normalized DryRun=true. + time.Sleep(10 * time.Millisecond) + + var stdout, stderr bytes.Buffer + code := run([]string{"refactor-plan", "-dry-run=false", path}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit = %d, want 0; stderr=%q", code, stderr.String()) + } + got, _ := os.ReadFile(path) + if string(got) != canonicalPlanSrc { + t.Errorf("file must NOT be mutated when -dry-run=false is passed without -fix; content changed:\n%s", string(got)) + } + stat1, _ := os.Stat(path) + if !stat1.ModTime().Equal(mtime0) { + t.Errorf("file mtime should be unchanged; before=%v after=%v", mtime0, stat1.ModTime()) + } +} diff --git a/go.mod b/go.mod index 1b046dbd..2e07b392 100644 --- a/go.mod +++ b/go.mod @@ -76,6 +76,7 @@ require ( golang.org/x/term v0.42.0 golang.org/x/text v0.36.0 golang.org/x/time v0.15.0 + golang.org/x/tools v0.43.0 google.golang.org/api v0.272.0 google.golang.org/grpc v1.80.0 google.golang.org/protobuf v1.36.11 @@ -301,7 +302,6 @@ require ( go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/net v0.53.0 // indirect golang.org/x/sys v0.43.0 // indirect - golang.org/x/tools v0.43.0 // indirect google.golang.org/genproto v0.0.0-20260319201613-d00831a3d3e7 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260406210006-6f92a3bedf2d // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260406210006-6f92a3bedf2d // indirect