feat(iac): step.iac_provider_* general steps — stateless two-phase (infra-admin migration PR-2)#837
Merged
Merged
Conversation
…lists resource statuses)
Adds IaCProviderListStep with resolveIaCProvider shared helper. Factory
requires `provider` config key; calls provider.Status(ctx, refs) with
optional ref list; returns {provider, resources[], count}. Unregistered
provider surfaces "not registered" error. Three tests cover happy path,
unregistered provider, and missing-config factory guard.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ies, static fallback) IaCProviderCatalogStep type-asserts to providerclient.RegionListerProvider; calls RegionLister().ListRegions when advertised (source:"live") and falls back to a 10-region static list when the accessor returns nil or the provider does not implement the interface (source:"static"). Merges Capabilities() for resource types. Four tests cover live, no-lister, nil-lister, and missing-config factory guard paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e hash)
IaCProviderPlanStep fetches current state via provider.Status, computes
computeDesiredStateHash (inlined from wfctlhelpers.DesiredStateHash with
no-op env resolver to break module→wfctlhelpers→module import cycle), calls
provider.Plan, attaches DesiredHash to the plan, and returns
{plan, desired_hash, provider}. Four tests: happy path returns plan + hash,
env-var-value change does NOT change hash (no-op resolver guard), unregistered
provider surfaces "not registered", missing 'provider' config fails factory.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ash-guard apply) IaCProviderApplyStep: recomputes DesiredStateHash from live Status, rejects with "plan hash mismatch (state changed or plan tampered); re-plan" if hashes diverge (apply never runs), else calls injected applyFn (prod: wfctlhelpers .ApplyPlanWithHooks via function injection to break module→wfctlhelpers→module cycle; wfctlhelpers/state.go imports module so direct import is impossible). Adds iac/applydispatch bridge package as a re-export shim (considered but insufficient — Go cycle detection spans the full graph; function injection is the correct pattern). Five tests: match→applies, mismatch→rejected+no-apply, provider error surfaced (not "denied"), missing provider guard, missing hash guard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…drift
destroy: calls provider.Destroy(refs), returns {destroyed[], destroy_errors[],
provider}. Three tests: returns destroyed list, unregistered provider, missing
provider config.
drift: type-asserts to providerclient.DriftDetectorProvider; if accessor non-nil
→ DetectDriftWithSpecs (config-aware); else falls through to required DetectDrift;
required surface error → {supported:false, reason}. Returns {supported, any_drifted,
drifts[], count, provider}. Five tests: with detector (any_drifted=true), nil
detector (existence-only fallback), no interface (existence-only), DetectDrift
error → supported=false, missing provider config.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pe registry, and schema plugins/platform/plugin.go: adds iacProviderApplyFn wrapper (wfctlhelpers.ApplyPlanWithHooks with empty hooks, called from plugin which can import wfctlhelpers); registers all 6 step factories (list/catalog/plan/apply/destroy/drift) in StepFactories() and manifest StepTypes. cmd/wfctl/type_registry.go: 6 StepTypeInfo entries under "platform plugin steps (iac provider)". schema/schema.go: 6 new entries in coreModuleTypes (alphabetically sorted with existing iac_ steps). schema/step_schema_builtins.go: 6 StepSchema entries with ConfigFields + Outputs in registerBuiltins(). schema/module_schema.go: 6 display-name entries in the inline step-type list. plugins/platform/plugin_test.go: adds 6 new step types to TestStepFactories expected list. wfctl validate accepts step.iac_provider_list in a pipeline config. All schema tests pass. Pre-existing cmd/wfctl subprocess failures (TestFallbackRuns, strict-contracts) are unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…p schemas The 6 new step type entries added in Task 9 expand the StepSchemaRegistry which is serialized into the golden file used by TestEditorSchemasGoldenFile. Regenerated via UPDATE_GOLDEN=1 go test ./schema/ -run TestEditorSchemasGoldenFile. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new set of provider-agnostic IaC pipeline steps (step.iac_provider_*) that resolve an interfaces.IaCProvider from the modular service registry (registered in PR-1), enabling stateless two-phase plan/apply and related operations. This extends the platform plugin and CLI/type/schema registries so these steps are discoverable and usable across the engine and wfctl.
Changes:
- Introduces six new platform steps: list, catalog, plan, apply (two-phase hash guard), destroy, drift—backed by
interfaces.IaCProviderservice resolution. - Registers the new step types across platform plugin factories/tests, schema registries, and wfctl’s known step type registry.
- Adds an
iac/applydispatchshim intended to re-exportwfctlhelpers.ApplyPlanWithHookswithout introducing import cycles.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/testdata/editor-schemas.golden.json | Adds editor schema entries for the new step.iac_provider_* steps |
| schema/step_schema_builtins.go | Registers builtin step schemas for the six new IaCProvider steps |
| schema/schema.go | Adds new step types to the core module type allowlist |
| schema/module_schema.go | Adds new step types to the builtin module schema registry |
| plugins/platform/plugin.go | Registers new step types and factories in the platform plugin |
| plugins/platform/plugin_test.go | Extends platform plugin step factory coverage to include new steps |
| module/pipeline_step_iac_provider_plan.go | Implements step.iac_provider_plan + hashing helpers |
| module/pipeline_step_iac_provider_plan_test.go | Tests planning + hash stability behavior |
| module/pipeline_step_iac_provider_list.go | Implements step.iac_provider_list and IaCProvider service resolution helper |
| module/pipeline_step_iac_provider_list_test.go | Tests list step behavior and provider resolution errors |
| module/pipeline_step_iac_provider_drift.go | Implements drift step with optional drift-detector fast-path |
| module/pipeline_step_iac_provider_destroy.go | Implements destroy step using IaCProvider.Destroy |
| module/pipeline_step_iac_provider_destroy_drift_test.go | Tests destroy + drift behaviors and fallbacks |
| module/pipeline_step_iac_provider_catalog.go | Implements catalog step (regions + capability types) with static fallback |
| module/pipeline_step_iac_provider_catalog_test.go | Tests live vs static region sources and types output |
| module/pipeline_step_iac_provider_apply.go | Implements stateless two-phase apply with injected apply dispatcher |
| module/pipeline_step_iac_provider_apply_test.go | Tests apply hash-guard acceptance/rejection and dispatcher error surfacing |
| iac/applydispatch/applydispatch.go | Adds a shim re-export for ApplyPlanWithHooks/ApplyPlanHooks |
| cmd/wfctl/type_registry.go | Registers new step types/config keys in wfctl’s known type registry |
Comment on lines
+211
to
+215
| // Step 4: SHA-256 over the canonical JSON. | ||
| data, err := json.Marshal(resolved) | ||
| if err != nil { | ||
| return "hash-error" | ||
| } |
Comment on lines
+68
to
+85
| // Existence-only drift via the required DetectDrift method. | ||
| drifts, driftErr := provider.DetectDrift(ctx, s.refs) | ||
| if driftErr != nil { | ||
| // ErrProviderMethodUnimplemented from the required surface means the plugin | ||
| // wired neither path — surface as unsupported, not as an error. The step | ||
| // intentionally swallows the error here and converts it to structured output | ||
| // so callers can gate on {supported: false} without pipeline failure. | ||
| return &StepResult{Output: map[string]any{ //nolint:nilerr | ||
| "provider": s.provider, | ||
| "supported": false, | ||
| "reason": driftErr.Error(), | ||
| }}, nil | ||
| } | ||
| return driftResult(s.provider, drifts, true), nil | ||
| } | ||
|
|
||
| // driftResult builds the step output map from a drift detection result. | ||
| func driftResult(providerName string, drifts []interfaces.DriftResult, supported bool) *StepResult { |
Comment on lines
+45
to
+66
| // Optional: list of refs to query; nil means pass empty slice to Status | ||
| // (providers should return all resources when refs is empty). | ||
| var refs []interfaces.ResourceRef | ||
| if rawRefs, ok := cfg["refs"]; ok { | ||
| if refList, ok := rawRefs.([]any); ok { | ||
| for _, r := range refList { | ||
| if rm, ok := r.(map[string]any); ok { | ||
| ref := interfaces.ResourceRef{} | ||
| if n, ok := rm["name"].(string); ok { | ||
| ref.Name = n | ||
| } | ||
| if t, ok := rm["type"].(string); ok { | ||
| ref.Type = t | ||
| } | ||
| if pid, ok := rm["provider_id"].(string); ok { | ||
| ref.ProviderID = pid | ||
| } | ||
| refs = append(refs, ref) | ||
| } | ||
| } | ||
| } | ||
| } |
Comment on lines
+100
to
+110
| // First hash: env var set to "secret1". | ||
| os.Setenv("DB_PASSWORD", "secret1") | ||
| result1, err := step.Execute(context.Background(), &module.PipelineContext{}) | ||
| if err != nil { | ||
| t.Fatalf("Execute (run 1) error: %v", err) | ||
| } | ||
| hash1 := result1.Output["desired_hash"].(string) | ||
|
|
||
| // Second hash: env var changed to "secret2". | ||
| os.Setenv("DB_PASSWORD", "secret2") | ||
| result2, err := step.Execute(context.Background(), &module.PipelineContext{}) |
Comment on lines
6
to
+24
| import ( | ||
| "context" | ||
|
|
||
| "github.com/GoCodeAlone/modular" | ||
| "github.com/GoCodeAlone/workflow/handlers" | ||
| "github.com/GoCodeAlone/workflow/iac/wfctlhelpers" | ||
| "github.com/GoCodeAlone/workflow/interfaces" | ||
| "github.com/GoCodeAlone/workflow/module" | ||
| "github.com/GoCodeAlone/workflow/plugin" | ||
| "github.com/GoCodeAlone/workflow/schema" | ||
| ) | ||
|
|
||
| // iacProviderApplyFn is the apply dispatch function passed to | ||
| // NewIaCProviderApplyStepFactory. It wraps wfctlhelpers.ApplyPlanWithHooks | ||
| // with an empty hooks struct so the step's function signature | ||
| // (ctx, provider, plan) is satisfied without the step importing wfctlhelpers. | ||
| func iacProviderApplyFn(ctx context.Context, p interfaces.IaCProvider, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { | ||
| return wfctlhelpers.ApplyPlanWithHooks(ctx, p, plan, wfctlhelpers.ApplyPlanHooks{}) | ||
| } |
Comment on lines
+12
to
+19
| // IaCApplyFn is the function signature for the v2 apply dispatch helper. | ||
| // Matches wfctlhelpers.ApplyPlanWithHooks exactly so the platform plugin can | ||
| // pass that function directly, and tests can inject a stub without importing | ||
| // wfctlhelpers (which would create a module→wfctlhelpers→module cycle via | ||
| // wfctlhelpers/state.go). | ||
| type IaCApplyFn func(ctx context.Context, p interfaces.IaCProvider, plan *interfaces.IaCPlan, hooks any) (*interfaces.ApplyResult, error) | ||
|
|
||
| // ─── step.iac_provider_apply ───────────────────────────────────────────────── |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Zero importers confirmed; plugins/platform/plugin.go calls wfctlhelpers.ApplyPlanWithHooks directly via the injected iacProviderApplyFn closure and never imported applydispatch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The unsupported/error path omitted any_drifted, drifts, and count, so downstream type-assertions like result.Output["any_drifted"].(bool) would panic. Emit zero-value entries on the unsupported path to match the normal-path schema. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The type had a wrong 4-parameter signature (hooks any) and was never referenced anywhere; the factory's injected applyFn is typed inline with the correct 3-parameter signature. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Old name implied supported=false (Unsupported) but the test asserts supported=true because nil detector falls through to provider.DetectDrift which succeeds. New name reflects the actual behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…llback_error Covers the case where the provider implements RegionListerProvider and returns a non-nil lister but ListRegions returns an error; the step must fall back to static regions and set source=static_fallback_error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rage gate) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment on lines
+69
to
+88
| drifts, driftErr := provider.DetectDrift(ctx, s.refs) | ||
| if driftErr != nil { | ||
| // ErrProviderMethodUnimplemented from the required surface means the plugin | ||
| // wired neither path — surface as unsupported, not as an error. The step | ||
| // intentionally swallows the error here and converts it to structured output | ||
| // so callers can gate on {supported: false} without pipeline failure. | ||
| // all_drifted, drifts, and count are included with zero values so that | ||
| // downstream type-assertions (e.g. result.Output["any_drifted"].(bool)) do | ||
| // not panic on the unsupported path. | ||
| return &StepResult{Output: map[string]any{ //nolint:nilerr | ||
| "provider": s.provider, | ||
| "supported": false, | ||
| "reason": driftErr.Error(), | ||
| "any_drifted": false, | ||
| "drifts": []map[string]any{}, | ||
| "count": 0, | ||
| }}, nil | ||
| } | ||
| return driftResult(s.provider, drifts, true), nil | ||
| } |
Comment on lines
+3
to
+10
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/GoCodeAlone/modular" | ||
| "github.com/GoCodeAlone/workflow/iac/providerclient" | ||
| "github.com/GoCodeAlone/workflow/interfaces" | ||
| ) |
Comment on lines
+45
to
+66
| // Optional: list of refs to query; nil means pass empty slice to Status | ||
| // (providers should return all resources when refs is empty). | ||
| var refs []interfaces.ResourceRef | ||
| if rawRefs, ok := cfg["refs"]; ok { | ||
| if refList, ok := rawRefs.([]any); ok { | ||
| for _, r := range refList { | ||
| if rm, ok := r.(map[string]any); ok { | ||
| ref := interfaces.ResourceRef{} | ||
| if n, ok := rm["name"].(string); ok { | ||
| ref.Name = n | ||
| } | ||
| if t, ok := rm["type"].(string); ok { | ||
| ref.Type = t | ||
| } | ||
| if pid, ok := rm["provider_id"].(string); ok { | ||
| ref.ProviderID = pid | ||
| } | ||
| refs = append(refs, ref) | ||
| } | ||
| } | ||
| } | ||
| } |
Comment on lines
+2205
to
+2211
| Outputs: []StepOutputDef{ | ||
| {Key: "provider", Type: "string", Description: "Provider service name"}, | ||
| {Key: "supported", Type: "boolean", Description: "Whether drift detection is supported"}, | ||
| {Key: "any_drifted", Type: "boolean", Description: "Whether any resource has drifted"}, | ||
| {Key: "drifts", Type: "[]any", Description: "Per-resource drift results"}, | ||
| {Key: "count", Type: "number", Description: "Number of resources checked"}, | ||
| }, |
Comment on lines
+100
to
+114
| // First hash: env var set to "secret1". | ||
| os.Setenv("DB_PASSWORD", "secret1") | ||
| result1, err := step.Execute(context.Background(), &module.PipelineContext{}) | ||
| if err != nil { | ||
| t.Fatalf("Execute (run 1) error: %v", err) | ||
| } | ||
| hash1 := result1.Output["desired_hash"].(string) | ||
|
|
||
| // Second hash: env var changed to "secret2". | ||
| os.Setenv("DB_PASSWORD", "secret2") | ||
| result2, err := step.Execute(context.Background(), &module.PipelineContext{}) | ||
| if err != nil { | ||
| t.Fatalf("Execute (run 2) error: %v", err) | ||
| } | ||
| hash2 := result2.Output["desired_hash"].(string) |
…onstant fallback A constant "hash-error" fallback on json.Marshal failure could silently match across plan+apply and bypass the tamper/drift guard. Changed computeDesiredStateHash to return (string, error); both plan and apply Execute paths propagate the error and emit no plan/apply on failure. Removed os.Setenv/os.Unsetenv in favor of t.Setenv (auto-restore, safe under -shuffle). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… as supported:false
Previously ANY DetectDrift error was swallowed into {supported:false},
hiding network failures, provider crashes, etc. as a structured
unsupported-feature response. Now only errors.Is(err,
ErrProviderMethodUnimplemented) produces {supported:false}; any other
error is returned as an error (HTTP 5xx path). Renamed existing
Unsupported_DetectDriftError test to use the actual sentinel; added
TestIaCProviderDriftStep_Execute_RealError_Propagated for the new path.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tly widening to list-all If "refs" is present but the wrong type or contains non-map items, the factory now returns a config error rather than silently falling through to an unfiltered Status (list everything). Absent refs remains list-all. Added three new tests: MalformedRefs_WrongTopType, MalformedRefs_WrongItemType, AbsentRefs_ListsAll. Co-Authored-By: Claude Sonnet 4.6 <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.
PR-2 of the infra-admin plugin migration (stacks on PR-1 #836, merged). Six general IaC-plugin-aware pipeline steps that resolve an
interfaces.IaCProviderfrom the modular service registry (registered by PR-1's WiringHook) via aprovider: <module-name>config key.Tasks (TDD, one commit each)
step.iac_provider_list— list current resource statuses.step.iac_provider_catalog— regions via theproviderclient.RegionListerProvideraccessor (static fallback when unadvertised) +Capabilities()types. Live-catalog source for dropdown forms.step.iac_provider_plan— stateless:DesiredStateHash(no-op env resolver — guards the v1.1 T3 regression) → returns{plan, desired_hash}.step.iac_provider_apply— stateless two-phase: recompute hash from current state, reject mismatch (tamper/drift guard), elseApplyPlanWithHooks.step.iac_provider_destroy+step.iac_provider_drift(drift viaDriftDetectorProvideraccessor;supported:falsefallback).type_registry+ schema.Deviation (within Task 7):
module/cannot importiac/wfctlhelpersdirectly (wfctlhelpers/state.goimportsmodule/→ cycle). Addediac/applydispatch— a thin re-export shim (type alias + one-line delegate, zero logic) so the apply step reachesApplyPlanWithHookswithout the cycle. No new feature/PR; ADR 0013/0015 unchanged.ADR 0013 (steps bind
interfaces.IaCProvider) + 0015 (stateless two-phase). ExistingPlatformProviderstep.iac_*untouched.🤖 Generated with Claude Code