From 6fdf5eb46a778e3c0a32331b5cf200823a1d046f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 6 May 2026 22:45:02 -0400 Subject: [PATCH 1/6] =?UTF-8?q?fix(iac):=20rename=20DriftConfigDetector=20?= =?UTF-8?q?method=20DetectDriftWithApplied=20=E2=86=92=20DetectDriftWithSp?= =?UTF-8?q?ecs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aligns workflow's interface with the signature shipped in workflow-plugin- digitalocean v0.10.5. The DO plugin chose map[string]ResourceSpec (typed wrapper with Name/Type/Config fields) over map[string]map[string]any; the typed form is cleaner and avoids callers having to reconstruct name/type from keys. Wire protocol change: remoteIaCProvider.DetectDriftWithSpecs now invokes IaCProvider.DetectDrift with a "specs" arg map rather than a separate IaCProvider.DetectDriftWithApplied RPC method. This matches the DO plugin v0.10.5 dispatch (specs injection is gated inside invokeProviderDetectDrift on arg presence). Plugins predating DriftConfigDetector support still handle IaCProvider.DetectDrift; they ignore the unknown "specs" key and return existence-only results. buildAppliedSpecMap return type changed to map[string]interfaces.ResourceSpec; each entry is wrapped as ResourceSpec{Name, Type, Config}. All callers and tests updated. ADR 0007 extended with addendum documenting the rename and wire protocol change. Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/deploy_providers.go | 42 +++++++---------- ...deploy_providers_remote_iac_compat_test.go | 46 +++++++------------ cmd/wfctl/deploy_providers_remote_iac_test.go | 17 ++++--- cmd/wfctl/infra_apply_refresh.go | 8 ++-- cmd/wfctl/infra_drift_applied.go | 18 +++++--- cmd/wfctl/infra_drift_applied_test.go | 31 ++++++++----- cmd/wfctl/infra_status_drift.go | 8 ++-- ...-driftconfigdetector-optional-interface.md | 22 +++++++++ interfaces/iac_provider.go | 10 ++-- interfaces/iac_provider_test.go | 2 +- 10 files changed, 110 insertions(+), 94 deletions(-) diff --git a/cmd/wfctl/deploy_providers.go b/cmd/wfctl/deploy_providers.go index fb588701..71d7b4fe 100644 --- a/cmd/wfctl/deploy_providers.go +++ b/cmd/wfctl/deploy_providers.go @@ -487,37 +487,29 @@ func (r *remoteIaCProvider) DetectDrift(_ context.Context, refs []interfaces.Res return drifts, nil } -// DetectDriftWithApplied dispatches to the v2 RPC if the remote plugin -// supports it, falling back to legacy DetectDrift when the remote returns -// "method not found". This is the wire-format counterpart of the type- -// assertion fallback in callers; both layers need the fallback because: -// - Caller-side: provider may not implement the interface (type- -// assertion fails → legacy path). -// - Wire-side: provider implements the interface in newer code but the -// LOADED plugin binary on disk is older (manifest accepted but RPC -// dispatch returns method-not-found). -func (r *remoteIaCProvider) DetectDriftWithApplied(ctx context.Context, refs []interfaces.ResourceRef, applied map[string]map[string]any) ([]interfaces.DriftResult, error) { +// 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.DetectDriftWithApplied: marshal refs: %w", err) + return nil, fmt.Errorf("IaCProvider.DetectDrift(specs): marshal refs: %w", err) } - appliedAny, err := jsonToAny(applied) + specsAny, err := jsonToAny(specs) if err != nil { - return nil, fmt.Errorf("IaCProvider.DetectDriftWithApplied: marshal applied: %w", err) + return nil, fmt.Errorf("IaCProvider.DetectDrift(specs): marshal specs: %w", err) } - res, err := r.invoker.InvokeService("IaCProvider.DetectDriftWithApplied", map[string]any{ - "refs": refsAny, "applied": appliedAny, + res, err := r.invoker.InvokeService("IaCProvider.DetectDrift", map[string]any{ + "refs": refsAny, "specs": specsAny, }) if err != nil { - // Method-not-found from the plugin: fall back to legacy DetectDrift. - // Match on substring to tolerate framework-specific error wrappers - // (HashiCorp go-plugin emits "method not found"; future schemes may - // wrap the literal differently). DO NOT fall back on other errors — - // a transient cloud failure must propagate so callers don't silently - // lose drift signal. - if isMethodNotFound(err) { - return r.DetectDrift(ctx, refs) - } return nil, err } raw, ok := res["drifts"] @@ -526,7 +518,7 @@ func (r *remoteIaCProvider) DetectDriftWithApplied(ctx context.Context, refs []i } var drifts []interfaces.DriftResult if err := anyToStruct(raw, &drifts); err != nil { - return nil, fmt.Errorf("IaCProvider.DetectDriftWithApplied: decode result: %w", err) + return nil, fmt.Errorf("IaCProvider.DetectDrift(specs): decode result: %w", err) } return drifts, nil } diff --git a/cmd/wfctl/deploy_providers_remote_iac_compat_test.go b/cmd/wfctl/deploy_providers_remote_iac_compat_test.go index 79a4ca53..5258378e 100644 --- a/cmd/wfctl/deploy_providers_remote_iac_compat_test.go +++ b/cmd/wfctl/deploy_providers_remote_iac_compat_test.go @@ -2,54 +2,42 @@ package main import ( "context" - "errors" "testing" "github.com/GoCodeAlone/workflow/interfaces" ) -// TestRemoteIaC_OptionalDriftConfigDetector_FallsBackOnLegacyPlugin pins the -// pipeline's compat story: when a remote plugin does NOT support -// IaCProvider.DetectDriftWithApplied (legacy plugin: returns "method not -// found"), the wfctl caller falls through to legacy IaCProvider.DetectDrift. -// -// This is the wire-format counterpart of the type-assertion fallback in -// runInfraApplyRefreshPhase / runInfraStatusDrift; without this gate, a v0.22 -// wfctl talking to a v0.10.3 DO plugin (or an out-of-tree plugin) could trip -// a hard error instead of falling back. -func TestRemoteIaC_OptionalDriftConfigDetector_FallsBackOnLegacyPlugin(t *testing.T) { +// 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{ - // Method not found = canonical signal a legacy plugin emits when - // it doesn't have a case for this RPC name. - errByMethod: map[string]error{ - "IaCProvider.DetectDriftWithApplied": errors.New("method not found: IaCProvider.DetectDriftWithApplied"), - }, - // Legacy DetectDrift returns success. respByMethod: map[string]map[string]any{ "IaCProvider.DetectDrift": { - "drifts": []any{map[string]any{"name": "x", "type": "infra.test", "drifted": false, "class": "in-sync"}}, + "drifts": []any{map[string]any{"name": "x", "type": "infra.test", "drifted": true, "class": "config"}}, }, }, } p := &remoteIaCProvider{invoker: si} - // Caller-side type-assertion: remoteIaCProvider implements - // DetectDriftWithApplied (it always does, since the wfctl-side wrapper - // ALWAYS exposes the method). The fallback happens INSIDE the wrapper: - // if the remote returns method-not-found, the wrapper retries with - // legacy DetectDrift. refs := []interfaces.ResourceRef{{Name: "x", Type: "infra.test"}} - drifts, err := p.DetectDriftWithApplied(context.Background(), refs, nil) + 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("DetectDriftWithApplied should NOT propagate method-not-found; should fall back. err=%v", err) + t.Fatalf("DetectDriftWithSpecs: unexpected error: %v", err) } - if len(drifts) != 1 || drifts[0].Class != interfaces.DriftClassInSync { - t.Errorf("expected fallback InSync drift; got %+v", drifts) + if len(drifts) != 1 || drifts[0].Class != interfaces.DriftClassConfig { + t.Errorf("expected config-drift result; got %+v", drifts) } - // Verify the second call to invoker WAS the legacy method. + // 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("expected fallback to call IaCProvider.DetectDrift; called methods: %v", si.calledMethods) + t.Errorf("DetectDriftWithSpecs must invoke IaCProvider.DetectDrift; called methods: %v", si.calledMethods) } } diff --git a/cmd/wfctl/deploy_providers_remote_iac_test.go b/cmd/wfctl/deploy_providers_remote_iac_test.go index 36cbdba6..359737d8 100644 --- a/cmd/wfctl/deploy_providers_remote_iac_test.go +++ b/cmd/wfctl/deploy_providers_remote_iac_test.go @@ -473,9 +473,9 @@ func TestRemoteIaC_DetectDrift_Empty(t *testing.T) { } } -// ── DetectDriftWithApplied ───────────────────────────────────────────────────── +// ── DetectDriftWithSpecs ─────────────────────────────────────────────────────── -func TestRemoteIaC_DetectDriftWithApplied_HappyPath(t *testing.T) { +func TestRemoteIaC_DetectDriftWithSpecs_HappyPath(t *testing.T) { si := &stubInvoker{resp: map[string]any{ "drifts": []any{map[string]any{ "name": "x", @@ -487,13 +487,16 @@ func TestRemoteIaC_DetectDriftWithApplied_HappyPath(t *testing.T) { }} p := newIaCProvider(si) refs := []interfaces.ResourceRef{{Name: "x", Type: "infra.test"}} - applied := map[string]map[string]any{"x": {"region": "nyc1"}} - drifts, err := p.DetectDriftWithApplied(context.Background(), refs, applied) + 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("DetectDriftWithApplied: %v", err) + t.Fatalf("DetectDriftWithSpecs: %v", err) } - if si.method != "IaCProvider.DetectDriftWithApplied" { - t.Errorf("method: got %q, want IaCProvider.DetectDriftWithApplied", si.method) + // 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) } if len(drifts) != 1 || drifts[0].Class != interfaces.DriftClassConfig { t.Errorf("drifts: %+v", drifts) diff --git a/cmd/wfctl/infra_apply_refresh.go b/cmd/wfctl/infra_apply_refresh.go index 3b4a2d40..ba7a7425 100644 --- a/cmd/wfctl/infra_apply_refresh.go +++ b/cmd/wfctl/infra_apply_refresh.go @@ -47,14 +47,14 @@ func runInfraApplyRefreshPhase( } // Use DriftConfigDetector when the provider supports it (optional interface). - // Short-circuits to legacy DetectDrift when appliedMap is nil (no "apply"- + // Short-circuits to legacy DetectDrift when specsMap is nil (no "apply"- // provenance entries available) to avoid unnecessary RPC round-trips. var results []interfaces.DriftResult var err error if d, ok := provider.(interfaces.DriftConfigDetector); ok { - appliedMap := buildAppliedSpecMap(states, refs) - if appliedMap != nil { - results, err = d.DetectDriftWithApplied(ctx, refs, appliedMap) + specsMap := buildAppliedSpecMap(states, refs) + if specsMap != nil { + results, err = d.DetectDriftWithSpecs(ctx, refs, specsMap) } else { results, err = provider.DetectDrift(ctx, refs) } diff --git a/cmd/wfctl/infra_drift_applied.go b/cmd/wfctl/infra_drift_applied.go index de7755f2..5a83c61c 100644 --- a/cmd/wfctl/infra_drift_applied.go +++ b/cmd/wfctl/infra_drift_applied.go @@ -2,10 +2,10 @@ package main import "github.com/GoCodeAlone/workflow/interfaces" -// buildAppliedSpecMap walks states + refs and returns the per-ref applied- -// config map for DriftConfigDetector.DetectDriftWithApplied. Entries whose -// AppliedConfigSource is anything other than "apply" are OMITTED from the -// returned map — providers cannot meaningfully compute config-drift against +// buildAppliedSpecMap walks states + refs and returns the per-ref +// ResourceSpec map for DriftConfigDetector.DetectDriftWithSpecs. Entries +// whose AppliedConfigSource is anything other than "apply" are OMITTED from +// the returned map — providers cannot meaningfully compute config-drift against // adoption-shaped Outputs (would yield false-positives) and legacy state // (no provenance recorded) defaults to adoption treatment per ADR 0010. // @@ -14,7 +14,7 @@ import "github.com/GoCodeAlone/workflow/interfaces" // // Returns nil when no safe entries exist, so callers can short-circuit the // type-assertion entirely and fall back to legacy DetectDrift. -func buildAppliedSpecMap(states []interfaces.ResourceState, refs []interfaces.ResourceRef) map[string]map[string]any { +func buildAppliedSpecMap(states []interfaces.ResourceState, refs []interfaces.ResourceRef) map[string]interfaces.ResourceSpec { if len(states) == 0 || len(refs) == 0 { return nil } @@ -22,7 +22,7 @@ func buildAppliedSpecMap(states []interfaces.ResourceState, refs []interfaces.Re for i := range states { byName[states[i].Name] = &states[i] } - out := make(map[string]map[string]any, len(refs)) + out := make(map[string]interfaces.ResourceSpec, len(refs)) for _, ref := range refs { st, ok := byName[ref.Name] if !ok { @@ -42,7 +42,11 @@ func buildAppliedSpecMap(states []interfaces.ResourceState, refs []interfaces.Re for k, v := range st.AppliedConfig { cfg[k] = v } - out[ref.Name] = cfg + out[ref.Name] = interfaces.ResourceSpec{ + Name: ref.Name, + Type: ref.Type, + Config: cfg, + } } if len(out) == 0 { return nil diff --git a/cmd/wfctl/infra_drift_applied_test.go b/cmd/wfctl/infra_drift_applied_test.go index 027848a6..44bc4f22 100644 --- a/cmd/wfctl/infra_drift_applied_test.go +++ b/cmd/wfctl/infra_drift_applied_test.go @@ -25,17 +25,23 @@ func TestBuildAppliedSpecMap_OmitsAdoptionAndLegacy(t *testing.T) { } got := buildAppliedSpecMap(states, refs) - want := map[string]map[string]any{ - "apply-resource": {"k": "v"}, - // adoption-resource: omitted (refuse false-positive on adoption) - // legacy-resource: omitted (legacy default-to-adoption) - // nil-config-resource: omitted (nil AppliedConfig) - // empty-map-config-resource: omitted (len 0 — same branch as nil) - // missing-from-state: omitted (no state) + // adoption-resource: omitted (refuse false-positive on adoption) + // legacy-resource: omitted (legacy default-to-adoption) + // nil-config-resource: omitted (nil AppliedConfig) + // empty-map-config-resource: omitted (len 0 — same branch as nil) + // missing-from-state: omitted (no state) + if len(got) != 1 { + t.Fatalf("expected 1 entry in result, got %d: %v", len(got), got) } - - if !reflect.DeepEqual(got, want) { - t.Errorf("got %v, want %v", got, want) + spec, ok := got["apply-resource"] + if !ok { + t.Fatalf("expected 'apply-resource' in result; got %v", got) + } + if spec.Name != "apply-resource" { + t.Errorf("spec.Name: got %q, want %q", spec.Name, "apply-resource") + } + if !reflect.DeepEqual(spec.Config, map[string]any{"k": "v"}) { + t.Errorf("spec.Config: got %v, want %v", spec.Config, map[string]any{"k": "v"}) } } @@ -75,8 +81,9 @@ func TestBuildAppliedSpecMap_ShallowCopyPreventsCallerMutation(t *testing.T) { refs := []interfaces.ResourceRef{{Name: "x"}} got := buildAppliedSpecMap(states, refs) - // Mutate the returned map; verify the source state is not affected. - got["x"]["k"] = "mutated" + // Mutate the returned spec's Config; verify the source state is not affected. + spec := got["x"] + spec.Config["k"] = "mutated" if states[0].AppliedConfig["k"] == "mutated" { t.Errorf("buildAppliedSpecMap must return a shallow copy; source state was mutated") } diff --git a/cmd/wfctl/infra_status_drift.go b/cmd/wfctl/infra_status_drift.go index 7e51fdb2..f9126deb 100644 --- a/cmd/wfctl/infra_status_drift.go +++ b/cmd/wfctl/infra_status_drift.go @@ -101,13 +101,13 @@ func driftInfraModules(ctx context.Context, cfgFile, envName string) error { } // Use DriftConfigDetector when the provider supports it (optional interface). - // Short-circuits to legacy DetectDrift when appliedMap is nil (no "apply"- + // Short-circuits to legacy DetectDrift when specsMap is nil (no "apply"- // provenance entries available) to avoid unnecessary RPC round-trips. var results []interfaces.DriftResult if d, ok := provider.(interfaces.DriftConfigDetector); ok { - appliedMap := buildAppliedSpecMap(states, g.refs) - if appliedMap != nil { - results, err = d.DetectDriftWithApplied(ctx, g.refs, appliedMap) + specsMap := buildAppliedSpecMap(states, g.refs) + if specsMap != nil { + results, err = d.DetectDriftWithSpecs(ctx, g.refs, specsMap) } else { results, err = provider.DetectDrift(ctx, g.refs) } diff --git a/decisions/0007-iac-driftconfigdetector-optional-interface.md b/decisions/0007-iac-driftconfigdetector-optional-interface.md index 55977d0a..38fdaaf8 100644 --- a/decisions/0007-iac-driftconfigdetector-optional-interface.md +++ b/decisions/0007-iac-driftconfigdetector-optional-interface.md @@ -61,3 +61,25 @@ Adversarial design review #1 (Critical finding) rejected changing `DetectDrift(c - `cmd/wfctl/infra_apply_refresh.go` — caller type-assertion - `cmd/wfctl/infra_status_drift.go` — caller type-assertion - `cmd/wfctl/infra_drift_applied.go` — `buildAppliedSpecMap` helper + +--- + +## 2026-05-07 update: rename DetectDriftWithApplied → DetectDriftWithSpecs (v0.22.3) + +**Context:** While v0.22.2 was being finalized, workflow-plugin-digitalocean shipped v0.10.5 with its own implementation of the DriftConfigDetector interface. The DO plugin chose the signature: + +```go +DetectDriftWithSpecs(ctx context.Context, resources []ResourceRef, specs map[string]ResourceSpec) ([]DriftResult, error) +``` + +This uses `ResourceSpec` (the typed wrapper with `Name`, `Type`, and `Config` fields) instead of `map[string]map[string]any`. The typed form is cleaner — callers pass all of Name/Type/Config in one value, providers can read Name and Type directly without key lookup. + +**Decision:** Adopt the DO plugin's signature as canonical. The autonomous mandate includes "build the right way; refactor where needed." Aligning the interface with the shipped implementation avoids a duplicate method on DOProvider and is the correct long-term design. + +**Wire protocol change:** The original plan called `IaCProvider.DetectDriftWithApplied` as a separate RPC method. The DO plugin v0.10.5 routes spec-injection through `IaCProvider.DetectDrift` by checking for a `"specs"` key in the args map — no new RPC case needed. The wfctl `remoteIaCProvider.DetectDriftWithSpecs` now calls `IaCProvider.DetectDrift` with `refs` + `specs` args. This is more robust: plugins that predate DriftConfigDetector support still handle `IaCProvider.DetectDrift` (they ignore the unknown `specs` key and return existence-only results). + +**Changes in this release (v0.22.3):** +- `interfaces.DriftConfigDetector.DetectDriftWithApplied` renamed to `DetectDriftWithSpecs`; parameter changed from `map[string]map[string]any` to `map[string]ResourceSpec` +- `buildAppliedSpecMap` return type changed from `map[string]map[string]any` to `map[string]interfaces.ResourceSpec`; each entry is now wrapped as `ResourceSpec{Name, Type, Config}` +- `remoteIaCProvider.DetectDriftWithSpecs` sends `IaCProvider.DetectDrift` with `specs` arg (not a separate `DetectDriftWithApplied` RPC name) +- All callers and tests updated accordingly diff --git a/interfaces/iac_provider.go b/interfaces/iac_provider.go index a217e52e..8aed679e 100644 --- a/interfaces/iac_provider.go +++ b/interfaces/iac_provider.go @@ -88,10 +88,10 @@ type Enumerator interface { // surface config-drift in addition to the existence-only Ghost / InSync / // Unknown classifications produced by DetectDrift. // -// applied is the per-ref applied_config map recorded in state. Callers -// pass it from ResourceState.AppliedConfig; missing or empty entries -// instruct the provider to fall back to existence-only behavior for -// that ref. The map key is ref.Name (matches ResourceState.Name). +// specs is the per-ref desired-spec map recorded in state. Callers build it +// from ResourceState.AppliedConfig (wrapped into ResourceSpec); missing or +// empty entries instruct the provider to fall back to existence-only behavior +// for that ref. The map key is ref.Name (matches ResourceState.Name). // // Callers MUST type-assert against this interface and fall back to // IaCProvider.DetectDrift(refs) on the negative case. Providers that do @@ -102,7 +102,7 @@ type Enumerator interface { // adoption-shaped Outputs reflow); see ResourceState.AppliedConfigSource // (iac_state.go) for the canonical discriminator. type DriftConfigDetector interface { - DetectDriftWithApplied(ctx context.Context, resources []ResourceRef, applied map[string]map[string]any) ([]DriftResult, error) + DetectDriftWithSpecs(ctx context.Context, resources []ResourceRef, specs map[string]ResourceSpec) ([]DriftResult, error) } // BootstrapResult contains metadata returned by a successful BootstrapStateBackend call. diff --git a/interfaces/iac_provider_test.go b/interfaces/iac_provider_test.go index 40e52d3c..fd4610f6 100644 --- a/interfaces/iac_provider_test.go +++ b/interfaces/iac_provider_test.go @@ -242,6 +242,6 @@ func TestDriftConfigDetector_OptionalInterface(t *testing.T) { // capableIaCProvider extends nonValidatingProvider with DriftConfigDetector. type capableIaCProvider struct{ nonValidatingProvider } -func (*capableIaCProvider) DetectDriftWithApplied(context.Context, []interfaces.ResourceRef, map[string]map[string]any) ([]interfaces.DriftResult, error) { +func (*capableIaCProvider) DetectDriftWithSpecs(context.Context, []interfaces.ResourceRef, map[string]interfaces.ResourceSpec) ([]interfaces.DriftResult, error) { return nil, nil } From 8f70be95e523e5817584c5b29e8aae6064902588 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 6 May 2026 22:53:25 -0400 Subject: [PATCH 2/6] fix(lint): remove unused isMethodNotFound helper after DetectDriftWithSpecs refactor The isMethodNotFound helper was only referenced inside the old DetectDriftWithApplied wrapper. DetectDriftWithSpecs now dispatches via IaCProvider.DetectDrift (always available), so no method-not-found fallback is needed at the wire level. Remove the unused function to satisfy golangci-lint. Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/deploy_providers.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/cmd/wfctl/deploy_providers.go b/cmd/wfctl/deploy_providers.go index 71d7b4fe..d49866ab 100644 --- a/cmd/wfctl/deploy_providers.go +++ b/cmd/wfctl/deploy_providers.go @@ -523,17 +523,6 @@ func (r *remoteIaCProvider) DetectDriftWithSpecs(ctx context.Context, refs []int return drifts, nil } -// isMethodNotFound reports whether err is the canonical signal a remote -// plugin emits when it has no case for the requested RPC method. Tolerant -// substring match across known wrapper formats. -func isMethodNotFound(err error) bool { - if err == nil { - return false - } - msg := err.Error() - return strings.Contains(msg, "method not found") || strings.Contains(msg, "Method not found") -} - 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, From d37d708129d45938712d0237dbcdf18941d45e50 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 6 May 2026 22:55:34 -0400 Subject: [PATCH 3/6] fix(review): address Copilot inline comments on DetectDriftWithSpecs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three real findings from Copilot review round 1: 1. buildAppliedSpecMap: prefer st.Type (canonical from ResourceState) over ref.Type when building ResourceSpec. Ref may be a lightweight lookup without full type info; state is authoritative. Falls back to ref.Type when st.Type is empty for backwards compat with state records that predate Type recording. 2. Test assertion: verify InvokeService is called with "specs" key present and legacy "applied" key absent — pins the wire contract. 3. New test TestBuildAppliedSpecMap_PrefersStateTypeOverRefType covers the st.Type → spec.Type path. One ghost flag resolved without change: Copilot flagged ctx parameter in DetectDriftWithSpecs as "will fail compilation" — Go does not fail on unused function parameters (only unused local variables). Build confirms this is safe. Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/deploy_providers_remote_iac_test.go | 7 +++++++ cmd/wfctl/infra_drift_applied.go | 8 +++++++- cmd/wfctl/infra_drift_applied_test.go | 19 +++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/cmd/wfctl/deploy_providers_remote_iac_test.go b/cmd/wfctl/deploy_providers_remote_iac_test.go index 359737d8..cbe075ab 100644 --- a/cmd/wfctl/deploy_providers_remote_iac_test.go +++ b/cmd/wfctl/deploy_providers_remote_iac_test.go @@ -498,6 +498,13 @@ func TestRemoteIaC_DetectDriftWithSpecs_HappyPath(t *testing.T) { 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) } diff --git a/cmd/wfctl/infra_drift_applied.go b/cmd/wfctl/infra_drift_applied.go index 5a83c61c..444d1d35 100644 --- a/cmd/wfctl/infra_drift_applied.go +++ b/cmd/wfctl/infra_drift_applied.go @@ -42,9 +42,15 @@ func buildAppliedSpecMap(states []interfaces.ResourceState, refs []interfaces.Re for k, v := range st.AppliedConfig { cfg[k] = v } + // Prefer st.Type (canonical from state) over ref.Type (from grouping + // which may be a lightweight ref without full type info). + specType := st.Type + if specType == "" { + specType = ref.Type + } out[ref.Name] = interfaces.ResourceSpec{ Name: ref.Name, - Type: ref.Type, + Type: specType, Config: cfg, } } diff --git a/cmd/wfctl/infra_drift_applied_test.go b/cmd/wfctl/infra_drift_applied_test.go index 44bc4f22..a57e5b57 100644 --- a/cmd/wfctl/infra_drift_applied_test.go +++ b/cmd/wfctl/infra_drift_applied_test.go @@ -45,6 +45,25 @@ func TestBuildAppliedSpecMap_OmitsAdoptionAndLegacy(t *testing.T) { } } +// TestBuildAppliedSpecMap_PrefersStateTypeOverRefType verifies that when +// ResourceState.Type is set, it takes precedence over ref.Type in the +// ResourceSpec output (state is canonical; ref may be a lightweight lookup). +func TestBuildAppliedSpecMap_PrefersStateTypeOverRefType(t *testing.T) { + states := []interfaces.ResourceState{ + {Name: "x", Type: "infra.database", AppliedConfig: map[string]any{"k": "v"}, AppliedConfigSource: "apply"}, + } + refs := []interfaces.ResourceRef{{Name: "x", Type: "infra.other"}} // ref has wrong type + + got := buildAppliedSpecMap(states, refs) + spec, ok := got["x"] + if !ok { + t.Fatalf("expected 'x' in result") + } + if spec.Type != "infra.database" { + t.Errorf("spec.Type: got %q, want %q (state type should win)", spec.Type, "infra.database") + } +} + func TestBuildAppliedSpecMap_NilStatesReturnsNil(t *testing.T) { refs := []interfaces.ResourceRef{{Name: "x"}} got := buildAppliedSpecMap(nil, refs) From ee34bb1a4684e8567f2354b9f93a5df5519adb8e Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 6 May 2026 23:02:28 -0400 Subject: [PATCH 4/6] Rename ctx to _ in DetectDriftWithSpecs (unused parameter pattern) Matches the _ context.Context pattern used by all other remoteIaCProvider methods that call InvokeService (which does not accept a context). Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/deploy_providers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/wfctl/deploy_providers.go b/cmd/wfctl/deploy_providers.go index d49866ab..330a0723 100644 --- a/cmd/wfctl/deploy_providers.go +++ b/cmd/wfctl/deploy_providers.go @@ -497,7 +497,7 @@ func (r *remoteIaCProvider) DetectDrift(_ context.Context, refs []interfaces.Res // 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) { +func (r *remoteIaCProvider) DetectDriftWithSpecs(_ 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) From 5a13b7ddf07dcefd3a19e10b1985c04db136bf4c Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 6 May 2026 23:05:47 -0400 Subject: [PATCH 5/6] Apply InvokeServiceContext pattern to DetectDriftWithSpecs; update ADR 0007 signatures - DetectDriftWithSpecs now mirrors RepairDirtyMigration: type-asserts invoker to remoteServiceContextInvoker and calls InvokeServiceContext(ctx, ...) when available, falling back to InvokeService with ctx.Err() guard. - ADR 0007 Decision code blocks updated from DetectDriftWithApplied to DetectDriftWithSpecs and map[string]map[string]any to map[string]ResourceSpec to match the v0.22.3 canonical interface; labels pre-v0.22.3 context as historical. Co-Authored-By: Claude Sonnet 4.6 --- cmd/wfctl/deploy_providers.go | 15 +++++++++++---- ...-iac-driftconfigdetector-optional-interface.md | 7 ++++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/cmd/wfctl/deploy_providers.go b/cmd/wfctl/deploy_providers.go index 330a0723..88f68b41 100644 --- a/cmd/wfctl/deploy_providers.go +++ b/cmd/wfctl/deploy_providers.go @@ -497,7 +497,7 @@ func (r *remoteIaCProvider) DetectDrift(_ context.Context, refs []interfaces.Res // 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(_ context.Context, refs []interfaces.ResourceRef, specs map[string]interfaces.ResourceSpec) ([]interfaces.DriftResult, error) { +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) @@ -506,9 +506,16 @@ func (r *remoteIaCProvider) DetectDriftWithSpecs(_ context.Context, refs []inter if err != nil { return nil, fmt.Errorf("IaCProvider.DetectDrift(specs): marshal specs: %w", err) } - res, err := r.invoker.InvokeService("IaCProvider.DetectDrift", map[string]any{ - "refs": refsAny, "specs": specsAny, - }) + 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 } diff --git a/decisions/0007-iac-driftconfigdetector-optional-interface.md b/decisions/0007-iac-driftconfigdetector-optional-interface.md index 38fdaaf8..33f80b14 100644 --- a/decisions/0007-iac-driftconfigdetector-optional-interface.md +++ b/decisions/0007-iac-driftconfigdetector-optional-interface.md @@ -18,8 +18,9 @@ Add `DriftConfigDetector` as an **OPTIONAL interface** (Path 2). `IaCProvider.DetectDrift` is unchanged. ```go +// As of v0.22.3 (see addendum below for rename rationale): type DriftConfigDetector interface { - DetectDriftWithApplied(ctx context.Context, resources []ResourceRef, applied map[string]map[string]any) ([]DriftResult, error) + DetectDriftWithSpecs(ctx context.Context, resources []ResourceRef, specs map[string]ResourceSpec) ([]DriftResult, error) } ``` @@ -27,13 +28,13 @@ Callers type-assert and fall back: ```go if d, ok := provider.(DriftConfigDetector); ok { - results, err = d.DetectDriftWithApplied(ctx, refs, appliedMap) + results, err = d.DetectDriftWithSpecs(ctx, refs, specsMap) } else { results, err = provider.DetectDrift(ctx, refs) } ``` -The `applied` map is keyed by `ref.Name` and sourced from `ResourceState.AppliedConfig`. The sentinel field `ResourceState.AppliedConfigSource` (see ADR 0010) discriminates "apply" (true user-supplied config) from "adoption" (Outputs reflow). Providers MUST refuse to compute config-drift on adoption-shaped entries to avoid false-positives. +The `specs` map is keyed by `ref.Name` and built from `ResourceState.AppliedConfig` via `buildAppliedSpecMap`. Each entry is a `ResourceSpec{Name, Type, Config}` — the typed form allows providers to read Name and Type directly without key lookup. The sentinel field `ResourceState.AppliedConfigSource` (see ADR 0010) discriminates "apply" (true user-supplied config) from "adoption" (Outputs reflow). Providers MUST refuse to compute config-drift on adoption-shaped entries to avoid false-positives. ## Consequences From 2cdfcf1e8789f6af0696e109123f4284c1d86990 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 6 May 2026 23:19:40 -0400 Subject: [PATCH 6/6] Fix DetectDriftWithSpecs doc comment; update ADR 0007/0010 for rename and wire protocol accuracy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - interfaces/iac_provider.go: change 'desired-spec map' to 'applied-config map sourced from state' in the DetectDriftWithSpecs doc comment — the specs come from ResourceState.AppliedConfig (applied/user-supplied config), not a desired/intended future state. - decisions/0007: qualify wire protocol claim about unknown-key handling — only true for LEGACY_STRUCT/PROTO_WITH_LEGACY_STRUCT plugins; STRICT_PROTO plugins must include an optional specs field in their DetectDrift proto message. - decisions/0010: rename DetectDriftWithApplied → DetectDriftWithSpecs in all references to match the v0.22.3 interface rename. Co-Authored-By: Claude Sonnet 4.6 --- .../0007-iac-driftconfigdetector-optional-interface.md | 2 +- decisions/0010-applied-config-source-on-resourcestate.md | 6 +++--- interfaces/iac_provider.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/decisions/0007-iac-driftconfigdetector-optional-interface.md b/decisions/0007-iac-driftconfigdetector-optional-interface.md index 33f80b14..6ebb1c11 100644 --- a/decisions/0007-iac-driftconfigdetector-optional-interface.md +++ b/decisions/0007-iac-driftconfigdetector-optional-interface.md @@ -77,7 +77,7 @@ This uses `ResourceSpec` (the typed wrapper with `Name`, `Type`, and `Config` fi **Decision:** Adopt the DO plugin's signature as canonical. The autonomous mandate includes "build the right way; refactor where needed." Aligning the interface with the shipped implementation avoids a duplicate method on DOProvider and is the correct long-term design. -**Wire protocol change:** The original plan called `IaCProvider.DetectDriftWithApplied` as a separate RPC method. The DO plugin v0.10.5 routes spec-injection through `IaCProvider.DetectDrift` by checking for a `"specs"` key in the args map — no new RPC case needed. The wfctl `remoteIaCProvider.DetectDriftWithSpecs` now calls `IaCProvider.DetectDrift` with `refs` + `specs` args. This is more robust: plugins that predate DriftConfigDetector support still handle `IaCProvider.DetectDrift` (they ignore the unknown `specs` key and return existence-only results). +**Wire protocol change:** The original plan called `IaCProvider.DetectDriftWithApplied` as a separate RPC method. The DO plugin v0.10.5 routes spec-injection through `IaCProvider.DetectDrift` by checking for a `"specs"` key in the args map — no new RPC case needed. The wfctl `remoteIaCProvider.DetectDriftWithSpecs` now calls `IaCProvider.DetectDrift` with `refs` + `specs` args. For plugins using `LEGACY_STRUCT` or `PROTO_WITH_LEGACY_STRUCT` contract mode (the current DO plugin default), the extra `specs` key in the args map is ignored by older plugins and they return existence-only results. For plugins using `STRICT_PROTO` contracts, the underlying `DetectDrift` proto message must include an optional `specs` field for this to work without an unknown-field encode error. **Changes in this release (v0.22.3):** - `interfaces.DriftConfigDetector.DetectDriftWithApplied` renamed to `DetectDriftWithSpecs`; parameter changed from `map[string]map[string]any` to `map[string]ResourceSpec` diff --git a/decisions/0010-applied-config-source-on-resourcestate.md b/decisions/0010-applied-config-source-on-resourcestate.md index 2e180d33..359c1fe1 100644 --- a/decisions/0010-applied-config-source-on-resourcestate.md +++ b/decisions/0010-applied-config-source-on-resourcestate.md @@ -7,15 +7,15 @@ ## Context -`DriftConfigDetector.DetectDriftWithApplied` (ADR 0007) receives the `applied` map from `ResourceState.AppliedConfig`. However, `AppliedConfig` can contain two distinct kinds of data: +`DriftConfigDetector.DetectDriftWithSpecs` (ADR 0007, formerly `DetectDriftWithApplied`) receives per-ref specs built from `ResourceState.AppliedConfig`. However, `AppliedConfig` can contain two distinct kinds of data: 1. **True user config** — set by `applyWithProviderAndStore` and `applyPrecomputedPlanWithStore` from `ResourceSpec.Config`. Comparing this against a fresh cloud Read is safe and meaningful. 2. **Adoption-shaped Outputs reflow** — set by `adoptExistingResources` via `liveConfigFromOutputs(live.Outputs)`. When no embedded "config" key exists in Outputs, `liveConfigFromOutputs` falls back to `cloneMap(live.Outputs)`. Comparing such an entry against a fresh Read yields false-positive config-drift (Outputs contain fields like `id`, `created_at`, `endpoint` that are not user-controllable via spec). -Without discrimination, `DetectDriftWithApplied` cannot tell safe from unsafe entries. Two placement options were considered: +Without discrimination, `DetectDriftWithSpecs` cannot tell safe from unsafe entries. Two placement options were considered: -1. **Magic key in AppliedConfig** — store the sentinel as `applied_config["_source"]`. Reads directly during `DetectDriftWithApplied` without a separate lookup. +1. **Magic key in AppliedConfig** — store the sentinel as `applied_config["_source"]`. Reads directly during `DetectDriftWithSpecs` without a separate lookup. 2. **Dedicated field on ResourceState** — `ResourceState.AppliedConfigSource string`. Clean separation of concerns; sentinel is not part of the user-config payload; JSON encoding is `omitempty` so legacy state files remain unaffected. ## Decision diff --git a/interfaces/iac_provider.go b/interfaces/iac_provider.go index 8aed679e..2f25d67e 100644 --- a/interfaces/iac_provider.go +++ b/interfaces/iac_provider.go @@ -88,7 +88,7 @@ type Enumerator interface { // surface config-drift in addition to the existence-only Ghost / InSync / // Unknown classifications produced by DetectDrift. // -// specs is the per-ref desired-spec map recorded in state. Callers build it +// specs is the per-ref applied-config map sourced from state. Callers build it // from ResourceState.AppliedConfig (wrapped into ResourceSpec); missing or // empty entries instruct the provider to fall back to existence-only behavior // for that ref. The map key is ref.Name (matches ResourceState.Name).