From 324d85684012b66e5029506db4aa2cd0873bb5b3 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:05:05 -0400 Subject: [PATCH 01/29] feat(iac): add IaCPlan.SchemaVersion + InputSnapshot + PlanAction.ResolvedConfigHash + DriftEntry type --- interfaces/iac_state.go | 22 ++++++++++++++++ interfaces/iac_state_test.go | 51 ++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) create mode 100644 interfaces/iac_state_test.go diff --git a/interfaces/iac_state.go b/interfaces/iac_state.go index d3d87d93..1b7a2739 100644 --- a/interfaces/iac_state.go +++ b/interfaces/iac_state.go @@ -51,6 +51,14 @@ type IaCPlan struct { // (sorted ResourceSpecs) at the time the plan was generated. wfctl infra apply // --plan compares this against the current config to detect stale plans. DesiredHash string `json:"plan_hash,omitempty"` + + // SchemaVersion is bumped when on-disk plan format changes (W-5 sets to 2 when JIT is required). + SchemaVersion int `json:"schema_version,omitempty"` + + // InputSnapshot records every env var name read during ${VAR} substitution + // at plan time, mapped to a 16-hex-char (64-bit) sha256 prefix of the value. + // Apply re-computes inputs and prints diagnostic on mismatch. + InputSnapshot map[string]string `json:"input_snapshot,omitempty"` } // PlanAction is a single planned change within an IaCPlan. @@ -59,6 +67,20 @@ type PlanAction struct { Resource ResourceSpec `json:"resource"` Current *ResourceState `json:"current,omitempty"` Changes []FieldChange `json:"changes,omitempty"` + + // ResolvedConfigHash is the SHA-256 of POST-substitution Resource.Config. + // Apply re-computes per-action and surfaces per-resource diagnostic on mismatch. + ResolvedConfigHash string `json:"resolved_config_hash,omitempty"` +} + +// DriftEntry names a single env-var whose fingerprint changed between plan-time +// and apply-time. Used by both the persisted-`--plan` path (cmd/wfctl/infra.go, +// wired in T1.5) and the in-process apply path (wfctlhelpers.ApplyPlan, wired +// in T3.1.5 — both via inputsnapshot.FormatStaleError). +type DriftEntry struct { + Name string `json:"name"` + PlanFingerprint string `json:"plan_fingerprint"` + ApplyFingerprint string `json:"apply_fingerprint"` } // ApplyResult summarises the outcome of applying a plan. diff --git a/interfaces/iac_state_test.go b/interfaces/iac_state_test.go new file mode 100644 index 00000000..029196af --- /dev/null +++ b/interfaces/iac_state_test.go @@ -0,0 +1,51 @@ +package interfaces + +import ( + "encoding/json" + "testing" +) + +func TestIaCPlan_SchemaVersionField(t *testing.T) { + p := IaCPlan{SchemaVersion: 2} + data, err := json.Marshal(p) + if err != nil { + t.Fatal(err) + } + var got IaCPlan + if err := json.Unmarshal(data, &got); err != nil { + t.Fatal(err) + } + if got.SchemaVersion != 2 { + t.Errorf("SchemaVersion roundtrip: got %d want 2", got.SchemaVersion) + } +} + +func TestIaCPlan_InputSnapshotField(t *testing.T) { + p := IaCPlan{InputSnapshot: map[string]string{"FOO": "deadbeefcafebabe"}} + data, err := json.Marshal(p) + if err != nil { + t.Fatal(err) + } + var got IaCPlan + if err := json.Unmarshal(data, &got); err != nil { + t.Fatal(err) + } + if got.InputSnapshot["FOO"] != "deadbeefcafebabe" { + t.Errorf("InputSnapshot roundtrip failed: %v", got.InputSnapshot) + } +} + +func TestPlanAction_ResolvedConfigHashField(t *testing.T) { + a := PlanAction{Action: "create", ResolvedConfigHash: "sha256:abc"} + data, err := json.Marshal(a) + if err != nil { + t.Fatal(err) + } + var got PlanAction + if err := json.Unmarshal(data, &got); err != nil { + t.Fatal(err) + } + if got.ResolvedConfigHash != "sha256:abc" { + t.Errorf("ResolvedConfigHash: got %q", got.ResolvedConfigHash) + } +} From 7c8c3ac1d4c8ee454454604db9c6efce3163ad52 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:06:20 -0400 Subject: [PATCH 02/29] feat(iac): add inputsnapshot.Compute + Snapshot + NewTolerantEnvProvider with preservation sentinel --- iac/inputsnapshot/snapshot.go | 80 ++++++++++++++++++++++++++++++ iac/inputsnapshot/snapshot_test.go | 62 +++++++++++++++++++++++ 2 files changed, 142 insertions(+) create mode 100644 iac/inputsnapshot/snapshot.go create mode 100644 iac/inputsnapshot/snapshot_test.go diff --git a/iac/inputsnapshot/snapshot.go b/iac/inputsnapshot/snapshot.go new file mode 100644 index 00000000..7a49a54f --- /dev/null +++ b/iac/inputsnapshot/snapshot.go @@ -0,0 +1,80 @@ +// Package inputsnapshot computes plan-time env-var fingerprints for the +// plan-stale diagnostic. Fingerprints are 16 hex chars (64 bits of preimage +// resistance); plan.json is treated as semi-sensitive and gitignored. +package inputsnapshot + +import ( + "crypto/sha256" + "encoding/hex" + "os" +) + +// Compute returns a map of env-var name → 16-hex-char sha256 prefix of the value. +// Variables that aren't set (lookup returns ok=false) are omitted from the snapshot. +func Compute(varNames []string, lookup func(string) (string, bool)) map[string]string { + out := make(map[string]string) + for _, name := range varNames { + val, ok := lookup(name) + if !ok { + continue + } + if val == preservedFingerprint { + // Sentinel from NewTolerantEnvProvider — pass through unhashed + // so ComputeDrift recognizes the preservation signal. (rev6 — + // unexported per cycle-5; in-package access only.) + out[name] = preservedFingerprint + continue + } + sum := sha256.Sum256([]byte(val)) + out[name] = hex.EncodeToString(sum[:])[:16] + } + return out +} + +// Snapshot is an alias for Compute that reads slightly more naturally at +// the in-process apply postcondition call site (T3.1.5). +func Snapshot(names []string, provider func(string) (string, bool)) map[string]string { + return Compute(names, provider) +} + +// OSEnvProvider is the canonical env-provider closure that reads from +// process env via os.LookupEnv. Used by start-of-apply InputSnapshot capture. +func OSEnvProvider(name string) (string, bool) { return os.LookupEnv(name) } + +// preservedFingerprint is a sentinel value indicating an env-var was set at +// plan time but is unset at apply time (sub-action cleanup is the canonical +// case). ComputeDrift (T1.5) skips drift detection for keys whose applySnap +// value is this sentinel. UNEXPORTED (rev6 — addresses cycle-5 Important on +// external-bypass channel): NewTolerantEnvProvider is the only sanctioned +// way to inject the sentinel; external callers cannot defeat drift detection. +// +// Cross-function contract: +// - Compute (this file, in-package) passes the sentinel through unhashed. +// - NewTolerantEnvProvider (this file) returns the sentinel for plan-time-set +// but apply-time-unset vars (in-package access to the constant). +// - ComputeDrift (compute_drift.go, T1.5, same package) honors the sentinel +// by skipping drift detection for that key. +const preservedFingerprint = "__plan_time_preserved__" + +// NewTolerantEnvProvider returns an EnvProvider closure used by the +// in-process apply postcondition (T3.1.5). When a var was set at plan time +// (present in planSnapshot) but is now unset (sub-action cleanup), the +// closure returns the in-package preservedFingerprint sentinel so +// ComputeDrift suppresses the (false-positive) drift entry. For vars +// genuinely unset at both times, returns ("", false) → Compute drops the +// key from the resulting map. +// +// This is the ONLY sanctioned way to inject the preservation sentinel. +// Direct callers of Compute with a custom env-provider cannot construct +// the sentinel value because it is unexported. +func NewTolerantEnvProvider(planSnapshot map[string]string) func(name string) (string, bool) { + return func(name string) (string, bool) { + if val, ok := os.LookupEnv(name); ok { + return val, true + } + if _, wasInPlan := planSnapshot[name]; wasInPlan { + return preservedFingerprint, true + } + return "", false + } +} diff --git a/iac/inputsnapshot/snapshot_test.go b/iac/inputsnapshot/snapshot_test.go new file mode 100644 index 00000000..b946086e --- /dev/null +++ b/iac/inputsnapshot/snapshot_test.go @@ -0,0 +1,62 @@ +package inputsnapshot + +import ( + "os" + "testing" +) + +func TestCompute_FingerprintIs16HexChars(t *testing.T) { + snap := Compute([]string{"FOO"}, func(k string) (string, bool) { + return "the-value", true + }) + if got := snap["FOO"]; len(got) != 16 { + t.Errorf("fingerprint len = %d, want 16; got %q", len(got), got) + } +} + +func TestCompute_DeterministicAcrossRuns(t *testing.T) { + env := func(k string) (string, bool) { return "v", true } + a := Compute([]string{"FOO"}, env) + b := Compute([]string{"FOO"}, env) + if a["FOO"] != b["FOO"] { + t.Errorf("non-deterministic: %q vs %q", a["FOO"], b["FOO"]) + } +} + +func TestCompute_DifferentValuesDifferentFingerprints(t *testing.T) { + env1 := func(k string) (string, bool) { return "value-one", true } + env2 := func(k string) (string, bool) { return "value-two", true } + a := Compute([]string{"FOO"}, env1) + b := Compute([]string{"FOO"}, env2) + if a["FOO"] == b["FOO"] { + t.Errorf("fingerprints should differ: %q == %q", a["FOO"], b["FOO"]) + } +} + +func TestCompute_MissingEnvVarOmitted(t *testing.T) { + snap := Compute([]string{"NOT_SET"}, func(k string) (string, bool) { + return "", false + }) + if _, ok := snap["NOT_SET"]; ok { + t.Errorf("missing env should be omitted, got %q", snap["NOT_SET"]) + } +} + +func TestNewTolerantEnvProvider_UnsetButPlanned_ReturnsSentinel(t *testing.T) { + os.Unsetenv("STAGING_PG_PASSWORD") + plan := map[string]string{"STAGING_PG_PASSWORD": "deadbeef00000000"} + provider := NewTolerantEnvProvider(plan) + val, ok := provider("STAGING_PG_PASSWORD") + if !ok || val != preservedFingerprint { + t.Errorf("expected (preservedFingerprint, true) for plan-time-set unset-now var; got (%q, %v)", val, ok) + } +} + +func TestCompute_PreservesSentinel(t *testing.T) { + snap := Compute([]string{"FOO"}, func(name string) (string, bool) { + return preservedFingerprint, true + }) + if snap["FOO"] != preservedFingerprint { + t.Errorf("Compute should pass sentinel through unhashed; got %q", snap["FOO"]) + } +} From 4774c3b7de0568c723b27a45204091f7cca54e7c Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:11:17 -0400 Subject: [PATCH 03/29] feat(iac): wfctl infra plan writes InputSnapshot to plan.json --- cmd/wfctl/infra.go | 11 +++ cmd/wfctl/infra_inputsnapshot.go | 90 ++++++++++++++++++++++ cmd/wfctl/infra_plan_inputsnapshot_test.go | 49 ++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 cmd/wfctl/infra_inputsnapshot.go create mode 100644 cmd/wfctl/infra_plan_inputsnapshot_test.go diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index f4fc129f..9e6b033e 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -201,6 +201,17 @@ func runInfraPlan(args []string) error { return fmt.Errorf("compute plan: %w", err) } + // Capture env-var fingerprints so apply (persisted-plan path: T1.5; in-process + // path: T3.1.5) can surface a per-key diagnostic when a referenced env var + // changed between plan and apply. Bumped to schema version 1 so older + // readers that predate this field can be detected and rejected. + snap, err := computeInfraInputSnapshot(cfgFile, envName) + if err != nil { + return fmt.Errorf("compute input snapshot: %w", err) + } + plan.InputSnapshot = snap + plan.SchemaVersion = 1 + switch *format { case "markdown": fmt.Print(formatPlanMarkdown(plan, showSensitive)) diff --git a/cmd/wfctl/infra_inputsnapshot.go b/cmd/wfctl/infra_inputsnapshot.go new file mode 100644 index 00000000..b94ba7d2 --- /dev/null +++ b/cmd/wfctl/infra_inputsnapshot.go @@ -0,0 +1,90 @@ +package main + +import ( + "os" + "sort" + + "github.com/GoCodeAlone/workflow/config" + "github.com/GoCodeAlone/workflow/iac/inputsnapshot" +) + +// collectInfraEnvVarRefs returns a sorted, de-duplicated list of env-var +// names referenced via ${VAR} or $VAR in the raw (pre-substitution) Configs +// of all infra.* and platform.* modules in cfgFile. +// +// When envName is non-empty, per-environment overrides are applied via +// ModuleConfig.ResolveForEnv before scanning, so env-specific substitution +// references are captured. +// +// Preserved-key submaps (env_vars / env_vars_secret / secret_env_vars) are +// scanned just like any other map: their ${VAR} literals are kept verbatim +// in the persisted plan but the plan-time fingerprint of the underlying env +// var is still recorded so apply-time drift is detectable. +func collectInfraEnvVarRefs(cfgFile, envName string) ([]string, error) { + cfg, err := config.LoadFromFile(cfgFile) + if err != nil { + return nil, err + } + seen := map[string]struct{}{} + record := func(name string) string { + if name != "" { + seen[name] = struct{}{} + } + return "" + } + for i := range cfg.Modules { + m := &cfg.Modules[i] + if !isInfraType(m.Type) { + continue + } + if envName == "" { + walkValueForEnvRefs(m.Config, record) + continue + } + resolved, ok := m.ResolveForEnv(envName) + if !ok { + continue + } + walkValueForEnvRefs(resolved.Config, record) + } + names := make([]string, 0, len(seen)) + for k := range seen { + names = append(names, k) + } + sort.Strings(names) + return names, nil +} + +// walkValueForEnvRefs recursively scans v for ${VAR} / $VAR references in +// any string values, calling record(name) for each. Maps and slices are +// walked element-wise; non-string scalars are ignored. +func walkValueForEnvRefs(v any, record func(string) string) { + switch val := v.(type) { + case string: + // os.Expand walks ${VAR} and $VAR references the same way os.ExpandEnv + // does at substitution time, so the name set captured here matches the + // set that ExpandEnvInMap[PreservingKeys] would actually substitute. + os.Expand(val, record) + case map[string]any: + for _, vv := range val { + walkValueForEnvRefs(vv, record) + } + case []any: + for _, vv := range val { + walkValueForEnvRefs(vv, record) + } + } +} + +// computeInfraInputSnapshot returns the env-var fingerprint map for cfgFile's +// infra/platform modules. Returns (nil, nil) when no ${VAR} references exist. +func computeInfraInputSnapshot(cfgFile, envName string) (map[string]string, error) { + names, err := collectInfraEnvVarRefs(cfgFile, envName) + if err != nil { + return nil, err + } + if len(names) == 0 { + return nil, nil + } + return inputsnapshot.Compute(names, inputsnapshot.OSEnvProvider), nil +} diff --git a/cmd/wfctl/infra_plan_inputsnapshot_test.go b/cmd/wfctl/infra_plan_inputsnapshot_test.go new file mode 100644 index 00000000..a6f7d9d3 --- /dev/null +++ b/cmd/wfctl/infra_plan_inputsnapshot_test.go @@ -0,0 +1,49 @@ +package main + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +func TestPlanWritesInputSnapshot(t *testing.T) { + t.Setenv("STAGING_DB_PASSWORD", "secret-value") + 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) + } + + data, err := os.ReadFile(planFile) + 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) + } + if plan.InputSnapshot["STAGING_DB_PASSWORD"] == "" { + t.Errorf("plan.InputSnapshot missing STAGING_DB_PASSWORD; got %v", plan.InputSnapshot) + } + if got := plan.InputSnapshot["STAGING_DB_PASSWORD"]; len(got) != 16 { + t.Errorf("fingerprint should be 16 hex chars, got %d (%q)", len(got), got) + } + if plan.SchemaVersion != 1 { + t.Errorf("SchemaVersion = %d, want 1", plan.SchemaVersion) + } +} From 295d354c301501b3da718029389a1a52418a4424 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:12:58 -0400 Subject: [PATCH 04/29] feat(iac): ComputePlan sets PlanAction.ResolvedConfigHash --- platform/differ.go | 12 +++++++----- platform/differ_test.go | 43 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/platform/differ.go b/platform/differ.go index dfc1d81e..f79a7539 100644 --- a/platform/differ.go +++ b/platform/differ.go @@ -36,15 +36,17 @@ func ComputePlan(desired []interfaces.ResourceSpec, current []interfaces.Resourc hash := configHash(spec.Config) if rs, exists := currentMap[spec.Name]; !exists { creates = append(creates, interfaces.PlanAction{ - Action: "create", - Resource: spec, + Action: "create", + Resource: spec, + ResolvedConfigHash: hash, }) } else if rs.ConfigHash != hash { rsCopy := rs updates = append(updates, interfaces.PlanAction{ - Action: "update", - Resource: spec, - Current: &rsCopy, + Action: "update", + Resource: spec, + Current: &rsCopy, + ResolvedConfigHash: hash, }) } // No change: skip. diff --git a/platform/differ_test.go b/platform/differ_test.go index 0d733f23..59024fc4 100644 --- a/platform/differ_test.go +++ b/platform/differ_test.go @@ -206,3 +206,46 @@ func TestDiffer_CycleDetection(t *testing.T) { t.Errorf("error = %q, expected 'cycle' in message", err.Error()) } } + +func TestComputePlan_PerActionResolvedConfigHash(t *testing.T) { + desired := []interfaces.ResourceSpec{ + {Name: "vpc", Type: "infra.vpc", Config: map[string]any{"region": "nyc1"}}, + } + plan, err := platform.ComputePlan(desired, nil) + if err != nil { + t.Fatal(err) + } + if len(plan.Actions) != 1 { + t.Fatalf("expected 1 action, got %d", len(plan.Actions)) + } + if plan.Actions[0].ResolvedConfigHash == "" { + t.Errorf("expected ResolvedConfigHash on create action, got %+v", plan.Actions[0]) + } + want := platform.ConfigHash(desired[0].Config) + if got := plan.Actions[0].ResolvedConfigHash; got != want { + t.Errorf("ResolvedConfigHash = %q, want %q", got, want) + } +} + +func TestComputePlan_ResolvedConfigHashOnUpdate(t *testing.T) { + desired := []interfaces.ResourceSpec{ + {Name: "db", Type: "infra.database", Config: map[string]any{"engine": "postgres", "size": "db-s"}}, + } + current := []interfaces.ResourceState{ + {Name: "db", Type: "infra.database", ConfigHash: "stale-hash"}, + } + plan, err := platform.ComputePlan(desired, current) + if err != nil { + t.Fatal(err) + } + if len(plan.Actions) != 1 || plan.Actions[0].Action != "update" { + t.Fatalf("expected 1 update action, got %+v", plan.Actions) + } + if plan.Actions[0].ResolvedConfigHash == "" { + t.Errorf("expected ResolvedConfigHash on update action, got %+v", plan.Actions[0]) + } + want := platform.ConfigHash(desired[0].Config) + if got := plan.Actions[0].ResolvedConfigHash; got != want { + t.Errorf("ResolvedConfigHash = %q, want %q", got, want) + } +} From b442dae9cac9cd4c68fa1c5409735fc6f66d69b4 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:15:35 -0400 Subject: [PATCH 05/29] feat(iac): wfctl infra plan warns when plan.json not in .gitignore --- cmd/wfctl/infra.go | 5 ++ cmd/wfctl/infra_plan_gitignore.go | 96 +++++++++++++++++++++++++ cmd/wfctl/infra_plan_gitignore_test.go | 97 ++++++++++++++++++++++++++ 3 files changed, 198 insertions(+) create mode 100644 cmd/wfctl/infra_plan_gitignore.go create mode 100644 cmd/wfctl/infra_plan_gitignore_test.go diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index 9e6b033e..c5e7161d 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -228,6 +228,11 @@ func runInfraPlan(args []string) error { return fmt.Errorf("write plan: %w", err) } fmt.Printf("\nPlan saved to %s\n", *output) + // Plan files carry semi-sensitive content (env-var fingerprints, + // resolved configs); warn the operator when none of the reachable + // .gitignore files cover the output path. Silent when the directory + // is not under a tracked repo (no .gitignore present). + warnIfPlanNotGitignored(os.Stderr, *output) } return nil diff --git a/cmd/wfctl/infra_plan_gitignore.go b/cmd/wfctl/infra_plan_gitignore.go new file mode 100644 index 00000000..48682b8e --- /dev/null +++ b/cmd/wfctl/infra_plan_gitignore.go @@ -0,0 +1,96 @@ +package main + +import ( + "bufio" + "fmt" + "io" + "os" + "path/filepath" + "strings" +) + +// warnIfPlanNotGitignored writes a stderr warning to w when planPath is not +// covered by any .gitignore reachable from the directory containing planPath +// up to the filesystem root. +// +// Why: plan.json carries semi-sensitive content (env-var fingerprints, +// resolved configs, sometimes provider IDs); committing it to source control +// is almost always accidental. We don't promise full gitignore semantics — +// the heuristic catches the common cases (literal basename, simple +// extension/path globs) and stays silent when no .gitignore exists at all +// (likely not a tracked repo). +// +// No warning is emitted when: +// - No .gitignore is found between planDir and the filesystem root. +// - At least one reachable .gitignore contains a line matching the plan's +// basename, the literal plan path, "*.json", "*", or a "**/" pattern +// ending with the basename. +func warnIfPlanNotGitignored(w io.Writer, planPath string) { + abs, err := filepath.Abs(planPath) + if err != nil { + return + } + base := filepath.Base(abs) + dir := filepath.Dir(abs) + + foundAny := false + covered := false + for { + gitignore := filepath.Join(dir, ".gitignore") + if data, err := os.ReadFile(gitignore); err == nil { + foundAny = true + if gitignoreCovers(data, base, abs, dir) { + covered = true + break + } + } + parent := filepath.Dir(dir) + if parent == dir { + break // reached filesystem root + } + dir = parent + } + if foundAny && !covered { + fmt.Fprintf(w, "warning: %s is not covered by .gitignore — plan.json may contain semi-sensitive data; add %q to .gitignore before committing.\n", planPath, base) + } +} + +// gitignoreCovers performs a pragmatic match against a .gitignore content for +// patterns that would exclude planAbs (basename = base, found at gitignoreDir). +// This is intentionally a heuristic, not full gitignore semantics: it covers +// the common cases (literal basename, "*.ext", "**/", and a path +// relative to the gitignore directory) and ignores negation rules. +func gitignoreCovers(data []byte, base, planAbs, gitignoreDir string) bool { + ext := filepath.Ext(base) + scanner := bufio.NewScanner(strings.NewReader(string(data))) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + if strings.HasPrefix(line, "!") { + continue // negation rules — skip; conservative (warn even if a later rule re-includes) + } + // Strip a leading "/" — gitignore-relative anchor; we treat both + // "/foo" and "foo" as candidates against the basename or relative path. + anchored := strings.TrimPrefix(line, "/") + + if anchored == base { + return true + } + if ext != "" && (anchored == "*"+ext || anchored == "**/*"+ext) { + return true + } + // "**/" matches at any depth. + if anchored == "**/"+base { + return true + } + // Relative path from .gitignore dir, e.g. "cmd/wfctl/plan.json". + if rel, err := filepath.Rel(gitignoreDir, planAbs); err == nil { + if anchored == rel || anchored == filepath.ToSlash(rel) { + return true + } + } + } + return false +} diff --git a/cmd/wfctl/infra_plan_gitignore_test.go b/cmd/wfctl/infra_plan_gitignore_test.go new file mode 100644 index 00000000..1e14bcd1 --- /dev/null +++ b/cmd/wfctl/infra_plan_gitignore_test.go @@ -0,0 +1,97 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestPlan_WarnsOnMissingGitignoreEntry verifies that runInfraPlan emits a +// stderr warning when the plan output path is not covered by .gitignore. +// plan.json carries semi-sensitive content (env-var fingerprints, resolved +// configs) and must not land in version control by default. +func TestPlan_WarnsOnMissingGitignoreEntry(t *testing.T) { + repo := t.TempDir() + if err := os.WriteFile(filepath.Join(repo, ".gitignore"), []byte("# empty\n"), 0o600); err != nil { + t.Fatalf("write .gitignore: %v", err) + } + cfgPath := filepath.Join(repo, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: vpc + type: infra.vpc + config: + region: nyc1 +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + planFile := filepath.Join(repo, "plan.json") + + stderr, fnErr := captureStderr(t, func() error { + return runInfraPlan([]string{"--config", cfgPath, "--output", planFile}) + }) + if fnErr != nil { + t.Fatalf("runInfraPlan: %v", fnErr) + } + if !strings.Contains(stderr, "plan.json") || !strings.Contains(stderr, "gitignore") { + t.Errorf("expected gitignore warning mentioning plan.json, got: %q", stderr) + } +} + +// TestPlan_NoWarningWhenGitignored verifies that runInfraPlan stays silent +// (no stderr warning) when the output file is already covered by .gitignore. +func TestPlan_NoWarningWhenGitignored(t *testing.T) { + repo := t.TempDir() + if err := os.WriteFile(filepath.Join(repo, ".gitignore"), []byte("plan.json\n"), 0o600); err != nil { + t.Fatalf("write .gitignore: %v", err) + } + cfgPath := filepath.Join(repo, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: vpc + type: infra.vpc + config: + region: nyc1 +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + planFile := filepath.Join(repo, "plan.json") + + stderr, fnErr := captureStderr(t, func() error { + return runInfraPlan([]string{"--config", cfgPath, "--output", planFile}) + }) + if fnErr != nil { + t.Fatalf("runInfraPlan: %v", fnErr) + } + if strings.Contains(stderr, "gitignore") { + t.Errorf("did not expect gitignore warning when plan.json is gitignored, got: %q", stderr) + } +} + +// TestPlan_NoGitignoreFile_NoWarning verifies the warning is silent when no +// .gitignore exists in the repo (no git context — likely not a tracked repo). +func TestPlan_NoGitignoreFile_NoWarning(t *testing.T) { + repo := t.TempDir() + cfgPath := filepath.Join(repo, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: vpc + type: infra.vpc + config: + region: nyc1 +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + planFile := filepath.Join(repo, "plan.json") + + stderr, fnErr := captureStderr(t, func() error { + return runInfraPlan([]string{"--config", cfgPath, "--output", planFile}) + }) + if fnErr != nil { + t.Fatalf("runInfraPlan: %v", fnErr) + } + if strings.Contains(stderr, "gitignore") { + t.Errorf("did not expect gitignore warning without .gitignore file, got: %q", stderr) + } +} From 43c8ced040276020fdfafc376bbd2e8d3af4be3c Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:19:07 -0400 Subject: [PATCH 06/29] feat(iac): typed ErrEnvVarChanged sentinel + plan-stale diagnostic + ComputeDrift sentinel-honoring --- cmd/wfctl/infra.go | 16 +++++ cmd/wfctl/infra_apply_plan_test.go | 95 +++++++++++++++++++++++++ iac/inputsnapshot/compute_drift.go | 46 ++++++++++++ iac/inputsnapshot/compute_drift_test.go | 47 ++++++++++++ iac/inputsnapshot/diagnostic.go | 35 +++++++++ iac/inputsnapshot/errors.go | 11 +++ 6 files changed, 250 insertions(+) create mode 100644 iac/inputsnapshot/compute_drift.go create mode 100644 iac/inputsnapshot/compute_drift_test.go create mode 100644 iac/inputsnapshot/diagnostic.go create mode 100644 iac/inputsnapshot/errors.go diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index c5e7161d..1ac1d39e 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -10,6 +10,7 @@ import ( "time" "github.com/GoCodeAlone/workflow/config" + "github.com/GoCodeAlone/workflow/iac/inputsnapshot" "github.com/GoCodeAlone/workflow/interfaces" "github.com/GoCodeAlone/workflow/platform" "github.com/GoCodeAlone/workflow/secrets" @@ -1082,6 +1083,21 @@ func runInfraApply(args []string) error { if plan.DesiredHash == "" { return fmt.Errorf("plan file has no hash — regenerate with: wfctl infra plan -o plan.json") } + // Check the input-fingerprint drift first so the operator gets a + // per-key diagnostic instead of the generic config-hash mismatch. + // (Env-var changes are a strict subset of config-hash differences; + // flagging them here yields the actionable message.) Names list is + // derived from plan.InputSnapshot keys — no separate InputNames field. + if len(plan.InputSnapshot) > 0 { + names := make([]string, 0, len(plan.InputSnapshot)) + for k := range plan.InputSnapshot { + names = append(names, k) + } + applySnap := inputsnapshot.Compute(names, inputsnapshot.OSEnvProvider) + if drift := inputsnapshot.ComputeDrift(plan.InputSnapshot, applySnap); len(drift) > 0 { + return fmt.Errorf("%w\n%s", inputsnapshot.ErrEnvVarChanged, inputsnapshot.FormatStaleError(drift)) + } + } currentHash := desiredStateHash(desired) if plan.DesiredHash != currentHash { return fmt.Errorf("plan stale: config hash mismatch (run wfctl infra plan again)") diff --git a/cmd/wfctl/infra_apply_plan_test.go b/cmd/wfctl/infra_apply_plan_test.go index 829e9cbd..a68954a8 100644 --- a/cmd/wfctl/infra_apply_plan_test.go +++ b/cmd/wfctl/infra_apply_plan_test.go @@ -2,7 +2,10 @@ package main import ( "context" + "crypto/sha256" + "encoding/hex" "encoding/json" + "errors" "io" "os" "path/filepath" @@ -10,9 +13,18 @@ import ( "testing" "time" + "github.com/GoCodeAlone/workflow/iac/inputsnapshot" "github.com/GoCodeAlone/workflow/interfaces" ) +// fingerprintForTest matches inputsnapshot.Compute's fingerprint format +// (16-hex-char sha256 prefix) so tests can construct expected plan-time +// snapshots without depending on the concrete env-provider closure. +func fingerprintForTest(value string) string { + sum := sha256.Sum256([]byte(value)) + return hex.EncodeToString(sum[:])[:16] +} + // TestInfraApplyConsumesPlan verifies that wfctl infra apply --plan : // 1. Reads actions from the plan file without calling ComputePlan. // 2. Calls provider.Apply with exactly the plan from the file (identified by plan ID). @@ -399,6 +411,89 @@ modules: } } +// TestApply_PlanStaleDiagnostic_NamesChangedKeys_Persisted verifies that the +// persisted-`--plan` apply path returns the typed inputsnapshot.ErrEnvVarChanged +// sentinel when an env-var fingerprint embedded in the plan differs from the +// env at apply time, and that the error message names the changed key. This +// is the W-1 cross-PR test for the persisted-plan path; the in-process apply +// path is wired in T3.1.5 (W-3a). +func TestApply_PlanStaleDiagnostic_NamesChangedKeys_Persisted(t *testing.T) { + // Plan was generated with old-value; embed its fingerprint in the plan. + t.Setenv("STAGING_PG_PASSWORD", "old-value") + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: test-provider + type: iac.provider + config: + provider: fake-cloud + token: "test-token" + + - name: my-db + type: infra.database + config: + provider: test-provider + engine: postgres + size: s + env_vars: + DATABASE_PASSWORD: "${STAGING_PG_PASSWORD}" +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + specs, err := parseInfraResourceSpecs(cfgPath) + if err != nil { + t.Fatalf("parseInfraResourceSpecs: %v", err) + } + plan := interfaces.IaCPlan{ + ID: "stale-input-plan", + DesiredHash: desiredStateHash(specs), + SchemaVersion: 1, + InputSnapshot: map[string]string{ + "STAGING_PG_PASSWORD": fingerprintForTest("old-value"), + }, + Actions: []interfaces.PlanAction{ + {Action: "create", Resource: specs[0]}, + }, + CreatedAt: time.Now().UTC(), + } + planData, err := json.Marshal(plan) + if err != nil { + t.Fatalf("marshal plan: %v", err) + } + planPath := filepath.Join(dir, "plan.json") + if err := os.WriteFile(planPath, planData, 0o600); err != nil { + t.Fatalf("write plan: %v", err) + } + + // Mock provider so apply doesn't try to reach a real cloud. + fake := &applyCapture{} + origResolve := resolveIaCProvider + resolveIaCProvider = func(_ context.Context, _ string, _ map[string]any) (interfaces.IaCProvider, io.Closer, error) { + return fake, nil, nil + } + defer func() { resolveIaCProvider = origResolve }() + + // Apply with a different value — should trigger the drift diagnostic. + t.Setenv("STAGING_PG_PASSWORD", "new-value") + err = runInfraApply([]string{"--auto-approve", "--config", cfgPath, "--plan", planPath}) + if err == nil { + t.Fatal("expected plan-stale error from changed env-var fingerprint, got nil") + } + if !errors.Is(err, inputsnapshot.ErrEnvVarChanged) { + t.Errorf("expected sentinel inputsnapshot.ErrEnvVarChanged; got %v", err) + } + if !strings.Contains(err.Error(), "STAGING_PG_PASSWORD") { + t.Errorf("error should name the changed key; got: %s", err.Error()) + } + if !strings.Contains(err.Error(), "plan stale") { + t.Errorf("error should preserve the 'plan stale' marker; got: %s", err.Error()) + } + if fake.applyCalled { + t.Error("provider.Apply should not be invoked when plan is stale on input snapshot") + } +} + // TestDesiredStateHash_EmptySpecsProducesStableHash verifies that an empty spec // slice hashes deterministically (not "") so delete-all plans are not blocked. func TestDesiredStateHash_EmptySpecsProducesStableHash(t *testing.T) { diff --git a/iac/inputsnapshot/compute_drift.go b/iac/inputsnapshot/compute_drift.go new file mode 100644 index 00000000..73a106ce --- /dev/null +++ b/iac/inputsnapshot/compute_drift.go @@ -0,0 +1,46 @@ +package inputsnapshot + +import "github.com/GoCodeAlone/workflow/interfaces" + +// unsetFingerprintPlaceholder is the in-package constant displayed in the +// ApplyFingerprint field when the var was set at plan time but is missing +// entirely from the applySnap. UNEXPORTED to keep the placeholder a private +// contract between ComputeDrift + FormatStaleError + tests. +const unsetFingerprintPlaceholder = "(unset)" + +// ComputeDrift compares plan-time vs apply-time fingerprint snapshots and +// produces a drift report. Iterates over planSnap keys (no phantom +// InputNames field needed; map keys ARE the names). Honors the in-package +// preservedFingerprint sentinel from snapshot.go — keys whose applySnap +// value equals the sentinel are skipped (sub-action cleanup case). +// +// Cross-function contract: +// - Compute (snapshot.go) passes the sentinel through unhashed. +// - NewTolerantEnvProvider (snapshot.go, sole sanctioned injector) returns +// the sentinel for plan-time-set apply-time-unset vars. +// - ComputeDrift (this function) honors the sentinel by skipping the entry. +func ComputeDrift(planSnap, applySnap map[string]string) []interfaces.DriftEntry { + var drift []interfaces.DriftEntry + for name, planFP := range planSnap { + applyFP, present := applySnap[name] + if !present { + drift = append(drift, interfaces.DriftEntry{ + Name: name, + PlanFingerprint: planFP, + ApplyFingerprint: unsetFingerprintPlaceholder, + }) + continue + } + if applyFP == preservedFingerprint { + continue // Sentinel — sub-action cleanup unset; not real drift. + } + if applyFP != planFP { + drift = append(drift, interfaces.DriftEntry{ + Name: name, + PlanFingerprint: planFP, + ApplyFingerprint: applyFP, + }) + } + } + return drift +} diff --git a/iac/inputsnapshot/compute_drift_test.go b/iac/inputsnapshot/compute_drift_test.go new file mode 100644 index 00000000..7394cea1 --- /dev/null +++ b/iac/inputsnapshot/compute_drift_test.go @@ -0,0 +1,47 @@ +package inputsnapshot + +import "testing" + +func TestComputeDrift_PreservedSentinelSkipsDrift(t *testing.T) { + planSnap := map[string]string{"FOO": "abcdef0000000000"} + applySnap := map[string]string{"FOO": preservedFingerprint} + drift := ComputeDrift(planSnap, applySnap) + if len(drift) != 0 { + t.Errorf("preserved-sentinel should suppress drift; got %+v", drift) + } +} + +func TestComputeDrift_DifferentFingerprint_ReportsDrift(t *testing.T) { + planSnap := map[string]string{"FOO": "abcdef0000000000"} + applySnap := map[string]string{"FOO": "deadbeef00000000"} + drift := ComputeDrift(planSnap, applySnap) + if len(drift) != 1 || drift[0].Name != "FOO" { + t.Errorf("differing fingerprints should produce one drift entry; got %+v", drift) + } +} + +func TestComputeDrift_KeyMissingInApplySnap_ReportsDrift(t *testing.T) { + planSnap := map[string]string{"FOO": "abcdef0000000000"} + applySnap := map[string]string{} // FOO missing entirely + drift := ComputeDrift(planSnap, applySnap) + // Assert behavior, not literal placeholder string. Drift exists, + // ApplyFingerprint differs from PlanFingerprint, and uses the in-package + // unsetFingerprintPlaceholder constant. + if len(drift) != 1 || drift[0].Name != "FOO" { + t.Fatalf("missing key should produce one drift entry for FOO; got %+v", drift) + } + if drift[0].ApplyFingerprint == drift[0].PlanFingerprint { + t.Errorf("ApplyFingerprint should differ from PlanFingerprint; got identical %q", drift[0].ApplyFingerprint) + } + if drift[0].ApplyFingerprint != unsetFingerprintPlaceholder { + t.Errorf("ApplyFingerprint should equal unsetFingerprintPlaceholder; got %q", drift[0].ApplyFingerprint) + } +} + +func TestComputeDrift_MatchingFingerprints_NoDrift(t *testing.T) { + planSnap := map[string]string{"FOO": "abcdef0000000000"} + applySnap := map[string]string{"FOO": "abcdef0000000000"} + if drift := ComputeDrift(planSnap, applySnap); len(drift) != 0 { + t.Errorf("matching fingerprints should produce no drift; got %+v", drift) + } +} diff --git a/iac/inputsnapshot/diagnostic.go b/iac/inputsnapshot/diagnostic.go new file mode 100644 index 00000000..138e18c0 --- /dev/null +++ b/iac/inputsnapshot/diagnostic.go @@ -0,0 +1,35 @@ +package inputsnapshot + +import ( + "fmt" + "sort" + "strings" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// FormatStaleError renders a drift report into the canonical human-readable +// message used at every plan-stale call site (cmd/wfctl/infra.go persisted +// `--plan` path; wfctlhelpers.ApplyPlan in-process path in T3.1.5). Output: +// +// plan stale: %d input(s) changed since plan +// KEY1: fingerprint planFP1 (plan) → applyFP1 (apply) +// KEY2: fingerprint planFP2 (plan) → applyFP2 (apply) +// hint: ensure all env vars referenced by infra.yaml are exported to both Plan and Apply steps +// +// Drift entries are sorted by Name for deterministic output. An empty drift +// report yields the singular line "plan stale: 0 input(s) changed since plan" +// — callers should avoid invoking the formatter when no drift exists. +func FormatStaleError(drift []interfaces.DriftEntry) string { + sorted := make([]interfaces.DriftEntry, len(drift)) + copy(sorted, drift) + sort.Slice(sorted, func(i, j int) bool { return sorted[i].Name < sorted[j].Name }) + + var b strings.Builder + fmt.Fprintf(&b, "plan stale: %d input(s) changed since plan\n", len(sorted)) + for _, d := range sorted { + fmt.Fprintf(&b, " %s: fingerprint %s (plan) → %s (apply)\n", d.Name, d.PlanFingerprint, d.ApplyFingerprint) + } + b.WriteString(" hint: ensure all env vars referenced by infra.yaml are exported to both Plan and Apply steps") + return b.String() +} diff --git a/iac/inputsnapshot/errors.go b/iac/inputsnapshot/errors.go new file mode 100644 index 00000000..ece08e19 --- /dev/null +++ b/iac/inputsnapshot/errors.go @@ -0,0 +1,11 @@ +package inputsnapshot + +import "errors" + +// ErrEnvVarChanged is the typed sentinel returned by the apply paths +// (cmd/wfctl/infra.go persisted-`--plan` path in W-1; wfctlhelpers.ApplyPlan +// in-process path in W-3a/T3.1.5) when an env var referenced at plan time +// has a different fingerprint at apply time. Callers can match with +// errors.Is(err, ErrEnvVarChanged) to detect the plan-stale case +// independently of the human-readable per-key drift message. +var ErrEnvVarChanged = errors.New("plan stale: env-var changed since plan") From cbc8319992dd589b2391b9b21b3be5d5271c0f1e Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:58:09 -0400 Subject: [PATCH 07/29] fix(iac): FormatStaleError omits hint when drift is empty (Copilot review) Empty drift report previously rendered as a 2-line message (header + hint), contradicting the doc comment that promised a singular header line. Gate the hint on len(drift) > 0 so the empty case stays minimal as documented. Co-Authored-By: Claude Opus 4.7 (1M context) --- iac/inputsnapshot/diagnostic.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/iac/inputsnapshot/diagnostic.go b/iac/inputsnapshot/diagnostic.go index 138e18c0..1f15284d 100644 --- a/iac/inputsnapshot/diagnostic.go +++ b/iac/inputsnapshot/diagnostic.go @@ -18,8 +18,9 @@ import ( // hint: ensure all env vars referenced by infra.yaml are exported to both Plan and Apply steps // // Drift entries are sorted by Name for deterministic output. An empty drift -// report yields the singular line "plan stale: 0 input(s) changed since plan" -// — callers should avoid invoking the formatter when no drift exists. +// report yields the singular header line "plan stale: 0 input(s) changed since +// plan" with no trailing hint — callers should avoid invoking the formatter +// when no drift exists, but if they do the output stays minimal. func FormatStaleError(drift []interfaces.DriftEntry) string { sorted := make([]interfaces.DriftEntry, len(drift)) copy(sorted, drift) @@ -30,6 +31,8 @@ func FormatStaleError(drift []interfaces.DriftEntry) string { for _, d := range sorted { fmt.Fprintf(&b, " %s: fingerprint %s (plan) → %s (apply)\n", d.Name, d.PlanFingerprint, d.ApplyFingerprint) } - b.WriteString(" hint: ensure all env vars referenced by infra.yaml are exported to both Plan and Apply steps") + if len(sorted) > 0 { + b.WriteString(" hint: ensure all env vars referenced by infra.yaml are exported to both Plan and Apply steps") + } return b.String() } From a116a1c81e25cdc8e51020ab7bd2c936b8ac8b85 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:58:15 -0400 Subject: [PATCH 08/29] fix(iac): shorten ErrEnvVarChanged message; FormatStaleError owns user-facing prefix (Copilot review) The sentinel was wrapped via fmt.Errorf("%w\n%s", ErrEnvVarChanged, FormatStaleError(...)), so its "plan stale:" prefix duplicated the formatter's own "plan stale: %d input(s)..." header. Reduce the sentinel to a short machine-only marker; FormatStaleError remains the sole owner of the human-facing prefix. Existing test assertions match strings.Contains(err.Error(), "plan stale") via the formatter portion of the wrapped error and continue to pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- iac/inputsnapshot/errors.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/iac/inputsnapshot/errors.go b/iac/inputsnapshot/errors.go index ece08e19..26395b87 100644 --- a/iac/inputsnapshot/errors.go +++ b/iac/inputsnapshot/errors.go @@ -5,7 +5,10 @@ import "errors" // ErrEnvVarChanged is the typed sentinel returned by the apply paths // (cmd/wfctl/infra.go persisted-`--plan` path in W-1; wfctlhelpers.ApplyPlan // in-process path in W-3a/T3.1.5) when an env var referenced at plan time -// has a different fingerprint at apply time. Callers can match with +// has a different fingerprint at apply time. Callers match with // errors.Is(err, ErrEnvVarChanged) to detect the plan-stale case -// independently of the human-readable per-key drift message. -var ErrEnvVarChanged = errors.New("plan stale: env-var changed since plan") +// programmatically; the user-facing "plan stale: ..." prefix is owned by +// FormatStaleError, so this sentinel deliberately uses a short +// machine-only marker to avoid duplicating that prefix when wrapped via +// fmt.Errorf("%w\n%s", ErrEnvVarChanged, FormatStaleError(...)). +var ErrEnvVarChanged = errors.New("env-var changed since plan") From a36bba692e384cad8c6fbb9ad89de89970ae24e2 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:58:21 -0400 Subject: [PATCH 09/29] =?UTF-8?q?fix(iac):=20soften=20preservedFingerprint?= =?UTF-8?q?=20doc=20=E2=80=94=20unexported=20is=20discipline=20boundary=20?= =?UTF-8?q?not=20security=20(Copilot=20review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous comment claimed external callers "cannot defeat drift detection" because the sentinel is unexported, but any caller can return the literal string "__plan_time_preserved__" from a custom env-provider closure. Update both the constant doc and NewTolerantEnvProvider doc to be honest: the unexported boundary is API hygiene, not a security guarantee. Sentinel value unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- iac/inputsnapshot/snapshot.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/iac/inputsnapshot/snapshot.go b/iac/inputsnapshot/snapshot.go index 7a49a54f..088ec49d 100644 --- a/iac/inputsnapshot/snapshot.go +++ b/iac/inputsnapshot/snapshot.go @@ -44,9 +44,12 @@ func OSEnvProvider(name string) (string, bool) { return os.LookupEnv(name) } // preservedFingerprint is a sentinel value indicating an env-var was set at // plan time but is unset at apply time (sub-action cleanup is the canonical // case). ComputeDrift (T1.5) skips drift detection for keys whose applySnap -// value is this sentinel. UNEXPORTED (rev6 — addresses cycle-5 Important on -// external-bypass channel): NewTolerantEnvProvider is the only sanctioned -// way to inject the sentinel; external callers cannot defeat drift detection. +// value is this sentinel. The constant is unexported, so external code cannot +// reference the value by name; NewTolerantEnvProvider is the sole sanctioned +// injector. A determined caller could return the literal string from a custom +// env-provider closure passed to Compute, but doing so is a deliberate +// discipline violation, not a tooling bypass — the unexported boundary is +// about API hygiene, not security. // // Cross-function contract: // - Compute (this file, in-package) passes the sentinel through unhashed. @@ -65,8 +68,10 @@ const preservedFingerprint = "__plan_time_preserved__" // key from the resulting map. // // This is the ONLY sanctioned way to inject the preservation sentinel. -// Direct callers of Compute with a custom env-provider cannot construct -// the sentinel value because it is unexported. +// The sentinel constant is unexported, so external code cannot reference it +// by name; a determined caller could still return the literal string from a +// custom env-provider, but doing so is a deliberate discipline violation +// rather than a tooling bypass. func NewTolerantEnvProvider(planSnapshot map[string]string) func(name string) (string, bool) { return func(name string) (string, bool) { if val, ok := os.LookupEnv(name); ok { From 03c81ce567259ea95130f3e75e8dc1b50bca7f72 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:58:28 -0400 Subject: [PATCH 10/29] fix(iac): gitignoreCovers uses bytes.NewReader + checks scanner.Err (Copilot review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bufio.NewScanner over strings.NewReader(string(data)) made an extra copy of the .gitignore contents; switch to bytes.NewReader(data) to scan the slice directly. Also check scanner.Err() after the loop — oversized lines (over bufio.MaxScanTokenSize) previously fell through silently as "not covered". Conservative behavior: scan errors return false so an operator-visible warning is emitted rather than silently letting plan.json land in source control. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wfctl/infra_plan_gitignore.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cmd/wfctl/infra_plan_gitignore.go b/cmd/wfctl/infra_plan_gitignore.go index 48682b8e..fc57678c 100644 --- a/cmd/wfctl/infra_plan_gitignore.go +++ b/cmd/wfctl/infra_plan_gitignore.go @@ -2,6 +2,7 @@ package main import ( "bufio" + "bytes" "fmt" "io" "os" @@ -62,7 +63,7 @@ func warnIfPlanNotGitignored(w io.Writer, planPath string) { // relative to the gitignore directory) and ignores negation rules. func gitignoreCovers(data []byte, base, planAbs, gitignoreDir string) bool { ext := filepath.Ext(base) - scanner := bufio.NewScanner(strings.NewReader(string(data))) + scanner := bufio.NewScanner(bytes.NewReader(data)) for scanner.Scan() { line := strings.TrimSpace(scanner.Text()) if line == "" || strings.HasPrefix(line, "#") { @@ -92,5 +93,12 @@ func gitignoreCovers(data []byte, base, planAbs, gitignoreDir string) bool { } } } + // Scanner errors (e.g. line longer than bufio.MaxScanTokenSize) cause + // silent fall-through if not checked. Conservative: treat scan failure + // as not-covered, which surfaces a warning the operator can investigate + // rather than silently letting plan.json land in source control. + if err := scanner.Err(); err != nil { + return false + } return false } From 0f764ba2f55d2d552831c047b5e76d95798e129c Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 19:58:33 -0400 Subject: [PATCH 11/29] docs(iac): clarify IaCPlan.InputSnapshot only records set vars (Copilot review) Comment said "every env var name read" but inputsnapshot.Compute and OSEnvProvider intentionally omit unset vars from the resulting map. Make the contract explicit: only set vars are fingerprinted; unset-at-plan + unset-at-apply yields no drift entry by design. Co-Authored-By: Claude Opus 4.7 (1M context) --- interfaces/iac_state.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/interfaces/iac_state.go b/interfaces/iac_state.go index 1b7a2739..3ca33491 100644 --- a/interfaces/iac_state.go +++ b/interfaces/iac_state.go @@ -56,8 +56,10 @@ type IaCPlan struct { SchemaVersion int `json:"schema_version,omitempty"` // InputSnapshot records every env var name read during ${VAR} substitution - // at plan time, mapped to a 16-hex-char (64-bit) sha256 prefix of the value. - // Apply re-computes inputs and prints diagnostic on mismatch. + // at plan time, fingerprinting only those that were SET (16-hex-char sha256 + // prefix of the value). Unset vars are omitted from the map; their absence + // at apply time is therefore not flagged as drift. Apply re-computes inputs + // and prints diagnostic on mismatch. InputSnapshot map[string]string `json:"input_snapshot,omitempty"` } From 5ece23779fced93a4943887246a59218fe041d00 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:08:33 -0400 Subject: [PATCH 12/29] docs(iac): document gitignoreCovers negation false-negative honestly (Copilot review) Heuristic skips !-prefixed negation rules and returns on first positive match, so a "*.json" then "!plan.json" pattern silently passes. Acceptable for a nudge-not-enforce warning; document the limitation in the negation branch comment so future maintainers know the boundary. Full last-matching-rule-wins semantics or git check-ignore shell-out are out of scope for W-1. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wfctl/infra_plan_gitignore.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/cmd/wfctl/infra_plan_gitignore.go b/cmd/wfctl/infra_plan_gitignore.go index fc57678c..28e316d7 100644 --- a/cmd/wfctl/infra_plan_gitignore.go +++ b/cmd/wfctl/infra_plan_gitignore.go @@ -70,7 +70,16 @@ func gitignoreCovers(data []byte, base, planAbs, gitignoreDir string) bool { continue } if strings.HasPrefix(line, "!") { - continue // negation rules — skip; conservative (warn even if a later rule re-includes) + // Negation rules are skipped entirely. Combined with the early-return + // on first positive match below, this means a pattern like "*.json" + // followed by "!plan.json" (re-include) is incorrectly treated as + // covered — producing a false-NEGATIVE warning (operator sees no + // warning when one was warranted). Acceptable for a heuristic whose + // purpose is to nudge, not enforce; full last-match-wins gitignore + // semantics are out of scope. If false negatives become a problem, + // either implement last-matching-rule-wins for the supported + // pattern set, or shell out to `git check-ignore`. + continue } // Strip a leading "/" — gitignore-relative anchor; we treat both // "/foo" and "foo" as candidates against the basename or relative path. From 351c09c98917acf675857ca09773524fbeae1256 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:08:41 -0400 Subject: [PATCH 13/29] fix(iac): preservedFingerprint embeds NUL byte to make value-collision impossible (Copilot review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously a real env var whose value happened to equal "__plan_time_preserved__" would be treated as a preservation sentinel and silently suppress drift detection. Embed a NUL byte: POSIX exec(3) and Windows CreateProcess both reject NUL inside env values, so no var the OS delivers to a Go process can collide with the constant. In-package call sites (Compute, NewTolerantEnvProvider, ComputeDrift) compare by string equality against the constant — value change is transparent to them. Co-Authored-By: Claude Opus 4.7 (1M context) --- iac/inputsnapshot/snapshot.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/iac/inputsnapshot/snapshot.go b/iac/inputsnapshot/snapshot.go index 088ec49d..9bd4de2e 100644 --- a/iac/inputsnapshot/snapshot.go +++ b/iac/inputsnapshot/snapshot.go @@ -51,13 +51,18 @@ func OSEnvProvider(name string) (string, bool) { return os.LookupEnv(name) } // discipline violation, not a tooling bypass — the unexported boundary is // about API hygiene, not security. // +// Collision-safety: the embedded NUL byte (\x00) makes value-collision with +// a real env var impossible — POSIX exec(3) and Windows CreateProcess both +// reject NUL inside env values, so no var the OS can deliver to a Go process +// could match this constant by accident. +// // Cross-function contract: // - Compute (this file, in-package) passes the sentinel through unhashed. // - NewTolerantEnvProvider (this file) returns the sentinel for plan-time-set // but apply-time-unset vars (in-package access to the constant). // - ComputeDrift (compute_drift.go, T1.5, same package) honors the sentinel // by skipping drift detection for that key. -const preservedFingerprint = "__plan_time_preserved__" +const preservedFingerprint = "__plan_time_preserved__\x00" // NewTolerantEnvProvider returns an EnvProvider closure used by the // in-process apply postcondition (T3.1.5). When a var was set at plan time From 5696cb2e153dda2447fc1d35e55710e32a2d7ff0 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:08:47 -0400 Subject: [PATCH 14/29] test(iac): NewTolerantEnvProvider test uses unique key, no os.Unsetenv (Copilot review) Previous test called os.Unsetenv("STAGING_PG_PASSWORD") without restoring the prior value, leaking state across the test process and creating order-dependence with any other test that reads STAGING_PG_PASSWORD. Switch to a test-unique env var name (WFCTL_TEST_INPUTSNAPSHOT_UNSET_KEY) the test never sets, so no cleanup is needed and there is no cross-test state leak. Co-Authored-By: Claude Opus 4.7 (1M context) --- iac/inputsnapshot/snapshot_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/iac/inputsnapshot/snapshot_test.go b/iac/inputsnapshot/snapshot_test.go index b946086e..edd9b456 100644 --- a/iac/inputsnapshot/snapshot_test.go +++ b/iac/inputsnapshot/snapshot_test.go @@ -1,7 +1,6 @@ package inputsnapshot import ( - "os" "testing" ) @@ -43,10 +42,13 @@ func TestCompute_MissingEnvVarOmitted(t *testing.T) { } func TestNewTolerantEnvProvider_UnsetButPlanned_ReturnsSentinel(t *testing.T) { - os.Unsetenv("STAGING_PG_PASSWORD") - plan := map[string]string{"STAGING_PG_PASSWORD": "deadbeef00000000"} + // Use a test-unique env-var name to avoid colliding with anything the + // process or other tests might rely on; we never set or unset it, so + // no cleanup is required and there is no cross-test state leak. + const key = "WFCTL_TEST_INPUTSNAPSHOT_UNSET_KEY" + plan := map[string]string{key: "deadbeef00000000"} provider := NewTolerantEnvProvider(plan) - val, ok := provider("STAGING_PG_PASSWORD") + val, ok := provider(key) if !ok || val != preservedFingerprint { t.Errorf("expected (preservedFingerprint, true) for plan-time-set unset-now var; got (%q, %v)", val, ok) } From 9a4e35877ead1825618269cb84beb299240e2ad9 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:08:53 -0400 Subject: [PATCH 15/29] docs(iac): clarify ResolvedConfigHash encoding format (Copilot review) Comment said "SHA-256 of POST-substitution Resource.Config" but didn't specify lower-case-hex encoding (no "sha256:" prefix) or the empty-string short-circuit when the config map is empty (platform.ConfigHash behavior). Make the contract explicit so downstream consumers don't guess. Co-Authored-By: Claude Opus 4.7 (1M context) --- interfaces/iac_state.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/interfaces/iac_state.go b/interfaces/iac_state.go index 3ca33491..0d4df2ba 100644 --- a/interfaces/iac_state.go +++ b/interfaces/iac_state.go @@ -70,8 +70,11 @@ type PlanAction struct { Current *ResourceState `json:"current,omitempty"` Changes []FieldChange `json:"changes,omitempty"` - // ResolvedConfigHash is the SHA-256 of POST-substitution Resource.Config. - // Apply re-computes per-action and surfaces per-resource diagnostic on mismatch. + // ResolvedConfigHash is the SHA-256 of POST-substitution Resource.Config, + // computed via platform.ConfigHash. Encoded as lower-case hex (no + // "sha256:" prefix); empty string when the config map is empty + // (platform.ConfigHash short-circuit). Apply re-computes per-action and + // surfaces per-resource diagnostic on mismatch. ResolvedConfigHash string `json:"resolved_config_hash,omitempty"` } From 42c1889e840ea70fe134a6c2a887f8bdbf1c7323 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:20:06 -0400 Subject: [PATCH 16/29] test(iac): use realistic 64-hex value for ResolvedConfigHash round-trip (Copilot review) Test fixture used "sha256:abc" but the actual format produced by platform.ConfigHash is a lower-case 64-hex sha256 digest with no prefix. Replace with a realistic 64-hex value so the test mirrors on-disk shape and won't mislead a future validator/refactor. Co-Authored-By: Claude Opus 4.7 (1M context) --- interfaces/iac_state_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/interfaces/iac_state_test.go b/interfaces/iac_state_test.go index 029196af..494bde58 100644 --- a/interfaces/iac_state_test.go +++ b/interfaces/iac_state_test.go @@ -36,7 +36,11 @@ func TestIaCPlan_InputSnapshotField(t *testing.T) { } func TestPlanAction_ResolvedConfigHashField(t *testing.T) { - a := PlanAction{Action: "create", ResolvedConfigHash: "sha256:abc"} + // platform.ConfigHash returns a lower-case hex sha256 digest with no + // "sha256:" prefix; use a realistic 64-hex value so the test's expected + // shape matches the on-disk format and won't mislead a future validator. + const realisticHash = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + a := PlanAction{Action: "create", ResolvedConfigHash: realisticHash} data, err := json.Marshal(a) if err != nil { t.Fatal(err) @@ -45,7 +49,7 @@ func TestPlanAction_ResolvedConfigHashField(t *testing.T) { if err := json.Unmarshal(data, &got); err != nil { t.Fatal(err) } - if got.ResolvedConfigHash != "sha256:abc" { - t.Errorf("ResolvedConfigHash: got %q", got.ResolvedConfigHash) + if got.ResolvedConfigHash != realisticHash { + t.Errorf("ResolvedConfigHash: got %q want %q", got.ResolvedConfigHash, realisticHash) } } From da9cbe7b94351f3e3eab75975409f13b2ce4c8b5 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:20:15 -0400 Subject: [PATCH 17/29] feat(iac): wfctl infra apply rejects plans with future schema_version (Copilot review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runInfraPlan stamps SchemaVersion=1 on every emitted plan, but runInfraApply was not validating it — a future bump (e.g. W-5 JIT plans) would be silently mis-read by an older binary. Add an infraPlanSchemaVersion constant + guard that rejects plans with schema_version > supported, returning a clear "newer than this wfctl supports" message. Plans with SchemaVersion=0 (predating the field) remain accepted for back-compat. Test: TestInfraApplyConsumesPlan_FutureSchemaRejected. Also reuse inputsnapshot.Compute in fingerprintForTest so the test always exercises the production fingerprint algorithm — re-implementing sha256+16-hex inline would silently drift if the scheme changed. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wfctl/infra.go | 15 +++++++- cmd/wfctl/infra_apply_plan_test.go | 60 ++++++++++++++++++++++++++---- 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index 1ac1d39e..95da8c73 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -16,6 +16,13 @@ import ( "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 + func runInfra(args []string) error { if len(args) < 1 { return infraUsage() @@ -211,7 +218,7 @@ func runInfraPlan(args []string) error { return fmt.Errorf("compute input snapshot: %w", err) } plan.InputSnapshot = snap - plan.SchemaVersion = 1 + plan.SchemaVersion = infraPlanSchemaVersion switch *format { case "markdown": @@ -1075,6 +1082,12 @@ func runInfraApply(args []string) error { if err != nil { return err } + // Reject plans whose on-disk schema is newer than this binary + // understands. SchemaVersion == 0 (unset) is grandfathered in for + // plans emitted by wfctl predating the field. + if plan.SchemaVersion > infraPlanSchemaVersion { + return fmt.Errorf("plan schema_version %d is newer than this wfctl supports (max %d) — upgrade wfctl or re-plan with the older format", plan.SchemaVersion, infraPlanSchemaVersion) + } // Validate that the plan is still current relative to the config. desired, err := parseInfraResourceSpecsForEnv(cfgFile, envName) if err != nil { diff --git a/cmd/wfctl/infra_apply_plan_test.go b/cmd/wfctl/infra_apply_plan_test.go index a68954a8..da950464 100644 --- a/cmd/wfctl/infra_apply_plan_test.go +++ b/cmd/wfctl/infra_apply_plan_test.go @@ -2,8 +2,6 @@ package main import ( "context" - "crypto/sha256" - "encoding/hex" "encoding/json" "errors" "io" @@ -17,12 +15,13 @@ import ( "github.com/GoCodeAlone/workflow/interfaces" ) -// fingerprintForTest matches inputsnapshot.Compute's fingerprint format -// (16-hex-char sha256 prefix) so tests can construct expected plan-time -// snapshots without depending on the concrete env-provider closure. +// fingerprintForTest delegates to inputsnapshot.Compute so the test always +// uses the production fingerprint algorithm. Re-implementing sha256 + 16-hex +// inline would silently drift if the scheme changed; routing through the +// same function the apply path uses makes that impossible. func fingerprintForTest(value string) string { - sum := sha256.Sum256([]byte(value)) - return hex.EncodeToString(sum[:])[:16] + snap := inputsnapshot.Compute([]string{"k"}, func(string) (string, bool) { return value, true }) + return snap["k"] } // TestInfraApplyConsumesPlan verifies that wfctl infra apply --plan : @@ -249,6 +248,53 @@ modules: } } +// TestInfraApplyConsumesPlan_FutureSchemaRejected verifies that a plan whose +// SchemaVersion is greater than the current binary supports is rejected with +// a clear "newer than this wfctl" message rather than being silently +// mis-read as a v1 plan with stray fields. +func TestInfraApplyConsumesPlan_FutureSchemaRejected(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: test-provider + type: iac.provider + config: + provider: fake-cloud + - name: my-db + type: infra.database + config: + provider: test-provider +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + + // Plan declares a schema version newer than this binary supports. + plan := interfaces.IaCPlan{ + ID: "future-schema", + SchemaVersion: infraPlanSchemaVersion + 1, + DesiredHash: "deadbeef", + Actions: []interfaces.PlanAction{{Action: "create", Resource: interfaces.ResourceSpec{Name: "my-db", Type: "infra.database"}}}, + CreatedAt: time.Now().UTC(), + } + planData, err := json.Marshal(plan) + if err != nil { + t.Fatalf("marshal plan: %v", err) + } + planPath := filepath.Join(dir, "plan.json") + if err := os.WriteFile(planPath, planData, 0o600); err != nil { + t.Fatalf("write plan: %v", err) + } + + err = runInfraApply([]string{"--auto-approve", "--config", cfgPath, "--plan", planPath}) + if err == nil { + t.Fatal("expected error for future schema_version, got nil") + } + if !strings.Contains(err.Error(), "schema_version") || !strings.Contains(err.Error(), "newer") { + t.Errorf("error should mention schema_version + newer; got: %v", err) + } +} + // applyCaptureFull is a mock provider that returns a real ApplyResult with // provisioned resources, enabling state-persistence path testing. type applyCaptureFull struct { From 9d30e3555e3907ba3ca93e4e2f673082790a9ed6 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:20:23 -0400 Subject: [PATCH 18/29] fix(iac): gitignoreCovers propagates scan errors so warning surfaces parse failure (Copilot review) Previous round added a scanner.Err() check that returned false either way, making the branch a no-op. Change the helper signature to (bool, error) and have warnIfPlanNotGitignored emit a "could not scan ... for plan.json coverage" stderr warning when the underlying bufio.Scanner fails (e.g. a line over bufio.MaxScanTokenSize). The "not covered" warning is suppressed on scan failure so the operator sees the parse error rather than a potentially-misleading coverage warning. Test: TestGitignoreCovers_ScanError_Propagates. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wfctl/infra_plan_gitignore.go | 36 ++++++++++++++++---------- cmd/wfctl/infra_plan_gitignore_test.go | 21 +++++++++++++++ 2 files changed, 44 insertions(+), 13 deletions(-) diff --git a/cmd/wfctl/infra_plan_gitignore.go b/cmd/wfctl/infra_plan_gitignore.go index 28e316d7..ca807e06 100644 --- a/cmd/wfctl/infra_plan_gitignore.go +++ b/cmd/wfctl/infra_plan_gitignore.go @@ -36,11 +36,20 @@ func warnIfPlanNotGitignored(w io.Writer, planPath string) { foundAny := false covered := false + scanFailed := false for { gitignore := filepath.Join(dir, ".gitignore") if data, err := os.ReadFile(gitignore); err == nil { foundAny = true - if gitignoreCovers(data, base, abs, dir) { + ok, scanErr := gitignoreCovers(data, base, abs, dir) + if scanErr != nil { + // Surface parse failure to the operator (line over + // bufio.MaxScanTokenSize, etc.) rather than silently + // pretending the file is/isn't covered. + fmt.Fprintf(w, "warning: could not scan %s for plan.json coverage: %v\n", gitignore, scanErr) + scanFailed = true + } + if ok { covered = true break } @@ -51,7 +60,7 @@ func warnIfPlanNotGitignored(w io.Writer, planPath string) { } dir = parent } - if foundAny && !covered { + if foundAny && !covered && !scanFailed { fmt.Fprintf(w, "warning: %s is not covered by .gitignore — plan.json may contain semi-sensitive data; add %q to .gitignore before committing.\n", planPath, base) } } @@ -61,7 +70,12 @@ func warnIfPlanNotGitignored(w io.Writer, planPath string) { // This is intentionally a heuristic, not full gitignore semantics: it covers // the common cases (literal basename, "*.ext", "**/", and a path // relative to the gitignore directory) and ignores negation rules. -func gitignoreCovers(data []byte, base, planAbs, gitignoreDir string) bool { +// +// Returns (covered, scanErr). scanErr is non-nil only when the underlying +// bufio.Scanner failed (e.g. a line over bufio.MaxScanTokenSize); the caller +// surfaces that to the operator via stderr instead of silently treating +// scan-failure as either covered or not-covered. +func gitignoreCovers(data []byte, base, planAbs, gitignoreDir string) (bool, error) { ext := filepath.Ext(base) scanner := bufio.NewScanner(bytes.NewReader(data)) for scanner.Scan() { @@ -86,28 +100,24 @@ func gitignoreCovers(data []byte, base, planAbs, gitignoreDir string) bool { anchored := strings.TrimPrefix(line, "/") if anchored == base { - return true + return true, nil } if ext != "" && (anchored == "*"+ext || anchored == "**/*"+ext) { - return true + return true, nil } // "**/" matches at any depth. if anchored == "**/"+base { - return true + return true, nil } // Relative path from .gitignore dir, e.g. "cmd/wfctl/plan.json". if rel, err := filepath.Rel(gitignoreDir, planAbs); err == nil { if anchored == rel || anchored == filepath.ToSlash(rel) { - return true + return true, nil } } } - // Scanner errors (e.g. line longer than bufio.MaxScanTokenSize) cause - // silent fall-through if not checked. Conservative: treat scan failure - // as not-covered, which surfaces a warning the operator can investigate - // rather than silently letting plan.json land in source control. if err := scanner.Err(); err != nil { - return false + return false, err } - return false + return false, nil } diff --git a/cmd/wfctl/infra_plan_gitignore_test.go b/cmd/wfctl/infra_plan_gitignore_test.go index 1e14bcd1..e7577da6 100644 --- a/cmd/wfctl/infra_plan_gitignore_test.go +++ b/cmd/wfctl/infra_plan_gitignore_test.go @@ -95,3 +95,24 @@ modules: t.Errorf("did not expect gitignore warning without .gitignore file, got: %q", stderr) } } + +// TestGitignoreCovers_ScanError_Propagates verifies that when the underlying +// bufio.Scanner fails (e.g. a single line over bufio.MaxScanTokenSize), the +// helper returns the error to the caller rather than silently treating +// scan-failure as either covered or not-covered. The caller (warnIfPlanNotGitignored) +// then surfaces this as an operator-visible "could not scan" warning. +func TestGitignoreCovers_ScanError_Propagates(t *testing.T) { + // One contiguous 70 KiB line — well over bufio.MaxScanTokenSize (64 KiB) + // so Scanner.Scan returns false and Scanner.Err returns the long-line error. + huge := make([]byte, 70*1024) + for i := range huge { + huge[i] = 'x' + } + covered, err := gitignoreCovers(huge, "plan.json", "/tmp/plan.json", "/tmp") + if err == nil { + t.Fatal("expected non-nil scan error for oversized line; got nil") + } + if covered { + t.Errorf("oversized line should not report covered=true; got %v", covered) + } +} From 77bcff161c96970fb54a5fa2e9dcdf738a73ee5d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:20:29 -0400 Subject: [PATCH 19/29] test(iac): TolerantEnvProvider test guarantees Unsetenv precondition with cleanup (Copilot review) Previous round used a unique env-var name and assumed it was unset, but a hostile CI environment could pre-set it and silently flip the test result. Explicitly Unsetenv at start, restore prior value (if any) via t.Cleanup so the test cannot leak state across the process. Co-Authored-By: Claude Opus 4.7 (1M context) --- iac/inputsnapshot/snapshot_test.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/iac/inputsnapshot/snapshot_test.go b/iac/inputsnapshot/snapshot_test.go index edd9b456..468ac268 100644 --- a/iac/inputsnapshot/snapshot_test.go +++ b/iac/inputsnapshot/snapshot_test.go @@ -1,6 +1,7 @@ package inputsnapshot import ( + "os" "testing" ) @@ -42,10 +43,18 @@ func TestCompute_MissingEnvVarOmitted(t *testing.T) { } func TestNewTolerantEnvProvider_UnsetButPlanned_ReturnsSentinel(t *testing.T) { - // Use a test-unique env-var name to avoid colliding with anything the - // process or other tests might rely on; we never set or unset it, so - // no cleanup is required and there is no cross-test state leak. + // Use a test-unique env-var name; even so a hostile CI could pre-set + // it, so explicitly Unsetenv to guarantee the precondition and restore + // any prior value via t.Cleanup so the test cannot leak state. const key = "WFCTL_TEST_INPUTSNAPSHOT_UNSET_KEY" + if prior, had := os.LookupEnv(key); had { + t.Cleanup(func() { _ = os.Setenv(key, prior) }) + } else { + t.Cleanup(func() { _ = os.Unsetenv(key) }) + } + if err := os.Unsetenv(key); err != nil { + t.Fatalf("Unsetenv(%q): %v", key, err) + } plan := map[string]string{key: "deadbeef00000000"} provider := NewTolerantEnvProvider(plan) val, ok := provider(key) From 06cae1cd4c33d90a9e5f40a47760e9cc2251bac8 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:28:53 -0400 Subject: [PATCH 20/29] fix(iac): FormatStaleError hint references "your infra config" not literal "infra.yaml" (Copilot review) wfctl supports configs at non-default paths (e.g. config/infra.yaml, explicit --config arg) so hard-coding "infra.yaml" in the diagnostic misleads operators using a different filename. Generic "your infra config" stays accurate without threading the config path through the formatter signature. Co-Authored-By: Claude Opus 4.7 (1M context) --- iac/inputsnapshot/diagnostic.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/iac/inputsnapshot/diagnostic.go b/iac/inputsnapshot/diagnostic.go index 1f15284d..dc27481a 100644 --- a/iac/inputsnapshot/diagnostic.go +++ b/iac/inputsnapshot/diagnostic.go @@ -15,7 +15,7 @@ import ( // plan stale: %d input(s) changed since plan // KEY1: fingerprint planFP1 (plan) → applyFP1 (apply) // KEY2: fingerprint planFP2 (plan) → applyFP2 (apply) -// hint: ensure all env vars referenced by infra.yaml are exported to both Plan and Apply steps +// hint: ensure all env vars referenced by your infra config are exported to both Plan and Apply steps // // Drift entries are sorted by Name for deterministic output. An empty drift // report yields the singular header line "plan stale: 0 input(s) changed since @@ -32,7 +32,7 @@ func FormatStaleError(drift []interfaces.DriftEntry) string { fmt.Fprintf(&b, " %s: fingerprint %s (plan) → %s (apply)\n", d.Name, d.PlanFingerprint, d.ApplyFingerprint) } if len(sorted) > 0 { - b.WriteString(" hint: ensure all env vars referenced by infra.yaml are exported to both Plan and Apply steps") + b.WriteString(" hint: ensure all env vars referenced by your infra config are exported to both Plan and Apply steps") } return b.String() } From 0f112be1409deb9c098e447b34dad1aac5b76d90 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:29:02 -0400 Subject: [PATCH 21/29] docs(iac): document collectInfraEnvVarRefs top-level envVars merge gap (Copilot review) Copilot correctly identified that collectInfraEnvVarRefs scans the module's own Config but does not apply top-level environments[env].envVars defaults the way planResourcesForEnv does. References that originate solely from a top-level envVars default will therefore be missing from InputSnapshot, so plan-stale drift detection will silently miss those vars changing between plan and apply. Closing the gap requires reusing planResourcesForEnv's merge logic (or extending ResolveForEnv to expose the merged form) and is beyond W-1's scope. Document the limitation in the function godoc so a follow-up can pick it up with full context. Tracking: post-W-1. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wfctl/infra_inputsnapshot.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cmd/wfctl/infra_inputsnapshot.go b/cmd/wfctl/infra_inputsnapshot.go index b94ba7d2..bfb4f132 100644 --- a/cmd/wfctl/infra_inputsnapshot.go +++ b/cmd/wfctl/infra_inputsnapshot.go @@ -20,6 +20,16 @@ import ( // scanned just like any other map: their ${VAR} literals are kept verbatim // in the persisted plan but the plan-time fingerprint of the underlying env // var is still recorded so apply-time drift is detectable. +// +// LIMITATION (tracked, not addressed in W-1): top-level +// environments[env].envVars defaults that planResourcesForEnv merges into +// container-style modules are NOT applied here. References that originate +// solely from a top-level envVars default (without appearing in the +// module's own Config map) won't appear in InputSnapshot, so plan-stale +// drift detection will miss those vars changing between plan and apply. +// Closing the gap requires reusing planResourcesForEnv's merge logic +// before walkValueForEnvRefs; deferred to a follow-up that can extend +// ResolveForEnv to expose the merged form. func collectInfraEnvVarRefs(cfgFile, envName string) ([]string, error) { cfg, err := config.LoadFromFile(cfgFile) if err != nil { From 8ae53e4979a31d8069473ea9fcffdf885a0daf36 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:37:32 -0400 Subject: [PATCH 22/29] fix(iac): introduce *StaleError so user output is FormatStaleError-only (Copilot review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous round used fmt.Errorf("%w\n%s", ErrEnvVarChanged, FormatStaleError(...)) which still printed the sentinel's "env-var changed since plan" text in front of the canonical FormatStaleError output. Introduce inputsnapshot.StaleError whose Error() is exactly FormatStaleError(drift) and whose Unwrap() returns ErrEnvVarChanged — callers get the clean human-readable diagnostic AND keep errors.Is(err, ErrEnvVarChanged) for programmatic detection. cmd/wfctl/infra.go now returns inputsnapshot.NewStaleError(drift) instead of wrapping. Tests: errors_test.go covers Error()/Unwrap()/empty-drift contracts; existing TestApply_PlanStaleDiagnostic_NamesChangedKeys_Persisted still passes unchanged because both errors.Is and "plan stale" text assertions hold. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wfctl/infra.go | 5 ++- iac/inputsnapshot/errors.go | 43 +++++++++++++++++++++--- iac/inputsnapshot/errors_test.go | 56 ++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 6 deletions(-) create mode 100644 iac/inputsnapshot/errors_test.go diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index 95da8c73..7582baba 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -1108,7 +1108,10 @@ func runInfraApply(args []string) error { } applySnap := inputsnapshot.Compute(names, inputsnapshot.OSEnvProvider) if drift := inputsnapshot.ComputeDrift(plan.InputSnapshot, applySnap); len(drift) > 0 { - return fmt.Errorf("%w\n%s", inputsnapshot.ErrEnvVarChanged, inputsnapshot.FormatStaleError(drift)) + // *StaleError: Error() yields the canonical FormatStaleError + // output (no sentinel prefix); Unwrap() yields ErrEnvVarChanged + // so errors.Is(err, inputsnapshot.ErrEnvVarChanged) still matches. + return inputsnapshot.NewStaleError(drift) } } currentHash := desiredStateHash(desired) diff --git a/iac/inputsnapshot/errors.go b/iac/inputsnapshot/errors.go index 26395b87..d8126c4e 100644 --- a/iac/inputsnapshot/errors.go +++ b/iac/inputsnapshot/errors.go @@ -1,14 +1,47 @@ package inputsnapshot -import "errors" +import ( + "errors" + + "github.com/GoCodeAlone/workflow/interfaces" +) // ErrEnvVarChanged is the typed sentinel returned by the apply paths // (cmd/wfctl/infra.go persisted-`--plan` path in W-1; wfctlhelpers.ApplyPlan // in-process path in W-3a/T3.1.5) when an env var referenced at plan time // has a different fingerprint at apply time. Callers match with // errors.Is(err, ErrEnvVarChanged) to detect the plan-stale case -// programmatically; the user-facing "plan stale: ..." prefix is owned by -// FormatStaleError, so this sentinel deliberately uses a short -// machine-only marker to avoid duplicating that prefix when wrapped via -// fmt.Errorf("%w\n%s", ErrEnvVarChanged, FormatStaleError(...)). +// programmatically. To avoid the sentinel's text appearing in user-facing +// output, callers SHOULD construct the user-visible error via +// NewStaleError(drift) — that returns a *StaleError whose Error() is +// exactly FormatStaleError(drift) and whose Unwrap() chain yields this +// sentinel for errors.Is. var ErrEnvVarChanged = errors.New("env-var changed since plan") + +// StaleError is the user-facing error returned by apply paths when env-var +// drift is detected. Its Error() is exactly FormatStaleError(drift) so the +// printed message is the canonical human diagnostic (no duplicated sentinel +// prefix); Unwrap() returns ErrEnvVarChanged so errors.Is works for +// programmatic detection. +type StaleError struct { + Drift []interfaces.DriftEntry +} + +// Error returns FormatStaleError(s.Drift) — the canonical human-readable +// per-key diagnostic with sorted entries and trailing hint. +func (s *StaleError) Error() string { return FormatStaleError(s.Drift) } + +// Unwrap returns ErrEnvVarChanged so callers can use +// errors.Is(err, inputsnapshot.ErrEnvVarChanged) to detect the plan-stale +// case without coupling to the Error() text. +func (s *StaleError) Unwrap() error { return ErrEnvVarChanged } + +// NewStaleError constructs the canonical *StaleError for a non-empty drift +// report. Returns nil when drift is empty (caller should treat that as +// "no plan-stale condition" rather than wrapping a no-op error). +func NewStaleError(drift []interfaces.DriftEntry) *StaleError { + if len(drift) == 0 { + return nil + } + return &StaleError{Drift: drift} +} diff --git a/iac/inputsnapshot/errors_test.go b/iac/inputsnapshot/errors_test.go new file mode 100644 index 00000000..05f49170 --- /dev/null +++ b/iac/inputsnapshot/errors_test.go @@ -0,0 +1,56 @@ +package inputsnapshot + +import ( + "errors" + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestStaleError_ErrorIsFormatStaleError verifies that *StaleError's Error() +// returns exactly FormatStaleError(drift) — no duplicated sentinel prefix +// from ErrEnvVarChanged appears in the user-facing message. +func TestStaleError_ErrorIsFormatStaleError(t *testing.T) { + drift := []interfaces.DriftEntry{ + {Name: "FOO", PlanFingerprint: "aaaa", ApplyFingerprint: "bbbb"}, + } + se := NewStaleError(drift) + if se == nil { + t.Fatal("NewStaleError returned nil for non-empty drift") + } + got := se.Error() + want := FormatStaleError(drift) + if got != want { + t.Errorf("StaleError.Error() = %q\nwant exactly %q", got, want) + } + // Sentinel text must NOT leak into user-facing output. + if strings.Contains(got, ErrEnvVarChanged.Error()) { + t.Errorf("StaleError.Error() leaks sentinel text %q: %q", ErrEnvVarChanged.Error(), got) + } +} + +// TestStaleError_UnwrapMatchesSentinel verifies that errors.Is finds +// ErrEnvVarChanged through *StaleError so callers can detect plan-stale +// programmatically without coupling to the message text. +func TestStaleError_UnwrapMatchesSentinel(t *testing.T) { + se := NewStaleError([]interfaces.DriftEntry{ + {Name: "FOO", PlanFingerprint: "aaaa", ApplyFingerprint: "bbbb"}, + }) + var err error = se + if !errors.Is(err, ErrEnvVarChanged) { + t.Errorf("errors.Is(err, ErrEnvVarChanged) = false; want true") + } +} + +// TestNewStaleError_EmptyDriftReturnsNil verifies the constructor returns +// nil for an empty drift report so callers don't accidentally wrap a no-op +// error. +func TestNewStaleError_EmptyDriftReturnsNil(t *testing.T) { + if got := NewStaleError(nil); got != nil { + t.Errorf("NewStaleError(nil) = %v; want nil", got) + } + if got := NewStaleError([]interfaces.DriftEntry{}); got != nil { + t.Errorf("NewStaleError(empty) = %v; want nil", got) + } +} From 37586b27e7b4ec331fd2f185896748056d2ecdb0 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:37:40 -0400 Subject: [PATCH 23/29] docs(iac): ResolvedConfigHash describes current behavior, not unwired apply consumer (Copilot review) Previous comment claimed apply "surfaces per-resource diagnostic on mismatch", but no apply-time consumer references the field yet. Reword to describe today's behavior (populated by ComputePlan, persisted in plan.json, observable via inspection) and explicitly tag the apply-time consumer as a follow-up (W-3a/T3.1.5) so future readers don't search for code that doesn't exist yet. Co-Authored-By: Claude Opus 4.7 (1M context) --- interfaces/iac_state.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/interfaces/iac_state.go b/interfaces/iac_state.go index 0d4df2ba..39e52443 100644 --- a/interfaces/iac_state.go +++ b/interfaces/iac_state.go @@ -73,8 +73,13 @@ type PlanAction struct { // ResolvedConfigHash is the SHA-256 of POST-substitution Resource.Config, // computed via platform.ConfigHash. Encoded as lower-case hex (no // "sha256:" prefix); empty string when the config map is empty - // (platform.ConfigHash short-circuit). Apply re-computes per-action and - // surfaces per-resource diagnostic on mismatch. + // (platform.ConfigHash short-circuit). + // + // Currently populated by ComputePlan and persisted in plan.json so apply + // has the per-action hash available; the apply-time consumer that surfaces + // a per-resource diagnostic on mismatch is wired in a follow-up PR (W-3a/ + // T3.1.5). Until then the field is observable via plan.json inspection but + // not yet enforced at apply. ResolvedConfigHash string `json:"resolved_config_hash,omitempty"` } From 2f16dc68b33cd804f3b7b0566f7b0131dbde4d80 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:44:25 -0400 Subject: [PATCH 24/29] fix(iac): warnIfPlanNotGitignored uses actual basename, not literal "plan.json" (Copilot review) Both warning messages (scan-failure path and not-covered path) referenced the literal string "plan.json" even though runInfraPlan accepts any --output filename. Use the resolved base variable so an operator who runs `wfctl infra plan -o my-staging-plan.json` sees their actual filename in the warning rather than a misleading "plan.json" reference. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wfctl/infra_plan_gitignore.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/wfctl/infra_plan_gitignore.go b/cmd/wfctl/infra_plan_gitignore.go index ca807e06..ac499906 100644 --- a/cmd/wfctl/infra_plan_gitignore.go +++ b/cmd/wfctl/infra_plan_gitignore.go @@ -46,7 +46,7 @@ func warnIfPlanNotGitignored(w io.Writer, planPath string) { // Surface parse failure to the operator (line over // bufio.MaxScanTokenSize, etc.) rather than silently // pretending the file is/isn't covered. - fmt.Fprintf(w, "warning: could not scan %s for plan.json coverage: %v\n", gitignore, scanErr) + fmt.Fprintf(w, "warning: could not scan %s for %s coverage: %v\n", gitignore, base, scanErr) scanFailed = true } if ok { @@ -61,7 +61,7 @@ func warnIfPlanNotGitignored(w io.Writer, planPath string) { dir = parent } if foundAny && !covered && !scanFailed { - fmt.Fprintf(w, "warning: %s is not covered by .gitignore — plan.json may contain semi-sensitive data; add %q to .gitignore before committing.\n", planPath, base) + fmt.Fprintf(w, "warning: %s is not covered by .gitignore — %s may contain semi-sensitive data; add %q to .gitignore before committing.\n", planPath, base, base) } } From ef7fe1157471e11decc1f32ae8fa2f8713e655e0 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:52:39 -0400 Subject: [PATCH 25/29] fix(iac): bound gitignore walk to enclosing git worktree (Copilot review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous heuristic walked .gitignore files from the plan dir up to the filesystem root, so an unrelated /tmp/.gitignore or $HOME/.gitignore could shadow the real coverage check (or flake the not-covered tests). Add findGitWorktreeRoot — pure stat-based discovery that walks up looking for a .git entry (handles both git directories and git-worktree pointer files). The walk now terminates at the worktree root so unrelated ancestor .gitignore files are ignored, and outside any git worktree the warning stays silent entirely. Tests updated to mark t.TempDir() as a worktree (mkdir .git) where they expect the heuristic to activate; TestPlan_NoGitWorktree_NoWarning added to cover the silent-when-untracked path. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wfctl/infra_plan_gitignore.go | 44 ++++++++++++++++++++---- cmd/wfctl/infra_plan_gitignore_test.go | 46 ++++++++++++++++++++++++-- 2 files changed, 82 insertions(+), 8 deletions(-) diff --git a/cmd/wfctl/infra_plan_gitignore.go b/cmd/wfctl/infra_plan_gitignore.go index ac499906..16640ec8 100644 --- a/cmd/wfctl/infra_plan_gitignore.go +++ b/cmd/wfctl/infra_plan_gitignore.go @@ -11,18 +11,23 @@ import ( ) // warnIfPlanNotGitignored writes a stderr warning to w when planPath is not -// covered by any .gitignore reachable from the directory containing planPath -// up to the filesystem root. +// covered by any .gitignore inside the enclosing git worktree (the nearest +// ancestor directory containing a .git entry). // // Why: plan.json carries semi-sensitive content (env-var fingerprints, // resolved configs, sometimes provider IDs); committing it to source control // is almost always accidental. We don't promise full gitignore semantics — // the heuristic catches the common cases (literal basename, simple -// extension/path globs) and stays silent when no .gitignore exists at all -// (likely not a tracked repo). +// extension/path globs) and stays silent when no enclosing git worktree +// exists (not a tracked repo). +// +// Scope is bounded by the git worktree root rather than walking to the +// filesystem root so an unrelated /tmp/.gitignore or $HOME/.gitignore +// can't generate spurious "covered" results or flake the not-covered tests. // // No warning is emitted when: -// - No .gitignore is found between planDir and the filesystem root. +// - planPath is not inside any git worktree. +// - No .gitignore is found between planDir and the worktree root. // - At least one reachable .gitignore contains a line matching the plan's // basename, the literal plan path, "*.json", "*", or a "**/" pattern // ending with the basename. @@ -33,6 +38,10 @@ func warnIfPlanNotGitignored(w io.Writer, planPath string) { } base := filepath.Base(abs) dir := filepath.Dir(abs) + gitRoot := findGitWorktreeRoot(dir) + if gitRoot == "" { + return // not inside a git worktree — stay silent + } foundAny := false covered := false @@ -54,9 +63,12 @@ func warnIfPlanNotGitignored(w io.Writer, planPath string) { break } } + if dir == gitRoot { + break // reached worktree root — don't walk past it into unrelated trees + } parent := filepath.Dir(dir) if parent == dir { - break // reached filesystem root + break // reached filesystem root (defensive — should hit gitRoot first) } dir = parent } @@ -65,6 +77,26 @@ func warnIfPlanNotGitignored(w io.Writer, planPath string) { } } +// findGitWorktreeRoot walks up from startDir looking for a directory +// containing a ".git" entry (file OR directory — the latter accommodates +// `git worktree add` which writes a .git file pointing into the parent +// repository). Returns the worktree root path, or "" if none is found +// before the filesystem root. Pure stat-based discovery; no shelling out +// to `git` keeps this safe in environments where git is not installed. +func findGitWorktreeRoot(startDir string) string { + d := startDir + for { + if _, err := os.Stat(filepath.Join(d, ".git")); err == nil { + return d + } + parent := filepath.Dir(d) + if parent == d { + return "" // reached filesystem root, no .git found + } + d = parent + } +} + // gitignoreCovers performs a pragmatic match against a .gitignore content for // patterns that would exclude planAbs (basename = base, found at gitignoreDir). // This is intentionally a heuristic, not full gitignore semantics: it covers diff --git a/cmd/wfctl/infra_plan_gitignore_test.go b/cmd/wfctl/infra_plan_gitignore_test.go index e7577da6..645abea9 100644 --- a/cmd/wfctl/infra_plan_gitignore_test.go +++ b/cmd/wfctl/infra_plan_gitignore_test.go @@ -13,6 +13,11 @@ import ( // configs) and must not land in version control by default. func TestPlan_WarnsOnMissingGitignoreEntry(t *testing.T) { repo := t.TempDir() + // Mark repo as a git worktree so warnIfPlanNotGitignored activates; + // .git can be a directory or file (git-worktree pointer) — empty dir is fine. + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o700); err != nil { + t.Fatalf("mkdir .git: %v", err) + } if err := os.WriteFile(filepath.Join(repo, ".gitignore"), []byte("# empty\n"), 0o600); err != nil { t.Fatalf("write .gitignore: %v", err) } @@ -43,6 +48,9 @@ modules: // (no stderr warning) when the output file is already covered by .gitignore. func TestPlan_NoWarningWhenGitignored(t *testing.T) { repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o700); err != nil { + t.Fatalf("mkdir .git: %v", err) + } if err := os.WriteFile(filepath.Join(repo, ".gitignore"), []byte("plan.json\n"), 0o600); err != nil { t.Fatalf("write .gitignore: %v", err) } @@ -69,10 +77,15 @@ modules: } } -// TestPlan_NoGitignoreFile_NoWarning verifies the warning is silent when no -// .gitignore exists in the repo (no git context — likely not a tracked repo). +// TestPlan_NoGitignoreFile_NoWarning verifies the warning is silent when +// the repo IS a git worktree but contains no .gitignore yet (a fresh repo +// is more likely to have an unrelated unconfigured tree than a hostile +// "leak my plan" intent). func TestPlan_NoGitignoreFile_NoWarning(t *testing.T) { repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o700); err != nil { + t.Fatalf("mkdir .git: %v", err) + } cfgPath := filepath.Join(repo, "infra.yaml") if err := os.WriteFile(cfgPath, []byte(` modules: @@ -96,6 +109,35 @@ modules: } } +// TestPlan_NoGitWorktree_NoWarning verifies that runInfraPlan stays silent +// when the plan output path is not inside any git worktree — operators +// running plan in /tmp or other untracked locations should not be nagged +// about an unrelated parent .gitignore that happens to live above them. +func TestPlan_NoGitWorktree_NoWarning(t *testing.T) { + repo := t.TempDir() // intentionally NO .git marker + cfgPath := filepath.Join(repo, "infra.yaml") + if err := os.WriteFile(cfgPath, []byte(` +modules: + - name: vpc + type: infra.vpc + config: + region: nyc1 +`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + planFile := filepath.Join(repo, "plan.json") + + stderr, fnErr := captureStderr(t, func() error { + return runInfraPlan([]string{"--config", cfgPath, "--output", planFile}) + }) + if fnErr != nil { + t.Fatalf("runInfraPlan: %v", fnErr) + } + if strings.Contains(stderr, "gitignore") { + t.Errorf("did not expect gitignore warning outside any git worktree, got: %q", stderr) + } +} + // TestGitignoreCovers_ScanError_Propagates verifies that when the underlying // bufio.Scanner fails (e.g. a single line over bufio.MaxScanTokenSize), the // helper returns the error to the caller rather than silently treating From 96e2327ffe3a8a9cfd61afd419d3676f2f0b8e9b Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:52:47 -0400 Subject: [PATCH 26/29] docs(iac): IaCPlan.InputSnapshot acknowledges top-level envVars merge gap (Copilot review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The InputSnapshot field comment claimed completeness ("env var names read during ${VAR} substitution"), but the cmd/wfctl scanner that populates the map intentionally does not apply top-level environments[env].envVars defaults — that limitation was already documented at collectInfraEnvVarRefs but not surfaced in the public interface contract. Add a "Completeness caveat" note pointing at the scanner's limitation so consumers don't assume the snapshot is exhaustive. Co-Authored-By: Claude Opus 4.7 (1M context) --- interfaces/iac_state.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/interfaces/iac_state.go b/interfaces/iac_state.go index 39e52443..b11103af 100644 --- a/interfaces/iac_state.go +++ b/interfaces/iac_state.go @@ -55,11 +55,18 @@ type IaCPlan struct { // SchemaVersion is bumped when on-disk plan format changes (W-5 sets to 2 when JIT is required). SchemaVersion int `json:"schema_version,omitempty"` - // InputSnapshot records every env var name read during ${VAR} substitution - // at plan time, fingerprinting only those that were SET (16-hex-char sha256 + // InputSnapshot records env var names read during ${VAR} substitution at + // plan time, fingerprinting only those that were SET (16-hex-char sha256 // prefix of the value). Unset vars are omitted from the map; their absence // at apply time is therefore not flagged as drift. Apply re-computes inputs // and prints diagnostic on mismatch. + // + // Completeness caveat: the cmd/wfctl scanner that populates this map + // (cmd/wfctl/infra_inputsnapshot.go::collectInfraEnvVarRefs) currently + // walks each module's own Config but does not apply top-level + // environments[env].envVars defaults. Vars that originate solely from a + // top-level envVars default may therefore be absent from the snapshot; + // closing this gap is tracked as a follow-up to W-1. InputSnapshot map[string]string `json:"input_snapshot,omitempty"` } From dd16006d2d4cbaf402db20b4581876544ae7918e Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 20:59:33 -0400 Subject: [PATCH 27/29] fix(iac): ComputeDrift sorts result slice by Name for deterministic order (Copilot review) Map iteration order in Go is randomized, so consumers that marshal / log / compare the returned drift slice (now exposed via *StaleError.Drift) would see non-deterministic output across runs. FormatStaleError already sorts independently for its printed output; sort the structured slice once at the source so all downstream consumers benefit. Test: TestComputeDrift_ResultIsSortedByName. Co-Authored-By: Claude Opus 4.7 (1M context) --- iac/inputsnapshot/compute_drift.go | 11 ++++++++- iac/inputsnapshot/compute_drift_test.go | 33 +++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/iac/inputsnapshot/compute_drift.go b/iac/inputsnapshot/compute_drift.go index 73a106ce..accaef7b 100644 --- a/iac/inputsnapshot/compute_drift.go +++ b/iac/inputsnapshot/compute_drift.go @@ -1,6 +1,10 @@ package inputsnapshot -import "github.com/GoCodeAlone/workflow/interfaces" +import ( + "sort" + + "github.com/GoCodeAlone/workflow/interfaces" +) // unsetFingerprintPlaceholder is the in-package constant displayed in the // ApplyFingerprint field when the var was set at plan time but is missing @@ -42,5 +46,10 @@ func ComputeDrift(planSnap, applySnap map[string]string) []interfaces.DriftEntry }) } } + // Sort by Name so the returned slice is deterministic across runs — + // callers may marshal/log/compare the slice (StaleError.Drift is + // publicly exposed). FormatStaleError sorts independently for its own + // output; this sort makes the structured slice match the printed order. + sort.Slice(drift, func(i, j int) bool { return drift[i].Name < drift[j].Name }) return drift } diff --git a/iac/inputsnapshot/compute_drift_test.go b/iac/inputsnapshot/compute_drift_test.go index 7394cea1..62446f0a 100644 --- a/iac/inputsnapshot/compute_drift_test.go +++ b/iac/inputsnapshot/compute_drift_test.go @@ -45,3 +45,36 @@ func TestComputeDrift_MatchingFingerprints_NoDrift(t *testing.T) { t.Errorf("matching fingerprints should produce no drift; got %+v", drift) } } + +// TestComputeDrift_ResultIsSortedByName verifies the returned slice is +// stable across map-iteration randomness so callers (logs, JSON marshal, +// test asserts) get deterministic output. Multiple keys + multiple runs +// would each produce a different non-deterministic order without the sort; +// asserting a single canonical order across one call is enough to catch +// regression of the sort. +func TestComputeDrift_ResultIsSortedByName(t *testing.T) { + planSnap := map[string]string{ + "ZULU": "ffff000000000000", + "ALPHA": "1111000000000000", + "MIKE": "8888000000000000", + "BRAVO": "2222000000000000", + "YANKEE": "eeee000000000000", + } + applySnap := map[string]string{ + "ZULU": "f0f0000000000000", + "ALPHA": "1010000000000000", + "MIKE": "8080000000000000", + "BRAVO": "2020000000000000", + "YANKEE": "e0e0000000000000", + } + drift := ComputeDrift(planSnap, applySnap) + if len(drift) != 5 { + t.Fatalf("expected 5 drift entries; got %d", len(drift)) + } + want := []string{"ALPHA", "BRAVO", "MIKE", "YANKEE", "ZULU"} + for i, w := range want { + if drift[i].Name != w { + t.Errorf("drift[%d].Name = %q; want %q (slice should be sorted by Name)", i, drift[i].Name, w) + } + } +} From 0a7a58d6d5825df6ae5d3ef09ab94d8bbc372936 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 21:05:26 -0400 Subject: [PATCH 28/29] docs(iac): note ResolvedConfigHash omitempty=field-absent on disk (Copilot review) Field is tagged json:",omitempty" so the empty-string case is dropped from plan.json entirely rather than persisted as ""; consumers should treat "key missing" and "value == empty string" as the same condition. Comment now states this explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) --- interfaces/iac_state.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/interfaces/iac_state.go b/interfaces/iac_state.go index b11103af..668b834b 100644 --- a/interfaces/iac_state.go +++ b/interfaces/iac_state.go @@ -80,7 +80,9 @@ type PlanAction struct { // ResolvedConfigHash is the SHA-256 of POST-substitution Resource.Config, // computed via platform.ConfigHash. Encoded as lower-case hex (no // "sha256:" prefix); empty string when the config map is empty - // (platform.ConfigHash short-circuit). + // (platform.ConfigHash short-circuit). The field uses `omitempty`, so the + // empty-string case is ABSENT from plan.json — consumers should treat + // "key missing" and "value == empty string" as the same condition. // // Currently populated by ComputePlan and persisted in plan.json so apply // has the per-action hash available; the apply-time consumer that surfaces From 88536a03a7fd7e19472b96dea470c8b85bc8a33e Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 21:11:00 -0400 Subject: [PATCH 29/29] perf(iac): Compute capacity hint + hoist gitignoreCovers per-line constants (Copilot review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two micro-optimizations: - inputsnapshot.Compute now allocates with len(varNames) capacity hint to avoid grow-resize cycles when many env vars are referenced. - gitignoreCovers hoists filepath.Rel/ToSlash and the base-derived pattern strings (starExt, doubleStarExt, doubleStarBase) out of the per-line scan loop — they're constant for the whole .gitignore file. No behavior change; less per-line allocation. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/wfctl/infra_plan_gitignore.go | 30 ++++++++++++++++++++++++------ iac/inputsnapshot/snapshot.go | 5 ++++- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/cmd/wfctl/infra_plan_gitignore.go b/cmd/wfctl/infra_plan_gitignore.go index 16640ec8..4a0ef618 100644 --- a/cmd/wfctl/infra_plan_gitignore.go +++ b/cmd/wfctl/infra_plan_gitignore.go @@ -109,6 +109,26 @@ func findGitWorktreeRoot(startDir string) string { // scan-failure as either covered or not-covered. func gitignoreCovers(data []byte, base, planAbs, gitignoreDir string) (bool, error) { ext := filepath.Ext(base) + // Hoist the constant relative-path computation out of the per-line + // scan loop — planAbs and gitignoreDir don't change per iteration, so + // computing rel/relSlash once avoids repeated filesystem-string work + // for every gitignore entry. Empty rel/relSlash + relErr means we + // skip the path-equality branch in-loop. + var rel, relSlash string + rel, relErr := filepath.Rel(gitignoreDir, planAbs) + if relErr == nil { + relSlash = filepath.ToSlash(rel) + } + + // Pre-compute the patterns derived from base/ext for the same reason. + starExt := "" + doubleStarExt := "" + if ext != "" { + starExt = "*" + ext + doubleStarExt = "**/*" + ext + } + doubleStarBase := "**/" + base + scanner := bufio.NewScanner(bytes.NewReader(data)) for scanner.Scan() { line := strings.TrimSpace(scanner.Text()) @@ -134,18 +154,16 @@ func gitignoreCovers(data []byte, base, planAbs, gitignoreDir string) (bool, err if anchored == base { return true, nil } - if ext != "" && (anchored == "*"+ext || anchored == "**/*"+ext) { + if ext != "" && (anchored == starExt || anchored == doubleStarExt) { return true, nil } // "**/" matches at any depth. - if anchored == "**/"+base { + if anchored == doubleStarBase { return true, nil } // Relative path from .gitignore dir, e.g. "cmd/wfctl/plan.json". - if rel, err := filepath.Rel(gitignoreDir, planAbs); err == nil { - if anchored == rel || anchored == filepath.ToSlash(rel) { - return true, nil - } + if relErr == nil && (anchored == rel || anchored == relSlash) { + return true, nil } } if err := scanner.Err(); err != nil { diff --git a/iac/inputsnapshot/snapshot.go b/iac/inputsnapshot/snapshot.go index 9bd4de2e..b0d06c1a 100644 --- a/iac/inputsnapshot/snapshot.go +++ b/iac/inputsnapshot/snapshot.go @@ -12,7 +12,10 @@ import ( // Compute returns a map of env-var name → 16-hex-char sha256 prefix of the value. // Variables that aren't set (lookup returns ok=false) are omitted from the snapshot. func Compute(varNames []string, lookup func(string) (string, bool)) map[string]string { - out := make(map[string]string) + // Capacity hint = len(varNames) — even if some keys are omitted (unset), + // over-allocating by at most that fraction is cheaper than the + // reallocations that would otherwise happen as the map grows. + out := make(map[string]string, len(varNames)) for _, name := range varNames { val, ok := lookup(name) if !ok {