From 1f939a6901c8583919c6f85da4d022cd59dae4d5 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 2 Jun 2026 17:43:41 -0400 Subject: [PATCH 1/4] feat(iac): specs_from context-input mode on step.iac_provider_plan Co-Authored-By: Claude Sonnet 4.6 --- module/pipeline_step_iac_provider_plan.go | 63 +++++++++++----- .../pipeline_step_iac_provider_plan_test.go | 75 +++++++++++++++++++ 2 files changed, 120 insertions(+), 18 deletions(-) diff --git a/module/pipeline_step_iac_provider_plan.go b/module/pipeline_step_iac_provider_plan.go index df9f100af..a92f8c948 100644 --- a/module/pipeline_step_iac_provider_plan.go +++ b/module/pipeline_step_iac_provider_plan.go @@ -19,11 +19,12 @@ import ( // a DesiredStateHash with a NO-OP env resolver, builds a plan, and returns the // plan JSON plus the stable hash. type IaCProviderPlanStep struct { - name string - provider string - env string - specs []interfaces.ResourceSpec - app modular.Application + name string + provider string + env string + specs []interfaces.ResourceSpec + specsFrom string // dotted context path; mutually exclusive with specs + app modular.Application } // NewIaCProviderPlanStepFactory returns a StepFactory for step.iac_provider_plan. @@ -35,19 +36,31 @@ func NewIaCProviderPlanStepFactory() StepFactory { } env, _ := cfg["env"].(string) - // Parse specs from config. Each spec is a map[string]any with at least - // name and type fields; config sub-map is optional. - specs, err := parseResourceSpecs(cfg["specs"]) - if err != nil { - return nil, fmt.Errorf("iac_provider_plan step %q: parse specs: %w", name, err) + specsFrom, _ := cfg["specs_from"].(string) + _, hasStaticSpecs := cfg["specs"] + + // specs and specs_from are mutually exclusive. + if specsFrom != "" && hasStaticSpecs { + return nil, fmt.Errorf("iac_provider_plan step %q: 'specs' and 'specs_from' are mutually exclusive", name) + } + + // Parse static specs from config (nil-safe; skipped when specs_from is set). + var specs []interfaces.ResourceSpec + if hasStaticSpecs { + var err error + specs, err = parseResourceSpecs(cfg["specs"]) + if err != nil { + return nil, fmt.Errorf("iac_provider_plan step %q: parse specs: %w", name, err) + } } return &IaCProviderPlanStep{ - name: name, - provider: providerName, - env: env, - specs: specs, - app: app, + name: name, + provider: providerName, + env: env, + specs: specs, + specsFrom: specsFrom, + app: app, }, nil } } @@ -93,7 +106,21 @@ func parseResourceRefs(raw any) ([]interfaces.ResourceRef, error) { func (s *IaCProviderPlanStep) Name() string { return s.name } -func (s *IaCProviderPlanStep) Execute(ctx context.Context, _ *PipelineContext) (*StepResult, error) { +func (s *IaCProviderPlanStep) Execute(ctx context.Context, pc *PipelineContext) (*StepResult, error) { + // Resolve specs: dynamic path takes precedence when specsFrom is configured. + specs := s.specs + if s.specsFrom != "" { + raw := resolveBodyFrom(s.specsFrom, pc) + var err error + specs, err = specparse.ParseResourceSpecs(raw) + if err != nil { + return nil, fmt.Errorf("iac_provider_plan step %q: resolve specs_from %q: %w", s.name, s.specsFrom, err) + } + if specs == nil { + return nil, fmt.Errorf("iac_provider_plan step %q: specs_from %q resolved to nil/empty", s.name, s.specsFrom) + } + } + provider, err := resolveIaCProvider(s.app, s.provider, s.name, "iac_provider_plan") if err != nil { return nil, err @@ -111,13 +138,13 @@ func (s *IaCProviderPlanStep) Execute(ctx context.Context, _ *PipelineContext) ( // Compute desired state hash with a NO-OP env resolver so that // ${ENV_VAR} and ${secret.*} placeholders hash as literal strings — // same hash at plan time and at apply time regardless of env values. - desiredHash, err := computeDesiredStateHash(s.specs, current) + desiredHash, err := computeDesiredStateHash(specs, current) if err != nil { return nil, fmt.Errorf("iac_provider_plan step %q: compute desired hash: %w", s.name, err) } // Build the plan from the provider. - plan, err := provider.Plan(ctx, s.specs, current) + plan, err := provider.Plan(ctx, specs, current) if err != nil { return nil, fmt.Errorf("iac_provider_plan step %q: Plan: %w", s.name, err) } diff --git a/module/pipeline_step_iac_provider_plan_test.go b/module/pipeline_step_iac_provider_plan_test.go index abf3c9758..5b1fc9964 100644 --- a/module/pipeline_step_iac_provider_plan_test.go +++ b/module/pipeline_step_iac_provider_plan_test.go @@ -141,3 +141,78 @@ func TestIaCProviderPlanStep_Factory_RequiresProvider(t *testing.T) { t.Fatal("expected error when 'provider' missing") } } + +// ─── specs_from tests ───────────────────────────────────────────────────────── + +// TestIaCProviderPlan_SpecsFromContext verifies that when specs_from is set, the +// step resolves specs from the pipeline context (StepOutputs) at Execute time and +// returns a valid desired_hash in its output. +func TestIaCProviderPlan_SpecsFromContext(t *testing.T) { + app := module.NewMockApplication() + provider := makePlanProvider(t) + if err := app.RegisterService("my-provider", provider); err != nil { + t.Fatal(err) + } + + factory := module.NewIaCProviderPlanStepFactory() + // No static specs — specs_from points into StepOutputs. + step, err := factory("plan-step", map[string]any{ + "provider": "my-provider", + "specs_from": "steps.parse-request.body.specs", + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + // Build a PipelineContext whose StepOutputs["parse-request"]["body"]["specs"] + // contains one resource spec with a secret:// ref in its config. + pc := &module.PipelineContext{ + StepOutputs: map[string]map[string]any{ + "parse-request": { + "body": map[string]any{ + "specs": []any{ + map[string]any{ + "name": "vault-db", + "type": "infra.database", + "config": map[string]any{ + "password": "secret://vault/x", + }, + }, + }, + }, + }, + }, + } + + result, err := step.Execute(context.Background(), pc) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + + hash, ok := result.Output["desired_hash"].(string) + if !ok || hash == "" { + t.Errorf("expected non-empty desired_hash, got %v", result.Output["desired_hash"]) + } + if result.Output["plan"] == nil { + t.Error("expected plan in output, got nil") + } +} + +// TestIaCProviderPlan_SpecsFromAndStatic_FactoryError verifies that setting both +// specs and specs_from is rejected at factory time (mutually exclusive). +func TestIaCProviderPlan_SpecsFromAndStatic_FactoryError(t *testing.T) { + factory := module.NewIaCProviderPlanStepFactory() + _, err := factory("plan-step", map[string]any{ + "provider": "my-provider", + "specs_from": "steps.parse-request.body.specs", + "specs": []any{ + map[string]any{"name": "my-db", "type": "infra.database"}, + }, + }, nil) + if err == nil { + t.Fatal("expected factory error when both specs and specs_from are set") + } + if !containsString(err.Error(), "mutually exclusive") { + t.Errorf("expected 'mutually exclusive' error, got: %v", err) + } +} From 4ddd07f7dec5a6d9efd67653b0c308d6921e5b1f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 2 Jun 2026 17:46:03 -0400 Subject: [PATCH 2/4] feat(iac): specs_from + desired_hash_from context-input mode on step.iac_provider_apply Co-Authored-By: Claude Sonnet 4.6 --- module/pipeline_step_iac_provider_apply.go | 87 ++++++-- .../pipeline_step_iac_provider_apply_test.go | 189 ++++++++++++++++++ 2 files changed, 256 insertions(+), 20 deletions(-) diff --git a/module/pipeline_step_iac_provider_apply.go b/module/pipeline_step_iac_provider_apply.go index d3f059869..88132afe3 100644 --- a/module/pipeline_step_iac_provider_apply.go +++ b/module/pipeline_step_iac_provider_apply.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/GoCodeAlone/modular" + "github.com/GoCodeAlone/workflow/iac/specparse" "github.com/GoCodeAlone/workflow/interfaces" ) @@ -18,12 +19,14 @@ import ( // 3. Compare against the client-submitted desired_hash — mismatch → reject. // 4. Dispatch via the injected applyFn (wfctlhelpers.ApplyPlanWithHooks in prod). type IaCProviderApplyStep struct { - name string - provider string - submittedHash string - specs []interfaces.ResourceSpec - app modular.Application - applyFn func(ctx context.Context, p interfaces.IaCProvider, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) + name string + provider string + submittedHash string + desiredHashFrom string // dotted context path; mutually exclusive with submittedHash + specs []interfaces.ResourceSpec + specsFrom string // dotted context path; mutually exclusive with specs + app modular.Application + applyFn func(ctx context.Context, p interfaces.IaCProvider, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) } // NewIaCProviderApplyStepFactory returns a StepFactory for step.iac_provider_apply. @@ -39,30 +42,74 @@ func NewIaCProviderApplyStepFactory(applyFn func(ctx context.Context, p interfac if providerName == "" { return nil, fmt.Errorf("iac_provider_apply step %q: 'provider' is required", name) } + + specsFrom, _ := cfg["specs_from"].(string) + _, hasStaticSpecs := cfg["specs"] + if specsFrom != "" && hasStaticSpecs { + return nil, fmt.Errorf("iac_provider_apply step %q: 'specs' and 'specs_from' are mutually exclusive", name) + } + + desiredHashFrom, _ := cfg["desired_hash_from"].(string) submittedHash, _ := cfg["desired_hash"].(string) - if submittedHash == "" { + if desiredHashFrom != "" && submittedHash != "" { + return nil, fmt.Errorf("iac_provider_apply step %q: 'desired_hash' and 'desired_hash_from' are mutually exclusive", name) + } + // Require exactly one hash source. + if desiredHashFrom == "" && submittedHash == "" { return nil, fmt.Errorf("iac_provider_apply step %q: 'desired_hash' is required", name) } - specs, err := parseResourceSpecs(cfg["specs"]) - if err != nil { - return nil, fmt.Errorf("iac_provider_apply step %q: parse specs: %w", name, err) + // Parse static specs (skipped when specs_from is set). + var specs []interfaces.ResourceSpec + if hasStaticSpecs { + var err error + specs, err = parseResourceSpecs(cfg["specs"]) + if err != nil { + return nil, fmt.Errorf("iac_provider_apply step %q: parse specs: %w", name, err) + } } return &IaCProviderApplyStep{ - name: name, - provider: providerName, - submittedHash: submittedHash, - specs: specs, - app: app, - applyFn: applyFn, + name: name, + provider: providerName, + submittedHash: submittedHash, + desiredHashFrom: desiredHashFrom, + specs: specs, + specsFrom: specsFrom, + app: app, + applyFn: applyFn, }, nil } } func (s *IaCProviderApplyStep) Name() string { return s.name } -func (s *IaCProviderApplyStep) Execute(ctx context.Context, _ *PipelineContext) (*StepResult, error) { +func (s *IaCProviderApplyStep) Execute(ctx context.Context, pc *PipelineContext) (*StepResult, error) { + // Resolve specs: dynamic path takes precedence when specsFrom is configured. + specs := s.specs + if s.specsFrom != "" { + raw := resolveBodyFrom(s.specsFrom, pc) + var err error + specs, err = specparse.ParseResourceSpecs(raw) + if err != nil { + return nil, fmt.Errorf("iac_provider_apply step %q: resolve specs_from %q: %w", s.name, s.specsFrom, err) + } + if specs == nil { + return nil, fmt.Errorf("iac_provider_apply step %q: specs_from %q resolved to nil/empty", s.name, s.specsFrom) + } + } + + // Resolve submitted hash: dynamic path takes precedence when desiredHashFrom is configured. + submittedHash := s.submittedHash + if s.desiredHashFrom != "" { + raw := resolveBodyFrom(s.desiredHashFrom, pc) + var ok bool + submittedHash, ok = raw.(string) + if !ok || submittedHash == "" { + return nil, fmt.Errorf("iac_provider_apply step %q: desired_hash_from %q did not resolve to a non-empty string (got %T)", s.name, s.desiredHashFrom, raw) + } + } + provider, err := resolveIaCProvider(s.app, s.provider, s.name, "iac_provider_apply") if err != nil { return nil, err @@ -74,18 +121,18 @@ func (s *IaCProviderApplyStep) Execute(ctx context.Context, _ *PipelineContext) return nil, fmt.Errorf("iac_provider_apply step %q: Status: %w", s.name, err) } current := statusesToResourceStates(statuses) - recomputedHash, err := computeDesiredStateHash(s.specs, current) + recomputedHash, err := computeDesiredStateHash(specs, current) if err != nil { return nil, fmt.Errorf("iac_provider_apply step %q: compute desired hash: %w", s.name, err) } // Phase 2: guard — reject if hashes diverge (state changed or plan tampered). - if recomputedHash != s.submittedHash { + if recomputedHash != submittedHash { return nil, fmt.Errorf("iac_provider_apply step %q: plan hash mismatch (state changed or plan tampered); re-plan", s.name) } // Phase 3: build the plan and dispatch via the injected apply function. - plan, err := provider.Plan(ctx, s.specs, current) + plan, err := provider.Plan(ctx, specs, current) if err != nil { return nil, fmt.Errorf("iac_provider_apply step %q: Plan: %w", s.name, err) } diff --git a/module/pipeline_step_iac_provider_apply_test.go b/module/pipeline_step_iac_provider_apply_test.go index 030b93e43..61f0655d6 100644 --- a/module/pipeline_step_iac_provider_apply_test.go +++ b/module/pipeline_step_iac_provider_apply_test.go @@ -193,3 +193,192 @@ func TestIaCProviderApplyStep_Factory_RequiresHash(t *testing.T) { t.Fatal("expected error when 'desired_hash' missing") } } + +// ─── specs_from / desired_hash_from (dynamic input) tests ──────────────────── + +// TestIaCProviderApply_DynamicInput verifies that specs_from + desired_hash_from +// pull specs and hash from the pipeline context at Execute time, and that the +// recompute-hash guard still fires (matching hash → apply proceeds). +func TestIaCProviderApply_DynamicInput(t *testing.T) { + app := module.NewMockApplication() + provider, correctHash := buildApplyProvider(t) + if err := app.RegisterService("my-provider", provider); err != nil { + t.Fatal(err) + } + + factory := module.NewIaCProviderApplyStepFactory(noopApplyFn) + // No static specs or hash — both come from context. + step, err := factory("apply-step", map[string]any{ + "provider": "my-provider", + "specs_from": "steps.plan-step.specs", + "desired_hash_from": "steps.plan-step.desired_hash", + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := &module.PipelineContext{ + StepOutputs: map[string]map[string]any{ + "plan-step": { + "desired_hash": correctHash, + "specs": []any{ + map[string]any{"name": "my-db", "type": "infra.database"}, + }, + }, + }, + } + + result, err := step.Execute(context.Background(), pc) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + if result.Output["apply_result"] == nil { + t.Error("expected apply_result in output") + } + if result.Output["desired_hash"] != correctHash { + t.Errorf("desired_hash mismatch: got %v", result.Output["desired_hash"]) + } +} + +// TestIaCProviderApply_HashMismatchRejected verifies that supplying a wrong +// desired_hash via context still triggers the TOCTOU/drift guard. +func TestIaCProviderApply_HashMismatchRejected(t *testing.T) { + app := module.NewMockApplication() + provider, _ := buildApplyProvider(t) + if err := app.RegisterService("my-provider", provider); err != nil { + t.Fatal(err) + } + + applied := false + trackingApplyFn := func(ctx context.Context, p interfaces.IaCProvider, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + applied = true + return noopApplyFn(ctx, p, plan) + } + + factory := module.NewIaCProviderApplyStepFactory(trackingApplyFn) + step, err := factory("apply-step", map[string]any{ + "provider": "my-provider", + "specs_from": "steps.plan-step.specs", + "desired_hash_from": "steps.plan-step.desired_hash", + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := &module.PipelineContext{ + StepOutputs: map[string]map[string]any{ + "plan-step": { + // Wrong hash — should trigger mismatch error. + "desired_hash": "deadbeef0000000000000000000000000000000000000000000000000000dead", + "specs": []any{ + map[string]any{"name": "my-db", "type": "infra.database"}, + }, + }, + }, + } + + _, err = step.Execute(context.Background(), pc) + if err == nil { + t.Fatal("expected error for hash mismatch, got nil") + } + if !containsString(err.Error(), "plan hash mismatch") { + t.Errorf("expected 'plan hash mismatch' error, got: %v", err) + } + if applied { + t.Error("applyFn must not be called when hash mismatches") + } +} + +// TestIaCProviderApply_SecretRefSurvives verifies that specs containing +// secret:// refs survive unchanged through the dynamic-input path — no resolver +// is invoked, and the literal ref is what reaches the applyFn. +func TestIaCProviderApply_SecretRefSurvives(t *testing.T) { + // specsWithSecret is the raw []any form that would come from the pipeline context + // (e.g. via step.request_parse output). It includes a secret:// ref that must + // not be expanded. + specsWithSecret := []any{ + map[string]any{ + "name": "secret-db", + "type": "infra.database", + "config": map[string]any{ + "password": "secret://vault/x", + }, + }, + } + + // Compute the correct hash by driving the plan step with the same []any data, + // so both plan and apply hash over identical parsed ResourceSpec values. + hashApp := module.NewMockApplication() + hashProvider := &stubIaCProvider{ + statusResult: nil, + planResult: &interfaces.IaCPlan{ID: "x"}, + } + if err := hashApp.RegisterService("hp", hashProvider); err != nil { + t.Fatal(err) + } + planFactory := module.NewIaCProviderPlanStepFactory() + hashStep, err := planFactory("h", map[string]any{"provider": "hp", "specs": specsWithSecret}, hashApp) + if err != nil { + t.Fatalf("hash step factory error: %v", err) + } + hashResult, err := hashStep.Execute(context.Background(), &module.PipelineContext{}) + if err != nil { + t.Fatalf("hash step Execute error: %v", err) + } + correctHash := hashResult.Output["desired_hash"].(string) + + // Build the actual provider under test (no existing resources). + app := module.NewMockApplication() + provider := &stubIaCProvider{ + statusResult: nil, + planResult: &interfaces.IaCPlan{ + ID: "plan-secret", + Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{Name: "secret-db", Type: "infra.database"}}, + }, + }, + } + if err := app.RegisterService("my-provider", provider); err != nil { + t.Fatal(err) + } + + // Capture what plan reaches the applyFn. + var capturedPlan *interfaces.IaCPlan + capturingApplyFn := func(ctx context.Context, p interfaces.IaCProvider, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + capturedPlan = plan + return noopApplyFn(ctx, p, plan) + } + + factory := module.NewIaCProviderApplyStepFactory(capturingApplyFn) + step, err := factory("apply-step", map[string]any{ + "provider": "my-provider", + "specs_from": "steps.plan-step.specs", + "desired_hash_from": "steps.plan-step.desired_hash", + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := &module.PipelineContext{ + StepOutputs: map[string]map[string]any{ + "plan-step": { + "desired_hash": correctHash, + "specs": specsWithSecret, + }, + }, + } + + _, err = step.Execute(context.Background(), pc) + if err != nil { + t.Fatalf("Execute error: %v", err) + } + + // The captured plan must not be nil — the applyFn was called. + if capturedPlan == nil { + t.Fatal("expected applyFn to be called with a plan") + } + // The plan's DesiredHash must match the hash we computed from the literal secret ref. + if capturedPlan.DesiredHash != correctHash { + t.Errorf("plan DesiredHash = %q, want %q", capturedPlan.DesiredHash, correctHash) + } +} From aed25d25d6192d05b2eedb8b45c33896af0b2a01 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 2 Jun 2026 18:00:39 -0400 Subject: [PATCH 3/4] fix(iac): empty-specs guard + canonical-wiring tests + schema fields (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR3 code review: - Empty-specs footgun: dynamic specs_from path now rejects zero specs via len(specs)==0 (ParseResourceSpecs returns a non-nil empty slice for []any{}, which the prior nil check let through → apply over zero specs = destroy-all). Applied to both step.iac_provider_plan and step.iac_provider_apply; static path unchanged. - Test fidelity: rewrote the apply dynamic tests to use the canonical request_parse wiring (steps.parse-request.body.specs and steps.parse-request.body.desired_hash) instead of injecting a specs key the plan step never emits. - Failure paths: added table-driven Execute-error tests on both steps for missing path, non-list scalar specs, empty []any specs, and (apply) a non-string desired_hash (guarded cast, no panic). - Schema: declared specs_from on plan and specs_from + desired_hash_from on apply in schema/step_schema_builtins.go; relaxed apply desired_hash from Required since desired_hash_from is now a valid alternative source. Co-Authored-By: Claude Opus 4.8 (1M context) --- module/pipeline_step_iac_provider_apply.go | 7 +- .../pipeline_step_iac_provider_apply_test.go | 206 +++++++++++++----- module/pipeline_step_iac_provider_plan.go | 7 +- .../pipeline_step_iac_provider_plan_test.go | 75 +++++++ schema/step_schema_builtins.go | 9 +- 5 files changed, 240 insertions(+), 64 deletions(-) diff --git a/module/pipeline_step_iac_provider_apply.go b/module/pipeline_step_iac_provider_apply.go index 88132afe3..ddfb28fed 100644 --- a/module/pipeline_step_iac_provider_apply.go +++ b/module/pipeline_step_iac_provider_apply.go @@ -94,8 +94,11 @@ func (s *IaCProviderApplyStep) Execute(ctx context.Context, pc *PipelineContext) if err != nil { return nil, fmt.Errorf("iac_provider_apply step %q: resolve specs_from %q: %w", s.name, s.specsFrom, err) } - if specs == nil { - return nil, fmt.Errorf("iac_provider_apply step %q: specs_from %q resolved to nil/empty", s.name, s.specsFrom) + // Guard against zero specs: ParseResourceSpecs returns a non-nil empty + // slice for []any{}, so a len check (not a nil check) is required — + // applying over zero specs is a destroy-everything footgun. + if len(specs) == 0 { + return nil, fmt.Errorf("iac_provider_apply step %q: specs_from %q resolved to empty/zero specs", s.name, s.specsFrom) } } diff --git a/module/pipeline_step_iac_provider_apply_test.go b/module/pipeline_step_iac_provider_apply_test.go index 61f0655d6..d14d5f50e 100644 --- a/module/pipeline_step_iac_provider_apply_test.go +++ b/module/pipeline_step_iac_provider_apply_test.go @@ -196,9 +196,45 @@ func TestIaCProviderApplyStep_Factory_RequiresHash(t *testing.T) { // ─── specs_from / desired_hash_from (dynamic input) tests ──────────────────── +// parseRequestBodyOutputs builds StepOutputs that mirror the canonical +// production wiring: step.request_parse writes the parsed POST body to its +// Output["body"] (a map[string]any), so specs resolve as +// steps.parse-request.body.specs and the hash as steps.parse-request.body.desired_hash. +func parseRequestBodyOutputs(specs []any, hash any) map[string]map[string]any { + body := map[string]any{"specs": specs} + if hash != nil { + body["desired_hash"] = hash + } + return map[string]map[string]any{ + "parse-request": {"body": body}, + } +} + +// hashForSpecs computes the canonical desired_hash for the given []any specs by +// driving the plan step with the same data, so both plan and apply hash over +// identical parsed ResourceSpec values. +func hashForSpecs(t *testing.T, specs []any) string { + t.Helper() + app := module.NewMockApplication() + provider := &stubIaCProvider{statusResult: nil, planResult: &interfaces.IaCPlan{ID: "x"}} + if err := app.RegisterService("hp", provider); err != nil { + t.Fatal(err) + } + step, err := module.NewIaCProviderPlanStepFactory()("h", map[string]any{"provider": "hp", "specs": specs}, app) + if err != nil { + t.Fatalf("hash step factory error: %v", err) + } + result, err := step.Execute(context.Background(), &module.PipelineContext{}) + if err != nil { + t.Fatalf("hash step Execute error: %v", err) + } + return result.Output["desired_hash"].(string) +} + // TestIaCProviderApply_DynamicInput verifies that specs_from + desired_hash_from -// pull specs and hash from the pipeline context at Execute time, and that the -// recompute-hash guard still fires (matching hash → apply proceeds). +// pull specs and hash from the pipeline context at Execute time using the +// canonical request_parse wiring, and that the recompute-hash guard still fires +// (matching hash → apply proceeds). func TestIaCProviderApply_DynamicInput(t *testing.T) { app := module.NewMockApplication() provider, correctHash := buildApplyProvider(t) @@ -207,25 +243,20 @@ func TestIaCProviderApply_DynamicInput(t *testing.T) { } factory := module.NewIaCProviderApplyStepFactory(noopApplyFn) - // No static specs or hash — both come from context. + // No static specs or hash — both come from context (request_parse body). step, err := factory("apply-step", map[string]any{ "provider": "my-provider", - "specs_from": "steps.plan-step.specs", - "desired_hash_from": "steps.plan-step.desired_hash", + "specs_from": "steps.parse-request.body.specs", + "desired_hash_from": "steps.parse-request.body.desired_hash", }, app) if err != nil { t.Fatalf("factory error: %v", err) } pc := &module.PipelineContext{ - StepOutputs: map[string]map[string]any{ - "plan-step": { - "desired_hash": correctHash, - "specs": []any{ - map[string]any{"name": "my-db", "type": "infra.database"}, - }, - }, - }, + StepOutputs: parseRequestBodyOutputs([]any{ + map[string]any{"name": "my-db", "type": "infra.database"}, + }, correctHash), } result, err := step.Execute(context.Background(), pc) @@ -241,7 +272,7 @@ func TestIaCProviderApply_DynamicInput(t *testing.T) { } // TestIaCProviderApply_HashMismatchRejected verifies that supplying a wrong -// desired_hash via context still triggers the TOCTOU/drift guard. +// desired_hash via the request_parse body still triggers the TOCTOU/drift guard. func TestIaCProviderApply_HashMismatchRejected(t *testing.T) { app := module.NewMockApplication() provider, _ := buildApplyProvider(t) @@ -258,23 +289,17 @@ func TestIaCProviderApply_HashMismatchRejected(t *testing.T) { factory := module.NewIaCProviderApplyStepFactory(trackingApplyFn) step, err := factory("apply-step", map[string]any{ "provider": "my-provider", - "specs_from": "steps.plan-step.specs", - "desired_hash_from": "steps.plan-step.desired_hash", + "specs_from": "steps.parse-request.body.specs", + "desired_hash_from": "steps.parse-request.body.desired_hash", }, app) if err != nil { t.Fatalf("factory error: %v", err) } pc := &module.PipelineContext{ - StepOutputs: map[string]map[string]any{ - "plan-step": { - // Wrong hash — should trigger mismatch error. - "desired_hash": "deadbeef0000000000000000000000000000000000000000000000000000dead", - "specs": []any{ - map[string]any{"name": "my-db", "type": "infra.database"}, - }, - }, - }, + StepOutputs: parseRequestBodyOutputs([]any{ + map[string]any{"name": "my-db", "type": "infra.database"}, + }, "deadbeef0000000000000000000000000000000000000000000000000000dead"), // wrong hash } _, err = step.Execute(context.Background(), pc) @@ -293,9 +318,8 @@ func TestIaCProviderApply_HashMismatchRejected(t *testing.T) { // secret:// refs survive unchanged through the dynamic-input path — no resolver // is invoked, and the literal ref is what reaches the applyFn. func TestIaCProviderApply_SecretRefSurvives(t *testing.T) { - // specsWithSecret is the raw []any form that would come from the pipeline context - // (e.g. via step.request_parse output). It includes a secret:// ref that must - // not be expanded. + // specsWithSecret is the raw []any form delivered by step.request_parse. + // It includes a secret:// ref that must not be expanded. specsWithSecret := []any{ map[string]any{ "name": "secret-db", @@ -305,27 +329,7 @@ func TestIaCProviderApply_SecretRefSurvives(t *testing.T) { }, }, } - - // Compute the correct hash by driving the plan step with the same []any data, - // so both plan and apply hash over identical parsed ResourceSpec values. - hashApp := module.NewMockApplication() - hashProvider := &stubIaCProvider{ - statusResult: nil, - planResult: &interfaces.IaCPlan{ID: "x"}, - } - if err := hashApp.RegisterService("hp", hashProvider); err != nil { - t.Fatal(err) - } - planFactory := module.NewIaCProviderPlanStepFactory() - hashStep, err := planFactory("h", map[string]any{"provider": "hp", "specs": specsWithSecret}, hashApp) - if err != nil { - t.Fatalf("hash step factory error: %v", err) - } - hashResult, err := hashStep.Execute(context.Background(), &module.PipelineContext{}) - if err != nil { - t.Fatalf("hash step Execute error: %v", err) - } - correctHash := hashResult.Output["desired_hash"].(string) + correctHash := hashForSpecs(t, specsWithSecret) // Build the actual provider under test (no existing resources). app := module.NewMockApplication() @@ -352,20 +356,15 @@ func TestIaCProviderApply_SecretRefSurvives(t *testing.T) { factory := module.NewIaCProviderApplyStepFactory(capturingApplyFn) step, err := factory("apply-step", map[string]any{ "provider": "my-provider", - "specs_from": "steps.plan-step.specs", - "desired_hash_from": "steps.plan-step.desired_hash", + "specs_from": "steps.parse-request.body.specs", + "desired_hash_from": "steps.parse-request.body.desired_hash", }, app) if err != nil { t.Fatalf("factory error: %v", err) } pc := &module.PipelineContext{ - StepOutputs: map[string]map[string]any{ - "plan-step": { - "desired_hash": correctHash, - "specs": specsWithSecret, - }, - }, + StepOutputs: parseRequestBodyOutputs(specsWithSecret, correctHash), } _, err = step.Execute(context.Background(), pc) @@ -382,3 +381,96 @@ func TestIaCProviderApply_SecretRefSurvives(t *testing.T) { t.Errorf("plan DesiredHash = %q, want %q", capturedPlan.DesiredHash, correctHash) } } + +// TestIaCProviderApply_DynamicInputFailures asserts the dynamic-input path fails +// cleanly (rather than proceeding with nil/zero specs or panicking on a bad hash +// cast) when the resolved context values are missing, the wrong type, or empty. +func TestIaCProviderApply_DynamicInputFailures(t *testing.T) { + // validSpecs + its correct hash, reused where a test isolates one failure. + validSpecs := []any{map[string]any{"name": "my-db", "type": "infra.database"}} + validHash := hashForSpecs(t, validSpecs) + + cases := []struct { + name string + stepOutputs map[string]map[string]any + wantErrSub string + }{ + { + name: "specs path missing from context (request_parse didn't run)", + stepOutputs: nil, + wantErrSub: "resolved to empty/zero specs", + }, + { + name: "body lacks specs key", + stepOutputs: map[string]map[string]any{ + "parse-request": {"body": map[string]any{"desired_hash": validHash}}, + }, + wantErrSub: "resolved to empty/zero specs", + }, + { + name: "specs resolves to a non-list scalar", + stepOutputs: map[string]map[string]any{ + "parse-request": {"body": map[string]any{"specs": "not-a-list", "desired_hash": validHash}}, + }, + wantErrSub: "resolve specs_from", + }, + { + name: "specs resolves to an empty list", + stepOutputs: map[string]map[string]any{ + "parse-request": {"body": map[string]any{"specs": []any{}, "desired_hash": validHash}}, + }, + wantErrSub: "resolved to empty/zero specs", + }, + { + name: "desired_hash resolves to a non-string number (guarded cast, no panic)", + stepOutputs: map[string]map[string]any{ + "parse-request": {"body": map[string]any{"specs": validSpecs, "desired_hash": 12345}}, + }, + wantErrSub: "did not resolve to a non-empty string", + }, + { + name: "desired_hash missing from body", + stepOutputs: map[string]map[string]any{ + "parse-request": {"body": map[string]any{"specs": validSpecs}}, + }, + wantErrSub: "did not resolve to a non-empty string", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + app := module.NewMockApplication() + provider, _ := buildApplyProvider(t) + if err := app.RegisterService("my-provider", provider); err != nil { + t.Fatal(err) + } + + applied := false + trackingApplyFn := func(ctx context.Context, p interfaces.IaCProvider, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + applied = true + return noopApplyFn(ctx, p, plan) + } + + factory := module.NewIaCProviderApplyStepFactory(trackingApplyFn) + step, err := factory("apply-step", map[string]any{ + "provider": "my-provider", + "specs_from": "steps.parse-request.body.specs", + "desired_hash_from": "steps.parse-request.body.desired_hash", + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + _, err = step.Execute(context.Background(), &module.PipelineContext{StepOutputs: tc.stepOutputs}) + if err == nil { + t.Fatal("expected error, got nil (must not proceed with nil/zero specs or bad hash)") + } + if !containsString(err.Error(), tc.wantErrSub) { + t.Errorf("expected error containing %q, got: %v", tc.wantErrSub, err) + } + if applied { + t.Error("applyFn must not be called on a dynamic-input failure") + } + }) + } +} diff --git a/module/pipeline_step_iac_provider_plan.go b/module/pipeline_step_iac_provider_plan.go index a92f8c948..81c639c59 100644 --- a/module/pipeline_step_iac_provider_plan.go +++ b/module/pipeline_step_iac_provider_plan.go @@ -116,8 +116,11 @@ func (s *IaCProviderPlanStep) Execute(ctx context.Context, pc *PipelineContext) if err != nil { return nil, fmt.Errorf("iac_provider_plan step %q: resolve specs_from %q: %w", s.name, s.specsFrom, err) } - if specs == nil { - return nil, fmt.Errorf("iac_provider_plan step %q: specs_from %q resolved to nil/empty", s.name, s.specsFrom) + // Guard against zero specs: ParseResourceSpecs returns a non-nil empty + // slice for []any{}, so a len check (not a nil check) is required — + // planning over zero specs is a destroy-everything footgun. + if len(specs) == 0 { + return nil, fmt.Errorf("iac_provider_plan step %q: specs_from %q resolved to empty/zero specs", s.name, s.specsFrom) } } diff --git a/module/pipeline_step_iac_provider_plan_test.go b/module/pipeline_step_iac_provider_plan_test.go index 5b1fc9964..a052f0d88 100644 --- a/module/pipeline_step_iac_provider_plan_test.go +++ b/module/pipeline_step_iac_provider_plan_test.go @@ -216,3 +216,78 @@ func TestIaCProviderPlan_SpecsFromAndStatic_FactoryError(t *testing.T) { t.Errorf("expected 'mutually exclusive' error, got: %v", err) } } + +// TestIaCProviderPlan_SpecsFromFailures asserts that the dynamic specs_from path +// fails cleanly (rather than silently planning over nil/zero specs) when the +// resolved value is missing, the wrong type, or empty. +func TestIaCProviderPlan_SpecsFromFailures(t *testing.T) { + cases := []struct { + name string + stepOutputs map[string]map[string]any + wantErrSub string + }{ + { + name: "path missing from context (request_parse didn't run)", + stepOutputs: nil, + wantErrSub: "resolved to empty/zero specs", + }, + { + name: "body present but lacks specs key", + stepOutputs: map[string]map[string]any{ + "parse-request": { + "body": map[string]any{}, + }, + }, + wantErrSub: "resolved to empty/zero specs", + }, + { + name: "specs resolves to a non-list scalar", + stepOutputs: map[string]map[string]any{ + "parse-request": { + "body": map[string]any{ + "specs": "not-a-list", + }, + }, + }, + wantErrSub: "resolve specs_from", + }, + { + name: "specs resolves to an empty list", + stepOutputs: map[string]map[string]any{ + "parse-request": { + "body": map[string]any{ + "specs": []any{}, + }, + }, + }, + wantErrSub: "resolved to empty/zero specs", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + app := module.NewMockApplication() + provider := makePlanProvider(t) + if err := app.RegisterService("my-provider", provider); err != nil { + t.Fatal(err) + } + + factory := module.NewIaCProviderPlanStepFactory() + step, err := factory("plan-step", map[string]any{ + "provider": "my-provider", + "specs_from": "steps.parse-request.body.specs", + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + _, err = step.Execute(context.Background(), &module.PipelineContext{StepOutputs: tc.stepOutputs}) + if err == nil { + t.Fatal("expected error, got nil (must not proceed with nil/zero specs)") + } + if !containsString(err.Error(), tc.wantErrSub) { + t.Errorf("expected error containing %q, got: %v", tc.wantErrSub, err) + } + }) + } +} diff --git a/schema/step_schema_builtins.go b/schema/step_schema_builtins.go index d44ae781d..45b23a677 100644 --- a/schema/step_schema_builtins.go +++ b/schema/step_schema_builtins.go @@ -2146,7 +2146,8 @@ func (r *StepSchemaRegistry) registerBuiltins() { Description: "Plans infrastructure changes against an IaCProvider service; computes a stable desired-state hash using a no-op env resolver.", ConfigFields: []ConfigFieldDef{ {Key: "provider", Type: FieldTypeString, Description: "Name of the registered IaCProvider service", Required: true}, - {Key: "specs", Type: FieldTypeArray, Description: "Desired resource specs (list of {name, type, config, size})"}, + {Key: "specs", Type: FieldTypeArray, Description: "Desired resource specs (list of {name, type, config, size}); mutually exclusive with specs_from"}, + {Key: "specs_from", Type: FieldTypeString, Description: "Dotted context path to resolve specs at Execute time (e.g. steps.parse-request.body.specs); mutually exclusive with specs"}, {Key: "env", Type: FieldTypeString, Description: "Environment name (reserved; unused by hash computation)"}, }, Outputs: []StepOutputDef{ @@ -2164,8 +2165,10 @@ func (r *StepSchemaRegistry) registerBuiltins() { Description: "Applies an IaC plan via two-phase hash guard: recomputes DesiredStateHash and rejects with 'plan hash mismatch' when hashes diverge.", ConfigFields: []ConfigFieldDef{ {Key: "provider", Type: FieldTypeString, Description: "Name of the registered IaCProvider service", Required: true}, - {Key: "specs", Type: FieldTypeArray, Description: "Desired resource specs passed to plan and hash recomputation"}, - {Key: "desired_hash", Type: FieldTypeString, Description: "Client-submitted hash from the plan step; must match recomputed hash", Required: true}, + {Key: "specs", Type: FieldTypeArray, Description: "Desired resource specs passed to plan and hash recomputation; mutually exclusive with specs_from"}, + {Key: "specs_from", Type: FieldTypeString, Description: "Dotted context path to resolve specs at Execute time (e.g. steps.parse-request.body.specs); mutually exclusive with specs"}, + {Key: "desired_hash", Type: FieldTypeString, Description: "Client-submitted hash from the plan step; must match recomputed hash; mutually exclusive with desired_hash_from"}, + {Key: "desired_hash_from", Type: FieldTypeString, Description: "Dotted context path to resolve the client-submitted hash at Execute time (e.g. steps.parse-request.body.desired_hash); mutually exclusive with desired_hash"}, }, Outputs: []StepOutputDef{ {Key: "apply_result", Type: "any", Description: "ApplyResult from wfctlhelpers.ApplyPlanWithHooks"}, From ed68f63201f2fb74fbd18c7679eaedf87f8d6123 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 2 Jun 2026 18:16:21 -0400 Subject: [PATCH 4/4] fix(iac): apply factory hash one-of uses key-presence + clearer error (Copilot) Co-Authored-By: Claude Opus 4.8 (1M context) --- module/pipeline_step_iac_provider_apply.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/module/pipeline_step_iac_provider_apply.go b/module/pipeline_step_iac_provider_apply.go index ddfb28fed..dabe68601 100644 --- a/module/pipeline_step_iac_provider_apply.go +++ b/module/pipeline_step_iac_provider_apply.go @@ -51,12 +51,16 @@ func NewIaCProviderApplyStepFactory(applyFn func(ctx context.Context, p interfac desiredHashFrom, _ := cfg["desired_hash_from"].(string) submittedHash, _ := cfg["desired_hash"].(string) - if desiredHashFrom != "" && submittedHash != "" { + // Use key-presence (not value) for the one-of check, mirroring specs/specs_from, + // so a config carrying both keys is rejected even if one decodes to null/"". + _, hasStaticHash := cfg["desired_hash"] + _, hasHashFrom := cfg["desired_hash_from"] + if hasHashFrom && hasStaticHash { return nil, fmt.Errorf("iac_provider_apply step %q: 'desired_hash' and 'desired_hash_from' are mutually exclusive", name) } // Require exactly one hash source. - if desiredHashFrom == "" && submittedHash == "" { - return nil, fmt.Errorf("iac_provider_apply step %q: 'desired_hash' is required", name) + if !hasHashFrom && !hasStaticHash { + return nil, fmt.Errorf("iac_provider_apply step %q: one of 'desired_hash' or 'desired_hash_from' is required", name) } // Parse static specs (skipped when specs_from is set).