diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index d28f02786..5a7e5f32d 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "flag" "fmt" "os" @@ -11,17 +12,47 @@ import ( "github.com/GoCodeAlone/workflow/config" "github.com/GoCodeAlone/workflow/iac/inputsnapshot" + "github.com/GoCodeAlone/workflow/iac/jitsubst" "github.com/GoCodeAlone/workflow/interfaces" "github.com/GoCodeAlone/workflow/platform" "github.com/GoCodeAlone/workflow/secrets" ) -// infraPlanSchemaVersion is the on-disk plan format version this wfctl -// binary writes and is willing to read. runInfraPlan stamps it on every -// emitted plan; runInfraApply rejects plans with a higher version so a -// future schema bump (e.g. W-5 JIT-required plans) fails fast rather than -// being silently mis-read by an older binary. -const infraPlanSchemaVersion = 1 +// infraPlanSchemaVersion is the maximum on-disk plan format version this +// wfctl binary is willing to read. runInfraApply rejects plans with a +// higher version so a future schema bump fails fast rather than being +// silently mis-read by an older binary. +// +// runInfraPlan stamps either V1 (no JIT references in plan.Actions) or +// V2 (any ${MODULE.field} or ${MODULE.id} surviving in +// plan.Actions[*].Resource.Config). The choice is per-plan via +// jitsubst.HasModuleRefs; see runInfraPlan and the persisted-plan +// rejection in T5.5 (V2 plans cannot be persisted via -o; canonical path +// is `wfctl infra apply` without --plan). +const ( + infraPlanSchemaVersion = 2 // max readable + infraPlanSchemaVersionV1 = 1 // pre-JIT baseline + infraPlanSchemaVersionJIT = 2 // bumped when plan has ${MODULE.field|id} refs +) + +// planRequiresJITSubstitution returns true when any action in plan +// carries a ${MODULE.field} or ${MODULE.id} reference somewhere in its +// resolved Resource.Config. Plain ${VAR} env-var references do NOT +// count — see jitsubst.HasModuleRefs for the exact rule. +// +// Used by runInfraPlan (T5.4) to gate plan.SchemaVersion = 2 stamping +// and by runInfraPlan's persisted-plan rejection (T5.5). +func planRequiresJITSubstitution(plan *interfaces.IaCPlan) bool { + if plan == nil { + return false + } + for i := range plan.Actions { + if jitsubst.HasModuleRefs(plan.Actions[i].Resource.Config) { + return true + } + } + return false +} func runInfra(args []string) error { if len(args) < 1 { @@ -228,7 +259,17 @@ func runInfraPlan(args []string) error { return fmt.Errorf("compute input snapshot: %w", err) } plan.InputSnapshot = snap - plan.SchemaVersion = infraPlanSchemaVersion + // Stamp SchemaVersion based on whether any plan action's resolved + // Config carries a JIT-required ${MODULE.field|id} reference. Plain + // ${VAR} env-var refs (no dot in body) do NOT trigger the bump — + // plan-time config.ExpandEnvInMapPreservingKeys has already + // collapsed them outside preserved keys, and inside preserved keys + // they remain operator-managed across plan/apply (drift detection + // in plan.InputSnapshot covers the change-after-plan case). + plan.SchemaVersion = infraPlanSchemaVersionV1 + if planRequiresJITSubstitution(&plan) { + plan.SchemaVersion = infraPlanSchemaVersionJIT + } switch *format { case "markdown": @@ -239,6 +280,23 @@ func runInfraPlan(args []string) error { } if *output != "" { + // T5.5: persisted plan.json is the wfctl-infra-apply --plan + // canonical input. JIT-style plans cannot be persisted because + // every ${MODULE.field|id} ref needs apply-time resolution + // against this-apply ReplaceIDMap + syncedOutputs (data that + // does NOT exist at plan time and CANNOT be preserved across + // the plan/apply boundary). Reject up-front with an exact + // error string the operator can grep for. Stdout-only emission + // (no -o) of a JIT-style plan IS allowed — it's a preview, not + // a contract — and falls through this guard untouched. + if plan.SchemaVersion == infraPlanSchemaVersionJIT { + // Plan literal per docs/plans/2026-05-03-iac-conformance-and-replace.md + // §T5.5 line 2104. NO leading "error:" — that's prepended by + // cmd/wfctl/main.go's top-level wrapper. errors.New (rather than + // fmt.Errorf) avoids govet's no-verbs noise and is canonical for + // fixed-string error literals per Go convention. + return errors.New("this plan requires JIT resolution; persisted plan.json is not supported. Run 'wfctl infra apply' directly without -o/--plan.") + } // Embed a hash of the desired-state inputs so wfctl infra apply --plan // can detect stale plans when the config changes after plan generation. plan.DesiredHash = desiredStateHash(desired) diff --git a/cmd/wfctl/infra_apply_jit_loader_test.go b/cmd/wfctl/infra_apply_jit_loader_test.go new file mode 100644 index 000000000..15ca9036a --- /dev/null +++ b/cmd/wfctl/infra_apply_jit_loader_test.go @@ -0,0 +1,218 @@ +package main + +import ( + "context" + "errors" + "io" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/GoCodeAlone/workflow/iac/wfctlhelpers" + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestApply_V2_LoaderSeam_JITResolution_EndToEnd is W-5 T5.7 Step 2's +// runtime-launch-validation: it exercises the full v2 dispatch chain +// for a JIT-substitution scenario through the same applyInfraModules +// entrypoint runInfraApply uses, then asserts the dependent module's +// driver call receives the post-substitution Config (parent's +// freshly-minted ProviderID swapped in for ${parent.id}). +// +// Why loader-seam (not real gRPC binary): same precedent as T3.9 (see +// docs/adr/007-t3-9-runtime-validation-via-loader-seam.md). A +// stub-provider plugin would add an out-of-process gRPC dependency +// solely for this one assertion; the loader seam exercises every +// in-process code path between cmd/wfctl and the driver — config +// parse, state load, provider load, ComputePlan dispatch, +// wfctlhelpers.ApplyPlan, jitsubst.ResolveSpec — the only mocked +// boundary is the cloud API the driver would have called. +// +// The scenario: +// +// - Two infra.vpc modules in declaration order: parent + dependent. +// - parent has no JIT refs. +// - dependent's Config carries env_vars.PARENT_VPC_UUID = ${parent.id}. +// - Empty state, so ComputePlan emits two Create actions. +// - At apply: parent's Create succeeds, returning the canonical +// stub ProviderID. ApplyPlan folds {id: } into +// syncedOutputs. The next iteration's pre-dispatch ResolveSpec +// swaps ${parent.id} for the ProviderID in dependent's Config +// before driver.Create. +// +// Driver assertion: dependent's recorded Config[env_vars][PARENT_VPC_UUID] +// == parent's ProviderID — proves end-to-end JIT substitution through +// the production binary code path. +func TestApply_V2_LoaderSeam_JITResolution_EndToEnd(t *testing.T) { + dir := t.TempDir() + stateDir := filepath.Join(dir, "state") + if err := os.MkdirAll(stateDir, 0o700); err != nil { + t.Fatalf("mkdir stateDir: %v", err) + } + + cfgPath := filepath.Join(dir, "infra.yaml") + cfgYAML := `name: t5-7-jit-loader-seam +modules: + - name: state + type: iac.state + config: + backend: filesystem + directory: ` + stateDir + ` + - name: stub + type: iac.provider + config: + provider: stub + - name: parent + type: infra.vpc + config: + provider: stub + region: nyc1 + - name: dependent + type: infra.vpc + config: + provider: stub + region: nyc1 + env_vars: + PARENT_VPC_UUID: "${parent.id}" +` + if err := os.WriteFile(cfgPath, []byte(cfgYAML), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + + stub := &jitLoaderStubProvider{ + driver: &jitLoaderStubDriver{ + seenCreate: make(map[string]map[string]any), + providerID: map[string]string{ + "parent": "vpc-parent-binary-uuid", + "dependent": "vpc-dependent-binary-uuid", + }, + }, + } + origLoader := resolveIaCProvider + resolveIaCProvider = func(_ context.Context, _ string, _ map[string]any) (interfaces.IaCProvider, io.Closer, error) { + return stub, nil, nil + } + t.Cleanup(func() { resolveIaCProvider = origLoader }) + + if err := applyInfraModules(t.Context(), cfgPath, ""); err != nil { + t.Fatalf("applyInfraModules: %v", err) + } + + // Both Creates must have run. + if len(stub.driver.seenCreate) != 2 { + t.Fatalf("expected 2 Create dispatches; got %d (seen: %+v)", + len(stub.driver.seenCreate), stub.driver.seenCreate) + } + + // Dependent's Create must have received the post-substitution Config. + depCfg := stub.driver.seenCreate["dependent"] + if depCfg == nil { + t.Fatalf("driver did not receive dependent's Config; seenCreate: %+v", + stub.driver.seenCreate) + } + envVars, ok := depCfg["env_vars"].(map[string]any) + if !ok { + t.Fatalf("dependent.Config[env_vars] not a map: %T (%+v)", depCfg["env_vars"], depCfg) + } + if got, want := envVars["PARENT_VPC_UUID"], "vpc-parent-binary-uuid"; got != want { + t.Errorf("dependent.Config[env_vars][PARENT_VPC_UUID] post-JIT: got %q want %q\n(full env_vars: %+v)", + got, want, envVars) + } +} + +// jitLoaderStubProvider is the W-5-T5.7-flavored loader-seam provider +// — declares ComputePlanVersion v2 to route through wfctlhelpers.ApplyPlan, +// returns the same jitLoaderStubDriver for every resource type so a +// single instance records every Create across the plan. Mirrors the +// v2LoaderStubProvider pattern from infra_apply_v2_loader_test.go but +// owns its own driver type so JIT-specific assertions (per-name Config +// recording, configurable per-name ProviderID) don't entangle with the +// T3.9 NeedsReplace-flavored stub. +type jitLoaderStubProvider struct { + driver *jitLoaderStubDriver +} + +var ( + _ interfaces.IaCProvider = (*jitLoaderStubProvider)(nil) + _ wfctlhelpers.ComputePlanVersionDeclarer = (*jitLoaderStubProvider)(nil) +) + +func (p *jitLoaderStubProvider) Name() string { return "stub" } +func (p *jitLoaderStubProvider) Version() string { return "0.0.1-jit-loader-seam" } +func (p *jitLoaderStubProvider) ComputePlanVersion() string { return "v2" } +func (p *jitLoaderStubProvider) Initialize(_ context.Context, _ map[string]any) error { return nil } +func (p *jitLoaderStubProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { return nil } +func (p *jitLoaderStubProvider) Plan(_ context.Context, _ []interfaces.ResourceSpec, _ []interfaces.ResourceState) (*interfaces.IaCPlan, error) { + return nil, nil +} +func (p *jitLoaderStubProvider) Apply(_ context.Context, _ *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + return nil, errors.New("v2 path must route through wfctlhelpers.ApplyPlan, not provider.Apply") +} +func (p *jitLoaderStubProvider) Destroy(_ context.Context, _ []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { + return nil, nil +} +func (p *jitLoaderStubProvider) Status(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { + return nil, nil +} +func (p *jitLoaderStubProvider) DetectDrift(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { + return nil, nil +} +func (p *jitLoaderStubProvider) Import(_ context.Context, _ string, _ string) (*interfaces.ResourceState, error) { + return nil, nil +} +func (p *jitLoaderStubProvider) ResolveSizing(_ string, _ interfaces.Size, _ *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { + return nil, nil +} +func (p *jitLoaderStubProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return p.driver, nil +} +func (p *jitLoaderStubProvider) SupportedCanonicalKeys() []string { return nil } +func (p *jitLoaderStubProvider) BootstrapStateBackend(_ context.Context, _ map[string]any) (*interfaces.BootstrapResult, error) { + return nil, nil +} +func (p *jitLoaderStubProvider) Close() error { return nil } + +// jitLoaderStubDriver records every spec.Config seen on Create per +// resource Name, and returns a per-name ProviderID (mints "stub-id-" +// + spec.Name when the providerID map has no override). The Diff +// method is a no-op since this test exercises only the empty-state +// → Create path; if a future revision needs Update/Replace cascades, +// extend Diff to honor a NeedsReplace toggle. +type jitLoaderStubDriver struct { + mu sync.Mutex + seenCreate map[string]map[string]any + providerID map[string]string +} + +var _ interfaces.ResourceDriver = (*jitLoaderStubDriver)(nil) + +func (d *jitLoaderStubDriver) Create(_ context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.mu.Lock() + d.seenCreate[spec.Name] = spec.Config + id, ok := d.providerID[spec.Name] + d.mu.Unlock() + if !ok { + id = "stub-id-" + spec.Name + } + return &interfaces.ResourceOutput{ + Name: spec.Name, Type: spec.Type, ProviderID: id, Status: "active", + }, nil +} +func (d *jitLoaderStubDriver) Read(_ context.Context, ref interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { + return &interfaces.ResourceOutput{Name: ref.Name, Type: ref.Type, ProviderID: ref.ProviderID, Status: "active"}, nil +} +func (d *jitLoaderStubDriver) Update(_ context.Context, ref interfaces.ResourceRef, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + return &interfaces.ResourceOutput{Name: spec.Name, Type: spec.Type, ProviderID: ref.ProviderID}, nil +} +func (d *jitLoaderStubDriver) Delete(_ context.Context, _ interfaces.ResourceRef) error { return nil } +func (d *jitLoaderStubDriver) Diff(_ context.Context, _ interfaces.ResourceSpec, _ *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { + return nil, nil +} +func (d *jitLoaderStubDriver) HealthCheck(_ context.Context, _ interfaces.ResourceRef) (*interfaces.HealthResult, error) { + return &interfaces.HealthResult{Healthy: true}, nil +} +func (d *jitLoaderStubDriver) Scale(_ context.Context, ref interfaces.ResourceRef, _ int) (*interfaces.ResourceOutput, error) { + return &interfaces.ResourceOutput{Name: ref.Name, Type: ref.Type, ProviderID: ref.ProviderID}, nil +} +func (d *jitLoaderStubDriver) SensitiveKeys() []string { return nil } diff --git a/cmd/wfctl/infra_plan_jit_reject_test.go b/cmd/wfctl/infra_plan_jit_reject_test.go new file mode 100644 index 000000000..b6dc21e76 --- /dev/null +++ b/cmd/wfctl/infra_plan_jit_reject_test.go @@ -0,0 +1,143 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// expectedJITRejectError is the EXACT error string runInfraPlan must +// emit when -o is passed alongside a config that produces a +// SchemaVersion=2 (JIT-required) plan. Verbatim from the plan literal +// at docs/plans/2026-05-03-iac-conformance-and-replace.md §T5.5 +// line 2104. The string is kept literal here (not built via fmt +// formatting) so any drift between the implementation and the +// operator-facing diagnostic fails this test loudly. Runbooks and +// operator search-engines may match on this exact phrase. +// +// Note: the operator's displayed error includes a leading "error: " +// prefix from cmd/wfctl/main.go's top-level error wrapper; the value +// returned by runInfraPlan (and asserted here) does NOT include that +// prefix. +const expectedJITRejectError = "this plan requires JIT resolution; persisted plan.json is not supported. Run 'wfctl infra apply' directly without -o/--plan." + +// TestInfraPlan_RejectsPersistedJITPlan_WithExactErrorString is the +// canonical T5.5 scenario: a config whose env_vars carry a +// ${MODULE.field} ref (preserved through ExpandEnvInMapPreservingKeys +// and surviving into plan.Actions[*].Resource.Config) is requested to +// be persisted via -o. runInfraPlan must REJECT before writing the +// plan file, with the exact T5.5 error string. The plan file MUST NOT +// be created — operators should not have a half-state plan.json on disk. +func TestInfraPlan_RejectsPersistedJITPlan_WithExactErrorString(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: app + type: infra.container_service + config: + env_vars: + VPC_UUID: "${vpc.id}" +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + planFile := filepath.Join(dir, "plan.json") + err := runInfraPlan([]string{"--config", cfgPath, "--output", planFile}) + if err == nil { + t.Fatalf("expected runInfraPlan to reject persistence of JIT-style plan; got nil error") + } + if got := err.Error(); got != expectedJITRejectError { + t.Errorf("error string mismatch:\n got: %q\nwant: %q", got, expectedJITRejectError) + } + // The plan file must NOT be present after rejection. + if _, statErr := os.Stat(planFile); statErr == nil { + t.Errorf("plan file was written despite rejection: %s", planFile) + } +} + +// TestInfraPlan_PermitsStdoutOnlyJITPlan verifies the inverse contract: +// a JIT-style plan WITHOUT -o is fine — the operator gets the plan +// preview on stdout and the rejection guard does NOT fire. This locks +// the design's "stdout-only is fine (operator can preview)" clause. +func TestInfraPlan_PermitsStdoutOnlyJITPlan(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: app + type: infra.container_service + config: + env_vars: + VPC_UUID: "${vpc.id}" +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + if err := runInfraPlan([]string{"--config", cfgPath}); err != nil { + t.Errorf("stdout-only JIT plan should not be rejected; got error: %v", err) + } +} + +// TestInfraPlan_PermitsPersistedNonJITPlan is the negative-control: +// a non-JIT plan persisted via -o still works exactly as before T5.5. +// Belt-and-suspenders that the rejection guard didn't break the V1 +// happy path. +func TestInfraPlan_PermitsPersistedNonJITPlan(t *testing.T) { + t.Setenv("STAGING_DB_PASSWORD", "secret") + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: app + type: infra.container_service + config: + env_vars: + DATABASE_URL: "postgres://user:${STAGING_DB_PASSWORD}@host:5432/db" +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + planFile := filepath.Join(dir, "plan.json") + if err := runInfraPlan([]string{"--config", cfgPath, "--output", planFile}); err != nil { + t.Fatalf("non-JIT plan with -o should succeed; got: %v", err) + } + if _, statErr := os.Stat(planFile); statErr != nil { + t.Errorf("non-JIT plan file should exist after -o; stat err: %v", statErr) + } +} + +// TestInfraPlan_RejectionErrorContainsCanonicalKeywords is a defensive +// substring assertion. If a future refactor reformats the error string +// for any reason (cycle-N reviewer feedback, e.g.) but keeps the +// keywords stable, this test still catches the canonical-vocab +// expectations. The exact-match test above is the strict contract; +// this one is the operator-search-engine safety net. +func TestInfraPlan_RejectionErrorContainsCanonicalKeywords(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: app + type: infra.container_service + config: + env_vars: + DEP: "${vpc.id}" +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + planFile := filepath.Join(dir, "plan.json") + err := runInfraPlan([]string{"--config", cfgPath, "--output", planFile}) + if err == nil { + t.Fatal("expected error") + } + keywords := []string{ + "JIT resolution", + "persisted plan.json", + "wfctl infra apply", + "-o/--plan", + } + for _, kw := range keywords { + if !strings.Contains(err.Error(), kw) { + t.Errorf("error missing canonical keyword %q; got: %q", kw, err.Error()) + } + } +} diff --git a/cmd/wfctl/infra_plan_schema_test.go b/cmd/wfctl/infra_plan_schema_test.go new file mode 100644 index 000000000..1e5bf5adf --- /dev/null +++ b/cmd/wfctl/infra_plan_schema_test.go @@ -0,0 +1,130 @@ +package main + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestInfraPlan_SchemaVersionV1_NoJITRefs covers the baseline: a config +// with no JIT references (only plain ${VAR} env-var refs, which are +// resolved at plan time outside preserved keys and pass through preserved +// keys but don't require module-output resolution at apply) stamps the +// plan as V1. +func TestInfraPlan_SchemaVersionV1_NoJITRefs(t *testing.T) { + t.Setenv("STAGING_DB_PASSWORD", "secret") + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: app + type: infra.container_service + config: + env_vars: + DATABASE_URL: "postgres://user:${STAGING_DB_PASSWORD}@host:5432/db" +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + planFile := filepath.Join(dir, "plan.json") + if err := runInfraPlan([]string{"--config", cfgPath, "--output", planFile}); err != nil { + t.Fatalf("runInfraPlan: %v", err) + } + plan := readPlanFile(t, planFile) + if plan.SchemaVersion != infraPlanSchemaVersionV1 { + t.Errorf("SchemaVersion: got %d want %d (V1, no JIT refs)", + plan.SchemaVersion, infraPlanSchemaVersionV1) + } +} + +// TestInfraPlan_SchemaVersionV2_WhenJITModuleFieldRef is the canonical T5.4 +// scenario: a config whose env_vars carry a ${MODULE.field} reference (a +// JIT-required ref) bumps the plan to V2 so older wfctl binaries (which +// only read V1) reject the plan with the standard "newer than supported" +// diagnostic. T5.5 owns the persisted-plan rejection contract end-to-end; +// this test focuses on the planRequiresJITSubstitution helper logic via a +// direct in-process assertion (no CLI driving needed). +func TestInfraPlan_SchemaVersionV2_WhenJITModuleFieldRef(t *testing.T) { + // env_vars is a preserve-key submap, so ${pg.private_ip} survives + // plan-time ExpandEnvInMapPreservingKeys verbatim and lands in + // plan.Actions[0].Resource.Config["env_vars"]["DB_HOST"]. We assert + // directly against the helper rather than re-driving runInfraPlan, which + // would not surface the SchemaVersion bump without -o (and -o is + // rejected for V2 plans by T5.5's contract). + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{{ + Action: "create", + Resource: interfaces.ResourceSpec{ + Name: "app", Type: "infra.container_service", + Config: map[string]any{ + "env_vars": map[string]any{"DB_HOST": "${pg.private_ip}"}, + }, + }, + }}, + } + if !planRequiresJITSubstitution(plan) { + t.Errorf("planRequiresJITSubstitution: false; want true (JIT ref in env_vars)") + } +} + +// TestInfraPlan_SchemaVersionV2_WhenJITModuleIDRef verifies the .id form +// is also detected — a separate test because ${MODULE.id} is the most +// common JIT pattern (cascade-replace ID propagation). +func TestInfraPlan_SchemaVersionV2_WhenJITModuleIDRef(t *testing.T) { + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{{ + Action: "create", + Resource: interfaces.ResourceSpec{ + Name: "app", Type: "infra.container_service", + Config: map[string]any{"vpc_uuid": "${vpc.id}"}, + }, + }}, + } + if !planRequiresJITSubstitution(plan) { + t.Errorf("planRequiresJITSubstitution: false; want true (${vpc.id})") + } +} + +// TestInfraPlan_SchemaVersionV1_OnlyEnvVarRef_NoJIT verifies the negative +// case: a ${VAR} ref (no dot) does NOT trigger the V2 bump even when +// preserved through env_vars submaps. This protects the common case +// where operators use env vars for secrets without needing JIT. +func TestInfraPlan_SchemaVersionV1_OnlyEnvVarRef_NoJIT(t *testing.T) { + plan := &interfaces.IaCPlan{ + Actions: []interfaces.PlanAction{{ + Action: "create", + Resource: interfaces.ResourceSpec{ + Name: "app", Type: "infra.container_service", + Config: map[string]any{ + "env_vars": map[string]any{"DB_PASSWORD": "${PG_PASSWORD}"}, + }, + }, + }}, + } + if planRequiresJITSubstitution(plan) { + t.Errorf("planRequiresJITSubstitution: true; want false (only ${VAR})") + } +} + +// (T5.4's earlier persisted-end-to-end test was superseded by T5.5's +// rejection contract — V2 plans now error at -o time rather than being +// written to disk. The helper-only assertions above already cover the +// stamp-logic correctness; T5.5's own test file owns the rejection +// scenario end-to-end.) + +// readPlanFile is a small helper that loads + unmarshals a plan.json, +// failing the test on any IO/decode error. +func readPlanFile(t *testing.T, path string) *interfaces.IaCPlan { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read plan: %v", err) + } + var plan interfaces.IaCPlan + if err := json.Unmarshal(data, &plan); err != nil { + t.Fatalf("unmarshal plan: %v", err) + } + return &plan +} diff --git a/docs/adr/008-jit-substitution-at-dispatch-loop.md b/docs/adr/008-jit-substitution-at-dispatch-loop.md new file mode 100644 index 000000000..b31e8d86d --- /dev/null +++ b/docs/adr/008-jit-substitution-at-dispatch-loop.md @@ -0,0 +1,53 @@ +# ADR 008: JIT substitution lives at the ApplyPlan dispatch loop, not inside per-action helpers + +## Status + +Accepted + +## Context + +W-5 Task T5.3 (`docs/plans/2026-05-03-iac-conformance-and-replace.md`, line 2078) specified that the Replace-cascade contract — dependents of a just-replaced parent must see the parent's new ProviderID — be implemented by adding an explicit `jitsubst.ResolveSpec` call inside `wfctlhelpers.doReplace`, right before the post-Delete `driver.Create`: + +> ### Task T5.3: Wire JIT substitution into Replace action +> +> **Files:** Modify: `iac/wfctlhelpers/apply.go::doReplace` + +By the time T5.3 was up for implementation, T5.2 (committed as `424f8e1`) had already wired `jitsubst.ResolveSpec` at the dispatch-loop level — once per action, immediately before `dispatchAction`. The cascade contract was already satisfied: `doReplace` populates `result.ReplaceIDMap[name] = newProviderID` in iteration N; the loop's pre-dispatch `ResolveSpec` call in iteration N+1 sees the fresh entry and resolves `${parent.id}` against it before the dependent's driver call. + +The plan-spec defect is real: T5.2 absorbed T5.3's intended substitution work. Two ways to honor T5.3 in code remain: + +1. **Loop-level only (test-only T5.3)** — accept that the cascade behavior already works; ship verification tests + godoc clarification on `doReplace`; zero functional change to `apply.go`. +2. **Inner-resolve (defense-in-depth T5.3)** — also call `jitsubst.ResolveSpec` inside `doReplace` against the freshest `result.ReplaceIDMap`, requiring `syncedOutputs` (currently a local of `applyPlanWithEnvProvider`) to be threaded through `dispatchAction → doReplace` — a one-caller-only signature change. + +Spec-reviewer flagged option 1 as an architectural reinterpretation of a locked plan task and escalated to team-lead for arbitration. + +## Decision + +JIT substitution lives at exactly one site — the `applyPlanWithEnvProvider` dispatch loop, immediately before `dispatchAction`. Per-action helpers (`doCreate`, `doUpdate`, `doReplace`, `doDelete`) stay JIT-naive: they receive an already-resolved `action.Resource.Config` and call drivers directly. The Replace-cascade contract is honored by the temporal ordering of `result.ReplaceIDMap` writes (inside `doReplace`) versus `ResolveSpec` reads (top of next iteration), NOT by an explicit substitution call inside `doReplace`. + +T5.3 ships as `apply_replace_cascade_test.go` (two scenarios: `Replace+Create` and `Replace+Replace` cascades) plus a "# Cascade contract (T5.3)" section added to `doReplace`'s godoc explaining the loop-ordering invariant. + +## Consequences + +**Positive:** + +- Single substitution site (DRY): adding a new per-action helper (e.g., a future `doImport`) does not require duplicating `ResolveSpec` plumbing. +- `dispatchAction`'s signature stays narrow: `(ctx, driver, action, result)`. Threading `syncedOutputs` through the dispatch tier would have made the helper boundary leaky for one call site. +- Per-action helpers stay testable in isolation against a fake driver without a JIT fixture — the existing `apply_test.go` patterns continue to work. +- Cascade behavior is verifiable by black-box assertion against `ApplyPlan` (the apply_replace_cascade_test.go scenarios), which is closer to operator-observable contract than per-helper white-box assertions. + +**Negative:** + +- The cascade contract relies on a subtle invariant — `result.ReplaceIDMap` writes inside `doReplace` MUST happen before the next iteration's `ResolveSpec` read. A future refactor that batches `doReplace` calls or moves substitution out of the iteration loop would silently break the cascade. Mitigated by `apply_replace_cascade_test.go::TestApplyPlan_ReplaceCascade_DependentCreateGetsNewParentID` and `..._DependentReplaceGetsNewParentID`, which fail loudly if either of those refactors lands. +- Plan-deviation: the implementation diverges from `docs/plans/2026-05-03-iac-conformance-and-replace.md` §T5.3's "Modify `iac/wfctlhelpers/apply.go::doReplace`" instruction. This ADR records the rejected alternative (inner-resolve) and the reasoning so future contributors see the trade-off rather than rediscovering it via `git blame` archaeology. A future plan revision should reflect the actual implementation; out of scope for this ADR. +- Operators reading the apply log have no per-action breadcrumb that JIT resolution ran for a given action — substitution happens transparently. Mitigated by the per-action `jit substitution: ` ActionError surfaced when resolution fails (apply_jit_test.go locks the prefix). + +**Provenance:** decided by Claude (autonomous-pipeline team-lead) after spec-reviewer / implementer / team-lead consensus on 2026-05-04. spec-reviewer flagged the structural divergence as a meaningful PlanDiagnostic worth escalating; implementer surfaced the redundant-with-T5.2 observation; team-lead chose option-1 (test-only T5.3 + this ADR) over option-2 (inner-resolve rework) because the threaded-`syncedOutputs` API change was more invasive than the documentation cost of recording the choice durably. + +## References + +- Plan task spec: `docs/plans/2026-05-03-iac-conformance-and-replace.md` §T5.3 (line 2078) +- Loop-level substitution implementation: `iac/wfctlhelpers/apply.go::applyPlanWithEnvProvider` (commit `424f8e1`) +- Cascade verification tests: `iac/wfctlhelpers/apply_replace_cascade_test.go` (commit `3aecc97`) +- Cascade godoc: `iac/wfctlhelpers/apply.go::doReplace` "# Cascade contract (T5.3)" section +- Failure-path test that locks the substitution-skips-dispatch contract: `iac/wfctlhelpers/apply_jit_test.go::TestApplyPlan_JIT_UnresolvedRef_RecordsActionErrorAndSkipsDispatch` diff --git a/iac/jitsubst/jitsubst.go b/iac/jitsubst/jitsubst.go new file mode 100644 index 000000000..1a36c4a68 --- /dev/null +++ b/iac/jitsubst/jitsubst.go @@ -0,0 +1,308 @@ +// Package jitsubst implements just-in-time substitution for an IaC +// [interfaces.ResourceSpec] at apply time. +// +// # Why JIT +// +// Plan time computes a [interfaces.IaCPlan] over modules whose configs may +// reference values that don't exist yet — e.g. a database password that a +// sub-action will populate, or the ProviderID of a sibling resource that +// the same apply will create. wfctlhelpers.ApplyPlan calls +// [ResolveSpec] right before dispatching each [interfaces.PlanAction] so +// the driver sees fully-substituted Config. +// +// # Reference forms +// +// Three substitution forms are recognized inside any string value of +// [interfaces.ResourceSpec.Config] (recursively, including nested maps and +// slices): +// +// ${VAR} → envLookup(VAR) +// ${MODULE.id} → replaceIDMap[MODULE], else syncedOutputs[MODULE]["id"] +// ${MODULE.field} → syncedOutputs[MODULE][field] +// +// The discriminator between ${VAR} and ${MODULE.field} is the presence of +// a "." inside the body. Env-var names cannot contain "." per POSIX, so +// the rule is unambiguous. +// +// # Source precedence for ${MODULE.id} +// +// replaceIDMap is consulted FIRST. The map is populated by W-3b's +// doReplace [interfaces.ApplyResult.ReplaceIDMap] every time a Replace +// action successfully Delete-then-Creates a resource — its value is the +// post-Replace ProviderID. syncedOutputs holds outputs read from durable +// state (per W-5's design "JIT resolution reads from STATE … and from +// replaceIDMap"); state may have a stale ProviderID for a just-replaced +// resource until the apply loop persists the new one. Preferring +// replaceIDMap means dependents of a cascade-replaced parent see the new +// ID without depending on state-write ordering. +// +// # Strict resolution semantics +// +// Every reference MUST resolve. An unset env var, missing module, or +// missing field returns an error — matching the JIT contract that +// unresolved-at-apply-time refs are operator-actionable bugs, NOT +// silent-empty-string substitutions like [os.ExpandEnv]. Plan-time has +// already collapsed every resolvable ${VAR} via config.ExpandEnvInMap; a +// surviving env-var reference at apply time is therefore meaningful. +// +// # Error handling and partial state +// +// On error the original input spec is returned unmodified — callers MUST +// NOT use a partially-resolved spec since some fields may have substituted +// and others not. The first unresolved reference wins; resolution within a +// single string is left-to-right via Go's [regexp.Regexp.ReplaceAllStringFunc]. +// Across maps, keys are walked in sorted order so that error messages are +// deterministic across Go map-iteration randomization. +// +// # Mutation contract +// +// ResolveSpec deep-copies Config (including nested maps and slices) before +// substitution. Caller-side mutation of the returned spec cannot poison +// the input. +// +// # Lifecycle +// +// - T5.1 (this file) defines the helper. +// - T5.2 wires it into wfctlhelpers.ApplyPlan's per-action dispatch — +// a single ResolveSpec call per PlanAction immediately before driver +// dispatch. +// - T5.3 verifies cascade behavior: doReplace populates +// ApplyResult.ReplaceIDMap when a Replace action successfully +// Delete-then-Creates, and the existing T5.2 ResolveSpec call site +// consumes that map on subsequent PlanActions in the same apply +// loop. T5.3 does NOT add a second ResolveSpec call inside +// doReplace — see ADR 008 for the rationale (single resolution +// boundary, ReplaceIDMap-first ordering). +package jitsubst + +import ( + "fmt" + "regexp" + "slices" + "sort" + "strings" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// refRE matches a single ${...} reference. The body may contain any +// non-} character; further parsing distinguishes ${VAR} from +// ${MODULE.field} by the presence of a ".". +// +// We intentionally do NOT support the bare $VAR form (without braces): IaC +// Config values are user-provided cloud-API knobs where adjacent characters +// are common (e.g., paths, hostnames), and unbraced $VAR has no clear +// terminator across resource-name slugs containing dots. Plan-time +// config.ExpandEnvInMap handles the simpler cases via os.ExpandEnv; JIT-time +// is the strict-form residue. +var refRE = regexp.MustCompile(`\$\{([^}]*)\}`) + +// moduleRefRE matches a JIT-style ${MODULE.field} reference — any ${...} +// whose body has a "." separator AND non-empty module + field segments. +// Used by HasModuleRefs to gate plan.SchemaVersion bumping (T5.4) and the +// persisted-plan rejection (T5.5). Plain ${VAR} env-var references (no +// dot in body) do NOT match — they are NOT JIT-required at apply time +// since plan-time config.ExpandEnvInMap has already collapsed them +// (preserved-key submaps aside; those still resolve from env, not from +// other modules' outputs). +// +// Anchoring contract: the body MUST have at least one non-`.` character on +// EACH side of the first dot — `${.}`, `${.x}`, `${x.}` are NOT counted as +// JIT-required, matching jitsubst.ResolveSpec's strict rejection of those +// forms as malformed (they could never resolve at apply time anyway). +var moduleRefRE = regexp.MustCompile(`\$\{[^}.]+\.[^}]+\}`) + +// ResolveSpec returns a deep copy of spec where every ${...} reference in +// any string value of spec.Config (recursively, including nested maps and +// slices) has been resolved against the supplied substitution sources. +// See the package docstring for reference forms, source precedence, and +// strict-resolution semantics. +// +// Inputs: +// +// - replaceIDMap: resource Name → new ProviderID for resources replaced +// earlier in this apply (W-3b/T3.4 doReplace). +// - syncedOutputs: resource Name → outputs map (typed via state). +// - envLookup: env-var resolver; pass [os.LookupEnv] in production. May +// be nil — every ${VAR} reference will then error as unset, but ${...} +// references that don't reach the env-var path will still resolve. +// +// On error the input spec is returned unmodified; callers MUST NOT use a +// partially-resolved spec. +func ResolveSpec( + spec interfaces.ResourceSpec, + replaceIDMap map[string]string, + syncedOutputs map[string]map[string]any, + envLookup func(string) (string, bool), +) (interfaces.ResourceSpec, error) { + if spec.Config == nil { + return spec, nil + } + resolved, err := resolveAny(spec.Config, replaceIDMap, syncedOutputs, envLookup) + if err != nil { + return spec, err + } + out := spec + out.Config = resolved.(map[string]any) + return out, nil +} + +// HasModuleRefs returns true when any string value reachable from v +// (recursively walking map[string]any and []any) contains a JIT-style +// ${MODULE.field} reference — i.e., a ${...} reference whose body has a +// "." separator with non-empty segments on both sides. Plain ${VAR} +// env-var references (no dot in body) do NOT count. +// +// Used by cmd/wfctl/infra.go::runInfraPlan (T5.4) to decide whether to +// stamp plan.SchemaVersion = 2 (JIT-required) and by the persisted-plan +// rejection in T5.5. Walking input is intentionally permissive: pass any +// value (a single map[string]any, a slice, a ResourceSpec.Config, even a +// raw string) — non-string scalars are ignored, nil yields false. +func HasModuleRefs(v any) bool { + switch val := v.(type) { + case nil: + return false + case string: + return moduleRefRE.MatchString(val) + case map[string]any: + for _, vv := range val { + if HasModuleRefs(vv) { + return true + } + } + return false + case []any: + return slices.ContainsFunc(val, HasModuleRefs) + default: + return false + } +} + +// resolveAny walks any value in a ResourceSpec.Config tree, deep-copying +// maps and slices, recursing into them, and resolving ${...} references in +// any string leaves. Non-string scalars (int, bool, float, nil, etc.) pass +// through unchanged. Map keys are walked in sorted order so the error +// surfaced on a multi-error spec is deterministic regardless of Go's +// map-iteration randomization. +func resolveAny( + v any, + replaceIDMap map[string]string, + syncedOutputs map[string]map[string]any, + envLookup func(string) (string, bool), +) (any, error) { + switch val := v.(type) { + case string: + return resolveString(val, replaceIDMap, syncedOutputs, envLookup) + case map[string]any: + out := make(map[string]any, len(val)) + keys := make([]string, 0, len(val)) + for k := range val { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + r, err := resolveAny(val[k], replaceIDMap, syncedOutputs, envLookup) + if err != nil { + return nil, err + } + out[k] = r + } + return out, nil + case []any: + out := make([]any, len(val)) + for i, vv := range val { + r, err := resolveAny(vv, replaceIDMap, syncedOutputs, envLookup) + if err != nil { + return nil, err + } + out[i] = r + } + return out, nil + default: + // int/bool/float/nil/typed scalars: pass through. Callers that + // store custom struct values in Config receive them unchanged — + // JIT substitution is a string-value-only contract. + return v, nil + } +} + +// resolveString substitutes every ${...} reference in s. The first +// unresolved reference wins (leftmost-first via ReplaceAllStringFunc); the +// returned error names the offending reference body so an operator can +// trace it back to the spec. +func resolveString( + s string, + replaceIDMap map[string]string, + syncedOutputs map[string]map[string]any, + envLookup func(string) (string, bool), +) (string, error) { + var firstErr error + out := refRE.ReplaceAllStringFunc(s, func(match string) string { + if firstErr != nil { + // Short-circuit subsequent matches once an error has been + // recorded; the unmodified placeholder we return is unused + // since resolveString returns "" + error below. + return match + } + // match has the form "${...}" — strip the wrapping 2 + 1 chars. + body := match[2 : len(match)-1] + val, err := resolveRef(body, replaceIDMap, syncedOutputs, envLookup) + if err != nil { + firstErr = err + return match + } + return val + }) + if firstErr != nil { + return "", firstErr + } + return out, nil +} + +// resolveRef resolves a single reference body (the text between ${ and }). +// Reference forms and source precedence are documented at the package level. +func resolveRef( + body string, + replaceIDMap map[string]string, + syncedOutputs map[string]map[string]any, + envLookup func(string) (string, bool), +) (string, error) { + if body == "" { + return "", fmt.Errorf("malformed reference ${}: empty body") + } + if module, field, hasDot := strings.Cut(body, "."); hasDot { + if module == "" || field == "" { + return "", fmt.Errorf("malformed reference ${%s}: empty module or field", body) + } + if field == "id" { + if id, ok := replaceIDMap[module]; ok { + return id, nil + } + if outs, ok := syncedOutputs[module]; ok { + if v, ok := outs["id"]; ok { + return fmt.Sprintf("%v", v), nil + } + } + return "", fmt.Errorf("unresolved reference ${%s}: module %q has no .id in replaceIDMap or syncedOutputs", body, module) + } + outs, ok := syncedOutputs[module] + if !ok { + return "", fmt.Errorf("unresolved reference ${%s}: module %q not found in syncedOutputs", body, module) + } + v, ok := outs[field] + if !ok { + return "", fmt.Errorf("unresolved reference ${%s}: module %q has no field %q", body, module, field) + } + return fmt.Sprintf("%v", v), nil + } + // No dot → ${VAR} env-var lookup. A nil envLookup means "no env source + // configured" — equivalent to an unset var, so the same error surfaces. + if envLookup == nil { + return "", fmt.Errorf("unresolved reference ${%s}: env var not set (envLookup not configured)", body) + } + val, ok := envLookup(body) + if !ok { + return "", fmt.Errorf("unresolved reference ${%s}: env var not set", body) + } + return val, nil +} diff --git a/iac/jitsubst/jitsubst_test.go b/iac/jitsubst/jitsubst_test.go new file mode 100644 index 000000000..f808ae8ff --- /dev/null +++ b/iac/jitsubst/jitsubst_test.go @@ -0,0 +1,465 @@ +package jitsubst + +import ( + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// envFn is a tiny helper for building envLookup closures from a static map. +func envFn(m map[string]string) func(string) (string, bool) { + return func(k string) (string, bool) { + v, ok := m[k] + return v, ok + } +} + +// TestResolveSpec_NoRefs_PassesThrough verifies that a spec with no ${...} +// references is returned with Config equal to the input — but as a deep copy +// so caller mutation cannot poison the input. +func TestResolveSpec_NoRefs_PassesThrough(t *testing.T) { + spec := interfaces.ResourceSpec{ + Name: "vpc", + Type: "infra.vpc", + Config: map[string]any{ + "cidr": "10.0.0.0/16", + "tag": "prod", + }, + } + got, err := ResolveSpec(spec, nil, nil, nil) + if err != nil { + t.Fatalf("ResolveSpec: unexpected error: %v", err) + } + if got.Config["cidr"] != "10.0.0.0/16" || got.Config["tag"] != "prod" { + t.Errorf("expected Config preserved; got %v", got.Config) + } + // Confirm deep copy: mutating returned Config must not touch input. + got.Config["cidr"] = "mutated" + if spec.Config["cidr"] != "10.0.0.0/16" { + t.Errorf("input Config was mutated: %v", spec.Config) + } +} + +// TestResolveSpec_EnvVarReferenceResolved verifies that a bare ${VAR} (no dot) +// is resolved through envLookup. +func TestResolveSpec_EnvVarReferenceResolved(t *testing.T) { + spec := interfaces.ResourceSpec{ + Name: "pg", + Config: map[string]any{ + "password": "${PG_PASSWORD}", + }, + } + env := envFn(map[string]string{"PG_PASSWORD": "s3cret"}) + got, err := ResolveSpec(spec, nil, nil, env) + if err != nil { + t.Fatalf("ResolveSpec: unexpected error: %v", err) + } + if got.Config["password"] != "s3cret" { + t.Errorf("password: got %q want %q", got.Config["password"], "s3cret") + } +} + +// TestResolveSpec_EnvVarUnset_Errors verifies that a ${VAR} whose name has no +// dot AND whose value is missing from envLookup returns an unresolved-reference +// error — JIT semantics demand strictness, unlike os.ExpandEnv. +func TestResolveSpec_EnvVarUnset_Errors(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{"x": "${MISSING}"}, + } + _, err := ResolveSpec(spec, nil, nil, envFn(nil)) + if err == nil { + t.Fatalf("expected error for unset env var, got nil") + } + if !strings.Contains(err.Error(), "MISSING") { + t.Errorf("error should mention the missing var name; got %q", err) + } +} + +// TestResolveSpec_NilEnvLookup_TreatsAllEnvVarsAsUnset verifies that callers +// that legitimately have no env-var source (e.g., test fixtures) get a clear +// error — not a nil-deref panic — when a ${VAR} is encountered. +func TestResolveSpec_NilEnvLookup_TreatsAllEnvVarsAsUnset(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{"x": "${FOO}"}, + } + _, err := ResolveSpec(spec, nil, nil, nil) + if err == nil { + t.Fatalf("expected error for nil envLookup with ${VAR} ref") + } +} + +// TestResolveSpec_ModuleField_ResolvedFromSyncedOutputs verifies that a +// ${MODULE.field} reference (non-id field) is resolved against the synced +// outputs of the named module. +func TestResolveSpec_ModuleField_ResolvedFromSyncedOutputs(t *testing.T) { + spec := interfaces.ResourceSpec{ + Name: "app", + Config: map[string]any{ + "db_host": "${pg.private_ip}", + }, + } + synced := map[string]map[string]any{ + "pg": {"private_ip": "10.0.0.5"}, + } + got, err := ResolveSpec(spec, nil, synced, nil) + if err != nil { + t.Fatalf("ResolveSpec: unexpected error: %v", err) + } + if got.Config["db_host"] != "10.0.0.5" { + t.Errorf("db_host: got %q want %q", got.Config["db_host"], "10.0.0.5") + } +} + +// TestResolveSpec_ModuleID_PrefersReplaceIDMap verifies that ${MODULE.id} +// resolves from the replaceIDMap first — even if syncedOutputs also has an +// `id` field. This makes cascade-replace ProviderID propagation authoritative +// over potentially-stale state outputs. +func TestResolveSpec_ModuleID_PrefersReplaceIDMap(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{"vpc_uuid": "${vpc.id}"}, + } + replace := map[string]string{"vpc": "new-uuid-after-replace"} + synced := map[string]map[string]any{ + "vpc": {"id": "old-uuid-from-state"}, + } + got, err := ResolveSpec(spec, replace, synced, nil) + if err != nil { + t.Fatalf("ResolveSpec: unexpected error: %v", err) + } + if got.Config["vpc_uuid"] != "new-uuid-after-replace" { + t.Errorf("vpc_uuid: got %q want replace-map value", got.Config["vpc_uuid"]) + } +} + +// TestResolveSpec_ModuleID_FallsBackToSyncedOutputs verifies that when the +// replaceIDMap has no entry for the module (the common case — module was +// created, not replaced, in this apply), ${MODULE.id} falls back to +// syncedOutputs[module]["id"]. +func TestResolveSpec_ModuleID_FallsBackToSyncedOutputs(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{"vpc_uuid": "${vpc.id}"}, + } + synced := map[string]map[string]any{ + "vpc": {"id": "vpc-from-state-12345"}, + } + got, err := ResolveSpec(spec, nil, synced, nil) + if err != nil { + t.Fatalf("ResolveSpec: unexpected error: %v", err) + } + if got.Config["vpc_uuid"] != "vpc-from-state-12345" { + t.Errorf("vpc_uuid: got %q want syncedOutputs.id", got.Config["vpc_uuid"]) + } +} + +// TestResolveSpec_ModuleID_UnknownModule_Errors verifies that ${MODULE.id} +// for a module absent from BOTH replaceIDMap and syncedOutputs returns an +// unresolved-reference error. +func TestResolveSpec_ModuleID_UnknownModule_Errors(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{"x": "${ghost.id}"}, + } + _, err := ResolveSpec(spec, nil, nil, nil) + if err == nil { + t.Fatalf("expected error for unknown module ${ghost.id}") + } + if !strings.Contains(err.Error(), "ghost") { + t.Errorf("error should mention the missing module; got %q", err) + } +} + +// TestResolveSpec_ModuleField_UnknownModule_Errors verifies that +// ${MODULE.field} (non-id) for an unknown module errors clearly. +func TestResolveSpec_ModuleField_UnknownModule_Errors(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{"x": "${ghost.private_ip}"}, + } + _, err := ResolveSpec(spec, nil, map[string]map[string]any{}, nil) + if err == nil { + t.Fatalf("expected error for unknown module ${ghost.private_ip}") + } +} + +// TestResolveSpec_ModuleField_UnknownField_Errors verifies that +// ${MODULE.field} for a known module but unknown field errors clearly. +func TestResolveSpec_ModuleField_UnknownField_Errors(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{"x": "${pg.nonexistent}"}, + } + synced := map[string]map[string]any{"pg": {"private_ip": "10.0.0.5"}} + _, err := ResolveSpec(spec, nil, synced, nil) + if err == nil { + t.Fatalf("expected error for unknown field ${pg.nonexistent}") + } + if !strings.Contains(err.Error(), "nonexistent") { + t.Errorf("error should mention the missing field; got %q", err) + } +} + +// TestResolveSpec_NestedMapsAndSlices_RecursivelyResolved verifies that +// substitution walks nested map[string]any and []any structures. +func TestResolveSpec_NestedMapsAndSlices_RecursivelyResolved(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{ + "env": map[string]any{ + "DATABASE_URL": "postgres://${pg.private_ip}/db", + }, + "args": []any{"--vpc=${vpc.id}", "--port=5432"}, + }, + } + replace := map[string]string{"vpc": "vpc-abc"} + synced := map[string]map[string]any{ + "pg": {"private_ip": "10.0.0.5"}, + } + got, err := ResolveSpec(spec, replace, synced, nil) + if err != nil { + t.Fatalf("ResolveSpec: unexpected error: %v", err) + } + envMap, ok := got.Config["env"].(map[string]any) + if !ok { + t.Fatalf("env: not a map: %T", got.Config["env"]) + } + if envMap["DATABASE_URL"] != "postgres://10.0.0.5/db" { + t.Errorf("DATABASE_URL: got %q", envMap["DATABASE_URL"]) + } + args, ok := got.Config["args"].([]any) + if !ok { + t.Fatalf("args: not a slice: %T", got.Config["args"]) + } + if args[0] != "--vpc=vpc-abc" { + t.Errorf("args[0]: got %q", args[0]) + } + if args[1] != "--port=5432" { + t.Errorf("args[1] (no refs) should pass through; got %q", args[1]) + } +} + +// TestResolveSpec_DoesNotMutateInputConfig is a defensive double-check that +// the deep-copy contract holds even when nested structures are involved. +func TestResolveSpec_DoesNotMutateInputConfig(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{ + "env": map[string]any{"X": "${FOO}"}, + }, + } + env := envFn(map[string]string{"FOO": "resolved"}) + _, err := ResolveSpec(spec, nil, nil, env) + if err != nil { + t.Fatalf("ResolveSpec: %v", err) + } + envMap := spec.Config["env"].(map[string]any) + if envMap["X"] != "${FOO}" { + t.Errorf("input nested map mutated: X = %q", envMap["X"]) + } +} + +// TestResolveSpec_NonStringScalars_PreservedAsIs verifies that ints, bools, +// and other non-string scalars in Config are passed through unchanged. +func TestResolveSpec_NonStringScalars_PreservedAsIs(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{ + "port": 5432, + "enabled": true, + "ratio": 0.5, + }, + } + got, err := ResolveSpec(spec, nil, nil, nil) + if err != nil { + t.Fatalf("ResolveSpec: %v", err) + } + if got.Config["port"] != 5432 || got.Config["enabled"] != true || got.Config["ratio"] != 0.5 { + t.Errorf("scalars not preserved: %v", got.Config) + } +} + +// TestResolveSpec_NonStringOutputValue_StringifiedFmtV verifies that when an +// output value in syncedOutputs is non-string (e.g., int port), it's +// stringified via fmt.Sprintf("%v", v) before substitution. +func TestResolveSpec_NonStringOutputValue_StringifiedFmtV(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{"x": "port=${pg.port}"}, + } + synced := map[string]map[string]any{"pg": {"port": 5432}} + got, err := ResolveSpec(spec, nil, synced, nil) + if err != nil { + t.Fatalf("ResolveSpec: %v", err) + } + if got.Config["x"] != "port=5432" { + t.Errorf("x: got %q want %q", got.Config["x"], "port=5432") + } +} + +// TestResolveSpec_MultipleRefsInSingleString_AllResolved verifies that a +// single string with multiple ${...} refs has every ref substituted. +func TestResolveSpec_MultipleRefsInSingleString_AllResolved(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{ + "url": "postgres://user:${PG_PASSWORD}@${pg.private_ip}:${pg.port}/db", + }, + } + env := envFn(map[string]string{"PG_PASSWORD": "s3cret"}) + synced := map[string]map[string]any{"pg": {"private_ip": "10.0.0.5", "port": 5432}} + got, err := ResolveSpec(spec, nil, synced, env) + if err != nil { + t.Fatalf("ResolveSpec: %v", err) + } + want := "postgres://user:s3cret@10.0.0.5:5432/db" + if got.Config["url"] != want { + t.Errorf("url: got %q want %q", got.Config["url"], want) + } +} + +// TestResolveSpec_MalformedRef_EmptyBody_Errors verifies that ${} (empty body) +// is rejected as a malformed reference rather than silently substituting "". +func TestResolveSpec_MalformedRef_EmptyBody_Errors(t *testing.T) { + spec := interfaces.ResourceSpec{ + Config: map[string]any{"x": "${}"}, + } + _, err := ResolveSpec(spec, nil, nil, nil) + if err == nil { + t.Fatalf("expected error for malformed ${} ref") + } +} + +// TestResolveSpec_MalformedRef_DotOnly_Errors verifies that ${.} or +// ${module.} or ${.field} are rejected. +func TestResolveSpec_MalformedRef_DotOnly_Errors(t *testing.T) { + cases := []string{"${.}", "${module.}", "${.field}"} + for _, ref := range cases { + spec := interfaces.ResourceSpec{ + Config: map[string]any{"x": ref}, + } + if _, err := ResolveSpec(spec, nil, nil, nil); err == nil { + t.Errorf("expected error for malformed ref %q", ref) + } + } +} + +// TestResolveSpec_NilConfig_NoOp verifies that a spec with nil Config is +// returned unchanged with no error. +func TestResolveSpec_NilConfig_NoOp(t *testing.T) { + spec := interfaces.ResourceSpec{Name: "x", Type: "infra.x"} + got, err := ResolveSpec(spec, nil, nil, nil) + if err != nil { + t.Fatalf("ResolveSpec: %v", err) + } + if got.Config != nil { + t.Errorf("Config should remain nil; got %v", got.Config) + } + if got.Name != "x" || got.Type != "infra.x" { + t.Errorf("identity fields not preserved: %+v", got) + } +} + +// TestHasModuleRefs_PlainEnvVar_False verifies that a ${VAR} (no dot in +// the body) is NOT classified as a JIT reference — env-var refs do not +// require apply-time module-output resolution. +func TestHasModuleRefs_PlainEnvVar_False(t *testing.T) { + cfg := map[string]any{"x": "${SOME_ENV}"} + if HasModuleRefs(cfg) { + t.Errorf("plain ${VAR} should NOT be classified as JIT") + } +} + +// TestHasModuleRefs_ModuleField_True verifies the canonical positive case. +func TestHasModuleRefs_ModuleField_True(t *testing.T) { + cfg := map[string]any{"x": "${pg.private_ip}"} + if !HasModuleRefs(cfg) { + t.Errorf("${pg.private_ip} should be classified as JIT") + } +} + +// TestHasModuleRefs_ModuleID_True verifies the .id form is also classified. +func TestHasModuleRefs_ModuleID_True(t *testing.T) { + cfg := map[string]any{"x": "${vpc.id}"} + if !HasModuleRefs(cfg) { + t.Errorf("${vpc.id} should be classified as JIT") + } +} + +// TestHasModuleRefs_NestedMap_True verifies recursion into nested maps. +func TestHasModuleRefs_NestedMap_True(t *testing.T) { + cfg := map[string]any{ + "env_vars": map[string]any{"DB_HOST": "${pg.private_ip}"}, + } + if !HasModuleRefs(cfg) { + t.Errorf("nested ${pg.private_ip} should be classified as JIT") + } +} + +// TestHasModuleRefs_NestedSlice_True verifies recursion into slice elements. +func TestHasModuleRefs_NestedSlice_True(t *testing.T) { + cfg := map[string]any{ + "args": []any{"--vpc=${vpc.id}"}, + } + if !HasModuleRefs(cfg) { + t.Errorf("ref inside slice element should be classified as JIT") + } +} + +// TestHasModuleRefs_NoRefs_False verifies the negative case: plain values +// with no ${...} references at all. +func TestHasModuleRefs_NoRefs_False(t *testing.T) { + cfg := map[string]any{ + "cidr": "10.0.0.0/16", + "region": "nyc3", + "port": 5432, + } + if HasModuleRefs(cfg) { + t.Errorf("ref-free config should NOT be classified as JIT") + } +} + +// TestHasModuleRefs_NilValue_False verifies the safe-on-nil contract. +func TestHasModuleRefs_NilValue_False(t *testing.T) { + if HasModuleRefs(nil) { + t.Errorf("nil should NOT be classified as JIT") + } +} + +// TestHasModuleRefs_MalformedRef_False verifies that ${.}, ${.x}, ${x.} +// are NOT classified as JIT — they could not resolve at apply time anyway, +// so bumping SchemaVersion=2 for them would force a rejection on a plan +// that's structurally broken regardless of JIT support. +func TestHasModuleRefs_MalformedRef_False(t *testing.T) { + for _, body := range []string{"${.}", "${.x}", "${x.}"} { + cfg := map[string]any{"x": body} + if HasModuleRefs(cfg) { + t.Errorf("malformed ref %q should NOT be classified as JIT", body) + } + } +} + +// TestHasModuleRefs_MixedString_True verifies a ref embedded in a longer +// string (prefix/suffix text) is still detected. +func TestHasModuleRefs_MixedString_True(t *testing.T) { + cfg := map[string]any{"x": "postgres://user:${PG_PASSWORD}@${pg.private_ip}/db"} + if !HasModuleRefs(cfg) { + t.Errorf("embedded ${pg.private_ip} should be classified as JIT") + } +} + +// TestResolveSpec_OnError_ReturnsInputSpecUnchanged verifies the error +// contract: when substitution fails, the returned ResourceSpec is the +// original (untouched) input — callers MUST NOT use a partially-resolved +// spec since some fields may have substituted and others not. +func TestResolveSpec_OnError_ReturnsInputSpecUnchanged(t *testing.T) { + spec := interfaces.ResourceSpec{ + Name: "app", + Config: map[string]any{ + "good": "${KNOWN}", + "bad": "${UNKNOWN}", + }, + } + env := envFn(map[string]string{"KNOWN": "ok"}) + got, err := ResolveSpec(spec, nil, nil, env) + if err == nil { + t.Fatalf("expected error; got %+v", got) + } + // The returned spec must be the input — same Config map identity-wise + // is not required (callers may rely on either), but the values must + // match the unresolved originals. + if got.Config["good"] != "${KNOWN}" || got.Config["bad"] != "${UNKNOWN}" { + t.Errorf("error path leaked partial substitution: %+v", got.Config) + } +} diff --git a/iac/wfctlhelpers/apply.go b/iac/wfctlhelpers/apply.go index 871ce431b..7f824b3f0 100644 --- a/iac/wfctlhelpers/apply.go +++ b/iac/wfctlhelpers/apply.go @@ -43,8 +43,11 @@ import ( "errors" "fmt" "log" + "maps" + "os" "github.com/GoCodeAlone/workflow/iac/inputsnapshot" + "github.com/GoCodeAlone/workflow/iac/jitsubst" "github.com/GoCodeAlone/workflow/interfaces" ) @@ -122,6 +125,17 @@ func applyPlanWithEnvProvider( result.InputDriftReport = inputsnapshot.ComputeDrift(plan.InputSnapshot, applyTimeSnap) }() + // syncedOutputs maps resource Name → flat outputs map (with the + // canonical "id" key shadowed by ProviderID when set). Pre-populated + // from every action.Current entry so a NEW action can reference + // existing in-state modules' outputs from action zero — not just + // modules whose action has already run in this loop. Mutated below + // after each successful dispatch so subsequent actions see this-apply + // outputs (the W-5 design's "JIT resolution reads from STATE" path: + // new outputs are written per-resource on success and become visible + // to later actions in the same plan). + syncedOutputs := buildInitialSyncedOutputs(plan.Actions) + for _, action := range plan.Actions { // Honor cancellation at the loop boundary. Drivers should also // check ctx internally for in-flight work, but the loop check @@ -132,6 +146,26 @@ func applyPlanWithEnvProvider( if err := ctx.Err(); err != nil { return result, err } + // Per-action JIT substitution — resolve ${VAR} / ${MODULE.field} + // / ${MODULE.id} in action.Resource.Config against + // result.ReplaceIDMap (this-apply Replace ProviderIDs) and + // syncedOutputs (state + this-apply prior outputs). On error, + // record a per-action diagnostic with the canonical "jit + // substitution:" prefix and SKIP dispatch — the unresolved spec + // must not reach the driver. The loop continues to the next + // action (best-effort apply contract). os.LookupEnv is the + // production env source; nil-safe inside ResolveSpec — refs that + // only need replaceIDMap / syncedOutputs still resolve. + resolved, err := jitsubst.ResolveSpec(action.Resource, result.ReplaceIDMap, syncedOutputs, os.LookupEnv) + if err != nil { + result.Errors = append(result.Errors, interfaces.ActionError{ + Resource: action.Resource.Name, + Action: action.Action, + Error: fmt.Sprintf("jit substitution: %v", err), + }) + continue + } + action.Resource = resolved d, err := p.ResourceDriver(action.Resource.Type) if err != nil { result.Errors = append(result.Errors, interfaces.ActionError{ @@ -141,6 +175,11 @@ func applyPlanWithEnvProvider( }) continue } + // Capture result.Resources length pre-dispatch so we can identify + // the entry (if any) that this action appended and propagate its + // outputs into syncedOutputs for subsequent actions. doCreate / + // doUpdate / doReplace each append on success; doDelete does not. + preLen := len(result.Resources) if err := dispatchAction(ctx, d, action, result); err != nil { result.Errors = append(result.Errors, interfaces.ActionError{ Resource: action.Resource.Name, @@ -148,10 +187,61 @@ func applyPlanWithEnvProvider( Error: err.Error(), }) } + if len(result.Resources) > preLen { + out := result.Resources[len(result.Resources)-1] + syncedOutputs[out.Name] = flattenOutputs(out) + } } return result, nil } +// buildInitialSyncedOutputs walks plan.Actions once and returns a map of +// every action.Current entry's outputs (keyed by Name). Used to seed the +// JIT substitution map before the dispatch loop begins so a brand-new +// action created in this plan can reference an in-state sibling module's +// outputs even if the sibling has no action of its own — or if its +// action runs later in the loop. Each entry is a flat copy (output +// fields plus the canonical "id" key shadowed with the state-side +// ProviderID). +func buildInitialSyncedOutputs(actions []interfaces.PlanAction) map[string]map[string]any { + out := make(map[string]map[string]any) + for _, a := range actions { + if a.Current == nil { + continue + } + out[a.Current.Name] = flattenStateOutputs(a.Current) + } + return out +} + +// flattenStateOutputs returns a flat outputs map for a ResourceState: +// the state's Outputs entries plus a canonical "id" key set to the +// state's ProviderID. The "id" override is intentional — JIT +// substitution treats ${MODULE.id} as the canonical ProviderID +// reference; if a state's Outputs map happens to also have an "id" +// key, the ProviderID still wins so JIT semantics stay predictable. +func flattenStateOutputs(s *interfaces.ResourceState) map[string]any { + m := make(map[string]any, len(s.Outputs)+1) + maps.Copy(m, s.Outputs) + if s.ProviderID != "" { + m["id"] = s.ProviderID + } + return m +} + +// flattenOutputs returns a flat outputs map for a freshly-applied +// ResourceOutput. Mirrors flattenStateOutputs but reads from +// ResourceOutput rather than ResourceState — same canonical "id" rule: +// ProviderID shadows any "id" entry in Outputs. +func flattenOutputs(o interfaces.ResourceOutput) map[string]any { + m := make(map[string]any, len(o.Outputs)+1) + maps.Copy(m, o.Outputs) + if o.ProviderID != "" { + m["id"] = o.ProviderID + } + return m +} + // snapshotKeys returns the keys of m as an unordered slice. ComputeDrift // sorts its output, and Snapshot iterates in any order, so no key sort // is needed at this stage. Inlined helper to keep the dependency @@ -271,9 +361,24 @@ func doUpdate(ctx context.Context, d interfaces.ResourceDriver, action interface // doReplace decomposes a Replace action into Delete-then-Create on the // driver and propagates the new ProviderID through -// result.ReplaceIDMap[action.Resource.Name] so JIT substitution in W-5 -// can patch dependent resources whose configs reference the replaced -// resource by name. +// result.ReplaceIDMap[action.Resource.Name] so the JIT substitution wired +// into ApplyPlan's loop (T5.2) can patch dependent resources whose +// configs reference the replaced resource by name. +// +// # Cascade contract (T5.3) +// +// When a plan has [Replace parent, X dependent] where dependent's +// Config carries ${parent.id}, the cascade lands automatically: +// doReplace's post-Create write to result.ReplaceIDMap completes +// BEFORE the dispatch loop's next iteration calls +// jitsubst.ResolveSpec on the dependent's spec, so the dependent's +// driver call (Create or Replace's post-Delete Create) sees the +// freshly-resolved parent ProviderID. Delete continues to use +// action.Current.ProviderID via refFromAction — JIT substitution does +// NOT alter action.Current, so Replace's Delete still targets the +// pre-Replace cloud resource. +// +// Verified by apply_replace_cascade_test.go. // // Failure semantics: // - Delete fails → return wrapped "replace: delete: "; Create diff --git a/iac/wfctlhelpers/apply_jit_test.go b/iac/wfctlhelpers/apply_jit_test.go new file mode 100644 index 000000000..a81c69c37 --- /dev/null +++ b/iac/wfctlhelpers/apply_jit_test.go @@ -0,0 +1,261 @@ +package wfctlhelpers + +import ( + "context" + "strings" + "sync" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// jitRecordingDriver remembers every ResourceSpec.Config map it receives +// across Create / Update so JIT-substitution tests can assert exactly what +// the driver saw post-substitution. Per-instance: each test gets its own +// driver via newJITRecordingProvider(). +// +// Per-resource Create return values are configurable via createReturns — +// when set, the driver returns the matching ResourceOutput for spec.Name; +// otherwise it falls back to the standard fake-id- shape. +type jitRecordingDriver struct { + *fakeDriver + mu sync.Mutex + seenConfigs map[string]map[string]any // resource Name → Config seen on Create + seenUpdateCfgs map[string]map[string]any // resource Name → Config seen on Update + createReturns map[string]*interfaces.ResourceOutput +} + +func (d *jitRecordingDriver) Create(_ context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.fakeDriver.createCount++ + d.mu.Lock() + if d.seenConfigs == nil { + d.seenConfigs = make(map[string]map[string]any) + } + d.seenConfigs[spec.Name] = spec.Config + d.mu.Unlock() + if out, ok := d.createReturns[spec.Name]; ok && out != nil { + return out, nil + } + return &interfaces.ResourceOutput{Name: spec.Name, Type: spec.Type, ProviderID: "fake-id-" + spec.Name}, nil +} + +func (d *jitRecordingDriver) Update(_ context.Context, ref interfaces.ResourceRef, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + d.fakeDriver.updateCount++ + d.mu.Lock() + if d.seenUpdateCfgs == nil { + d.seenUpdateCfgs = make(map[string]map[string]any) + } + d.seenUpdateCfgs[spec.Name] = spec.Config + d.mu.Unlock() + return &interfaces.ResourceOutput{Name: spec.Name, Type: spec.Type, ProviderID: ref.ProviderID}, nil +} + +// jitRecordingProvider returns the same jitRecordingDriver for every +// resource type so a single instance records all dispatches in one +// ApplyPlan call. +type jitRecordingProvider struct { + *fakeProvider + driver *jitRecordingDriver +} + +func newJITRecordingProvider() *jitRecordingProvider { + base := newFakeProvider() + return &jitRecordingProvider{ + fakeProvider: base, + driver: &jitRecordingDriver{fakeDriver: base.driver}, + } +} + +func (p *jitRecordingProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return p.driver, nil +} + +// specWithConfig builds a ResourceSpec with a Config map for JIT tests. +// Mirrors the existing spec() helper but adds the Config field that +// JIT-substitution targets. +func specWithConfig(name, typ string, cfg map[string]any) interfaces.ResourceSpec { + return interfaces.ResourceSpec{Name: name, Type: typ, Config: cfg} +} + +// TestApplyPlan_JIT_TwoCreate_BSpecResolvesAID is the canonical T5.2 +// scenario from the plan: a 2-action plan (create A, then create B) where +// B's Config references ${A.id}. ApplyPlan must call ResolveSpec on B +// before dispatching, so the driver sees B's Config["vpc_uuid"] resolved +// to A's freshly-minted ProviderID. +func TestApplyPlan_JIT_TwoCreate_BSpecResolvesAID(t *testing.T) { + fp := newJITRecordingProvider() + fp.driver.createReturns = map[string]*interfaces.ResourceOutput{ + "a-vpc": {Name: "a-vpc", Type: "infra.vpc", ProviderID: "vpc-uuid-12345"}, + } + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: specWithConfig("a-vpc", "infra.vpc", map[string]any{ + "cidr": "10.0.0.0/16", + })}, + {Action: "create", Resource: specWithConfig("b-app", "infra.app", map[string]any{ + "vpc_uuid": "${a-vpc.id}", + })}, + }} + + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 0 { + t.Fatalf("unexpected per-action errors: %+v", result.Errors) + } + + bConfig := fp.driver.seenConfigs["b-app"] + if bConfig == nil { + t.Fatalf("driver did not receive B's Config; seenConfigs=%+v", fp.driver.seenConfigs) + } + if got, want := bConfig["vpc_uuid"], "vpc-uuid-12345"; got != want { + t.Errorf("B.Config[vpc_uuid] post-substitution: got %q want %q (full B Config: %+v)", + got, want, bConfig) + } +} + +// TestApplyPlan_JIT_PreSyncedFromActionCurrentState verifies that +// syncedOutputs is pre-populated from action.Current entries — modules +// already in state can be referenced by later actions in the same plan +// without depending on their action running first. +func TestApplyPlan_JIT_PreSyncedFromActionCurrentState(t *testing.T) { + fp := newJITRecordingProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + // "vpc" is in state; the only action against it here is delete (a + // no-op for syncedOutputs propagation, but Current.Outputs + + // Current.ProviderID seeds syncedOutputs at start-of-apply). + {Action: "delete", Resource: spec("old-thing", "infra.misc"), + Current: &interfaces.ResourceState{Name: "old-thing", ProviderID: "old"}}, + // New action references the *existing* in-state module's outputs + // (here: the "vpc" Current passed alongside another action). + {Action: "create", Resource: specWithConfig("app", "infra.app", map[string]any{ + "vpc_uuid": "${vpc.id}", + "vpc_cidr": "${vpc.cidr}", + "db_secret": "${vpc.region}", + }), + Current: nil, + }, + }} + // Plant "vpc" via a third action.Current — could be on any non-app + // action in the same plan. We attach it to the first action's + // Current.Name to demonstrate that all action.Current entries seed + // syncedOutputs, not just the action's own. + plan.Actions[0].Current = &interfaces.ResourceState{ + Name: "vpc", ProviderID: "vpc-state-id", + Outputs: map[string]any{"cidr": "10.0.0.0/16", "region": "nyc3"}, + } + + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 0 { + t.Fatalf("unexpected per-action errors: %+v", result.Errors) + } + + appConfig := fp.driver.seenConfigs["app"] + if appConfig == nil { + t.Fatalf("driver did not receive app's Config") + } + if got, want := appConfig["vpc_uuid"], "vpc-state-id"; got != want { + t.Errorf("vpc_uuid: got %q want %q", got, want) + } + if got, want := appConfig["vpc_cidr"], "10.0.0.0/16"; got != want { + t.Errorf("vpc_cidr: got %q want %q", got, want) + } + if got, want := appConfig["db_secret"], "nyc3"; got != want { + t.Errorf("db_secret: got %q want %q", got, want) + } +} + +// TestApplyPlan_JIT_UnresolvedRef_RecordsActionErrorAndSkipsDispatch +// verifies the JIT failure contract: a reference that cannot be resolved +// (unknown module / field / env var) must NOT be silently swallowed. The +// per-action error surfaces with the canonical "jit substitution:" prefix +// and the driver MUST NOT see the un-resolved spec — Create count for +// the offending action must be 0. +func TestApplyPlan_JIT_UnresolvedRef_RecordsActionErrorAndSkipsDispatch(t *testing.T) { + fp := newJITRecordingProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: specWithConfig("dependent", "infra.app", map[string]any{ + "vpc_uuid": "${ghost.id}", // ghost has no state, no replace-id + })}, + }} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 1 { + t.Fatalf("expected 1 per-action error from unresolved ref; got %d (%+v)", + len(result.Errors), result.Errors) + } + got := result.Errors[0] + if got.Resource != "dependent" || got.Action != "create" { + t.Errorf("error shape: got %+v", got) + } + if !strings.HasPrefix(got.Error, "jit substitution:") { + t.Errorf("error must use canonical 'jit substitution:' prefix; got %q", got.Error) + } + if !strings.Contains(got.Error, "ghost") { + t.Errorf("error should mention the missing module name; got %q", got.Error) + } + if fp.driver.createCount != 0 { + t.Errorf("dispatch must be skipped on JIT failure; createCount=%d", fp.driver.createCount) + } +} + +// TestApplyPlan_JIT_NoRefsInConfig_PassesThroughUnchanged covers the +// common case (most plan actions have no JIT refs): the driver receives +// the same Config it had in plan.Actions[i].Resource.Config — no +// behavior change. Locks the contract that JIT is purely additive. +func TestApplyPlan_JIT_NoRefsInConfig_PassesThroughUnchanged(t *testing.T) { + fp := newJITRecordingProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: specWithConfig("vpc", "infra.vpc", map[string]any{ + "cidr": "10.0.0.0/16", + "region": "nyc3", + })}, + }} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 0 { + t.Fatalf("unexpected per-action errors: %+v", result.Errors) + } + got := fp.driver.seenConfigs["vpc"] + if got["cidr"] != "10.0.0.0/16" || got["region"] != "nyc3" { + t.Errorf("Config altered for ref-free spec; got %+v", got) + } +} + +// TestApplyPlan_JIT_LoopContinuesAfterPerActionJITError is a +// best-effort-loop check: if action[0]'s JIT fails, action[1] (which +// has no refs) must still dispatch. Mirrors the +// TestApplyPlan_LoopContinuesAfterPerActionFailure contract for the new +// JIT failure mode. +func TestApplyPlan_JIT_LoopContinuesAfterPerActionJITError(t *testing.T) { + fp := newJITRecordingProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: specWithConfig("bad", "infra.app", map[string]any{ + "x": "${ghost.id}", + })}, + {Action: "create", Resource: specWithConfig("good", "infra.vpc", map[string]any{ + "cidr": "10.0.0.0/16", + })}, + }} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 1 { + t.Fatalf("expected exactly 1 per-action error from action[0]; got %d (%+v)", + len(result.Errors), result.Errors) + } + if fp.driver.createCount != 1 { + t.Errorf("action[1] should have dispatched; createCount=%d want 1", fp.driver.createCount) + } + if fp.driver.seenConfigs["good"]["cidr"] != "10.0.0.0/16" { + t.Errorf("action[1] Config not seen by driver; seenConfigs=%+v", fp.driver.seenConfigs) + } +} diff --git a/iac/wfctlhelpers/apply_replace_cascade_test.go b/iac/wfctlhelpers/apply_replace_cascade_test.go new file mode 100644 index 000000000..9f8a639b0 --- /dev/null +++ b/iac/wfctlhelpers/apply_replace_cascade_test.go @@ -0,0 +1,131 @@ +package wfctlhelpers + +import ( + "context" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestApplyPlan_ReplaceCascade_DependentCreateGetsNewParentID is the +// canonical T5.3 scenario: a 2-action plan where action[0] is a Replace +// of "parent" (Delete + Create yielding a NEW ProviderID), and action[1] +// is a Create of "dependent" whose Config references ${parent.id}. +// +// The cascade contract (specified in W-5): +// +// 1. Action 0 (Replace parent): doReplace populates +// result.ReplaceIDMap["parent"] = new-uuid via the post-Delete Create. +// 2. Action 1 (Create dependent): the loop's pre-dispatch +// jitsubst.ResolveSpec call sees result.ReplaceIDMap["parent"] = +// new-uuid and substitutes ${parent.id} → "new-uuid" before the +// driver's Create receives the spec. +// +// The key assertion: dependent's Create call receives Config["vpc_ref"] +// = "new-uuid" (the post-Replace ProviderID), NOT the unresolved literal +// "${parent.id}". A regression in either T3.4's ReplaceIDMap population +// or T5.2's loop-level substitution causes this test to fail with the +// unresolved literal showing up in seenConfigs. +func TestApplyPlan_ReplaceCascade_DependentCreateGetsNewParentID(t *testing.T) { + fp := newJITRecordingProvider() + // Parent's Create (the post-Delete one) returns the freshly-minted + // ProviderID that JIT substitution should propagate downstream. + fp.driver.createReturns = map[string]*interfaces.ResourceOutput{ + "parent": {Name: "parent", Type: "infra.vpc", ProviderID: "vpc-new-uuid-after-replace"}, + } + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + { + Action: "replace", + Resource: interfaces.ResourceSpec{ + Name: "parent", Type: "infra.vpc", + Config: map[string]any{"cidr": "10.0.0.0/16"}, + }, + Current: &interfaces.ResourceState{Name: "parent", ProviderID: "vpc-old-uuid"}, + }, + { + Action: "create", + Resource: specWithConfig("dependent", "infra.app", map[string]any{ + "vpc_ref": "${parent.id}", + }), + }, + }} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 0 { + t.Fatalf("unexpected per-action errors: %+v", result.Errors) + } + // ReplaceIDMap must record the new ProviderID for parent. + if got := result.ReplaceIDMap["parent"]; got != "vpc-new-uuid-after-replace" { + t.Errorf("ReplaceIDMap[parent]: got %q want %q (full map: %+v)", + got, "vpc-new-uuid-after-replace", result.ReplaceIDMap) + } + // Dependent's Config must have been substituted before the driver + // saw it. A failure here indicates JIT did not run for the dependent + // action — either ReplaceIDMap wasn't populated yet or the loop + // failed to call ResolveSpec. + depConfig := fp.driver.seenConfigs["dependent"] + if depConfig == nil { + t.Fatalf("driver did not receive dependent's Config; seenConfigs=%+v", fp.driver.seenConfigs) + } + if got, want := depConfig["vpc_ref"], "vpc-new-uuid-after-replace"; got != want { + t.Errorf("dependent.Config[vpc_ref]: got %q want %q", got, want) + } +} + +// TestApplyPlan_ReplaceCascade_DependentReplaceGetsNewParentID extends +// the cascade scenario to a Replace-on-Replace shape: the dependent is +// also being Replaced (e.g., it carries a forced field change). The +// post-Delete Create for the dependent must STILL see the freshly- +// resolved ${parent.id} — Delete uses the OLD ProviderID via +// action.Current (unchanged by JIT), Create uses the new resolved spec. +// +// This isolates the "doReplace's Create receives the JIT-resolved spec" +// contract: an early version that built ref-from-action AFTER Delete +// could conceivably stale-reference the un-resolved spec for Create. +func TestApplyPlan_ReplaceCascade_DependentReplaceGetsNewParentID(t *testing.T) { + fp := newJITRecordingProvider() + fp.driver.createReturns = map[string]*interfaces.ResourceOutput{ + "parent": {Name: "parent", Type: "infra.vpc", ProviderID: "vpc-new-uuid"}, + "dependent": {Name: "dependent", Type: "infra.droplet", ProviderID: "droplet-new-uuid"}, + } + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + { + Action: "replace", + Resource: interfaces.ResourceSpec{ + Name: "parent", Type: "infra.vpc", + Config: map[string]any{"cidr": "10.0.0.0/16"}, + }, + Current: &interfaces.ResourceState{Name: "parent", ProviderID: "vpc-old-uuid"}, + }, + { + Action: "replace", + Resource: specWithConfig("dependent", "infra.droplet", map[string]any{ + "vpc_ref": "${parent.id}", + }), + Current: &interfaces.ResourceState{Name: "dependent", ProviderID: "droplet-old-uuid"}, + }, + }} + result, err := ApplyPlan(context.Background(), fp, plan) + if err != nil { + t.Fatal(err) + } + if len(result.Errors) != 0 { + t.Fatalf("unexpected per-action errors: %+v", result.Errors) + } + depConfig := fp.driver.seenConfigs["dependent"] + if depConfig == nil { + t.Fatalf("driver did not receive dependent's Config; seenConfigs=%+v", fp.driver.seenConfigs) + } + if got, want := depConfig["vpc_ref"], "vpc-new-uuid"; got != want { + t.Errorf("dependent.Config[vpc_ref] (cascade Replace): got %q want %q", got, want) + } + // Both Replaces should have populated ReplaceIDMap. + if got := result.ReplaceIDMap["parent"]; got != "vpc-new-uuid" { + t.Errorf("ReplaceIDMap[parent]: got %q", got) + } + if got := result.ReplaceIDMap["dependent"]; got != "droplet-new-uuid" { + t.Errorf("ReplaceIDMap[dependent]: got %q", got) + } +}