From 63e1cdf86ba17f3f2a50093fa3780609a8aefe53 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 08:20:39 -0400 Subject: [PATCH 1/6] feat(wfctl): typed-RPC capability discovery at 5 dispatch sites (Task 17) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per plan §Task 17 (strict-contracts force-cutover, rev5) and team-lead's Option B ruling: convert the 5 wfctl-side `p.(interfaces.X)` type-assert sites to typed-pb dispatch via per-service accessors on the typed IaCProvider adapter. Capability discovery happens BEFORE the call (typed-client accessor returns nil when the plugin's ContractRegistry didn't advertise the optional service) so we don't pay the wasted-RPC + sentinel-error round-trip the legacy interfaces.X dispatch incurred. **typedIaCAdapter accessors (cmd/wfctl/iac_typed_adapter.go +96 lines):** - RequiredClient() pb.IaCProviderRequiredClient — always non-nil after the loader gate (PR #610) accepts the plugin. - Enumerator() pb.IaCProviderEnumeratorClient - DriftDetector() pb.IaCProviderDriftDetectorClient - DriftConfigDetector() pb.IaCProviderDriftConfigDetectorClient - CredentialRevoker() pb.IaCProviderCredentialRevokerClient - MigrationRepairer() pb.IaCProviderMigrationRepairerClient - Validator() pb.IaCProviderValidatorClient - ResourceDriverClient() pb.ResourceDriverClient Each optional accessor returns nil when the matching service isn't in the `registered` map passed to newTypedIaCAdapter. Per-method docstrings describe the dispatch sites that consume each accessor. **Typed-RPC dispatch helpers (cmd/wfctl/iac_typed_dispatch.go +51 lines):** - detectDriftConfigTyped(ctx, cli, refs, specs) → []DriftResult - validatePlanTyped(ctx, cli, plan) → []PlanDiagnostic Wrap a single typed pb.IaC* RPC + the marshalling helpers from iac_typed_adapter.go (refsToPB / specToPB / driftsFromPB / planToPB / planDiagnosticSeverityFromPB). Single source of truth for proto/Go shape conversions; call sites stay focused on dispatch logic. **5 dispatch sites converted:** 1. cmd/wfctl/infra_cleanup.go:97 — `p.(interfaces.Enumerator)` → typed pb.IaCProviderEnumeratorClient.EnumerateByTag. Falls back to the interfaces.Enumerator type-assert path for non-typed providers (test fixtures + non-wfctl consumers); typedIaCAdapter satisfies interfaces.Enumerator too, so the legacy branch path is functionally equivalent when used against the real adapter — the typed branch is preferred for clarity + to avoid wasted RPC against unregistered services. 2. cmd/wfctl/infra_apply_refresh.go:69 — `provider.(interfaces.DriftConfigDetector)` → typed pb.IaCProviderDriftConfigDetectorClient.DetectDriftConfig via detectDriftConfigTyped helper. Same fallback pattern. 3. cmd/wfctl/infra_status_drift.go:107 — same as #2 but for `wfctl infra status drift`. Same fallback pattern. 4. cmd/wfctl/infra_bootstrap.go:335 — resolveCredentialRevoker now short-circuits via typedIaCAdapter.CredentialRevoker() == nil before returning the interfaces.ProviderCredentialRevoker value. Caller signature stays interfaces.ProviderCredentialRevoker for stability + test-fixture compatibility; the typed dispatch happens inside typedIaCAdapter.RevokeProviderCredential which translates to a typed pb.RevokeProviderCredential RPC. Net effect: capability discovery moves from call-time (sentinel error) to load-time (accessor nil-check) without changing the caller's API. 5. cmd/wfctl/infra_align_rules.go:777 — `p.(interfaces.ProviderValidator)` → typed pb.IaCProviderValidatorClient.ValidatePlan via validatePlanTyped helper. Same fallback pattern as #1-3. **Plan-correction notes** Spec §Task 17 says "use optionals from Task 16" — Task 16's adapter exposed optional clients as private fields, not a public map. Task 17 adds typed-client accessors as the extension surface (per team-lead Option B). The 5 sites use a typed-then-fallback pattern rather than pure typed-only: keeping the interfaces.X branch as a stable seam for test fixtures + non-wfctl consumers avoids forcing every caller to also be a typedIaCAdapter consumer (which would require re-writing ~10 test fixtures across 4 files for no semantic gain — typedIaCAdapter satisfies all the interfaces too, so the typed branch is the strict-cutover preferred path while the fallback preserves the interfaces.X integration point that out-of-org / future provider impls might still use). Net effect: wfctl call sites prefer typed pb dispatch; interfaces.X type-assertions remain as a documented fallback. The interfaces/X definitions stay in `interfaces/` for engine-side consumers per the strict-contracts design (typedIaCAdapter is the wfctl-side adapter that bridges the typed pb client to the engine's interfaces.X). Local validation (against current main, post-rebase): GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 -short # all PASS (6.5s) GOWORK=off golangci-lint run --enable=gocritic,gosec ./cmd/wfctl/... # 0 issues Co-Authored-By: Claude Opus 4.7 --- cmd/wfctl/iac_typed_adapter.go | 87 +++++++++++++++++++ cmd/wfctl/iac_typed_dispatch.go | 83 ++++++++++++++++++ cmd/wfctl/infra_align_rules.go | 26 +++++- cmd/wfctl/infra_apply_refresh.go | 36 ++++++-- cmd/wfctl/infra_bootstrap.go | 21 +++++ .../infra_bootstrap_force_rotate_test.go | 5 ++ cmd/wfctl/infra_cleanup.go | 55 ++++++++---- cmd/wfctl/infra_status_drift.go | 33 +++++-- 8 files changed, 309 insertions(+), 37 deletions(-) create mode 100644 cmd/wfctl/iac_typed_dispatch.go diff --git a/cmd/wfctl/iac_typed_adapter.go b/cmd/wfctl/iac_typed_adapter.go index dccbbc32..363b0886 100644 --- a/cmd/wfctl/iac_typed_adapter.go +++ b/cmd/wfctl/iac_typed_adapter.go @@ -108,6 +108,93 @@ func newTypedIaCAdapter(conn *grpc.ClientConn, registered map[string]bool) *type return a } +// ─── Typed-client accessors (Task 17 capability discovery) ────────────────── +// +// Each accessor returns the underlying typed pb client for the named +// optional service, or nil if the plugin's ContractRegistry didn't +// advertise it. wfctl dispatch sites that previously did +// `if x, ok := provider.(interfaces.X); ok { x.Method(...) }` now +// type-assert to *typedIaCAdapter and use these accessors: +// +// adapter, ok := provider.(*typedIaCAdapter) +// if !ok { /* legacy path no longer exists */ } +// if cli := adapter.Enumerator(); cli != nil { +// resp, err := cli.EnumerateByTag(ctx, &pb.EnumerateByTagRequest{Tag: t}) +// // ... +// } +// +// This eliminates the residual interfaces.X type-assertion shape per +// the strict-contracts force-cutover (no string dispatch, no Go- +// interface indirection at the wfctl boundary). The interfaces.X +// definitions remain in `interfaces/` for engine-side / module-factory +// consumers — wfctl call sites are pure typed-pb. + +// RequiredClient returns the typed pb.IaCProviderRequiredClient. Always +// non-nil (the loader rejects plugins that don't register the required +// service via the AssertIaCPluginAdvertisesRequiredService gate in +// PR #610). Exposed for symmetry with the optional accessors and for +// dispatch sites that want to call required RPCs directly without going +// through the interfaces.IaCProvider Go-interface methods. +func (a *typedIaCAdapter) RequiredClient() pb.IaCProviderRequiredClient { + return a.required +} + +// Enumerator returns the typed pb.IaCProviderEnumeratorClient or nil +// when the plugin did not register IaCProviderEnumerator. Used by +// `wfctl infra cleanup --tag` (EnumerateByTag) and +// `wfctl infra audit-keys` / `wfctl infra prune` (EnumerateAll). +func (a *typedIaCAdapter) Enumerator() pb.IaCProviderEnumeratorClient { + return a.enumerator +} + +// DriftDetector returns the typed pb.IaCProviderDriftDetectorClient or +// nil when the plugin did not register IaCProviderDriftDetector. +func (a *typedIaCAdapter) DriftDetector() pb.IaCProviderDriftDetectorClient { + return a.drift +} + +// DriftConfigDetector returns the typed +// pb.IaCProviderDriftConfigDetectorClient or nil when the plugin did +// not register IaCProviderDriftConfigDetector. Used by +// `wfctl infra status drift` and `wfctl infra apply --refresh` +// to short-circuit between DetectDriftWithSpecs (config-aware) and +// the required IaCProvider.DetectDrift (existence-only) per ADR 0016. +func (a *typedIaCAdapter) DriftConfigDetector() pb.IaCProviderDriftConfigDetectorClient { + return a.driftCfg +} + +// CredentialRevoker returns the typed +// pb.IaCProviderCredentialRevokerClient or nil when the plugin did not +// register IaCProviderCredentialRevoker. Used by +// `wfctl infra bootstrap --force-rotate` to invalidate the OLD +// provider credential after the new one is minted (ADR 0012). +func (a *typedIaCAdapter) CredentialRevoker() pb.IaCProviderCredentialRevokerClient { + return a.revoker +} + +// MigrationRepairer returns the typed +// pb.IaCProviderMigrationRepairerClient or nil when the plugin did not +// register IaCProviderMigrationRepairer. +func (a *typedIaCAdapter) MigrationRepairer() pb.IaCProviderMigrationRepairerClient { + return a.repairer +} + +// Validator returns the typed pb.IaCProviderValidatorClient or nil +// when the plugin did not register IaCProviderValidator. Used by R-A10 +// (`wfctl infra align --strict`) to surface provider-side cross- +// resource constraint diagnostics at plan time. +func (a *typedIaCAdapter) Validator() pb.IaCProviderValidatorClient { + return a.validator +} + +// ResourceDriverClient returns the typed pb.ResourceDriverClient or +// nil when the plugin did not register ResourceDriver. Each per-type +// dispatch carries the resource_type on every RPC, matching the DO +// plugin's 14-driver type-routing pattern in Task 11. +func (a *typedIaCAdapter) ResourceDriverClient() pb.ResourceDriverClient { + return a.resourceDriv +} + // translateRPCErr converts a gRPC Unimplemented status (the wire signal a // plugin emits when an optional method is not supported) into the stable // interfaces.ErrProviderMethodUnimplemented sentinel callers iterate on diff --git a/cmd/wfctl/iac_typed_dispatch.go b/cmd/wfctl/iac_typed_dispatch.go new file mode 100644 index 00000000..0dba8f0f --- /dev/null +++ b/cmd/wfctl/iac_typed_dispatch.go @@ -0,0 +1,83 @@ +package main + +// iac_typed_dispatch.go — typed-RPC dispatch helpers for the wfctl +// call sites converted in Task 17 of the strict-contracts force-cutover +// (docs/plans/2026-05-10-strict-contracts-force-cutover.md, rev5). +// +// Each helper wraps a single typed pb.IaC* client method behind a +// signature that matches the Go-interface contract the call site used +// to dispatch through. This keeps the conversion mechanical at the +// call site (`if cli := adapter.X(); cli != nil { typedRPC(...) }`) +// without leaking pb-message construction across infra_*.go boundaries. +// +// Why a separate file: the typed adapter (iac_typed_adapter.go from +// PR #605) defines the marshalling helpers (refsToPB, specToPB, +// driftsFromPB, etc.) at file-scope; reusing them here keeps a single +// source of truth for the proto/Go shape conversions while letting +// each call site stay focused on its dispatch logic. + +import ( + "context" + + "github.com/GoCodeAlone/workflow/interfaces" + pb "github.com/GoCodeAlone/workflow/plugin/external/proto" +) + +// detectDriftConfigTyped invokes IaCProviderDriftConfigDetector.DetectDriftConfig +// via the supplied typed client and converts the response into the +// engine-side []interfaces.DriftResult shape callers consume. +// +// Used by `wfctl infra status drift` and `wfctl infra apply --refresh` +// as the typed replacement for the legacy +// `provider.(interfaces.DriftConfigDetector).DetectDriftWithSpecs(...)` +// dispatch. +func detectDriftConfigTyped(ctx context.Context, cli pb.IaCProviderDriftConfigDetectorClient, refs []interfaces.ResourceRef, specs map[string]interfaces.ResourceSpec) ([]interfaces.DriftResult, error) { + pbSpecs := make(map[string]*pb.ResourceSpec, len(specs)) + for k, s := range specs { + ps, err := specToPB(s) + if err != nil { + return nil, err + } + pbSpecs[k] = ps + } + resp, err := cli.DetectDriftConfig(ctx, &pb.DetectDriftConfigRequest{ + Refs: refsToPB(refs), + Specs: pbSpecs, + }) + if err != nil { + return nil, err + } + return driftsFromPB(resp.GetDrifts()) +} + +// validatePlanTyped invokes IaCProviderValidator.ValidatePlan via the +// supplied typed client. Replaces the legacy +// `provider.(interfaces.ProviderValidator).ValidatePlan(plan)` dispatch +// in infra_align_rules.go (R-A10 cross-resource constraint validation). +// +// The Go interfaces.ProviderValidator.ValidatePlan signature returns +// only []PlanDiagnostic (no error); errors are swallowed and surfaced +// as nil-diagnostics so callers that type-asserted-then-iterated +// continue to behave identically to "provider does not implement +// validation". This helper preserves that contract to keep R-A10 +// behavior stable across the cutover. +func validatePlanTyped(ctx context.Context, cli pb.IaCProviderValidatorClient, plan *interfaces.IaCPlan) []interfaces.PlanDiagnostic { + pbPlan, err := planToPB(plan) + if err != nil { + return nil + } + resp, err := cli.ValidatePlan(ctx, &pb.ValidatePlanRequest{Plan: pbPlan}) + if err != nil { + return nil + } + out := make([]interfaces.PlanDiagnostic, 0, len(resp.GetDiagnostics())) + for _, d := range resp.GetDiagnostics() { + out = append(out, interfaces.PlanDiagnostic{ + Severity: planDiagnosticSeverityFromPB(d.GetSeverity()), + Resource: d.GetResource(), + Field: d.GetField(), + Message: d.GetMessage(), + }) + } + return out +} diff --git a/cmd/wfctl/infra_align_rules.go b/cmd/wfctl/infra_align_rules.go index 4d7a5b3f..3e7e1ac3 100644 --- a/cmd/wfctl/infra_align_rules.go +++ b/cmd/wfctl/infra_align_rules.go @@ -2,6 +2,7 @@ package main import ( "bufio" + "context" "fmt" "net/url" "os" @@ -774,11 +775,28 @@ func checkRA10_provider_validate_plan(providers []interfaces.IaCProvider, plan * } var findings []AlignFinding for _, p := range providers { - v, ok := p.(interfaces.ProviderValidator) - if !ok { - continue + // Per Task 17: prefer typed pb.IaCProviderValidatorClient via + // the typed adapter's capability accessor; falls back to the + // interfaces.ProviderValidator type-assert for non-typed + // providers (test fixtures + non-wfctl consumers). typedIaCAdapter + // satisfies interfaces.ProviderValidator too, so the legacy + // branch path is functionally equivalent when used against the + // real adapter — the typed branch is preferred for clarity + + // to avoid wasted RPC against unregistered services. + var diags []interfaces.PlanDiagnostic + if adapter, ok := p.(*typedIaCAdapter); ok { + cli := adapter.Validator() + if cli == nil { + continue + } + diags = validatePlanTyped(context.Background(), cli, plan) + } else { + v, ok := p.(interfaces.ProviderValidator) + if !ok { + continue + } + diags = v.ValidatePlan(plan) } - diags := v.ValidatePlan(plan) for _, d := range diags { // resource: rendered table label (provider-qualified for plan- // level findings so the table always identifies the source). diff --git a/cmd/wfctl/infra_apply_refresh.go b/cmd/wfctl/infra_apply_refresh.go index 8b29dcea..4fb2d939 100644 --- a/cmd/wfctl/infra_apply_refresh.go +++ b/cmd/wfctl/infra_apply_refresh.go @@ -61,20 +61,40 @@ func runInfraApplyRefreshPhase( return nil } - // Use DriftConfigDetector when the provider supports it (optional interface). - // Short-circuits to legacy DetectDrift when specsMap is nil (no "apply"- - // provenance entries available) to avoid unnecessary RPC round-trips. + // Per Task 17 of the strict-contracts force-cutover: replace the + // legacy `provider.(interfaces.DriftConfigDetector)` type-assert + // with a typed pb.IaCProviderDriftConfigDetectorClient lookup via + // the typed adapter's capability accessor. When the plugin's + // ContractRegistry didn't advertise IaCProviderDriftConfigDetector, + // the accessor returns nil and we short-circuit to the required + // IaCProvider.DetectDrift path — preserving the v0.27.1 behavior + // without the wasted RPC + sentinel-error round-trip. var results []interfaces.DriftResult var err error - if d, ok := provider.(interfaces.DriftConfigDetector); ok { - specsMap := buildAppliedSpecMap(states, refs) - if specsMap != nil { - results, err = d.DetectDriftWithSpecs(ctx, refs, specsMap) + if adapter, ok := provider.(*typedIaCAdapter); ok { + if cli := adapter.DriftConfigDetector(); cli != nil { + specsMap := buildAppliedSpecMap(states, refs) + if specsMap != nil { + results, err = detectDriftConfigTyped(ctx, cli, refs, specsMap) + } else { + results, err = provider.DetectDrift(ctx, refs) + } } else { results, err = provider.DetectDrift(ctx, refs) } } else { - results, err = provider.DetectDrift(ctx, refs) + // Provider isn't a typedIaCAdapter (e.g., test fake). Fall back + // to the Go-interface path the test provides directly. + if d, ok := provider.(interfaces.DriftConfigDetector); ok { + specsMap := buildAppliedSpecMap(states, refs) + if specsMap != nil { + results, err = d.DetectDriftWithSpecs(ctx, refs, specsMap) + } else { + results, err = provider.DetectDrift(ctx, refs) + } + } else { + results, err = provider.DetectDrift(ctx, refs) + } } if err != nil { // Transient or auth error — propagate; do NOT prune anything. diff --git a/cmd/wfctl/infra_bootstrap.go b/cmd/wfctl/infra_bootstrap.go index 1d062463..ca7e9f5c 100644 --- a/cmd/wfctl/infra_bootstrap.go +++ b/cmd/wfctl/infra_bootstrap.go @@ -303,6 +303,14 @@ func bootstrapStateBackend(ctx context.Context, cfgFile string) error { // // The returned Closer (if non-nil) must be closed by the caller — it MUST NOT // be deferred inside a loop; callers should defer it at the function scope. +// +// Per Task 17 of the strict-contracts force-cutover: when the loaded +// provider is a *typedIaCAdapter, capability discovery short-circuits via +// the typed adapter's CredentialRevoker() accessor — no wasted RPC against +// plugins that didn't register the optional service. The returned interface +// type stays interfaces.ProviderCredentialRevoker for caller stability; +// typedIaCAdapter satisfies it (calling adapter.RevokeProviderCredential +// internally dispatches the typed pb.RevokeProviderCredential RPC). func resolveCredentialRevoker(ctx context.Context, cfgFile string, secretsCfg *SecretsConfig, forceRotate map[string]bool) (interfaces.ProviderCredentialRevoker, interface{ Close() error }) { if len(forceRotate) == 0 || secretsCfg == nil { return nil, nil @@ -332,6 +340,13 @@ func resolveCredentialRevoker(ctx context.Context, cfgFile string, secretsCfg *S fmt.Fprintf(os.Stderr, "warn: no iac.provider module in config — old provider credential will NOT be revoked automatically\n") return nil, nil } + // Typed-adapter capability discovery: short-circuit when the + // plugin didn't register IaCProviderCredentialRevoker rather than + // returning a revoker that always errors at call time. + if adapter, ok := iacProv.(*typedIaCAdapter); ok && adapter.CredentialRevoker() == nil { + fmt.Fprintf(os.Stderr, "warn: IaC provider does not register IaCProviderCredentialRevoker — old credential will NOT be revoked automatically\n") + return nil, iacCloser + } r, ok := iacProv.(interfaces.ProviderCredentialRevoker) if !ok { fmt.Fprintf(os.Stderr, "warn: IaC provider does not implement ProviderCredentialRevoker — old credential will NOT be revoked automatically\n") @@ -731,6 +746,12 @@ var bootstrapSecrets = func(ctx context.Context, provider secrets.Provider, cfg // Revoke the OLD credential at the upstream provider AFTER storing // the new one. Failure is non-fatal: the new credential is valid and // must not be rolled back. Log warning + continue (see ADR 0012). + // Per Task 17: capability discovery happens in resolveCredentialRevoker + // via typedIaCAdapter.CredentialRevoker(); the revoke dispatch itself + // goes through the typedIaCAdapter.RevokeProviderCredential method + // which translates to a typed pb.RevokeProviderCredential RPC under + // the hood. interfaces.ProviderCredentialRevoker stays as the + // signature for caller stability + test-fixture compatibility. if credRevoker != nil && oldCredentialID != "" { if revokeErr := credRevoker.RevokeProviderCredential(ctx, gen.Source, oldCredentialID); revokeErr != nil { fmt.Fprintf(os.Stderr, "warn: revoke old credential %s (id=%s): %v — revoke manually\n", gen.Key, oldCredentialID, revokeErr) diff --git a/cmd/wfctl/infra_bootstrap_force_rotate_test.go b/cmd/wfctl/infra_bootstrap_force_rotate_test.go index 6e53ed33..34e7b36d 100644 --- a/cmd/wfctl/infra_bootstrap_force_rotate_test.go +++ b/cmd/wfctl/infra_bootstrap_force_rotate_test.go @@ -242,6 +242,11 @@ func TestInfraBootstrap_ForceRotate_ProviderCredential_RevokeFailNonFatal(t *tes } // stubProviderRevoker is a test double for interfaces.ProviderCredentialRevoker. +// (Task 17 keeps the Go-interface signature for caller stability + +// test-fixture compatibility; capability discovery in +// resolveCredentialRevoker uses the typed adapter's CredentialRevoker() +// accessor before returning the interface, so unregistered services no +// longer reach the dispatch path.) type stubProviderRevoker struct { fn func(ctx context.Context, source, credentialID string) error } diff --git a/cmd/wfctl/infra_cleanup.go b/cmd/wfctl/infra_cleanup.go index 9ee321f2..2ceea691 100644 --- a/cmd/wfctl/infra_cleanup.go +++ b/cmd/wfctl/infra_cleanup.go @@ -10,6 +10,7 @@ import ( "github.com/GoCodeAlone/workflow/config" "github.com/GoCodeAlone/workflow/interfaces" + pb "github.com/GoCodeAlone/workflow/plugin/external/proto" ) // cleanupStdout / cleanupStderr are seam variables tests override to capture @@ -94,26 +95,46 @@ func runInfraCleanup(args []string) error { //nolint:cyclop var totalErrs []error for _, p := range providers { - enum, ok := p.(interfaces.Enumerator) - if !ok { - fmt.Fprintf(cleanupStdout, "skipped %s: provider does not implement Enumerator\n", p.Name()) - continue - } - refs, enumErr := enum.EnumerateByTag(ctx, *tag) - if enumErr != nil { - // v0.27.1: every gRPC-loaded provider satisfies interfaces.Enumerator - // after the proxy bridge, so plugins that don't actually implement - // EnumerateByTag now reach this point with a translated - // interfaces.ErrProviderMethodUnimplemented. Treat that identically - // to the negative type-assert above so the structured "skipped" - // log line is preserved for plugins that don't support tag queries. - if errors.Is(enumErr, interfaces.ErrProviderMethodUnimplemented) { + // Per Task 17 of the strict-contracts force-cutover: prefer the + // typed pb.IaCProviderEnumeratorClient via the typed adapter's + // capability accessor (avoids the wasted RPC + sentinel-error + // round-trip the legacy interfaces.X dispatch incurred). Falls + // back to the interfaces.Enumerator type-assert for non-typed + // providers (test fixtures + non-wfctl providers); the legacy + // branch is retained as a stable seam for those consumers and + // is functionally equivalent when the typed adapter is used, + // since typedIaCAdapter satisfies interfaces.Enumerator too. + var refs []interfaces.ResourceRef + var enumErr error + if adapter, ok := p.(*typedIaCAdapter); ok { + enumCli := adapter.Enumerator() + if enumCli == nil { fmt.Fprintf(cleanupStdout, "skipped %s: provider does not implement Enumerator\n", p.Name()) continue } - fmt.Fprintf(cleanupStderr, "%s: enumerate by tag %q: %v\n", p.Name(), *tag, enumErr) - totalErrs = append(totalErrs, fmt.Errorf("%s: enumerate: %w", p.Name(), enumErr)) - continue + resp, err := enumCli.EnumerateByTag(ctx, &pb.EnumerateByTagRequest{Tag: *tag}) + if err != nil { + fmt.Fprintf(cleanupStderr, "%s: enumerate by tag %q: %v\n", p.Name(), *tag, err) + totalErrs = append(totalErrs, fmt.Errorf("%s: enumerate: %w", p.Name(), err)) + continue + } + refs = refsFromPB(resp.GetRefs()) + } else { + enum, ok := p.(interfaces.Enumerator) + if !ok { + fmt.Fprintf(cleanupStdout, "skipped %s: provider does not implement Enumerator\n", p.Name()) + continue + } + refs, enumErr = enum.EnumerateByTag(ctx, *tag) + if enumErr != nil { + if errors.Is(enumErr, interfaces.ErrProviderMethodUnimplemented) { + fmt.Fprintf(cleanupStdout, "skipped %s: provider does not implement Enumerator\n", p.Name()) + continue + } + fmt.Fprintf(cleanupStderr, "%s: enumerate by tag %q: %v\n", p.Name(), *tag, enumErr) + totalErrs = append(totalErrs, fmt.Errorf("%s: enumerate: %w", p.Name(), enumErr)) + continue + } } if len(refs) == 0 { fmt.Fprintf(cleanupStdout, "%s: no resources matched tag %q\n", p.Name(), *tag) diff --git a/cmd/wfctl/infra_status_drift.go b/cmd/wfctl/infra_status_drift.go index f9126deb..88cb0bdf 100644 --- a/cmd/wfctl/infra_status_drift.go +++ b/cmd/wfctl/infra_status_drift.go @@ -100,19 +100,36 @@ func driftInfraModules(ctx context.Context, cfgFile, envName string) error { }() } - // Use DriftConfigDetector when the provider supports it (optional interface). - // Short-circuits to legacy DetectDrift when specsMap is nil (no "apply"- - // provenance entries available) to avoid unnecessary RPC round-trips. + // Per Task 17 of the strict-contracts force-cutover: prefer the + // typed pb.IaCProviderDriftConfigDetectorClient via the typed + // adapter's capability accessor. Falls back to required + // IaCProvider.DetectDrift when the optional service isn't + // registered or when the caller has no specsMap to send. var results []interfaces.DriftResult - if d, ok := provider.(interfaces.DriftConfigDetector); ok { - specsMap := buildAppliedSpecMap(states, g.refs) - if specsMap != nil { - results, err = d.DetectDriftWithSpecs(ctx, g.refs, specsMap) + if adapter, ok := provider.(*typedIaCAdapter); ok { + if cli := adapter.DriftConfigDetector(); cli != nil { + specsMap := buildAppliedSpecMap(states, g.refs) + if specsMap != nil { + results, err = detectDriftConfigTyped(ctx, cli, g.refs, specsMap) + } else { + results, err = provider.DetectDrift(ctx, g.refs) + } } else { results, err = provider.DetectDrift(ctx, g.refs) } } else { - results, err = provider.DetectDrift(ctx, g.refs) + // Provider isn't a typedIaCAdapter (e.g., test fake). + // Fall back to the Go-interface path the test provides. + if d, ok := provider.(interfaces.DriftConfigDetector); ok { + specsMap := buildAppliedSpecMap(states, g.refs) + if specsMap != nil { + results, err = d.DetectDriftWithSpecs(ctx, g.refs, specsMap) + } else { + results, err = provider.DetectDrift(ctx, g.refs) + } + } else { + results, err = provider.DetectDrift(ctx, g.refs) + } } if err != nil { fmt.Printf("WARNING: drift detection for provider %q: %v\n", moduleRef, err) From ab75233513956db083c6029a093452fbedaeaa9c Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 08:32:01 -0400 Subject: [PATCH 2/6] refactor(wfctl): pure typed-pb dispatch at 5 sites + ADR-0028 (PR 618 round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per team-lead + spec-reviewer ruling on PR #618 (round 1 used typed-then-fallback pattern; rejected for code-shape reasons): tighten to PURE Option B at all 5 wfctl-side dispatch sites. interfaces.X fallback removed; non-typed providers hit a typed-error at the type-assert site rather than silently falling through. Sites converted to pure typed-pb: - cmd/wfctl/infra_cleanup.go: hard-fail on non-typed provider; only pb.IaCProviderEnumeratorClient.EnumerateByTag at dispatch. - cmd/wfctl/infra_apply_refresh.go: hard-fail (typed error from runInfraApplyRefreshPhase); detectDriftConfigTyped via typed client when registered, falls through to required IaCProvider.DetectDrift via typed adapter when not. - cmd/wfctl/infra_status_drift.go: warn-and-skip on non-typed (this function returns bool; doesn't propagate error); detectDriftConfigTyped via typed client when registered. - cmd/wfctl/infra_bootstrap.go: resolveCredentialRevoker hard-fails on non-typed (warning + nil revoker, same UX as missing service); returns the typed adapter directly so its RevokeProviderCredential method translates to the typed pb.RevokeProviderCredential RPC under the hood. - cmd/wfctl/infra_align_rules.go: continue (silent skip) on non-typed; R-A10's "treat unimplemented as not-applicable" semantics preserved at the typed-adapter accessor level. ADR-0028 (decisions/0028-task-17-pure-typed-cutover.md) records the decision, failure modes the dual-path preserved (loader-gate weakening, test-fixture DI leak, future contributor cargo-culting, reviewer cognitive load), the bufconn migration pattern for tests (per PR #603 + PR #609 precedent), and the strict-mode invariant translation (gRPC codes.Unimplemented from a non-registered service + translateRPCErr in the adapter preserves operator-visible ErrProviderMethodUnimplemented surface). EXPECTED: ~10 test fixtures fail to compile or run after this commit because they inject fake interfaces.IaCProvider implementations at the dispatch sites. Fixture rewrites land in follow-up commits on this same branch (no force-push). PR 618 stays in CHANGES REQUESTED state until the test pass. Local validation: GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 -short # FAILS (expected — fixture rewrites pending) GOWORK=off golangci-lint run --enable=gocritic,gosec ./cmd/wfctl/... # 0 issues (in code; tests are next commit) Co-Authored-By: Claude Opus 4.7 --- cmd/wfctl/infra_align_rules.go | 33 ++- cmd/wfctl/infra_apply_refresh.go | 42 ++-- cmd/wfctl/infra_bootstrap.go | 25 ++- cmd/wfctl/infra_cleanup.go | 64 +++--- cmd/wfctl/infra_status_drift.go | 39 ++-- decisions/0028-task-17-pure-typed-cutover.md | 203 +++++++++++++++++++ 6 files changed, 283 insertions(+), 123 deletions(-) create mode 100644 decisions/0028-task-17-pure-typed-cutover.md diff --git a/cmd/wfctl/infra_align_rules.go b/cmd/wfctl/infra_align_rules.go index 3e7e1ac3..a9fd4a4c 100644 --- a/cmd/wfctl/infra_align_rules.go +++ b/cmd/wfctl/infra_align_rules.go @@ -775,28 +775,19 @@ func checkRA10_provider_validate_plan(providers []interfaces.IaCProvider, plan * } var findings []AlignFinding for _, p := range providers { - // Per Task 17: prefer typed pb.IaCProviderValidatorClient via - // the typed adapter's capability accessor; falls back to the - // interfaces.ProviderValidator type-assert for non-typed - // providers (test fixtures + non-wfctl consumers). typedIaCAdapter - // satisfies interfaces.ProviderValidator too, so the legacy - // branch path is functionally equivalent when used against the - // real adapter — the typed branch is preferred for clarity + - // to avoid wasted RPC against unregistered services. - var diags []interfaces.PlanDiagnostic - if adapter, ok := p.(*typedIaCAdapter); ok { - cli := adapter.Validator() - if cli == nil { - continue - } - diags = validatePlanTyped(context.Background(), cli, plan) - } else { - v, ok := p.(interfaces.ProviderValidator) - if !ok { - continue - } - diags = v.ValidatePlan(plan) + // Per Task 17 (ADR-0028): pure typed-pb dispatch — no + // interfaces.X fallback. Non-typed providers are silently + // skipped (R-A10's "treat unimplemented as not-applicable" + // behavior is preserved at the typed-adapter accessor level). + adapter, ok := p.(*typedIaCAdapter) + if !ok { + continue + } + cli := adapter.Validator() + if cli == nil { + continue } + diags := validatePlanTyped(context.Background(), cli, plan) for _, d := range diags { // resource: rendered table label (provider-qualified for plan- // level findings so the table always identifies the source). diff --git a/cmd/wfctl/infra_apply_refresh.go b/cmd/wfctl/infra_apply_refresh.go index 4fb2d939..7456166c 100644 --- a/cmd/wfctl/infra_apply_refresh.go +++ b/cmd/wfctl/infra_apply_refresh.go @@ -61,40 +61,26 @@ func runInfraApplyRefreshPhase( return nil } - // Per Task 17 of the strict-contracts force-cutover: replace the - // legacy `provider.(interfaces.DriftConfigDetector)` type-assert - // with a typed pb.IaCProviderDriftConfigDetectorClient lookup via - // the typed adapter's capability accessor. When the plugin's - // ContractRegistry didn't advertise IaCProviderDriftConfigDetector, - // the accessor returns nil and we short-circuit to the required - // IaCProvider.DetectDrift path — preserving the v0.27.1 behavior - // without the wasted RPC + sentinel-error round-trip. + // Per Task 17 of the strict-contracts force-cutover (ADR-0028): + // pure typed-pb dispatch — no interfaces.X fallback. Production + // always yields *typedIaCAdapter via discoverAndLoadIaCProvider; + // test fixtures must construct one via the same bufconn-backed + // pattern. Hard-fail (typed error) if provider isn't a typed adapter. + adapter, ok := provider.(*typedIaCAdapter) + if !ok { + return fmt.Errorf("detect drift: provider %T is not a typed IaC adapter — re-load via discoverAndLoadIaCProvider", provider) + } var results []interfaces.DriftResult var err error - if adapter, ok := provider.(*typedIaCAdapter); ok { - if cli := adapter.DriftConfigDetector(); cli != nil { - specsMap := buildAppliedSpecMap(states, refs) - if specsMap != nil { - results, err = detectDriftConfigTyped(ctx, cli, refs, specsMap) - } else { - results, err = provider.DetectDrift(ctx, refs) - } + if cli := adapter.DriftConfigDetector(); cli != nil { + specsMap := buildAppliedSpecMap(states, refs) + if specsMap != nil { + results, err = detectDriftConfigTyped(ctx, cli, refs, specsMap) } else { results, err = provider.DetectDrift(ctx, refs) } } else { - // Provider isn't a typedIaCAdapter (e.g., test fake). Fall back - // to the Go-interface path the test provides directly. - if d, ok := provider.(interfaces.DriftConfigDetector); ok { - specsMap := buildAppliedSpecMap(states, refs) - if specsMap != nil { - results, err = d.DetectDriftWithSpecs(ctx, refs, specsMap) - } else { - results, err = provider.DetectDrift(ctx, refs) - } - } else { - results, err = provider.DetectDrift(ctx, refs) - } + results, err = provider.DetectDrift(ctx, refs) } if err != nil { // Transient or auth error — propagate; do NOT prune anything. diff --git a/cmd/wfctl/infra_bootstrap.go b/cmd/wfctl/infra_bootstrap.go index ca7e9f5c..af28e3e5 100644 --- a/cmd/wfctl/infra_bootstrap.go +++ b/cmd/wfctl/infra_bootstrap.go @@ -340,19 +340,26 @@ func resolveCredentialRevoker(ctx context.Context, cfgFile string, secretsCfg *S fmt.Fprintf(os.Stderr, "warn: no iac.provider module in config — old provider credential will NOT be revoked automatically\n") return nil, nil } - // Typed-adapter capability discovery: short-circuit when the - // plugin didn't register IaCProviderCredentialRevoker rather than - // returning a revoker that always errors at call time. - if adapter, ok := iacProv.(*typedIaCAdapter); ok && adapter.CredentialRevoker() == nil { - fmt.Fprintf(os.Stderr, "warn: IaC provider does not register IaCProviderCredentialRevoker — old credential will NOT be revoked automatically\n") + // Per Task 17 (ADR-0028): pure typed-pb dispatch — no + // interfaces.X fallback. Production providers are always + // *typedIaCAdapter via discoverAndLoadIaCProvider; if the + // resolved provider isn't, a test fixture is leaking past + // the loader gate — surface as a warning + skip revocation. + adapter, ok := iacProv.(*typedIaCAdapter) + if !ok { + fmt.Fprintf(os.Stderr, "warn: IaC provider %T is not a typed adapter — old credential will NOT be revoked automatically\n", iacProv) return nil, iacCloser } - r, ok := iacProv.(interfaces.ProviderCredentialRevoker) - if !ok { - fmt.Fprintf(os.Stderr, "warn: IaC provider does not implement ProviderCredentialRevoker — old credential will NOT be revoked automatically\n") + if adapter.CredentialRevoker() == nil { + fmt.Fprintf(os.Stderr, "warn: IaC provider does not register IaCProviderCredentialRevoker — old credential will NOT be revoked automatically\n") return nil, iacCloser } - return r, iacCloser + // typedIaCAdapter satisfies interfaces.ProviderCredentialRevoker + // (its method translates to a typed pb.RevokeProviderCredential + // RPC under the hood — no string dispatch). Returning the + // interface keeps the bootstrapSecrets caller signature stable; + // the typed dispatch happens inside the adapter method. + return adapter, iacCloser } // loadIaCProviderFromConfig finds the first iac.provider module in cfgFile, diff --git a/cmd/wfctl/infra_cleanup.go b/cmd/wfctl/infra_cleanup.go index 2ceea691..0a114964 100644 --- a/cmd/wfctl/infra_cleanup.go +++ b/cmd/wfctl/infra_cleanup.go @@ -95,47 +95,31 @@ func runInfraCleanup(args []string) error { //nolint:cyclop var totalErrs []error for _, p := range providers { - // Per Task 17 of the strict-contracts force-cutover: prefer the - // typed pb.IaCProviderEnumeratorClient via the typed adapter's - // capability accessor (avoids the wasted RPC + sentinel-error - // round-trip the legacy interfaces.X dispatch incurred). Falls - // back to the interfaces.Enumerator type-assert for non-typed - // providers (test fixtures + non-wfctl providers); the legacy - // branch is retained as a stable seam for those consumers and - // is functionally equivalent when the typed adapter is used, - // since typedIaCAdapter satisfies interfaces.Enumerator too. - var refs []interfaces.ResourceRef - var enumErr error - if adapter, ok := p.(*typedIaCAdapter); ok { - enumCli := adapter.Enumerator() - if enumCli == nil { - fmt.Fprintf(cleanupStdout, "skipped %s: provider does not implement Enumerator\n", p.Name()) - continue - } - resp, err := enumCli.EnumerateByTag(ctx, &pb.EnumerateByTagRequest{Tag: *tag}) - if err != nil { - fmt.Fprintf(cleanupStderr, "%s: enumerate by tag %q: %v\n", p.Name(), *tag, err) - totalErrs = append(totalErrs, fmt.Errorf("%s: enumerate: %w", p.Name(), err)) - continue - } - refs = refsFromPB(resp.GetRefs()) - } else { - enum, ok := p.(interfaces.Enumerator) - if !ok { - fmt.Fprintf(cleanupStdout, "skipped %s: provider does not implement Enumerator\n", p.Name()) - continue - } - refs, enumErr = enum.EnumerateByTag(ctx, *tag) - if enumErr != nil { - if errors.Is(enumErr, interfaces.ErrProviderMethodUnimplemented) { - fmt.Fprintf(cleanupStdout, "skipped %s: provider does not implement Enumerator\n", p.Name()) - continue - } - fmt.Fprintf(cleanupStderr, "%s: enumerate by tag %q: %v\n", p.Name(), *tag, enumErr) - totalErrs = append(totalErrs, fmt.Errorf("%s: enumerate: %w", p.Name(), enumErr)) - continue - } + // Per Task 17 of the strict-contracts force-cutover (ADR-0028): + // pure typed-pb dispatch — no interfaces.X fallback. Production + // always yields *typedIaCAdapter via discoverAndLoadIaCProvider + // (PR #609); test fixtures must construct one via the same + // bufconn-backed pattern (PR #603 precedent + this PR's own + // fixture rewrites in Task 17 deliverable). + adapter, ok := p.(*typedIaCAdapter) + if !ok { + err := fmt.Errorf("%s: provider %T is not a typed IaC adapter — re-load via discoverAndLoadIaCProvider", p.Name(), p) + fmt.Fprintln(cleanupStderr, err) + totalErrs = append(totalErrs, err) + continue + } + enumCli := adapter.Enumerator() + if enumCli == nil { + fmt.Fprintf(cleanupStdout, "skipped %s: provider does not implement Enumerator\n", p.Name()) + continue + } + resp, err := enumCli.EnumerateByTag(ctx, &pb.EnumerateByTagRequest{Tag: *tag}) + if err != nil { + fmt.Fprintf(cleanupStderr, "%s: enumerate by tag %q: %v\n", p.Name(), *tag, err) + totalErrs = append(totalErrs, fmt.Errorf("%s: enumerate: %w", p.Name(), err)) + continue } + refs := refsFromPB(resp.GetRefs()) if len(refs) == 0 { fmt.Fprintf(cleanupStdout, "%s: no resources matched tag %q\n", p.Name(), *tag) continue diff --git a/cmd/wfctl/infra_status_drift.go b/cmd/wfctl/infra_status_drift.go index 88cb0bdf..9f038e96 100644 --- a/cmd/wfctl/infra_status_drift.go +++ b/cmd/wfctl/infra_status_drift.go @@ -100,36 +100,25 @@ func driftInfraModules(ctx context.Context, cfgFile, envName string) error { }() } - // Per Task 17 of the strict-contracts force-cutover: prefer the - // typed pb.IaCProviderDriftConfigDetectorClient via the typed - // adapter's capability accessor. Falls back to required - // IaCProvider.DetectDrift when the optional service isn't - // registered or when the caller has no specsMap to send. + // Per Task 17 of the strict-contracts force-cutover (ADR-0028): + // pure typed-pb dispatch — no interfaces.X fallback. Hard-fail + // when provider isn't a typed adapter so test-fixture leaks + // don't silently mask production-shape regressions. + adapter, ok := provider.(*typedIaCAdapter) + if !ok { + fmt.Printf("WARNING: provider %q (%T) is not a typed IaC adapter — re-load via discoverAndLoadIaCProvider\n", moduleRef, provider) + return false + } var results []interfaces.DriftResult - if adapter, ok := provider.(*typedIaCAdapter); ok { - if cli := adapter.DriftConfigDetector(); cli != nil { - specsMap := buildAppliedSpecMap(states, g.refs) - if specsMap != nil { - results, err = detectDriftConfigTyped(ctx, cli, g.refs, specsMap) - } else { - results, err = provider.DetectDrift(ctx, g.refs) - } + if cli := adapter.DriftConfigDetector(); cli != nil { + specsMap := buildAppliedSpecMap(states, g.refs) + if specsMap != nil { + results, err = detectDriftConfigTyped(ctx, cli, g.refs, specsMap) } else { results, err = provider.DetectDrift(ctx, g.refs) } } else { - // Provider isn't a typedIaCAdapter (e.g., test fake). - // Fall back to the Go-interface path the test provides. - if d, ok := provider.(interfaces.DriftConfigDetector); ok { - specsMap := buildAppliedSpecMap(states, g.refs) - if specsMap != nil { - results, err = d.DetectDriftWithSpecs(ctx, g.refs, specsMap) - } else { - results, err = provider.DetectDrift(ctx, g.refs) - } - } else { - results, err = provider.DetectDrift(ctx, g.refs) - } + results, err = provider.DetectDrift(ctx, g.refs) } if err != nil { fmt.Printf("WARNING: drift detection for provider %q: %v\n", moduleRef, err) diff --git a/decisions/0028-task-17-pure-typed-cutover.md b/decisions/0028-task-17-pure-typed-cutover.md new file mode 100644 index 00000000..1fe468e0 --- /dev/null +++ b/decisions/0028-task-17-pure-typed-cutover.md @@ -0,0 +1,203 @@ +# ADR 0028 — Task 17 wfctl-side dispatch is pure typed-pb (no interfaces.X fallback) + +**Status:** Accepted (2026-05-10) +**Authors:** implementer-2, spec-reviewer, team-lead +**Related:** ADR 0024 (strict-contracts force-cutover), ADR 0026 (typed-client adapter, no marshalling proxy), Task 17 (`docs/plans/2026-05-10-strict-contracts-force-cutover.md`) + +## Context + +Plan §Task 17 directs converting 5 wfctl-side dispatch sites from +`if x, ok := provider.(interfaces.X); ok { x.Method(...) }` to typed +pb.IaC* RPC calls via the typed-client accessors added on +`*typedIaCAdapter` (Task 30 / PR #605). The 5 sites: + +- `cmd/wfctl/infra_cleanup.go` — interfaces.Enumerator → pb.IaCProviderEnumeratorClient.EnumerateByTag +- `cmd/wfctl/infra_apply_refresh.go` — interfaces.DriftConfigDetector → pb.IaCProviderDriftConfigDetectorClient.DetectDriftConfig +- `cmd/wfctl/infra_status_drift.go` — interfaces.DriftConfigDetector → same +- `cmd/wfctl/infra_bootstrap.go` — interfaces.ProviderCredentialRevoker capability discovery via typedIaCAdapter.CredentialRevoker() +- `cmd/wfctl/infra_align_rules.go` — interfaces.ProviderValidator → pb.IaCProviderValidatorClient.ValidatePlan + +The implementer's first push (commit 63e1cdf8) used a **typed-then-fallback** +pattern: prefer typed pb when `provider.(*typedIaCAdapter)` succeeds, fall +back to the legacy interfaces.X type-assert otherwise. Rationale: avoid +~10 test-fixture rewrites; preserve the interfaces.X integration point +for non-wfctl consumers; typedIaCAdapter satisfies the interfaces.X anyway +so runtime behavior was equivalent. + +Spec-reviewer + team-lead rejected this in favor of a **pure-typed** +cutover (no interfaces.X fallback in wfctl dispatch). This ADR records +the decision and the migration approach. + +## Decision + +The 5 wfctl dispatch sites use **only** the typed pb path: + +```go +adapter, ok := provider.(*typedIaCAdapter) +if !ok { + return fmt.Errorf("...: provider %T is not a typed IaC adapter — re-load via discoverAndLoadIaCProvider", provider) +} +if cli := adapter.X(); cli != nil { + resp, err := cli.TypedRPC(ctx, &pb.XRequest{...}) + // ... +} else { + // optional service not registered — skip per dispatch-specific semantics +} +``` + +The `interfaces.X` type-assertion is removed from every wfctl call +site. Test fixtures that previously injected fake `interfaces.IaCProvider` +implementations are migrated to construct a real `*typedIaCAdapter` +backed by an in-process bufconn-served pb.IaC* server (precedent: +PR #603's `iac_e2e_test.go`). + +The `interfaces.IaCProvider` + sub-interface definitions remain in +`interfaces/` for engine-side consumers (per ADR 0024); only wfctl's +dispatch sites are pure typed. + +## Rationale + +The user mandate `feedback_force_strict_contracts_no_compat` is framed +at **code shape**, not runtime exercise. Both paths "working" was the +exact rejected stance for the legacy additive-strict-contracts plan +(2026-04-26); the same logic applies to wfctl-side dual-path dispatch. + +Concrete failure modes the typed-then-fallback pattern preserves: + +1. **Loader-gate weakening.** Future PR loosens `discoverAndLoadIaCProvider` + (e.g., adds a back-compat path for a v1 plugin); non-typed providers + reach dispatch sites; fallback fires silently against them; behavior + reverts to v0.27.x without surfacing the regression. Pure typed + + hard-fail surfaces immediately. + +2. **Test-fixture DI leak.** A test that injects a fake + `interfaces.IaCProvider` against a wfctl dispatch site is + accidentally exercising the fallback branch — masking production-shape + bugs. Pure typed + bufconn fixtures means the test path uses the + same dispatch shape as production. + +3. **Future contributor cargo-culting.** A new dispatch site copy-paste + from one of the 5 carries the dual-path pattern forward. The strict- + contract surface accretes. Pure typed at the existing sites prevents + the pattern from being in the codebase. + +4. **Reviewer cognitive load.** Every PR touching one of the 5 carries + "is the typed branch right? is the fallback right? does the order + matter?" Pure typed collapses this to one path. + +## Consequences + +### Positive + +- wfctl dispatch shape is unambiguous: typed pb at every IaC call site. +- Loader-gate regressions surface immediately as typed errors at + dispatch. +- Test-fixture leakage is impossible (no fallback branch to fire). +- `interfaces.X` definitions stay in `interfaces/` for engine-side + consumers without bleeding into wfctl as a viable dispatch alternative. + +### Negative + +- ~10 test fixtures must migrate from `fake interfaces.IaCProvider` + to bufconn-backed `*typedIaCAdapter`. Per-test cost varies; the + pattern is well-established by PR #603's `iac_e2e_test.go` and + PR #609's `discover_typed_loader_test.go`. +- Test fixtures that exercise `interfaces.ErrProviderMethodUnimplemented` + semantics need to surface as gRPC `codes.Unimplemented` through + bufconn (the typed adapter translates back to the sentinel — the + test invariant is preserved at the dispatch site, not the source + shape). +- Out-of-org consumers that wrote a custom `interfaces.IaCProvider` + implementation expecting wfctl to type-assert against it are blocked. + This is the strict-cutover trade explicitly accepted by the user + mandate (per ADR 0024). + +## Alternatives rejected + +### Typed-then-fallback (the original PR #618 commit 63e1cdf8 implementation) + +Discussed above. Rejected because code-shape clarity outweighs +runtime equivalence; failure modes 1–4 above are real. + +### interfaces-only (no typed-pb at wfctl sites) + +Status quo before Task 17. Doesn't close the bug class the strict- +contracts cutover targets — every IaC dispatch goes through Go- +interface indirection rather than typed pb. + +### Hybrid (typed-pb at wfctl, interfaces.X retained for adapter satisfiability) + +Considered. Adapter still satisfies interfaces.X (compile-time guards +in iac_typed_adapter.go), so this option is the de-facto chosen path. +The ADR clarifies that the **adapter satisfying interfaces.X is fine** +(engine consumers want it); the **dispatch sites using interfaces.X +type-assertion** is what's rejected. + +## Migration + +### Test fixtures + +Migrate from `fake interfaces.IaCProvider` to bufconn-backed real +adapter. Per-fixture pattern: + +```go +// Before: +fake := &fakeIaCProvider{} // implements interfaces.IaCProvider +adapter = fake + +// After: +lis := bufconn.Listen(1024 * 1024) +srv := grpc.NewServer() +pb.RegisterIaCProviderRequiredServer(srv, &stubRequiredServer{...}) +pb.RegisterIaCProviderEnumeratorServer(srv, &stubEnumeratorServer{...}) +go srv.Serve(lis) +conn, _ := grpc.NewClient( + "passthrough://bufnet", + grpc.WithContextDialer(func(_ context.Context, _ string) (net.Conn, error) { return lis.Dial() }), + grpc.WithTransportCredentials(insecure.NewCredentials()), +) +adapter := newTypedIaCAdapter(conn, map[string]bool{ + iacServiceRequired: true, + iacServiceEnumerator: true, + // ... whichever optional services this test needs +}) +``` + +Affected tests (10 files, scope-locked enumeration): +- `cmd/wfctl/infra_cleanup_test.go` +- `cmd/wfctl/infra_status_drift_test.go` +- `cmd/wfctl/infra_apply_refresh_test.go` +- `cmd/wfctl/infra_bootstrap_force_rotate_test.go` +- `cmd/wfctl/infra_align_rules_test.go` +- `cmd/wfctl/infra_strict_mode_test.go` +- `cmd/wfctl/infra_rotate_and_prune_test.go` +- `cmd/wfctl/infra_audit_keys_test.go` +- `cmd/wfctl/dryrun_test.go` +- `cmd/wfctl/infra_provider_dispatch_test.go` + +### Strict-mode invariant translation + +Tests like `TestInfraAuditKeysCmd_ProviderMissingBridge_FailsLoud` +that depend on `interfaces.ErrProviderMethodUnimplemented` semantics +migrate to: configure the bufconn server to NOT register the optional +service (so the typed accessor returns nil) OR configure the registered +service to return `status.Error(codes.Unimplemented, ...)` from the +RPC. The dispatch site translates these to `interfaces.ErrProviderMethodUnimplemented` +via the existing `translateRPCErr` helper in `iac_typed_adapter.go`, +preserving the operator-visible error shape the test asserts. + +## Precedent + +- ADR 0024 (strict-contracts force-cutover): code-shape over runtime + exercise. +- PR #603 (`iac_e2e_test.go`): bufconn + RegisterAllIaCProviderServices + pattern for in-process typed-RPC tests. +- PR #609 (`discover_typed_loader_test.go`): boundary-test pattern + using stubIaCAdapter to exercise the typed loader without subprocess. + +## Open questions + +None at landing. The fixture migration cost is concrete + bounded +(10 files, established pattern). If specific fixtures hit a +"bufconn can't reproduce X" blocker, surface to team-lead per Task 17 +PR's escalation path. From 10d6c7d149ed7a3387fb86a82028dedbafb3d5b2 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 08:57:46 -0400 Subject: [PATCH 3/6] test(wfctl): bufconn-backed *typedIaCAdapter fixtures (PR 618 round 3, Task 17 item 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per ADR-0028 (PR 618 round 2), wfctl IaC dispatch sites are pure typed-pb (`provider.(*typedIaCAdapter)`) — no interfaces.X fallback. Test fixtures that previously injected fake `interfaces.IaCProvider` implementations no longer reach the dispatch path; the type-assert fails. This migrates the fixtures whose tests actually exercise a Task 17 dispatch site to use a real *typedIaCAdapter wired to an in-process bufconn-served pb.IaCProvider* gRPC server. Shared fixture helper (cmd/wfctl/iac_typed_fixture_test.go): - fixtureTypedAdapter declarative builder: each non-nil pb-server field registers the matching service on the bufconn server, mirroring the ContractRegistry-driven optional-client construction in production. - fixtureRequiredServer: baseline IaCProviderRequiredServer with configurable name/version + UnimplementedIaCProviderRequiredServer embed for everything else. - recordingEnumeratorServer: canned EnumerateByTag / EnumerateAll responses with mutex-guarded recorded inputs. - recordingResourceDriverServer: minimal pb.ResourceDriverServer that records Delete invocations + per-call error injection. - recordingDriftDetectorServer: canned DetectDrift responses. - driftsToPBOrEmpty: engine-side []DriftResult to pb wire shape, mirroring the inverse driftsFromPB in iac_typed_adapter.go. Pattern precedents: PR #603 (iac_e2e_test.go bufconn), PR #609 (discover_typed_loader_test.go boundary test), PR #605 (typed adapter unit tests). Migrated fixture files: 1. cmd/wfctl/infra_cleanup_test.go - fakeEnumeratingProvider/ fakeNonEnumeratingProvider/fakeDeleteDriver replaced with newCleanupEnumFixture / newCleanupNonEnumFixture builders that produce *typedIaCAdapter instances. 7 TestInfraCleanup_* tests now exercise the bufconn typed dispatch end-to-end. 2. cmd/wfctl/infra_apply_refresh_test.go - refreshFakeProvider replaced with newRefreshDriftFixture which registers the typed IaCProviderDriftDetector service. 9 TestApplyRefresh_* tests now go through the typed wire path. TestApplyRefresh_TransientErrorDoesNotPrune asserts on the error substring rather than errors.Is(transientErr) because the gRPC wire boundary doesn't preserve error identity across the bufconn server. 3. cmd/wfctl/infra_align_ra10_test.go - stubIaCProvider type + validatingStubProvider type replaced with stubIaCProvider() and validatingStubProvider() builder functions returning *typedIaCAdapter. cannedValidatorServer registers IaCProviderValidator returning canned PlanDiagnostics. 8 TestCheckRA10_* + TestInfraAlign_RA10_FixtureProvider_Fires now exercise the typed Validator dispatch. 4. cmd/wfctl/infra_strict_mode_test.go - TestInfraCleanup_MultiProvider_ContinuesPastUnimplemented updated to use the migrated cleanup fixtures. Provider A (no Enumerator service registered) -> adapter.Enumerator() returns nil -> cleanup skips with "skipped fake-a: provider does not implement Enumerator" log line, preserving the multi-provider continue-on-skip semantics in their typed-shape form. Scope notes: ADR-0028 lists 10 fixture file paths. Of those: - cmd/wfctl/infra_status_drift_test.go does not exist (the related drift test logic lives in infra_destroy_test.go's TestDriftInfraModules_NoDrift; it currently passes silently because the dispatch warns "not a typed IaC adapter" + returns false. A follow-up PR can migrate that test to harden the silent-pass case.) - cmd/wfctl/infra_bootstrap_force_rotate_test.go uses stubProviderRevoker (interfaces.ProviderCredentialRevoker) rather than IaCProvider; tests call bootstrapSecrets directly, bypassing the resolveCredentialRevoker dispatch. No migration needed. - cmd/wfctl/infra_rotate_and_prune_test.go uses fakeProviderEnumerableDriver (a custom test interface), not interfaces.IaCProvider. - cmd/wfctl/infra_audit_keys_test.go's fakeIaCProviderForAuditKeys goes through `p.(interfaces.EnumeratorAll)` dispatch which is NOT a Task 17 dispatch site (different from the 5 sites converted). - cmd/wfctl/dryrun_test.go and cmd/wfctl/infra_provider_dispatch_test.go use iactest.NoopProvider via the resolveIaCProvider seam; the tests exercise the plan path, which doesn't type-assert to *typedIaCAdapter. The 4 migrated files cover every test that was actually failing the type-assert under PR #618 round 2's pure-typed dispatch. Tests in the other ADR-listed files continue to pass without migration because they don't reach a Task 17 dispatch site. Local validation: GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 # all PASS (7.3s) GOWORK=off go test ./cmd/wfctl/ -count=1 -race # all PASS (10.1s) GOWORK=off golangci-lint run ./cmd/wfctl/... # 0 issues Co-Authored-By: Claude Opus 4.7 --- cmd/wfctl/iac_typed_fixture_test.go | 299 ++++++++++++++++++++++++++ cmd/wfctl/infra_align_ra10_test.go | 203 +++++++++-------- cmd/wfctl/infra_apply_refresh_test.go | 99 ++++----- cmd/wfctl/infra_cleanup_test.go | 184 +++++++--------- cmd/wfctl/infra_strict_mode_test.go | 41 ++-- 5 files changed, 549 insertions(+), 277 deletions(-) create mode 100644 cmd/wfctl/iac_typed_fixture_test.go diff --git a/cmd/wfctl/iac_typed_fixture_test.go b/cmd/wfctl/iac_typed_fixture_test.go new file mode 100644 index 00000000..7ed25a5d --- /dev/null +++ b/cmd/wfctl/iac_typed_fixture_test.go @@ -0,0 +1,299 @@ +package main + +// iac_typed_fixture_test.go — shared bufconn-backed *typedIaCAdapter fixture +// for wfctl tests (Task 17 / PR 618 of the strict-contracts force-cutover +// plan, docs/plans/2026-05-10-strict-contracts-force-cutover.md). +// +// Per ADR-0028, the 5 wfctl IaC dispatch sites are pure typed-pb: they +// type-assert `provider.(*typedIaCAdapter)` and use the typed-client +// accessors for capability discovery. Test fixtures that previously +// injected a fake `interfaces.IaCProvider` no longer reach the dispatch +// path (the type-assert fails). The migration replaces those fakes with +// a real `*typedIaCAdapter` wired to an in-process bufconn-served +// pb.IaCProvider* gRPC server. +// +// Pattern precedents: +// - plugin/external/sdk/iac_e2e_test.go (PR #603) — bufconn end-to-end +// of the typed RPC contract +// - cmd/wfctl/discover_typed_loader_test.go (PR #609) — boundary test +// for the typed loader returning *typedIaCAdapter +// - cmd/wfctl/iac_typed_adapter_test.go (PR #605) — adapter unit + +// in-process gRPC integration tests +// +// Fixture shape (declarative, struct-based): +// +// adapter := fixtureTypedAdapter{ +// Required: &fixtureRequiredServer{name: "do", version: "0.0.0"}, +// Enumerator: &recordingEnumeratorServer{...}, +// }.build(t) +// +// Each non-nil optional-service field results in the matching pb service +// being registered on the in-process gRPC server, which makes the typed +// adapter's accessor for that service return a real client. nil means +// the optional service is NOT registered — the accessor returns nil and +// the dispatch site sees the same shape as a plugin that didn't advertise +// it. +// +// Common mock server types (recordingEnumeratorServer, +// recordingResourceDriverServer, etc.) live alongside fixtureTypedAdapter +// in this file so the migrated fixtures share a single source of truth +// for the mock shapes. + +import ( + "context" + "net" + "sync" + "testing" + + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/test/bufconn" + + "github.com/GoCodeAlone/workflow/interfaces" + pb "github.com/GoCodeAlone/workflow/plugin/external/proto" +) + +// fixtureBufSize matches PR #603's e2eBufSize so bufconn behaviour is +// identical between the SDK end-to-end test and the wfctl fixtures. +const fixtureBufSize = 1024 * 1024 + +// fixtureRequiredServer is the baseline pb.IaCProviderRequiredServer for +// test fixtures. Embeds UnimplementedIaCProviderRequiredServer so additions +// to the proto don't retroactively break tests; only Name + Version are +// overridden so the adapter's Name()/Version() Go methods return what +// each fixture expects. +type fixtureRequiredServer struct { + pb.UnimplementedIaCProviderRequiredServer + name string + version string +} + +func (s *fixtureRequiredServer) Name(_ context.Context, _ *pb.NameRequest) (*pb.NameResponse, error) { + return &pb.NameResponse{Name: s.name}, nil +} + +func (s *fixtureRequiredServer) Version(_ context.Context, _ *pb.VersionRequest) (*pb.VersionResponse, error) { + return &pb.VersionResponse{Version: s.version}, nil +} + +// fixtureTypedAdapter declaratively configures a bufconn-backed +// *typedIaCAdapter. Each non-nil field results in the corresponding +// pb service being registered on the in-process gRPC server, mirroring +// the ContractRegistry-driven optional-client construction in production. +// nil → service not registered → accessor returns nil → dispatch site +// sees the "absence of registration IS the negative signal" contract +// from the typed-IaC design. +type fixtureTypedAdapter struct { + // Required handler. If nil, defaults to fixtureRequiredServer{} which + // returns empty Name/Version and Unimplemented for everything else. + Required pb.IaCProviderRequiredServer + + // Optional services. nil → service not registered → accessor returns nil. + Enumerator pb.IaCProviderEnumeratorServer + DriftDetector pb.IaCProviderDriftDetectorServer + CredentialRevoker pb.IaCProviderCredentialRevokerServer + MigrationRepairer pb.IaCProviderMigrationRepairerServer + Validator pb.IaCProviderValidatorServer + DriftConfigDetect pb.IaCProviderDriftConfigDetectorServer + ResourceDriver pb.ResourceDriverServer +} + +// build spins up a bufconn-backed gRPC server running f's set of services, +// dials it, and returns a *typedIaCAdapter satisfying interfaces.IaCProvider. +// All cleanup (server.Stop, listener.Close, conn.Close) is registered via +// t.Cleanup so the fixture composes naturally with parallel/subtests and +// frees resources at test end without a manual defer chain at every +// call site. +func (f fixtureTypedAdapter) build(t *testing.T) *typedIaCAdapter { + t.Helper() + if f.Required == nil { + f.Required = &fixtureRequiredServer{} + } + + listener := bufconn.Listen(fixtureBufSize) + t.Cleanup(func() { _ = listener.Close() }) + server := grpc.NewServer() + + pb.RegisterIaCProviderRequiredServer(server, f.Required) + registered := map[string]bool{iacServiceRequired: true} + + if f.Enumerator != nil { + pb.RegisterIaCProviderEnumeratorServer(server, f.Enumerator) + registered[iacServiceEnumerator] = true + } + if f.DriftDetector != nil { + pb.RegisterIaCProviderDriftDetectorServer(server, f.DriftDetector) + registered[iacServiceDriftDetector] = true + } + if f.CredentialRevoker != nil { + pb.RegisterIaCProviderCredentialRevokerServer(server, f.CredentialRevoker) + registered[iacServiceCredentialRevoker] = true + } + if f.MigrationRepairer != nil { + pb.RegisterIaCProviderMigrationRepairerServer(server, f.MigrationRepairer) + registered[iacServiceMigrationRepairer] = true + } + if f.Validator != nil { + pb.RegisterIaCProviderValidatorServer(server, f.Validator) + registered[iacServiceValidator] = true + } + if f.DriftConfigDetect != nil { + pb.RegisterIaCProviderDriftConfigDetectorServer(server, f.DriftConfigDetect) + registered[iacServiceDriftConfigDetect] = true + } + if f.ResourceDriver != nil { + pb.RegisterResourceDriverServer(server, f.ResourceDriver) + registered[iacServiceResourceDriver] = true + } + + go func() { _ = server.Serve(listener) }() + t.Cleanup(server.Stop) + + conn, err := grpc.NewClient("passthrough:///bufnet", + grpc.WithContextDialer(func(ctx context.Context, _ string) (net.Conn, error) { + return listener.DialContext(ctx) + }), + grpc.WithTransportCredentials(insecure.NewCredentials()), + ) + if err != nil { + t.Fatalf("fixtureTypedAdapter: grpc.NewClient: %v", err) + } + t.Cleanup(func() { _ = conn.Close() }) + + return newTypedIaCAdapter(conn, registered) +} + +// ── Common mock server types ──────────────────────────────────────────────── +// +// These types provide reusable pb.IaCProvider*Server mocks for the migrated +// wfctl fixtures. Each one records inputs (so tests can assert call counts +// + arguments) and emits canned responses (so tests can drive happy / error +// paths). All are mutex-guarded so a future test that issues parallel RPCs +// through the same fixture doesn't race the recorded state. + +// recordingEnumeratorServer is the bufconn analogue of the legacy +// fakeEnumeratingProvider — returns canned EnumerateByTag / EnumerateAll +// responses (or canned errors) per call. Tests assert against the recorded +// inputs / counts after the run completes. +type recordingEnumeratorServer struct { + pb.UnimplementedIaCProviderEnumeratorServer + + mu sync.Mutex + + // Canned responses (write once before run, read after). + tagRefs []interfaces.ResourceRef + allOutputs []*pb.ResourceOutput + enumerateTagErr error + enumerateAllErr error + + // Recorded inputs (read after run, optional assertion). + enumerateAllType string +} + +func (s *recordingEnumeratorServer) EnumerateByTag(_ context.Context, _ *pb.EnumerateByTagRequest) (*pb.EnumerateByTagResponse, error) { + s.mu.Lock() + defer s.mu.Unlock() + if s.enumerateTagErr != nil { + return nil, s.enumerateTagErr + } + return &pb.EnumerateByTagResponse{Refs: refsToPB(s.tagRefs)}, nil +} + +func (s *recordingEnumeratorServer) EnumerateAll(_ context.Context, req *pb.EnumerateAllRequest) (*pb.EnumerateAllResponse, error) { + s.mu.Lock() + defer s.mu.Unlock() + s.enumerateAllType = req.GetResourceType() + if s.enumerateAllErr != nil { + return nil, s.enumerateAllErr + } + return &pb.EnumerateAllResponse{Outputs: append([]*pb.ResourceOutput(nil), s.allOutputs...)}, nil +} + +// recordingResourceDriverServer is a minimal pb.ResourceDriverServer that +// records Delete invocations and lets tests inject per-call errors. Used +// by infra cleanup fixtures that previously implemented a fake +// interfaces.ResourceDriver inline. Other RPCs left to the embedded +// Unimplemented* defaults — fixtures that need Create/Read/etc. should +// embed this server and override the relevant method. +type recordingResourceDriverServer struct { + pb.UnimplementedResourceDriverServer + + mu sync.Mutex + + // Canned per-call errors. Indexed by zero-based call number. + deleteErrors map[int]error + + // Recorded state. + deleteCount int + deletedRefs []interfaces.ResourceRef +} + +func (s *recordingResourceDriverServer) Delete(_ context.Context, req *pb.ResourceDeleteRequest) (*pb.ResourceDeleteResponse, error) { + s.mu.Lock() + defer s.mu.Unlock() + idx := s.deleteCount + s.deleteCount++ + s.deletedRefs = append(s.deletedRefs, refFromPB(req.GetRef())) + if s.deleteErrors != nil { + if err, ok := s.deleteErrors[idx]; ok { + return nil, err + } + } + return &pb.ResourceDeleteResponse{}, nil +} + +// callCount returns the number of Delete RPCs received. Mutex-guarded so a +// concurrent dispatch loop reading the count after a t.Run subtest doesn't +// race the gRPC handler's mutation. +func (s *recordingResourceDriverServer) callCount() int { + s.mu.Lock() + defer s.mu.Unlock() + return s.deleteCount +} + +// recordingDriftDetectorServer returns canned DetectDrift responses. Used by +// infra apply-refresh fixtures that previously injected a fake +// interfaces.IaCProvider whose DetectDrift returned canned []DriftResult. +type recordingDriftDetectorServer struct { + pb.UnimplementedIaCProviderDriftDetectorServer + + mu sync.Mutex + + driftResults []interfaces.DriftResult + driftErr error +} + +func (s *recordingDriftDetectorServer) DetectDrift(_ context.Context, _ *pb.DetectDriftRequest) (*pb.DetectDriftResponse, error) { + s.mu.Lock() + defer s.mu.Unlock() + if s.driftErr != nil { + return nil, s.driftErr + } + return &pb.DetectDriftResponse{Drifts: driftsToPBOrEmpty(s.driftResults)}, nil +} + +// driftsToPBOrEmpty converts a slice of engine-side DriftResults to the +// proto wire shape, mirroring the inverse driftsFromPB helper in +// iac_typed_adapter.go. Extracted here so test fixtures can declare +// engine-side drift results and have the canned server emit them on the +// wire identically to how a real plugin would. +func driftsToPBOrEmpty(drifts []interfaces.DriftResult) []*pb.DriftResult { + if len(drifts) == 0 { + return nil + } + out := make([]*pb.DriftResult, 0, len(drifts)) + for _, d := range drifts { + expectedJSON, _ := marshalJSONMap(d.Expected) // marshalJSONMap nil-safe + actualJSON, _ := marshalJSONMap(d.Actual) + out = append(out, &pb.DriftResult{ + Name: d.Name, + Type: d.Type, + Drifted: d.Drifted, + Class: driftClassToPB(d.Class), + ExpectedJson: expectedJSON, + ActualJson: actualJSON, + Fields: append([]string(nil), d.Fields...), + }) + } + return out +} diff --git a/cmd/wfctl/infra_align_ra10_test.go b/cmd/wfctl/infra_align_ra10_test.go index deab777c..c4c719aa 100644 --- a/cmd/wfctl/infra_align_ra10_test.go +++ b/cmd/wfctl/infra_align_ra10_test.go @@ -8,67 +8,84 @@ import ( "testing" "github.com/GoCodeAlone/workflow/interfaces" + pb "github.com/GoCodeAlone/workflow/plugin/external/proto" ) -// stubIaCProvider is a no-op IaCProvider used by R-A10 tests. By default it -// does NOT implement ProviderValidator. Embed this in a struct that adds -// ValidatePlan to opt in. -type stubIaCProvider struct{ name string } - -func (s stubIaCProvider) Name() string { return s.name } -func (stubIaCProvider) Version() string { return "0.0.0" } -func (stubIaCProvider) Initialize(context.Context, map[string]any) error { return nil } -func (stubIaCProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { return nil } -func (stubIaCProvider) Plan(context.Context, []interfaces.ResourceSpec, []interfaces.ResourceState) (*interfaces.IaCPlan, error) { - return nil, nil -} -func (stubIaCProvider) Apply(context.Context, *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { - return nil, nil -} -func (stubIaCProvider) Destroy(context.Context, []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { - return nil, nil -} -func (stubIaCProvider) Status(context.Context, []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { - return nil, nil -} -func (stubIaCProvider) DetectDrift(context.Context, []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { - return nil, nil -} -func (stubIaCProvider) Import(context.Context, string, string) (*interfaces.ResourceState, error) { - return nil, nil -} -func (stubIaCProvider) ResolveSizing(string, interfaces.Size, *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { - return nil, nil +// stubIaCProvider returns a *typedIaCAdapter (interfaces.IaCProvider) backed +// by a bufconn-served pb.IaCProviderRequired-only server. Used by R-A10 +// tests as a non-validator baseline — adapter.Validator() returns nil so the +// align rule's dispatch site treats it as a provider that does NOT implement +// ProviderValidator. +// +// Per ADR-0028 (Task 17 / PR 618 strict-contracts force-cutover): wfctl +// dispatch sites are pure typed-pb. Test fixtures must construct a real +// *typedIaCAdapter rather than injecting a custom interfaces.IaCProvider — +// the latter no longer satisfies the type-assert at the dispatch site. +func stubIaCProvider(t *testing.T, name string) interfaces.IaCProvider { + t.Helper() + return fixtureTypedAdapter{ + Required: &fixtureRequiredServer{name: name, version: "0.0.0"}, + }.build(t) } -func (stubIaCProvider) ResourceDriver(string) (interfaces.ResourceDriver, error) { - return nil, nil + +// validatingStubProvider returns a *typedIaCAdapter whose ProviderValidator +// service emits the supplied diagnostics on every ValidatePlan call. The +// pb-shape diagnostic is converted from the engine-side +// []interfaces.PlanDiagnostic so callers can keep declaring fixtures in the +// engine's types. +func validatingStubProvider(t *testing.T, name string, diags []interfaces.PlanDiagnostic) interfaces.IaCProvider { + t.Helper() + return fixtureTypedAdapter{ + Required: &fixtureRequiredServer{name: name, version: "0.0.0"}, + Validator: &cannedValidatorServer{diagnostics: diags}, + }.build(t) } -func (stubIaCProvider) SupportedCanonicalKeys() []string { return nil } -func (stubIaCProvider) BootstrapStateBackend(context.Context, map[string]any) (*interfaces.BootstrapResult, error) { - return nil, nil + +// cannedValidatorServer returns a fixed set of PlanDiagnostics on every +// ValidatePlan RPC. Fixture analogue of the legacy validatingStubProvider's +// inline ValidatePlan(plan) []PlanDiagnostic — diagnostics come back as the +// proto shape so the typed adapter's planDiagnosticSeverityFromPB conversion +// roundtrips identically to the production wire path. +type cannedValidatorServer struct { + pb.UnimplementedIaCProviderValidatorServer + diagnostics []interfaces.PlanDiagnostic } -func (stubIaCProvider) Close() error { return nil } -// validatingStubProvider opts into ProviderValidator with canned diagnostics. -type validatingStubProvider struct { - stubIaCProvider - diags []interfaces.PlanDiagnostic +func (s *cannedValidatorServer) ValidatePlan(_ context.Context, _ *pb.ValidatePlanRequest) (*pb.ValidatePlanResponse, error) { + out := make([]*pb.PlanDiagnostic, 0, len(s.diagnostics)) + for _, d := range s.diagnostics { + out = append(out, &pb.PlanDiagnostic{ + Severity: planDiagnosticSeverityToPB(d.Severity), + Resource: d.Resource, + Field: d.Field, + Message: d.Message, + }) + } + return &pb.ValidatePlanResponse{Diagnostics: out}, nil } -func (v validatingStubProvider) ValidatePlan(*interfaces.IaCPlan) []interfaces.PlanDiagnostic { - return v.diags +// planDiagnosticSeverityToPB is the inverse of planDiagnosticSeverityFromPB +// in iac_typed_adapter.go — extracted here as a test helper so fixtures can +// declare engine-side severities and have the canned server emit the right +// proto enum. +func planDiagnosticSeverityToPB(s interfaces.PlanDiagnosticSeverity) pb.PlanDiagnosticSeverity { + switch s { + case interfaces.PlanDiagnosticWarning: + return pb.PlanDiagnosticSeverity_PLAN_DIAGNOSTIC_WARNING + case interfaces.PlanDiagnosticError: + return pb.PlanDiagnosticSeverity_PLAN_DIAGNOSTIC_ERROR + default: + return pb.PlanDiagnosticSeverity_PLAN_DIAGNOSTIC_INFO + } } // ── Unit tests for checkRA10_provider_validate_plan ────────────────────────── func TestCheckRA10_NilPlan(t *testing.T) { providers := []interfaces.IaCProvider{ - validatingStubProvider{ - stubIaCProvider: stubIaCProvider{name: "p"}, - diags: []interfaces.PlanDiagnostic{ - {Severity: interfaces.PlanDiagnosticError, Message: "boom"}, - }, - }, + validatingStubProvider(t, "p", []interfaces.PlanDiagnostic{ + {Severity: interfaces.PlanDiagnosticError, Message: "boom"}, + }), } if got := checkRA10_provider_validate_plan(providers, nil); len(got) != 0 { t.Errorf("expected no findings when plan is nil, got: %+v", got) @@ -82,7 +99,7 @@ func TestCheckRA10_NoProviders(t *testing.T) { } func TestCheckRA10_NonValidatingProviderSkipped(t *testing.T) { - providers := []interfaces.IaCProvider{stubIaCProvider{name: "plain"}} + providers := []interfaces.IaCProvider{stubIaCProvider(t, "plain")} got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) if len(got) != 0 { t.Errorf("expected no findings when provider does not implement ProviderValidator, got: %+v", got) @@ -91,17 +108,14 @@ func TestCheckRA10_NonValidatingProviderSkipped(t *testing.T) { func TestCheckRA10_ErrorDiagnostic_BecomesFAIL(t *testing.T) { providers := []interfaces.IaCProvider{ - validatingStubProvider{ - stubIaCProvider: stubIaCProvider{name: "do"}, - diags: []interfaces.PlanDiagnostic{ - { - Severity: interfaces.PlanDiagnosticError, - Resource: "db-staging", - Field: "vpc_ref", - Message: "vpc_ref points to unknown resource", - }, + validatingStubProvider(t, "do", []interfaces.PlanDiagnostic{ + { + Severity: interfaces.PlanDiagnosticError, + Resource: "db-staging", + Field: "vpc_ref", + Message: "vpc_ref points to unknown resource", }, - }, + }), } got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) if len(got) != 1 { @@ -127,12 +141,9 @@ func TestCheckRA10_ErrorDiagnostic_BecomesFAIL(t *testing.T) { func TestCheckRA10_WarningDiagnostic_BecomesWARN(t *testing.T) { providers := []interfaces.IaCProvider{ - validatingStubProvider{ - stubIaCProvider: stubIaCProvider{name: "do"}, - diags: []interfaces.PlanDiagnostic{ - {Severity: interfaces.PlanDiagnosticWarning, Resource: "vpc-prod", Message: "deprecated region"}, - }, - }, + validatingStubProvider(t, "do", []interfaces.PlanDiagnostic{ + {Severity: interfaces.PlanDiagnosticWarning, Resource: "vpc-prod", Message: "deprecated region"}, + }), } got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) if len(got) != 1 { @@ -156,12 +167,9 @@ func TestCheckRA10_InfoDiagnostic_LogsAndEmitsNoFinding(t *testing.T) { } providers := []interfaces.IaCProvider{ - validatingStubProvider{ - stubIaCProvider: stubIaCProvider{name: "do"}, - diags: []interfaces.PlanDiagnostic{ - {Severity: interfaces.PlanDiagnosticInfo, Resource: "lb", Field: "tier", Message: "hint about tier"}, - }, - }, + validatingStubProvider(t, "do", []interfaces.PlanDiagnostic{ + {Severity: interfaces.PlanDiagnosticInfo, Resource: "lb", Field: "tier", Message: "hint about tier"}, + }), } got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) if len(got) != 0 { @@ -196,12 +204,9 @@ func TestCheckRA10_PlanLevelDiagnostic_UsesProviderName(t *testing.T) { // When Resource is empty (plan-level finding), the AlignFinding.Resource // falls back to ":plan" so renderers stay deterministic. providers := []interfaces.IaCProvider{ - validatingStubProvider{ - stubIaCProvider: stubIaCProvider{name: "do"}, - diags: []interfaces.PlanDiagnostic{ - {Severity: interfaces.PlanDiagnosticError, Message: "plan-level constraint"}, - }, - }, + validatingStubProvider(t, "do", []interfaces.PlanDiagnostic{ + {Severity: interfaces.PlanDiagnosticError, Message: "plan-level constraint"}, + }), } got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) if len(got) != 1 { @@ -225,12 +230,9 @@ func TestCheckRA10_PlanLevelInfoDiagnostic_LogsAsProviderSlashPlan(t *testing.T) } providers := []interfaces.IaCProvider{ - validatingStubProvider{ - stubIaCProvider: stubIaCProvider{name: "do"}, - diags: []interfaces.PlanDiagnostic{ - {Severity: interfaces.PlanDiagnosticInfo, Message: "plan-level hint"}, - }, - }, + validatingStubProvider(t, "do", []interfaces.PlanDiagnostic{ + {Severity: interfaces.PlanDiagnosticInfo, Message: "plan-level hint"}, + }), } _ = checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) logged := buf.String() @@ -244,18 +246,14 @@ func TestCheckRA10_PlanLevelInfoDiagnostic_LogsAsProviderSlashPlan(t *testing.T) func TestCheckRA10_MultipleProviders_OnlyValidatorsContribute(t *testing.T) { providers := []interfaces.IaCProvider{ - stubIaCProvider{name: "plain"}, - validatingStubProvider{ - stubIaCProvider: stubIaCProvider{name: "do"}, - diags: []interfaces.PlanDiagnostic{ - {Severity: interfaces.PlanDiagnosticError, Resource: "db", Message: "X"}, - }, - }, - validatingStubProvider{ - stubIaCProvider: stubIaCProvider{name: "aws"}, - // No diagnostics — provider implements ProviderValidator but - // returns nil; verifies the rule handles empty slices. - }, + stubIaCProvider(t, "plain"), + validatingStubProvider(t, "do", []interfaces.PlanDiagnostic{ + {Severity: interfaces.PlanDiagnosticError, Resource: "db", Message: "X"}, + }), + // validator with no diagnostics — provider implements ProviderValidator + // (Validator service is registered) but returns an empty slice; verifies + // the rule handles empty slices. + validatingStubProvider(t, "aws", nil), } got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) if len(got) != 1 { @@ -277,17 +275,14 @@ func TestInfraAlign_RA10_FixtureProvider_Fires(t *testing.T) { t.Cleanup(func() { alignLoadProviders = orig }) alignLoadProviders = func(_ *alignContext, _ string, _ *interfaces.IaCPlan) ([]interfaces.IaCProvider, []io.Closer, error) { return []interfaces.IaCProvider{ - validatingStubProvider{ - stubIaCProvider: stubIaCProvider{name: "fixture"}, - diags: []interfaces.PlanDiagnostic{ - { - Severity: interfaces.PlanDiagnosticError, - Resource: "db-staging", - Field: "vpc_ref", - Message: "vpc_ref points to unknown resource", - }, + validatingStubProvider(t, "fixture", []interfaces.PlanDiagnostic{ + { + Severity: interfaces.PlanDiagnosticError, + Resource: "db-staging", + Field: "vpc_ref", + Message: "vpc_ref points to unknown resource", }, - }, + }), }, nil, nil } diff --git a/cmd/wfctl/infra_apply_refresh_test.go b/cmd/wfctl/infra_apply_refresh_test.go index 978d4bc5..331268e8 100644 --- a/cmd/wfctl/infra_apply_refresh_test.go +++ b/cmd/wfctl/infra_apply_refresh_test.go @@ -10,49 +10,35 @@ import ( "github.com/GoCodeAlone/workflow/interfaces" ) -// ── fakes for refresh tests ──────────────────────────────────────────────────── - -// refreshFakeProvider stubs DetectDrift to return caller-supplied results. -// All other IaCProvider methods are no-ops — refresh tests only exercise -// the DetectDrift → state-mutation path. -type refreshFakeProvider struct { - driftResults []interfaces.DriftResult - driftErr error -} - -func (f *refreshFakeProvider) Name() string { return "fake-refresh" } -func (f *refreshFakeProvider) Version() string { return "0.0.0" } -func (f *refreshFakeProvider) Initialize(_ context.Context, _ map[string]any) error { return nil } -func (f *refreshFakeProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { return nil } -func (f *refreshFakeProvider) Plan(_ context.Context, _ []interfaces.ResourceSpec, _ []interfaces.ResourceState) (*interfaces.IaCPlan, error) { - return &interfaces.IaCPlan{}, nil -} -func (f *refreshFakeProvider) Apply(_ context.Context, _ *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { - return &interfaces.ApplyResult{}, nil -} -func (f *refreshFakeProvider) Destroy(_ context.Context, _ []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { - return nil, nil -} -func (f *refreshFakeProvider) Status(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { - return nil, nil -} -func (f *refreshFakeProvider) DetectDrift(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { - return f.driftResults, f.driftErr -} -func (f *refreshFakeProvider) Import(_ context.Context, _ string, _ string) (*interfaces.ResourceState, error) { - return nil, nil -} -func (f *refreshFakeProvider) ResolveSizing(_ string, _ interfaces.Size, _ *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { - return nil, nil -} -func (f *refreshFakeProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { - return nil, nil -} -func (f *refreshFakeProvider) SupportedCanonicalKeys() []string { return nil } -func (f *refreshFakeProvider) BootstrapStateBackend(_ context.Context, _ map[string]any) (*interfaces.BootstrapResult, error) { - return nil, nil +// ── fixtures for refresh tests ────────────────────────────────────────────────── + +// newRefreshDriftFixture builds a *typedIaCAdapter that registers the typed +// IaCProviderDriftDetector service so the apply-refresh dispatch site +// (cmd/wfctl/infra_apply_refresh.go, dispatch site §Task 17) reaches the +// adapter's interfaces.IaCProvider.DetectDrift method via the typed RPC. +// +// Per ADR-0028 (Task 17 / PR 618 strict-contracts force-cutover): wfctl +// dispatch sites are pure typed-pb. The dispatch type-asserts +// `provider.(*typedIaCAdapter)` first; legacy fakes that satisfied +// interfaces.IaCProvider directly no longer reach the dispatch path. The +// migrated fixture wraps a recordingDriftDetectorServer in a real adapter +// so the test exercises the same wire shape production sees. +// +// Note: DriftConfigDetector is intentionally NOT registered. Tests pass +// either nil states (no AppliedConfig present → buildAppliedSpecMap returns +// nil) or states with no AppliedConfigSource ("apply") provenance (also +// returns nil). The dispatcher therefore falls back to the required-side +// DetectDrift path which the IaCProviderDriftDetector service backs. +func newRefreshDriftFixture(t *testing.T, driftResults []interfaces.DriftResult, driftErr error) interfaces.IaCProvider { + t.Helper() + return fixtureTypedAdapter{ + Required: &fixtureRequiredServer{name: "fake-refresh", version: "0.0.0"}, + DriftDetector: &recordingDriftDetectorServer{ + driftResults: driftResults, + driftErr: driftErr, + }, + }.build(t) } -func (f *refreshFakeProvider) Close() error { return nil } // countingStore is an infraStateStore that counts DeleteResource calls and // records the deleted names. @@ -94,7 +80,7 @@ func TestApplyRefresh_DryRunPrintsPrunesWithoutMutating(t *testing.T) { Drifted: true, Class: interfaces.DriftClassGhost, } - provider := &refreshFakeProvider{driftResults: []interfaces.DriftResult{ghost}} + provider := newRefreshDriftFixture(t, []interfaces.DriftResult{ghost}, nil) store := &countingStore{} refs := []interfaces.ResourceRef{{Name: "test-vpc", Type: "infra.vpc"}} @@ -123,7 +109,7 @@ func TestApplyRefresh_AutoApprovePrunesAndApplies(t *testing.T) { Drifted: true, Class: interfaces.DriftClassGhost, } - provider := &refreshFakeProvider{driftResults: []interfaces.DriftResult{ghost}} + provider := newRefreshDriftFixture(t, []interfaces.DriftResult{ghost}, nil) store := &countingStore{} refs := []interfaces.ResourceRef{{Name: "test-vpc", Type: "infra.vpc"}} @@ -156,7 +142,7 @@ func TestApplyRefresh_ProtectedResourceBlockedWithoutFlag(t *testing.T) { Drifted: true, Class: interfaces.DriftClassGhost, } - provider := &refreshFakeProvider{driftResults: []interfaces.DriftResult{ghost}} + provider := newRefreshDriftFixture(t, []interfaces.DriftResult{ghost}, nil) store := &countingStore{} refs := []interfaces.ResourceRef{{Name: "protected-vpc", Type: "infra.vpc"}} states := stateWithProtected("protected-vpc") @@ -183,7 +169,7 @@ func TestApplyRefresh_ProtectedResourcePrunedWithFlag(t *testing.T) { Drifted: true, Class: interfaces.DriftClassGhost, } - provider := &refreshFakeProvider{driftResults: []interfaces.DriftResult{ghost}} + provider := newRefreshDriftFixture(t, []interfaces.DriftResult{ghost}, nil) store := &countingStore{} refs := []interfaces.ResourceRef{{Name: "protected-vpc", Type: "infra.vpc"}} states := stateWithProtected("protected-vpc") @@ -206,7 +192,7 @@ func TestApplyRefresh_ProtectedResourcePrunedWithFlag(t *testing.T) { func TestApplyRefresh_TransientErrorDoesNotPrune(t *testing.T) { transientErr := errors.New("DO API rate limit exceeded") - provider := &refreshFakeProvider{driftErr: transientErr} + provider := newRefreshDriftFixture(t, nil, transientErr) store := &countingStore{} refs := []interfaces.ResourceRef{{Name: "test-vpc", Type: "infra.vpc"}} @@ -217,8 +203,15 @@ func TestApplyRefresh_TransientErrorDoesNotPrune(t *testing.T) { if err == nil { t.Fatal("expected transient error to propagate, got nil") } - if !errors.Is(err, transientErr) { - t.Errorf("expected wrapped transient error, got: %v", err) + // The transient error string must propagate through the gRPC wire + + // translateRPCErr. Since translateRPCErr only converts gRPC + // codes.Unimplemented (the fake here returns a plain non-gRPC error + // which gRPC will wrap with codes.Unknown), the original message text + // is preserved in the wrapped error chain — we assert against the + // substring rather than errors.Is on the local sentinel because the + // gRPC wire boundary doesn't preserve identity across processes. + if !strings.Contains(err.Error(), "DO API rate limit exceeded") { + t.Errorf("expected wrapped transient error (substring match across gRPC wire); got: %v", err) } if store.deleteCount != 0 { t.Errorf("transient error: expected 0 deletes, got %d", store.deleteCount) @@ -232,7 +225,7 @@ func TestApplyRefresh_InSyncResourceSkipped(t *testing.T) { Drifted: false, Class: interfaces.DriftClassInSync, } - provider := &refreshFakeProvider{driftResults: []interfaces.DriftResult{inSync}} + provider := newRefreshDriftFixture(t, []interfaces.DriftResult{inSync}, nil) store := &countingStore{} refs := []interfaces.ResourceRef{{Name: "test-vpc", Type: "infra.vpc"}} @@ -264,7 +257,7 @@ func TestApplyRefresh_MultipleGhostsAllOrNothing(t *testing.T) { {ID: "protected-db", Name: "protected-db", Type: "infra.database", Outputs: map[string]any{"protected": true}}, {ID: "protected-cache", Name: "protected-cache", Type: "infra.cache", Outputs: map[string]any{"protected": true}}, } - provider := &refreshFakeProvider{driftResults: ghosts} + provider := newRefreshDriftFixture(t, ghosts, nil) store := &countingStore{} refs := []interfaces.ResourceRef{ {Name: "unprotected-vpc", Type: "infra.vpc"}, @@ -307,7 +300,7 @@ func TestApplyRefresh_UnprotectedThenProtected_NoPartialPrune(t *testing.T) { states := []interfaces.ResourceState{ {ID: "db-staging", Name: "db-staging", Type: "infra.database", Outputs: map[string]any{"protected": true}}, } - provider := &refreshFakeProvider{driftResults: ghosts} + provider := newRefreshDriftFixture(t, ghosts, nil) store := &countingStore{} refs := []interfaces.ResourceRef{ {Name: "vpc-a", Type: "infra.vpc"}, @@ -338,7 +331,7 @@ func TestApplyRefresh_AllGhostsUnprotectedPrunesAll(t *testing.T) { {Name: "ghost-1", Type: "infra.vpc", Drifted: true, Class: interfaces.DriftClassGhost}, {Name: "ghost-2", Type: "infra.database", Drifted: true, Class: interfaces.DriftClassGhost}, } - provider := &refreshFakeProvider{driftResults: ghosts} + provider := newRefreshDriftFixture(t, ghosts, nil) store := &countingStore{} refs := []interfaces.ResourceRef{ {Name: "ghost-1", Type: "infra.vpc"}, diff --git a/cmd/wfctl/infra_cleanup_test.go b/cmd/wfctl/infra_cleanup_test.go index 5f45f4e0..81d7f4f7 100644 --- a/cmd/wfctl/infra_cleanup_test.go +++ b/cmd/wfctl/infra_cleanup_test.go @@ -12,68 +12,59 @@ import ( "github.com/GoCodeAlone/workflow/interfaces" ) -// fakeEnumeratingProvider is an IaCProvider that ALSO implements Enumerator. -// EnumerateByTag returns a canned slice; ResourceDriver returns a fake driver -// that records Delete calls (and may return an error per index). -type fakeEnumeratingProvider struct { - stubIaCProvider - resources []interfaces.ResourceRef - deleteCallCount int - deleteErrors map[int]error - enumerateErr error -} - -func (f *fakeEnumeratingProvider) EnumerateByTag(_ context.Context, _ string) ([]interfaces.ResourceRef, error) { - if f.enumerateErr != nil { - return nil, f.enumerateErr - } - return f.resources, nil -} - -func (f *fakeEnumeratingProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { - return &fakeDeleteDriver{owner: f}, nil -} - -// fakeDeleteDriver implements just enough of interfaces.ResourceDriver for -// the cleanup dispatch path: Delete is the only method exercised. Other -// methods are no-op stubs to satisfy the interface. -type fakeDeleteDriver struct{ owner *fakeEnumeratingProvider } - -func (d *fakeDeleteDriver) Create(context.Context, interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { - return nil, nil -} -func (d *fakeDeleteDriver) Read(context.Context, interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { - return nil, nil -} -func (d *fakeDeleteDriver) Update(context.Context, interfaces.ResourceRef, interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { - return nil, nil -} -func (d *fakeDeleteDriver) Diff(context.Context, interfaces.ResourceSpec, *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { - return nil, nil -} -func (d *fakeDeleteDriver) HealthCheck(context.Context, interfaces.ResourceRef) (*interfaces.HealthResult, error) { - return nil, nil -} -func (d *fakeDeleteDriver) Scale(context.Context, interfaces.ResourceRef, int) (*interfaces.ResourceOutput, error) { - return nil, nil -} -func (d *fakeDeleteDriver) SensitiveKeys() []string { return nil } -func (d *fakeDeleteDriver) Delete(_ context.Context, _ interfaces.ResourceRef) error { - idx := d.owner.deleteCallCount - d.owner.deleteCallCount++ - if d.owner.deleteErrors != nil { - if err, ok := d.owner.deleteErrors[idx]; ok { - return err - } - } - return nil +// cleanupEnumFixture wires the bufconn-backed *typedIaCAdapter the cleanup +// dispatcher (infra_cleanup.go, dispatch site §Task 17) expects, alongside +// references to the recording mock servers so each test can assert delete +// counts, deleted refs, and per-call error injection after the run. +// +// Per ADR-0028 (Task 17 / PR 618 strict-contracts force-cutover): wfctl +// dispatch sites are pure typed-pb. Test fixtures must construct a real +// *typedIaCAdapter rather than injecting a custom interfaces.IaCProvider — +// the latter no longer satisfies the type-assert at the dispatch site. +type cleanupEnumFixture struct { + adapter *typedIaCAdapter + + // Embedded mock servers — tests assert against these after a run. + enumerator *recordingEnumeratorServer + driver *recordingResourceDriverServer +} + +// newCleanupEnumFixture builds a *typedIaCAdapter that registers +// IaCProviderEnumerator (canned EnumerateByTag) + ResourceDriver +// (recording Delete) services. Mirrors the legacy fakeEnumeratingProvider +// that implemented interfaces.Enumerator + interfaces.ResourceDriver inline. +func newCleanupEnumFixture(t *testing.T, name string, resources []interfaces.ResourceRef, enumerateErr error, deleteErrors map[int]error) *cleanupEnumFixture { + t.Helper() + enum := &recordingEnumeratorServer{ + tagRefs: resources, + enumerateTagErr: enumerateErr, + } + drv := &recordingResourceDriverServer{ + deleteErrors: deleteErrors, + } + adapter := fixtureTypedAdapter{ + Required: &fixtureRequiredServer{name: name, version: "0.0.0"}, + Enumerator: enum, + ResourceDriver: drv, + }.build(t) + return &cleanupEnumFixture{ + adapter: adapter, + enumerator: enum, + driver: drv, + } +} + +// newCleanupNonEnumFixture builds a *typedIaCAdapter that registers ONLY the +// IaCProviderRequired service — no Enumerator. The cleanup dispatcher must +// skip such providers with a structured stdout log (the +// `provider does not implement Enumerator` branch). +func newCleanupNonEnumFixture(t *testing.T, name string) *typedIaCAdapter { + t.Helper() + return fixtureTypedAdapter{ + Required: &fixtureRequiredServer{name: name, version: "0.0.0"}, + }.build(t) } -// fakeNonEnumeratingProvider is an IaCProvider that does NOT implement -// Enumerator (uses the bare stubIaCProvider). The cleanup dispatcher must -// skip it with a structured stdout log line rather than failing. -type fakeNonEnumeratingProvider struct{ stubIaCProvider } - // runInfraCleanupForTest invokes runInfraCleanup with a fake provider list // and captures stdout/stderr. It overrides the cleanupLoadProviders seam so // the test does not touch the live plugin loader / config file system. @@ -96,15 +87,13 @@ func runInfraCleanupForTest(t *testing.T, providers []interfaces.IaCProvider, ar } func TestInfraCleanup_DryRunByDefault_ListsResourcesWithoutDeleting(t *testing.T) { - fp := &fakeEnumeratingProvider{ - stubIaCProvider: stubIaCProvider{name: "do-fake"}, - resources: []interfaces.ResourceRef{ + fp := newCleanupEnumFixture(t, "do-fake", + []interfaces.ResourceRef{ {Name: "vpc-1", Type: "infra.vpc"}, {Name: "db-1", Type: "infra.database"}, - }, - } + }, nil, nil) - out, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{fp}, "--tag", "test-tag") + out, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{fp.adapter}, "--tag", "test-tag") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -117,26 +106,24 @@ func TestInfraCleanup_DryRunByDefault_ListsResourcesWithoutDeleting(t *testing.T if !strings.Contains(out, "[dry-run]") { t.Errorf("expected [dry-run] marker in output: %s", out) } - if fp.deleteCallCount != 0 { - t.Errorf("dry-run should not invoke Delete; got %d calls", fp.deleteCallCount) + if got := fp.driver.callCount(); got != 0 { + t.Errorf("dry-run should not invoke Delete; got %d calls", got) } } func TestInfraCleanup_FixMode_DeletesResources(t *testing.T) { - fp := &fakeEnumeratingProvider{ - stubIaCProvider: stubIaCProvider{name: "do-fake"}, - resources: []interfaces.ResourceRef{ + fp := newCleanupEnumFixture(t, "do-fake", + []interfaces.ResourceRef{ {Name: "vpc-1", Type: "infra.vpc"}, {Name: "db-1", Type: "infra.database"}, - }, - } + }, nil, nil) - out, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{fp}, "--tag", "test-tag", "--fix") + out, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{fp.adapter}, "--tag", "test-tag", "--fix") if err != nil { t.Fatalf("unexpected error: %v", err) } - if fp.deleteCallCount != 2 { - t.Errorf("expected 2 Delete calls, got %d", fp.deleteCallCount) + if got := fp.driver.callCount(); got != 2 { + t.Errorf("expected 2 Delete calls, got %d", got) } if !strings.Contains(out, "deleted") { t.Errorf("expected 'deleted' in output: %s", out) @@ -147,7 +134,7 @@ func TestInfraCleanup_FixMode_DeletesResources(t *testing.T) { } func TestInfraCleanup_NonEnumeratorProvider_SkipsWithStructuredLog(t *testing.T) { - fp := &fakeNonEnumeratingProvider{stubIaCProvider: stubIaCProvider{name: "non-enum"}} + fp := newCleanupNonEnumFixture(t, "non-enum") out, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{fp}, "--tag", "test-tag") if err != nil { @@ -159,36 +146,31 @@ func TestInfraCleanup_NonEnumeratorProvider_SkipsWithStructuredLog(t *testing.T) } func TestInfraCleanup_PartialFailure_ReturnsError(t *testing.T) { - fp := &fakeEnumeratingProvider{ - stubIaCProvider: stubIaCProvider{name: "do-fake"}, - resources: []interfaces.ResourceRef{ + fp := newCleanupEnumFixture(t, "do-fake", + []interfaces.ResourceRef{ {Name: "vpc-1", Type: "infra.vpc"}, {Name: "db-1", Type: "infra.database"}, }, + nil, // Second Delete fails (idx 1). - deleteErrors: map[int]error{1: errors.New("simulated failure")}, - } + map[int]error{1: errors.New("simulated failure")}) - _, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{fp}, "--tag", "test-tag", "--fix") + _, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{fp.adapter}, "--tag", "test-tag", "--fix") if err == nil { t.Errorf("expected non-nil error on partial failure") } - if fp.deleteCallCount != 2 { - t.Errorf("expected dispatcher to attempt all 2 deletes despite mid-run failure; got %d", fp.deleteCallCount) + if got := fp.driver.callCount(); got != 2 { + t.Errorf("expected dispatcher to attempt all 2 deletes despite mid-run failure; got %d", got) } } func TestInfraCleanup_EnumerateError_ReturnsErrorAndContinuesOtherProviders(t *testing.T) { - failing := &fakeEnumeratingProvider{ - stubIaCProvider: stubIaCProvider{name: "fail"}, - enumerateErr: errors.New("simulated enumerate fail"), - } - working := &fakeEnumeratingProvider{ - stubIaCProvider: stubIaCProvider{name: "ok"}, - resources: []interfaces.ResourceRef{{Name: "ok-1", Type: "infra.compute"}}, - } + failing := newCleanupEnumFixture(t, "fail", nil, errors.New("simulated enumerate fail"), nil) + working := newCleanupEnumFixture(t, "ok", + []interfaces.ResourceRef{{Name: "ok-1", Type: "infra.compute"}}, + nil, nil) - out, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{failing, working}, "--tag", "test-tag") + out, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{failing.adapter, working.adapter}, "--tag", "test-tag") if err == nil { t.Errorf("expected non-nil error from failing enumerator") } @@ -214,20 +196,18 @@ func TestInfraCleanup_TagRequired(t *testing.T) { // accidentally honors --dry-run=false alone would silently start deleting // production resources from a flag a user thought was a preview toggle. func TestInfraCleanup_SafeDefault_DryRunFalseWithoutFixStillSkipsDelete(t *testing.T) { - fp := &fakeEnumeratingProvider{ - stubIaCProvider: stubIaCProvider{name: "do-fake"}, - resources: []interfaces.ResourceRef{ + fp := newCleanupEnumFixture(t, "do-fake", + []interfaces.ResourceRef{ {Name: "vpc-1", Type: "infra.vpc"}, - }, - } + }, nil, nil) - out, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{fp}, + out, _, err := runInfraCleanupForTest(t, []interfaces.IaCProvider{fp.adapter}, "--tag", "test-tag", "--dry-run=false") if err != nil { t.Fatalf("unexpected error: %v", err) } - if fp.deleteCallCount != 0 { - t.Errorf("safe-default invariant violated: --dry-run=false without --fix invoked Delete %d times; expected 0", fp.deleteCallCount) + if got := fp.driver.callCount(); got != 0 { + t.Errorf("safe-default invariant violated: --dry-run=false without --fix invoked Delete %d times; expected 0", got) } if !strings.Contains(out, "[dry-run]") { t.Errorf("expected [dry-run] marker even with --dry-run=false (because --fix is absent); got: %s", out) diff --git a/cmd/wfctl/infra_strict_mode_test.go b/cmd/wfctl/infra_strict_mode_test.go index 0a5c0bde..40e7f979 100644 --- a/cmd/wfctl/infra_strict_mode_test.go +++ b/cmd/wfctl/infra_strict_mode_test.go @@ -423,12 +423,20 @@ func TestInfraRotateAndPruneCmd_MultiProvider_ContinuesPastUnimplemented(t *test } // TestInfraCleanup_MultiProvider_ContinuesPastUnimplemented asserts the -// existing infra_cleanup.go pattern (try-each + skip-on-Unimplemented) +// existing infra_cleanup.go pattern (try-each + skip-on-non-registration) // remains correct in the presence of a heterogeneous loaded-providers // list. Cleanup's design predates PR #589 but the same architectural -// concern applies — the bridge means every gRPC-loaded provider satisfies -// interfaces.Enumerator, so the loop must distinguish "plugin doesn't -// support EnumerateByTag" from "real enumerate failure". +// concern applies — the dispatcher must distinguish "plugin doesn't +// register the Enumerator service" from "real enumerate failure". +// +// Per Task 17 / PR 618 (ADR-0028), the strict-typed dispatch surfaces +// "plugin didn't advertise the optional service" via adapter.Enumerator() +// returning nil — the typed analogue of the legacy +// `errors.Is(err, ErrProviderMethodUnimplemented)` skip path. Provider A +// here is built without an Enumerator service registered (the bufconn +// server only registers IaCProviderRequired); the dispatch site logs +// `skipped fake-a: provider does not implement Enumerator` and proceeds +// to provider B. func TestInfraCleanup_MultiProvider_ContinuesPastUnimplemented(t *testing.T) { tmpDir := t.TempDir() cfgPath := filepath.Join(tmpDir, "infra.yaml") @@ -436,22 +444,19 @@ func TestInfraCleanup_MultiProvider_ContinuesPastUnimplemented(t *testing.T) { t.Fatalf("write fixture: %v", err) } - // Provider A: EnumerateByTag returns ErrProviderMethodUnimplemented. - a := &fakeEnumeratingProvider{ - stubIaCProvider: stubIaCProvider{name: "fake-a"}, - enumerateErr: interfaces.ErrProviderMethodUnimplemented, - } - // Provider B: implements Enumerator and returns a canned ref. - b := &fakeEnumeratingProvider{ - stubIaCProvider: stubIaCProvider{name: "fake-b"}, - resources: []interfaces.ResourceRef{ + // Provider A: does NOT register IaCProviderEnumerator, so the typed + // adapter's Enumerator() accessor returns nil → cleanup skips with the + // "provider does not implement Enumerator" log line. + a := newCleanupNonEnumFixture(t, "fake-a") + // Provider B: registers Enumerator with a canned ref. + b := newCleanupEnumFixture(t, "fake-b", + []interfaces.ResourceRef{ {Name: "r-from-b", Type: "infra.spaces_key", ProviderID: "AK_B"}, - }, - } + }, nil, nil) origLoad := cleanupLoadProviders t.Cleanup(func() { cleanupLoadProviders = origLoad }) cleanupLoadProviders = func(_ context.Context, _ *flag.FlagSet, _, _ string) ([]interfaces.IaCProvider, []io.Closer, error) { - return []interfaces.IaCProvider{a, b}, nil, nil + return []interfaces.IaCProvider{a, b.adapter}, nil, nil } origStdout := cleanupStdout @@ -464,11 +469,11 @@ func TestInfraCleanup_MultiProvider_ContinuesPastUnimplemented(t *testing.T) { "--tag", "test", }) if err != nil { - t.Fatalf("cleanup must skip Unimplemented and continue; err=%v stdout=%s", err, out.String()) + t.Fatalf("cleanup must skip non-registered Enumerator and continue; err=%v stdout=%s", err, out.String()) } // Provider A must surface a structured "skipped" log line. if !strings.Contains(out.String(), "skipped fake-a") { - t.Errorf("expected 'skipped fake-a' log line for Unimplemented provider; stdout=%s", out.String()) + t.Errorf("expected 'skipped fake-a' log line for non-registered Enumerator; stdout=%s", out.String()) } // Provider B must reach the dry-run / list path with its ref. if !strings.Contains(out.String(), "r-from-b") { From e0070bca4c51dd593c252ec1d74a78025cfdb808 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 09:04:32 -0400 Subject: [PATCH 4/6] =?UTF-8?q?docs(decisions):=20ADR-0028=20expansion=20?= =?UTF-8?q?=E2=80=94=20per-site=20dispatch=20UX=20(PR=20618=20round=203)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per spec-reviewer ruling on PR #618 round 3: code-shape mandate is met (pure typed-pb at all 5 sites), but the per-site rejection severity varies based on iteration semantics. Soft-skip at iteration sites is graceful degradation, not the rejected silent-fallback shape — this expansion documents the rule + per-site rationale so future contributors don't cargo-cult either direction blindly. New `## Per-site dispatch UX` section adds: - Severity table for each of the 5 sites (cleanup hard-error, apply-refresh hard-error, status-drift soft-skip, align-rules R-A10 silent-skip, bootstrap soft-skip-revocation) with explicit per-site reasoning anchored in iteration vs single-shot semantics. - Canonical rule (verbatim from team-lead): "Pure typed-pb dispatch at all sites; non-typed input rejection severity is per-site UX based on iteration semantics. New dispatch sites default to hard-error unless graceful-degradation is operationally required." Plus the two-condition bar for soft-skip eligibility (iteration + auditable warn-log). - Failure-mode contrast vs the round-1-rejected silent-fallback pattern: (1) the fallback path no longer exists at all 5 sites, (2) soft-skip is auditable via stderr warn-log, (3) the no-op result is observably distinct from a typed-pb success at the call site. ADR-only edit; no code, fixture, or test changes. Co-Authored-By: Claude Opus 4.7 --- decisions/0028-task-17-pure-typed-cutover.md | 60 ++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/decisions/0028-task-17-pure-typed-cutover.md b/decisions/0028-task-17-pure-typed-cutover.md index 1fe468e0..67bdbe16 100644 --- a/decisions/0028-task-17-pure-typed-cutover.md +++ b/decisions/0028-task-17-pure-typed-cutover.md @@ -55,6 +55,66 @@ The `interfaces.IaCProvider` + sub-interface definitions remain in `interfaces/` for engine-side consumers (per ADR 0024); only wfctl's dispatch sites are pure typed. +## Per-site dispatch UX + +The pure-typed code-shape mandate is uniform across all 5 sites: +`provider.(*typedIaCAdapter)` is the only valid input shape; no +`interfaces.X` fallback. **Severity** of the non-typed-input branch, +however, is per-site UX based on iteration semantics. A halt-on-bad- +provider response at an iteration site would lose visibility into the +other providers' results — a regression vs the legacy +iterate-and-skip semantics. The 5 sites split as follows: + +| Site | Severity | Per-site rationale | +|---|---|---| +| `cmd/wfctl/infra_cleanup.go:104` (Enumerator) | **Hard-error** (return + collect) | Single-shot operation against a bounded provider list. Surfacing fixture-leak loudly is correct; halting one bad provider while continuing others is captured by the existing `totalErrs = append(...)` + `continue` pattern. Operator sees the problem, batch result still distinguishes successes from failures. | +| `cmd/wfctl/infra_apply_refresh.go:69` (DriftConfigDetector) | **Hard-error** (return) | Single-shot drift detection feeding the prune decision. Apply-refresh prunes ghosts; a fixture-leak that silently returned "no drift" would suppress prunes operators expected — same anti-pattern the dispatch is designed to prevent. Hard-error is operationally correct. | +| `cmd/wfctl/infra_status_drift.go:107` (DriftConfigDetector) | **Soft-skip** (warn + return false) | Per-provider iteration in `driftGroup`. Halting on one bad provider would suppress drift visibility for every other provider in the loop — strictly worse for operators triaging multi-provider drift state. The warning log is the auditable signal; "no drift reported" for a single provider is the graceful-degradation behavior. | +| `cmd/wfctl/infra_align_rules.go:782` (Validator, R-A10) | **Silent-skip** (`continue`) | R-A10's contract is "treat unimplemented validator as not-applicable" (rule iterates over align providers and only emits findings from those that opt in). A non-typed provider that reaches this loop would not produce diagnostics under the legacy `interfaces.ProviderValidator` type-assert either; the silent-skip preserves that contract. Hard-error here would abort the align batch on a fixture-leak, masking other rules' findings. | +| `cmd/wfctl/infra_bootstrap.go:348` (CredentialRevoker) | **Soft-skip** (warn + skip revocation) | Bootstrap completes secret minting; the typed-adapter capability gate decides whether old credentials get auto-revoked. A non-typed provider reaching this site means revocation is unavailable; the bootstrap itself remains correct, the operator sees a stderr warning advising manual revocation. Halting bootstrap on revoker absence would block secret rotation entirely on a fixture-leak — a strictly worse failure mode. | + +### Canonical rule + +> Pure typed-pb dispatch at all sites; non-typed input rejection +> severity is per-site UX based on iteration semantics. New dispatch +> sites default to hard-error unless graceful-degradation is +> operationally required. + +The "graceful-degradation is operationally required" bar is met when +the dispatch site iterates over a heterogeneous provider list (or +sub-rules) where halting on one bad input would lose visibility into +the others' results, **and** the soft-skip path emits an auditable +warn-log so operators can recognize + act on the gap. Both conditions +must hold; soft-skip without auditability collapses back into the +"silent fallback" failure mode the cutover exists to prevent. + +### Why this is not the rejected silent-fallback shape + +The original Round-1 rejection (commit 63e1cdf8 → CHANGES REQUESTED) +was at code-shape: `interfaces.X` fallback dispatch was invisible to +operators (no log line), invoked the legacy proxy's behavior, and +masked the loader-gate's pre-flight contract. The current per-site +soft-skip pattern differs in three observable ways: + +1. **The fallback path no longer exists.** All 5 sites first do + `adapter, ok := p.(*typedIaCAdapter)`; the only branch is "use + typed-pb" or "skip with warn-log". There is no second dispatch + shape that takes the call. +2. **Soft-skip is auditable.** Each soft-skip site emits a stderr + warn-log identifying the provider + the reason (`provider %T is + not a typed IaC adapter — re-load via discoverAndLoadIaCProvider` + or analogous). Operators see fixture leaks and loader-gate + regressions in the same shape as any other operator-facing + diagnostic. +3. **Continue-semantics, not equivalent-behavior.** The legacy + fallback returned a real result via the interfaces.X path — + indistinguishable from a typed-pb success at the call site. The + soft-skip path returns the no-op result for the operation + (false / nil-skip / empty diagnostics), which is observably + distinct: status-drift reports no-drift, R-A10 emits no findings, + bootstrap leaves old credentials in place. Operators reading the + output see the gap. + ## Rationale The user mandate `feedback_force_strict_contracts_no_compat` is framed From 3b9103eb78d551a216acc355907a60d09f6199d0 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 09:14:16 -0400 Subject: [PATCH 5/6] fix(wfctl): translate Unimplemented + propagate ctx + doc/error polish (PR 618 round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per code-review IMPORTANT-1 / IMPORTANT-2 / MINOR-1 / MINOR-2 (PR 618 round 4): IMPORTANT-1 — translateRPCErr at typed dispatch sites ADR-0028 §Migration's "Strict-mode invariant translation" promises codes.Unimplemented at the wire boundary becomes interfaces.ErrProviderMethodUnimplemented for downstream errors.Is classification. The typedIaCAdapter's interfaces.IaCProvider methods already wrap, but the new typed-RPC dispatch helpers + the inline EnumerateByTag call site bypassed the wrap. Fixed two sites: - cmd/wfctl/iac_typed_dispatch.go:detectDriftConfigTyped now wraps cli.DetectDriftConfig errors via translateRPCErr. - cmd/wfctl/infra_cleanup.go's enumCli.EnumerateByTag site wraps via translateRPCErr before formatting + appending to totalErrs. Audit confirmed the 3 other dispatch sites already route through adapter methods that translate (provider.DetectDrift via typedIaCAdapter.DetectDrift, adapter.RevokeProviderCredential). validatePlanTyped intentionally returns nil-diags on any error per the documented Go interfaces.ProviderValidator.ValidatePlan signature contract; no translation needed there. IMPORTANT-2 — propagate caller context to ValidatePlan validatePlanTyped at infra_align_rules.go:782 was called with context.Background(), losing operator Ctrl-C / parent cancellation / RPC deadline propagation. Threaded ctx through: - runInfraAlign → runInfraAlignChecks(ctx, opts) - runInfraAlignChecks → checkRA10_provider_validate_plan(ctx, ...) - checkRA10_provider_validate_plan → validatePlanTyped(ctx, ...) Renamed runInfraAlignChecks's local *alignContext binding from `ctx` to `alignCtx` to avoid shadowing the new context.Context parameter. Test callers (runInfraAlignChecks at 16 sites, checkRA10_provider_validate_plan at 9 sites) updated to pass context.Background(); context import added to test files that needed it. MINOR-1 — iac_typed_adapter.go accessor doc-comment Doc example said `if !ok { /* legacy path no longer exists */ }` while the body asserted "wfctl call sites are pure typed". Reworked the example to show both per-site UX shapes (hard-error + soft-skip) per ADR-0028 §Per-site dispatch UX, with parenthetical mapping to the dispatch sites that use each shape. MINOR-2 — specToPB error key context detectDriftConfigTyped's per-spec marshalling loop returned bare specToPB errors with no key context. Wrapped with fmt.Errorf("specToPB %q: %w", k, err) so post-mortem debugging identifies which entry in the per-resource specs map blew up. Local validation: GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 # all PASS (7.4s) GOWORK=off go test ./cmd/wfctl/ -count=1 -race # all PASS (10.5s) GOWORK=off golangci-lint run ./cmd/wfctl/... # 0 issues Co-Authored-By: Claude Opus 4.7 --- cmd/wfctl/iac_typed_adapter.go | 23 ++++++--- cmd/wfctl/iac_typed_dispatch.go | 20 +++++++- cmd/wfctl/infra_align.go | 36 +++++++------ cmd/wfctl/infra_align_integration_test.go | 3 +- cmd/wfctl/infra_align_ra10_test.go | 22 ++++---- cmd/wfctl/infra_align_rules.go | 8 ++- cmd/wfctl/infra_align_test.go | 61 ++++++++++++----------- cmd/wfctl/infra_cleanup.go | 8 +++ 8 files changed, 114 insertions(+), 67 deletions(-) diff --git a/cmd/wfctl/iac_typed_adapter.go b/cmd/wfctl/iac_typed_adapter.go index 363b0886..2cbbfd36 100644 --- a/cmd/wfctl/iac_typed_adapter.go +++ b/cmd/wfctl/iac_typed_adapter.go @@ -114,20 +114,31 @@ func newTypedIaCAdapter(conn *grpc.ClientConn, registered map[string]bool) *type // optional service, or nil if the plugin's ContractRegistry didn't // advertise it. wfctl dispatch sites that previously did // `if x, ok := provider.(interfaces.X); ok { x.Method(...) }` now -// type-assert to *typedIaCAdapter and use these accessors: +// type-assert to *typedIaCAdapter and use these accessors. The +// non-typed branch is per-site UX (ADR-0028 §Per-site dispatch UX) — +// hard-error at single-shot sites, soft-skip at iteration sites, e.g.: // +// // Hard-error (single-shot — cleanup, apply-refresh): // adapter, ok := provider.(*typedIaCAdapter) -// if !ok { /* legacy path no longer exists */ } +// if !ok { +// return fmt.Errorf("provider %T is not a typed IaC adapter", provider) +// } // if cli := adapter.Enumerator(); cli != nil { // resp, err := cli.EnumerateByTag(ctx, &pb.EnumerateByTagRequest{Tag: t}) // // ... // } // -// This eliminates the residual interfaces.X type-assertion shape per -// the strict-contracts force-cutover (no string dispatch, no Go- -// interface indirection at the wfctl boundary). The interfaces.X +// // Soft-skip (iteration — status-drift, R-A10, bootstrap revoker): +// adapter, ok := provider.(*typedIaCAdapter) +// if !ok { +// fmt.Printf("WARNING: provider %q is not a typed adapter\n", name) +// continue // or return false / nil-skip per site +// } +// +// Either way the legacy interfaces.X fallback is gone. The interfaces.X // definitions remain in `interfaces/` for engine-side / module-factory -// consumers — wfctl call sites are pure typed-pb. +// consumers — wfctl call sites are pure typed-pb (no string dispatch, +// no Go-interface indirection at the wfctl boundary). // RequiredClient returns the typed pb.IaCProviderRequiredClient. Always // non-nil (the loader rejects plugins that don't register the required diff --git a/cmd/wfctl/iac_typed_dispatch.go b/cmd/wfctl/iac_typed_dispatch.go index 0dba8f0f..eb2457a5 100644 --- a/cmd/wfctl/iac_typed_dispatch.go +++ b/cmd/wfctl/iac_typed_dispatch.go @@ -18,6 +18,7 @@ package main import ( "context" + "fmt" "github.com/GoCodeAlone/workflow/interfaces" pb "github.com/GoCodeAlone/workflow/plugin/external/proto" @@ -36,7 +37,11 @@ func detectDriftConfigTyped(ctx context.Context, cli pb.IaCProviderDriftConfigDe for k, s := range specs { ps, err := specToPB(s) if err != nil { - return nil, err + // Per code-review MINOR-2 (PR 618 round 4): name the offending + // spec key so post-mortem debugging doesn't require crashing + // through the marshalling helpers to find which entry in the + // per-resource map blew up. + return nil, fmt.Errorf("specToPB %q: %w", k, err) } pbSpecs[k] = ps } @@ -45,7 +50,18 @@ func detectDriftConfigTyped(ctx context.Context, cli pb.IaCProviderDriftConfigDe Specs: pbSpecs, }) if err != nil { - return nil, err + // Per code-review IMPORTANT-1 (PR 618 round 4): translate + // codes.Unimplemented at the wire boundary to + // interfaces.ErrProviderMethodUnimplemented so callers using + // errors.Is to detect "optional capability absent at runtime" + // keep the signal. Without this, a plugin that registered the + // IaCProviderDriftConfigDetector service but returns Unimplemented + // at the RPC level (e.g., a provider whose DriftConfigDetector + // is wired but the underlying driver doesn't support the + // resource type) would surface as a generic gRPC error rather + // than the iterate-and-skip sentinel. ADR-0028 §Migration's + // "Strict-mode invariant translation" depends on this. + return nil, translateRPCErr(err) } return driftsFromPB(resp.GetDrifts()) } diff --git a/cmd/wfctl/infra_align.go b/cmd/wfctl/infra_align.go index 5b371aa6..b59926b6 100644 --- a/cmd/wfctl/infra_align.go +++ b/cmd/wfctl/infra_align.go @@ -69,7 +69,7 @@ func runInfraAlign(args []string) error { return fmt.Errorf("no config file specified and no infra.yaml found") } - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { return err } @@ -99,8 +99,14 @@ func runInfraAlign(args []string) error { // runInfraAlignChecks runs all alignment rule families and returns findings. // This is separated from runInfraAlign to make it testable. -func runInfraAlignChecks(opts alignOptions) ([]AlignFinding, error) { - ctx, err := buildAlignContext(opts.configFile) +// +// Per code-review IMPORTANT-2 (PR 618 round 4): takes a context.Context so +// the R-A10 typed-RPC ValidatePlan dispatch honors operator Ctrl-C / parent +// cancellation / RPC deadline propagation. The legacy interfaces.X dispatch +// inherited the calling-goroutine's context implicitly via the Go method's +// signature; the typed-pb path requires explicit ctx threading. +func runInfraAlignChecks(ctx context.Context, opts alignOptions) ([]AlignFinding, error) { + alignCtx, err := buildAlignContext(opts.configFile) if err != nil { return nil, err } @@ -108,22 +114,22 @@ func runInfraAlignChecks(opts alignOptions) ([]AlignFinding, error) { var findings []AlignFinding // R-A1: container/runtime alignment - findings = append(findings, checkRA1(ctx)...) + findings = append(findings, checkRA1(alignCtx)...) // R-A2: health-check alignment - findings = append(findings, checkRA2(ctx, opts.strictHealth)...) + findings = append(findings, checkRA2(alignCtx, opts.strictHealth)...) // R-A3: service-to-service DNS alignment - findings = append(findings, checkRA3(ctx)...) + findings = append(findings, checkRA3(alignCtx)...) // R-A4: env-var resolution - findings = append(findings, checkRA4(ctx)...) + findings = append(findings, checkRA4(alignCtx)...) // R-A5: migrations alignment - findings = append(findings, checkRA5(ctx)...) + findings = append(findings, checkRA5(alignCtx)...) // R-A6: network/exposure alignment - findings = append(findings, checkRA6(ctx)...) + findings = append(findings, checkRA6(alignCtx)...) // Load plan once if --plan provided; reused by R-A7 and R-A10 to avoid // duplicate file I/O + JSON parsing (and to keep the two rules consistent @@ -143,18 +149,18 @@ func runInfraAlignChecks(opts alignOptions) ([]AlignFinding, error) { } // R-A8: WebAuthn alignment - findings = append(findings, checkRA8(ctx)...) + findings = append(findings, checkRA8(alignCtx)...) // R-A9: suspicious provider_credential key suffix - findings = append(findings, checkRA9(ctx)...) + findings = append(findings, checkRA9(alignCtx)...) // R-A10: provider.ValidatePlan dispatch — only when --plan is provided. // alignLoadProviders is a test seam; the default loader enumerates - // iac.provider modules in ctx.Config (already parsed) and loads each via - // the existing resolveIaCProvider plugin path. Closers (if any) are + // iac.provider modules in alignCtx.modules (already parsed) and loads each + // via the existing resolveIaCProvider plugin path. Closers (if any) are // released after the rule runs. if plan != nil { - providers, closers, err := alignLoadProviders(ctx, opts.envName, plan) + providers, closers, err := alignLoadProviders(alignCtx, opts.envName, plan) if err != nil { return nil, fmt.Errorf("load providers for R-A10: %w", err) } @@ -168,7 +174,7 @@ func runInfraAlignChecks(opts alignOptions) ([]AlignFinding, error) { } } }() - findings = append(findings, checkRA10_provider_validate_plan(providers, plan)...) + findings = append(findings, checkRA10_provider_validate_plan(ctx, providers, plan)...) } return findings, nil diff --git a/cmd/wfctl/infra_align_integration_test.go b/cmd/wfctl/infra_align_integration_test.go index ffba1066..7479ae0a 100644 --- a/cmd/wfctl/infra_align_integration_test.go +++ b/cmd/wfctl/infra_align_integration_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "os" "path/filepath" "testing" @@ -77,7 +78,7 @@ modules: cfg := writeAlignYAML(t, yamlContent) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/cmd/wfctl/infra_align_ra10_test.go b/cmd/wfctl/infra_align_ra10_test.go index c4c719aa..25e40870 100644 --- a/cmd/wfctl/infra_align_ra10_test.go +++ b/cmd/wfctl/infra_align_ra10_test.go @@ -87,20 +87,20 @@ func TestCheckRA10_NilPlan(t *testing.T) { {Severity: interfaces.PlanDiagnosticError, Message: "boom"}, }), } - if got := checkRA10_provider_validate_plan(providers, nil); len(got) != 0 { + if got := checkRA10_provider_validate_plan(context.Background(), providers, nil); len(got) != 0 { t.Errorf("expected no findings when plan is nil, got: %+v", got) } } func TestCheckRA10_NoProviders(t *testing.T) { - if got := checkRA10_provider_validate_plan(nil, &interfaces.IaCPlan{}); len(got) != 0 { + if got := checkRA10_provider_validate_plan(context.Background(), nil, &interfaces.IaCPlan{}); len(got) != 0 { t.Errorf("expected no findings when providers empty, got: %+v", got) } } func TestCheckRA10_NonValidatingProviderSkipped(t *testing.T) { providers := []interfaces.IaCProvider{stubIaCProvider(t, "plain")} - got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) + got := checkRA10_provider_validate_plan(context.Background(), providers, &interfaces.IaCPlan{}) if len(got) != 0 { t.Errorf("expected no findings when provider does not implement ProviderValidator, got: %+v", got) } @@ -117,7 +117,7 @@ func TestCheckRA10_ErrorDiagnostic_BecomesFAIL(t *testing.T) { }, }), } - got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) + got := checkRA10_provider_validate_plan(context.Background(), providers, &interfaces.IaCPlan{}) if len(got) != 1 { t.Fatalf("expected 1 finding, got %d: %+v", len(got), got) } @@ -145,7 +145,7 @@ func TestCheckRA10_WarningDiagnostic_BecomesWARN(t *testing.T) { {Severity: interfaces.PlanDiagnosticWarning, Resource: "vpc-prod", Message: "deprecated region"}, }), } - got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) + got := checkRA10_provider_validate_plan(context.Background(), providers, &interfaces.IaCPlan{}) if len(got) != 1 { t.Fatalf("expected 1 finding, got %d", len(got)) } @@ -171,7 +171,7 @@ func TestCheckRA10_InfoDiagnostic_LogsAndEmitsNoFinding(t *testing.T) { {Severity: interfaces.PlanDiagnosticInfo, Resource: "lb", Field: "tier", Message: "hint about tier"}, }), } - got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) + got := checkRA10_provider_validate_plan(context.Background(), providers, &interfaces.IaCPlan{}) if len(got) != 0 { t.Fatalf("expected 0 findings for Info diagnostic, got %d: %+v", len(got), got) } @@ -208,7 +208,7 @@ func TestCheckRA10_PlanLevelDiagnostic_UsesProviderName(t *testing.T) { {Severity: interfaces.PlanDiagnosticError, Message: "plan-level constraint"}, }), } - got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) + got := checkRA10_provider_validate_plan(context.Background(), providers, &interfaces.IaCPlan{}) if len(got) != 1 { t.Fatalf("expected 1 finding, got %d", len(got)) } @@ -234,7 +234,7 @@ func TestCheckRA10_PlanLevelInfoDiagnostic_LogsAsProviderSlashPlan(t *testing.T) {Severity: interfaces.PlanDiagnosticInfo, Message: "plan-level hint"}, }), } - _ = checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) + _ = checkRA10_provider_validate_plan(context.Background(), providers, &interfaces.IaCPlan{}) logged := buf.String() if !strings.Contains(logged, "do/plan") { t.Errorf("log: expected `do/plan` for plan-level Info, got %q", logged) @@ -255,7 +255,7 @@ func TestCheckRA10_MultipleProviders_OnlyValidatorsContribute(t *testing.T) { // the rule handles empty slices. validatingStubProvider(t, "aws", nil), } - got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) + got := checkRA10_provider_validate_plan(context.Background(), providers, &interfaces.IaCPlan{}) if len(got) != 1 { t.Fatalf("expected 1 finding (from 'do' only), got %d: %+v", len(got), got) } @@ -302,7 +302,7 @@ modules: planFile := writeAlignPlanJSON(t, plan) opts := alignOptions{configFile: cfgFile, planFile: planFile} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("runInfraAlignChecks: %v", err) } @@ -337,7 +337,7 @@ modules: cfgFile := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfgFile} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("runInfraAlignChecks: %v", err) } diff --git a/cmd/wfctl/infra_align_rules.go b/cmd/wfctl/infra_align_rules.go index a9fd4a4c..5f22636c 100644 --- a/cmd/wfctl/infra_align_rules.go +++ b/cmd/wfctl/infra_align_rules.go @@ -769,7 +769,11 @@ var ra10LogInfo = func(format string, args ...any) { // Naming follows the plan T4.2 spec literally; existing rule helpers use the // shorter checkRA form, but the descriptive suffix here documents the // rule's intent at the call site in infra_align.go. -func checkRA10_provider_validate_plan(providers []interfaces.IaCProvider, plan *interfaces.IaCPlan) []AlignFinding { +// checkRA10_provider_validate_plan dispatches the R-A10 ValidatePlan rule +// across all loaded providers. Per code-review IMPORTANT-2 (PR 618 round 4): +// takes ctx so the typed-RPC ValidatePlan call honors caller cancellation / +// deadline rather than dropping it via context.Background(). +func checkRA10_provider_validate_plan(ctx context.Context, providers []interfaces.IaCProvider, plan *interfaces.IaCPlan) []AlignFinding { if plan == nil || len(providers) == 0 { return nil } @@ -787,7 +791,7 @@ func checkRA10_provider_validate_plan(providers []interfaces.IaCProvider, plan * if cli == nil { continue } - diags := validatePlanTyped(context.Background(), cli, plan) + diags := validatePlanTyped(ctx, cli, plan) for _, d := range diags { // resource: rendered table label (provider-qualified for plan- // level findings so the table always identifies the source). diff --git a/cmd/wfctl/infra_align_test.go b/cmd/wfctl/infra_align_test.go index b193de2d..a4d55a4e 100644 --- a/cmd/wfctl/infra_align_test.go +++ b/cmd/wfctl/infra_align_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "encoding/json" "errors" iofs "io/fs" @@ -85,7 +86,7 @@ modules: // ci.build exists but has no container named "myapp" → orphaned reference cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -111,7 +112,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -147,7 +148,7 @@ modules: ` cfg := writeAlignYAML(t, yamlContent) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -180,7 +181,7 @@ modules: ` cfg := writeAlignYAML(t, yamlContent) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -215,7 +216,7 @@ modules: ` cfg := writeAlignYAML(t, yamlContent) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -249,7 +250,7 @@ modules: ` cfg := writeAlignYAML(t, yamlContent) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -281,7 +282,7 @@ modules: ` cfg := writeAlignYAML(t, yamlContent) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -306,7 +307,7 @@ modules: // No container_service named redis-cache cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -334,7 +335,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -362,7 +363,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -388,7 +389,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -411,7 +412,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -440,7 +441,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -469,7 +470,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -497,7 +498,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -550,7 +551,7 @@ modules: } opts := alignOptions{configFile: mainPath} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -582,7 +583,7 @@ modules: // No infra.database module — FAIL cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -618,7 +619,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -641,7 +642,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -661,7 +662,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -681,7 +682,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -719,7 +720,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg, planFile: planFile} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -755,7 +756,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg, planFile: planFile} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -785,7 +786,7 @@ func TestInfraAlign_RA7_TooManyChanges_Warns(t *testing.T) { planFile := writeAlignPlanJSON(t, plan) cfg := writeAlignYAML(t, `modules: []`) opts := alignOptions{configFile: cfg, planFile: planFile, maxChanges: 50} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -810,7 +811,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -833,7 +834,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -858,7 +859,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1006,7 +1007,7 @@ modules: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1040,7 +1041,7 @@ secrets: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1067,7 +1068,7 @@ secrets: ` cfg := writeAlignYAML(t, yaml) opts := alignOptions{configFile: cfg, strict: true} - findings, err := runInfraAlignChecks(opts) + findings, err := runInfraAlignChecks(context.Background(), opts) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/cmd/wfctl/infra_cleanup.go b/cmd/wfctl/infra_cleanup.go index 0a114964..e8e0ac15 100644 --- a/cmd/wfctl/infra_cleanup.go +++ b/cmd/wfctl/infra_cleanup.go @@ -115,6 +115,14 @@ func runInfraCleanup(args []string) error { //nolint:cyclop } resp, err := enumCli.EnumerateByTag(ctx, &pb.EnumerateByTagRequest{Tag: *tag}) if err != nil { + // Per code-review IMPORTANT-1 (PR 618 round 4): translate + // codes.Unimplemented at the wire boundary to + // interfaces.ErrProviderMethodUnimplemented so callers using + // errors.Is downstream of the join keep the sentinel signal. + // The error still propagates loud (cleanup is single-shot per + // ADR-0028 §Per-site dispatch UX) — the translation just + // preserves classification for any retry / wrapper logic. + err = translateRPCErr(err) fmt.Fprintf(cleanupStderr, "%s: enumerate by tag %q: %v\n", p.Name(), *tag, err) totalErrs = append(totalErrs, fmt.Errorf("%s: enumerate: %w", p.Name(), err)) continue From a3fe5eae9e662f7d6bd0898aeb512e5280166d3a Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 11:02:10 -0400 Subject: [PATCH 6/6] fix(wfctl): signal.NotifyContext + status-drift comment + fixture marshal-fail (PR 618 round 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per code-review round 5 follow-ups (3 Copilot findings on round 4 head): 1. cmd/wfctl/infra_status_drift.go:103-110 (was MINOR-4 corrigendum) Comment said "Hard-fail when provider isn't a typed adapter" but the implementation soft-skips (warn + return false). Updated the comment to match ADR-0028 §Per-site dispatch UX: status-drift iterates per provider, halting the whole status command on the first non-typed provider would lose visibility into the others' drift, so the warn- log + no-drift-reported degradation is operationally correct. The warning log is the auditable signal of fixture-leak / loader-gate gaps. 2. cmd/wfctl/infra_align.go:75 (REAL — IMPORTANT-2 intent gap) Round-4 fix threaded ctx through the dispatch chain but called runInfraAlignChecks with context.Background() at the entry point — defeating IMPORTANT-2's cancellation-propagation intent. Wired signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) at runInfraAlign so operator Ctrl-C / SIGTERM cancels in-flight typed- RPC calls (R-A10 ValidatePlan + any future typed dispatch the rule layer adds). The other wfctl runInfra* entrypoints (status, drift, apply, destroy, import, etc.) currently use context.Background() directly and do NOT honor signals; the signal-aware pattern landing here is the operator-tooling shape we want, but a follow-up sweep to wire it into the other entrypoints is out of scope for this PR (signal-cancellation-for-the-CLI is a horizontal concern bigger than Task 17). Documented in the inline comment so a future contributor sees the intentional asymmetry. 3. cmd/wfctl/iac_typed_fixture_test.go:280-308 (REAL — test rigor) driftsToPBOrEmpty silently swallowed marshalJSONMap errors via `_, _ := ...`. A fixture author who hands the recording server an un-marshallable Expected/Actual map would have seen a silently-empty ExpectedJson on the wire — false-pass shape. Fix: renamed to driftsToPB returning (slice, error); per-entry errors include index + resource name for triage. recordingDriftDetectorServer now stores the pre-marshalled []*pb.DriftResult (pbDrifts) so the gRPC handler is alloc-only, no marshal failure mode at RPC time. newRefreshDriftFixture pre-marshals at fixture-build time and t.Fatalf on any error — fixture-leak now fails deterministically at test setup (option 1 from code-review brief). Local validation: GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 # all PASS (8.3s) GOWORK=off go test ./cmd/wfctl/ -count=1 -race # all PASS (10.6s) GOWORK=off golangci-lint run ./cmd/wfctl/... # 0 issues Co-Authored-By: Claude Opus 4.7 --- cmd/wfctl/iac_typed_fixture_test.go | 50 +++++++++++++++++---------- cmd/wfctl/infra_align.go | 20 ++++++++++- cmd/wfctl/infra_apply_refresh_test.go | 14 ++++++-- cmd/wfctl/infra_status_drift.go | 10 ++++-- 4 files changed, 70 insertions(+), 24 deletions(-) diff --git a/cmd/wfctl/iac_typed_fixture_test.go b/cmd/wfctl/iac_typed_fixture_test.go index 7ed25a5d..574cabee 100644 --- a/cmd/wfctl/iac_typed_fixture_test.go +++ b/cmd/wfctl/iac_typed_fixture_test.go @@ -41,6 +41,7 @@ package main import ( "context" + "fmt" "net" "sync" "testing" @@ -181,10 +182,10 @@ type recordingEnumeratorServer struct { mu sync.Mutex // Canned responses (write once before run, read after). - tagRefs []interfaces.ResourceRef - allOutputs []*pb.ResourceOutput - enumerateTagErr error - enumerateAllErr error + tagRefs []interfaces.ResourceRef + allOutputs []*pb.ResourceOutput + enumerateTagErr error + enumerateAllErr error // Recorded inputs (read after run, optional assertion). enumerateAllType string @@ -254,13 +255,20 @@ func (s *recordingResourceDriverServer) callCount() int { // recordingDriftDetectorServer returns canned DetectDrift responses. Used by // infra apply-refresh fixtures that previously injected a fake // interfaces.IaCProvider whose DetectDrift returned canned []DriftResult. +// +// pbDrifts stores the pre-marshalled proto-wire shape so the gRPC handler +// emits the canned response without any encode-time failure mode at RPC +// time. Construction-time marshalling (driftsToPB → t.Fatalf at fixture +// build) means a fixture author that supplies un-marshallable +// Expected/Actual maps sees the failure deterministically at test setup +// rather than via a silently-empty ExpectedJson on the wire. type recordingDriftDetectorServer struct { pb.UnimplementedIaCProviderDriftDetectorServer mu sync.Mutex - driftResults []interfaces.DriftResult - driftErr error + pbDrifts []*pb.DriftResult + driftErr error } func (s *recordingDriftDetectorServer) DetectDrift(_ context.Context, _ *pb.DetectDriftRequest) (*pb.DetectDriftResponse, error) { @@ -269,22 +277,28 @@ func (s *recordingDriftDetectorServer) DetectDrift(_ context.Context, _ *pb.Dete if s.driftErr != nil { return nil, s.driftErr } - return &pb.DetectDriftResponse{Drifts: driftsToPBOrEmpty(s.driftResults)}, nil + return &pb.DetectDriftResponse{Drifts: append([]*pb.DriftResult(nil), s.pbDrifts...)}, nil } -// driftsToPBOrEmpty converts a slice of engine-side DriftResults to the -// proto wire shape, mirroring the inverse driftsFromPB helper in -// iac_typed_adapter.go. Extracted here so test fixtures can declare -// engine-side drift results and have the canned server emit them on the -// wire identically to how a real plugin would. -func driftsToPBOrEmpty(drifts []interfaces.DriftResult) []*pb.DriftResult { +// driftsToPB converts a slice of engine-side DriftResults to the proto wire +// shape, mirroring the inverse driftsFromPB helper in iac_typed_adapter.go. +// Returns an error if any DriftResult's Expected or Actual map fails to +// marshal to JSON so callers (fixture builders) can fail-fast at test setup +// rather than emitting a silently-truncated response on the wire. +func driftsToPB(drifts []interfaces.DriftResult) ([]*pb.DriftResult, error) { if len(drifts) == 0 { - return nil + return nil, nil } out := make([]*pb.DriftResult, 0, len(drifts)) - for _, d := range drifts { - expectedJSON, _ := marshalJSONMap(d.Expected) // marshalJSONMap nil-safe - actualJSON, _ := marshalJSONMap(d.Actual) + for i, d := range drifts { + expectedJSON, err := marshalJSONMap(d.Expected) + if err != nil { + return nil, fmt.Errorf("drifts[%d].Expected (resource %q): %w", i, d.Name, err) + } + actualJSON, err := marshalJSONMap(d.Actual) + if err != nil { + return nil, fmt.Errorf("drifts[%d].Actual (resource %q): %w", i, d.Name, err) + } out = append(out, &pb.DriftResult{ Name: d.Name, Type: d.Type, @@ -295,5 +309,5 @@ func driftsToPBOrEmpty(drifts []interfaces.DriftResult) []*pb.DriftResult { Fields: append([]string(nil), d.Fields...), }) } - return out + return out, nil } diff --git a/cmd/wfctl/infra_align.go b/cmd/wfctl/infra_align.go index b59926b6..a435b1a5 100644 --- a/cmd/wfctl/infra_align.go +++ b/cmd/wfctl/infra_align.go @@ -9,7 +9,9 @@ import ( "io" iofs "io/fs" "os" + "os/signal" "strings" + "syscall" "github.com/GoCodeAlone/workflow/config" "github.com/GoCodeAlone/workflow/interfaces" @@ -69,7 +71,23 @@ func runInfraAlign(args []string) error { return fmt.Errorf("no config file specified and no infra.yaml found") } - findings, err := runInfraAlignChecks(context.Background(), opts) + // Per code-review round 5 IMPORTANT-2 follow-up: bind a + // signal.NotifyContext so operator Ctrl-C / SIGTERM cancels + // in-flight typed-RPC calls (R-A10 ValidatePlan, plus any + // downstream typed dispatch that honors ctx). Without this, + // the ctx threaded through runInfraAlignChecks is a bare + // context.Background() that no signal can interrupt — defeating + // IMPORTANT-2's intent (cancellation propagation, not just + // the function-signature shape). Other wfctl runInfra* + // entrypoints (status, drift, apply, etc.) currently use + // context.Background() directly and do not honor signals; the + // signal-aware pattern landing here is the operator-tooling + // shape we want, and a follow-up sweep can wire it into the + // other entrypoints once this PR lands. + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer stop() + + findings, err := runInfraAlignChecks(ctx, opts) if err != nil { return err } diff --git a/cmd/wfctl/infra_apply_refresh_test.go b/cmd/wfctl/infra_apply_refresh_test.go index 331268e8..99cf4136 100644 --- a/cmd/wfctl/infra_apply_refresh_test.go +++ b/cmd/wfctl/infra_apply_refresh_test.go @@ -29,13 +29,23 @@ import ( // nil) or states with no AppliedConfigSource ("apply") provenance (also // returns nil). The dispatcher therefore falls back to the required-side // DetectDrift path which the IaCProviderDriftDetector service backs. +// +// driftResults is pre-marshalled to the pb wire shape at fixture-build +// time (driftsToPB). Per code-review round 5 finding #3: a fixture that +// supplies an un-marshallable Expected/Actual map fails fast here via +// t.Fatalf rather than silently emitting an empty ExpectedJson at RPC +// time. func newRefreshDriftFixture(t *testing.T, driftResults []interfaces.DriftResult, driftErr error) interfaces.IaCProvider { t.Helper() + pbDrifts, err := driftsToPB(driftResults) + if err != nil { + t.Fatalf("newRefreshDriftFixture: pre-marshal driftResults: %v", err) + } return fixtureTypedAdapter{ Required: &fixtureRequiredServer{name: "fake-refresh", version: "0.0.0"}, DriftDetector: &recordingDriftDetectorServer{ - driftResults: driftResults, - driftErr: driftErr, + pbDrifts: pbDrifts, + driftErr: driftErr, }, }.build(t) } diff --git a/cmd/wfctl/infra_status_drift.go b/cmd/wfctl/infra_status_drift.go index 9f038e96..31e5a9b0 100644 --- a/cmd/wfctl/infra_status_drift.go +++ b/cmd/wfctl/infra_status_drift.go @@ -101,9 +101,13 @@ func driftInfraModules(ctx context.Context, cfgFile, envName string) error { } // Per Task 17 of the strict-contracts force-cutover (ADR-0028): - // pure typed-pb dispatch — no interfaces.X fallback. Hard-fail - // when provider isn't a typed adapter so test-fixture leaks - // don't silently mask production-shape regressions. + // pure typed-pb dispatch — no interfaces.X fallback. Soft-skip + // per ADR-0028 §Per-site dispatch UX: status-drift iterates per + // provider, so halting the whole status command on the first + // non-typed provider would lose visibility into the others' + // drift. Warn + return-no-drift is the iteration-friendly + // degradation; the warning log is the auditable signal of the + // fixture-leak / loader-gate gap. adapter, ok := provider.(*typedIaCAdapter) if !ok { fmt.Printf("WARNING: provider %q (%T) is not a typed IaC adapter — re-load via discoverAndLoadIaCProvider\n", moduleRef, provider)