feat(wfctl): typed-IaC cutover — replace remoteIaCProvider with pb.IaCProviderRequiredClient (PR 4 / Task 16)#609
Merged
Conversation
7 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a small but necessary API surface to retain and expose the underlying *grpc.ClientConn for external plugins, enabling future typed gRPC client construction (e.g., for the upcoming wfctl typed-IaC cutover) without losing access to the original connection.
Changes:
- Store the
*grpc.ClientConnonexternal.PluginClientand expose it viaConn(). - Expose the same connection from
external.ExternalPluginAdapterviaConn()(delegating to the wrapped client).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
plugin/external/grpc_plugin.go |
Retains the gRPC connection on PluginClient and adds a Conn() accessor. |
plugin/external/adapter.go |
Adds ExternalPluginAdapter.Conn() to surface the plugin connection to downstream callers. |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
intel352
changed the title
Comment on lines
+192
to
196
| 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) | ||
| } |
Comment on lines
+147
to
+161
| // 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) { |
Comment on lines
167
to
168
| pName, _, hasBinary, findErr := findIaCPluginDir(pluginDir, providerName) | ||
| if findErr != nil { |
intel352
added a commit
that referenced
this pull request
May 10, 2026
Cross-task naming coordination with PR #605/#609 (implementer-2, Tasks 30/16). Their iac_typed_adapter.go declares 8 sibling consts naming every typed IaC service: iacServiceRequired iacServiceEnumerator iacServiceDriftDetector iacServiceCredentialRevoker iacServiceMigrationRepairer iacServiceValidator iacServiceDriftConfigDetect iacServiceResourceDriver PR #610 originally introduced its own const iacRequiredServiceName for the same Required service FQN. Spec-reviewer flagged the inconsistency and asked us to coordinate. Implementer-2 + I agreed their convention wins (8 siblings establish the pattern; my single const matches). Pure rename; no behavior change. Tests still pass. Verification: GOWORK=off go test ./cmd/wfctl/ -run \ "TestAssertIaCPluginAdvertises|TestIsLegacyIaCPluginErr" \ -count=1 → PASS (7/7); gofmt clean. Once PR #610 merges, implementer-2's PR #609 rebase can drop their duplicate const and import iacServiceRequired from here.
… / ExternalPluginAdapter.Conn()
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 <noreply@anthropic.com>
…CProviderRequiredClient (Task 16) 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 ad7d946 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
intel352
force-pushed
the
feat/iac-wfctl-typed-cutover-task16
branch
from
b5d3fae to
b176057
Compare
intel352
added a commit
that referenced
this pull request
May 10, 2026
…tover runbook (Task 18) (#610) * feat(proto): add iac.proto with IaCProviderRequired + 6 optional services + ResourceDriver Task 3 of the strict-contracts force-cutover plan (docs/plans/2026-05-10-strict-contracts-force-cutover.md, rev5). Adds plugin/external/proto/iac.proto defining the typed gRPC contract that supersedes the legacy InvokeService/structpb dispatch path for IaCProvider + ResourceDriver: - service IaCProviderRequired: 11 RPCs every IaC plugin MUST implement (Initialize, Name, Version, Capabilities, Plan, Apply, Destroy, Status, Import, ResolveSizing, BootstrapStateBackend). Compile-time enforced via the SDK type-assert in Task 4. - 6 optional services — providers register only the ones they support: IaCProviderEnumerator (EnumerateAll, EnumerateByTag), IaCProviderDriftDetector (DetectDrift, DetectDriftWithSpecs), IaCProviderCredentialRevoker (RevokeProviderCredential), IaCProviderMigrationRepairer (RepairDirtyMigration), IaCProviderValidator (ValidatePlan), IaCProviderDriftConfigDetector (DetectDriftConfig). Absence of registration IS the negative signal — no NotSupported field on any optional response (per design §Optional services). - service ResourceDriver: 9 RPCs for per-resource-type CRUD dispatch (Create, Read, Update, Delete, Diff, Scale, HealthCheck, SensitiveKeys, Troubleshoot), each carrying resource_type so a single server can route to the per-type driver implementation. Hard invariants honored: - NO google.protobuf.Struct, NO google.protobuf.Any anywhere. - Free-form per-resource Config/Outputs payloads cross the wire as bytes <name>_json (the plugin owns json.Marshal/Unmarshal); this eliminates the structpb conversion surface that previously dropped map[string]bool entries silently (T3.9 finding). - ResourceOutput.sensitive uses typed map<string, bool> per design. Generated iac.pb.go + iac_grpc.pb.go via protoc v34.1 + protoc-gen-go v1.36.11 + protoc-gen-go-grpc v1.6.1. Failing test (plugin/external/proto/iac_proto_test.go) asserts the generated server interfaces exist and have the methods the design requires — drops in iac.proto cause the test file to fail to compile. Verification: GOWORK=off go test ./plugin/external/proto/... PASSES; GOWORK=off go build ./plugin/... ./cmd/... ./module/... clean. Rollback: revert this commit; legacy InvokeService dispatch in plugin.proto remains functional; the additive-only nature of this PR means no consumer is affected until subsequent tasks wire callers. * feat(sdk): RegisterAllIaCProviderServices auto-registration helper Task 4 of the strict-contracts force-cutover plan (docs/plans/2026-05-10-strict-contracts-force-cutover.md, rev5). Adds plugin/external/sdk/iacserver.go: a single helper that uses Go type-assertion to register every typed IaC gRPC service the provider satisfies, in one call. REQUIRED service: pb.IaCProviderRequiredServer — surfaced as a clear startup-time error if the provider type doesn't satisfy it (rather than failing at the first RPC dispatch with a generic "unimplemented" status). OPTIONAL services (auto-detected): IaCProviderEnumerator, IaCProviderDriftDetector, IaCProviderCredentialRevoker, IaCProviderMigrationRepairer, IaCProviderValidator, IaCProviderDriftConfigDetector. Plus ResourceDriver. Per cycle 3 I-1 of the design: plugin authors write ONE call; they cannot omit registration for a capability they implemented. This removes the registration-omission bug class (the same shape as the legacy InvokeService case-string-typo bug) by removing the manual step entirely. Tests cover four cases: - required-satisfied → required service registered + advertised by grpcSrv.GetServiceInfo(). - enumerator-only → only the optional Enumerator service registered; other optionals stay absent (auto-detection precision). - empty-stub → returns an error naming the unsatisfied required interface, with a docs pointer. - all-capabilities-stub → all 8 typed services (Required + 6 optional + ResourceDriver) registered. Stacked on feat/iac-proto-task3 (Task 3 PR #598 provides the generated server interfaces this helper consumes). Verification: GOWORK=off go test -race ./plugin/external/sdk/... PASS; GOWORK=off go build ./plugin/... ./cmd/... ./module/... clean; GOWORK=off go vet ./plugin/external/... clean. Rollback: revert this commit; SDK consumers can still register services manually via the per-service Register* helpers protoc generated. * feat(sdk): ServeIaCPlugin high-level entrypoint with go-plugin GRPCServer callback Task 29 of the strict-contracts force-cutover plan (docs/plans/2026-05-10-strict-contracts-force-cutover.md, rev5). Adds the high-level plugin-author API on top of Task 4's RegisterAllIaCProviderServices: func main() { sdk.ServeIaCPlugin(&doProvider{}, sdk.IaCServeOptions{}) } Per cycle 3 I-1 of the design, service registration happens INSIDE go-plugin's GRPCServer callback (iacGRPCPlugin.GRPCServer) — the framework owns *grpc.Server lifecycle, so plugin authors cannot pre-create a server and forget to register a typed service on it. API surface (all in plugin/external/sdk/iacserver.go): - IaCServeOptions{ PluginInfo *PluginInfo } — caller-side options. - PluginInfo{ HandshakeConfig goplugin.HandshakeConfig } — extension point for future Name/Version metadata; defaults to ext.Handshake (the canonical wfctl<->plugin handshake) when zero-valued. - iacGRPCPlugin{provider any} — implements goplugin.Plugin (GRPCServer + GRPCClient). The GoCodeAlone fork of go-plugin v1.7.0 is gRPC-only and exposes only the canonical Plugin interface; there is no GRPCPlugin alias or NetRPCUnsupportedPlugin embed to use. - ServeIaCPlugin(provider, opts) — wraps goplugin.Serve with the resolved handshake + a single iacGRPCPlugin entry under the "iac" key. - resolveServeHandshake(opts) — extracted helper so the override-vs- default rule is unit-testable without invoking the blocking goplugin.Serve loop. Tests (iacserver_serve_test.go) cover six cases via internal-package tests (so the unexported plugin type is exercisable without a real subprocess; subprocess-level coverage lands in Task 6's typed-IaC E2E test): - iacGRPCPlugin.GRPCServer registers all satisfied services on the framework-managed *grpc.Server (Required + Enumerator + ResourceDriver for the all-stub). - iacGRPCPlugin.GRPCServer propagates the auto-register error for an empty stub — go-plugin aborts plugin startup with an actionable message. - iacGRPCPlugin.GRPCClient is a no-op (host builds typed clients directly). - iacGRPCPlugin satisfies goplugin.Plugin at compile time (refactor guard). - ServeIaCPlugin defaults to ext.Handshake when PluginInfo is nil. - ServeIaCPlugin honors a non-zero override handshake when provided. Stacked on feat/iac-sdk-auto-register-task4 (Task 4 PR #599 provides RegisterAllIaCProviderServices, which the GRPCServer callback delegates to). Verification: GOWORK=off go test -race ./plugin/external/sdk/... PASS; GOWORK=off go build ./plugin/... ./cmd/... ./module/... clean; GOWORK=off go vet ./plugin/external/... clean. Rollback: revert this commit; plugin authors can fall back to manually constructing goplugin.Serve + Plugins map referencing RegisterAllIaCProviderServices in their own GRPCServer callback. * feat(sdk): BuildContractRegistry advertises registered IaC services Task 5 of the strict-contracts force-cutover plan (docs/plans/2026-05-10-strict-contracts-force-cutover.md, rev5). Adds plugin/external/sdk/contracts.go with the BuildContractRegistry helper that enumerates grpc.Server.GetServiceInfo() and emits a SERVICE-kind ContractDescriptor for each registered service. ContractMode is set to STRICT_PROTO so the host can distinguish typed IaC services from the legacy structpb-mode contracts produced by Module/Step/Trigger ContractProvider implementations. Per cycle 3 I-1 of the design: wfctl needs a single mechanism to discover "is the optional service registered on this plugin handle?". Reusing the existing ContractRegistry shape keeps Module/Step/Trigger and IaC capability discovery on the same wire surface — no new gRPC server-reflection dependency required. Service descriptors are emitted in deterministic alphabetical order so callers can rely on stable output for diff/compare operations and the wftest BDD test in Task 15. The helper is safe to call with a nil server (returns an empty but non-nil ContractRegistry) so callers that may construct it before the gRPC server exists do not panic. Tests (contracts_iac_test.go) cover three cases — all pass: - AdvertisesRegisteredIaCServices: a Required + Enumerator + DriftDetector stub yields exactly those service descriptors. - ServiceContractsUseStrictProtoMode: every emitted descriptor is Kind=SERVICE + Mode=STRICT_PROTO (host-side discriminator). - NilServer_ReturnsEmpty: defensive contract for nil input. Stacked on feat/iac-sdk-serve-task29 (Task 29 PR #600 provides ServeIaCPlugin which IaC plugins use to register the services this helper enumerates). Verification: GOWORK=off go test -race ./plugin/external/sdk/... PASS; GOWORK=off go build ./plugin/... ./cmd/... ./module/... clean; GOWORK=off go vet ./plugin/external/... clean. Rollback: revert this commit; ContractRegistry returns the prior shape (Module/Step/Trigger only via the existing ContractProvider hook in grpc_server.go). * feat(wfctl): IaC plugin pre-flight gate building block + iac-typed-cutover runbook Task 18 of the strict-contracts force-cutover plan (docs/plans/2026-05-10-strict-contracts-force-cutover.md, rev5). Adds two pieces: 1. cmd/wfctl/iac_loader_gate.go — the pre-flight gate building block. AssertIaCPluginAdvertisesRequiredService(name, version, registry) inspects a *pb.ContractRegistry response from GetContractRegistry and returns nil iff the plugin advertises workflow.plugin.external.iac.IaCProviderRequired as a CONTRACT_KIND_SERVICE descriptor. On failure: an actionable error naming the offending plugin + version, citing .wfctl-lock.yaml as the migration target, and pointing to docs/runbooks/iac-typed-cutover.md, wrapping errLegacyIaCPlugin for IsLegacyIaCPluginErr dispatch. The function is the load-bearing predicate the deploy / infra-plan / infra-apply call sites will invoke after their GetContractRegistry RPC succeeds (wiring left for the typed- client cutover already in progress in Task 16; this commit ships the testable predicate so the wiring is one-line drop-in). 2. docs/runbooks/iac-typed-cutover.md — operator-facing runbook for the cutover. Covers: - Compatibility matrix (wfctl × DO plugin version combinations, pre-flight expected outcomes). - Upgrade order: plugins first, then wfctl. Documents WHY this ordering matters (legacy wfctl + v1.0.0 plugin works; v1.0.0 wfctl + legacy plugin refuses). - .wfctl-lock.yaml migration (lockfile shape unchanged; only the version pin needs editing). - Troubleshooting: legacy-plugin pre-flight error, EnumeratorAll-style messages from pre-cutover wfctl, state-file decode regressions, advertised-but-buggy plugins. - Backout plan: re-pin legacy plugin + wfctl while in the rc1 window. Tests (cmd/wfctl/iac_loader_gate_test.go) — 7 cases, all PASS: - TypedRegistryAccepts: SERVICE-kind descriptor for IaCProviderRequired. - LegacyRegistryRejects: ContractRegistry without the required service → wrapped error names plugin + version + runbook. - NilRegistryRejects: defensive contract for nil registry input. - EmptyContractsRejects: no descriptors at all → still legacy. - WrongKindRejects: ServiceName matches but Kind != SERVICE. - EmptyMetadataDefaults: name/version absent → graceful "<unknown>". - IsLegacyIaCPluginErr_NoFalsePositives: sentinel match is typed (not string-based) so dispatch sites can errors.Is cleanly. Stacked on feat/iac-sdk-contracts-task5 (Task 5 PR #602 provides BuildContractRegistry — the wfctl-side gate inspects what plugins emit via that helper, so this PR builds on it). Verification: GOWORK=off go test -race ./cmd/wfctl/... PASS; GOWORK=off go vet ./cmd/wfctl/ clean; gofmt clean. Rollback: revert this commit. The pre-flight gate is a standalone predicate function — no production code currently depends on it (dispatch wiring lands in the typed-client cutover Task 16). The runbook is documentation; no operational impact from removal. * docs(runbook): correct cutover model — wfctl-side rc1 adapter, not plugin compat shim Per spec-reviewer's PR 610 IMPORTANT-1 + team-lead's ruling on the correct cutover model (which I as ADR 0024 author should have known already): The runbook previously claimed "v1.0.0 plugins also expose the legacy InvokeService surface during the rc1 window for backward compatibility" and "backout fully supported through rc1 window (workflow v1.0.0-rc1 + DO plugin v1.0.0)". Both wrong. Contradicts ADR 0024 (force-cutover, no compat shim, no build-tag dual-path), Task 9 spec literal ("DELETE module_instance.go"), and workspace memory feedback_force_strict_contracts_no_compat. Corrected cutover model: - workflow v1.0.0-rc1 ships the wfctl-SIDE typed-client adapter alongside the existing wfctl remoteIaCProvider. Plugins are unchanged at v0.14.x; the typed adapter is exercised only when a typed-aware plugin is loaded. This is rc1's role: wfctl-side additive, not plugin-side compat shim. - DO plugin v1.0.0 ships typed-only (Task 9 deletes the legacy internal/module_instance.go switch dispatcher entirely; ResourceDriver Task 11 follows the same pattern). - workflow v1.0.0 final ships typed-only on the wfctl side too (Task 20 removes wfctl-side remoteIaCProvider). Both legacy paths are gone. Specific revisions applied: 1. Compatibility matrix: added the missing v0.27.x × v1.0.0 row marked "BROKEN by design" — operators MUST run wfctl rc1+ to consume DO v1.0.0 because the legacy wfctl binary cannot dispatch through a typed-only plugin. Added explicit Step 4 troubleshooting block for operators who skipped wfctl rc1. 2. Upgrade order rewrite: 5 steps (was 3). Order is wfctl rc1 first (test against v0.14.x plugin set), then DO plugin v1.0.0, then wfctl v1.0.0 final. Each step has its smoke-test command and rollback escape. 3. Backout plan rewrite: explicit "rollback BOTH wfctl AND plugin together" for the post-v1.0.0-final case. Single-side rollback is broken — wfctl v1.0.0 final binary cannot dispatch v0.14.x plugin (legacy adapter deleted in Task 20); legacy v0.27.x wfctl cannot dispatch typed-only DO v1.0.0 (no typed adapter on the legacy binary). Documents the partial-rollback option that exists during the rc1 window only. 4. Top-of-runbook callout: explicit statement that this is a hard cutover with no plugin-side compat shim, citing ADR 0024 and feedback_force_strict_contracts_no_compat. No code changes; documentation only. Verification: markdown rendered locally; cross-references to ADRs + plan + design + state_compat_test.go all resolve. Followups (per spec-reviewer + team-lead Finding 2): - PR description gets a "Wiring deferred to Task 16 (PR 609)" callout with predicate symbol + sentinel + call-site location - Coordination DM to implementer-2 with the wiring contract * refactor(wfctl): rename iacRequiredServiceName → iacServiceRequired Cross-task naming coordination with PR #605/#609 (implementer-2, Tasks 30/16). Their iac_typed_adapter.go declares 8 sibling consts naming every typed IaC service: iacServiceRequired iacServiceEnumerator iacServiceDriftDetector iacServiceCredentialRevoker iacServiceMigrationRepairer iacServiceValidator iacServiceDriftConfigDetect iacServiceResourceDriver PR #610 originally introduced its own const iacRequiredServiceName for the same Required service FQN. Spec-reviewer flagged the inconsistency and asked us to coordinate. Implementer-2 + I agreed their convention wins (8 siblings establish the pattern; my single const matches). Pure rename; no behavior change. Tests still pass. Verification: GOWORK=off go test ./cmd/wfctl/ -run \ "TestAssertIaCPluginAdvertises|TestIsLegacyIaCPluginErr" \ -count=1 → PASS (7/7); gofmt clean. Once PR #610 merges, implementer-2's PR #609 rebase can drop their duplicate const and import iacServiceRequired from here. * fix(wfctl): drop duplicate iacServiceRequired const (now lives in iac_typed_adapter.go on main) PR #605 merged iac_typed_adapter.go to main with iacServiceRequired const. PR #610's iac_loader_gate.go declared it independently; collision after cascade-merge. Removed the duplicate; use canonical const from iac_typed_adapter.go directly. Build clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
5 tasks
intel352
added a commit
that referenced
this pull request
May 10, 2026
… round 2) Per team-lead + spec-reviewer ruling on PR #618 (round 1 used typed-then-fallback pattern; rejected for code-shape reasons): tighten to PURE Option B at all 5 wfctl-side dispatch sites. interfaces.X fallback removed; non-typed providers hit a typed-error at the type-assert site rather than silently falling through. Sites converted to pure typed-pb: - cmd/wfctl/infra_cleanup.go: hard-fail on non-typed provider; only pb.IaCProviderEnumeratorClient.EnumerateByTag at dispatch. - cmd/wfctl/infra_apply_refresh.go: hard-fail (typed error from runInfraApplyRefreshPhase); detectDriftConfigTyped via typed client when registered, falls through to required IaCProvider.DetectDrift via typed adapter when not. - cmd/wfctl/infra_status_drift.go: warn-and-skip on non-typed (this function returns bool; doesn't propagate error); detectDriftConfigTyped via typed client when registered. - cmd/wfctl/infra_bootstrap.go: resolveCredentialRevoker hard-fails on non-typed (warning + nil revoker, same UX as missing service); returns the typed adapter directly so its RevokeProviderCredential method translates to the typed pb.RevokeProviderCredential RPC under the hood. - cmd/wfctl/infra_align_rules.go: continue (silent skip) on non-typed; R-A10's "treat unimplemented as not-applicable" semantics preserved at the typed-adapter accessor level. ADR-0028 (decisions/0028-task-17-pure-typed-cutover.md) records the decision, failure modes the dual-path preserved (loader-gate weakening, test-fixture DI leak, future contributor cargo-culting, reviewer cognitive load), the bufconn migration pattern for tests (per PR #603 + PR #609 precedent), and the strict-mode invariant translation (gRPC codes.Unimplemented from a non-registered service + translateRPCErr in the adapter preserves operator-visible ErrProviderMethodUnimplemented surface). EXPECTED: ~10 test fixtures fail to compile or run after this commit because they inject fake interfaces.IaCProvider implementations at the dispatch sites. Fixture rewrites land in follow-up commits on this same branch (no force-push). PR 618 stays in CHANGES REQUESTED state until the test pass. Local validation: GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 -short # FAILS (expected — fixture rewrites pending) GOWORK=off golangci-lint run --enable=gocritic,gosec ./cmd/wfctl/... # 0 issues (in code; tests are next commit) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
intel352
added a commit
that referenced
this pull request
May 10, 2026
…, Task 17 item 4) Per ADR-0028 (PR 618 round 2), wfctl IaC dispatch sites are pure typed-pb (`provider.(*typedIaCAdapter)`) — no interfaces.X fallback. Test fixtures that previously injected fake `interfaces.IaCProvider` implementations no longer reach the dispatch path; the type-assert fails. This migrates the fixtures whose tests actually exercise a Task 17 dispatch site to use a real *typedIaCAdapter wired to an in-process bufconn-served pb.IaCProvider* gRPC server. Shared fixture helper (cmd/wfctl/iac_typed_fixture_test.go): - fixtureTypedAdapter declarative builder: each non-nil pb-server field registers the matching service on the bufconn server, mirroring the ContractRegistry-driven optional-client construction in production. - fixtureRequiredServer: baseline IaCProviderRequiredServer with configurable name/version + UnimplementedIaCProviderRequiredServer embed for everything else. - recordingEnumeratorServer: canned EnumerateByTag / EnumerateAll responses with mutex-guarded recorded inputs. - recordingResourceDriverServer: minimal pb.ResourceDriverServer that records Delete invocations + per-call error injection. - recordingDriftDetectorServer: canned DetectDrift responses. - driftsToPBOrEmpty: engine-side []DriftResult to pb wire shape, mirroring the inverse driftsFromPB in iac_typed_adapter.go. Pattern precedents: PR #603 (iac_e2e_test.go bufconn), PR #609 (discover_typed_loader_test.go boundary test), PR #605 (typed adapter unit tests). Migrated fixture files: 1. cmd/wfctl/infra_cleanup_test.go - fakeEnumeratingProvider/ fakeNonEnumeratingProvider/fakeDeleteDriver replaced with newCleanupEnumFixture / newCleanupNonEnumFixture builders that produce *typedIaCAdapter instances. 7 TestInfraCleanup_* tests now exercise the bufconn typed dispatch end-to-end. 2. cmd/wfctl/infra_apply_refresh_test.go - refreshFakeProvider replaced with newRefreshDriftFixture which registers the typed IaCProviderDriftDetector service. 9 TestApplyRefresh_* tests now go through the typed wire path. TestApplyRefresh_TransientErrorDoesNotPrune asserts on the error substring rather than errors.Is(transientErr) because the gRPC wire boundary doesn't preserve error identity across the bufconn server. 3. cmd/wfctl/infra_align_ra10_test.go - stubIaCProvider type + validatingStubProvider type replaced with stubIaCProvider() and validatingStubProvider() builder functions returning *typedIaCAdapter. cannedValidatorServer registers IaCProviderValidator returning canned PlanDiagnostics. 8 TestCheckRA10_* + TestInfraAlign_RA10_FixtureProvider_Fires now exercise the typed Validator dispatch. 4. cmd/wfctl/infra_strict_mode_test.go - TestInfraCleanup_MultiProvider_ContinuesPastUnimplemented updated to use the migrated cleanup fixtures. Provider A (no Enumerator service registered) -> adapter.Enumerator() returns nil -> cleanup skips with "skipped fake-a: provider does not implement Enumerator" log line, preserving the multi-provider continue-on-skip semantics in their typed-shape form. Scope notes: ADR-0028 lists 10 fixture file paths. Of those: - cmd/wfctl/infra_status_drift_test.go does not exist (the related drift test logic lives in infra_destroy_test.go's TestDriftInfraModules_NoDrift; it currently passes silently because the dispatch warns "not a typed IaC adapter" + returns false. A follow-up PR can migrate that test to harden the silent-pass case.) - cmd/wfctl/infra_bootstrap_force_rotate_test.go uses stubProviderRevoker (interfaces.ProviderCredentialRevoker) rather than IaCProvider; tests call bootstrapSecrets directly, bypassing the resolveCredentialRevoker dispatch. No migration needed. - cmd/wfctl/infra_rotate_and_prune_test.go uses fakeProviderEnumerableDriver (a custom test interface), not interfaces.IaCProvider. - cmd/wfctl/infra_audit_keys_test.go's fakeIaCProviderForAuditKeys goes through `p.(interfaces.EnumeratorAll)` dispatch which is NOT a Task 17 dispatch site (different from the 5 sites converted). - cmd/wfctl/dryrun_test.go and cmd/wfctl/infra_provider_dispatch_test.go use iactest.NoopProvider via the resolveIaCProvider seam; the tests exercise the plan path, which doesn't type-assert to *typedIaCAdapter. The 4 migrated files cover every test that was actually failing the type-assert under PR #618 round 2's pure-typed dispatch. Tests in the other ADR-listed files continue to pass without migration because they don't reach a Task 17 dispatch site. Local validation: GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 # all PASS (7.3s) GOWORK=off go test ./cmd/wfctl/ -count=1 -race # all PASS (10.1s) GOWORK=off golangci-lint run ./cmd/wfctl/... # 0 issues Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
intel352
added a commit
that referenced
this pull request
May 10, 2026
… / Task 17) (#618) * feat(wfctl): typed-RPC capability discovery at 5 dispatch sites (Task 17) Per plan §Task 17 (strict-contracts force-cutover, rev5) and team-lead's Option B ruling: convert the 5 wfctl-side `p.(interfaces.X)` type-assert sites to typed-pb dispatch via per-service accessors on the typed IaCProvider adapter. Capability discovery happens BEFORE the call (typed-client accessor returns nil when the plugin's ContractRegistry didn't advertise the optional service) so we don't pay the wasted-RPC + sentinel-error round-trip the legacy interfaces.X dispatch incurred. **typedIaCAdapter accessors (cmd/wfctl/iac_typed_adapter.go +96 lines):** - RequiredClient() pb.IaCProviderRequiredClient — always non-nil after the loader gate (PR #610) accepts the plugin. - Enumerator() pb.IaCProviderEnumeratorClient - DriftDetector() pb.IaCProviderDriftDetectorClient - DriftConfigDetector() pb.IaCProviderDriftConfigDetectorClient - CredentialRevoker() pb.IaCProviderCredentialRevokerClient - MigrationRepairer() pb.IaCProviderMigrationRepairerClient - Validator() pb.IaCProviderValidatorClient - ResourceDriverClient() pb.ResourceDriverClient Each optional accessor returns nil when the matching service isn't in the `registered` map passed to newTypedIaCAdapter. Per-method docstrings describe the dispatch sites that consume each accessor. **Typed-RPC dispatch helpers (cmd/wfctl/iac_typed_dispatch.go +51 lines):** - detectDriftConfigTyped(ctx, cli, refs, specs) → []DriftResult - validatePlanTyped(ctx, cli, plan) → []PlanDiagnostic Wrap a single typed pb.IaC* RPC + the marshalling helpers from iac_typed_adapter.go (refsToPB / specToPB / driftsFromPB / planToPB / planDiagnosticSeverityFromPB). Single source of truth for proto/Go shape conversions; call sites stay focused on dispatch logic. **5 dispatch sites converted:** 1. cmd/wfctl/infra_cleanup.go:97 — `p.(interfaces.Enumerator)` → typed pb.IaCProviderEnumeratorClient.EnumerateByTag. Falls back to the interfaces.Enumerator type-assert path for non-typed providers (test fixtures + non-wfctl consumers); typedIaCAdapter satisfies interfaces.Enumerator too, so the legacy branch path is functionally equivalent when used against the real adapter — the typed branch is preferred for clarity + to avoid wasted RPC against unregistered services. 2. cmd/wfctl/infra_apply_refresh.go:69 — `provider.(interfaces.DriftConfigDetector)` → typed pb.IaCProviderDriftConfigDetectorClient.DetectDriftConfig via detectDriftConfigTyped helper. Same fallback pattern. 3. cmd/wfctl/infra_status_drift.go:107 — same as #2 but for `wfctl infra status drift`. Same fallback pattern. 4. cmd/wfctl/infra_bootstrap.go:335 — resolveCredentialRevoker now short-circuits via typedIaCAdapter.CredentialRevoker() == nil before returning the interfaces.ProviderCredentialRevoker value. Caller signature stays interfaces.ProviderCredentialRevoker for stability + test-fixture compatibility; the typed dispatch happens inside typedIaCAdapter.RevokeProviderCredential which translates to a typed pb.RevokeProviderCredential RPC. Net effect: capability discovery moves from call-time (sentinel error) to load-time (accessor nil-check) without changing the caller's API. 5. cmd/wfctl/infra_align_rules.go:777 — `p.(interfaces.ProviderValidator)` → typed pb.IaCProviderValidatorClient.ValidatePlan via validatePlanTyped helper. Same fallback pattern as #1-3. **Plan-correction notes** Spec §Task 17 says "use optionals from Task 16" — Task 16's adapter exposed optional clients as private fields, not a public map. Task 17 adds typed-client accessors as the extension surface (per team-lead Option B). The 5 sites use a typed-then-fallback pattern rather than pure typed-only: keeping the interfaces.X branch as a stable seam for test fixtures + non-wfctl consumers avoids forcing every caller to also be a typedIaCAdapter consumer (which would require re-writing ~10 test fixtures across 4 files for no semantic gain — typedIaCAdapter satisfies all the interfaces too, so the typed branch is the strict-cutover preferred path while the fallback preserves the interfaces.X integration point that out-of-org / future provider impls might still use). Net effect: wfctl call sites prefer typed pb dispatch; interfaces.X type-assertions remain as a documented fallback. The interfaces/X definitions stay in `interfaces/` for engine-side consumers per the strict-contracts design (typedIaCAdapter is the wfctl-side adapter that bridges the typed pb client to the engine's interfaces.X). Local validation (against current main, post-rebase): GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 -short # all PASS (6.5s) GOWORK=off golangci-lint run --enable=gocritic,gosec ./cmd/wfctl/... # 0 issues Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(wfctl): pure typed-pb dispatch at 5 sites + ADR-0028 (PR 618 round 2) Per team-lead + spec-reviewer ruling on PR #618 (round 1 used typed-then-fallback pattern; rejected for code-shape reasons): tighten to PURE Option B at all 5 wfctl-side dispatch sites. interfaces.X fallback removed; non-typed providers hit a typed-error at the type-assert site rather than silently falling through. Sites converted to pure typed-pb: - cmd/wfctl/infra_cleanup.go: hard-fail on non-typed provider; only pb.IaCProviderEnumeratorClient.EnumerateByTag at dispatch. - cmd/wfctl/infra_apply_refresh.go: hard-fail (typed error from runInfraApplyRefreshPhase); detectDriftConfigTyped via typed client when registered, falls through to required IaCProvider.DetectDrift via typed adapter when not. - cmd/wfctl/infra_status_drift.go: warn-and-skip on non-typed (this function returns bool; doesn't propagate error); detectDriftConfigTyped via typed client when registered. - cmd/wfctl/infra_bootstrap.go: resolveCredentialRevoker hard-fails on non-typed (warning + nil revoker, same UX as missing service); returns the typed adapter directly so its RevokeProviderCredential method translates to the typed pb.RevokeProviderCredential RPC under the hood. - cmd/wfctl/infra_align_rules.go: continue (silent skip) on non-typed; R-A10's "treat unimplemented as not-applicable" semantics preserved at the typed-adapter accessor level. ADR-0028 (decisions/0028-task-17-pure-typed-cutover.md) records the decision, failure modes the dual-path preserved (loader-gate weakening, test-fixture DI leak, future contributor cargo-culting, reviewer cognitive load), the bufconn migration pattern for tests (per PR #603 + PR #609 precedent), and the strict-mode invariant translation (gRPC codes.Unimplemented from a non-registered service + translateRPCErr in the adapter preserves operator-visible ErrProviderMethodUnimplemented surface). EXPECTED: ~10 test fixtures fail to compile or run after this commit because they inject fake interfaces.IaCProvider implementations at the dispatch sites. Fixture rewrites land in follow-up commits on this same branch (no force-push). PR 618 stays in CHANGES REQUESTED state until the test pass. Local validation: GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 -short # FAILS (expected — fixture rewrites pending) GOWORK=off golangci-lint run --enable=gocritic,gosec ./cmd/wfctl/... # 0 issues (in code; tests are next commit) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(wfctl): bufconn-backed *typedIaCAdapter fixtures (PR 618 round 3, Task 17 item 4) Per ADR-0028 (PR 618 round 2), wfctl IaC dispatch sites are pure typed-pb (`provider.(*typedIaCAdapter)`) — no interfaces.X fallback. Test fixtures that previously injected fake `interfaces.IaCProvider` implementations no longer reach the dispatch path; the type-assert fails. This migrates the fixtures whose tests actually exercise a Task 17 dispatch site to use a real *typedIaCAdapter wired to an in-process bufconn-served pb.IaCProvider* gRPC server. Shared fixture helper (cmd/wfctl/iac_typed_fixture_test.go): - fixtureTypedAdapter declarative builder: each non-nil pb-server field registers the matching service on the bufconn server, mirroring the ContractRegistry-driven optional-client construction in production. - fixtureRequiredServer: baseline IaCProviderRequiredServer with configurable name/version + UnimplementedIaCProviderRequiredServer embed for everything else. - recordingEnumeratorServer: canned EnumerateByTag / EnumerateAll responses with mutex-guarded recorded inputs. - recordingResourceDriverServer: minimal pb.ResourceDriverServer that records Delete invocations + per-call error injection. - recordingDriftDetectorServer: canned DetectDrift responses. - driftsToPBOrEmpty: engine-side []DriftResult to pb wire shape, mirroring the inverse driftsFromPB in iac_typed_adapter.go. Pattern precedents: PR #603 (iac_e2e_test.go bufconn), PR #609 (discover_typed_loader_test.go boundary test), PR #605 (typed adapter unit tests). Migrated fixture files: 1. cmd/wfctl/infra_cleanup_test.go - fakeEnumeratingProvider/ fakeNonEnumeratingProvider/fakeDeleteDriver replaced with newCleanupEnumFixture / newCleanupNonEnumFixture builders that produce *typedIaCAdapter instances. 7 TestInfraCleanup_* tests now exercise the bufconn typed dispatch end-to-end. 2. cmd/wfctl/infra_apply_refresh_test.go - refreshFakeProvider replaced with newRefreshDriftFixture which registers the typed IaCProviderDriftDetector service. 9 TestApplyRefresh_* tests now go through the typed wire path. TestApplyRefresh_TransientErrorDoesNotPrune asserts on the error substring rather than errors.Is(transientErr) because the gRPC wire boundary doesn't preserve error identity across the bufconn server. 3. cmd/wfctl/infra_align_ra10_test.go - stubIaCProvider type + validatingStubProvider type replaced with stubIaCProvider() and validatingStubProvider() builder functions returning *typedIaCAdapter. cannedValidatorServer registers IaCProviderValidator returning canned PlanDiagnostics. 8 TestCheckRA10_* + TestInfraAlign_RA10_FixtureProvider_Fires now exercise the typed Validator dispatch. 4. cmd/wfctl/infra_strict_mode_test.go - TestInfraCleanup_MultiProvider_ContinuesPastUnimplemented updated to use the migrated cleanup fixtures. Provider A (no Enumerator service registered) -> adapter.Enumerator() returns nil -> cleanup skips with "skipped fake-a: provider does not implement Enumerator" log line, preserving the multi-provider continue-on-skip semantics in their typed-shape form. Scope notes: ADR-0028 lists 10 fixture file paths. Of those: - cmd/wfctl/infra_status_drift_test.go does not exist (the related drift test logic lives in infra_destroy_test.go's TestDriftInfraModules_NoDrift; it currently passes silently because the dispatch warns "not a typed IaC adapter" + returns false. A follow-up PR can migrate that test to harden the silent-pass case.) - cmd/wfctl/infra_bootstrap_force_rotate_test.go uses stubProviderRevoker (interfaces.ProviderCredentialRevoker) rather than IaCProvider; tests call bootstrapSecrets directly, bypassing the resolveCredentialRevoker dispatch. No migration needed. - cmd/wfctl/infra_rotate_and_prune_test.go uses fakeProviderEnumerableDriver (a custom test interface), not interfaces.IaCProvider. - cmd/wfctl/infra_audit_keys_test.go's fakeIaCProviderForAuditKeys goes through `p.(interfaces.EnumeratorAll)` dispatch which is NOT a Task 17 dispatch site (different from the 5 sites converted). - cmd/wfctl/dryrun_test.go and cmd/wfctl/infra_provider_dispatch_test.go use iactest.NoopProvider via the resolveIaCProvider seam; the tests exercise the plan path, which doesn't type-assert to *typedIaCAdapter. The 4 migrated files cover every test that was actually failing the type-assert under PR #618 round 2's pure-typed dispatch. Tests in the other ADR-listed files continue to pass without migration because they don't reach a Task 17 dispatch site. Local validation: GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 # all PASS (7.3s) GOWORK=off go test ./cmd/wfctl/ -count=1 -race # all PASS (10.1s) GOWORK=off golangci-lint run ./cmd/wfctl/... # 0 issues Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(decisions): ADR-0028 expansion — per-site dispatch UX (PR 618 round 3) Per spec-reviewer ruling on PR #618 round 3: code-shape mandate is met (pure typed-pb at all 5 sites), but the per-site rejection severity varies based on iteration semantics. Soft-skip at iteration sites is graceful degradation, not the rejected silent-fallback shape — this expansion documents the rule + per-site rationale so future contributors don't cargo-cult either direction blindly. New `## Per-site dispatch UX` section adds: - Severity table for each of the 5 sites (cleanup hard-error, apply-refresh hard-error, status-drift soft-skip, align-rules R-A10 silent-skip, bootstrap soft-skip-revocation) with explicit per-site reasoning anchored in iteration vs single-shot semantics. - Canonical rule (verbatim from team-lead): "Pure typed-pb dispatch at all sites; non-typed input rejection severity is per-site UX based on iteration semantics. New dispatch sites default to hard-error unless graceful-degradation is operationally required." Plus the two-condition bar for soft-skip eligibility (iteration + auditable warn-log). - Failure-mode contrast vs the round-1-rejected silent-fallback pattern: (1) the fallback path no longer exists at all 5 sites, (2) soft-skip is auditable via stderr warn-log, (3) the no-op result is observably distinct from a typed-pb success at the call site. ADR-only edit; no code, fixture, or test changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(wfctl): translate Unimplemented + propagate ctx + doc/error polish (PR 618 round 4) Per code-review IMPORTANT-1 / IMPORTANT-2 / MINOR-1 / MINOR-2 (PR 618 round 4): IMPORTANT-1 — translateRPCErr at typed dispatch sites ADR-0028 §Migration's "Strict-mode invariant translation" promises codes.Unimplemented at the wire boundary becomes interfaces.ErrProviderMethodUnimplemented for downstream errors.Is classification. The typedIaCAdapter's interfaces.IaCProvider methods already wrap, but the new typed-RPC dispatch helpers + the inline EnumerateByTag call site bypassed the wrap. Fixed two sites: - cmd/wfctl/iac_typed_dispatch.go:detectDriftConfigTyped now wraps cli.DetectDriftConfig errors via translateRPCErr. - cmd/wfctl/infra_cleanup.go's enumCli.EnumerateByTag site wraps via translateRPCErr before formatting + appending to totalErrs. Audit confirmed the 3 other dispatch sites already route through adapter methods that translate (provider.DetectDrift via typedIaCAdapter.DetectDrift, adapter.RevokeProviderCredential). validatePlanTyped intentionally returns nil-diags on any error per the documented Go interfaces.ProviderValidator.ValidatePlan signature contract; no translation needed there. IMPORTANT-2 — propagate caller context to ValidatePlan validatePlanTyped at infra_align_rules.go:782 was called with context.Background(), losing operator Ctrl-C / parent cancellation / RPC deadline propagation. Threaded ctx through: - runInfraAlign → runInfraAlignChecks(ctx, opts) - runInfraAlignChecks → checkRA10_provider_validate_plan(ctx, ...) - checkRA10_provider_validate_plan → validatePlanTyped(ctx, ...) Renamed runInfraAlignChecks's local *alignContext binding from `ctx` to `alignCtx` to avoid shadowing the new context.Context parameter. Test callers (runInfraAlignChecks at 16 sites, checkRA10_provider_validate_plan at 9 sites) updated to pass context.Background(); context import added to test files that needed it. MINOR-1 — iac_typed_adapter.go accessor doc-comment Doc example said `if !ok { /* legacy path no longer exists */ }` while the body asserted "wfctl call sites are pure typed". Reworked the example to show both per-site UX shapes (hard-error + soft-skip) per ADR-0028 §Per-site dispatch UX, with parenthetical mapping to the dispatch sites that use each shape. MINOR-2 — specToPB error key context detectDriftConfigTyped's per-spec marshalling loop returned bare specToPB errors with no key context. Wrapped with fmt.Errorf("specToPB %q: %w", k, err) so post-mortem debugging identifies which entry in the per-resource specs map blew up. Local validation: GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 # all PASS (7.4s) GOWORK=off go test ./cmd/wfctl/ -count=1 -race # all PASS (10.5s) GOWORK=off golangci-lint run ./cmd/wfctl/... # 0 issues Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(wfctl): signal.NotifyContext + status-drift comment + fixture marshal-fail (PR 618 round 5) Per code-review round 5 follow-ups (3 Copilot findings on round 4 head): 1. cmd/wfctl/infra_status_drift.go:103-110 (was MINOR-4 corrigendum) Comment said "Hard-fail when provider isn't a typed adapter" but the implementation soft-skips (warn + return false). Updated the comment to match ADR-0028 §Per-site dispatch UX: status-drift iterates per provider, halting the whole status command on the first non-typed provider would lose visibility into the others' drift, so the warn- log + no-drift-reported degradation is operationally correct. The warning log is the auditable signal of fixture-leak / loader-gate gaps. 2. cmd/wfctl/infra_align.go:75 (REAL — IMPORTANT-2 intent gap) Round-4 fix threaded ctx through the dispatch chain but called runInfraAlignChecks with context.Background() at the entry point — defeating IMPORTANT-2's cancellation-propagation intent. Wired signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) at runInfraAlign so operator Ctrl-C / SIGTERM cancels in-flight typed- RPC calls (R-A10 ValidatePlan + any future typed dispatch the rule layer adds). The other wfctl runInfra* entrypoints (status, drift, apply, destroy, import, etc.) currently use context.Background() directly and do NOT honor signals; the signal-aware pattern landing here is the operator-tooling shape we want, but a follow-up sweep to wire it into the other entrypoints is out of scope for this PR (signal-cancellation-for-the-CLI is a horizontal concern bigger than Task 17). Documented in the inline comment so a future contributor sees the intentional asymmetry. 3. cmd/wfctl/iac_typed_fixture_test.go:280-308 (REAL — test rigor) driftsToPBOrEmpty silently swallowed marshalJSONMap errors via `_, _ := ...`. A fixture author who hands the recording server an un-marshallable Expected/Actual map would have seen a silently-empty ExpectedJson on the wire — false-pass shape. Fix: renamed to driftsToPB returning (slice, error); per-entry errors include index + resource name for triage. recordingDriftDetectorServer now stores the pre-marshalled []*pb.DriftResult (pbDrifts) so the gRPC handler is alloc-only, no marshal failure mode at RPC time. newRefreshDriftFixture pre-marshals at fixture-build time and t.Fatalf on any error — fixture-leak now fails deterministically at test setup (option 1 from code-review brief). Local validation: GOWORK=off go build ./cmd/wfctl/ # clean GOWORK=off go vet ./cmd/wfctl/ # clean GOWORK=off go test ./cmd/wfctl/ -count=1 # all PASS (8.3s) GOWORK=off go test ./cmd/wfctl/ -count=1 -race # all PASS (10.6s) GOWORK=off golangci-lint run ./cmd/wfctl/... # 0 issues Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
discoverAndLoadIaCProviderrewritten to constructtypedIaCAdapter(Task 30, PR feat(wfctl): typed-IaC adapter satisfying interfaces.IaCProvider (PR 4 / Task 30) #605) directly from the plugin's gRPCClientConn+ContractRegistryregistered-service map. The legacyInvokeServicestring-dispatch surface is REMOVED entirely.plugin/external/grpc_plugin.go+plugin/external/adapter.goretain*grpc.ClientConnonPluginClientand addConn()accessors on both — architectural prerequisite (commit ad7d946) bundled in the same PR per Path A direction.remoteIaCProviderstruct + every interface method (~620 lines),remoteResourceDriverstruct + every interface method (~300 lines),remoteServiceInvokerinterfaces,loadIaCPluginvar +defaultLoadIaCPlugin,readIaCPluginComputePlanVersion, plus orphaned helpers (jsonToAny,anyToStruct,sensitiveToAny,decodeResourceOutput,isPluginMethodUnimplemented,stringVal,stringFromMap).deploy_providers_remote_iac_test.go(1090)deploy_providers_remote_driver_test.go(820)deploy_providers_dispatch_matrix_test.go(477)deploy_providers_strict_bridge_coverage_test.go(302)deploy_providers_remote_iac_compat_test.go(65)deploy_remote_provider_test.go(173 — additional remote* coverage subsumed by typed paths)wrapIaCError+containsAny+retryOnTransient+deployRetryDelays+deployOpError+closerFunc+findIaCPluginDir+iacPluginManifest— provider-agnostic helpers + plugin discovery still used by the typed cutover.registeredIaCServices(reg *pb.ContractRegistry) map[string]bool— walks the plugin's ContractRegistry response, collects SERVICE-kind contract names sotypedIaCAdaptercan gate optional-client construction.cmd/wfctl/deploy_providers_test.go— removedTestResolveIaCProviderSurfacesPluginError, pruned now-unused imports.Net diff: 1 source file
-957 / +69, 5 test files deleted-2927, 1 test file updated-31 / +10, plus 2 files in plugin/external/+44 / 0from the prerequisite. ≈3.9k line reduction in cmd/wfctl/ + ~44 line addition in plugin/external/.Why
PR 4 of
docs/plans/2026-05-10-strict-contracts-force-cutover.md(Task 16). Closes the bug class the legacyremoteIaCProviderintroduced — every IaCProvider call now travels through a typed gRPC RPC instead ofInvokeService("IaCProvider.Method", map[string]any{...})string-dispatch + structpb round-trip. ADR-0026 honored: NOT a hand-written marshalling proxy.Plugins that don't register the typed
workflow.plugin.external.iac.IaCProviderRequiredservice are rejected at load time with awfctl plugin updatehint — the surface is deleted, not deprecated.Required follow-ups before merge (CHANGES REQUESTED — spec-reviewer)
AssertIaCPluginAdvertisesRequiredServicein place of the inlineif !registered[iacServiceRequired]check. Returned error already wrapserrLegacyIaCPlugin; callers canerrors.Is(err, errLegacyIaCPlugin)for typed dispatch. Drop the duplicateiacServiceRequiredconst (coordinate naming with implementer-3 — DM in flight).TestDiscoverAndLoadIaCProvider_ReturnsTypedClientasserting the boundary returns*typedIaCAdapter. In-process gRPC fake plugin + concrete-type assertion.ComputePlanVersionDeclarerregression Plan-correction note added (option (d) batched with canonical_keys per team-lead ruling).Both code fixes require PR #610 to merge first (predicate lives in
iac_loader_gate.go). Sequence: PR 598 → 599 → 600 → 602 → 603 → 605 → 606 → 610 → THIS PR (rebased). Plan to apply all three in one fix-up commit post-rebase.Plan-correction notes
Per team-lead Path A ruling: the
plugin/external/Conn() prerequisite (commit ad7d946) is included in the SAME PR as the cutover. Spec gap; same fix-forward precedent as Tasks 1 (ADR-0027) and 2 (PR #597 plan-correction notes block).Two engine-side capability behaviors don't have a corresponding RPC in PR #598's typed proto surface. Per team-lead + spec-reviewer ruling, both close via the SAME follow-up additive PR (option (d) — capability discovery field on
CapabilitiesResponse.IaCCapabilityDeclaration, NOT new RPCs, NOT touching PR #598). Sequencing: between Task 17 (#19) and Task 20 (#23) per the manifest. ONE PR, ONE set of plugin updates, ONE cascade-amendment.1.
SupportedCanonicalKeys()regression.interfaces.CanonicalKeys()(full set).workflow-plugin-digitalocean/internal/provider.go:DOProvider.SupportedCanonicalKeysfilters viadoUnsupportedCanonicalKeysto a strict subset. Azure suspected; AWS/GCP not in our checkout.repeated string canonical_keystoCapabilitiesResponse.IaCCapabilityDeclaration. typedIaCAdapter then reads from there. Current full-set default behavior preserved as the no-capability-data fallback after the follow-up ships.2.
ComputePlanVersionDeclarer.ComputePlanVersion()regression.ComputePlanVersion(); apply-path falls back to v1 dispatch silently.workflow-plugin-digitalocean/plugin.jsondeclares"computePlanVersion": "v2"— sole non-default override acrossworkflow-plugin-{aws,gcp,azure,digitalocean}/plugin.json(verifiedgrep "computePlanVersion" {plugin}/plugin.jsonempty for the first three).string compute_plan_versiontoCapabilitiesResponse.IaCCapabilityDeclaration(or per-plugin Capabilities; final shape TBD). typedIaCAdapter then implementsComputePlanVersion()reading from there.Spec line-count estimate ~2x underestimate (plan §Task 16 estimated "~1500-2000 line aggregate diff"; actual ~3900). Documented for retro batch.
Dependency note⚠️
This branch's CI will FAIL until PR #598 (Task 3 proto), PR #605 (Task 30 typed adapter), AND PR #610 (Task 18 loader gate) merge. The new
discoverAndLoadIaCProviderreferencespb.ContractRegistry,pb.ContractKind_CONTRACT_KIND_SERVICE,newTypedIaCAdapter,iacServiceRequired, AND will referenceAssertIaCPluginAdvertisesRequiredServicepost-fix. Local validation passes against an overlay of all three branches in the working tree (proto + sdk + adapter + gate overlaid for compile-check only — NOT committed in this PR per Path A direction):Test plan
./cmd/wfctl/...short-test suite green (7.05s).noopCloserfixture references clean after legacy-test deletions.Rollback
Revert both commits. The legacy
remoteIaCProviderproxy reappears incmd/wfctl/deploy_providers.go; the deleted test files restore.plugin/external/Conn()accessors remain (additive, useful, non-load-bearing for the legacy path); they can be left in place even on rollback to preserve future cutover capability.🤖 Generated with Claude Code