feat(wfctl): typed-RPC capability discovery at 5 dispatch sites (PR 4 / Task 17)#618
Merged
Merged
Conversation
… 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>
Contributor
There was a problem hiding this comment.
Pull request overview
Extends wfctl’s typed-IaC cutover by adding typed-client capability accessors on *typedIaCAdapter and switching several optional-method dispatch sites from Go-interface type assertions to “typed-first” pb client calls with a legacy interface fallback.
Changes:
- Added typed pb client accessors on
*typedIaCAdapterfor optional IaC services (capability discovery via ContractRegistry-gated wiring). - Introduced
iac_typed_dispatch.gohelper wrappers for typed drift-config detection and plan validation RPCs. - Updated 5 wfctl call sites to prefer typed pb dispatch (with interfaces.X fallback for non-typed providers / tests).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/wfctl/infra_status_drift.go | Prefer typed drift-config detector client when available; fallback to required drift detection / legacy interface path. |
| cmd/wfctl/infra_cleanup.go | Prefer typed enumerator client for EnumerateByTag; fallback to interfaces.Enumerator. |
| cmd/wfctl/infra_bootstrap.go | Short-circuit credential-revoker availability via typed capability accessor before returning legacy interface. |
| cmd/wfctl/infra_bootstrap_force_rotate_test.go | Clarifies intent in test-double comment with Task 17 behavior. |
| cmd/wfctl/infra_apply_refresh.go | Prefer typed drift-config detector client when available; preserve legacy fallback behavior. |
| cmd/wfctl/infra_align_rules.go | Prefer typed validator client for R-A10 provider plan validation; fallback to interfaces.ProviderValidator. |
| cmd/wfctl/iac_typed_dispatch.go | Adds typed-RPC helper functions for drift-config detection and plan validation. |
| cmd/wfctl/iac_typed_adapter.go | Adds typed-client accessors (+ RequiredClient) to enable pre-call capability discovery at dispatch sites. |
… 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>
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…, 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>
Comment on lines
+181
to
+187
| that depend on `interfaces.ErrProviderMethodUnimplemented` semantics | ||
| migrate to: configure the bufconn server to NOT register the optional | ||
| service (so the typed accessor returns nil) OR configure the registered | ||
| service to return `status.Error(codes.Unimplemented, ...)` from the | ||
| RPC. The dispatch site translates these to `interfaces.ErrProviderMethodUnimplemented` | ||
| via the existing `translateRPCErr` helper in `iac_typed_adapter.go`, | ||
| preserving the operator-visible error shape the test asserts. |
Comment on lines
+35
to
+41
| pbSpecs := make(map[string]*pb.ResourceSpec, len(specs)) | ||
| for k, s := range specs { | ||
| ps, err := specToPB(s) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| pbSpecs[k] = ps |
Comment on lines
+111
to
120
| enumCli := adapter.Enumerator() | ||
| if enumCli == nil { | ||
| fmt.Fprintf(cleanupStdout, "skipped %s: provider does not implement Enumerator\n", p.Name()) | ||
| continue | ||
| } | ||
| refs, enumErr := enum.EnumerateByTag(ctx, *tag) | ||
| if enumErr != nil { | ||
| // v0.27.1: every gRPC-loaded provider satisfies interfaces.Enumerator | ||
| // after the proxy bridge, so plugins that don't actually implement | ||
| // EnumerateByTag now reach this point with a translated | ||
| // interfaces.ErrProviderMethodUnimplemented. Treat that identically | ||
| // to the negative type-assert above so the structured "skipped" | ||
| // log line is preserved for plugins that don't support tag queries. | ||
| if errors.Is(enumErr, interfaces.ErrProviderMethodUnimplemented) { | ||
| fmt.Fprintf(cleanupStdout, "skipped %s: provider does not implement Enumerator\n", p.Name()) | ||
| continue | ||
| } | ||
| fmt.Fprintf(cleanupStderr, "%s: enumerate by tag %q: %v\n", p.Name(), *tag, enumErr) | ||
| totalErrs = append(totalErrs, fmt.Errorf("%s: enumerate: %w", p.Name(), enumErr)) | ||
| resp, err := enumCli.EnumerateByTag(ctx, &pb.EnumerateByTagRequest{Tag: *tag}) | ||
| if err != nil { | ||
| fmt.Fprintf(cleanupStderr, "%s: enumerate by tag %q: %v\n", p.Name(), *tag, err) | ||
| totalErrs = append(totalErrs, fmt.Errorf("%s: enumerate: %w", p.Name(), err)) | ||
| continue |
Comment on lines
+103
to
+110
| // Per Task 17 of the strict-contracts force-cutover (ADR-0028): | ||
| // pure typed-pb dispatch — no interfaces.X fallback. Hard-fail | ||
| // when provider isn't a typed adapter so test-fixture leaks | ||
| // don't silently mask production-shape regressions. | ||
| adapter, ok := provider.(*typedIaCAdapter) | ||
| if !ok { | ||
| fmt.Printf("WARNING: provider %q (%T) is not a typed IaC adapter — re-load via discoverAndLoadIaCProvider\n", moduleRef, provider) | ||
| return false |
…und 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>
…h (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>
Comment on lines
+103
to
+110
| // Per Task 17 of the strict-contracts force-cutover (ADR-0028): | ||
| // pure typed-pb dispatch — no interfaces.X fallback. Hard-fail | ||
| // when provider isn't a typed adapter so test-fixture leaks | ||
| // don't silently mask production-shape regressions. | ||
| adapter, ok := provider.(*typedIaCAdapter) | ||
| if !ok { | ||
| fmt.Printf("WARNING: provider %q (%T) is not a typed IaC adapter — re-load via discoverAndLoadIaCProvider\n", moduleRef, provider) | ||
| return false |
Comment on lines
69
to
75
| return fmt.Errorf("no config file specified and no infra.yaml found") | ||
| } | ||
|
|
||
| findings, err := runInfraAlignChecks(opts) | ||
| findings, err := runInfraAlignChecks(context.Background(), opts) | ||
| if err != nil { | ||
| return err | ||
| } |
Comment on lines
+284
to
+288
| out := make([]*pb.DriftResult, 0, len(drifts)) | ||
| for _, d := range drifts { | ||
| expectedJSON, _ := marshalJSONMap(d.Expected) // marshalJSONMap nil-safe | ||
| actualJSON, _ := marshalJSONMap(d.Actual) | ||
| out = append(out, &pb.DriftResult{ |
…shal-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>
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
*typedIaCAdapter(Enumerator(),DriftDetector(),DriftConfigDetector(),CredentialRevoker(),MigrationRepairer(),Validator(),ResourceDriverClient(), plusRequiredClient()for symmetry). Each optional accessor returns nil when the matching pb.IaC* service wasn't in the plugin's ContractRegistry.cmd/wfctl/iac_typed_dispatch.gowith two typed-RPC helpers (detectDriftConfigTyped,validatePlanTyped) that wrap the pb.IaC* RPCs + reuse the marshalling helpers fromiac_typed_adapter.go.if x, ok := p.(interfaces.X); okto pure typed-pb dispatch via the typed adapter accessors (no interfaces.X fallback at the dispatch site). Hard-fails with a typed error whenprovider.(*typedIaCAdapter)returns false.decisions/0028-task-17-pure-typed-cutover.mdrecording the pure-typed decision + bufconn-migration pattern for test fixtures.Why
PR 4 of
docs/plans/2026-05-10-strict-contracts-force-cutover.md. Task 17 closes the residual interfaces.X dispatch surface in wfctl. Capability discovery happens at the accessor level (pre-call), eliminating wasted RPC + sentinel-error round-trips against unregistered services.Plan-correction notes (round 2 — pure typed)
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).
Round 1 of this PR (commit 63e1cdf) used a typed-then-fallback pattern: typed pb when
provider.(*typedIaCAdapter)succeeded; interfaces.X type-assert otherwise. Spec-reviewer + team-lead rejected that for code-shape reasons:feedback_force_strict_contracts_no_compatis framed at code shape, not runtime exercise.decisions/0028-task-17-pure-typed-cutover.md) records the full decision + alternatives rejected.Round 2 (commit ab75233) strips the fallback at all 5 dispatch sites. Test fixtures must construct a real
*typedIaCAdapterbacked by an in-process bufconn-served pb.IaC* server (precedent: PR #603'siac_e2e_test.go+ PR #609'sdiscover_typed_loader_test.go). Migration cost: ~10 fixture files; rewrites land in follow-up commits on this same branch (no force-push).The
interfaces.IaCProvider+ sub-interface definitions remain ininterfaces/for engine-side consumers per ADR 0024; only wfctl's dispatch sites are pure typed.5 dispatch sites converted (pure typed-pb; round 2)
cmd/wfctl/infra_cleanup.gointerfaces.Enumerator.EnumerateByTag+ ErrProviderMethodUnimplemented sentinelcmd/wfctl/infra_apply_refresh.gointerfaces.DriftConfigDetector.DetectDriftWithSpecsdetectDriftConfigTyped(cli, refs, specs)when registeredcmd/wfctl/infra_status_drift.godetectDriftConfigTypedwhen registeredcmd/wfctl/infra_bootstrap.gointerfaces.ProviderCredentialRevokerreturned directlycmd/wfctl/infra_align_rules.gointerfaces.ProviderValidator.ValidatePlanvalidatePlanTyped(cli, plan)when registeredLocal validation (round 2 commit ab75233)
Test plan
*typedIaCAdapterper ADR-0028. Lands in follow-up commits on this branch.Rollback
Revert this commit. The ADR + accessors stay; the dispatch-site reverts re-introduce the typed-then-fallback pattern (round-1 commit 63e1cdf is the prior shape). Operational behavior at the typed adapter is unchanged.
🤖 Generated with Claude Code