fix(plugin/external): bridge IaCProvider.EnumerateAll + strict-contracts hardening (v0.27.1)#589
Merged
Merged
Conversation
…RPC + strict-contracts coverage gate (v0.27.1)
The gap
-------
v0.27.0 declared interfaces.EnumeratorAll (used by `wfctl infra audit-keys`
+ `wfctl infra prune`) and added it to plugin-side providers (e.g. DO
v0.14.0 DOProvider.EnumerateAll). The wfctl-side gRPC proxy
*remoteIaCProvider* in cmd/wfctl/deploy_providers.go was NOT updated to
implement the optional interface, so the type-assert in
runInfraAuditKeysCmd (cmd/wfctl/infra_audit_keys.go:140) always fell
through to "no loaded provider implements EnumeratorAll" — even when the
plugin process correctly implemented EnumerateAll.
Same root gap existed for interfaces.Enumerator (EnumerateByTag).
Why strict-contracts didn't catch it
-------------------------------------
The pre-existing strict-contracts gate (cmd/wfctl/plugin_audit.go) audits
PLUGIN-SIDE manifest contract descriptors. It has no visibility into
workflow-side proxy method coverage. Compile-time interface satisfaction
was only asserted for IaCProvider + ProviderCredentialRevoker; optional
sub-interfaces (EnumeratorAll, Enumerator, DriftConfigDetector,
ProviderMigrationRepairer) had no enforced coverage. There was no
fallback or skip to remove — the test category simply didn't exist.
Fixes
-----
1. cmd/wfctl/deploy_providers.go — add EnumerateAll + EnumerateByTag
methods to remoteIaCProvider, dispatching via
InvokeServiceContext (with InvokeService fallback for legacy
invokers), modeled exactly on the existing RepairDirtyMigration
pattern.
2. cmd/wfctl/deploy_providers_strict_bridge_coverage_test.go (new) —
strict-contracts coverage gate that:
- compile-time-asserts *remoteIaCProvider satisfies every optional
IaCProvider sub-interface declared in interfaces/iac_provider.go
(catches missing-bridge gaps when a new interface is added);
- source-grep-asserts every interface method has a corresponding
"IaCProvider.<Method>" string literal in deploy_providers.go
(catches typos and "wired-but-not-actually-dispatched" bugs);
- meta-asserts the gate bodies contain no t.Skip, t.SkipNow,
testing.Short, os.Getenv, or build-tag bypasses (per the v0.27.1
user mandate "remove the fallback and force strict mode").
3. cmd/wfctl/deploy_providers_dispatch_matrix_test.go — add EnumerateAll
and EnumerateByTag rows to the IaCProvider dispatch matrix (pins
wire method name + required arg keys).
4. cmd/wfctl/deploy_providers_remote_iac_test.go — 8 new tests covering
the new methods: happy path, nil response, error propagation, decode
error (EnumerateAll only), and context-aware invoker dispatch for
both methods.
Per workspace memory feedback_workflow_plugin_structpb_boundary +
project_workflow_strict_grpc_contracts: this gate is an INTERIM
defense-in-depth measure until typed proto messages replace the
map[string]any dispatch surface (after which the bridge gap class
becomes a compile error rather than a runtime gap).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a wfctl ↔ external-plugin gRPC bridge gap where optional IaC provider sub-interfaces (EnumeratorAll, Enumerator) were implemented by the plugin process but not by the wfctl-side proxy (remoteIaCProvider), causing type-assert based dispatchers (e.g. infra audit-keys) to incorrectly conclude “not implemented”.
Changes:
- Added
EnumerateAllandEnumerateByTaggRPC dispatch methods to*remoteIaCProvider. - Added a new “strict bridge coverage” test gate (compile-time interface assertions + source-grep wire coverage + meta no-skip check).
- Expanded dispatch-matrix + remote-provider unit tests to cover the new enumeration methods.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/wfctl/deploy_providers.go | Implements EnumerateAll/EnumerateByTag on the wfctl proxy and wires them to InvokeService{,Context}. |
| cmd/wfctl/deploy_providers_strict_bridge_coverage_test.go | Adds strict-contracts style gates to prevent future proxy/bridge method omissions. |
| cmd/wfctl/deploy_providers_remote_iac_test.go | Adds unit tests for EnumerateAll/EnumerateByTag behavior, decoding, error propagation, and context usage. |
| cmd/wfctl/deploy_providers_dispatch_matrix_test.go | Pins RPC method names + required arg keys for the new proxy methods. |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…Validator + harden no-fallback meta-check Addresses Copilot inline review (PR #589 round 1): 1. Behavior preservation for non-implementing plugins (Findings 1+2) ───────────────────────────────────────────────────────────────── The round-1 bridge of EnumerateAll + EnumerateByTag made every gRPC-loaded provider satisfy the optional interfaces at the type level. Dispatchers (infra_audit_keys, infra_cleanup, infra_prune, infra_rotate_and_prune) used `p.(interfaces.X)` as the "supports this method?" gate; the fall-through-to-next-provider semantics regressed once every provider satisfied the assertion. Fix: introduce interfaces.ErrProviderMethodUnimplemented sentinel. Proxy methods translate gRPC codes.Unimplemented (canonical signal) plus a small set of message-string fallbacks ("unimplemented", "not implemented", "does not implement ServiceInvoker") into the sentinel. Dispatchers errors.Is on the sentinel to preserve the pre-v0.27.1 iterate-and-skip behavior. - infra_cleanup.go: errors.Is check inside the existing enumErr path; emits the same "skipped <provider>: ..." stdout line as the type-assert-fail branch. - infra_audit_keys.go: probedEnumerator wrapper traps the sentinel during runInfraAuditKeys's single call (no double RPC) and signals "skip and try next" via a recorded flag. - infra_prune.go: probedPruneProvider wrapper applies the same pattern; reused by rotate-and-prune dispatcher. - infra_rotate_and_prune.go: uses probedPruneProvider. 2. Bridge ProviderValidator (Finding 4) ────────────────────────────────────── Copilot flagged that interfaces.ProviderValidator was missing from the strict-bridge-coverage registry even though cmd/wfctl/infra_align_rules.go does `p.(interfaces.ProviderValidator)` for R-A10. Bridged by adding ValidatePlan to remoteIaCProvider. Contract caveat: ProviderValidator.ValidatePlan returns only []PlanDiagnostic (no error). The proxy silences ALL errors (Unimplemented + transport + plugin) to preserve the pre-v0.27.1 R-A10 behavior where non-implementing plugins were silently skipped via the type-assert gate. Surfacing transport errors through R-A10 requires extending the ProviderValidator contract to return an error — left to a follow-up PR per feedback_proper_fixes_over_workarounds. 3. Harden no-fallback meta-check (Finding 3) ────────────────────────────────────────── Copilot caught that the original "build tag" string check was ineffective: actual Go build constraints live in the file header as //go:build or // +build directives, not in function bodies. Fix: two-layer enforcement. Layer 1 scans the file header (above the package clause) for either directive form. Layer 2 keeps the function-body scan for runtime bypasses (t.Skip, testing.Short, os.Getenv). Layer 1 catches bypasses Layer 2 cannot see. 4. Tests ───── - TestRemoteIaCProvider_EnumerateAll_TranslatesUnimplemented (4 sub-cases: gRPC code, "unimplemented" string, "not implemented" string, "does not implement ServiceInvoker" string). - TestRemoteIaCProvider_EnumerateByTag_TranslatesUnimplemented. - TestRemoteIaCProvider_ValidatePlan_HappyPath + TestRemoteIaCProvider_ValidatePlan_SilentOnError. - Strict-bridge-coverage gate: ProviderValidator added to compile-time + reflective registries; existing tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…l loud on missing bridge - audit-keys: drop probedEnumerator (nil, nil) swallow; return error - prune: drop probedPruneProvider (nil, nil) swallow; return error - rotate-and-prune: pre-flight EnumerateAll check BEFORE Step 1 rotation - deploy_providers: align isPluginMethodUnimplemented + ValidatePlan docs with impl - tests: 3 new strict-mode regressions assert loud failure on bridge gap Per user mandate: no fallback. Missing bridge wiring must fail loud, not silently return zero results.
…ider dispatcher loops By bridging EnumerateAll/EnumerateByTag through remoteIaCProvider, every gRPC-loaded provider satisfies the optional interface even if the plugin doesn't actually support the method. Update dispatcher loops in infra_audit_keys, infra_prune, infra_rotate_and_prune to call the method, treat ErrProviderMethodUnimplemented as "this provider doesn't support it, try next", and error loud only if ALL providers fail to implement. infra_cleanup already had the correct pattern (try-each + skip-on-Unimplemented log line). Add a regression test asserting it stays correct under heterogeneous loaded providers. infra_audit_keys: extract renderAuditKeys() so the dispatcher can call EnumerateAll directly and inspect the error to continue-on-Unimplemented. infra_prune: probe EnumerateAll once in the dispatcher; on success cache the result in cachedPruneProvider so runInfraPrune's internal call serves from cache and we don't double-bill the cloud API. infra_rotate_and_prune: the pre-flight probe now continues past Unimplemented providers rather than aborting the whole run; loud error only fires when no provider can serve the resource type. Rotation (Step 1, destructive) still gated behind a successful pre-flight on the chosen provider. Plus 4 new tests asserting multi-provider continue-on-Unimplemented for audit-keys, prune, rotate-and-prune, and cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…patcher comments with impl - audit-keys: dispatcher comment now describes the direct EnumerateAll + renderAuditKeys path. The previous wording referenced synthesizing an inner-args slice and dispatching to runInfraAuditKeys, but the impl invokes EnumerateAll itself across all providers and renders via renderAuditKeys to keep the cloud-API call count to one per provider. - isPluginMethodUnimplemented: doc lists the literal strings actually emitted by plugin/external/sdk/grpc_server.go (verified at line 527 for ServiceInvoker; ServiceContextInvoker is NOT emitted today and is kept only as a defensive forward-compat catch; TypedServiceInvoker at line 498 is intentionally not matched because IaCProvider RPCs use the untyped path). - rotate-and-prune: thread the pre-flight EnumerateAll result into runInfraRotateAndPrune via cachedPruneProvider so the inner runInfraPrune serves from cache rather than re-issuing the cloud enumeration. This closes the double-EnumerateAll on the successful path that Copilot flagged: previously the dispatcher probed EnumerateAll for verification then handed runInfraRotateAndPrune a raw pruneProviderAdapter, and runInfraPrune (called after rotation) re-enumerated. The cache key is resourceType; the freshly-rotated key is excluded by --exclude-access-key in the prune step so serving the pre-rotation snapshot is safe. Co-Authored-By: Claude Opus 4.7 (1M context) <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.
Bug
wfctl infra audit-keys --type infra.spaces_keyfailed at staging with:even though
workflow-plugin-digitaloceanv0.14.0 implementsEnumerateAllcorrectly on its
DOProvider(spaces-key plan Tasks 14+15).Root cause: v0.27.0 declared the new optional interface
interfaces.EnumeratorAlland added it to plugin-side providers, but the wfctl-side gRPC proxy
*remoteIaCProviderincmd/wfctl/deploy_providers.gowas NOT updated toimplement the new optional interface. The dispatcher in
runInfraAuditKeysCmdtype-asserts the loaded provider against
interfaces.EnumeratorAll(
cmd/wfctl/infra_audit_keys.go:140), so the assertion always fell through tothe negative branch even though the plugin process implemented the method.
The same root gap exists for
interfaces.Enumerator(EnumerateByTag); bothare fixed here.
Why strict-contracts didn't catch it
The pre-existing strict-contracts gate (
cmd/wfctl/plugin_audit.go) auditsplugin-side manifest contract descriptors (
plugin.contracts.json). Ithas no visibility into workflow-side proxy method coverage. Compile-time
interface-satisfaction assertions on
*remoteIaCProviderexisted only forIaCProvider+ProviderCredentialRevoker; optional sub-interfaces(
EnumeratorAll,Enumerator,DriftConfigDetector,ProviderMigrationRepairer) had no enforced coverage.There was no fallback or skip to remove — the test category simply didn't exist.
The user mandate's "remove the fallback" intent is fulfilled by adding a new
unconditional gate that forecloses this entire class of cross-repo bridge gap.
Fixes
cmd/wfctl/deploy_providers.go— addedEnumerateAll(ctx, resourceType)and
EnumerateByTag(ctx, tag)methods toremoteIaCProvider, dispatchingvia
InvokeServiceContextwhen available (falls back toInvokeServicefor legacy invokers). Modeled exactly on the existing
RepairDirtyMigration/RevokeProviderCredentialpattern.cmd/wfctl/deploy_providers_strict_bridge_coverage_test.go(new) — threestrict-contracts coverage gates that catch the entire class of bridge gap:
TestStrictBridgeCoverage_CompileTimeAssertions: package-initvar _ interfaces.X = (*remoteIaCProvider)(nil)block for every optional sub-interface — adding a new interface without bridging will fail to compile.TestStrictBridgeCoverage_WireMethodCoverage: source-grep asserts every interface method has a corresponding"IaCProvider.<Method>"string literal indeploy_providers.go. Catches typos in RPC method names and "method declared but not actually wired through gRPC" placeholders. Exempt list (Name, Version, Close, etc.) is small + each entry justified.TestStrictBridgeCoverage_NoFallbackOrSkip: meta-check that the gate bodies contain not.Skip,testing.Short,os.Getenv, or build-tag bypasses. Per the user mandate ("remove the fallback and force strict mode"), the gate is unconditional.cmd/wfctl/deploy_providers_dispatch_matrix_test.go— addedEnumerateAlland
EnumerateByTagrows toTestDispatchMatrix_RemoteIaCProvider(pinswire method name + required arg keys, mirroring the existing
RepairDirtyMigrationrow).cmd/wfctl/deploy_providers_remote_iac_test.go— 8 new tests:TestRemoteIaCProvider_EnumerateAll(happy path)TestRemoteIaCProvider_EnumerateAll_NilResponse(empty account)TestRemoteIaCProvider_EnumerateAll_PropagatesErrorTestRemoteIaCProvider_EnumerateAll_DecodeErrorTestRemoteIaCProvider_EnumerateAll_UsesContextTestRemoteIaCProvider_EnumerateByTag(happy path)TestRemoteIaCProvider_EnumerateByTag_PropagatesErrorTestRemoteIaCProvider_EnumerateByTag_UsesContextWhat about server-side dispatch?
The brief originally suggested a Fix 2 in
plugin/external/sdk/grpc_server.goadding a
case "IaCProvider.EnumerateAll"next to aRepairDirtyMigrationcase. Verified during investigation: the workflow-side
grpc_server.goismethod-name-agnostic — it delegates to the plugin's own
InvokeMethod/InvokeMethodContextdispatcher. There are no per-method casesto extend on the workflow side. The plugin (DO v0.14.0) owns its own method
string → typed-method dispatch, and that side is already correct.
Verification
The pre-existing
gosec G115warning incmd/wfctl/secrets_detect.go:186is on
mainand unrelated to this PR.References
feedback_workflow_plugin_structpb_boundary(interim until typed proto contracts land)project_workflow_strict_grpc_contracts(long-term direction)feedback_no_invented_interfaces,feedback_local_ci_validation_for_ci_touching_tasks,feedback_proper_fixes_over_workarounds(review discipline applied)🤖 Generated with Claude Code