feat(sdk): RegisterAllIaCProviderServices auto-registration helper (Task 4) [reopens #599]#611
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a typed gRPC contract surface for external IaC plugins and an SDK helper to auto-register all IaC-related services a provider implements, reducing the chance of registration omissions and enabling host-side capability discovery via service registration.
Changes:
- Introduce
RegisterAllIaCProviderServicesSDK helper to register required + optional IaC services (andResourceDriver) via type assertions. - Add unit tests validating required/optional service auto-registration behavior and actionable errors when required interface isn’t satisfied.
- Add the IaC proto contract and generated gRPC bindings plus compile-time conformance tests for required/optional interfaces.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/external/sdk/iacserver.go | Adds the auto-registration helper for all IaC provider/driver gRPC services. |
| plugin/external/sdk/iacserver_test.go | Tests registration behavior for required, optional, and full-capability providers. |
| plugin/external/proto/iac.proto | Defines the typed IaCProvider (required + optional) and ResourceDriver service contracts and messages. |
| plugin/external/proto/iac_proto_test.go | Compile-time assertions that generated server interfaces exist and have the expected shapes. |
| plugin/external/proto/iac_grpc.pb.go | Generated gRPC service/client/server bindings for the IaC proto contract. |
Comment on lines
+52
to
+56
| if provider == nil { | ||
| return fmt.Errorf("RegisterAllIaCProviderServices: provider is nil") | ||
| } | ||
| required, ok := provider.(pb.IaCProviderRequiredServer) | ||
| if !ok { |
Comment on lines
+57
to
+61
| return fmt.Errorf( | ||
| "RegisterAllIaCProviderServices: provider %T does not satisfy "+ | ||
| "pb.IaCProviderRequiredServer (missing methods); see "+ | ||
| "docs/plans/2026-05-10-strict-contracts-force-cutover-design.md", | ||
| provider, |
Comment on lines
+3
to
+4
| // Design: docs/plans/2026-05-10-strict-contracts-force-cutover-design.md | ||
| // Plan: docs/plans/2026-05-10-strict-contracts-force-cutover.md (Task 3) |
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.
intel352
force-pushed
the
feat/iac-sdk-auto-register-task4
branch
from
31cfd5a to
a88917b
Compare
Per cycle 4 code-review PR 611 typed-nil hardening (Copilot finding): The previous `if provider == nil` check missed the typed-nil case. A typed-nil pointer wrapped in `any` (e.g. `var p *MyProvider; sdk.RegisterAllIaCProviderServices(s, p)`) carries a non-nil interface header (type-info present), so the bare nil-check passes. The subsequent `provider.(pb.IaCProviderRequiredServer)` type assertion ALSO succeeds (the type satisfies the interface). Registration proceeds with the typed-nil receiver. First RPC dispatch panics on the nil pointer dereference. Adds a reflect-based check that rejects typed-nil pointers BEFORE any registration happens, with the same error shape as the bare nil-check (plus the underlying type for actionable diagnostic). Test (TestRegisterAllIaCProviderServices_TypedNilPointer_ReturnsError) exercises the failure mode end-to-end: a typed-nil *fullProviderStub must produce an error naming "typed-nil" + zero registered services. Verification: GOWORK=off go test -race ./plugin/external/sdk/... -run TestRegisterAllIaCProvider → PASS (5/5); gofmt clean. Rollback: revert this commit; the bare nil-check returns and typed-nil panics at first RPC dispatch (the bug class Copilot caught).
| // RegisterAll(s, p)) wraps as a non-nil interface value but | ||
| // dereferences to nil at first method call → panic. Reject early | ||
| // with the same pattern the user-visible nil-check uses. | ||
| if rv := reflect.ValueOf(provider); rv.Kind() == reflect.Ptr && rv.IsNil() { |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Per cycle 4 code-review PR 611 lint fix: govet's `inline` analyzer flagged reflect.Ptr as the deprecated-alias name. Go 1.18+ canonical is reflect.Pointer; reflect.Ptr is retained for backward compat but should not be used in new code. One-character substitution. No semantic change. Verification: GOWORK=off go test ./plugin/external/sdk/ -run TestRegisterAllIaCProvider -count=1 → PASS (5/5); golangci-lint run ./plugin/external/sdk/... → 0 issues.
intel352
added a commit
that referenced
this pull request
May 10, 2026
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
added a commit
that referenced
this pull request
May 10, 2026
…CProviderRequiredClient (PR 4 / Task 16) (#609) * feat(plugin/external): expose grpc.ClientConn via PluginClient.Conn() / 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> * feat(wfctl): typed-IaC cutover — replace remoteIaCProvider with pb.IaCProviderRequiredClient (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> * fix(wfctl): PR #609 review fixes — Copilot + Step 1 boundary test 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> --------- 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.
Reopens #599 (auto-closed by GitHub when PR #598's --delete-branch removed the stacked base branch). Same content as #599 head 31cfd5a.
Summary
Per plan §Task 4: SDK helper that uses Go type-assertion to auto-register every typed IaC service interface a provider satisfies. Required services compile-fail if missing; 6 optionals auto-detected; ResourceDriver registered.
Original review approval
Spec-reviewer + code-reviewer both APPROVED on PR #599 (held flip on cascade-CI which is now resolved by 598 merging). See #599 review comments for details.
🤖 Generated with Claude Code