diff --git a/cmd/wfctl/iac_typed_adapter.go b/cmd/wfctl/iac_typed_adapter.go index dccbbc32..2cbbfd36 100644 --- a/cmd/wfctl/iac_typed_adapter.go +++ b/cmd/wfctl/iac_typed_adapter.go @@ -108,6 +108,104 @@ 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. 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 { +// 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}) +// // ... +// } +// +// // 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 (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 +// 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..eb2457a5 --- /dev/null +++ b/cmd/wfctl/iac_typed_dispatch.go @@ -0,0 +1,99 @@ +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" + "fmt" + + "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 { + // 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 + } + resp, err := cli.DetectDriftConfig(ctx, &pb.DetectDriftConfigRequest{ + Refs: refsToPB(refs), + Specs: pbSpecs, + }) + 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 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()) +} + +// 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/iac_typed_fixture_test.go b/cmd/wfctl/iac_typed_fixture_test.go new file mode 100644 index 00000000..574cabee --- /dev/null +++ b/cmd/wfctl/iac_typed_fixture_test.go @@ -0,0 +1,313 @@ +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" + "fmt" + "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. +// +// 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 + + pbDrifts []*pb.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: append([]*pb.DriftResult(nil), s.pbDrifts...)}, nil +} + +// 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, nil + } + out := make([]*pb.DriftResult, 0, len(drifts)) + 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, + Drifted: d.Drifted, + Class: driftClassToPB(d.Class), + ExpectedJson: expectedJSON, + ActualJson: actualJSON, + Fields: append([]string(nil), d.Fields...), + }) + } + return out, nil +} diff --git a/cmd/wfctl/infra_align.go b/cmd/wfctl/infra_align.go index 5b371aa6..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(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 } @@ -99,8 +117,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 +132,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 +167,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 +192,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 deab777c..25e40870 100644 --- a/cmd/wfctl/infra_align_ra10_test.go +++ b/cmd/wfctl/infra_align_ra10_test.go @@ -8,82 +8,99 @@ 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 { + 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{name: "plain"}} - got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) + providers := []interfaces.IaCProvider{stubIaCProvider(t, "plain")} + 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) } @@ -91,19 +108,16 @@ 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{}) + 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) } @@ -127,14 +141,11 @@ 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{}) + got := checkRA10_provider_validate_plan(context.Background(), providers, &interfaces.IaCPlan{}) if len(got) != 1 { t.Fatalf("expected 1 finding, got %d", len(got)) } @@ -156,14 +167,11 @@ 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{}) + 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) } @@ -196,14 +204,11 @@ 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{}) + got := checkRA10_provider_validate_plan(context.Background(), providers, &interfaces.IaCPlan{}) if len(got) != 1 { t.Fatalf("expected 1 finding, got %d", len(got)) } @@ -225,14 +230,11 @@ 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{}) + _ = 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) @@ -244,20 +246,16 @@ 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. - }, - } - got := checkRA10_provider_validate_plan(providers, &interfaces.IaCPlan{}) + 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(context.Background(), providers, &interfaces.IaCPlan{}) if len(got) != 1 { t.Fatalf("expected 1 finding (from 'do' only), got %d: %+v", len(got), got) } @@ -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 } @@ -307,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) } @@ -342,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 4d7a5b3f..5f22636c 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" @@ -768,17 +769,29 @@ 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 } var findings []AlignFinding for _, p := range providers { - v, ok := p.(interfaces.ProviderValidator) + // 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 } - diags := v.ValidatePlan(plan) + cli := adapter.Validator() + if cli == nil { + continue + } + 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_apply_refresh.go b/cmd/wfctl/infra_apply_refresh.go index 8b29dcea..7456166c 100644 --- a/cmd/wfctl/infra_apply_refresh.go +++ b/cmd/wfctl/infra_apply_refresh.go @@ -61,15 +61,21 @@ 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 (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 d, ok := provider.(interfaces.DriftConfigDetector); ok { + if cli := adapter.DriftConfigDetector(); cli != nil { specsMap := buildAppliedSpecMap(states, refs) if specsMap != nil { - results, err = d.DetectDriftWithSpecs(ctx, refs, specsMap) + results, err = detectDriftConfigTyped(ctx, cli, refs, specsMap) } else { results, err = provider.DetectDrift(ctx, refs) } diff --git a/cmd/wfctl/infra_apply_refresh_test.go b/cmd/wfctl/infra_apply_refresh_test.go index 978d4bc5..99cf4136 100644 --- a/cmd/wfctl/infra_apply_refresh_test.go +++ b/cmd/wfctl/infra_apply_refresh_test.go @@ -10,49 +10,45 @@ 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. +// +// 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{ + pbDrifts: pbDrifts, + 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 +90,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 +119,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 +152,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 +179,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 +202,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 +213,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 +235,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 +267,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 +310,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 +341,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_bootstrap.go b/cmd/wfctl/infra_bootstrap.go index 1d062463..af28e3e5 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,12 +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 } - r, ok := iacProv.(interfaces.ProviderCredentialRevoker) + // 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 does not implement ProviderCredentialRevoker — old credential will NOT be revoked automatically\n") + 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 + } + 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, @@ -731,6 +753,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..e8e0ac15 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,27 +95,39 @@ func runInfraCleanup(args []string) error { //nolint:cyclop var totalErrs []error for _, p := range providers { - enum, ok := p.(interfaces.Enumerator) + // 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 } - 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) { - 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)) + 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 } + 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_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_status_drift.go b/cmd/wfctl/infra_status_drift.go index f9126deb..31e5a9b0 100644 --- a/cmd/wfctl/infra_status_drift.go +++ b/cmd/wfctl/infra_status_drift.go @@ -100,14 +100,24 @@ 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 (ADR-0028): + // 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) + return false + } var results []interfaces.DriftResult - if d, ok := provider.(interfaces.DriftConfigDetector); ok { + if cli := adapter.DriftConfigDetector(); cli != nil { specsMap := buildAppliedSpecMap(states, g.refs) if specsMap != nil { - results, err = d.DetectDriftWithSpecs(ctx, g.refs, specsMap) + results, err = detectDriftConfigTyped(ctx, cli, g.refs, specsMap) } else { results, err = provider.DetectDrift(ctx, g.refs) } 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") { 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..67bdbe16 --- /dev/null +++ b/decisions/0028-task-17-pure-typed-cutover.md @@ -0,0 +1,263 @@ +# 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. + +## 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 +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.