From 38b5e0243e8594efd270be443c1c6de6377a5610 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 02:47:53 -0400 Subject: [PATCH 1/3] feat(plugin/external): expose grpc.ClientConn via PluginClient.Conn() / ExternalPluginAdapter.Conn() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Architectural prerequisite for plan §Task 16 (wfctl typed-IaC cutover). The current PluginClient drops the *grpc.ClientConn after constructing its embedded pb.PluginServiceClient, so callers had no way to build additional typed gRPC service clients (e.g. pb.IaCProviderRequiredClient, pb.ResourceDriverClient) against the same plugin process. The legacy remoteIaCProvider sidestepped this by routing every call through PluginServiceClient.InvokeService string-dispatch — which is the bug class force-cutover Task 16 is closing. Surface (additive — zero call-site impact): - `PluginClient.conn *grpc.ClientConn` — retained from GRPCPlugin GRPCClient construction. - `(p *PluginClient) Conn() *grpc.ClientConn` — opaque accessor; exposed via method (not public field) so the rest of PluginClient stays internal. - `(a *ExternalPluginAdapter) Conn() *grpc.ClientConn` — delegates to client.Conn(); nil-safe for adapters constructed via `newExternalPluginAdapterWithContractRegistry` (test fixtures). The connection lifecycle is owned by the host's plugin manager — callers MUST NOT Close() the returned conn. The plugin shutdown path tears it down via the registered Closer; closing it externally would break every other typed-client constructed against the same process. ## Plan-correction notes This commit is NOT in plan §Task 16 Files: section (spec gap): - Spec assumes the typed pb.IaCProviderRequiredClient can be constructed from the existing plugin loader output, but the plugin/external surface as it stands strips the underlying conn. Task 16 is physically impossible without first exposing it. Per scope-lock skill the prerequisite lands in the SAME PR as the Task 16 cutover (this branch) rather than as a separate PR — same precedent as Task 2's plan-correction notes block. Documented in the PR description. Local validation: GOWORK=off go build ./plugin/external/... → clean GOWORK=off go vet ./plugin/external/... → clean GOWORK=off go test ./plugin/external/... -count=1 -short → all PASS Co-Authored-By: Claude Opus 4.7 --- plugin/external/adapter.go | 21 +++++++++++++++++++++ plugin/external/grpc_plugin.go | 23 +++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/plugin/external/adapter.go b/plugin/external/adapter.go index 40db9ee3..181ac26b 100644 --- a/plugin/external/adapter.go +++ b/plugin/external/adapter.go @@ -13,6 +13,7 @@ import ( "github.com/GoCodeAlone/workflow/plugin" pb "github.com/GoCodeAlone/workflow/plugin/external/proto" "github.com/GoCodeAlone/workflow/schema" + "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/reflect/protodesc" @@ -357,6 +358,26 @@ func (a *ExternalPluginAdapter) ContractRegistryError() error { return a.contractRegistryErr } +// Conn returns the underlying gRPC client connection used to talk to +// the plugin process. Callers (notably wfctl's typed-IaC cutover in +// plan Task 16) construct additional typed gRPC service clients +// against this conn — for example +// `pb.NewIaCProviderRequiredClient(adapter.Conn())`. +// +// Returns nil for adapters constructed without a backing client (e.g. +// `newExternalPluginAdapterWithContractRegistry` test fixtures); callers +// MUST nil-check before constructing typed clients. +// +// The connection lifecycle is owned by the host's plugin manager — +// callers MUST NOT call Close() on the returned conn. The plugin +// shutdown path tears it down via the registered Closer. +func (a *ExternalPluginAdapter) Conn() *grpc.ClientConn { + if a.client == nil { + return nil + } + return a.client.Conn() +} + func (a *ExternalPluginAdapter) Capabilities() []capability.Contract { return nil } diff --git a/plugin/external/grpc_plugin.go b/plugin/external/grpc_plugin.go index e55811fc..b0e7349e 100644 --- a/plugin/external/grpc_plugin.go +++ b/plugin/external/grpc_plugin.go @@ -44,6 +44,7 @@ func (p *GRPCPlugin) GRPCClient(_ context.Context, broker *goplugin.GRPCBroker, // We pass the broker ID via the initial GetManifest metadata or // a dedicated setup call. For simplicity, we embed it in the client wrapper. return &PluginClient{ + conn: c, client: client, broker: broker, callbackBrokerID: brokerID, @@ -51,14 +52,36 @@ func (p *GRPCPlugin) GRPCClient(_ context.Context, broker *goplugin.GRPCBroker, } return &PluginClient{ + conn: c, client: client, broker: broker, }, nil } // PluginClient wraps the gRPC client for the plugin service. +// +// The underlying *grpc.ClientConn is retained so callers that need to +// instantiate additional typed gRPC clients (e.g. the typed +// pb.IaCProviderRequiredClient that wfctl's typedIaCAdapter wraps in +// the strict-contracts cutover, plan Task 16) can do so without going +// through the legacy pb.PluginServiceClient string-dispatch path. +// Exposed via Conn() rather than as a public field so the rest of the +// PluginClient surface stays opaque. type PluginClient struct { + conn *grpc.ClientConn client pb.PluginServiceClient broker *goplugin.GRPCBroker callbackBrokerID uint32 } + +// Conn returns the underlying gRPC client connection to the plugin +// process. Callers MAY use it to construct additional typed gRPC +// service clients (for example pb.NewIaCProviderRequiredClient). +// +// The connection lifecycle is owned by the host's plugin manager — +// callers MUST NOT call Close() on it. The connection is shared across +// every typed-client constructed against it; closing it would tear +// down every other consumer too. +func (p *PluginClient) Conn() *grpc.ClientConn { + return p.conn +} From 156cfb3851f6ca61f59f8b9771f89b054eacfdd0 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 03:07:57 -0400 Subject: [PATCH 2/3] =?UTF-8?q?feat(wfctl):=20typed-IaC=20cutover=20?= =?UTF-8?q?=E2=80=94=20replace=20remoteIaCProvider=20with=20pb.IaCProvider?= =?UTF-8?q?RequiredClient=20(Task=2016)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See above commit body in this branch's PR description for full detail. Summary: - discoverAndLoadIaCProvider rewritten to construct typedIaCAdapter from adapter.Conn() + ContractRegistry-derived registered service map. - DELETED ~3856 lines: remoteIaCProvider, remoteResourceDriver, remoteServiceInvoker/ContextInvoker, jsonToAny, anyToStruct, sensitiveToAny, decodeResourceOutput, isPluginMethodUnimplemented, stringVal, stringFromMap, loadIaCPlugin var, defaultLoadIaCPlugin, readIaCPluginComputePlanVersion, plus 5 dependent test files (~2767 lines) and the loadIaCPlugin-using TestResolveIaCProviderSurfacesPluginError. - KEPT wrapIaCError + retryOnTransient + deployOpError as provider-agnostic helpers used by pluginDeployProvider against typed RPC errors. - ADDED registeredIaCServices(reg) helper. DEPENDENCY: branch CI red until PRs #598 (Task 3 proto) + #605 (Task 30 adapter) merge. Local validation passes against working-tree overlay of both: build clean, vet clean, ./cmd/wfctl/... -short PASS. Plan-correction notes (per Path A ruling): Conn() prerequisite shipped as commit ad7d946e in this same PR. SupportedCanonicalKeys regression acceptable transient (closes via follow-up additive PR per team-lead ruling). ComputePlanVersionDeclarer regression tracked for follow-up. Co-Authored-By: Claude Opus 4.7 --- cmd/wfctl/deploy_providers.go | 1026 +--------------- .../deploy_providers_dispatch_matrix_test.go | 477 -------- .../deploy_providers_remote_driver_test.go | 820 ------------- ...deploy_providers_remote_iac_compat_test.go | 65 - cmd/wfctl/deploy_providers_remote_iac_test.go | 1090 ----------------- ...y_providers_strict_bridge_coverage_test.go | 302 ----- cmd/wfctl/deploy_providers_test.go | 41 +- cmd/wfctl/deploy_remote_provider_test.go | 173 --- 8 files changed, 68 insertions(+), 3926 deletions(-) delete mode 100644 cmd/wfctl/deploy_providers_dispatch_matrix_test.go delete mode 100644 cmd/wfctl/deploy_providers_remote_driver_test.go delete mode 100644 cmd/wfctl/deploy_providers_remote_iac_compat_test.go delete mode 100644 cmd/wfctl/deploy_providers_remote_iac_test.go delete mode 100644 cmd/wfctl/deploy_providers_strict_bridge_coverage_test.go delete mode 100644 cmd/wfctl/deploy_remote_provider_test.go diff --git a/cmd/wfctl/deploy_providers.go b/cmd/wfctl/deploy_providers.go index 93118070..993db94d 100644 --- a/cmd/wfctl/deploy_providers.go +++ b/cmd/wfctl/deploy_providers.go @@ -18,13 +18,9 @@ import ( "time" "github.com/GoCodeAlone/workflow/config" - "github.com/GoCodeAlone/workflow/iac/wfctlhelpers" "github.com/GoCodeAlone/workflow/interfaces" - "github.com/GoCodeAlone/workflow/platform" - "github.com/GoCodeAlone/workflow/plugin" "github.com/GoCodeAlone/workflow/plugin/external" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" + pb "github.com/GoCodeAlone/workflow/plugin/external/proto" ) // DeployConfig holds all parameters needed to execute a deployment. @@ -148,757 +144,97 @@ func findIaCPluginDir(pluginDir, providerName string) (name, computePlanVersion return "", "", false, nil } -// loadIaCPlugin finds and loads the IaC plugin for the given provider, returning -// its name, module factories, and a closer that shuts down the plugin subprocess. -// Tests replace this var to inject fakes without touching the filesystem. -var loadIaCPlugin = defaultLoadIaCPlugin +// discoverAndLoadIaCProvider implements the default resolveIaCProvider: it +// scans the plugin directory for a plugin that declares +// iacProvider.name == providerName, loads it via ExternalPluginManager, and +// returns a typedIaCAdapter (satisfying interfaces.IaCProvider) plus a +// Closer that shuts down the plugin subprocess. The caller must call +// Close() on the returned Closer when done. +// +// Per plan §Task 16 (strict-contracts force-cutover, rev5): the loader +// constructs the typed pb.IaCProviderRequiredClient + per-optional-service +// clients directly from the plugin's gRPC connection and wraps them in +// typedIaCAdapter (Task 30, PR #605). The legacy remoteIaCProvider +// InvokeService string-dispatch surface is removed entirely — plugins +// that do not register the typed IaCProviderRequired service are +// rejected at load time with an actionable upgrade message. +func discoverAndLoadIaCProvider(ctx context.Context, providerName string, cfg map[string]any) (interfaces.IaCProvider, io.Closer, error) { + pluginDir := os.Getenv("WFCTL_PLUGIN_DIR") + if pluginDir == "" { + pluginDir = "./data/plugins" + } -func defaultLoadIaCPlugin(pluginDir, providerName string) (pluginName string, factories map[string]plugin.ModuleFactory, closer io.Closer, err error) { pName, _, hasBinary, findErr := findIaCPluginDir(pluginDir, providerName) if findErr != nil { - return "", nil, nil, fmt.Errorf("resolve IaC provider %q: %w", providerName, findErr) + return nil, nil, fmt.Errorf("resolve IaC provider %q: %w", providerName, findErr) } if pName == "" { - return "", nil, nil, fmt.Errorf("no plugin found for IaC provider %q in %s — run: wfctl plugin install ", providerName, pluginDir) + return nil, nil, fmt.Errorf("no plugin found for IaC provider %q in %s — run: wfctl plugin install ", providerName, pluginDir) } if !hasBinary { - return "", nil, nil, fmt.Errorf("plugin %q declares provider %q but binary is missing — run: wfctl plugin install %s", pName, providerName, pName) + return nil, nil, fmt.Errorf("plugin %q declares provider %q but binary is missing — run: wfctl plugin install %s", pName, providerName, pName) } + mgr := external.NewExternalPluginManager(pluginDir, nil) adapter, loadErr := mgr.LoadPlugin(pName) if loadErr != nil { mgr.Shutdown() - return "", nil, nil, fmt.Errorf("load plugin %q for provider %q: %w", pName, providerName, loadErr) - } - return pName, adapter.ModuleFactories(), closerFunc(func() error { mgr.Shutdown(); return nil }), nil -} - -// readIaCPluginComputePlanVersion re-reads plugin.json under -// pluginDir to extract iacProvider.computePlanVersion for providerName. -// Returns "" (treated as v1 by the dispatcher) when the manifest -// can't be read or the field is omitted. Callers MUST tolerate empty -// — apply behavior degrades to the legacy v1 path on any read -// failure rather than blocking the apply. -// -// Re-reads rather than threading the version through loadIaCPlugin's -// existing 4-tuple return so the var-seam signature (and its 1 test -// override) stays stable. Cost: one extra os.ReadFile of a tiny JSON -// file per provider load — negligible vs. the gRPC plugin start. -func readIaCPluginComputePlanVersion(pluginDir, providerName string) string { - _, version, _, err := findIaCPluginDir(pluginDir, providerName) - if err != nil { - return "" - } - return version -} - -// discoverAndLoadIaCProvider implements the default resolveIaCProvider: it scans -// the plugin directory for a plugin that declares iacProvider.name == providerName, -// loads it via ExternalPluginManager, and returns the IaCProvider plus a Closer -// that shuts down the plugin subprocess. The caller must call Close() when done. -func discoverAndLoadIaCProvider(ctx context.Context, providerName string, cfg map[string]any) (interfaces.IaCProvider, io.Closer, error) { - pluginDir := os.Getenv("WFCTL_PLUGIN_DIR") - if pluginDir == "" { - pluginDir = "./data/plugins" - } - - pluginName, factories, closer, err := loadIaCPlugin(pluginDir, providerName) - if err != nil { - return nil, nil, err + return nil, nil, fmt.Errorf("load plugin %q for provider %q: %w", pName, providerName, loadErr) } + closer := closerFunc(func() error { mgr.Shutdown(); return nil }) - factory, ok := factories["iac.provider"] - if !ok { + conn := adapter.Conn() + if conn == nil { _ = closer.Close() - return nil, nil, fmt.Errorf("plugin %q does not expose an iac.provider module type — upgrade with: wfctl plugin update %s", pluginName, pluginName) + return nil, nil, fmt.Errorf("plugin %q does not expose a gRPC connection (host adapter missing PluginClient.Conn) — upgrade with: wfctl plugin update %s", pName, pName) } - mod := factory("iac-provider", cfg) - if pluginErr := external.AsModuleError(mod); pluginErr != nil { - _ = closer.Close() - return nil, nil, fmt.Errorf("plugin %q iac.provider factory failed: %w", pluginName, pluginErr) - } - if mod == nil { + registered := registeredIaCServices(adapter.ContractRegistry()) + if !registered[iacServiceRequired] { _ = closer.Close() - return nil, nil, fmt.Errorf("plugin %q iac.provider factory returned nil (unexpected — file an issue)", pluginName) + return nil, nil, fmt.Errorf("plugin %q does not register the required %q gRPC service — upgrade with: wfctl plugin update %s", pName, iacServiceRequired, pName) } - // RemoteModule does not directly implement interfaces.IaCProvider; instead it - // exposes InvokeService for cross-process method dispatch. Wrap it in a - // remoteIaCProvider that routes each IaCProvider call through InvokeService. - invoker, ok := mod.(remoteServiceInvoker) - if !ok { - _ = closer.Close() - return nil, nil, fmt.Errorf("plugin %q iac.provider module (%T) does not support service invocation — upgrade with: wfctl plugin update %s", pluginName, mod, pluginName) - } - - iacProvider := &remoteIaCProvider{ - invoker: invoker, - computePlanVersion: readIaCPluginComputePlanVersion(pluginDir, providerName), - } - // Notify the plugin that Initialize has been called (the plugin may treat - // this as a no-op if it already ran Initialize inside CreateModule). - if initErr := iacProvider.Initialize(ctx, cfg); initErr != nil { + typed := newTypedIaCAdapter(conn, registered) + if initErr := typed.Initialize(ctx, cfg); initErr != nil { _ = closer.Close() return nil, nil, fmt.Errorf("initialize provider %q: %w", providerName, initErr) } - return iacProvider, closer, nil -} - -// remoteServiceInvoker is satisfied by *external.RemoteModule, which provides -// InvokeService for cross-process method dispatch. -type remoteServiceInvoker interface { - InvokeService(method string, args map[string]any) (map[string]any, error) + return typed, closer, nil } -type remoteServiceContextInvoker interface { - InvokeServiceContext(ctx context.Context, method string, args map[string]any) (map[string]any, error) -} - -// remoteIaCProvider implements interfaces.IaCProvider by routing every method -// through InvokeService to the plugin subprocess. Only the methods needed by -// wfctl ci run deploy are fully implemented; the rest return a clear error. -// -// W-3b T3.7: also satisfies wfctlhelpers.ComputePlanVersionDeclarer via -// ComputePlanVersion(), populated from the plugin.json SDK manifest at -// load time. wfctl's apply path type-asserts the interface to choose -// between v1 (legacy provider.Apply) and v2 (wfctlhelpers.ApplyPlan) -// dispatch. -type remoteIaCProvider struct { - invoker remoteServiceInvoker - computePlanVersion string -} - -// ComputePlanVersion satisfies wfctlhelpers.ComputePlanVersionDeclarer. -// Returns the SDK-manifest value cached at load time ("", "v1", or -// "v2"); empty defaults to v1 in the dispatcher. -func (r *remoteIaCProvider) ComputePlanVersion() string { - return r.computePlanVersion -} - -func (r *remoteIaCProvider) Name() string { - res, err := r.invoker.InvokeService("IaCProvider.Name", nil) - if err != nil { - return "" - } - name, _ := res["name"].(string) - return name -} - -func (r *remoteIaCProvider) Version() string { - res, err := r.invoker.InvokeService("IaCProvider.Version", nil) - if err != nil { - return "" - } - v, _ := res["version"].(string) - return v -} - -func (r *remoteIaCProvider) Initialize(_ context.Context, cfg map[string]any) error { - _, err := r.invoker.InvokeService("IaCProvider.Initialize", cfg) - return err -} - -// jsonToAny converts any typed value to a JSON-compatible any (map[string]any, -// []any, etc.) suitable for embedding in InvokeService arg maps. -func jsonToAny(v any) (any, error) { - b, err := json.Marshal(v) - if err != nil { - return nil, err - } - var out any - if err := json.Unmarshal(b, &out); err != nil { - return nil, err - } - return out, nil -} - -// anyToStruct decodes a JSON-compatible any value (map[string]any / []any) into -// a typed struct using a JSON round-trip. -func anyToStruct(src any, dst any) error { - b, err := json.Marshal(src) - if err != nil { - return err - } - return json.Unmarshal(b, dst) -} - -func (r *remoteIaCProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { - res, err := r.invoker.InvokeService("IaCProvider.Capabilities", nil) - if err != nil { +// registeredIaCServices walks a plugin's ContractRegistry response and +// collects the fully-qualified gRPC service names of every SERVICE-kind +// contract. typedIaCAdapter uses this map to gate optional-client +// construction — only services the plugin actually advertised get a +// typed client; the rest yield interfaces.ErrProviderMethodUnimplemented +// at call time so dispatch sites can errors.Is and skip the provider. +func registeredIaCServices(reg *pb.ContractRegistry) map[string]bool { + if reg == nil { return nil } - raw, ok := res["capabilities"] - if !ok { - return nil - } - var caps []interfaces.IaCCapabilityDeclaration - if err := anyToStruct(raw, &caps); err != nil { - return nil - } - return caps -} - -// Plan branches on the plugin manifest's iacProvider.computePlanVersion: -// -// - "v2" — delegates to platform.ComputePlan, the canonical wfctl-side -// diff engine. ResourceDriver.Diff for per-resource classification -// hits the plugin via remoteResourceDriver, so the gRPC fan-out is -// preserved while wfctl owns the plan-shape contract. -// - "" / "v1" / unknown — proxies the legacy monolithic IaCProvider.Plan -// call to the plugin via InvokeService, preserving the v1 contract -// for plugins that compute their own plans. This is the safe-default -// branch per dispatch.go's "default-to-v1" doctrine. -// -// The branch mirrors infra_apply.go's DispatchVersionFor gate so the -// wfctl-side proxy honors the same plugin-author-controlled routing that -// the in-tree apply path honors. PR #556 R1 review correction: the -// initial implementation collapsed both branches into the v2 path, which -// silently bypassed v1 plugins' own Plan endpoint via this proxy. -// -// wfctl:skip-iac-codemod (manifest-conditional dispatch is intentional — -// canonical-delegation lint expects unconditional v2 forms for plugin- -// side providers; this is a wfctl-side proxy that supports BOTH versions -// per dispatch.go's "default-to-v1" safety net.) -func (r *remoteIaCProvider) Plan(ctx context.Context, desired []interfaces.ResourceSpec, current []interfaces.ResourceState) (*interfaces.IaCPlan, error) { - if r.computePlanVersion == wfctlhelpers.DispatchVersionV2 { - plan, err := platform.ComputePlan(ctx, r, desired, current) - return &plan, err - } - desiredAny, err := jsonToAny(desired) - if err != nil { - return nil, fmt.Errorf("IaCProvider.Plan: marshal desired: %w", err) - } - currentAny, err := jsonToAny(current) - if err != nil { - return nil, fmt.Errorf("IaCProvider.Plan: marshal current: %w", err) - } - res, err := r.invoker.InvokeService("IaCProvider.Plan", map[string]any{ - "desired": desiredAny, - "current": currentAny, - }) - if err != nil { - return nil, err - } - var plan interfaces.IaCPlan - if err := anyToStruct(res, &plan); err != nil { - return nil, fmt.Errorf("IaCProvider.Plan: decode result: %w", err) - } - return &plan, nil -} - -// Apply branches on the plugin manifest's iacProvider.computePlanVersion, -// matching the same dispatch policy as Plan above: -// -// - "v2" — delegates to wfctlhelpers.ApplyPlan, which fans out per- -// action through ResourceDriver.{Create,Update,Delete,Replace} (the -// remoteResourceDriver proxies each over gRPC) and runs the -// drift-postcondition + per-action error decomposition. -// - "" / "v1" / unknown — proxies the legacy monolithic IaCProvider. -// Apply call to the plugin via InvokeService, preserving the v1 -// contract for plugins that own their own apply orchestration. -// -// The branch keeps the v1/v2 routing contract documented in dispatch.go -// intact: the dispatch gate in infra_apply.go and this proxy now agree. -// PR #556 R1 review correction (Copilot inline finding) — the initial -// implementation collapsed both branches into the v2 path, which -// silently broke v1 plugins routed through this proxy. -// -// wfctl:skip-iac-codemod (manifest-conditional dispatch is intentional — -// see Plan above for the canonical-delegation-lint rationale.) -func (r *remoteIaCProvider) Apply(ctx context.Context, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { - if r.computePlanVersion == wfctlhelpers.DispatchVersionV2 { - return wfctlhelpers.ApplyPlan(ctx, r, plan) - } - planAny, err := jsonToAny(plan) - if err != nil { - return nil, fmt.Errorf("IaCProvider.Apply: marshal plan: %w", err) - } - res, err := r.invoker.InvokeService("IaCProvider.Apply", map[string]any{"plan": planAny}) - if err != nil { - return nil, err - } - var result interfaces.ApplyResult - if err := anyToStruct(res, &result); err != nil { - return nil, fmt.Errorf("IaCProvider.Apply: decode result: %w", err) - } - return &result, nil -} - -func (r *remoteIaCProvider) Destroy(_ context.Context, refs []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { - refsAny, err := jsonToAny(refs) - if err != nil { - return nil, fmt.Errorf("IaCProvider.Destroy: marshal refs: %w", err) - } - res, err := r.invoker.InvokeService("IaCProvider.Destroy", map[string]any{ - "refs": refsAny, - }) - if err != nil { - return nil, err - } - var result interfaces.DestroyResult - if err := anyToStruct(res, &result); err != nil { - return nil, fmt.Errorf("IaCProvider.Destroy: decode result: %w", err) - } - return &result, nil -} - -func (r *remoteIaCProvider) Status(_ context.Context, refs []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { - refsAny, err := jsonToAny(refs) - if err != nil { - return nil, fmt.Errorf("IaCProvider.Status: marshal refs: %w", err) - } - res, err := r.invoker.InvokeService("IaCProvider.Status", map[string]any{ - "refs": refsAny, - }) - if err != nil { - return nil, err - } - raw, ok := res["statuses"] - if !ok { - return nil, nil - } - var statuses []interfaces.ResourceStatus - if err := anyToStruct(raw, &statuses); err != nil { - return nil, fmt.Errorf("IaCProvider.Status: decode result: %w", err) - } - return statuses, nil -} - -func (r *remoteIaCProvider) DetectDrift(_ context.Context, refs []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { - refsAny, err := jsonToAny(refs) - if err != nil { - return nil, fmt.Errorf("IaCProvider.DetectDrift: marshal refs: %w", err) - } - res, err := r.invoker.InvokeService("IaCProvider.DetectDrift", map[string]any{ - "refs": refsAny, - }) - if err != nil { - return nil, err - } - raw, ok := res["drifts"] - if !ok { - return nil, nil - } - var drifts []interfaces.DriftResult - if err := anyToStruct(raw, &drifts); err != nil { - return nil, fmt.Errorf("IaCProvider.DetectDrift: decode result: %w", err) - } - return drifts, nil -} - -// DetectDriftWithSpecs dispatches IaCProvider.DetectDrift with a "specs" arg -// map so the remote plugin can perform config-level drift detection (in -// addition to ghost/in-sync existence checks). This aligns with the DO plugin -// v0.10.5 wire protocol: the plugin dispatches on "IaCProvider.DetectDrift" -// and checks for a "specs" key in the args to activate the spec-injection -// path. No separate RPC method name is needed. -// -// The fallback to legacy DetectDrift (no specs) is preserved at the caller -// level (infra_apply_refresh.go, infra_status_drift.go) when buildAppliedSpecMap -// returns nil — this function is only invoked when specs are available. -func (r *remoteIaCProvider) DetectDriftWithSpecs(ctx context.Context, refs []interfaces.ResourceRef, specs map[string]interfaces.ResourceSpec) ([]interfaces.DriftResult, error) { - refsAny, err := jsonToAny(refs) - if err != nil { - return nil, fmt.Errorf("IaCProvider.DetectDrift(specs): marshal refs: %w", err) - } - specsAny, err := jsonToAny(specs) - if err != nil { - return nil, fmt.Errorf("IaCProvider.DetectDrift(specs): marshal specs: %w", err) - } - args := map[string]any{"refs": refsAny, "specs": specsAny} - var res map[string]any - if invoker, ok := r.invoker.(remoteServiceContextInvoker); ok { - res, err = invoker.InvokeServiceContext(ctx, "IaCProvider.DetectDrift", args) - } else { - if err := ctx.Err(); err != nil { - return nil, err - } - res, err = r.invoker.InvokeService("IaCProvider.DetectDrift", args) - } - if err != nil { - return nil, err - } - raw, ok := res["drifts"] - if !ok { - return nil, nil - } - var drifts []interfaces.DriftResult - if err := anyToStruct(raw, &drifts); err != nil { - return nil, fmt.Errorf("IaCProvider.DetectDrift(specs): decode result: %w", err) - } - return drifts, nil -} - -func (r *remoteIaCProvider) Import(_ context.Context, cloudID string, resourceType string) (*interfaces.ResourceState, error) { - res, err := r.invoker.InvokeService("IaCProvider.Import", map[string]any{ - "provider_id": cloudID, - "resource_type": resourceType, - }) - if err != nil { - return nil, err - } - var state interfaces.ResourceState - if err := anyToStruct(res, &state); err != nil { - return nil, fmt.Errorf("IaCProvider.Import: decode result: %w", err) - } - return &state, nil -} - -func (r *remoteIaCProvider) ResolveSizing(resourceType string, size interfaces.Size, hints *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { - hintsAny, err := jsonToAny(hints) - if err != nil { - return nil, fmt.Errorf("IaCProvider.ResolveSizing: marshal hints: %w", err) - } - res, err := r.invoker.InvokeService("IaCProvider.ResolveSizing", map[string]any{ - "resource_type": resourceType, - "size": string(size), - "hints": hintsAny, - }) - if err != nil { - return nil, err - } - var sizing interfaces.ProviderSizing - if err := anyToStruct(res, &sizing); err != nil { - return nil, fmt.Errorf("IaCProvider.ResolveSizing: decode result: %w", err) - } - return &sizing, nil -} - -// isPluginMethodUnimplemented returns true when err indicates the remote -// plugin's InvokeMethod / InvokeMethodContext dispatcher does not recognise -// the requested method name. This translates the gRPC codes.Unimplemented -// status (canonical signal) AND a small set of message-string fallbacks -// emitted by older plugins that did not adopt the gRPC status convention. -// -// Why string fallbacks: per workspace memory feedback_workflow_plugin_structpb_boundary, -// errors crossing the gRPC plugin boundary lose sentinel identity (structpb -// roundtrip), so message-string matching is the cross-process robust check. -// -// Matched message strings (verified against plugin/external/sdk/grpc_server.go -// at grpcServer.InvokeService): -// -// - "unimplemented" — substring match for gRPC codes.Unimplemented status -// messages whose .Error() lowercases to include the literal. -// - "not implemented" — generic catch for older plugin sdks. -// - "does not implement serviceinvoker" — emitted at line 527 when a module -// handle dispatches an untyped call but does not satisfy ServiceInvoker -// after the ServiceContextInvoker assertion also failed. -// - "does not implement servicecontextinvoker" — defensive forward-compat -// match. The current grpc_server.go does NOT emit this literal (its -// untyped path silently falls through from the optional -// ServiceContextInvoker check at line 509 to the ServiceInvoker check at -// line 524), so this arm is never hit in production today. Retained so -// a future grpc_server change that surfaces an explicit -// ServiceContextInvoker-missing error continues to translate cleanly. -// -// Not matched (intentional): -// -// - "does not implement typedserviceinvoker" (grpc_server line 498) — -// emitted only on the typed-input path. IaCProvider RPCs use the -// untyped args path, so this literal is unreachable for the proxy -// this helper serves. Keeping the matcher narrow avoids accidentally -// marking unrelated typed-call failures as Unimplemented. -// - "no service handler" — never produced by any current plugin sdk path. -// -// Strict-mode role (v0.27.1): this helper exists ONLY to translate transport -// errors into interfaces.ErrProviderMethodUnimplemented at the proxy -// boundary so callers can errors.Is on a stable sentinel. Per the v0.27.1 -// user mandate ("remove the fallback and force strict mode"), every call -// site that previously swallowed the sentinel into (nil, nil) has been -// changed to propagate loudly. Do NOT introduce new swallowing call sites. -func isPluginMethodUnimplemented(err error) bool { - if err == nil { - return false - } - if status.Code(err) == codes.Unimplemented { - return true - } - msg := strings.ToLower(err.Error()) - switch { - case strings.Contains(msg, "unimplemented"): - return true - case strings.Contains(msg, "not implemented"): - return true - case strings.Contains(msg, "does not implement serviceinvoker"): - return true - case strings.Contains(msg, "does not implement servicecontextinvoker"): - return true - } - return false -} - -// EnumerateAll implements interfaces.EnumeratorAll for the gRPC-loaded IaC -// provider. Dispatches via InvokeServiceContext when the underlying invoker -// supports it; falls back to InvokeService for legacy invokers. -// -// The plugin-side dispatcher (e.g. DO v0.14.0's DOProvider.InvokeMethod) routes -// the "IaCProvider.EnumerateAll" method string to the provider's typed -// EnumerateAll(ctx, resourceType) implementation and returns []*ResourceOutput -// under the "outputs" key. -// -// Unimplemented translation -// ───────────────────────── -// Because every gRPC-loaded provider now satisfies interfaces.EnumeratorAll -// (compile-time bridge), dispatch sites (cmd/wfctl/infra_audit_keys.go, -// infra_prune.go, infra_rotate_and_prune.go) need a runtime way to detect -// "this remote plugin does not actually support EnumerateAll". This proxy -// translates gRPC codes.Unimplemented and equivalent message-matched errors -// from the plugin's InvokeMethod dispatcher into interfaces.ErrProviderMethodUnimplemented, -// which dispatch sites errors.Is on to preserve the pre-v0.27.1 -// iterate-and-skip semantics. -// -// This bridge closes the v0.27.0 gap surfaced by `wfctl infra audit-keys`: the -// audit-keys command type-asserted the loaded provider against EnumeratorAll, -// but remoteIaCProvider did not implement EnumerateAll, so the assertion -// always failed even when the plugin process itself implemented it. -func (r *remoteIaCProvider) EnumerateAll(ctx context.Context, resourceType string) ([]*interfaces.ResourceOutput, error) { - args := map[string]any{ - "resource_type": resourceType, - } - var ( - res map[string]any - err error - ) - if invoker, ok := r.invoker.(remoteServiceContextInvoker); ok { - res, err = invoker.InvokeServiceContext(ctx, "IaCProvider.EnumerateAll", args) - } else { - if ctxErr := ctx.Err(); ctxErr != nil { - return nil, ctxErr - } - res, err = r.invoker.InvokeService("IaCProvider.EnumerateAll", args) - } - if err != nil { - if isPluginMethodUnimplemented(err) { - return nil, fmt.Errorf("IaCProvider.EnumerateAll: %w: %v", - interfaces.ErrProviderMethodUnimplemented, err) - } - return nil, fmt.Errorf("IaCProvider.EnumerateAll: %w", err) - } - if res == nil { - return nil, nil - } - raw, ok := res["outputs"] - if !ok { - return nil, nil - } - // Round-trip through any to avoid carrying structpb-only types into the - // typed slice. anyToStruct wraps json.Marshal → json.Unmarshal. - var outs []*interfaces.ResourceOutput - if err := anyToStruct(raw, &outs); err != nil { - return nil, fmt.Errorf("IaCProvider.EnumerateAll: decode result: %w", err) - } - return outs, nil -} - -// EnumerateByTag implements interfaces.Enumerator for the gRPC-loaded IaC -// provider. Dispatches via InvokeServiceContext when available; falls back -// to InvokeService for legacy invokers. -// -// The plugin-side dispatcher routes "IaCProvider.EnumerateByTag" to the -// provider's typed EnumerateByTag(ctx, tag) implementation and returns -// []ResourceRef under the "refs" key. Returns Name + Type + ProviderID per -// the contract documented in interfaces/iac_provider.go. -// -// Bridged in v0.27.1 alongside EnumerateAll; both optional interfaces had -// the same root cause (no compile-time enforcement of proxy coverage for -// optional IaCProvider sub-interfaces). Same Unimplemented-translation -// applies so infra_cleanup.go's "skipped : does not implement -// Enumerator" log line is preserved for plugins that don't support tag -// queries (e.g. AppPlatform-only plugins). -func (r *remoteIaCProvider) EnumerateByTag(ctx context.Context, tag string) ([]interfaces.ResourceRef, error) { - args := map[string]any{ - "tag": tag, - } - var ( - res map[string]any - err error - ) - if invoker, ok := r.invoker.(remoteServiceContextInvoker); ok { - res, err = invoker.InvokeServiceContext(ctx, "IaCProvider.EnumerateByTag", args) - } else { - if ctxErr := ctx.Err(); ctxErr != nil { - return nil, ctxErr - } - res, err = r.invoker.InvokeService("IaCProvider.EnumerateByTag", args) - } - if err != nil { - if isPluginMethodUnimplemented(err) { - return nil, fmt.Errorf("IaCProvider.EnumerateByTag: %w: %v", - interfaces.ErrProviderMethodUnimplemented, err) + out := make(map[string]bool, len(reg.GetContracts())) + for _, c := range reg.GetContracts() { + if c.GetKind() == pb.ContractKind_CONTRACT_KIND_SERVICE { + out[c.GetServiceName()] = true } - return nil, fmt.Errorf("IaCProvider.EnumerateByTag: %w", err) - } - if res == nil { - return nil, nil - } - raw, ok := res["refs"] - if !ok { - return nil, nil - } - var refs []interfaces.ResourceRef - if err := anyToStruct(raw, &refs); err != nil { - return nil, fmt.Errorf("IaCProvider.EnumerateByTag: decode result: %w", err) - } - return refs, nil -} - -func (r *remoteIaCProvider) RepairDirtyMigration(ctx context.Context, req interfaces.MigrationRepairRequest) (*interfaces.MigrationRepairResult, error) { - reqAny, err := jsonToAny(req) - if err != nil { - return nil, fmt.Errorf("IaCProvider.RepairDirtyMigration: marshal request: %w", err) - } - args := map[string]any{ - "request": reqAny, - } - var res map[string]any - if invoker, ok := r.invoker.(remoteServiceContextInvoker); ok { - res, err = invoker.InvokeServiceContext(ctx, "IaCProvider.RepairDirtyMigration", args) - } else { - if err := ctx.Err(); err != nil { - return nil, err - } - res, err = r.invoker.InvokeService("IaCProvider.RepairDirtyMigration", args) - } - if err != nil { - return nil, err - } - var result interfaces.MigrationRepairResult - if err := anyToStruct(res, &result); err != nil { - return nil, fmt.Errorf("IaCProvider.RepairDirtyMigration: decode result: %w", err) } - return &result, nil -} - -func (r *remoteIaCProvider) ResourceDriver(resourceType string) (interfaces.ResourceDriver, error) { - return &remoteResourceDriver{invoker: r.invoker, resourceType: resourceType}, nil + return out } -// SupportedCanonicalKeys returns the full canonical key set for remote providers. -// External plugin providers may override this via the plugin manifest in a future phase. -func (r *remoteIaCProvider) BootstrapStateBackend(ctx context.Context, cfg map[string]any) (*interfaces.BootstrapResult, error) { - res, err := r.invoker.InvokeService("IaCProvider.BootstrapStateBackend", cfg) - if err != nil { - return nil, err - } - if res == nil { - return nil, nil - } - var result interfaces.BootstrapResult - if err := anyToStruct(res, &result); err != nil { - return nil, fmt.Errorf("BootstrapStateBackend: decode result: %w", err) - } - return &result, nil -} +// closerFunc adapts a func() error to io.Closer. +type closerFunc func() error -func (r *remoteIaCProvider) SupportedCanonicalKeys() []string { - return interfaces.CanonicalKeys() -} +func (f closerFunc) Close() error { return f() } -// ValidatePlan implements interfaces.ProviderValidator for the gRPC-loaded -// IaC provider. Dispatches "IaCProvider.ValidatePlan" via InvokeService and -// decodes the result["diagnostics"] entry as []PlanDiagnostic. -// -// Contract preservation -// ───────────────────── -// interfaces.ProviderValidator.ValidatePlan returns ONLY a []PlanDiagnostic -// (no error). To preserve the pre-v0.27.1 R-A10 semantics where plugins that -// don't implement ValidatePlan are silently skipped (the type-assert at -// cmd/wfctl/infra_align_rules.go:777 fails and the loop continues), this -// proxy returns nil (which []PlanDiagnostic permits) on transport errors, -// interfaces.ErrProviderMethodUnimplemented, and decode failures. Plugins -// that genuinely error during ValidatePlan (e.g. malformed plan input) will -// appear silent under R-A10 in this PR; emitting a warning at that boundary -// is left to a follow-up PR that extends the ProviderValidator contract to -// return an error. +// ─── Error classification + retry helpers ─────────────────────────────────── // -// Bridged in v0.27.1 to close the strict-bridge-coverage gate; without -// this method *remoteIaCProvider would not satisfy interfaces.ProviderValidator -// and R-A10 would silently skip every gRPC-loaded provider. -func (r *remoteIaCProvider) ValidatePlan(plan *interfaces.IaCPlan) []interfaces.PlanDiagnostic { - planAny, err := jsonToAny(plan) - if err != nil { - return nil - } - args := map[string]any{"plan": planAny} - res, err := r.invoker.InvokeService("IaCProvider.ValidatePlan", args) - if err != nil { - // All errors (Unimplemented + transport + plugin) are silenced to - // preserve the pre-v0.27.1 type-assert-skip behavior. The contract - // has no error channel; surfacing failures requires an upstream - // interface change. - return nil - } - if res == nil { - return nil - } - raw, ok := res["diagnostics"] - if !ok { - return nil - } - var diags []interfaces.PlanDiagnostic - if err := anyToStruct(raw, &diags); err != nil { - return nil - } - return diags -} - -// RevokeProviderCredential implements interfaces.ProviderCredentialRevoker. -// It dispatches through InvokeService to the plugin subprocess, which calls -// the provider's upstream DELETE endpoint (e.g. DO SpacesKeys.Delete). -// This method is called by `wfctl infra bootstrap --force-rotate ` after -// the new credential has been minted and stored (mint-new-then-revoke-old, -// see ADR 0012 in the workflow repo decisions/). -func (r *remoteIaCProvider) RevokeProviderCredential(ctx context.Context, source string, credentialID string) error { - args := map[string]any{ - "source": source, - "credential_id": credentialID, - } - var ( - res map[string]any - err error - ) - if invoker, ok := r.invoker.(remoteServiceContextInvoker); ok { - res, err = invoker.InvokeServiceContext(ctx, "IaCProvider.RevokeProviderCredential", args) - } else { - if ctxErr := ctx.Err(); ctxErr != nil { - return ctxErr - } - res, err = r.invoker.InvokeService("IaCProvider.RevokeProviderCredential", args) - } - if err != nil { - return fmt.Errorf("IaCProvider.RevokeProviderCredential: %w", err) - } - _ = res - return nil -} - -func (r *remoteIaCProvider) Close() error { return nil } - -// remoteResourceDriver routes ResourceDriver calls to the plugin via InvokeService. -type remoteResourceDriver struct { - invoker remoteServiceInvoker - resourceType string -} - -// sensitiveToAny converts a map[string]bool (the Sensitive field on -// ResourceOutput) into the map[string]any shape structpb.NewStruct -// accepts. Returns nil for empty/nil input so the wire stays -// trim-friendly. Without this conversion, the upstream -// plugin/external/convert.go::mapToStruct silently drops the entire -// args struct on NewStruct failure (it returns &structpb.Struct{} -// rather than surfacing the typing error) — the bug T3.9 -// runtime-launch-validation surfaced. -func sensitiveToAny(s map[string]bool) map[string]any { - if len(s) == 0 { - return nil - } - out := make(map[string]any, len(s)) - for k, v := range s { - out[k] = v - } - return out -} +// Used by pluginDeployProvider to wrap typed-RPC errors with stable IaC +// sentinels so callers can errors.Is + classify. Independent of the +// transport layer — kept after the strict-contracts cutover (Task 16) +// because typed gRPC errors arrive as plain text on the wire too; +// wrapIaCError sniffs the message for HTTP-status / common-phrase +// patterns, which is provider-agnostic. // wrapIaCError categorizes plugin errors by matching HTTP status codes and // common message patterns, wrapping with the appropriate IaC sentinel so @@ -976,7 +312,7 @@ func retryOnTransient(ctx context.Context, op func() error) error { func deployOpError(resourceName, op string, err error) error { switch { case errors.Is(err, interfaces.ErrUnauthorized) || errors.Is(err, interfaces.ErrForbidden): - return fmt.Errorf("plugin deploy %q: %s: auth failed — check DIGITALOCEAN_TOKEN permissions: %w", resourceName, op, err) + return fmt.Errorf("plugin deploy %q: %s: auth failed — check provider credentials: %w", resourceName, op, err) case errors.Is(err, interfaces.ErrValidation): return fmt.Errorf("plugin deploy %q: %s: validation error: %w", resourceName, op, err) default: @@ -984,250 +320,6 @@ func deployOpError(resourceName, op string, err error) error { } } -// decodeResourceOutput converts an InvokeService response map into a *interfaces.ResourceOutput, -// including the Outputs map and Sensitive flags that the previous Update implementation discarded. -func decodeResourceOutput(m map[string]any) *interfaces.ResourceOutput { - out := &interfaces.ResourceOutput{ - ProviderID: stringFromMap(m, "provider_id"), - Name: stringFromMap(m, "name"), - Type: stringFromMap(m, "type"), - Status: stringFromMap(m, "status"), - } - if raw, ok := m["outputs"]; ok { - if outputs, ok := raw.(map[string]any); ok { - out.Outputs = outputs - } - } - if raw, ok := m["sensitive"]; ok { - switch v := raw.(type) { - case map[string]bool: - out.Sensitive = v - case map[string]any: - sens := make(map[string]bool, len(v)) - for k, val := range v { - if b, ok := val.(bool); ok { - sens[k] = b - } - } - out.Sensitive = sens - } - } - return out -} - -func (d *remoteResourceDriver) Create(_ context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { - res, err := d.invoker.InvokeService("ResourceDriver.Create", map[string]any{ - "resource_type": d.resourceType, - "spec_name": spec.Name, - "spec_type": spec.Type, - "spec_config": spec.Config, - }) - if err != nil { - return nil, wrapIaCError(err) - } - return decodeResourceOutput(res), nil -} - -func (d *remoteResourceDriver) Read(_ context.Context, ref interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { - res, err := d.invoker.InvokeService("ResourceDriver.Read", map[string]any{ - "resource_type": d.resourceType, - "ref_name": ref.Name, - "ref_type": ref.Type, - "ref_provider_id": ref.ProviderID, - }) - if err != nil { - return nil, wrapIaCError(err) - } - return decodeResourceOutput(res), nil -} - -func (d *remoteResourceDriver) Update(_ context.Context, ref interfaces.ResourceRef, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { - res, err := d.invoker.InvokeService("ResourceDriver.Update", map[string]any{ - "resource_type": d.resourceType, - "ref_name": ref.Name, - "ref_type": ref.Type, - "ref_provider_id": ref.ProviderID, - "spec_name": spec.Name, - "spec_type": spec.Type, - "spec_config": spec.Config, - }) - if err != nil { - return nil, wrapIaCError(err) - } - return decodeResourceOutput(res), nil -} - -func (d *remoteResourceDriver) Delete(_ context.Context, ref interfaces.ResourceRef) error { - _, err := d.invoker.InvokeService("ResourceDriver.Delete", map[string]any{ - "resource_type": d.resourceType, - "ref_name": ref.Name, - "ref_type": ref.Type, - "ref_provider_id": ref.ProviderID, - }) - return wrapIaCError(err) -} - -func (d *remoteResourceDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, current *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { - args := map[string]any{ - "resource_type": d.resourceType, - "spec_name": desired.Name, - "spec_type": desired.Type, - "spec_config": desired.Config, - "current_name": current.Name, - "current_type": current.Type, - "current_provider_id": current.ProviderID, - "current_status": current.Status, - "current_outputs": current.Outputs, - } - // Sensitive crosses the gRPC boundary as map[string]any. - // structpb.NewStruct rejects map[string]bool; without this - // conversion the entire args struct silently drops to empty - // (mapToStruct in plugin/external/convert.go falls back to - // &structpb.Struct{} on err) and the plugin observes args=map[] - // — the bug T3.9 runtime-launch-validation surfaced. - // - // Only include the key when the converted map is non-empty, so the - // wire stays trim-friendly (matches sensitiveToAny's docstring). - // Setting `args["current_sensitive"] = nil` would serialize as a - // NullValue rather than omitting the field, defeating that intent. - if conv := sensitiveToAny(current.Sensitive); conv != nil { - args["current_sensitive"] = conv - } - res, err := d.invoker.InvokeService("ResourceDriver.Diff", args) - if err != nil { - return nil, wrapIaCError(err) - } - result := &interfaces.DiffResult{} - result.NeedsUpdate, _ = res["needs_update"].(bool) - result.NeedsReplace, _ = res["needs_replace"].(bool) - if rawChanges, ok := res["changes"]; ok { - if changes, ok := rawChanges.([]any); ok { - for _, c := range changes { - if cm, ok := c.(map[string]any); ok { - fc := interfaces.FieldChange{ - Path: stringFromMap(cm, "path"), - Old: cm["old"], - New: cm["new"], - } - fc.ForceNew, _ = cm["force_new"].(bool) - result.Changes = append(result.Changes, fc) - } - } - } - } - return result, nil -} - -func (d *remoteResourceDriver) HealthCheck(_ context.Context, ref interfaces.ResourceRef) (*interfaces.HealthResult, error) { - res, err := d.invoker.InvokeService("ResourceDriver.HealthCheck", map[string]any{ - "resource_type": d.resourceType, - "ref_name": ref.Name, - "ref_type": ref.Type, - "ref_provider_id": ref.ProviderID, - }) - if err != nil { - return nil, wrapIaCError(err) - } - healthy, _ := res["healthy"].(bool) - message, _ := res["message"].(string) - return &interfaces.HealthResult{Healthy: healthy, Message: message}, nil -} - -func (d *remoteResourceDriver) Scale(_ context.Context, ref interfaces.ResourceRef, replicas int) (*interfaces.ResourceOutput, error) { - res, err := d.invoker.InvokeService("ResourceDriver.Scale", map[string]any{ - "resource_type": d.resourceType, - "ref_name": ref.Name, - "ref_type": ref.Type, - "ref_provider_id": ref.ProviderID, - "replicas": replicas, - }) - if err != nil { - return nil, wrapIaCError(err) - } - return decodeResourceOutput(res), nil -} - -func (d *remoteResourceDriver) SensitiveKeys() []string { - res, err := d.invoker.InvokeService("ResourceDriver.SensitiveKeys", map[string]any{ - "resource_type": d.resourceType, - }) - if err != nil { - return nil - } - raw, ok := res["keys"] - if !ok { - return nil - } - items, ok := raw.([]any) - if !ok { - return nil - } - keys := make([]string, 0, len(items)) - for _, item := range items { - if s, ok := item.(string); ok { - keys = append(keys, s) - } - } - return keys -} - -// Troubleshoot calls the plugin's optional Troubleshooter.Troubleshoot. -// Returns (nil, nil) silently when the plugin returns Unimplemented so -// the caller doesn't need to probe for capability — absence is a valid answer. -func (d *remoteResourceDriver) Troubleshoot(ctx context.Context, ref interfaces.ResourceRef, failureMsg string) ([]interfaces.Diagnostic, error) { - // Pass ref as flat primitives — structpb.NewStruct (the gRPC transport) - // cannot encode arbitrary Go structs; each field must be a scalar. - res, err := d.invoker.InvokeService("ResourceDriver.Troubleshoot", map[string]any{ - "resource_type": d.resourceType, - "ref_name": ref.Name, - "ref_provider_id": ref.ProviderID, - "ref_type": ref.Type, - "failure_msg": failureMsg, - }) - if err != nil { - if st, ok := status.FromError(err); ok && st.Code() == codes.Unimplemented { - return nil, nil - } - return nil, fmt.Errorf("resource driver Troubleshoot: %w", err) - } - raw, _ := res["diagnostics"].([]any) - out := make([]interfaces.Diagnostic, 0, len(raw)) - for _, r := range raw { - m, _ := r.(map[string]any) - diag := interfaces.Diagnostic{ - ID: stringVal(m, "id"), - Phase: stringVal(m, "phase"), - Cause: stringVal(m, "cause"), - Detail: stringVal(m, "detail"), - } - if s := stringVal(m, "at"); s != "" { - if t, perr := time.Parse(time.RFC3339, s); perr == nil { - diag.At = t - } - } - out = append(out, diag) - } - return out, nil -} - -// stringVal returns a string field from a map or "" if missing/wrong type. -func stringVal(m map[string]any, k string) string { - if v, ok := m[k].(string); ok { - return v - } - return "" -} - -func stringFromMap(m map[string]any, key string) string { - v, _ := m[key].(string) - return v -} - -// closerFunc adapts a func() error to io.Closer. -type closerFunc func() error - -func (f closerFunc) Close() error { return f() } - // newPluginDeployProvider looks up a matching iac.provider + infra.container_service // module pair in wfCfg and wraps them as a DeployProvider. envName selects the // per-environment config overlay via ModuleConfig.ResolveForEnv; pass "" to use diff --git a/cmd/wfctl/deploy_providers_dispatch_matrix_test.go b/cmd/wfctl/deploy_providers_dispatch_matrix_test.go deleted file mode 100644 index 771941b4..00000000 --- a/cmd/wfctl/deploy_providers_dispatch_matrix_test.go +++ /dev/null @@ -1,477 +0,0 @@ -package main - -// deploy_providers_dispatch_matrix_test.go — comprehensive IaCProvider + ResourceDriver -// gRPC dispatch matrix. -// -// Design: this file tests the wfctl DISPATCH LAYER only — specifically whether -// remoteResourceDriver and remoteIaCProvider forward the correct RPC method name -// and all required arg keys to InvokeService. The invoker is mocked; no real plugin -// binary runs. This means the matrix is plugin-agnostic: it does not care whether the -// backing provider is DigitalOcean, AWS, Terraform, OpenTofu, or any other plugin. -// -// Error-classification tests (TestDispatchMatrix_ErrorClassification) verify wfctl's -// wrapIaCError normalization — the contract between wfctl and ANY plugin implementation. -// A plugin that emits a non-matching error string will not be normalized, which is a -// bug in that plugin, not in this matrix. -// -// For every method on remoteResourceDriver and remoteIaCProvider this file verifies: -// 1. The exact RPC method name passed to InvokeService. -// 2. All required arg keys are present in the args map. -// 3. Zero-value inputs (empty strings, nil slices) still produce the key — they must -// not be silently dropped. -// 4. Error class matches when the invoker returns a sentinel-triggering message. -// -// Invariant proof: temporarily comment out one key in the real dispatcher and this -// test will fail for the corresponding row. Restore to make it pass again. -// -// Run: go test ./cmd/wfctl/ -run TestDispatchMatrix -v - -import ( - "context" - "errors" - "testing" - - "github.com/GoCodeAlone/workflow/interfaces" -) - -// ── recordingInvoker ───────────────────────────────────────────────────────── - -// recordingInvoker captures every InvokeService call (method + args) and -// returns a configurable response. Unlike stubInvoker it stores the last call -// only; that is sufficient because all matrix cases call InvokeService once. -type recordingInvoker struct { - capturedMethod string - capturedArgs map[string]any - resp map[string]any - err error -} - -func (r *recordingInvoker) InvokeService(method string, args map[string]any) (map[string]any, error) { - r.capturedMethod = method - r.capturedArgs = args - return r.resp, r.err -} - -func newRecorder(resp map[string]any, err error) *recordingInvoker { - return &recordingInvoker{resp: resp, err: err} -} - -// ── assertion helpers ───────────────────────────────────────────────────────── - -// assertMethod fails unless the captured method equals want. -func assertMethod(t *testing.T, ri *recordingInvoker, want string) { - t.Helper() - if ri.capturedMethod != want { - t.Errorf("RPC method = %q, want %q", ri.capturedMethod, want) - } -} - -// assertKeys fails if any required key is absent from the captured args map, -// including when the value is the zero value (empty string, nil, false, 0). -// The check is presence-only — it does NOT assert the value. -func assertKeys(t *testing.T, ri *recordingInvoker, requiredKeys ...string) { - t.Helper() - for _, k := range requiredKeys { - if _, ok := ri.capturedArgs[k]; !ok { - t.Errorf("required arg key %q missing from InvokeService call; got keys: %v", - k, mapKeys(ri.capturedArgs)) - } - } -} - -func mapKeys(m map[string]any) []string { - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, k) - } - return keys -} - -// ── ResourceDriver dispatch matrix ─────────────────────────────────────────── - -// TestDispatchMatrix_RemoteResourceDriver exercises every method on -// remoteResourceDriver, verifying RPC method name and required arg keys. -func TestDispatchMatrix_RemoteResourceDriver(t *testing.T) { - ctx := context.Background() - const rt = "container_service" - - // emptyRef / emptySpec use zero values to prove keys aren't silently dropped. - emptyRef := interfaces.ResourceRef{} - emptySpec := interfaces.ResourceSpec{} - zeroOutput := &interfaces.ResourceOutput{} - - cases := []struct { - name string - wantMethod string - requiredKeys []string - invoke func(d *remoteResourceDriver, ri *recordingInvoker) error - }{ - { - name: "Create", - wantMethod: "ResourceDriver.Create", - requiredKeys: []string{"resource_type", "spec_name", "spec_type", "spec_config"}, - invoke: func(d *remoteResourceDriver, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := d.Create(ctx, emptySpec) - return err - }, - }, - { - name: "Read", - wantMethod: "ResourceDriver.Read", - requiredKeys: []string{"resource_type", "ref_name", "ref_type", "ref_provider_id"}, - invoke: func(d *remoteResourceDriver, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := d.Read(ctx, emptyRef) - return err - }, - }, - { - name: "Update", - wantMethod: "ResourceDriver.Update", - requiredKeys: []string{ - "resource_type", - "ref_name", "ref_type", "ref_provider_id", - "spec_name", "spec_type", "spec_config", - }, - invoke: func(d *remoteResourceDriver, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := d.Update(ctx, emptyRef, emptySpec) - return err - }, - }, - { - name: "Delete", - wantMethod: "ResourceDriver.Delete", - requiredKeys: []string{"resource_type", "ref_name", "ref_type", "ref_provider_id"}, - invoke: func(d *remoteResourceDriver, ri *recordingInvoker) error { - return d.Delete(ctx, emptyRef) - }, - }, - { - name: "Diff", - wantMethod: "ResourceDriver.Diff", - requiredKeys: []string{ - "resource_type", - "spec_name", "spec_type", "spec_config", - "current_name", "current_type", "current_provider_id", "current_status", - }, - invoke: func(d *remoteResourceDriver, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := d.Diff(ctx, emptySpec, zeroOutput) - return err - }, - }, - { - name: "HealthCheck", - wantMethod: "ResourceDriver.HealthCheck", - requiredKeys: []string{"resource_type", "ref_name", "ref_type", "ref_provider_id"}, - invoke: func(d *remoteResourceDriver, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := d.HealthCheck(ctx, emptyRef) - return err - }, - }, - { - name: "Scale", - wantMethod: "ResourceDriver.Scale", - requiredKeys: []string{"resource_type", "ref_name", "ref_type", "ref_provider_id", "replicas"}, - invoke: func(d *remoteResourceDriver, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := d.Scale(ctx, emptyRef, 0) - return err - }, - }, - { - name: "SensitiveKeys", - wantMethod: "ResourceDriver.SensitiveKeys", - requiredKeys: []string{"resource_type"}, - invoke: func(d *remoteResourceDriver, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _ = d.SensitiveKeys() - return nil - }, - }, - { - // Task #80 regression: resource_type was missing from Troubleshoot args. - // This entry will FAIL until that bug is fixed (resource_type is now required). - name: "Troubleshoot", - wantMethod: "ResourceDriver.Troubleshoot", - requiredKeys: []string{"resource_type", "ref_name", "ref_type", "ref_provider_id", "failure_msg"}, - invoke: func(d *remoteResourceDriver, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := d.Troubleshoot(ctx, emptyRef, "") - return err - }, - }, - } - - for _, tc := range cases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - ri := newRecorder(nil, nil) - d := &remoteResourceDriver{invoker: ri, resourceType: rt} - _ = tc.invoke(d, ri) - assertMethod(t, ri, tc.wantMethod) - assertKeys(t, ri, tc.requiredKeys...) - }) - } -} - -// TestDispatchMatrix_RemoteResourceDriver_ZeroValues proves that zero-value -// inputs still emit the arg keys (no silent omission). -func TestDispatchMatrix_RemoteResourceDriver_ZeroValues(t *testing.T) { - ctx := context.Background() - - // resource_type is empty string — key must still be present. - ri := newRecorder(map[string]any{}, nil) - d := &remoteResourceDriver{invoker: ri, resourceType: ""} - _, _ = d.Read(ctx, interfaces.ResourceRef{}) - if _, ok := ri.capturedArgs["resource_type"]; !ok { - t.Error("resource_type key missing when resourceType is empty string") - } - if ri.capturedArgs["resource_type"] != "" { - t.Errorf("resource_type = %q, want empty string", ri.capturedArgs["resource_type"]) - } -} - -// ── IaCProvider dispatch matrix ─────────────────────────────────────────────── - -// TestDispatchMatrix_RemoteIaCProvider exercises every method on remoteIaCProvider, -// verifying RPC method name and required arg keys. -func TestDispatchMatrix_RemoteIaCProvider(t *testing.T) { - ctx := context.Background() - - cases := []struct { - name string - wantMethod string - requiredKeys []string - invoke func(p *remoteIaCProvider, ri *recordingInvoker) error - }{ - // Plan and Apply exercise the v1-default branch: per W-Refactor - // (PR 5), remoteIaCProvider's Plan / Apply are manifest-conditional - // — they proxy IaCProvider.Plan / IaCProvider.Apply via - // InvokeService when computePlanVersion is "" / "v1" / unknown, - // and delegate to platform.ComputePlan / wfctlhelpers.ApplyPlan - // when computePlanVersion == "v2". The matrix below pins the v1 - // wire shape (the default constructed *remoteIaCProvider has - // computePlanVersion: "" so the legacy proxy fires). v2 branch - // behavior is pinned in deploy_providers_remote_iac_test.go. - { - name: "Plan", - wantMethod: "IaCProvider.Plan", - requiredKeys: []string{"desired", "current"}, - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := p.Plan(ctx, nil, nil) - return err - }, - }, - { - name: "Apply", - wantMethod: "IaCProvider.Apply", - requiredKeys: []string{"plan"}, - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := p.Apply(ctx, &interfaces.IaCPlan{}) - return err - }, - }, - { - name: "Destroy", - wantMethod: "IaCProvider.Destroy", - requiredKeys: []string{"refs"}, - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := p.Destroy(ctx, nil) - return err - }, - }, - { - name: "Status", - wantMethod: "IaCProvider.Status", - requiredKeys: []string{"refs"}, - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := p.Status(ctx, nil) - return err - }, - }, - { - name: "DetectDrift", - wantMethod: "IaCProvider.DetectDrift", - requiredKeys: []string{"refs"}, - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := p.DetectDrift(ctx, nil) - return err - }, - }, - { - name: "Import", - wantMethod: "IaCProvider.Import", - requiredKeys: []string{"provider_id", "resource_type"}, - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := p.Import(ctx, "", "") - return err - }, - }, - { - name: "ResolveSizing", - wantMethod: "IaCProvider.ResolveSizing", - requiredKeys: []string{"resource_type", "size", "hints"}, - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := p.ResolveSizing("", "", nil) - return err - }, - }, - { - name: "RepairDirtyMigration", - wantMethod: "IaCProvider.RepairDirtyMigration", - requiredKeys: []string{"request"}, - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := p.RepairDirtyMigration(ctx, interfaces.MigrationRepairRequest{}) - return err - }, - }, - { - // v0.27.1 — bridges interfaces.EnumeratorAll. The dispatcher in - // runInfraAuditKeysCmd type-asserts the loaded provider against - // EnumeratorAll, so this row pins the wire shape. - name: "EnumerateAll", - wantMethod: "IaCProvider.EnumerateAll", - requiredKeys: []string{"resource_type"}, - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := p.EnumerateAll(ctx, "infra.spaces_key") - return err - }, - }, - { - // v0.27.1 — bridges interfaces.Enumerator. wfctl infra cleanup - // type-asserts against this and skips providers that don't - // implement; the proxy now satisfies the assertion for all - // gRPC-loaded plugins, with the actual provider-side support - // surfaced as a normal RPC error. - name: "EnumerateByTag", - wantMethod: "IaCProvider.EnumerateByTag", - requiredKeys: []string{"tag"}, - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - _, err := p.EnumerateByTag(ctx, "wfctl-managed") - return err - }, - }, - { - // BootstrapStateBackend sends cfg directly as the args map (no wrapper key). - // When cfg is nil/empty InvokeService must still be called (args may be nil). - name: "BootstrapStateBackend_nilCfg", - wantMethod: "IaCProvider.BootstrapStateBackend", - requiredKeys: []string{}, // flat cfg — no fixed wrapper key to assert - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = nil - _, err := p.BootstrapStateBackend(ctx, nil) - return err - }, - }, - { - // BootstrapStateBackend with populated cfg: keys from cfg pass through flat. - name: "BootstrapStateBackend_withCfg", - wantMethod: "IaCProvider.BootstrapStateBackend", - requiredKeys: []string{"bucket", "region"}, // caller-supplied cfg keys - invoke: func(p *remoteIaCProvider, ri *recordingInvoker) error { - ri.resp = map[string]any{} - cfg := map[string]any{"bucket": "my-bucket", "region": "nyc3"} - _, err := p.BootstrapStateBackend(ctx, cfg) - return err - }, - }, - } - - for _, tc := range cases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - ri := newRecorder(nil, nil) - p := &remoteIaCProvider{invoker: ri} - _ = tc.invoke(p, ri) - assertMethod(t, ri, tc.wantMethod) - assertKeys(t, ri, tc.requiredKeys...) - }) - } -} - -// TestDispatchMatrix_RemoteIaCProvider_ZeroValueKeys proves that zero-value -// string args still appear as keys (not omitted when value is ""). -func TestDispatchMatrix_RemoteIaCProvider_ZeroValueKeys(t *testing.T) { - ri := newRecorder(map[string]any{}, nil) - p := &remoteIaCProvider{invoker: ri} - _, _ = p.Import(context.Background(), "", "") - - for _, k := range []string{"provider_id", "resource_type"} { - if _, ok := ri.capturedArgs[k]; !ok { - t.Errorf("Import: key %q missing when value is empty string", k) - } - } -} - -// ── Error classification matrix ─────────────────────────────────────────────── - -// TestDispatchMatrix_ErrorClassification verifies that wrapIaCError wraps -// plugin error strings into the correct sentinel errors. -func TestDispatchMatrix_ErrorClassification(t *testing.T) { - cases := []struct { - msg string - wantErr error - }{ - {"resource not found", interfaces.ErrResourceNotFound}, - {"404 not found", interfaces.ErrResourceNotFound}, - {"does not exist", interfaces.ErrResourceNotFound}, - {"already exists", interfaces.ErrResourceAlreadyExists}, - {"409 conflict", interfaces.ErrResourceAlreadyExists}, - {"rate limit exceeded", interfaces.ErrRateLimited}, - {"429 too many requests", interfaces.ErrRateLimited}, - {"500 internal server error", interfaces.ErrTransient}, - {"503 service unavailable", interfaces.ErrTransient}, - {"401 unauthorized", interfaces.ErrUnauthorized}, - {"unable to authenticate", interfaces.ErrUnauthorized}, - {"403 forbidden", interfaces.ErrForbidden}, - {"400 validation failed", interfaces.ErrValidation}, - {"422 invalid input", interfaces.ErrValidation}, - {"some unknown error", nil}, // no sentinel — returned unchanged - } - - for _, tc := range cases { - tc := tc - t.Run(tc.msg, func(t *testing.T) { - wrapped := wrapIaCError(errors.New(tc.msg)) - if tc.wantErr == nil { - if wrapped == nil || wrapped.Error() != tc.msg { - t.Errorf("expected unchanged error %q, got %q", tc.msg, wrapped) - } - return - } - if !errors.Is(wrapped, tc.wantErr) { - t.Errorf("errors.Is(%q) = false for sentinel %v", tc.msg, tc.wantErr) - } - }) - } -} - -// TestDispatchMatrix_RemoteResourceDriver_ErrorPropagation verifies that -// InvokeService errors are wrapped via wrapIaCError before returning to caller. -func TestDispatchMatrix_RemoteResourceDriver_ErrorPropagation(t *testing.T) { - ctx := context.Background() - ri := newRecorder(nil, errors.New("resource not found")) - d := &remoteResourceDriver{invoker: ri, resourceType: "container_service"} - - _, err := d.Read(ctx, interfaces.ResourceRef{}) - if err == nil { - t.Fatal("expected error, got nil") - } - if !errors.Is(err, interfaces.ErrResourceNotFound) { - t.Errorf("err = %v; want errors.Is(ErrResourceNotFound) = true", err) - } -} diff --git a/cmd/wfctl/deploy_providers_remote_driver_test.go b/cmd/wfctl/deploy_providers_remote_driver_test.go deleted file mode 100644 index 04064291..00000000 --- a/cmd/wfctl/deploy_providers_remote_driver_test.go +++ /dev/null @@ -1,820 +0,0 @@ -package main - -import ( - "context" - "errors" - "fmt" - "strings" - "testing" - - "github.com/GoCodeAlone/workflow/interfaces" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" -) - -// stubInvoker is a test double for remoteServiceInvoker that records calls -// and returns a preset response map. -type stubInvoker struct { - method string - args map[string]any - resp map[string]any - err error -} - -func (s *stubInvoker) InvokeService(method string, args map[string]any) (map[string]any, error) { - s.method = method - s.args = args - return s.resp, s.err -} - -// sampleOutputMap returns a populated ResourceOutput-shaped map for testing. -func sampleOutputMap() map[string]any { - return map[string]any{ - "provider_id": "pid-123", - "name": "my-resource", - "type": "container_service", - "status": "running", - "outputs": map[string]any{"endpoint": "https://example.com"}, - "sensitive": map[string]any{"endpoint": true}, - } -} - -func sampleRef() interfaces.ResourceRef { - return interfaces.ResourceRef{ - Name: "my-resource", - Type: "container_service", - ProviderID: "pid-123", - } -} - -func sampleSpec() interfaces.ResourceSpec { - return interfaces.ResourceSpec{ - Name: "my-resource", - Type: "container_service", - Config: map[string]any{"image": "myapp:v1"}, - } -} - -func newDriver(si *stubInvoker) *remoteResourceDriver { - return &remoteResourceDriver{invoker: si, resourceType: "container_service"} -} - -// ── decodeResourceOutput ────────────────────────────────────────────────────── - -func TestRemoteDriver_OutputsDecoded(t *testing.T) { - m := sampleOutputMap() - out := decodeResourceOutput(m) - if out.ProviderID != "pid-123" { - t.Errorf("ProviderID: got %q", out.ProviderID) - } - if out.Name != "my-resource" { - t.Errorf("Name: got %q", out.Name) - } - if out.Type != "container_service" { - t.Errorf("Type: got %q", out.Type) - } - if out.Status != "running" { - t.Errorf("Status: got %q", out.Status) - } - if out.Outputs["endpoint"] != "https://example.com" { - t.Errorf("Outputs[endpoint]: got %v", out.Outputs["endpoint"]) - } - if !out.Sensitive["endpoint"] { - t.Error("Sensitive[endpoint]: expected true") - } -} - -// ── Create ──────────────────────────────────────────────────────────────────── - -func TestRemoteDriver_Create(t *testing.T) { - si := &stubInvoker{resp: sampleOutputMap()} - d := newDriver(si) - spec := sampleSpec() - - out, err := d.Create(context.Background(), spec) - if err != nil { - t.Fatalf("Create: unexpected error: %v", err) - } - if si.method != "ResourceDriver.Create" { - t.Errorf("method: got %q, want ResourceDriver.Create", si.method) - } - // Verify arg keys - for _, key := range []string{"resource_type", "spec_name", "spec_type", "spec_config"} { - if _, ok := si.args[key]; !ok { - t.Errorf("missing arg key %q", key) - } - } - if si.args["resource_type"] != "container_service" { - t.Errorf("resource_type: got %v", si.args["resource_type"]) - } - if si.args["spec_name"] != spec.Name { - t.Errorf("spec_name: got %v", si.args["spec_name"]) - } - if out.ProviderID != "pid-123" { - t.Errorf("ProviderID: got %q", out.ProviderID) - } - if out.Outputs["endpoint"] != "https://example.com" { - t.Errorf("Outputs not decoded: %v", out.Outputs) - } -} - -func TestRemoteDriver_Create_Error(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("rpc error")} - d := newDriver(si) - _, err := d.Create(context.Background(), sampleSpec()) - if err == nil { - t.Fatal("expected error") - } -} - -// ── Read ────────────────────────────────────────────────────────────────────── - -func TestRemoteDriver_Read(t *testing.T) { - si := &stubInvoker{resp: sampleOutputMap()} - d := newDriver(si) - ref := sampleRef() - - out, err := d.Read(context.Background(), ref) - if err != nil { - t.Fatalf("Read: unexpected error: %v", err) - } - if si.method != "ResourceDriver.Read" { - t.Errorf("method: got %q, want ResourceDriver.Read", si.method) - } - for _, key := range []string{"resource_type", "ref_name", "ref_type", "ref_provider_id"} { - if _, ok := si.args[key]; !ok { - t.Errorf("missing arg key %q", key) - } - } - if si.args["ref_name"] != ref.Name { - t.Errorf("ref_name: got %v", si.args["ref_name"]) - } - if out.ProviderID != "pid-123" { - t.Errorf("ProviderID: got %q", out.ProviderID) - } -} - -// ── Update ──────────────────────────────────────────────────────────────────── - -func TestRemoteDriver_Update(t *testing.T) { - si := &stubInvoker{resp: sampleOutputMap()} - d := newDriver(si) - ref := sampleRef() - spec := sampleSpec() - - out, err := d.Update(context.Background(), ref, spec) - if err != nil { - t.Fatalf("Update: unexpected error: %v", err) - } - if si.method != "ResourceDriver.Update" { - t.Errorf("method: got %q, want ResourceDriver.Update", si.method) - } - // Update must also decode outputs/sensitive - if out.Outputs["endpoint"] != "https://example.com" { - t.Errorf("Update: Outputs not decoded: %v", out.Outputs) - } - if !out.Sensitive["endpoint"] { - t.Error("Update: Sensitive[endpoint]: expected true") - } -} - -// ── Delete ──────────────────────────────────────────────────────────────────── - -func TestRemoteDriver_Delete(t *testing.T) { - si := &stubInvoker{resp: map[string]any{}} - d := newDriver(si) - ref := sampleRef() - - err := d.Delete(context.Background(), ref) - if err != nil { - t.Fatalf("Delete: unexpected error: %v", err) - } - if si.method != "ResourceDriver.Delete" { - t.Errorf("method: got %q, want ResourceDriver.Delete", si.method) - } - for _, key := range []string{"resource_type", "ref_name", "ref_type", "ref_provider_id"} { - if _, ok := si.args[key]; !ok { - t.Errorf("missing arg key %q", key) - } - } -} - -func TestRemoteDriver_Delete_Error(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("not found")} - d := newDriver(si) - err := d.Delete(context.Background(), sampleRef()) - if err == nil { - t.Fatal("expected error") - } -} - -// ── Diff ────────────────────────────────────────────────────────────────────── - -func TestRemoteDriver_Diff(t *testing.T) { - diffResp := map[string]any{ - "needs_update": true, - "needs_replace": false, - "changes": []any{ - map[string]any{ - "path": "config.image", - "old": "myapp:v1", - "new": "myapp:v2", - "force_new": false, - }, - }, - } - si := &stubInvoker{resp: diffResp} - d := newDriver(si) - spec := sampleSpec() - current := &interfaces.ResourceOutput{ - Name: "my-resource", - Type: "container_service", - ProviderID: "pid-123", - Status: "running", - Outputs: map[string]any{"image": "myapp:v1"}, - Sensitive: map[string]bool{"password": true}, - } - - result, err := d.Diff(context.Background(), spec, current) - if err != nil { - t.Fatalf("Diff: unexpected error: %v", err) - } - if si.method != "ResourceDriver.Diff" { - t.Errorf("method: got %q, want ResourceDriver.Diff", si.method) - } - // Check that both spec and current fields were sent - for _, key := range []string{"resource_type", "spec_name", "spec_type", "spec_config", - "current_name", "current_type", "current_provider_id", "current_status"} { - if _, ok := si.args[key]; !ok { - t.Errorf("missing arg key %q", key) - } - } - if !result.NeedsUpdate { - t.Error("NeedsUpdate: expected true") - } - if result.NeedsReplace { - t.Error("NeedsReplace: expected false") - } - if len(result.Changes) != 1 { - t.Fatalf("Changes: expected 1, got %d", len(result.Changes)) - } - if result.Changes[0].Path != "config.image" { - t.Errorf("Changes[0].Path: got %q", result.Changes[0].Path) - } -} - -// TestRemoteDriver_Diff_OmitsCurrentSensitiveWhenEmpty pins -// sensitiveToAny's "wire stays trim-friendly" docstring: when -// current.Sensitive is nil/empty, the "current_sensitive" arg is -// omitted entirely rather than serialized as null. (Setting -// args["current_sensitive"] = nil would round-trip through structpb -// as a NullValue, defeating the trim intent.) -func TestRemoteDriver_Diff_OmitsCurrentSensitiveWhenEmpty(t *testing.T) { - si := &stubInvoker{resp: map[string]any{"needs_update": false, "needs_replace": false}} - d := newDriver(si) - spec := sampleSpec() - current := &interfaces.ResourceOutput{ - Name: "my-resource", - Type: "container_service", - ProviderID: "pid-123", - Outputs: map[string]any{}, - Sensitive: nil, // explicit empty - } - if _, err := d.Diff(context.Background(), spec, current); err != nil { - t.Fatalf("Diff: %v", err) - } - if _, present := si.args["current_sensitive"]; present { - t.Errorf("current_sensitive arg present with empty Sensitive map; should be omitted (got %v)", si.args["current_sensitive"]) - } -} - -// TestRemoteDriver_Diff_IncludesCurrentSensitiveWhenPopulated is the -// positive complement: when Sensitive is non-empty, the converted -// map[string]any is sent across the wire so the plugin can observe -// per-key sensitivity flags (the round-trip that T3.9 runtime-launch- -// validation surfaced as silently dropped before sensitiveToAny existed). -func TestRemoteDriver_Diff_IncludesCurrentSensitiveWhenPopulated(t *testing.T) { - si := &stubInvoker{resp: map[string]any{"needs_update": false, "needs_replace": false}} - d := newDriver(si) - spec := sampleSpec() - current := &interfaces.ResourceOutput{ - Name: "my-resource", - Type: "container_service", - ProviderID: "pid-123", - Outputs: map[string]any{}, - Sensitive: map[string]bool{"password": true, "api_key": false}, - } - if _, err := d.Diff(context.Background(), spec, current); err != nil { - t.Fatalf("Diff: %v", err) - } - v, ok := si.args["current_sensitive"] - if !ok { - t.Fatal("current_sensitive missing; expected populated map[string]any") - } - got, ok := v.(map[string]any) - if !ok { - t.Fatalf("current_sensitive type: got %T, want map[string]any", v) - } - if got["password"] != true { - t.Errorf("current_sensitive[password] = %v, want true", got["password"]) - } - if got["api_key"] != false { - t.Errorf("current_sensitive[api_key] = %v, want false", got["api_key"]) - } -} - -// ── Scale ───────────────────────────────────────────────────────────────────── - -func TestRemoteDriver_Scale(t *testing.T) { - si := &stubInvoker{resp: sampleOutputMap()} - d := newDriver(si) - ref := sampleRef() - - out, err := d.Scale(context.Background(), ref, 3) - if err != nil { - t.Fatalf("Scale: unexpected error: %v", err) - } - if si.method != "ResourceDriver.Scale" { - t.Errorf("method: got %q, want ResourceDriver.Scale", si.method) - } - for _, key := range []string{"resource_type", "ref_name", "ref_type", "ref_provider_id", "replicas"} { - if _, ok := si.args[key]; !ok { - t.Errorf("missing arg key %q", key) - } - } - if si.args["replicas"] != 3 { - t.Errorf("replicas: got %v", si.args["replicas"]) - } - if out.ProviderID != "pid-123" { - t.Errorf("ProviderID: got %q", out.ProviderID) - } -} - -// ── SensitiveKeys ───────────────────────────────────────────────────────────── - -func TestRemoteDriver_SensitiveKeys(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "keys": []any{"password", "token"}, - }} - d := newDriver(si) - - keys := d.SensitiveKeys() - if si.method != "ResourceDriver.SensitiveKeys" { - t.Errorf("method: got %q, want ResourceDriver.SensitiveKeys", si.method) - } - if si.args["resource_type"] != "container_service" { - t.Errorf("resource_type: got %v", si.args["resource_type"]) - } - if len(keys) != 2 { - t.Fatalf("expected 2 keys, got %d: %v", len(keys), keys) - } - if keys[0] != "password" || keys[1] != "token" { - t.Errorf("keys: got %v", keys) - } -} - -func TestRemoteDriver_SensitiveKeys_Empty(t *testing.T) { - si := &stubInvoker{resp: map[string]any{}} - d := newDriver(si) - keys := d.SensitiveKeys() - if len(keys) != 0 { - t.Errorf("expected empty keys, got %v", keys) - } -} - -func TestRemoteDriver_SensitiveKeys_Error(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("rpc error")} - d := newDriver(si) - // SensitiveKeys returns []string (no error); on invoker error it should return nil/empty - keys := d.SensitiveKeys() - if len(keys) != 0 { - t.Errorf("expected empty keys on error, got %v", keys) - } -} - -// ── wrapIaCError ────────────────────────────────────────────────────────────── - -func TestWrapIaCError_Nil(t *testing.T) { - if wrapIaCError(nil) != nil { - t.Error("wrapIaCError(nil) should return nil") - } -} - -func TestWrapIaCError_Sentinels(t *testing.T) { - cases := []struct { - msg string - sentinel error - }{ - // ErrResourceNotFound - {"not found", interfaces.ErrResourceNotFound}, - {"NOT FOUND", interfaces.ErrResourceNotFound}, - {"404 returned", interfaces.ErrResourceNotFound}, - {"405 method not allowed", interfaces.ErrResourceNotFound}, - {"does not exist", interfaces.ErrResourceNotFound}, - {"Does Not Exist", interfaces.ErrResourceNotFound}, - {"no such resource", interfaces.ErrResourceNotFound}, - {"No Such Resource", interfaces.ErrResourceNotFound}, - // ErrResourceAlreadyExists - {"app already exists", interfaces.ErrResourceAlreadyExists}, - {"ALREADY EXISTS", interfaces.ErrResourceAlreadyExists}, - {"409 conflict", interfaces.ErrResourceAlreadyExists}, - {"conflict: name taken", interfaces.ErrResourceAlreadyExists}, - // ErrRateLimited - {"rate limit exceeded", interfaces.ErrRateLimited}, - {"Rate Limit", interfaces.ErrRateLimited}, - {"429 too many requests", interfaces.ErrRateLimited}, - {"too many requests", interfaces.ErrRateLimited}, - // ErrTransient - {"500 internal server error", interfaces.ErrTransient}, - {"502 bad gateway", interfaces.ErrTransient}, - {"503 service unavailable", interfaces.ErrTransient}, - {"504 gateway timeout", interfaces.ErrTransient}, - {"bad gateway", interfaces.ErrTransient}, - {"gateway timeout", interfaces.ErrTransient}, - {"service unavailable", interfaces.ErrTransient}, - // ErrUnauthorized - {"401 unauthorized", interfaces.ErrUnauthorized}, - {"unauthorized", interfaces.ErrUnauthorized}, - {"unable to authenticate", interfaces.ErrUnauthorized}, - // ErrForbidden - {"403 forbidden", interfaces.ErrForbidden}, - {"forbidden", interfaces.ErrForbidden}, - // ErrValidation - {"400 bad request", interfaces.ErrValidation}, - {"422 unprocessable entity", interfaces.ErrValidation}, - {"validation failed", interfaces.ErrValidation}, - {"invalid field: name", interfaces.ErrValidation}, - } - for _, tc := range cases { - err := wrapIaCError(fmt.Errorf("%s", tc.msg)) - if !errors.Is(err, tc.sentinel) { - t.Errorf("msg %q: expected %v, got %v", tc.msg, tc.sentinel, err) - } - // Original message must be preserved. - if !strings.Contains(err.Error(), tc.msg) { - t.Errorf("msg %q: original message not preserved in %q", tc.msg, err.Error()) - } - } -} - -func TestWrapIaCError_PassThrough(t *testing.T) { - msgs := []string{ - "connection reset by peer", - "timeout waiting for lock", - "unexpected end of stream", - } - for _, msg := range msgs { - orig := fmt.Errorf("%s", msg) - err := wrapIaCError(orig) - for _, s := range []error{ - interfaces.ErrResourceNotFound, - interfaces.ErrResourceAlreadyExists, - interfaces.ErrRateLimited, - interfaces.ErrTransient, - interfaces.ErrUnauthorized, - interfaces.ErrForbidden, - interfaces.ErrValidation, - } { - if errors.Is(err, s) { - t.Errorf("msg %q: should not match %v", msg, s) - } - } - if err.Error() != orig.Error() { - t.Errorf("msg %q: error string changed: got %q", msg, err.Error()) - } - } -} - -// ── per-method wrapIaCError coverage ───────────────────────────────────────── - -// methodSentinelCases lists (error message, expected sentinel) pairs used -// across all driver method wrapping tests below. -var methodSentinelCases = []struct { - msg string - sentinel error -}{ - {"404 not found", interfaces.ErrResourceNotFound}, - {"already exists", interfaces.ErrResourceAlreadyExists}, - {"429 too many requests", interfaces.ErrRateLimited}, - {"503 service unavailable", interfaces.ErrTransient}, - {"401 unauthorized", interfaces.ErrUnauthorized}, - {"403 forbidden", interfaces.ErrForbidden}, - {"422 validation failed", interfaces.ErrValidation}, -} - -func TestRemoteDriver_Create_WrapsAllSentinels(t *testing.T) { - for _, tc := range methodSentinelCases { - si := &stubInvoker{err: fmt.Errorf("%s", tc.msg)} - d := newDriver(si) - _, err := d.Create(context.Background(), sampleSpec()) - if err == nil { - t.Fatalf("Create %q: expected error", tc.msg) - } - if !errors.Is(err, tc.sentinel) { - t.Errorf("Create %q: expected %v, got %v", tc.msg, tc.sentinel, err) - } - } -} - -func TestRemoteDriver_Read_WrapsAllSentinels(t *testing.T) { - for _, tc := range methodSentinelCases { - si := &stubInvoker{err: fmt.Errorf("%s", tc.msg)} - d := newDriver(si) - _, err := d.Read(context.Background(), sampleRef()) - if err == nil { - t.Fatalf("Read %q: expected error", tc.msg) - } - if !errors.Is(err, tc.sentinel) { - t.Errorf("Read %q: expected %v, got %v", tc.msg, tc.sentinel, err) - } - } -} - -func TestRemoteDriver_Update_WrapsAllSentinels(t *testing.T) { - for _, tc := range methodSentinelCases { - si := &stubInvoker{err: fmt.Errorf("%s", tc.msg)} - d := newDriver(si) - _, err := d.Update(context.Background(), sampleRef(), sampleSpec()) - if err == nil { - t.Fatalf("Update %q: expected error", tc.msg) - } - if !errors.Is(err, tc.sentinel) { - t.Errorf("Update %q: expected %v, got %v", tc.msg, tc.sentinel, err) - } - } -} - -func TestRemoteDriver_Delete_WrapsAllSentinels(t *testing.T) { - for _, tc := range methodSentinelCases { - si := &stubInvoker{err: fmt.Errorf("%s", tc.msg)} - d := newDriver(si) - err := d.Delete(context.Background(), sampleRef()) - if err == nil { - t.Fatalf("Delete %q: expected error", tc.msg) - } - if !errors.Is(err, tc.sentinel) { - t.Errorf("Delete %q: expected %v, got %v", tc.msg, tc.sentinel, err) - } - } -} - -func TestRemoteDriver_Diff_WrapsAllSentinels(t *testing.T) { - current := &interfaces.ResourceOutput{ProviderID: "pid-123"} - for _, tc := range methodSentinelCases { - si := &stubInvoker{err: fmt.Errorf("%s", tc.msg)} - d := newDriver(si) - _, err := d.Diff(context.Background(), sampleSpec(), current) - if err == nil { - t.Fatalf("Diff %q: expected error", tc.msg) - } - if !errors.Is(err, tc.sentinel) { - t.Errorf("Diff %q: expected %v, got %v", tc.msg, tc.sentinel, err) - } - } -} - -func TestRemoteDriver_Scale_WrapsAllSentinels(t *testing.T) { - for _, tc := range methodSentinelCases { - si := &stubInvoker{err: fmt.Errorf("%s", tc.msg)} - d := newDriver(si) - _, err := d.Scale(context.Background(), sampleRef(), 2) - if err == nil { - t.Fatalf("Scale %q: expected error", tc.msg) - } - if !errors.Is(err, tc.sentinel) { - t.Errorf("Scale %q: expected %v, got %v", tc.msg, tc.sentinel, err) - } - } -} - -func TestRemoteDriver_HealthCheck_WrapsAllSentinels(t *testing.T) { - for _, tc := range methodSentinelCases { - si := &stubInvoker{err: fmt.Errorf("%s", tc.msg)} - d := newDriver(si) - _, err := d.HealthCheck(context.Background(), sampleRef()) - if err == nil { - t.Fatalf("HealthCheck %q: expected error", tc.msg) - } - if !errors.Is(err, tc.sentinel) { - t.Errorf("HealthCheck %q: expected %v, got %v", tc.msg, tc.sentinel, err) - } - } -} - -// ── Troubleshoot ────────────────────────────────────────────────────────────── - -func TestRemoteDriver_Troubleshoot_Success(t *testing.T) { - si := &stubInvoker{ - resp: map[string]any{ - "diagnostics": []any{ - map[string]any{ - "id": "dep-1", "phase": "pre_deploy", - "cause": "exit 1", "at": "2026-04-24T00:00:00Z", - "detail": "log tail", - }, - }, - }, - } - d := newDriver(si) - ref := interfaces.ResourceRef{Name: "bmw-staging", Type: "app_platform", ProviderID: "abc-123"} - diags, err := d.Troubleshoot(context.Background(), ref, "boom") - if err != nil { - t.Fatalf("unexpected: %v", err) - } - if len(diags) != 1 || diags[0].Cause != "exit 1" { - t.Fatalf("unexpected diags: %+v", diags) - } - if si.method != "ResourceDriver.Troubleshoot" { - t.Errorf("wrong svc: %s", si.method) - } - if diags[0].Detail != "log tail" { - t.Errorf("Detail: got %q", diags[0].Detail) - } - - // Assert args are flat primitives — structpb.NewStruct (gRPC transport) - // cannot encode Go structs; ref must be decomposed to scalar fields. - if si.args["ref_name"] != ref.Name { - t.Errorf("args[ref_name] = %q, want %q", si.args["ref_name"], ref.Name) - } - if si.args["ref_type"] != ref.Type { - t.Errorf("args[ref_type] = %q, want %q", si.args["ref_type"], ref.Type) - } - if si.args["ref_provider_id"] != ref.ProviderID { - t.Errorf("args[ref_provider_id] = %q, want %q", si.args["ref_provider_id"], ref.ProviderID) - } - if si.args["failure_msg"] != "boom" { - t.Errorf("args[failure_msg] = %q, want %q", si.args["failure_msg"], "boom") - } - if _, hasOldRef := si.args["ref"]; hasOldRef { - t.Error("args must not contain a nested 'ref' struct — structpb cannot encode it") - } - // resource_type assertion lives in TestRemoteDriver_AllMethodsSendResourceType -} - -func TestRemoteDriver_Troubleshoot_UnimplementedSilent(t *testing.T) { - si := &stubInvoker{err: status.Error(codes.Unimplemented, "method not implemented")} - d := newDriver(si) - diags, err := d.Troubleshoot(context.Background(), interfaces.ResourceRef{Name: "x"}, "boom") - if err != nil { - t.Fatalf("Unimplemented should not surface: %v", err) - } - if diags != nil { - t.Fatalf("expected nil diags, got %+v", diags) - } -} - -func TestRemoteDriver_Troubleshoot_OtherErrorSurfaces(t *testing.T) { - si := &stubInvoker{err: errors.New("network oops")} - d := newDriver(si) - _, err := d.Troubleshoot(context.Background(), interfaces.ResourceRef{Name: "x"}, "boom") - if err == nil { - t.Fatal("expected error to surface") - } -} - -// TestRemoteDriver_AllMethodsSendResourceType is a regression gate for the -// class of bug where a new or modified ResourceDriver method omits -// "resource_type" from its InvokeService args map. Every method on -// remoteResourceDriver must include "resource_type": d.resourceType so the -// plugin side can dispatch to the correct driver implementation. -// -// The Troubleshoot method regressed in v0.18.11 (missing resource_type); -// this table test ensures the invariant holds across all 9 public methods. -func TestRemoteDriver_AllMethodsSendResourceType(t *testing.T) { - ref := sampleRef() - spec := sampleSpec() - current := &interfaces.ResourceOutput{ - ProviderID: "pid-123", - Name: "my-resource", - Type: "container_service", - Status: "running", - Outputs: map[string]any{"endpoint": "https://example.com"}, - Sensitive: map[string]bool{}, - } - - type testCase struct { - name string - call func(d *remoteResourceDriver, si *stubInvoker) - resp map[string]any - } - - cases := []testCase{ - { - name: "Create", - resp: sampleOutputMap(), - call: func(d *remoteResourceDriver, _ *stubInvoker) { - _, _ = d.Create(context.Background(), spec) - }, - }, - { - name: "Read", - resp: sampleOutputMap(), - call: func(d *remoteResourceDriver, _ *stubInvoker) { - _, _ = d.Read(context.Background(), ref) - }, - }, - { - name: "Update", - resp: sampleOutputMap(), - call: func(d *remoteResourceDriver, _ *stubInvoker) { - _, _ = d.Update(context.Background(), ref, spec) - }, - }, - { - name: "Delete", - resp: map[string]any{}, - call: func(d *remoteResourceDriver, _ *stubInvoker) { - _ = d.Delete(context.Background(), ref) - }, - }, - { - name: "Diff", - resp: map[string]any{"needs_update": false, "needs_replace": false}, - call: func(d *remoteResourceDriver, _ *stubInvoker) { - _, _ = d.Diff(context.Background(), spec, current) - }, - }, - { - name: "HealthCheck", - resp: map[string]any{"healthy": true, "message": "ok"}, - call: func(d *remoteResourceDriver, _ *stubInvoker) { - _, _ = d.HealthCheck(context.Background(), ref) - }, - }, - { - name: "Scale", - resp: sampleOutputMap(), - call: func(d *remoteResourceDriver, _ *stubInvoker) { - _, _ = d.Scale(context.Background(), ref, 3) - }, - }, - { - name: "SensitiveKeys", - resp: map[string]any{"keys": []any{"secret"}}, - call: func(d *remoteResourceDriver, _ *stubInvoker) { - _ = d.SensitiveKeys() - }, - }, - { - name: "Troubleshoot", - resp: map[string]any{"diagnostics": []any{}}, - call: func(d *remoteResourceDriver, _ *stubInvoker) { - _, _ = d.Troubleshoot(context.Background(), ref, "boom") - }, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - si := &stubInvoker{resp: tc.resp} - d := newDriver(si) - tc.call(d, si) - got, ok := si.args["resource_type"] - if !ok { - t.Errorf("%s: args missing \"resource_type\" — plugin will return 'missing resource_type arg'", tc.name) - return - } - if got != "container_service" { - t.Errorf("%s: resource_type = %q, want %q", tc.name, got, "container_service") - } - }) - } -} - -func TestRemoteDriver_PassThroughUnknownErrors(t *testing.T) { - msg := "connection reset by peer" - for _, method := range []string{"create", "read", "update", "delete", "diff", "scale", "hc"} { - var err error - si := &stubInvoker{err: fmt.Errorf("%s", msg)} - d := newDriver(si) - current := &interfaces.ResourceOutput{ProviderID: "pid-123"} - switch method { - case "create": - _, err = d.Create(context.Background(), sampleSpec()) - case "read": - _, err = d.Read(context.Background(), sampleRef()) - case "update": - _, err = d.Update(context.Background(), sampleRef(), sampleSpec()) - case "delete": - err = d.Delete(context.Background(), sampleRef()) - case "diff": - _, err = d.Diff(context.Background(), sampleSpec(), current) - case "scale": - _, err = d.Scale(context.Background(), sampleRef(), 2) - case "hc": - _, err = d.HealthCheck(context.Background(), sampleRef()) - } - if err == nil { - t.Fatalf("%s: expected error", method) - } - for _, s := range []error{ - interfaces.ErrResourceNotFound, interfaces.ErrResourceAlreadyExists, - interfaces.ErrRateLimited, interfaces.ErrTransient, - interfaces.ErrUnauthorized, interfaces.ErrForbidden, interfaces.ErrValidation, - } { - if errors.Is(err, s) { - t.Errorf("%s: unknown error %q should not match sentinel %v", method, msg, s) - } - } - } -} diff --git a/cmd/wfctl/deploy_providers_remote_iac_compat_test.go b/cmd/wfctl/deploy_providers_remote_iac_compat_test.go deleted file mode 100644 index 5258378e..00000000 --- a/cmd/wfctl/deploy_providers_remote_iac_compat_test.go +++ /dev/null @@ -1,65 +0,0 @@ -package main - -import ( - "context" - "testing" - - "github.com/GoCodeAlone/workflow/interfaces" -) - -// TestRemoteIaC_DriftConfigDetector_SendsSpecsViaDetectDrift pins the wire -// protocol: DetectDriftWithSpecs sends "IaCProvider.DetectDrift" with a -// "specs" arg map. The DO plugin v0.10.5+ dispatches spec-injection inside -// IaCProvider.DetectDrift when the "specs" key is present — no separate RPC -// method name is required. -func TestRemoteIaC_DriftConfigDetector_SendsSpecsViaDetectDrift(t *testing.T) { - si := &multiMethodStubInvoker{ - respByMethod: map[string]map[string]any{ - "IaCProvider.DetectDrift": { - "drifts": []any{map[string]any{"name": "x", "type": "infra.test", "drifted": true, "class": "config"}}, - }, - }, - } - p := &remoteIaCProvider{invoker: si} - - refs := []interfaces.ResourceRef{{Name: "x", Type: "infra.test"}} - specs := map[string]interfaces.ResourceSpec{ - "x": {Name: "x", Type: "infra.test", Config: map[string]any{"region": "nyc3"}}, - } - drifts, err := p.DetectDriftWithSpecs(context.Background(), refs, specs) - if err != nil { - t.Fatalf("DetectDriftWithSpecs: unexpected error: %v", err) - } - if len(drifts) != 1 || drifts[0].Class != interfaces.DriftClassConfig { - t.Errorf("expected config-drift result; got %+v", drifts) - } - - // Verify the invoker was called with IaCProvider.DetectDrift (not a - // separate DetectDriftWithSpecs method) — this is the wire contract. - if !si.calledMethods["IaCProvider.DetectDrift"] { - t.Errorf("DetectDriftWithSpecs must invoke IaCProvider.DetectDrift; called methods: %v", si.calledMethods) - } -} - -// multiMethodStubInvoker is a test double for remoteServiceInvoker that supports -// per-method response and error configuration (unlike the basic stubInvoker -// which records only a single method/resp/err). -type multiMethodStubInvoker struct { - calledMethods map[string]bool - respByMethod map[string]map[string]any - errByMethod map[string]error -} - -func (s *multiMethodStubInvoker) InvokeService(method string, args map[string]any) (map[string]any, error) { - if s.calledMethods == nil { - s.calledMethods = map[string]bool{} - } - s.calledMethods[method] = true - if err, ok := s.errByMethod[method]; ok && err != nil { - return nil, err - } - if resp, ok := s.respByMethod[method]; ok { - return resp, nil - } - return nil, nil -} diff --git a/cmd/wfctl/deploy_providers_remote_iac_test.go b/cmd/wfctl/deploy_providers_remote_iac_test.go deleted file mode 100644 index e8ccb486..00000000 --- a/cmd/wfctl/deploy_providers_remote_iac_test.go +++ /dev/null @@ -1,1090 +0,0 @@ -package main - -import ( - "context" - "errors" - "fmt" - "strings" - "testing" - "time" - - "github.com/GoCodeAlone/workflow/iac/wfctlhelpers" - "github.com/GoCodeAlone/workflow/interfaces" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" -) - -// newIaCProvider builds a remoteIaCProvider backed by the given stubInvoker. -// Defaults to empty computePlanVersion (the safe-default v1 branch in -// dispatch.go's "default-to-v1" doctrine). Tests that need the v2 branch -// set p.computePlanVersion = wfctlhelpers.DispatchVersionV2 directly. -func newIaCProvider(si *stubInvoker) *remoteIaCProvider { - return &remoteIaCProvider{invoker: si} -} - -// ── Capabilities ────────────────────────────────────────────────────────────── - -func TestRemoteIaC_Capabilities(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "capabilities": []any{ - map[string]any{ - "resource_type": "infra.database", - "tier": float64(1), - "operations": []any{"create", "read", "update", "delete"}, - }, - }, - }} - p := newIaCProvider(si) - - caps := p.Capabilities() - if si.method != "IaCProvider.Capabilities" { - t.Errorf("method: got %q, want IaCProvider.Capabilities", si.method) - } - if len(caps) != 1 { - t.Fatalf("expected 1 capability, got %d", len(caps)) - } - if caps[0].ResourceType != "infra.database" { - t.Errorf("ResourceType: got %q", caps[0].ResourceType) - } - if caps[0].Tier != 1 { - t.Errorf("Tier: got %d", caps[0].Tier) - } - if len(caps[0].Operations) != 4 { - t.Errorf("Operations: got %v", caps[0].Operations) - } -} - -func TestRemoteIaC_Capabilities_Empty(t *testing.T) { - si := &stubInvoker{resp: map[string]any{}} - p := newIaCProvider(si) - caps := p.Capabilities() - if len(caps) != 0 { - t.Errorf("expected empty capabilities, got %v", caps) - } -} - -func TestRemoteIaC_Capabilities_Error(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("rpc error")} - p := newIaCProvider(si) - caps := p.Capabilities() - if len(caps) != 0 { - t.Errorf("expected nil on error, got %v", caps) - } -} - -// ── Plan ────────────────────────────────────────────────────────────────────── -// -// Plan() is manifest-conditional after W-Refactor (PR 5): -// - computePlanVersion == "v2" → delegates to platform.ComputePlan -// (wfctl owns plan classification; ResourceDriver.Diff dispatches -// remotely on a per-resource basis); -// - otherwise (default v1) → proxies the legacy monolithic -// IaCProvider.Plan call to the plugin via InvokeService. -// -// Tests below pin BOTH branches. - -// v1-default branch: legacy proxy to plugin. - -func samplePlanResponse() map[string]any { - return map[string]any{ - "id": "plan-abc", - "actions": []any{ - map[string]any{ - "action": "create", - "resource": map[string]any{ - "name": "db", - "type": "infra.database", - "config": map[string]any{}, - }, - }, - }, - "created_at": time.Now().Format(time.RFC3339Nano), - } -} - -func TestRemoteIaC_Plan_V1Default_ProxiesIaCProviderPlan(t *testing.T) { - si := &stubInvoker{resp: samplePlanResponse()} - p := newIaCProvider(si) // default computePlanVersion == "" - - desired := []interfaces.ResourceSpec{ - {Name: "db", Type: "infra.database", Config: map[string]any{"engine": "postgres"}}, - } - current := []interfaces.ResourceState{ - {Name: "old-db", Type: "infra.database", ProviderID: "pid-old"}, - } - - plan, err := p.Plan(context.Background(), desired, current) - if err != nil { - t.Fatalf("Plan: unexpected error: %v", err) - } - if si.method != "IaCProvider.Plan" { - t.Errorf("v1-default branch must proxy IaCProvider.Plan; got %q", si.method) - } - if _, ok := si.args["desired"]; !ok { - t.Error("missing arg key 'desired'") - } - if _, ok := si.args["current"]; !ok { - t.Error("missing arg key 'current'") - } - if plan.ID != "plan-abc" { - t.Errorf("plan ID: got %q", plan.ID) - } - if len(plan.Actions) != 1 { - t.Fatalf("expected 1 action, got %d", len(plan.Actions)) - } - if plan.Actions[0].Action != "create" { - t.Errorf("action: got %q", plan.Actions[0].Action) - } -} - -func TestRemoteIaC_Plan_V1Default_PropagatesError(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("rpc error")} - p := newIaCProvider(si) - _, err := p.Plan(context.Background(), nil, nil) - if err == nil { - t.Fatal("expected error from v1 IaCProvider.Plan proxy") - } -} - -// v2 branch: delegates to platform.ComputePlan. - -func TestRemoteIaC_Plan_V2_DelegatesToComputePlan_NetNewResource(t *testing.T) { - // stubInvoker tracks the LAST InvokeService call. With ComputePlan - // delegation, a net-new resource emits "create" without touching the - // invoker — confirming the v2 branch routes through wfctl-side - // classification rather than the v1 IaCProvider.Plan wire. - si := &stubInvoker{} - p := newIaCProvider(si) - p.computePlanVersion = wfctlhelpers.DispatchVersionV2 - - desired := []interfaces.ResourceSpec{ - {Name: "db", Type: "infra.database", Config: map[string]any{"engine": "postgres"}}, - } - plan, err := p.Plan(context.Background(), desired, nil) - if err != nil { - t.Fatalf("Plan: unexpected error: %v", err) - } - if si.method != "" { - t.Errorf("v2 branch + net-new create should not hit InvokeService; got %q", si.method) - } - if plan == nil { - t.Fatal("Plan returned nil plan") - } - if len(plan.Actions) != 1 { - t.Fatalf("expected 1 action (create), got %d", len(plan.Actions)) - } - if plan.Actions[0].Action != "create" { - t.Errorf("action: got %q, want %q", plan.Actions[0].Action, "create") - } - if plan.Actions[0].Resource.Name != "db" { - t.Errorf("action resource name: got %q, want %q", plan.Actions[0].Resource.Name, "db") - } -} - -func TestRemoteIaC_Plan_V2_DelegatesToComputePlan_DeleteEmittedForRemoved(t *testing.T) { - si := &stubInvoker{} - p := newIaCProvider(si) - p.computePlanVersion = wfctlhelpers.DispatchVersionV2 - - current := []interfaces.ResourceState{ - {Name: "old-db", Type: "infra.database", ProviderID: "pid-old"}, - } - plan, err := p.Plan(context.Background(), nil, current) - if err != nil { - t.Fatalf("Plan: unexpected error: %v", err) - } - if si.method != "" { - t.Errorf("v2 branch + delete path should not hit InvokeService; got %q", si.method) - } - if plan == nil { - t.Fatal("Plan returned nil plan") - } - if len(plan.Actions) != 1 { - t.Fatalf("expected 1 action (delete), got %d", len(plan.Actions)) - } - if plan.Actions[0].Action != "delete" { - t.Errorf("action: got %q, want %q", plan.Actions[0].Action, "delete") - } -} - -// ── Apply ───────────────────────────────────────────────────────────────────── -// -// Apply() is manifest-conditional after W-Refactor (PR 5): -// - computePlanVersion == "v2" → delegates to wfctlhelpers.ApplyPlan -// (per-action driver dispatch + drift postcondition); -// - otherwise (default v1) → proxies the legacy monolithic -// IaCProvider.Apply call to the plugin via InvokeService. -// -// Tests below pin BOTH branches. - -// v1-default branch: legacy proxy to plugin. - -func TestRemoteIaC_Apply_V1Default_ProxiesIaCProviderApply(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "plan_id": "plan-abc", - "resources": []any{ - map[string]any{ - "provider_id": "pid-123", - "name": "db", - "type": "infra.database", - "status": "running", - }, - }, - }} - p := newIaCProvider(si) // default computePlanVersion == "" - - plan := &interfaces.IaCPlan{ - ID: "plan-abc", - Actions: []interfaces.PlanAction{ - {Action: "create", Resource: interfaces.ResourceSpec{Name: "db", Type: "infra.database"}}, - }, - } - result, err := p.Apply(context.Background(), plan) - if err != nil { - t.Fatalf("Apply: unexpected error: %v", err) - } - if si.method != "IaCProvider.Apply" { - t.Errorf("v1-default branch must proxy IaCProvider.Apply; got %q", si.method) - } - if _, ok := si.args["plan"]; !ok { - t.Error("missing arg key 'plan'") - } - if result.PlanID != "plan-abc" { - t.Errorf("PlanID: got %q", result.PlanID) - } - if len(result.Resources) != 1 { - t.Fatalf("expected 1 resource, got %d", len(result.Resources)) - } - if result.Resources[0].Name != "db" { - t.Errorf("resource name: got %q", result.Resources[0].Name) - } -} - -func TestRemoteIaC_Apply_V1Default_PropagatesError(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("apply failed")} - p := newIaCProvider(si) - _, err := p.Apply(context.Background(), &interfaces.IaCPlan{ID: "p1"}) - if err == nil { - t.Fatal("expected error from v1 IaCProvider.Apply proxy") - } -} - -// v2 branch: delegates to wfctlhelpers.ApplyPlan (per-action driver dispatch). - -func TestRemoteIaC_Apply_V2_DelegatesToApplyPlan_PerDriverDispatch(t *testing.T) { - // ApplyPlan dispatches Create on a single-create plan via - // remoteResourceDriver, which invokes "ResourceDriver.Create" through - // the stub invoker. The v1 monolithic "IaCProvider.Apply" wire is - // not used in the v2 branch. - si := &stubInvoker{resp: map[string]any{ - "output": map[string]any{ - "provider_id": "pid-123", - "name": "db", - "type": "infra.database", - "status": "running", - }, - }} - p := newIaCProvider(si) - p.computePlanVersion = wfctlhelpers.DispatchVersionV2 - - plan := &interfaces.IaCPlan{ - ID: "plan-abc", - Actions: []interfaces.PlanAction{ - {Action: "create", Resource: interfaces.ResourceSpec{Name: "db", Type: "infra.database"}}, - }, - } - result, err := p.Apply(context.Background(), plan) - if err != nil { - t.Fatalf("Apply: unexpected error: %v", err) - } - if si.method == "IaCProvider.Apply" { - t.Error("v2 branch must NOT invoke legacy IaCProvider.Apply wire") - } - if !strings.HasPrefix(si.method, "ResourceDriver.") { - t.Errorf("v2 branch: expected ResourceDriver.* per-driver dispatch, got %q", si.method) - } - if result == nil { - t.Fatal("Apply returned nil result") - } - if result.PlanID != "plan-abc" { - t.Errorf("PlanID: got %q, want %q (ApplyPlan stamps plan.ID onto result)", result.PlanID, "plan-abc") - } -} - -func TestRemoteIaC_Apply_V2_DelegatesToApplyPlan_RecordsErrorsPerAction(t *testing.T) { - // When the underlying driver returns an error, ApplyPlan records it - // in result.Errors rather than returning the error from the top-level - // call (per the per-action error decomposition contract). - si := &stubInvoker{err: fmt.Errorf("driver create failed")} - p := newIaCProvider(si) - p.computePlanVersion = wfctlhelpers.DispatchVersionV2 - - plan := &interfaces.IaCPlan{ - ID: "p1", - Actions: []interfaces.PlanAction{ - {Action: "create", Resource: interfaces.ResourceSpec{Name: "db", Type: "infra.database"}}, - }, - } - result, err := p.Apply(context.Background(), plan) - if err != nil { - t.Fatalf("Apply: unexpected top-level error (wfctlhelpers.ApplyPlan records per-action errors): %v", err) - } - if result == nil { - t.Fatal("Apply returned nil result") - } - if len(result.Errors) == 0 { - t.Error("expected ApplyResult.Errors to contain the per-action driver error") - } -} - -// ── Destroy ─────────────────────────────────────────────────────────────────── - -func TestRemoteIaC_Destroy(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "destroyed": []any{"db", "cache"}, - }} - p := newIaCProvider(si) - - refs := []interfaces.ResourceRef{ - {Name: "db", Type: "infra.database", ProviderID: "pid-1"}, - {Name: "cache", Type: "infra.cache", ProviderID: "pid-2"}, - } - - result, err := p.Destroy(context.Background(), refs) - if err != nil { - t.Fatalf("Destroy: unexpected error: %v", err) - } - if si.method != "IaCProvider.Destroy" { - t.Errorf("method: got %q, want IaCProvider.Destroy", si.method) - } - if _, ok := si.args["refs"]; !ok { - t.Error("missing arg key 'refs'") - } - if len(result.Destroyed) != 2 { - t.Fatalf("expected 2 destroyed, got %d", len(result.Destroyed)) - } -} - -func TestRemoteIaC_Destroy_Error(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("destroy failed")} - p := newIaCProvider(si) - _, err := p.Destroy(context.Background(), nil) - if err == nil { - t.Fatal("expected error") - } -} - -// ── Status ──────────────────────────────────────────────────────────────────── - -func TestRemoteIaC_Status(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "statuses": []any{ - map[string]any{ - "name": "db", - "type": "infra.database", - "provider_id": "pid-1", - "status": "running", - "outputs": map[string]any{"endpoint": "db.example.com"}, - }, - }, - }} - p := newIaCProvider(si) - - refs := []interfaces.ResourceRef{{Name: "db", Type: "infra.database", ProviderID: "pid-1"}} - statuses, err := p.Status(context.Background(), refs) - if err != nil { - t.Fatalf("Status: unexpected error: %v", err) - } - if si.method != "IaCProvider.Status" { - t.Errorf("method: got %q, want IaCProvider.Status", si.method) - } - if _, ok := si.args["refs"]; !ok { - t.Error("missing arg key 'refs'") - } - if len(statuses) != 1 { - t.Fatalf("expected 1 status, got %d", len(statuses)) - } - if statuses[0].Name != "db" { - t.Errorf("Name: got %q", statuses[0].Name) - } - if statuses[0].Status != "running" { - t.Errorf("Status: got %q", statuses[0].Status) - } -} - -func TestRemoteIaC_Status_Empty(t *testing.T) { - si := &stubInvoker{resp: map[string]any{}} - p := newIaCProvider(si) - statuses, err := p.Status(context.Background(), nil) - if err != nil { - t.Fatalf("Status: unexpected error: %v", err) - } - if len(statuses) != 0 { - t.Errorf("expected empty statuses, got %v", statuses) - } -} - -// ── DetectDrift ─────────────────────────────────────────────────────────────── - -func TestRemoteIaC_DetectDrift(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "drifts": []any{ - map[string]any{ - "name": "db", - "type": "infra.database", - "drifted": true, - "expected": map[string]any{"engine": "postgres"}, - "actual": map[string]any{"engine": "mysql"}, - "fields": []any{"engine"}, - }, - }, - }} - p := newIaCProvider(si) - - refs := []interfaces.ResourceRef{{Name: "db", Type: "infra.database", ProviderID: "pid-1"}} - drifts, err := p.DetectDrift(context.Background(), refs) - if err != nil { - t.Fatalf("DetectDrift: unexpected error: %v", err) - } - if si.method != "IaCProvider.DetectDrift" { - t.Errorf("method: got %q, want IaCProvider.DetectDrift", si.method) - } - if _, ok := si.args["refs"]; !ok { - t.Error("missing arg key 'refs'") - } - if len(drifts) != 1 { - t.Fatalf("expected 1 drift, got %d", len(drifts)) - } - if !drifts[0].Drifted { - t.Error("Drifted: expected true") - } - if len(drifts[0].Fields) != 1 || drifts[0].Fields[0] != "engine" { - t.Errorf("Fields: got %v", drifts[0].Fields) - } -} - -func TestRemoteIaC_DetectDrift_Empty(t *testing.T) { - si := &stubInvoker{resp: map[string]any{}} - p := newIaCProvider(si) - drifts, err := p.DetectDrift(context.Background(), nil) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if len(drifts) != 0 { - t.Errorf("expected empty drifts, got %v", drifts) - } -} - -// ── DetectDriftWithSpecs ─────────────────────────────────────────────────────── - -func TestRemoteIaC_DetectDriftWithSpecs_HappyPath(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "drifts": []any{map[string]any{ - "name": "x", - "type": "infra.test", - "drifted": true, - "class": "config", - "fields": []any{"region"}, - }}, - }} - p := newIaCProvider(si) - refs := []interfaces.ResourceRef{{Name: "x", Type: "infra.test"}} - specs := map[string]interfaces.ResourceSpec{ - "x": {Name: "x", Type: "infra.test", Config: map[string]any{"region": "nyc1"}}, - } - drifts, err := p.DetectDriftWithSpecs(context.Background(), refs, specs) - if err != nil { - t.Fatalf("DetectDriftWithSpecs: %v", err) - } - // Wire protocol: specs are sent via IaCProvider.DetectDrift with "specs" arg. - if si.method != "IaCProvider.DetectDrift" { - t.Errorf("method: got %q, want IaCProvider.DetectDrift", si.method) - } - // "specs" key must be present; legacy "applied" key must not be present. - if _, ok := si.args["specs"]; !ok { - t.Errorf("InvokeService args must contain 'specs' key; got %v", si.args) - } - if _, ok := si.args["applied"]; ok { - t.Errorf("InvokeService args must NOT contain legacy 'applied' key; got %v", si.args) - } - if len(drifts) != 1 || drifts[0].Class != interfaces.DriftClassConfig { - t.Errorf("drifts: %+v", drifts) - } -} - -// ── Import ──────────────────────────────────────────────────────────────────── - -func TestRemoteIaC_Import(t *testing.T) { - now := time.Now().UTC().Truncate(time.Second) - si := &stubInvoker{resp: map[string]any{ - "id": "state-xyz", - "name": "my-db", - "type": "infra.database", - "provider": "digitalocean", - "provider_id": "do-db-123", - "created_at": now.Format(time.RFC3339), - "updated_at": now.Format(time.RFC3339), - }} - p := newIaCProvider(si) - - state, err := p.Import(context.Background(), "do-db-123", "infra.database") - if err != nil { - t.Fatalf("Import: unexpected error: %v", err) - } - if si.method != "IaCProvider.Import" { - t.Errorf("method: got %q, want IaCProvider.Import", si.method) - } - if si.args["provider_id"] != "do-db-123" { - t.Errorf("provider_id arg: got %v", si.args["provider_id"]) - } - if si.args["resource_type"] != "infra.database" { - t.Errorf("resource_type arg: got %v", si.args["resource_type"]) - } - if state.ProviderID != "do-db-123" { - t.Errorf("ProviderID: got %q", state.ProviderID) - } - if state.Type != "infra.database" { - t.Errorf("Type: got %q", state.Type) - } -} - -func TestRemoteIaC_Import_Error(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("not found")} - p := newIaCProvider(si) - _, err := p.Import(context.Background(), "pid-x", "infra.database") - if err == nil { - t.Fatal("expected error") - } -} - -// ── ResolveSizing ───────────────────────────────────────────────────────────── - -func TestRemoteIaC_ResolveSizing(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "instance_type": "db-s-1vcpu-1gb", - "specs": map[string]any{ - "cpu": "1", - "memory": "1Gi", - }, - }} - p := newIaCProvider(si) - - hints := &interfaces.ResourceHints{CPU: "1", Memory: "1Gi"} - sizing, err := p.ResolveSizing("infra.database", interfaces.SizeS, hints) - if err != nil { - t.Fatalf("ResolveSizing: unexpected error: %v", err) - } - if si.method != "IaCProvider.ResolveSizing" { - t.Errorf("method: got %q, want IaCProvider.ResolveSizing", si.method) - } - if si.args["resource_type"] != "infra.database" { - t.Errorf("resource_type: got %v", si.args["resource_type"]) - } - if si.args["size"] != "s" { - t.Errorf("size: got %v", si.args["size"]) - } - if _, ok := si.args["hints"]; !ok { - t.Error("missing arg key 'hints'") - } - if sizing.InstanceType != "db-s-1vcpu-1gb" { - t.Errorf("InstanceType: got %q", sizing.InstanceType) - } -} - -func TestRemoteIaC_ResolveSizing_NilHints(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "instance_type": "db-s-1vcpu-1gb", - "specs": map[string]any{}, - }} - p := newIaCProvider(si) - sizing, err := p.ResolveSizing("infra.database", interfaces.SizeM, nil) - if err != nil { - t.Fatalf("ResolveSizing: unexpected error: %v", err) - } - if sizing.InstanceType != "db-s-1vcpu-1gb" { - t.Errorf("InstanceType: got %q", sizing.InstanceType) - } -} - -func TestRemoteIaC_ResolveSizing_Error(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("unsupported size")} - p := newIaCProvider(si) - _, err := p.ResolveSizing("infra.database", interfaces.SizeXL, nil) - if err == nil { - t.Fatal("expected error") - } -} - -// ── EnumerateAll (interfaces.EnumeratorAll) ───────────────────────────────── -// -// Bridged in v0.27.1 to close the audit-keys gap: remoteIaCProvider missed -// EnumerateAll, so `wfctl infra audit-keys` errored "no loaded provider -// implements EnumeratorAll" even when the plugin process implemented it. - -func TestRemoteIaCProvider_EnumerateAll(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "outputs": []any{ - map[string]any{ - "name": "key-1", - "type": "infra.spaces_key", - "provider_id": "AKID0000000000000001", - "status": "active", - "outputs": map[string]any{"created_at": "2026-05-01T00:00:00Z"}, - }, - map[string]any{ - "name": "key-2", - "type": "infra.spaces_key", - "provider_id": "AKID0000000000000002", - "status": "active", - "outputs": map[string]any{"created_at": "2026-05-08T00:00:00Z"}, - }, - }, - }} - p := newIaCProvider(si) - - outs, err := p.EnumerateAll(context.Background(), "infra.spaces_key") - if err != nil { - t.Fatalf("EnumerateAll: unexpected error: %v", err) - } - if si.method != "IaCProvider.EnumerateAll" { - t.Errorf("method: got %q, want IaCProvider.EnumerateAll", si.method) - } - if si.args["resource_type"] != "infra.spaces_key" { - t.Errorf("args[resource_type]: got %v, want infra.spaces_key", si.args["resource_type"]) - } - if len(outs) != 2 { - t.Fatalf("outs: got %d, want 2", len(outs)) - } - if outs[0].Name != "key-1" { - t.Errorf("outs[0].Name: got %q, want key-1", outs[0].Name) - } - if outs[1].ProviderID != "AKID0000000000000002" { - t.Errorf("outs[1].ProviderID: got %q, want AKID0000000000000002", outs[1].ProviderID) - } - createdAt, _ := outs[0].Outputs["created_at"].(string) - if createdAt != "2026-05-01T00:00:00Z" { - t.Errorf("outs[0].Outputs[created_at]: got %q", createdAt) - } -} - -func TestRemoteIaCProvider_EnumerateAll_NilResponse(t *testing.T) { - // Plugins are allowed to return an empty result for an empty account; the - // proxy must not crash when "outputs" is missing or response is nil. - si := &stubInvoker{resp: nil} - p := newIaCProvider(si) - - outs, err := p.EnumerateAll(context.Background(), "infra.spaces_key") - if err != nil { - t.Fatalf("EnumerateAll: unexpected error: %v", err) - } - if len(outs) != 0 { - t.Errorf("outs: got %d, want 0", len(outs)) - } -} - -func TestRemoteIaCProvider_EnumerateAll_PropagatesError(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("upstream listing failed")} - p := newIaCProvider(si) - - _, err := p.EnumerateAll(context.Background(), "infra.spaces_key") - if err == nil { - t.Fatal("expected error from EnumerateAll when invoker fails") - } - if !strings.Contains(err.Error(), "IaCProvider.EnumerateAll") { - t.Errorf("error should include method name, got: %v", err) - } -} - -// TestRemoteIaCProvider_EnumerateAll_TranslatesUnimplemented verifies that -// gRPC codes.Unimplemented from the plugin's InvokeMethod dispatcher is -// translated to interfaces.ErrProviderMethodUnimplemented so dispatch sites -// can errors.Is on the sentinel and skip non-implementing providers. This -// preserves the pre-v0.27.1 iterate-and-skip semantics now that every -// gRPC-loaded provider satisfies interfaces.EnumeratorAll at the type level -// (per Copilot review feedback on PR #589 round 1). -func TestRemoteIaCProvider_EnumerateAll_TranslatesUnimplemented(t *testing.T) { - cases := []struct { - name string - err error - }{ - { - name: "grpc_codes_unimplemented", - err: status.Error(codes.Unimplemented, "method not implemented"), - }, - { - name: "string_unimplemented", - err: fmt.Errorf("provider does not support EnumerateAll: unimplemented"), - }, - { - name: "string_not_implemented", - err: fmt.Errorf("method EnumerateAll not implemented by this plugin"), - }, - { - name: "string_does_not_implement_serviceinvoker", - err: fmt.Errorf("module handle abc does not implement ServiceInvoker"), - }, - } - for _, tc := range cases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - si := &stubInvoker{err: tc.err} - p := newIaCProvider(si) - - _, err := p.EnumerateAll(context.Background(), "infra.spaces_key") - if err == nil { - t.Fatal("expected error from EnumerateAll") - } - if !errors.Is(err, interfaces.ErrProviderMethodUnimplemented) { - t.Errorf("err = %v; want errors.Is(ErrProviderMethodUnimplemented) = true", err) - } - }) - } -} - -// TestRemoteIaCProvider_EnumerateByTag_TranslatesUnimplemented mirrors the -// EnumerateAll Unimplemented-translation test for the Enumerator bridge. -func TestRemoteIaCProvider_EnumerateByTag_TranslatesUnimplemented(t *testing.T) { - si := &stubInvoker{err: status.Error(codes.Unimplemented, "tag query unsupported")} - p := newIaCProvider(si) - - _, err := p.EnumerateByTag(context.Background(), "any") - if err == nil { - t.Fatal("expected error from EnumerateByTag") - } - if !errors.Is(err, interfaces.ErrProviderMethodUnimplemented) { - t.Errorf("err = %v; want errors.Is(ErrProviderMethodUnimplemented) = true", err) - } -} - -// TestRemoteIaCProvider_ValidatePlan_HappyPath verifies that the v0.27.1 -// ProviderValidator bridge dispatches "IaCProvider.ValidatePlan" and decodes -// the result["diagnostics"] entry as []PlanDiagnostic. -func TestRemoteIaCProvider_ValidatePlan_HappyPath(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "diagnostics": []any{ - map[string]any{ - "severity": float64(interfaces.PlanDiagnosticError), - "resource": "db-1", - "field": "vpc_ref", - "message": "vpc_ref points to an unknown VPC", - }, - }, - }} - p := newIaCProvider(si) - - diags := p.ValidatePlan(&interfaces.IaCPlan{}) - if si.method != "IaCProvider.ValidatePlan" { - t.Errorf("method: got %q, want IaCProvider.ValidatePlan", si.method) - } - if _, ok := si.args["plan"]; !ok { - t.Errorf("plan arg missing; got keys: %v", mapKeys(si.args)) - } - if len(diags) != 1 { - t.Fatalf("diags: got %d, want 1", len(diags)) - } - if diags[0].Severity != interfaces.PlanDiagnosticError { - t.Errorf("Severity: got %v, want PlanDiagnosticError", diags[0].Severity) - } - if diags[0].Field != "vpc_ref" { - t.Errorf("Field: got %q", diags[0].Field) - } -} - -// TestRemoteIaCProvider_ValidatePlan_SilentOnError verifies that the -// ProviderValidator bridge silently returns nil on error, preserving the -// pre-v0.27.1 R-A10 behavior where plugins that don't implement ValidatePlan -// are skipped (no diagnostics surface). The contract has no error channel, -// so this is the architecturally correct trade-off. -func TestRemoteIaCProvider_ValidatePlan_SilentOnError(t *testing.T) { - si := &stubInvoker{err: status.Error(codes.Unimplemented, "plugin does not implement ValidatePlan")} - p := newIaCProvider(si) - - diags := p.ValidatePlan(&interfaces.IaCPlan{}) - if diags != nil { - t.Errorf("ValidatePlan should return nil on Unimplemented; got %v", diags) - } -} - -func TestRemoteIaCProvider_EnumerateAll_DecodeError(t *testing.T) { - // outputs as a non-array value (string) cannot be decoded into - // []*ResourceOutput. The proxy must wrap the error with method context. - si := &stubInvoker{resp: map[string]any{ - "outputs": "not-an-array", - }} - p := newIaCProvider(si) - - _, err := p.EnumerateAll(context.Background(), "infra.spaces_key") - if err == nil { - t.Fatal("expected decode error") - } - if !strings.Contains(err.Error(), "IaCProvider.EnumerateAll: decode result") { - t.Fatalf("error %q missing decode context", err) - } -} - -func TestRemoteIaCProvider_EnumerateAll_UsesContext(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() // pre-cancel - ci := &contextRecordingInvoker{resp: map[string]any{}} - p := &remoteIaCProvider{invoker: ci} - - _, err := p.EnumerateAll(ctx, "infra.spaces_key") - if err == nil { - t.Fatal("expected canceled context error") - } - if !errors.Is(err, context.Canceled) { - t.Fatalf("error = %v, want context.Canceled", err) - } - if !ci.usedContext { - t.Fatal("EnumerateAll did not use context-aware invoker") - } - if ci.fallbackUsed { - t.Fatal("EnumerateAll used context-free fallback when context invoker available") - } -} - -// ── EnumerateByTag (interfaces.Enumerator) ─────────────────────────────────── -// -// Bridged in v0.27.1 alongside EnumerateAll. Same root gap: the optional -// Enumerator interface had no proxy method, so any caller that -// type-asserted a remote provider against interfaces.Enumerator silently -// fell through to the negative branch. - -func TestRemoteIaCProvider_EnumerateByTag(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "refs": []any{ - map[string]any{ - "name": "vpc-1", - "type": "infra.vpc", - "provider_id": "vpc-aaaa", - }, - map[string]any{ - "name": "vpc-2", - "type": "infra.vpc", - "provider_id": "vpc-bbbb", - }, - }, - }} - p := newIaCProvider(si) - - refs, err := p.EnumerateByTag(context.Background(), "wfctl-managed:env=staging") - if err != nil { - t.Fatalf("EnumerateByTag: unexpected error: %v", err) - } - if si.method != "IaCProvider.EnumerateByTag" { - t.Errorf("method: got %q, want IaCProvider.EnumerateByTag", si.method) - } - if si.args["tag"] != "wfctl-managed:env=staging" { - t.Errorf("args[tag]: got %v", si.args["tag"]) - } - if len(refs) != 2 { - t.Fatalf("refs: got %d, want 2", len(refs)) - } - if refs[0].Name != "vpc-1" || refs[0].ProviderID != "vpc-aaaa" { - t.Errorf("refs[0]: got %+v", refs[0]) - } -} - -func TestRemoteIaCProvider_EnumerateByTag_PropagatesError(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("tag query unsupported")} - p := newIaCProvider(si) - - _, err := p.EnumerateByTag(context.Background(), "any") - if err == nil { - t.Fatal("expected error from EnumerateByTag when invoker fails") - } - if !strings.Contains(err.Error(), "IaCProvider.EnumerateByTag") { - t.Errorf("error should include method name, got: %v", err) - } -} - -func TestRemoteIaCProvider_EnumerateByTag_UsesContext(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() - ci := &contextRecordingInvoker{resp: map[string]any{}} - p := &remoteIaCProvider{invoker: ci} - - _, err := p.EnumerateByTag(ctx, "any") - if err == nil { - t.Fatal("expected canceled context error") - } - if !errors.Is(err, context.Canceled) { - t.Fatalf("error = %v, want context.Canceled", err) - } - if !ci.usedContext { - t.Fatal("EnumerateByTag did not use context-aware invoker") - } - if ci.fallbackUsed { - t.Fatal("EnumerateByTag used context-free fallback when context invoker available") - } -} - -// ── Migration Repair ──────────────────────────────────────────────────────── - -func TestRemoteIaCProvider_RepairDirtyMigration(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "provider_job_id": "job-123", - "status": interfaces.MigrationRepairStatusSucceeded, - "applied": []any{"20260426000006"}, - "logs": "repair complete", - }} - p := newIaCProvider(si) - - result, err := p.RepairDirtyMigration(context.Background(), interfaces.MigrationRepairRequest{ - AppResourceName: "bmw-app", - DatabaseResourceName: "bmw-db", - JobImage: "registry.example/workflow-migrate:sha", - SourceDir: "/migrations", - ExpectedDirtyVersion: "20260426000005", - ForceVersion: "20260426000004", - ThenUp: true, - ConfirmForce: interfaces.MigrationRepairConfirmation, - Env: map[string]string{"DATABASE_URL": "postgres://example"}, - TimeoutSeconds: 600, - }) - if err != nil { - t.Fatalf("RepairDirtyMigration: unexpected error: %v", err) - } - if si.method != "IaCProvider.RepairDirtyMigration" { - t.Errorf("method: got %q, want IaCProvider.RepairDirtyMigration", si.method) - } - request, ok := si.args["request"].(map[string]any) - if !ok { - t.Fatalf("request arg: got %T, want map[string]any", si.args["request"]) - } - if request["expected_dirty_version"] != "20260426000005" { - t.Errorf("expected_dirty_version arg: got %v", request["expected_dirty_version"]) - } - if result.ProviderJobID != "job-123" { - t.Errorf("ProviderJobID: got %q", result.ProviderJobID) - } - if result.Status != interfaces.MigrationRepairStatusSucceeded { - t.Errorf("Status: got %q", result.Status) - } - if len(result.Applied) != 1 || result.Applied[0] != "20260426000006" { - t.Errorf("Applied: got %v", result.Applied) - } -} - -func TestRemoteIaCProvider_RepairDirtyMigration_DecodeError(t *testing.T) { - si := &stubInvoker{resp: map[string]any{ - "status": 123, - }} - p := newIaCProvider(si) - - _, err := p.RepairDirtyMigration(context.Background(), interfaces.MigrationRepairRequest{}) - if err == nil { - t.Fatal("expected decode error") - } - if !strings.Contains(err.Error(), "IaCProvider.RepairDirtyMigration: decode result") { - t.Fatalf("error %q missing decode context", err) - } -} - -func TestRemoteIaCProvider_RepairDirtyMigration_UsesContext(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() - ci := &contextRecordingInvoker{resp: map[string]any{ - "status": interfaces.MigrationRepairStatusSucceeded, - }} - p := &remoteIaCProvider{invoker: ci} - - _, err := p.RepairDirtyMigration(ctx, interfaces.MigrationRepairRequest{}) - if err == nil { - t.Fatal("expected canceled context error") - } - if !errors.Is(err, context.Canceled) { - t.Fatalf("error = %v, want context.Canceled", err) - } - if !ci.usedContext { - t.Fatal("RepairDirtyMigration did not use context-aware invoker") - } - if ci.fallbackUsed { - t.Fatal("RepairDirtyMigration used context-free fallback") - } -} - -// ── RevokeProviderCredential ───────────────────────────────────────────────── - -func TestRemoteIaCProvider_RevokeProviderCredential(t *testing.T) { - si := &stubInvoker{resp: map[string]any{}} - p := newIaCProvider(si) - - err := p.RevokeProviderCredential(context.Background(), "digitalocean.spaces", "AKID123") - if err != nil { - t.Fatalf("RevokeProviderCredential: unexpected error: %v", err) - } - if si.method != "IaCProvider.RevokeProviderCredential" { - t.Errorf("method: got %q, want IaCProvider.RevokeProviderCredential", si.method) - } - if si.args["source"] != "digitalocean.spaces" { - t.Errorf("args[source]: got %q, want digitalocean.spaces", si.args["source"]) - } - if si.args["credential_id"] != "AKID123" { - t.Errorf("args[credential_id]: got %q, want AKID123", si.args["credential_id"]) - } -} - -func TestRemoteIaCProvider_RevokeProviderCredential_PropagatesError(t *testing.T) { - si := &stubInvoker{err: fmt.Errorf("upstream revoke failed")} - p := newIaCProvider(si) - - err := p.RevokeProviderCredential(context.Background(), "digitalocean.spaces", "AKID_FAIL") - if err == nil { - t.Fatal("expected error from RevokeProviderCredential when invoker fails") - } - if !strings.Contains(err.Error(), "IaCProvider.RevokeProviderCredential") { - t.Errorf("error message should include method name, got: %v", err) - } -} - -func TestRemoteIaCProvider_RevokeProviderCredential_UsesContext(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() // pre-cancel - - ci := &contextRecordingInvoker{resp: map[string]any{}} - p := &remoteIaCProvider{invoker: ci} - - err := p.RevokeProviderCredential(ctx, "digitalocean.spaces", "AKID_CTX") - if !errors.Is(err, context.Canceled) { - t.Fatalf("error = %v, want context.Canceled", err) - } - if !ci.usedContext { - t.Fatal("RevokeProviderCredential did not use context-aware invoker") - } - if ci.fallbackUsed { - t.Fatal("RevokeProviderCredential used context-free fallback") - } -} - -// ── ProviderCredentialRevoker interface satisfaction ───────────────────────── - -// TestRemoteIaCProvider_ImplementsProviderCredentialRevoker asserts that -// *remoteIaCProvider satisfies interfaces.ProviderCredentialRevoker at compile -// time. The type assertion below will cause a compile error if the method -// signature doesn't match — this is the compile-time contract check that was -// deferred from the initial ADR 0012 implementation. -func TestRemoteIaCProvider_ImplementsProviderCredentialRevoker(t *testing.T) { - p := &remoteIaCProvider{} - var _ interfaces.ProviderCredentialRevoker = p // compile-time assertion - t.Log("remoteIaCProvider satisfies interfaces.ProviderCredentialRevoker") -} - -type contextRecordingInvoker struct { - resp map[string]any - usedContext bool - fallbackUsed bool -} - -func (c *contextRecordingInvoker) InvokeService(_ string, _ map[string]any) (map[string]any, error) { - c.fallbackUsed = true - return c.resp, nil -} - -func (c *contextRecordingInvoker) InvokeServiceContext(ctx context.Context, _ string, _ map[string]any) (map[string]any, error) { - c.usedContext = true - if err := ctx.Err(); err != nil { - return nil, err - } - return c.resp, nil -} diff --git a/cmd/wfctl/deploy_providers_strict_bridge_coverage_test.go b/cmd/wfctl/deploy_providers_strict_bridge_coverage_test.go deleted file mode 100644 index b7f1cb32..00000000 --- a/cmd/wfctl/deploy_providers_strict_bridge_coverage_test.go +++ /dev/null @@ -1,302 +0,0 @@ -package main - -// deploy_providers_strict_bridge_coverage_test.go — strict-contracts coverage -// gate for the wfctl-side gRPC IaCProvider proxy. -// -// Why this exists -// ─────────────── -// The v0.27.0 → v0.27.1 audit-keys bug demonstrated a class of cross-repo -// gRPC bridge gaps that the existing strict-contracts gate does NOT catch: -// -// 1. interfaces/iac_provider.go declares an OPTIONAL sub-interface -// (EnumeratorAll, Enumerator, DriftConfigDetector, ...). -// 2. Plugin-side providers (e.g. workflow-plugin-digitalocean's DOProvider) -// implement it AND advertise the method via their InvokeMethod / -// InvokeMethodContext dispatcher. -// 3. The wfctl-side proxy `*remoteIaCProvider` lives in -// cmd/wfctl/deploy_providers.go and routes ALL plugin calls through -// InvokeService. If the proxy is missing a method, every type-assert -// against the optional interface in wfctl call-sites fails — the -// plugin process implements the method, but wfctl can never reach it. -// -// The pre-existing strict-contracts gate (cmd/wfctl/plugin_audit.go) audits -// PLUGIN-SIDE manifest contract descriptors. It has no visibility into -// workflow-side proxy method coverage. The v0.27.0 EnumeratorAll gap slipped -// through with green CI because no test enforced "every interface method -// declared in interfaces/iac_provider.go has a corresponding bridge entry". -// -// What this test enforces -// ─────────────────────── -// F-1 (compile-time): `*remoteIaCProvider` satisfies EVERY optional -// IaCProvider sub-interface declared in interfaces/iac_provider.go. -// Adding a new optional interface without bridging will fail at -// compile time once the new interface is added to opt-in list below. -// -// F-2 (wire coverage): for every method on every optional interface, -// deploy_providers.go contains the literal string "IaCProvider." -// — the RPC method name passed to InvokeService / InvokeServiceContext. -// This catches the case where someone declares the proxy method but -// forgets to actually dispatch through gRPC (or uses a typo in the -// method-name string). -// -// F-3 (no skip / fallback): this test is unconditional. It does not -// respect a -short flag, an env var, or a build tag. There is no -// path that downgrades it to non-strict. Per the v0.27.1 user -// mandate: "remove the fallback and force strict mode". -// -// How to extend -// ───────────── -// When adding a new optional IaCProvider sub-interface (e.g. ProviderXyz): -// 1. Declare it in interfaces/iac_provider.go (or a sibling file). -// 2. Add `_ interfaces.ProviderXyz = (*remoteIaCProvider)(nil)` to -// the compile-time block below. -// 3. Append the interface's reflect.TypeOf entry to optionalIaCProviderInterfaces. -// 4. Add proxy methods to remoteIaCProvider in deploy_providers.go. -// 5. The wire-coverage test will fail until step 4 is complete; that -// failure is the bridge gap surfacing immediately. -// -// Run: go test ./cmd/wfctl/ -run TestStrictBridgeCoverage -v - -import ( - "os" - "path/filepath" - "reflect" - "runtime" - "strings" - "testing" - - "github.com/GoCodeAlone/workflow/interfaces" -) - -// Compile-time assertions that *remoteIaCProvider satisfies every optional -// IaCProvider sub-interface that wfctl call-sites type-assert against. -// -// If a future commit adds a new optional interface without bridging it, -// adding the assertion line below will fail to compile until the proxy -// implements the methods — surfacing the gap at PR-review time, not at -// runtime in production. -var ( - _ interfaces.IaCProvider = (*remoteIaCProvider)(nil) - _ interfaces.Enumerator = (*remoteIaCProvider)(nil) // v0.27.1 - _ interfaces.EnumeratorAll = (*remoteIaCProvider)(nil) // v0.27.1 - _ interfaces.DriftConfigDetector = (*remoteIaCProvider)(nil) // v0.20.x - _ interfaces.ProviderMigrationRepairer = (*remoteIaCProvider)(nil) // v0.21.x - _ interfaces.ProviderCredentialRevoker = (*remoteIaCProvider)(nil) // v0.22.x - _ interfaces.ProviderValidator = (*remoteIaCProvider)(nil) // v0.27.1 (R-A10 bridge) -) - -// optionalIaCProviderInterfaces is the canonical registry of every optional -// sub-interface that wfctl call-sites type-assert IaC providers against. -// The wire-coverage test below iterates this set and demands each declared -// method appear as an "IaCProvider." string literal in -// deploy_providers.go. -// -// Add new optional interfaces here whenever they're declared in -// interfaces/iac_provider.go (or sibling files) AND a wfctl call-site -// type-asserts against them. -var optionalIaCProviderInterfaces = []reflect.Type{ - reflect.TypeOf((*interfaces.IaCProvider)(nil)).Elem(), - reflect.TypeOf((*interfaces.Enumerator)(nil)).Elem(), - reflect.TypeOf((*interfaces.EnumeratorAll)(nil)).Elem(), - reflect.TypeOf((*interfaces.DriftConfigDetector)(nil)).Elem(), - reflect.TypeOf((*interfaces.ProviderMigrationRepairer)(nil)).Elem(), - reflect.TypeOf((*interfaces.ProviderCredentialRevoker)(nil)).Elem(), - reflect.TypeOf((*interfaces.ProviderValidator)(nil)).Elem(), -} - -// methodsExemptFromWireCoverage are interface methods that legitimately do -// NOT correspond to a wire RPC. Specifically: -// - Name / Version: trivial accessors that route through InvokeService but -// also have other concerns (return zero on error rather than propagate). -// - Close: local resource cleanup; never crosses the gRPC boundary. -// - SupportedCanonicalKeys: returns a wfctl-side canonical-keys enum, -// not a remote call (per comment at deploy_providers.go). -// - ResourceDriver: returns a sub-driver proxy (remoteResourceDriver) — -// the actual RPC dispatch happens on the returned driver, not here. -// - DetectDriftWithSpecs: routes through "IaCProvider.DetectDrift" with a -// "specs" arg per DO plugin v0.10.5+ wire protocol — same RPC as -// DetectDrift, no separate method name. -// -// This list is intentionally small. Adding entries here REQUIRES a comment -// justifying why the method does not need wire-coverage; the test fails -// loudly on unrecognised exemptions. -var methodsExemptFromWireCoverage = map[string]string{ - "Name": "trivial accessor; wire shape covered by other tests", - "Version": "trivial accessor; wire shape covered by other tests", - "Close": "local cleanup, no gRPC call", - "SupportedCanonicalKeys": "returns wfctl-side canonical-keys enum, no RPC", - "ResourceDriver": "returns remoteResourceDriver sub-proxy; per-method RPC happens there", - "DetectDriftWithSpecs": "routes through IaCProvider.DetectDrift with 'specs' arg per DO v0.10.5+ wire protocol", -} - -// TestStrictBridgeCoverage_CompileTimeAssertions documents the compile-time -// guarantees enforced at file-init time (the var block above). The test body -// is a no-op marker: if the var block had compiled-out a missing interface, -// the package would not have compiled and this test could not run. -func TestStrictBridgeCoverage_CompileTimeAssertions(t *testing.T) { - // Intentionally empty: the value is in the var block above. Documenting - // the guarantee here gives `go test -run TestStrictBridgeCoverage` a hit. - t.Log("compile-time interface-satisfaction assertions checked at package-load time") -} - -// TestStrictBridgeCoverage_WireMethodCoverage iterates every method on every -// optional IaCProvider sub-interface and demands the literal string -// "IaCProvider." appear in deploy_providers.go (the file that -// implements the wfctl-side proxy). Methods listed in -// methodsExemptFromWireCoverage are skipped. -// -// This is a coarse but effective test: a missing proxy method, a typo in -// the RPC method-name string, or a "we'll wire it later" placeholder all -// fail this gate. -func TestStrictBridgeCoverage_WireMethodCoverage(t *testing.T) { - src := readDeployProvidersSource(t) - - for _, iface := range optionalIaCProviderInterfaces { - ifaceName := iface.Name() - for i := 0; i < iface.NumMethod(); i++ { - method := iface.Method(i) - if reason, exempt := methodsExemptFromWireCoverage[method.Name]; exempt { - t.Logf("[%s.%s] exempt: %s", ifaceName, method.Name, reason) - continue - } - wantToken := "IaCProvider." + method.Name - if !strings.Contains(src, wantToken) { - t.Errorf("strict-bridge-coverage: interface %s declares method %s "+ - "but deploy_providers.go has no occurrence of %q. "+ - "Either add the proxy method to remoteIaCProvider that calls "+ - "InvokeService(%q, ...) or, if the method legitimately does "+ - "not require wire dispatch, document it in "+ - "methodsExemptFromWireCoverage with justification.", - ifaceName, method.Name, wantToken, wantToken) - } - } - } -} - -// TestStrictBridgeCoverage_NoFallbackOrSkip is a meta-check that the gate -// methods (TestStrictBridgeCoverage_CompileTimeAssertions and -// TestStrictBridgeCoverage_WireMethodCoverage) cannot be silently downgraded -// by an env var, build constraint, or testing.Short skip. Per the v0.27.1 -// user mandate ("remove the fallback and force strict mode"), the strict- -// bridge-coverage test must always run unconditionally. -// -// Two-layer enforcement -// ───────────────────── -// 1. File-header scan: reject build constraints (//go:build or // +build) -// anywhere in the file header — these can disable the entire test file -// via tag, which the function-body scan can't see. -// 2. Function-body scan: reject t.Skip / t.SkipNow / testing.Short() / -// os.Getenv inside either gate function body. This deliberately excludes -// this meta-check's own body, where the banned tokens appear as data. -// -// A future commit that adds `t.Skip(...)` inside either gate body, or -// `//go:build !strict` at the file header, will fail this check. -func TestStrictBridgeCoverage_NoFallbackOrSkip(t *testing.T) { - src := readThisFile(t) - - // Layer 1: file-header build-constraint scan. Build constraints in Go - // must appear before the package clause, so we scan everything above - // the `package main` line. This catches both old-style `// +build` and - // modern `//go:build` directives regardless of what's inside any test - // function body. The package clause may appear at offset 0 (no leading - // build constraint) or later (with constraints above it). - pkgIdx := strings.Index(src, "package ") - if pkgIdx < 0 { - t.Fatal("could not locate package declaration in source") - } - header := src[:pkgIdx] - // Tokens assembled from fragments so the banned literal does not - // appear here as a single string. - if strings.Contains(header, "//"+"go:build") { - t.Errorf("strict-bridge-coverage file header must not declare a //go:build constraint "+ - "— the gate is unconditional per v0.27.1 user mandate. Header was:\n%s", header) - } - if strings.Contains(header, "// +"+"build") { - t.Errorf("strict-bridge-coverage file header must not declare a // +build constraint "+ - "— the gate is unconditional per v0.27.1 user mandate. Header was:\n%s", header) - } - - // Layer 2: function-body scan for runtime bypass tokens. - gateBodies := []string{ - extractFunctionBody(t, src, "TestStrictBridgeCoverage_CompileTimeAssertions"), - extractFunctionBody(t, src, "TestStrictBridgeCoverage_WireMethodCoverage"), - } - banned := []string{ - "t" + ".Skip(", - "t" + ".SkipNow(", - "testing" + ".Short()", - "os" + ".Getenv", - } - for _, body := range gateBodies { - for _, b := range banned { - if strings.Contains(body, b) { - t.Errorf("strict-bridge-coverage gate body must not contain %q "+ - "— the gate is unconditional per v0.27.1 user mandate "+ - "(\"remove the fallback and force strict mode\")", b) - } - } - } -} - -// extractFunctionBody returns the body of the named test function. Used by -// the no-fallback meta-check to inspect ONLY the gate bodies, not its own. -// The implementation is a best-effort substring scan — it locates -// `func (` then balances braces from the first `{` after that point. -func extractFunctionBody(t *testing.T, src string, name string) string { - t.Helper() - marker := "func " + name + "(" - start := strings.Index(src, marker) - if start < 0 { - t.Fatalf("could not locate %s in source", name) - } - open := strings.Index(src[start:], "{") - if open < 0 { - t.Fatalf("could not find open brace for %s", name) - } - open += start - depth := 0 - for i := open; i < len(src); i++ { - switch src[i] { - case '{': - depth++ - case '}': - depth-- - if depth == 0 { - return src[open : i+1] - } - } - } - t.Fatalf("unbalanced braces in %s", name) - return "" -} - -// readDeployProvidersSource reads cmd/wfctl/deploy_providers.go from the -// repo root, locating it relative to this test file's path so the test -// works regardless of `go test` working directory. -func readDeployProvidersSource(t *testing.T) string { - t.Helper() - _, thisFile, _, ok := runtime.Caller(0) - if !ok { - t.Fatal("runtime.Caller failed") - } - target := filepath.Join(filepath.Dir(thisFile), "deploy_providers.go") - data, err := os.ReadFile(target) - if err != nil { - t.Fatalf("read deploy_providers.go: %v", err) - } - return string(data) -} - -// readThisFile reads the test file itself for the no-skip meta-check. -func readThisFile(t *testing.T) string { - t.Helper() - _, thisFile, _, ok := runtime.Caller(0) - if !ok { - t.Fatal("runtime.Caller failed") - } - data, err := os.ReadFile(thisFile) - if err != nil { - t.Fatalf("read self: %v", err) - } - return string(data) -} diff --git a/cmd/wfctl/deploy_providers_test.go b/cmd/wfctl/deploy_providers_test.go index 88f9aec0..3e37b9c3 100644 --- a/cmd/wfctl/deploy_providers_test.go +++ b/cmd/wfctl/deploy_providers_test.go @@ -2,16 +2,11 @@ package main import ( "context" - "errors" - "io" "os" "strings" "testing" - "github.com/GoCodeAlone/modular" "github.com/GoCodeAlone/workflow/config" - "github.com/GoCodeAlone/workflow/plugin" - "github.com/GoCodeAlone/workflow/plugin/external" ) // noopCloser is an io.Closer that does nothing — used in test stubs. @@ -20,33 +15,15 @@ type noopCloser struct{} func (noopCloser) Close() error { return nil } // ── discoverAndLoadIaCProvider — error propagation ──────────────────────────── - -// TestResolveIaCProviderSurfacesPluginError is the regression gate for the -// caller-side fix: discoverAndLoadIaCProvider must surface the error message -// from an *errorModule returned by the factory, not fall through to a generic -// "does not support service invocation" message. -func TestResolveIaCProviderSurfacesPluginError(t *testing.T) { - const pluginErrMsg = "digitalocean: missing required config key 'token'" - - oldLoader := loadIaCPlugin - loadIaCPlugin = func(_, _ string) (string, map[string]plugin.ModuleFactory, io.Closer, error) { - factory := func(name string, _ map[string]any) modular.Module { - return external.NewErrorModule(name, errors.New(pluginErrMsg)) - } - return "workflow-plugin-digitalocean", map[string]plugin.ModuleFactory{ - "iac.provider": factory, - }, noopCloser{}, nil - } - defer func() { loadIaCPlugin = oldLoader }() - - _, _, err := discoverAndLoadIaCProvider(context.Background(), "digitalocean", map[string]any{}) - if err == nil { - t.Fatal("expected error, got nil") - } - if !strings.Contains(err.Error(), pluginErrMsg) { - t.Errorf("expected plugin error message %q in returned error, got: %v", pluginErrMsg, err) - } -} +// +// Per plan §Task 16 (strict-contracts force-cutover), the legacy +// `loadIaCPlugin` factory-based load path is removed; discoverAndLoadIaCProvider +// now constructs typedIaCAdapter directly from the gRPC ClientConn + +// ContractRegistry. Plugin-error propagation is exercised end-to-end by the +// typed-RPC integration tests in iac_typed_adapter_test.go (Task 30) and +// the cross-plugin-build matrix in .github/workflows/cross-plugin-build-test.yml +// (Task 6). The legacy TestResolveIaCProviderSurfacesPluginError test is +// removed alongside the loadIaCPlugin var it depended on. // ── newDeployProvider ───────────────────────────────────────────────────────── diff --git a/cmd/wfctl/deploy_remote_provider_test.go b/cmd/wfctl/deploy_remote_provider_test.go deleted file mode 100644 index 6973e245..00000000 --- a/cmd/wfctl/deploy_remote_provider_test.go +++ /dev/null @@ -1,173 +0,0 @@ -package main - -import ( - "context" - "strings" - "testing" - - "github.com/GoCodeAlone/workflow/interfaces" -) - -// fakeRemoteInvoker implements remoteServiceInvoker using an in-memory dispatch -// table, so tests exercise remoteIaCProvider without a live plugin subprocess. -type fakeRemoteInvoker struct { - methods map[string]map[string]any // method → result - errors map[string]string // method → error string -} - -func (f *fakeRemoteInvoker) InvokeService(method string, _ map[string]any) (map[string]any, error) { - if errStr, ok := f.errors[method]; ok { - return nil, errString(errStr) - } - if res, ok := f.methods[method]; ok { - return res, nil - } - return map[string]any{}, nil -} - -type errString string - -func (e errString) Error() string { return string(e) } - -func newFakeInvoker() *fakeRemoteInvoker { - return &fakeRemoteInvoker{ - methods: map[string]map[string]any{ - "IaCProvider.Name": {"name": "test-provider"}, - "IaCProvider.Version": {"version": "1.0.0"}, - "IaCProvider.Initialize": {}, - "ResourceDriver.Update": { - "provider_id": "app-123", - "status": "running", - }, - "ResourceDriver.HealthCheck": { - "healthy": true, - "message": "", - }, - }, - errors: map[string]string{}, - } -} - -// ── remoteIaCProvider ───────────────────────────────────────────────────────── - -func TestRemoteIaCProvider_Name(t *testing.T) { - p := &remoteIaCProvider{invoker: newFakeInvoker()} - if got := p.Name(); got != "test-provider" { - t.Errorf("Name() = %q, want %q", got, "test-provider") - } -} - -func TestRemoteIaCProvider_Initialize_RoutesViaInvoker(t *testing.T) { - inv := newFakeInvoker() - p := &remoteIaCProvider{invoker: inv} - if err := p.Initialize(context.Background(), map[string]any{"token": "x"}); err != nil { - t.Fatalf("Initialize: %v", err) - } -} - -func TestRemoteIaCProvider_Initialize_PropagatesError(t *testing.T) { - inv := newFakeInvoker() - inv.errors["IaCProvider.Initialize"] = "invalid token" - p := &remoteIaCProvider{invoker: inv} - err := p.Initialize(context.Background(), nil) - if err == nil { - t.Fatal("expected error") - } - if !strings.Contains(err.Error(), "invalid token") { - t.Errorf("expected 'invalid token' in error, got: %v", err) - } -} - -func TestRemoteIaCProvider_ResourceDriver_ReturnsRemoteDriver(t *testing.T) { - p := &remoteIaCProvider{invoker: newFakeInvoker()} - drv, err := p.ResourceDriver("infra.container_service") - if err != nil { - t.Fatalf("ResourceDriver: %v", err) - } - if _, ok := drv.(*remoteResourceDriver); !ok { - t.Fatalf("expected *remoteResourceDriver, got %T", drv) - } -} - -// ── remoteResourceDriver ────────────────────────────────────────────────────── - -func TestRemoteResourceDriver_Update_RoutesViaInvoker(t *testing.T) { - drv := &remoteResourceDriver{ - invoker: newFakeInvoker(), - resourceType: "infra.container_service", - } - ref := interfaces.ResourceRef{Name: "bmw-app", Type: "infra.container_service"} - spec := interfaces.ResourceSpec{ - Name: "bmw-app", - Type: "infra.container_service", - Config: map[string]any{"image": "registry.example.com/bmw:v2"}, - } - out, err := drv.Update(context.Background(), ref, spec) - if err != nil { - t.Fatalf("Update: %v", err) - } - if out.ProviderID != "app-123" { - t.Errorf("ProviderID = %q, want %q", out.ProviderID, "app-123") - } -} - -func TestRemoteResourceDriver_HealthCheck_Healthy(t *testing.T) { - drv := &remoteResourceDriver{ - invoker: newFakeInvoker(), - resourceType: "infra.container_service", - } - ref := interfaces.ResourceRef{Name: "bmw-app", Type: "infra.container_service"} - result, err := drv.HealthCheck(context.Background(), ref) - if err != nil { - t.Fatalf("HealthCheck: %v", err) - } - if !result.Healthy { - t.Error("expected Healthy=true") - } -} - -func TestRemoteResourceDriver_HealthCheck_Unhealthy(t *testing.T) { - inv := newFakeInvoker() - inv.methods["ResourceDriver.HealthCheck"] = map[string]any{ - "healthy": false, - "message": "app is degraded", - } - drv := &remoteResourceDriver{ - invoker: inv, - resourceType: "infra.container_service", - } - ref := interfaces.ResourceRef{Name: "bmw-app", Type: "infra.container_service"} - result, err := drv.HealthCheck(context.Background(), ref) - if err != nil { - t.Fatalf("HealthCheck: %v", err) - } - if result.Healthy { - t.Error("expected Healthy=false") - } - if result.Message != "app is degraded" { - t.Errorf("Message = %q, want %q", result.Message, "app is degraded") - } -} - -func TestRemoteResourceDriver_Update_PropagatesError(t *testing.T) { - inv := newFakeInvoker() - inv.errors["ResourceDriver.Update"] = "deployment failed" - drv := &remoteResourceDriver{ - invoker: inv, - resourceType: "infra.container_service", - } - _, err := drv.Update(context.Background(), interfaces.ResourceRef{}, interfaces.ResourceSpec{}) - if err == nil || !strings.Contains(err.Error(), "deployment failed") { - t.Errorf("expected 'deployment failed' error, got: %v", err) - } -} - -// TestDiscoverAndLoadIaCProvider_WrapsModuleAsRemoteIaCProvider verifies that -// when a plugin's iac.provider module does NOT directly implement IaCProvider -// (the normal case for gRPC plugins), discoverAndLoadIaCProvider wraps it in -// remoteIaCProvider instead of failing the type assertion. -func TestDiscoverAndLoadIaCProvider_WrapsModuleAsRemoteIaCProvider(t *testing.T) { - // This is covered end-to-end by the plugin tests; here we just confirm that - // remoteIaCProvider satisfies interfaces.IaCProvider at compile time. - var _ interfaces.IaCProvider = (*remoteIaCProvider)(nil) -} From b176057781370b92dcb164e3fa8ee55f77d63a26 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 10 May 2026 07:11:28 -0400 Subject: [PATCH 3/3] =?UTF-8?q?fix(wfctl):=20PR=20#609=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20Copilot=20+=20Step=201=20boundary=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review feedback on PR #609 (Task 16) and post-rebase consolidation after PR #605 merged. Addresses 4 of 6 Copilot findings + the IMPORTANT spec-reviewer Step 1 test gap. Fix #1 (Task 18 wiring) deferred until PR #610 merges (predicate not yet on main); Fix #5 (test file comment) already cleaned up post-rebase. Fix #6 (computePlanVersion stale return) documented in-place per team-lead recommendation (avoids 2-cycle churn when the follow-up capability-extension PR wires it back in). **Copilot fix #1 — `plugin/external/adapter.go:Conn()` doc broadened.** The previous doc claimed `nil` only for adapters constructed without a backing PluginClient (test fixtures). It can ALSO return nil when the PluginClient is non-nil but its underlying *grpc.ClientConn is nil (in-process test plumbing wiring only the PluginServiceClient interface without a real conn). Doc now enumerates both cases. **Copilot fix #3 — `cmd/wfctl/deploy_providers.go` surfaces ContractRegistryError.** The previous loader path called `registeredIaCServices(adapter.ContractRegistry())` without first checking `adapter.ContractRegistryError()`. A transport- level RPC failure (codes.Unimplemented from a legacy plugin, transient network reset, etc.) silently degraded to an empty registry, then the next `if !registered[iacServiceRequired]` branch fired the misleading "does not register the required service" error — masking the real cause. Now: surface ContractRegistryError() FIRST with `wfctl plugin update` hint; fall through to the registration-check only when the RPC succeeded. Test coverage: `TestDiscoverAndLoadIaCProvider_SurfacesContractRegistryError`. **Copilot fix #6 — `findIaCPluginDir.computePlanVersion` documented as reserved for follow-up.** discoverAndLoadIaCProvider no longer reads the value (the legacy reader `readIaCPluginComputePlanVersion` was deleted with remoteIaCProvider). Per team-lead: leave the return in place rather than churning the signature now; a follow-up PR adds `compute_plan_version` to `CapabilitiesResponse.IaCCapabilityDeclaration` (option (d), batched with canonical_keys between Task 17 and Task 20) and wires it back in via the typed Capabilities RPC. In-line comment documents the reservation + the follow-up plan. **IMPORTANT spec-reviewer Fix 2 — Step 1 boundary test added.** New file `cmd/wfctl/discover_typed_loader_test.go` extracts a unit- testable seam `buildTypedIaCAdapterFrom(adapter)` from the loader's post-LoadPlugin half (factored out of discoverAndLoadIaCProvider with the `iacAdapterAccessor` interface so tests don't pay the subprocess cost). Three boundary tests: - TestDiscoverAndLoadIaCProvider_ReturnsTypedClient — asserts the cutover invariant: loader returns `*typedIaCAdapter`, NOT the legacy `*remoteIaCProvider` (which no longer compiles post-cutover). In-process gRPC server with a stub IaCProviderRequiredServer + Initialize-only response. - TestDiscoverAndLoadIaCProvider_RejectsMissingRequiredService — asserts the strict-contracts hard-cutover invariant: plugins whose ContractRegistry omits `IaCProviderRequired` are rejected at load time with an actionable `wfctl plugin update` hint. Verifies message contract for operator UX. - TestDiscoverAndLoadIaCProvider_SurfacesContractRegistryError — asserts Copilot fix #3 above; transport-level ContractRegistry failure is surfaced via errors.Is + RPC-failure framing. **Test-file comment refresh** in deploy_providers_test.go cite of iac_typed_adapter_test.go updated post-rebase: file now lives on main via PR #605 (no longer "not present in this PR"). Also dropped the unused `noopCloser` helper that lint flagged after the legacy TestResolveIaCProviderSurfacesPluginError removal. **Cutover dependency status:** PR #605 (typed adapter) + PR #611 (sdk auto-register) MERGED to main; rebase clean. PR #610 (Task 18 loader gate) still pending — Fix 1 (replace inline gate with AssertIaCPluginAdvertisesRequiredService predicate) lands in a follow-up commit on this branch once PR #610 merges and I rebase again. Const naming aligned via impl-3's PR #610 rename (`iacServiceRequired` is canonical across both files). Local validation: GOWORK=off go build ./... → clean GOWORK=off go vet ./cmd/wfctl/ ./plugin/external/... → clean GOWORK=off go test ./cmd/wfctl/ -count=1 -short → all PASS (7.0s) GOWORK=off go test ./cmd/wfctl/ -run TestDiscoverAndLoadIaCProvider -count=1 → 3 PASS (1.5s) GOWORK=off golangci-lint run --enable=gocritic,gosec ./cmd/wfctl/... → 0 issues Co-Authored-By: Claude Opus 4.7 --- cmd/wfctl/deploy_providers.go | 73 +++++++--- cmd/wfctl/deploy_providers_test.go | 16 +-- cmd/wfctl/discover_typed_loader_test.go | 170 ++++++++++++++++++++++++ plugin/external/adapter.go | 14 +- 4 files changed, 244 insertions(+), 29 deletions(-) create mode 100644 cmd/wfctl/discover_typed_loader_test.go diff --git a/cmd/wfctl/deploy_providers.go b/cmd/wfctl/deploy_providers.go index 993db94d..7e274b49 100644 --- a/cmd/wfctl/deploy_providers.go +++ b/cmd/wfctl/deploy_providers.go @@ -21,6 +21,7 @@ import ( "github.com/GoCodeAlone/workflow/interfaces" "github.com/GoCodeAlone/workflow/plugin/external" pb "github.com/GoCodeAlone/workflow/plugin/external/proto" + "google.golang.org/grpc" ) // DeployConfig holds all parameters needed to execute a deployment. @@ -102,17 +103,21 @@ type iacPluginManifest struct { // the executable is present). // // The computePlanVersion return is the RAW value from the SDK manifest's -// iacProvider.computePlanVersion field. This loader path performs only -// minimal json.Unmarshal — no schema validation — so callers must NOT -// assume the returned string is constrained to {"", "v1", "v2"}. Use -// the wfctlhelpers.DispatchVersionV2 constant for the v2-equality check -// (anything else, including unknown values and empty, defaults to v1): +// iacProvider.computePlanVersion field. Currently UNUSED by +// discoverAndLoadIaCProvider after the strict-contracts force-cutover +// (the old caller `readIaCPluginComputePlanVersion` was deleted with +// remoteIaCProvider). Reserved for the follow-up +// CapabilitiesResponse.IaCCapabilityDeclaration.compute_plan_version +// capability-extension PR (option (d) per team-lead ruling, batched with +// canonical_keys between Task 17 and Task 20). Removing the return now +// would force a signature churn round when the follow-up wires it back +// in via a different reader (typed proto Capabilities RPC instead of +// plugin.json scan); kept in place to avoid that churn. // -// if computePlanVersion == wfctlhelpers.DispatchVersionV2 { ... v2 path ... } -// -// (wfctlhelpers.DispatchVersionFor takes a provider value, not a raw -// string, so it does not apply at this loader-level seam where only -// the string is in hand.) +// Until the follow-up: callers receive the value but discard it. The raw +// string is unconstrained — schema-validated values are {"", "v1", "v2"} +// per wfctlhelpers.DispatchVersionV2, but this loader path performs only +// minimal json.Unmarshal so MUST NOT assume. func findIaCPluginDir(pluginDir, providerName string) (name, computePlanVersion string, hasBinary bool, err error) { entries, err := os.ReadDir(pluginDir) if err != nil { @@ -183,24 +188,58 @@ func discoverAndLoadIaCProvider(ctx context.Context, providerName string, cfg ma } closer := closerFunc(func() error { mgr.Shutdown(); return nil }) + typed, err := buildTypedIaCAdapterFrom(ctx, providerName, pName, cfg, adapter) + if err != nil { + _ = closer.Close() + return nil, nil, err + } + return typed, closer, nil +} + +// iacAdapterAccessor is the slice of *external.ExternalPluginAdapter the +// typed-IaC loader needs after a successful LoadPlugin. Extracted as an +// interface so buildTypedIaCAdapterFrom is unit-testable against an +// in-process gRPC server without spawning a real plugin subprocess — +// the spec Step 1 boundary test (TestDiscoverAndLoadIaCProvider_ReturnsTypedClient) +// constructs a stub adapter satisfying this interface and verifies the +// returned interfaces.IaCProvider is *typedIaCAdapter. +type iacAdapterAccessor interface { + Conn() *grpc.ClientConn + ContractRegistry() *pb.ContractRegistry + ContractRegistryError() error +} + +// buildTypedIaCAdapterFrom is the post-LoadPlugin half of +// discoverAndLoadIaCProvider, factored out so it's unit-testable in +// isolation against an in-process gRPC server. Returns the typed +// IaCProvider on success, a typed error otherwise. Caller is +// responsible for closing the plugin manager on error. +func buildTypedIaCAdapterFrom(ctx context.Context, providerName, pName string, cfg map[string]any, adapter iacAdapterAccessor) (interfaces.IaCProvider, error) { conn := adapter.Conn() if conn == nil { - _ = closer.Close() - return nil, nil, fmt.Errorf("plugin %q does not expose a gRPC connection (host adapter missing PluginClient.Conn) — upgrade with: wfctl plugin update %s", pName, pName) + return nil, fmt.Errorf("plugin %q does not expose a gRPC connection (host adapter missing PluginClient.Conn) — upgrade with: wfctl plugin update %s", pName, pName) + } + + // Surface a ContractRegistry RPC failure FIRST. Without this guard, + // a transport / Unimplemented error against GetContractRegistry + // silently degrades to an empty registry, and the next + // `if !registered[iacServiceRequired]` branch fires the misleading + // "does not register the required service" message — masking the + // real cause. Per Copilot finding on PR #609. + if regErr := adapter.ContractRegistryError(); regErr != nil { + return nil, fmt.Errorf("plugin %q ContractRegistry RPC failed: %w — upgrade with: wfctl plugin update %s", pName, regErr, pName) } registered := registeredIaCServices(adapter.ContractRegistry()) if !registered[iacServiceRequired] { - _ = closer.Close() - return nil, nil, fmt.Errorf("plugin %q does not register the required %q gRPC service — upgrade with: wfctl plugin update %s", pName, iacServiceRequired, pName) + return nil, fmt.Errorf("plugin %q does not register the required %q gRPC service — upgrade with: wfctl plugin update %s", pName, iacServiceRequired, pName) } typed := newTypedIaCAdapter(conn, registered) if initErr := typed.Initialize(ctx, cfg); initErr != nil { - _ = closer.Close() - return nil, nil, fmt.Errorf("initialize provider %q: %w", providerName, initErr) + return nil, fmt.Errorf("initialize provider %q: %w", providerName, initErr) } - return typed, closer, nil + return typed, nil } // registeredIaCServices walks a plugin's ContractRegistry response and diff --git a/cmd/wfctl/deploy_providers_test.go b/cmd/wfctl/deploy_providers_test.go index 3e37b9c3..db61076b 100644 --- a/cmd/wfctl/deploy_providers_test.go +++ b/cmd/wfctl/deploy_providers_test.go @@ -9,21 +9,19 @@ import ( "github.com/GoCodeAlone/workflow/config" ) -// noopCloser is an io.Closer that does nothing — used in test stubs. -type noopCloser struct{} - -func (noopCloser) Close() error { return nil } - // ── discoverAndLoadIaCProvider — error propagation ──────────────────────────── // // Per plan §Task 16 (strict-contracts force-cutover), the legacy // `loadIaCPlugin` factory-based load path is removed; discoverAndLoadIaCProvider // now constructs typedIaCAdapter directly from the gRPC ClientConn + // ContractRegistry. Plugin-error propagation is exercised end-to-end by the -// typed-RPC integration tests in iac_typed_adapter_test.go (Task 30) and -// the cross-plugin-build matrix in .github/workflows/cross-plugin-build-test.yml -// (Task 6). The legacy TestResolveIaCProviderSurfacesPluginError test is -// removed alongside the loadIaCPlugin var it depended on. +// typed-RPC integration tests in iac_typed_adapter_test.go (PR #605, on +// main) and the cross-plugin-build matrix in +// .github/workflows/cross-plugin-build-test.yml (Task 6, on main). The +// boundary-level typed-return assertion lives in this file as +// TestDiscoverAndLoadIaCProvider_ReturnsTypedClient (per spec Step 1). +// The legacy TestResolveIaCProviderSurfacesPluginError test was removed +// alongside the loadIaCPlugin var it depended on. // ── newDeployProvider ───────────────────────────────────────────────────────── diff --git a/cmd/wfctl/discover_typed_loader_test.go b/cmd/wfctl/discover_typed_loader_test.go new file mode 100644 index 00000000..34cbc893 --- /dev/null +++ b/cmd/wfctl/discover_typed_loader_test.go @@ -0,0 +1,170 @@ +package main + +// discover_typed_loader_test.go — boundary test for the typed-IaC +// loader cutover (PR #609 / Task 16). Per spec Step 1: assert +// discoverAndLoadIaCProvider's post-LoadPlugin path returns the typed +// adapter (*typedIaCAdapter) rather than the legacy +// *remoteIaCProvider (which no longer exists post-cutover anyway). +// +// Tests the unit-testable seam buildTypedIaCAdapterFrom(adapter), not +// discoverAndLoadIaCProvider end-to-end — the latter spawns a real +// plugin subprocess + reads the filesystem, which is the cross-plugin- +// build matrix's job (Task 6, on main). Surfacing the boundary +// invariant here catches signature drift between the loader and +// typedIaCAdapter without paying the subprocess cost. + +import ( + "context" + "errors" + "net" + "strings" + "testing" + + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + + pb "github.com/GoCodeAlone/workflow/plugin/external/proto" +) + +// stubIaCAdapter satisfies iacAdapterAccessor against an in-process +// gRPC server registered with a stub IaCProviderRequiredServer. Used +// by the boundary tests to avoid spawning a real plugin subprocess. +type stubIaCAdapter struct { + conn *grpc.ClientConn + registry *pb.ContractRegistry + regErr error +} + +func (s *stubIaCAdapter) Conn() *grpc.ClientConn { return s.conn } +func (s *stubIaCAdapter) ContractRegistry() *pb.ContractRegistry { return s.registry } +func (s *stubIaCAdapter) ContractRegistryError() error { return s.regErr } + +// requiredOnlyServer satisfies pb.IaCProviderRequiredServer with the +// absolute minimum: only Initialize responds (every other method left +// to UnimplementedIaCProviderRequiredServer's defaults). Initialize is +// the one method buildTypedIaCAdapterFrom calls during loader path. +type requiredOnlyServer struct { + pb.UnimplementedIaCProviderRequiredServer +} + +func (s *requiredOnlyServer) Initialize(_ context.Context, _ *pb.InitializeRequest) (*pb.InitializeResponse, error) { + return &pb.InitializeResponse{}, nil +} + +// startInProcessTypedServer spins up an in-process gRPC server that +// registers the typed IaCProviderRequired service and returns a +// dial-back conn the test can hand to a stubIaCAdapter. +func startInProcessTypedServer(t *testing.T) (*grpc.Server, *grpc.ClientConn) { + t.Helper() + lis, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("net.Listen: %v", err) + } + srv := grpc.NewServer() + pb.RegisterIaCProviderRequiredServer(srv, &requiredOnlyServer{}) + go func() { _ = srv.Serve(lis) }() + conn, err := grpc.NewClient(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + if err != nil { + srv.Stop() + t.Fatalf("grpc.NewClient: %v", err) + } + return srv, conn +} + +// TestDiscoverAndLoadIaCProvider_ReturnsTypedClient asserts that the +// loader's post-LoadPlugin path returns the typed adapter +// (*typedIaCAdapter) — the cutover invariant. Per Spec Step 1. +func TestDiscoverAndLoadIaCProvider_ReturnsTypedClient(t *testing.T) { + srv, conn := startInProcessTypedServer(t) + defer srv.Stop() + defer conn.Close() + + stub := &stubIaCAdapter{ + conn: conn, + registry: &pb.ContractRegistry{ + Contracts: []*pb.ContractDescriptor{ + { + Kind: pb.ContractKind_CONTRACT_KIND_SERVICE, + ServiceName: iacServiceRequired, + }, + }, + }, + } + + provider, err := buildTypedIaCAdapterFrom(context.Background(), "stub-provider", "workflow-plugin-stub", map[string]any{}, stub) + if err != nil { + t.Fatalf("buildTypedIaCAdapterFrom: %v", err) + } + if provider == nil { + t.Fatal("expected non-nil interfaces.IaCProvider; got nil") + } + // Cutover invariant: the loader returns *typedIaCAdapter, NOT + // the legacy *remoteIaCProvider (which is deleted in this PR + // alongside the InvokeService string-dispatch surface). A + // regression that re-introduces the legacy proxy would fail + // this assertion at compile time (remoteIaCProvider type no + // longer exists) AND at runtime via the explicit cast below. + if _, ok := provider.(*typedIaCAdapter); !ok { + t.Fatalf("expected *typedIaCAdapter; got %T", provider) + } +} + +// TestDiscoverAndLoadIaCProvider_RejectsMissingRequiredService asserts +// that the loader rejects plugins whose ContractRegistry omits the +// IaCProviderRequired service — the strict-contracts hard-cutover +// invariant. Plugins that haven't migrated to the typed protocol +// fail loud at load time with a `wfctl plugin update` hint. +func TestDiscoverAndLoadIaCProvider_RejectsMissingRequiredService(t *testing.T) { + srv, conn := startInProcessTypedServer(t) + defer srv.Stop() + defer conn.Close() + + stub := &stubIaCAdapter{ + conn: conn, + registry: &pb.ContractRegistry{}, // empty: no IaCProviderRequired + } + + _, err := buildTypedIaCAdapterFrom(context.Background(), "stub-provider", "workflow-plugin-stub", map[string]any{}, stub) + if err == nil { + t.Fatal("expected error when ContractRegistry omits IaCProviderRequired; got nil") + } + // Message contract: must name the missing service + actionable + // upgrade hint so operators know how to recover. + msg := err.Error() + for _, want := range []string{ + "workflow-plugin-stub", + iacServiceRequired, + "wfctl plugin update", + } { + if !strings.Contains(msg, want) { + t.Errorf("error message %q missing expected substring %q", msg, want) + } + } +} + +// TestDiscoverAndLoadIaCProvider_SurfacesContractRegistryError asserts +// that a transport-level ContractRegistry RPC failure is surfaced AS +// the underlying error rather than masked by the generic "does not +// register the required service" message — per Copilot finding on PR #609. +func TestDiscoverAndLoadIaCProvider_SurfacesContractRegistryError(t *testing.T) { + srv, conn := startInProcessTypedServer(t) + defer srv.Stop() + defer conn.Close() + + regErr := errors.New("ContractRegistry transport reset") + stub := &stubIaCAdapter{ + conn: conn, + regErr: regErr, + } + + _, err := buildTypedIaCAdapterFrom(context.Background(), "stub-provider", "workflow-plugin-stub", map[string]any{}, stub) + if err == nil { + t.Fatal("expected error when ContractRegistry RPC failed; got nil") + } + if !errors.Is(err, regErr) { + t.Fatalf("expected errors.Is(err, regErr); got %v", err) + } + if !strings.Contains(err.Error(), "ContractRegistry RPC failed") { + t.Errorf("expected RPC-failure framing in error; got %q", err.Error()) + } +} diff --git a/plugin/external/adapter.go b/plugin/external/adapter.go index 181ac26b..9f582c9c 100644 --- a/plugin/external/adapter.go +++ b/plugin/external/adapter.go @@ -364,9 +364,17 @@ func (a *ExternalPluginAdapter) ContractRegistryError() error { // against this conn — for example // `pb.NewIaCProviderRequiredClient(adapter.Conn())`. // -// Returns nil for adapters constructed without a backing client (e.g. -// `newExternalPluginAdapterWithContractRegistry` test fixtures); callers -// MUST nil-check before constructing typed clients. +// Returns nil in two cases: +// 1. The adapter was constructed without a backing PluginClient +// (e.g. `newExternalPluginAdapterWithContractRegistry` test +// fixtures populate manifest + registry directly without a +// gRPC subprocess). +// 2. The adapter has a non-nil PluginClient but its underlying +// PluginClient.Conn() is itself nil (in-process test plumbing +// that wires only the PluginServiceClient interface without a +// real *grpc.ClientConn). +// +// Callers MUST nil-check before constructing typed clients. // // The connection lifecycle is owned by the host's plugin manager — // callers MUST NOT call Close() on the returned conn. The plugin