From 4de6d8e297fb57b8801563fd0d4c8eef2bfbf3cb Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 31 May 2026 16:26:45 -0400 Subject: [PATCH 1/8] docs(cigen): design (adversarial PASS) + implementation plan for #3 per-phase secret scoping + #4 migration flags Co-Authored-By: Claude Opus 4.8 (1M context) --- ...y-secret-scoping-migration-flags-design.md | 107 ++++ ...fidelity-secret-scoping-migration-flags.md | 602 ++++++++++++++++++ 2 files changed, 709 insertions(+) create mode 100644 docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags-design.md create mode 100644 docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md diff --git a/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags-design.md b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags-design.md new file mode 100644 index 00000000..0c3b8edd --- /dev/null +++ b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags-design.md @@ -0,0 +1,107 @@ +# cigen fidelity: per-phase secret scoping + migration operational flags — design + +**Date:** 2026-05-31 +**Author:** autonomous pipeline (codingsloth@pm.me approved) +**Status:** Design — adversarial review PASS (2 cycles, 2026-05-31) +**Repo:** `workflow` (the `cigen` package + GHA renderer). Follow-on to v0.67.0; closes 2 gaps measured in the multisite regen evidence (retro 2026-05-30). + +## 1. Problem + +Two fidelity gaps in `cigen`'s GitHub Actions output vs the hand-tuned `gocodealone-multisite/infra.yml` (both documented in `cigen/testdata/multisite/GAP.md`): + +- **#3 over-broad secret scoping.** `cigen.Analyze` builds ONE global `Secrets` union (from the primary config only) and `render_gha` writes that union into **every** apply job's `env:`. In a two-phase deploy, `apply-prereq` thus receives secrets it never uses (e.g. `MULTISITE_DB_URL`), widening blast radius. The hand-written workflow scopes each job to its own secrets. +- **#4 bare migration step.** `cigen` emits `wfctl migrations up --config `; the real workflow uses `wfctl migrations up --config deploy.yaml --env prod --format json`. `--format json` (machine-readable output) and `--env ` are missing. (`MigrationsSpec.Env` field already exists from PR4 but is never populated, and `--format json` is never emitted.) + +## 2. Goal + +`cigen` GHA output: (a) each apply job's `env:` carries only the secrets that phase's config references (+ the migration DB secret only on the migrating phase); (b) the migrations step emits `--format json` always and `--env ` when an environment is unambiguously derivable. `wfctl ci generate` benefits immediately; the ci-generator plugin picks it up on its next workflow-dep bump (noted, not required). + +## 3. Non-goals + +- Per-phase scoping for **single-phase** deploys (one apply job → the union IS its scope; no change). +- Deriving `--env` when ambiguous (multiple `ci.migrations[0].environments` keys) — omit + warn rather than guess. +- Scoping when the phase-config is given only as a **logical alias** (MCP `--phase-config-alias`, no real file to load) — fall back to the union + a warning. +- A plugin release/bump (separate optional follow-on; `wfctl ci generate` gets the fix directly). +- Touching GitLab/Jenkins/CircleCI renderers (GHA only — that's where phases + the measured gap live; GitLab uses project-level CI vars so per-job scoping is N/A). + +## 4. Architecture + +### #3 — per-phase secret scoping + +- **Model:** add to `DeployPhase`: `Secrets []SecretRef` (the secrets that phase's apply job needs) **and `Scoped bool`** (true iff per-phase derivation actually ran on a real loaded config). Keep `CIPlan.Secrets` (union) for single-phase, JSON consumers, and the unscoped fallback. **The renderer branches on `Scoped`, NOT on `len(Secrets)>0`** — so a genuinely zero-secret phase (Scoped=true, empty Secrets) emits an empty `env:`, while an unloadable phase (Scoped=false) falls back to the union. (Resolves adversarial C2: empty-slice must not mean "fall back".) +- **Phase config source (resolves I1):** the prereq config is loaded from **`opts.PhaseConfig`** (a real filesystem path), NOT from `configs[1:]` — `Analyze`'s `configs []string` is single-element in all current call paths (only `configs[0]` is read). Load it via `config.LoadFromFile(opts.PhaseConfig)`. +- **Analyze:** when `opts.PhaseConfig` is a real, loadable file (not alias-only, file exists+parses): load it, run existing `deriveSecrets` on it → prereq phase `{Secrets, Scoped:true}`; the primary config's secrets → the deploy (last) phase `{Secrets, Scoped:true}`. `derivePhases` order unchanged (prereq-first, deploy/primary-last). +- **Migrating phase = the LAST phase (resolves I2).** This matches `render_gha`'s existing `isLastPhase` step placement. `deriveMigrations` is called ONLY on the primary config (unchanged); the migration DB secret (`Migrations.DBEnv`) is added to the **last** phase's `Secrets`, never the prereq's. (If a future design needs migrations in a non-last phase, that's a separate change — documented as a constraint, not silently assumed.) +- **Fallback (Scoped=false):** single-phase, OR `opts.PhaseConfig` alias-only/missing/unparseable → phases keep `Scoped:false`; renderer uses `CIPlan.Secrets` union (current behavior). Emit a `Warnings[]` note only for the alias-only/unloadable *multi-phase* case ("per-phase secret scoping unavailable: phase config not loadable; using union"). Single-phase needs no warning (union IS its scope). +- **render_gha (`writeApplyJob`):** `env:` = `phase.Secrets` when `phase.Scoped`, else `CIPlan.Secrets`. + +### #4 — migration operational flags + +- **`--format json`:** appended to the generated `wfctl migrations up` step. **Operator explicitly requested this** (match the deployed multisite workflow). Tradeoff acknowledged (resolves I3): the generated step does not pipe the JSON downstream, so a failed migration logs a JSON blob rather than text — but it matches the real workflow, exit-code on failure is unchanged (`runMigrationsUp` still errors non-zero), and an operator can edit it out. Kept as requested; noted in GAP.md as a deliberate match-the-deployed-pattern choice. +- **`--env `:** populate `MigrationsSpec.Env` in `deriveMigrations` from `ci.migrations[0].environments` — **only when exactly one** key exists (unambiguous). Zero keys → `Env` empty, no `--env` (this is the multisite case — see C1). ≥2 keys → `Env` empty + a `Warnings[]` note ("migrations environment ambiguous (N declared); --env omitted — set it in the workflow"). Render already emits `--env ` iff `Env != ""`. + +### Evidence refresh (HONEST — resolves adversarial C1) + +The multisite `deploy.yaml` `ci.migrations` block declares **NO `environments:`** key → `--env` is NOT derivable for multisite; only `--format json` lands. So in `GAP.md`: +- **Move to "now derivable / matched":** per-phase secret scoping (apply-prereq no longer carries the deploy-only DB secret) + `--format json` on the migrations step. +- **KEEP in "not derivable" (corrected, not removed):** the migrations `--env ` — requires an `environments:` declaration in `ci.migrations` that multisite does not have (also the hash-suffixed DB secret name, GHCR image-wait, GHCR creds, GA4 step, smoke matrix, concurrency, SHA-pins). +Regenerate `cigen/testdata/multisite/{plan.json,generated-infra.yml}` with the real binary; the measured diff must show apply-prereq's `env:` shrunk (no DB secret) + the migrations step with `--format json` (and NO `--env`, honestly, since multisite declares no environments). The claim is exactly the measured diff — never "matches the hand-tuned `--env prod`". + +## 5. Data flow + +`Analyze(primary, opts{PhaseConfig})` → loads primary (+ phase-config if real) → `deriveSecrets` per config → `DeployPhase.Secrets` per phase + `CIPlan.Secrets` union + `Migrations{DBEnv,Env}` → `RenderGitHubActions` emits per-apply-job `env:` (phase scope, union fallback) + migrations step `wfctl migrations up --config [--env ] --format json`. + +## 6. Error handling / partial failure + +- Phase-config path set but file missing/unparseable → do NOT fail; warn + fall back to union scoping (the path may be a future-checkout-relative or alias path). Log to `Warnings[]`. +- `ci.migrations[0].environments` ambiguous (≥2) → omit `--env`, warn. +- Single phase → union (unchanged); no warning (it's correct). +- A secret in the migrating phase that's ALSO in prereq (shared cred, e.g. SPACES_*) → appears in both jobs' env legitimately (each phase needs it). Per-phase scoping is per-config-reference, so shared creds correctly appear in both. + +## 7. Testing + +- Unit (golden): a two-phase fixture (primary + prereq config) → assert prereq phase `Secrets` excludes the deploy-only secret (e.g. DB url) and the deploy phase includes it; assert `render_gha` apply-prereq `env:` lacks it and apply-deploy `env:` has it. Single-phase fixture → union unchanged. +- Migrations: fixture with `ci.migrations.environments: {prod: …}` → `--env prod --format json`; fixture with 2 envs → `--format json` only + warning; fixture with 0 → `--format json` only. +- Alias-only phase-config → union fallback + warning (no load). +- Renderer output parses as YAML (existing parse-back assertion). +- **Runtime / demonstration-fidelity:** real `wfctl ci generate` on the committed multisite testdata → regenerate evidence; the real diff must show apply-prereq no longer carrying the DB secret and the migrations step carrying `--format json` (+ `--env` iff multisite declares a single env). Commit the literal output. + +## Global Design Guidance + +Source: `docs/design-guidance.md`. + +| guidance | response | +|---|---| +| Dogfood; wfctl is primary surface | Improves `wfctl ci generate` directly | +| Reuse over rebuild | Reuses `deriveSecrets`/`MigrationsSpec.Env`/render scaffolding; additive `DeployPhase.Secrets` | +| Secrets never logged | Scoping reduces secret exposure in generated CI (names only, as before; tighter blast radius) | +| Multi-component validation; real-consumer proof | Multisite real-config regen + measured diff (demonstration-fidelity) | + +## Security Review + +Net **reduction** in secret exposure: per-phase scoping removes secrets from jobs that don't use them (smaller blast radius if a job/log is compromised). Still names-only in manifests; no values. `--format json` changes only output format (no secret in output). No new trust boundary. + +## Infrastructure Impact + +None — generates YAML. No cloud resources, no release (the cigen change ships in `wfctl ci generate`; plugin bump is a separate optional follow-on). The committed multisite evidence is a testdata artifact; the live `infra.yml` is untouched (ADR 0004 stands). + +## Multi-Component Validation + +Real `wfctl ci generate` against the committed multisite configs (primary + prereq) — exercises Analyze (two-config load) → render → measured diff. Golden tests assert the per-phase env split + migration flags. + +## Assumptions + +1. `config.LoadFromFile` can load the phase-config (deploy.prereq.yaml) the same way as the primary. *(It's the same config shape; verify in impl.)* +2. The migrating phase = the **LAST** phase (matches `render_gha`'s `isLastPhase` step placement; `deriveMigrations` runs only on the primary/deploy config — prereq provisions DB, deploy/last runs migrations). *(See §4 #3.)* +3. `ci.migrations[0].environments` keys are the deploy env names; a single key is the unambiguous `--env`. *(Matches the config schema. NOTE: multisite declares NO `environments:` → `--env` NOT derivable for multisite — see C1/§4 Evidence refresh.)* +4. Per-phase scoping only matters for multi-phase; single-phase union is already correct. *(By construction.)* + +## Rollback + +Runtime-affecting class: generator output (CI workflow content). Rollback = revert the cigen PR; `wfctl ci generate` reverts to the union-scoping + bare-migrations output. Additive `DeployPhase.Secrets` field (JSON omitempty) — old plan.json consumers unaffected. No release, no migration, no state. + +## Self-challenge — top doubts + +1. **Is per-phase scoping worth the Analyze two-config-load complexity?** It's the operator-requested fidelity fix + a real (if modest) blast-radius reduction; bounded (load + existing deriveSecrets per config, union fallback). Worth it. +2. **`--env` derivation from `ci.migrations.environments` may not match the operator's intent** (they might run migrations against a different env than declared). Mitigated: only derive when exactly one env declared; warn + omit otherwise — never guess. +3. **Alias-only phase-config (MCP path) can't be scoped.** Falls back to the union (today's behavior) + a warning — no regression, just no improvement in that path. diff --git a/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md new file mode 100644 index 00000000..4bec613e --- /dev/null +++ b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md @@ -0,0 +1,602 @@ +# cigen fidelity: per-phase secret scoping + migration operational flags — Implementation Plan + +> **For the implementing agent:** REQUIRED SUB-SKILL: Use autodev:executing-plans to implement this plan task-by-task. + +**Goal:** Narrow two measured fidelity gaps in cigen's GitHub Actions output: (#3) scope each apply job's `env:` to only the secrets that phase's config references; (#4) emit `wfctl migrations up … --format json` always plus `--env ` when unambiguously derivable. + +**Architecture:** Additive `DeployPhase.{Secrets,Scoped}` fields; `Analyze` loads the prereq config from `opts.PhaseConfig` and runs the existing `deriveSecrets` per config; `render_gha` branches each apply job's `env:` source on `phase.Scoped` (else the union fallback). `deriveMigrations` populates `MigrationsSpec.Env` only when exactly one `ci.migrations[0].environments` key exists; `migrationsUpCommand` appends `--format json` unconditionally. + +**Tech Stack:** Go (stdlib + `github.com/GoCodeAlone/workflow/config`). No new deps. No release. + +**Base branch:** main (worktree branch `feat/cigen-phase-secret-scoping`) + +--- + +## Scope Manifest + +**PR Count:** 1 +**Tasks:** 5 +**Estimated Lines of Change:** ~180 (informational; not enforced) + +**Out of scope:** +- Per-phase scoping for single-phase deploys (union IS the scope; no change). +- Deriving `--env` when ambiguous (≥2 `environments` keys) — omit + warn. +- Scoping when the phase-config is alias-only (no loadable file) — union fallback + warn. +- A plugin (ci-generator) release/bump — `wfctl ci generate` gets the fix directly; plugin picks it up on its next workflow-dep bump (noted, optional follow-on). +- GitLab/Jenkins/CircleCI renderers — GHA only (where phases + the measured gap live). +- `--env prod` for multisite specifically — multisite declares NO `environments:`, so `--env` stays not-derivable for it (honest; design C1). + +**PR Grouping:** + +| PR # | Title | Tasks | Branch | +|------|-------|-------|--------| +| 1 | feat(cigen): per-phase secret scoping + migration --format json/--env | Task 1, Task 2, Task 3, Task 4, Task 5 | feat/cigen-phase-secret-scoping | + +**Status:** Draft + +--- + +### Task 1: DeployPhase model — add Secrets + Scoped + +**Files:** +- Modify: `cigen/plan.go:51-58` (DeployPhase struct) +- Test: `cigen/plan_test.go` (new — minimal struct/JSON round-trip) + +**Step 1: Write the failing test** + +Create `cigen/plan_test.go`: + +```go +package cigen + +import ( + "encoding/json" + "testing" +) + +func TestDeployPhase_ScopedSecretsJSON(t *testing.T) { + p := DeployPhase{ + Name: "prereq", + ConfigPath: "deploy.prereq.yaml", + Secrets: []SecretRef{{Name: "DIGITALOCEAN_TOKEN"}}, + Scoped: true, + } + b, err := json.Marshal(p) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var got DeployPhase + if err := json.Unmarshal(b, &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if !got.Scoped || len(got.Secrets) != 1 || got.Secrets[0].Name != "DIGITALOCEAN_TOKEN" { + t.Fatalf("round-trip lost fields: %+v", got) + } + // Unscoped phase with no secrets must omit both in JSON (additive, back-compat). + b2, _ := json.Marshal(DeployPhase{Name: "deploy", ConfigPath: "deploy.yaml"}) + if string(b2) != `{"name":"deploy","config_path":"deploy.yaml"}` { + t.Fatalf("unexpected JSON for unscoped phase: %s", b2) + } +} +``` + +**Step 2: Run test to verify it fails** + +Run: `GOWORK=off go test ./cigen/ -run TestDeployPhase_ScopedSecretsJSON -v` +Expected: FAIL (compile error — `Secrets`/`Scoped` undefined). + +**Step 3: Implement** + +Edit `cigen/plan.go` DeployPhase struct to: + +```go +// DeployPhase is a single phase in a potentially multi-phase deploy pipeline. +type DeployPhase struct { + // Name is the human-readable phase name (e.g. "prereq", "deploy"). + Name string `json:"name"` + // ConfigPath is the workflow config file for this phase. + ConfigPath string `json:"config_path"` + // Include is an optional list of module names to include in this phase. + Include []string `json:"include,omitempty"` + // Secrets is the set of secrets this phase's apply job needs. Populated + // only when Scoped is true; otherwise the renderer uses CIPlan.Secrets. + Secrets []SecretRef `json:"secrets,omitempty"` + // Scoped is true when per-phase secret derivation ran against a real, + // loaded config for this phase. The renderer branches its env: source on + // this flag — NOT on len(Secrets) — so a genuinely zero-secret scoped + // phase emits no union, while an unscoped phase falls back to the union. + Scoped bool `json:"scoped,omitempty"` +} +``` + +**Step 4: Run test to verify it passes** + +Run: `GOWORK=off go test ./cigen/ -run TestDeployPhase_ScopedSecretsJSON -v` +Expected: PASS. + +**Step 5: Commit** + +```bash +git add cigen/plan.go cigen/plan_test.go +git commit -m "feat(cigen): add DeployPhase.Secrets + Scoped for per-phase scoping" +``` + +--- + +### Task 2: Analyze — per-phase secret scoping + migration env derivation + +**Files:** +- Modify: `cigen/analyze.go` — `Analyze` (per-phase secrets), `derivePhases` (carry secrets/scoped), `deriveMigrations` (populate Env), `deriveWarnings` (ambiguity + unscopable notes) +- Test: `cigen/analyze_test.go` (extend) + +**Change class:** internal logic (analyze pure function) → unit tests sufficient. + +**Step 1: Write the failing tests** + +Append to `cigen/analyze_test.go`: + +```go +func TestAnalyze_PerPhaseScoping_PrereqExcludesDeploySecret(t *testing.T) { + plan, err := Analyze([]string{"testdata/multisite/deploy.yaml"}, Options{ + PhaseConfig: "testdata/multisite/deploy.prereq.yaml", + }) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if len(plan.Phases) != 2 { + t.Fatalf("expected 2 phases, got %d", len(plan.Phases)) + } + prereq, deploy := plan.Phases[0], plan.Phases[1] + if !prereq.Scoped || !deploy.Scoped { + t.Fatalf("expected both phases Scoped, got prereq=%v deploy=%v", prereq.Scoped, deploy.Scoped) + } + if hasSecret(prereq.Secrets, "MULTISITE_DB_URL") { + t.Errorf("prereq phase must NOT carry the deploy-only migration secret MULTISITE_DB_URL; got %v", names(prereq.Secrets)) + } + if !hasSecret(deploy.Secrets, "MULTISITE_DB_URL") { + t.Errorf("deploy (last) phase must carry MULTISITE_DB_URL; got %v", names(deploy.Secrets)) + } + // prereq genuinely needs the provider token — sanity that scoping isn't empty. + if !hasSecret(prereq.Secrets, "DIGITALOCEAN_TOKEN") { + t.Errorf("prereq phase should carry DIGITALOCEAN_TOKEN; got %v", names(prereq.Secrets)) + } +} + +func TestAnalyze_SinglePhase_NotScoped(t *testing.T) { + plan, err := Analyze([]string{"testdata/multisite/deploy.yaml"}, Options{}) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if len(plan.Phases) != 1 { + t.Fatalf("expected 1 phase, got %d", len(plan.Phases)) + } + if plan.Phases[0].Scoped { + t.Errorf("single-phase deploy must not be Scoped (union is its scope)") + } +} + +func TestAnalyze_PhaseConfigAliasOnly_FallsBackToUnion(t *testing.T) { + plan, err := Analyze([]string{"testdata/multisite/deploy.yaml"}, Options{ + PhaseConfig: "/nonexistent/deploy.prereq.yaml", + PhaseConfigAlias: "deploy.prereq.yaml", + }) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if plan.Phases[0].Scoped { + t.Errorf("alias-only/unloadable phase config must fall back (Scoped=false)") + } + if !containsSubstr(plan.Warnings, "per-phase secret scoping unavailable") { + t.Errorf("expected an unscopable warning; got %v", plan.Warnings) + } +} + +func TestDeriveMigrations_SingleEnvDerived(t *testing.T) { + cfg, err := config.LoadFromFile("testdata/migrations-one-env.yaml") + if err != nil { + t.Fatalf("load: %v", err) + } + m := deriveMigrations(cfg) + if m == nil || m.Env != "prod" { + t.Fatalf("expected Env=prod, got %+v", m) + } +} + +func TestDeriveMigrations_TwoEnvsAmbiguous(t *testing.T) { + cfg, err := config.LoadFromFile("testdata/migrations-two-envs.yaml") + if err != nil { + t.Fatalf("load: %v", err) + } + m := deriveMigrations(cfg) + if m == nil || m.Env != "" { + t.Fatalf("expected Env empty (ambiguous), got %+v", m) + } + w := deriveWarnings(cfg, m, deriveSecrets(cfg, m)) + if !containsSubstr(w, "migrations environment ambiguous") { + t.Errorf("expected ambiguity warning; got %v", w) + } +} + +// test helpers +func hasSecret(s []SecretRef, name string) bool { + for _, r := range s { + if r.Name == name { + return true + } + } + return false +} +func names(s []SecretRef) []string { + out := make([]string, 0, len(s)) + for _, r := range s { + out = append(out, r.Name) + } + return out +} +func containsSubstr(ss []string, sub string) bool { + for _, s := range ss { + if strings.Contains(s, sub) { + return true + } + } + return false +} +``` + +Add two fixtures. `cigen/testdata/migrations-one-env.yaml`: + +```yaml +ci: + migrations: + - name: app + source_dir: migrations + database: + env: APP_DB_URL + environments: + prod: + database: + env: APP_DB_URL +``` + +`cigen/testdata/migrations-two-envs.yaml`: + +```yaml +ci: + migrations: + - name: app + source_dir: migrations + database: + env: APP_DB_URL + environments: + staging: + database: + env: STAGING_DB_URL + prod: + database: + env: PROD_DB_URL +``` + +(If `config.LoadFromFile` rejects a config with no `modules:`, add a minimal `modules: [{name: noop, type: http.server, config: {address: ":0"}}]` block to each fixture — verify by running the test; adjust until it loads.) + +**Step 2: Run tests to verify they fail** + +Run: `GOWORK=off go test ./cigen/ -run 'TestAnalyze_PerPhase|TestAnalyze_SinglePhase|TestAnalyze_PhaseConfigAlias|TestDeriveMigrations' -v` +Expected: FAIL (per-phase secrets not populated; Env not derived; warnings absent). + +**Step 3: Implement** + +In `cigen/analyze.go`: + +(a) `deriveMigrations` — derive `Env` from a single `environments` key: + +```go +func deriveMigrations(cfg *config.WorkflowConfig) *MigrationsSpec { + if cfg.CI == nil || len(cfg.CI.Migrations) == 0 { + return nil + } + m := cfg.CI.Migrations[0] + spec := &MigrationsSpec{ + DBEnv: m.Database.Env, + Source: m.SourceDir, + } + if spec.DBEnv == "" { + return nil + } + // Derive --env only when exactly one environment is declared (unambiguous). + if len(m.Environments) == 1 { + for envName := range m.Environments { + spec.Env = envName + } + } + return spec +} +``` + +(b) `Analyze` — compute per-phase secrets and pass into `derivePhases`. Replace the phases block (currently lines ~95-112) with: + +```go + primaryConfigPath := opts.ConfigPathAlias + if primaryConfigPath == "" { + primaryConfigPath = relativizeConfigPath(primaryPath) + } + + // Per-phase secret scoping (multi-phase only). The prereq phase is scoped to + // the secrets ITS config references; the deploy (last) phase keeps the + // primary union (which already includes the migration DBEnv via deriveSecrets). + var prereqSecrets []SecretRef + scoped := false + phaseConfigPath := opts.PhaseConfig + if phaseConfigPath != "" { + if opts.PhaseConfigAlias != "" { + phaseConfigPath = opts.PhaseConfigAlias + } else { + phaseConfigPath = relativizeConfigPath(opts.PhaseConfig) + } + if pcfg, perr := config.LoadFromFile(opts.PhaseConfig); perr == nil { + // deriveMigrations is primary-only: the migrating phase is the LAST + // phase, so the prereq never gets a migration DBEnv (pass nil). + prereqSecrets = deriveSecrets(pcfg, nil) + scoped = true + } else { + plan.Warnings = append(plan.Warnings, + fmt.Sprintf("per-phase secret scoping unavailable: phase config %q not loadable (%v); using union", opts.PhaseConfig, perr)) + } + } + + plan.Phases = derivePhases(primaryConfigPath, phaseConfigPath, plan.Secrets, prereqSecrets, scoped) +``` + +(c) `derivePhases` — new signature carrying secrets/scoped: + +```go +func derivePhases(primaryPath, phaseConfig string, primarySecrets, prereqSecrets []SecretRef, scoped bool) []DeployPhase { + var phases []DeployPhase + if phaseConfig != "" { + phases = append(phases, DeployPhase{ + Name: "prereq", + ConfigPath: phaseConfig, + Secrets: prereqSecrets, + Scoped: scoped, + }) + } + deploy := DeployPhase{ + Name: "deploy", + ConfigPath: primaryPath, + } + // The deploy phase is scoped to the primary union only in the multi-phase + // case (scoped==true means the prereq loaded). Single-phase stays unscoped: + // the union IS its scope, so the renderer's union fallback applies unchanged. + if scoped { + deploy.Secrets = primarySecrets + deploy.Scoped = true + } + phases = append(phases, deploy) + return phases +} +``` + +(d) `deriveWarnings` — add the ambiguity warning: + +```go + // (c) migrations environment ambiguity: ≥2 declared → --env omitted. + if cfg.CI != nil && len(cfg.CI.Migrations) > 0 { + if n := len(cfg.CI.Migrations[0].Environments); n >= 2 { + warnings = append(warnings, + fmt.Sprintf("migrations environment ambiguous (%d declared); --env omitted — set it in the generated workflow", n)) + } + } +``` + +**Step 4: Run tests to verify they pass** + +Run: `GOWORK=off go test ./cigen/ -v` +Expected: PASS (all cigen tests, including the new ones AND the pre-existing golden tests). + +**Step 5: Commit** + +```bash +git add cigen/analyze.go cigen/analyze_test.go cigen/testdata/migrations-one-env.yaml cigen/testdata/migrations-two-envs.yaml +git commit -m "feat(cigen): scope per-phase secrets in Analyze; derive single-env migrations --env" +``` + +--- + +### Task 3: render_gha — per-phase env block + --format json + +**Files:** +- Modify: `cigen/render_gha.go` — `writeApplyJob` (~177-184), `migrationsUpCommand` (~227-233) +- Test: `cigen/render_gha_test.go` (extend) + +**Change class:** generator output (CI workflow content). Verification: render → parse-back YAML + literal-substring asserts. **Rollback: revert the PR; `wfctl ci generate` reverts to union-scoping + bare-migrations output (additive field, JSON omitempty — old plan.json consumers unaffected).** + +**Step 1: Write the failing tests** + +Append to `cigen/render_gha_test.go`: + +```go +func TestRenderGHA_PerPhaseEnvScoping(t *testing.T) { + plan, err := Analyze([]string{"testdata/multisite/deploy.yaml"}, Options{ + PhaseConfig: "testdata/multisite/deploy.prereq.yaml", + }) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + files, err := RenderGitHubActions(plan) + if err != nil { + t.Fatalf("render: %v", err) + } + var yml string + for _, c := range files { + yml = c + } + prereqEnv := jobEnvBlock(t, yml, "apply-prereq") + deployEnv := jobEnvBlock(t, yml, "apply-deploy") + if strings.Contains(prereqEnv, "MULTISITE_DB_URL") { + t.Errorf("apply-prereq env must NOT contain MULTISITE_DB_URL.\nenv:\n%s", prereqEnv) + } + if !strings.Contains(deployEnv, "MULTISITE_DB_URL") { + t.Errorf("apply-deploy env must contain MULTISITE_DB_URL.\nenv:\n%s", deployEnv) + } + // Output still parses as YAML (reuse existing parse-back helper). + assertValidYAML(t, yml) +} + +func TestMigrationsUpCommand_AlwaysFormatJSON(t *testing.T) { + if got := migrationsUpCommand("deploy.yaml", ""); got != "wfctl migrations up --config 'deploy.yaml' --format json" { + t.Errorf("no-env: got %q", got) + } + if got := migrationsUpCommand("deploy.yaml", "prod"); got != "wfctl migrations up --config 'deploy.yaml' --env prod --format json" { + t.Errorf("with-env: got %q", got) + } +} +``` + +Add a `jobEnvBlock` helper to the test file if one does not already exist — extract the lines between ` :` and its ` steps:` and return the `env:` portion. (If the existing test file already has a YAML-parse helper, reuse it to read `jobs..env` instead of string-slicing; check `render_gha_test.go` first and prefer the existing approach. `assertValidYAML` likewise — reuse the existing parse-back assertion already used by the file's tests; if it's named differently, use that name.) + +**Step 2: Run tests to verify they fail** + +Run: `GOWORK=off go test ./cigen/ -run 'TestRenderGHA_PerPhaseEnvScoping|TestMigrationsUpCommand' -v` +Expected: FAIL (apply-prereq currently carries the full union incl MULTISITE_DB_URL; migrations command lacks `--format json`). + +**Step 3: Implement** + +In `cigen/render_gha.go` `writeApplyJob`, replace the secrets env block (currently `if len(p.Secrets) > 0 { … p.Secrets … }`) with: + +```go + // Secrets env block. Branch the SOURCE on phase.Scoped (NOT len): a scoped + // phase uses its own subset (possibly empty → no env block); an unscoped + // phase falls back to the plan-wide union. + secrets := p.Secrets + if phase.Scoped { + secrets = phase.Secrets + } + if len(secrets) > 0 { + b.WriteString(" env:\n") + for _, s := range secrets { + fmt.Fprintf(b, " %s: ${{ secrets.%s }}\n", s.Name, s.Name) + } + } +``` + +And `migrationsUpCommand` — append `--format json` unconditionally (after `--env`): + +```go +// migrationsUpCommand builds the `wfctl migrations up` invocation. `--env ` +// is included only when MigrationsSpec.Env is set; `--format json` is always +// appended (machine-readable output; matches the deployed multisite workflow). +func migrationsUpCommand(configPath, env string) string { + cmd := fmt.Sprintf("wfctl migrations up --config '%s'", configPath) + if env != "" { + cmd += fmt.Sprintf(" --env %s", env) + } + cmd += " --format json" + return cmd +} +``` + +Also update the comment block above the migrations step (render_gha.go ~208-213): the DBEnv is now in the *last phase's* scoped `env:` (still present, since the deploy phase carries the primary union) — keep the "no step-level env needed" rationale but reword "secrets union" → "the last phase's env block". + +**Step 4: Run tests to verify they pass** + +Run: `GOWORK=off go test ./cigen/ -v` +Expected: PASS (all cigen tests). + +**Step 5: Commit** + +```bash +git add cigen/render_gha.go cigen/render_gha_test.go +git commit -m "feat(cigen): per-phase env block + wfctl migrations up --format json" +``` + +--- + +### Task 4: golangci-lint + full-package gate + +**Files:** none (verification task) + +**Change class:** Go-repo code change → lint gate before push. + +**Step 1:** Run the full cigen package (not a `-run` filter — pre-existing golden tests must stay green): + +Run: `GOWORK=off go test ./cigen/...` +Expected: `ok github.com/GoCodeAlone/workflow/cigen` + +**Step 2:** Build wfctl (the consumer surface) to confirm no break: + +Run: `GOWORK=off go build ./cmd/wfctl` +Expected: exit 0. + +**Step 3:** Lint only the changed lines: + +Run: `GOWORK=off golangci-lint run --new-from-rev=origin/main ./cigen/...` +Expected: exit 0. + +**Step 4: Commit** (only if lint auto-fixes or doc tweaks were needed; otherwise skip). + +--- + +### Task 5: Regenerate multisite evidence + honest GAP.md + +**Files:** +- Regenerate: `cigen/testdata/multisite/plan.json`, `cigen/testdata/multisite/generated-infra.yml` +- Modify: `cigen/testdata/multisite/GAP.md` + +**Change class:** generator output artifact (demonstration-fidelity). Verification: real binary regen + measured diff; literal output committed. + +**Step 1:** Find how the committed evidence is generated. Inspect the existing golden/regen test or a `//go:generate` directive: + +Run: `grep -rn "generated-infra.yml\|plan.json\|UPDATE\|golden\|regen" cigen/*_test.go` +Identify the regen mechanism (e.g. an `UPDATE=1 go test` golden-update env, or a small regen helper). If a golden-update path exists, use it. If not, regenerate via the real CLI: + +```bash +cd cigen/testdata/multisite +GOWORK=off go run ../../../cmd/wfctl ci plan --config deploy.yaml --phase-config deploy.prereq.yaml --format json > plan.json +GOWORK=off go run ../../../cmd/wfctl ci generate --config deploy.yaml --phase-config deploy.prereq.yaml --platform github_actions --output-dir . --stdout > generated-infra.yml +``` + +(Verify the exact `ci plan` / `ci generate` flag names with `--help` first — `--phase-config`, `--stdout`, `--format`, and `--output-dir` must match the real command. Adjust to the real flags; do NOT hand-edit the output to fake the shape.) + +**Step 2:** Verify the measured diff (the honest claim): + +Run: `git --no-pager diff cigen/testdata/multisite/generated-infra.yml` +Expected diff content: +- `apply-prereq` `env:` block **no longer contains** `MULTISITE_DB_URL` (and any other deploy-only secrets the prereq does not reference). +- `apply-deploy` `env:` block **still contains** `MULTISITE_DB_URL`. +- The `Run migrations` step changes from `wfctl migrations up --config 'deploy.yaml'` to `wfctl migrations up --config 'deploy.yaml' --format json` — **no `--env`** (multisite declares no `environments:`). + +If the diff does not match these expectations, STOP — the implementation is wrong, not the evidence. Do not edit the committed output to force the expected shape. + +**Step 3:** Update `cigen/testdata/multisite/GAP.md` — HONEST (design C1): +- **Move to "now derivable / matched":** per-phase secret scoping (apply-prereq no longer carries the deploy-only DB secret) + `--format json` on the migrations step. +- **KEEP in "not derivable":** migrations `--env ` (requires an `environments:` block multisite does not declare) + the hash-suffixed DB secret name, GHCR image-wait, GHCR creds, GA4 step, smoke matrix, concurrency, SHA-pins. +- State the claim is exactly the measured diff — never "matches the hand-tuned `--env prod`". + +**Step 4:** Re-run the package to confirm the golden tests accept the regenerated artifacts: + +Run: `GOWORK=off go test ./cigen/...` +Expected: PASS (the multisite golden test now matches the regenerated files). + +**Step 5: Commit** + +```bash +git add cigen/testdata/multisite/plan.json cigen/testdata/multisite/generated-infra.yml cigen/testdata/multisite/GAP.md +git commit -m "test(cigen): regen multisite evidence (scoped prereq env + migrations --format json); honest GAP.md" +``` + +--- + +## Global Design Guidance + +Source: `docs/design-guidance.md` (workspace). Dogfood (improves `wfctl ci generate`); reuse over rebuild (reuses `deriveSecrets`/`MigrationsSpec.Env`/render scaffolding); secrets never logged (scoping tightens blast radius; names only); multi-component/real-consumer proof (multisite real-config regen + measured diff). + +## Verification per change class + +- Task 1: internal logic → unit test (JSON round-trip). +- Task 2: internal logic → unit tests (per-phase scoping, env derivation, ambiguity/unscopable warnings) + pre-existing golden tests stay green. +- Task 3: generator output → render + YAML parse-back + literal-substring asserts. Rollback note inline. +- Task 4: Go-repo gate → full-package `go test`, `go build ./cmd/wfctl`, `golangci-lint run --new-from-rev=origin/main`. +- Task 5: generator artifact (demonstration-fidelity) → real-binary regen + measured diff + honest GAP.md; literal output committed. + +## Rollback + +Revert the PR. `wfctl ci generate` reverts to union-scoping + bare-migrations output. `DeployPhase.{Secrets,Scoped}` are additive (`json:",omitempty"`) — old `plan.json` consumers are unaffected. No release, no migration, no state. From 5d3fc53e278f993880438e45b6a3d2efdfde5d0c Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 31 May 2026 16:32:51 -0400 Subject: [PATCH 2/8] docs(cigen): revise plan per adversarial C1/C2/C3 + Important MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - C1/C2: new tests in internal package cigen files (analyze_phase_test.go, render_gha_phase_test.go) — unexported funcs reachable, own config/strings imports - C3: Task 5 regen uses REAL ci plan/generate flags (--out, --write; no --stdout/--format/--output-dir) per GAP.md recipe - Important: add on-disk golden test (multisite_evidence_test.go) locking the committed evidence - note pre-existing render tests survive (Contains-asserts) Co-Authored-By: Claude Opus 4.8 (1M context) --- ...fidelity-secret-scoping-migration-flags.md | 158 +++++++++++++++--- 1 file changed, 131 insertions(+), 27 deletions(-) diff --git a/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md index 4bec613e..35d9c378 100644 --- a/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md +++ b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md @@ -127,15 +127,26 @@ git commit -m "feat(cigen): add DeployPhase.Secrets + Scoped for per-phase scopi **Files:** - Modify: `cigen/analyze.go` — `Analyze` (per-phase secrets), `derivePhases` (carry secrets/scoped), `deriveMigrations` (populate Env), `deriveWarnings` (ambiguity + unscopable notes) -- Test: `cigen/analyze_test.go` (extend) +- Test: `cigen/analyze_phase_test.go` (NEW — **`package cigen` internal**, NOT `cigen_test`) **Change class:** internal logic (analyze pure function) → unit tests sufficient. +> **Why an internal test file (resolves plan-review C1/C2):** the existing `cigen/analyze_test.go` is `package cigen_test` (external). The new tests call **unexported** funcs (`deriveMigrations`, `deriveSecrets`, `deriveWarnings`) and `config.LoadFromFile`, so they MUST live in a `package cigen` (internal) file — from there the unexported symbols are reachable unqualified and the file declares its own `config`/`strings` imports. Do NOT append these to `analyze_test.go`. + **Step 1: Write the failing tests** -Append to `cigen/analyze_test.go`: +Create `cigen/analyze_phase_test.go`: ```go +package cigen + +import ( + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/config" +) + func TestAnalyze_PerPhaseScoping_PrereqExcludesDeploySecret(t *testing.T) { plan, err := Analyze([]string{"testdata/multisite/deploy.yaml"}, Options{ PhaseConfig: "testdata/multisite/deploy.prereq.yaml", @@ -405,15 +416,46 @@ git commit -m "feat(cigen): scope per-phase secrets in Analyze; derive single-en **Files:** - Modify: `cigen/render_gha.go` — `writeApplyJob` (~177-184), `migrationsUpCommand` (~227-233) -- Test: `cigen/render_gha_test.go` (extend) +- Test: `cigen/render_gha_phase_test.go` (NEW — **`package cigen` internal**) + +**Change class:** generator output (CI workflow content). Verification: render → parse-back YAML (`gopkg.in/yaml.v3`, a direct dep) + literal-substring asserts. **Rollback: revert the PR; `wfctl ci generate` reverts to union-scoping + bare-migrations output (additive field, JSON omitempty — old plan.json consumers unaffected).** -**Change class:** generator output (CI workflow content). Verification: render → parse-back YAML + literal-substring asserts. **Rollback: revert the PR; `wfctl ci generate` reverts to union-scoping + bare-migrations output (additive field, JSON omitempty — old plan.json consumers unaffected).** +> **Why an internal test file (resolves plan-review C1):** `migrationsUpCommand` is unexported. The new test must be `package cigen`. The existing `render_gha_test.go` (`cigen_test`, external) helpers (`richCIPlan`, etc.) are NOT reachable from it — the new file is self-contained (parses YAML with `yaml.Unmarshal`, same lib the external tests use). +> +> **Pre-existing tests survive (verified):** `TestRenderGitHubActions_MigrationsStep` (render_gha_test.go:82) and `_WithEnv` (:128) assert with `strings.Contains(...,"wfctl migrations up --config")` / `"--env prod"` — substring checks that remain true after ` --format json` is appended. `richCIPlan()`-built plans leave `Scoped=false` → union fallback → secret-env tests unchanged. No edits to the existing external tests are required; the Task 4 full-package run confirms it. **Step 1: Write the failing tests** -Append to `cigen/render_gha_test.go`: +Create `cigen/render_gha_phase_test.go`: ```go +package cigen + +import ( + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +// jobEnv returns the rendered `env:` map for a named job, parsed from YAML. +func jobEnv(t *testing.T, yml, job string) map[string]any { + t.Helper() + var doc struct { + Jobs map[string]struct { + Env map[string]any `yaml:"env"` + } `yaml:"jobs"` + } + if err := yaml.Unmarshal([]byte(yml), &doc); err != nil { + t.Fatalf("output is not valid YAML: %v\n%s", err, yml) + } + j, ok := doc.Jobs[job] + if !ok { + t.Fatalf("job %q not found in:\n%s", job, yml) + } + return j.Env +} + func TestRenderGHA_PerPhaseEnvScoping(t *testing.T) { plan, err := Analyze([]string{"testdata/multisite/deploy.yaml"}, Options{ PhaseConfig: "testdata/multisite/deploy.prereq.yaml", @@ -429,16 +471,14 @@ func TestRenderGHA_PerPhaseEnvScoping(t *testing.T) { for _, c := range files { yml = c } - prereqEnv := jobEnvBlock(t, yml, "apply-prereq") - deployEnv := jobEnvBlock(t, yml, "apply-deploy") - if strings.Contains(prereqEnv, "MULTISITE_DB_URL") { - t.Errorf("apply-prereq env must NOT contain MULTISITE_DB_URL.\nenv:\n%s", prereqEnv) + prereqEnv := jobEnv(t, yml, "apply-prereq") // also asserts valid YAML + deployEnv := jobEnv(t, yml, "apply-deploy") + if _, ok := prereqEnv["MULTISITE_DB_URL"]; ok { + t.Errorf("apply-prereq env must NOT contain MULTISITE_DB_URL; got %v", prereqEnv) } - if !strings.Contains(deployEnv, "MULTISITE_DB_URL") { - t.Errorf("apply-deploy env must contain MULTISITE_DB_URL.\nenv:\n%s", deployEnv) + if _, ok := deployEnv["MULTISITE_DB_URL"]; !ok { + t.Errorf("apply-deploy env must contain MULTISITE_DB_URL; got %v", deployEnv) } - // Output still parses as YAML (reuse existing parse-back helper). - assertValidYAML(t, yml) } func TestMigrationsUpCommand_AlwaysFormatJSON(t *testing.T) { @@ -451,8 +491,6 @@ func TestMigrationsUpCommand_AlwaysFormatJSON(t *testing.T) { } ``` -Add a `jobEnvBlock` helper to the test file if one does not already exist — extract the lines between ` :` and its ` steps:` and return the `env:` portion. (If the existing test file already has a YAML-parse helper, reuse it to read `jobs..env` instead of string-slicing; check `render_gha_test.go` first and prefer the existing approach. `assertValidYAML` likewise — reuse the existing parse-back assertion already used by the file's tests; if it's named differently, use that name.) - **Step 2: Run tests to verify they fail** Run: `GOWORK=off go test ./cigen/ -run 'TestRenderGHA_PerPhaseEnvScoping|TestMigrationsUpCommand' -v` @@ -538,23 +576,89 @@ Expected: exit 0. ### Task 5: Regenerate multisite evidence + honest GAP.md **Files:** +- Create: `cigen/multisite_evidence_test.go` (NEW — **`package cigen` internal** on-disk golden test) - Regenerate: `cigen/testdata/multisite/plan.json`, `cigen/testdata/multisite/generated-infra.yml` - Modify: `cigen/testdata/multisite/GAP.md` -**Change class:** generator output artifact (demonstration-fidelity). Verification: real binary regen + measured diff; literal output committed. +**Change class:** generator output artifact (demonstration-fidelity). Verification: real binary regen + measured diff + **on-disk golden test** (resolves plan-review Important: no existing test reads these committed files; this adds one, satisfying the "verify the artifact behaves" mandate); literal output committed. + +**Step 0: Write the on-disk golden test FIRST (fails against the current buggy committed evidence).** + +Create `cigen/multisite_evidence_test.go`: + +```go +package cigen + +import ( + "os" + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +// TestMultisiteEvidence_Honest locks the committed demonstration-fidelity +// artifact. It reads the on-disk generated-infra.yml (NOT a freshly rendered +// plan) and asserts exactly the honest claims in GAP.md: apply-prereq is +// scoped (no deploy-only DB secret), apply-deploy keeps it, and the migrations +// step carries --format json but NOT --env (multisite declares no environments). +func TestMultisiteEvidence_Honest(t *testing.T) { + b, err := os.ReadFile("testdata/multisite/generated-infra.yml") + if err != nil { + t.Fatalf("read committed evidence: %v", err) + } + yml := string(b) + + var doc struct { + Jobs map[string]struct { + Env map[string]any `yaml:"env"` + } `yaml:"jobs"` + } + if err := yaml.Unmarshal(b, &doc); err != nil { + t.Fatalf("committed evidence is not valid YAML: %v", err) + } + if _, ok := doc.Jobs["apply-prereq"].Env["MULTISITE_DB_URL"]; ok { + t.Errorf("apply-prereq must NOT carry the deploy-only MULTISITE_DB_URL (scoping gap #3)") + } + if _, ok := doc.Jobs["apply-deploy"].Env["MULTISITE_DB_URL"]; !ok { + t.Errorf("apply-deploy must carry MULTISITE_DB_URL") + } + if !strings.Contains(yml, "wfctl migrations up --config 'deploy.yaml' --format json") { + t.Errorf("migrations step must emit --format json (gap #4)") + } + if strings.Contains(yml, "migrations up") && strings.Contains(yml, "--env ") { + t.Errorf("multisite declares no ci.migrations.environments → --env must NOT be emitted (honest, design C1)") + } +} +``` -**Step 1:** Find how the committed evidence is generated. Inspect the existing golden/regen test or a `//go:generate` directive: +Run: `GOWORK=off go test ./cigen/ -run TestMultisiteEvidence_Honest -v` +Expected: FAIL (current committed `generated-infra.yml` has the union in apply-prereq incl `MULTISITE_DB_URL`, and a bare `migrations up` with no `--format json`). -Run: `grep -rn "generated-infra.yml\|plan.json\|UPDATE\|golden\|regen" cigen/*_test.go` -Identify the regen mechanism (e.g. an `UPDATE=1 go test` golden-update env, or a small regen helper). If a golden-update path exists, use it. If not, regenerate via the real CLI: +**Step 1: Regenerate the evidence with the REAL CLI flags (the exact recipe documented in `GAP.md` "How this was produced" — resolves plan-review C3).** `ci plan` has no `--format` (output is JSON to `--out`, `-`=stdout); `ci generate` writes files to `--out`/`--output` dir with `--write` (there is NO `--stdout`/`--output-dir` flag). From the repo root of the worktree: ```bash -cd cigen/testdata/multisite -GOWORK=off go run ../../../cmd/wfctl ci plan --config deploy.yaml --phase-config deploy.prereq.yaml --format json > plan.json -GOWORK=off go run ../../../cmd/wfctl ci generate --config deploy.yaml --phase-config deploy.prereq.yaml --platform github_actions --output-dir . --stdout > generated-infra.yml +GOWORK=off go run ./cmd/wfctl ci plan \ + -c cigen/testdata/multisite/deploy.yaml \ + --phase-config cigen/testdata/multisite/deploy.prereq.yaml \ + --config-path-alias deploy.yaml \ + --phase-config-alias deploy.prereq.yaml \ + --out cigen/testdata/multisite/plan.json + +GOWORK=off go run ./cmd/wfctl ci generate \ + -c cigen/testdata/multisite/deploy.yaml \ + --phase-config cigen/testdata/multisite/deploy.prereq.yaml \ + --config-path-alias deploy.yaml \ + --phase-config-alias deploy.prereq.yaml \ + --platform github_actions \ + --out cigen/testdata/multisite \ + --write +# emits cigen/testdata/multisite/.github/workflows/multisite.yml — move it: +mv cigen/testdata/multisite/.github/workflows/multisite.yml cigen/testdata/multisite/generated-infra.yml +rm -rf cigen/testdata/multisite/.github ``` -(Verify the exact `ci plan` / `ci generate` flag names with `--help` first — `--phase-config`, `--stdout`, `--format`, and `--output-dir` must match the real command. Adjust to the real flags; do NOT hand-edit the output to fake the shape.) +(These are the flags verified in `cmd/wfctl/ci_plan.go:13-22` and `cmd/wfctl/ci.go:72-86`. Do NOT hand-edit the output to fake the shape — if the diff is wrong the implementation is wrong.) **Step 2:** Verify the measured diff (the honest claim): @@ -571,16 +675,16 @@ If the diff does not match these expectations, STOP — the implementation is wr - **KEEP in "not derivable":** migrations `--env ` (requires an `environments:` block multisite does not declare) + the hash-suffixed DB secret name, GHCR image-wait, GHCR creds, GA4 step, smoke matrix, concurrency, SHA-pins. - State the claim is exactly the measured diff — never "matches the hand-tuned `--env prod`". -**Step 4:** Re-run the package to confirm the golden tests accept the regenerated artifacts: +**Step 4:** Re-run the package — the on-disk golden test from Step 0 now PASSES against the regenerated evidence, and the whole package stays green: Run: `GOWORK=off go test ./cigen/...` -Expected: PASS (the multisite golden test now matches the regenerated files). +Expected: PASS (incl `TestMultisiteEvidence_Honest`). **Step 5: Commit** ```bash -git add cigen/testdata/multisite/plan.json cigen/testdata/multisite/generated-infra.yml cigen/testdata/multisite/GAP.md -git commit -m "test(cigen): regen multisite evidence (scoped prereq env + migrations --format json); honest GAP.md" +git add cigen/multisite_evidence_test.go cigen/testdata/multisite/plan.json cigen/testdata/multisite/generated-infra.yml cigen/testdata/multisite/GAP.md +git commit -m "test(cigen): regen multisite evidence (scoped prereq env + migrations --format json) + on-disk golden test; honest GAP.md" ``` --- From ef8cc27bee090d071f78ac74973d2d34879c1f93 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 31 May 2026 16:34:30 -0400 Subject: [PATCH 3/8] docs(cigen): plan PASS adversarial cycle-2 (fix Task 2/3 git-add paths to *_phase_test.go) Co-Authored-By: Claude Opus 4.8 (1M context) --- ...026-05-31-cigen-fidelity-secret-scoping-migration-flags.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md index 35d9c378..7f30b8e0 100644 --- a/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md +++ b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md @@ -406,7 +406,7 @@ Expected: PASS (all cigen tests, including the new ones AND the pre-existing gol **Step 5: Commit** ```bash -git add cigen/analyze.go cigen/analyze_test.go cigen/testdata/migrations-one-env.yaml cigen/testdata/migrations-two-envs.yaml +git add cigen/analyze.go cigen/analyze_phase_test.go cigen/testdata/migrations-one-env.yaml cigen/testdata/migrations-two-envs.yaml git commit -m "feat(cigen): scope per-phase secrets in Analyze; derive single-env migrations --env" ``` @@ -542,7 +542,7 @@ Expected: PASS (all cigen tests). **Step 5: Commit** ```bash -git add cigen/render_gha.go cigen/render_gha_test.go +git add cigen/render_gha.go cigen/render_gha_phase_test.go git commit -m "feat(cigen): per-phase env block + wfctl migrations up --format json" ``` From 058f655230897a7505bdd515f488b60da9a0d4d6 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 31 May 2026 16:36:04 -0400 Subject: [PATCH 4/8] chore: lock scope for cigen fidelity (alignment passed) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md | 2 +- ...-cigen-fidelity-secret-scoping-migration-flags.md.scope-lock | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md.scope-lock diff --git a/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md index 7f30b8e0..5187a7bb 100644 --- a/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md +++ b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md @@ -32,7 +32,7 @@ |------|-------|-------|--------| | 1 | feat(cigen): per-phase secret scoping + migration --format json/--env | Task 1, Task 2, Task 3, Task 4, Task 5 | feat/cigen-phase-secret-scoping | -**Status:** Draft +**Status:** Locked 2026-05-31T20:35:52Z --- diff --git a/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md.scope-lock b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md.scope-lock new file mode 100644 index 00000000..469d6216 --- /dev/null +++ b/docs/plans/2026-05-31-cigen-fidelity-secret-scoping-migration-flags.md.scope-lock @@ -0,0 +1 @@ +8971de05ed6574be183523a5388ea1a69a96bd454e752da30ddf35b86723de08 From 34390b71cc7051f6bc67099740c9482f3d37c663 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 31 May 2026 16:38:22 -0400 Subject: [PATCH 5/8] feat(cigen): add DeployPhase.Secrets + Scoped for per-phase scoping --- cigen/plan.go | 8 ++++++++ cigen/plan_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 cigen/plan_test.go diff --git a/cigen/plan.go b/cigen/plan.go index a9779a37..6fc176ec 100644 --- a/cigen/plan.go +++ b/cigen/plan.go @@ -55,6 +55,14 @@ type DeployPhase struct { ConfigPath string `json:"config_path"` // Include is an optional list of module names to include in this phase. Include []string `json:"include,omitempty"` + // Secrets is the set of secrets this phase's apply job needs. Populated + // only when Scoped is true; otherwise the renderer uses CIPlan.Secrets. + Secrets []SecretRef `json:"secrets,omitempty"` + // Scoped is true when per-phase secret derivation ran against a real, + // loaded config for this phase. The renderer branches its env: source on + // this flag — NOT on len(Secrets) — so a genuinely zero-secret scoped + // phase emits no union, while an unscoped phase falls back to the union. + Scoped bool `json:"scoped,omitempty"` } // MigrationsSpec describes the database migration step. diff --git a/cigen/plan_test.go b/cigen/plan_test.go new file mode 100644 index 00000000..f0c087bf --- /dev/null +++ b/cigen/plan_test.go @@ -0,0 +1,31 @@ +package cigen + +import ( + "encoding/json" + "testing" +) + +func TestDeployPhase_ScopedSecretsJSON(t *testing.T) { + p := DeployPhase{ + Name: "prereq", + ConfigPath: "deploy.prereq.yaml", + Secrets: []SecretRef{{Name: "DIGITALOCEAN_TOKEN"}}, + Scoped: true, + } + b, err := json.Marshal(p) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var got DeployPhase + if err := json.Unmarshal(b, &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if !got.Scoped || len(got.Secrets) != 1 || got.Secrets[0].Name != "DIGITALOCEAN_TOKEN" { + t.Fatalf("round-trip lost fields: %+v", got) + } + // Unscoped phase with no secrets must omit both in JSON (additive, back-compat). + b2, _ := json.Marshal(DeployPhase{Name: "deploy", ConfigPath: "deploy.yaml"}) + if string(b2) != `{"name":"deploy","config_path":"deploy.yaml"}` { + t.Fatalf("unexpected JSON for unscoped phase: %s", b2) + } +} From 25c795d7bb08cb7b3ac6acbe6fff464118c8460f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 31 May 2026 16:40:33 -0400 Subject: [PATCH 6/8] feat(cigen): scope per-phase secrets in Analyze; derive single-env migrations --env --- cigen/analyze.go | 50 +++++++++-- cigen/analyze_phase_test.go | 114 ++++++++++++++++++++++++ cigen/testdata/migrations-one-env.yaml | 10 +++ cigen/testdata/migrations-two-envs.yaml | 13 +++ 4 files changed, 182 insertions(+), 5 deletions(-) create mode 100644 cigen/analyze_phase_test.go create mode 100644 cigen/testdata/migrations-one-env.yaml create mode 100644 cigen/testdata/migrations-two-envs.yaml diff --git a/cigen/analyze.go b/cigen/analyze.go index 7ef4490e..651bdefa 100644 --- a/cigen/analyze.go +++ b/cigen/analyze.go @@ -101,15 +101,31 @@ func Analyze(configs []string, opts Options) (*CIPlan, error) { if primaryConfigPath == "" { primaryConfigPath = relativizeConfigPath(primaryPath) } + + // Per-phase secret scoping (multi-phase only). The prereq phase is scoped to + // the secrets ITS config references; the deploy (last) phase keeps the + // primary union (which already includes the migration DBEnv via deriveSecrets). + var prereqSecrets []SecretRef + scoped := false phaseConfigPath := opts.PhaseConfig if phaseConfigPath != "" { if opts.PhaseConfigAlias != "" { phaseConfigPath = opts.PhaseConfigAlias } else { - phaseConfigPath = relativizeConfigPath(phaseConfigPath) + phaseConfigPath = relativizeConfigPath(opts.PhaseConfig) + } + if pcfg, perr := config.LoadFromFile(opts.PhaseConfig); perr == nil { + // deriveMigrations is primary-only: the migrating phase is the LAST + // phase, so the prereq never gets a migration DBEnv (pass nil). + prereqSecrets = deriveSecrets(pcfg, nil) + scoped = true + } else { + plan.Warnings = append(plan.Warnings, + fmt.Sprintf("per-phase secret scoping unavailable: phase config %q not loadable (%v); using union", opts.PhaseConfig, perr)) } } - plan.Phases = derivePhases(primaryConfigPath, phaseConfigPath) + + plan.Phases = derivePhases(primaryConfigPath, phaseConfigPath, plan.Secrets, prereqSecrets, scoped) return plan, nil } @@ -223,6 +239,12 @@ func deriveMigrations(cfg *config.WorkflowConfig) *MigrationsSpec { if spec.DBEnv == "" { return nil } + // Derive --env only when exactly one environment is declared (unambiguous). + if len(m.Environments) == 1 { + for envName := range m.Environments { + spec.Env = envName + } + } return spec } @@ -390,18 +412,28 @@ func extractPrimaryDomain(cfg map[string]any) string { } // derivePhases builds the ordered list of deploy phases. -func derivePhases(primaryPath, phaseConfig string) []DeployPhase { +func derivePhases(primaryPath, phaseConfig string, primarySecrets, prereqSecrets []SecretRef, scoped bool) []DeployPhase { var phases []DeployPhase if phaseConfig != "" { phases = append(phases, DeployPhase{ Name: "prereq", ConfigPath: phaseConfig, + Secrets: prereqSecrets, + Scoped: scoped, }) } - phases = append(phases, DeployPhase{ + deploy := DeployPhase{ Name: "deploy", ConfigPath: primaryPath, - }) + } + // The deploy phase is scoped to the primary union only in the multi-phase + // case (scoped==true means the prereq loaded). Single-phase stays unscoped: + // the union IS its scope, so the renderer's union fallback applies unchanged. + if scoped { + deploy.Secrets = primarySecrets + deploy.Scoped = true + } + phases = append(phases, deploy) return phases } @@ -428,5 +460,13 @@ func deriveWarnings(cfg *config.WorkflowConfig, migrations *MigrationsSpec, secr } } + // (c) migrations environment ambiguity: ≥2 declared → --env omitted. + if cfg.CI != nil && len(cfg.CI.Migrations) > 0 { + if n := len(cfg.CI.Migrations[0].Environments); n >= 2 { + warnings = append(warnings, + fmt.Sprintf("migrations environment ambiguous (%d declared); --env omitted — set it in the generated workflow", n)) + } + } + return warnings } diff --git a/cigen/analyze_phase_test.go b/cigen/analyze_phase_test.go new file mode 100644 index 00000000..dc994026 --- /dev/null +++ b/cigen/analyze_phase_test.go @@ -0,0 +1,114 @@ +package cigen + +import ( + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/config" +) + +func TestAnalyze_PerPhaseScoping_PrereqExcludesDeploySecret(t *testing.T) { + plan, err := Analyze([]string{"testdata/multisite/deploy.yaml"}, Options{ + PhaseConfig: "testdata/multisite/deploy.prereq.yaml", + }) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if len(plan.Phases) != 2 { + t.Fatalf("expected 2 phases, got %d", len(plan.Phases)) + } + prereq, deploy := plan.Phases[0], plan.Phases[1] + if !prereq.Scoped || !deploy.Scoped { + t.Fatalf("expected both phases Scoped, got prereq=%v deploy=%v", prereq.Scoped, deploy.Scoped) + } + if hasSecret(prereq.Secrets, "MULTISITE_DB_URL") { + t.Errorf("prereq phase must NOT carry the deploy-only migration secret MULTISITE_DB_URL; got %v", names(prereq.Secrets)) + } + if !hasSecret(deploy.Secrets, "MULTISITE_DB_URL") { + t.Errorf("deploy (last) phase must carry MULTISITE_DB_URL; got %v", names(deploy.Secrets)) + } + // prereq genuinely needs the provider token — sanity that scoping isn't empty. + if !hasSecret(prereq.Secrets, "DIGITALOCEAN_TOKEN") { + t.Errorf("prereq phase should carry DIGITALOCEAN_TOKEN; got %v", names(prereq.Secrets)) + } +} + +func TestAnalyze_SinglePhase_NotScoped(t *testing.T) { + plan, err := Analyze([]string{"testdata/multisite/deploy.yaml"}, Options{}) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if len(plan.Phases) != 1 { + t.Fatalf("expected 1 phase, got %d", len(plan.Phases)) + } + if plan.Phases[0].Scoped { + t.Errorf("single-phase deploy must not be Scoped (union is its scope)") + } +} + +func TestAnalyze_PhaseConfigAliasOnly_FallsBackToUnion(t *testing.T) { + plan, err := Analyze([]string{"testdata/multisite/deploy.yaml"}, Options{ + PhaseConfig: "/nonexistent/deploy.prereq.yaml", + PhaseConfigAlias: "deploy.prereq.yaml", + }) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if plan.Phases[0].Scoped { + t.Errorf("alias-only/unloadable phase config must fall back (Scoped=false)") + } + if !containsSubstr(plan.Warnings, "per-phase secret scoping unavailable") { + t.Errorf("expected an unscopable warning; got %v", plan.Warnings) + } +} + +func TestDeriveMigrations_SingleEnvDerived(t *testing.T) { + cfg, err := config.LoadFromFile("testdata/migrations-one-env.yaml") + if err != nil { + t.Fatalf("load: %v", err) + } + m := deriveMigrations(cfg) + if m == nil || m.Env != "prod" { + t.Fatalf("expected Env=prod, got %+v", m) + } +} + +func TestDeriveMigrations_TwoEnvsAmbiguous(t *testing.T) { + cfg, err := config.LoadFromFile("testdata/migrations-two-envs.yaml") + if err != nil { + t.Fatalf("load: %v", err) + } + m := deriveMigrations(cfg) + if m == nil || m.Env != "" { + t.Fatalf("expected Env empty (ambiguous), got %+v", m) + } + w := deriveWarnings(cfg, m, deriveSecrets(cfg, m)) + if !containsSubstr(w, "migrations environment ambiguous") { + t.Errorf("expected ambiguity warning; got %v", w) + } +} + +// test helpers +func hasSecret(s []SecretRef, name string) bool { + for _, r := range s { + if r.Name == name { + return true + } + } + return false +} +func names(s []SecretRef) []string { + out := make([]string, 0, len(s)) + for _, r := range s { + out = append(out, r.Name) + } + return out +} +func containsSubstr(ss []string, sub string) bool { + for _, s := range ss { + if strings.Contains(s, sub) { + return true + } + } + return false +} diff --git a/cigen/testdata/migrations-one-env.yaml b/cigen/testdata/migrations-one-env.yaml new file mode 100644 index 00000000..7adc3370 --- /dev/null +++ b/cigen/testdata/migrations-one-env.yaml @@ -0,0 +1,10 @@ +ci: + migrations: + - name: app + source_dir: migrations + database: + env: APP_DB_URL + environments: + prod: + database: + env: APP_DB_URL diff --git a/cigen/testdata/migrations-two-envs.yaml b/cigen/testdata/migrations-two-envs.yaml new file mode 100644 index 00000000..eb95fab3 --- /dev/null +++ b/cigen/testdata/migrations-two-envs.yaml @@ -0,0 +1,13 @@ +ci: + migrations: + - name: app + source_dir: migrations + database: + env: APP_DB_URL + environments: + staging: + database: + env: STAGING_DB_URL + prod: + database: + env: PROD_DB_URL From 77f31fbb15559e1e00217276c16488fe792adc48 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 31 May 2026 16:41:27 -0400 Subject: [PATCH 7/8] feat(cigen): per-phase env block + wfctl migrations up --format json --- cigen/render_gha.go | 23 ++++++----- cigen/render_gha_phase_test.go | 72 ++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 9 deletions(-) create mode 100644 cigen/render_gha_phase_test.go diff --git a/cigen/render_gha.go b/cigen/render_gha.go index d267f046..83073e85 100644 --- a/cigen/render_gha.go +++ b/cigen/render_gha.go @@ -174,11 +174,16 @@ func writeApplyJob(b *strings.Builder, jobName string, phase DeployPhase, needs } fmt.Fprintf(b, " runs-on: %s\n", runner) - // Secrets env block - if len(p.Secrets) > 0 { + // Secrets env block. Branch the SOURCE on phase.Scoped (NOT len): a scoped + // phase uses its own subset (possibly empty → no env block); an unscoped + // phase falls back to the plan-wide union. + secrets := p.Secrets + if phase.Scoped { + secrets = phase.Secrets + } + if len(secrets) > 0 { b.WriteString(" env:\n") - for _, s := range p.Secrets { - // Use ${{ secrets.NAME }} — use raw string to avoid template interpretation + for _, s := range secrets { fmt.Fprintf(b, " %s: ${{ secrets.%s }}\n", s.Name, s.Name) } } @@ -209,8 +214,7 @@ func writeApplyJob(b *strings.Builder, jobName string, phase DeployPhase, needs // the real migration runner — `wfctl ci run --phase migrate` is NOT a valid // phase (ci run only accepts build|test|deploy) and would fail at runtime. // No step-level `env:` is needed: deriveSecrets always adds the migrations - // DBEnv to the secrets union, so it is already in the apply job's job-level - // `env:` block above; re-declaring it here would be redundant. + // DBEnv to the last phase's env block above; re-declaring it here would be redundant. isLastPhase := phase.Name == p.Phases[len(p.Phases)-1].Name if isLastPhase && p.Migrations != nil { b.WriteString(" - name: Run migrations\n") @@ -221,14 +225,15 @@ func writeApplyJob(b *strings.Builder, jobName string, phase DeployPhase, needs fmt.Fprintf(b, " run: wfctl infra apply --config '%s' --auto-approve\n", phase.ConfigPath) } -// migrationsUpCommand builds the `wfctl migrations up` invocation. The deploy -// env is included as `--env ` only when MigrationsSpec.Env is set; -// otherwise it is omitted (the command defaults to the empty environment). +// migrationsUpCommand builds the `wfctl migrations up` invocation. `--env ` +// is included only when MigrationsSpec.Env is set; `--format json` is always +// appended (machine-readable output; matches the deployed multisite workflow). func migrationsUpCommand(configPath, env string) string { cmd := fmt.Sprintf("wfctl migrations up --config '%s'", configPath) if env != "" { cmd += fmt.Sprintf(" --env %s", env) } + cmd += " --format json" return cmd } diff --git a/cigen/render_gha_phase_test.go b/cigen/render_gha_phase_test.go new file mode 100644 index 00000000..43840848 --- /dev/null +++ b/cigen/render_gha_phase_test.go @@ -0,0 +1,72 @@ +package cigen + +import ( + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +// jobEnv returns the rendered `env:` map for a named job, parsed from YAML. +func jobEnv(t *testing.T, yml, job string) map[string]any { + t.Helper() + var doc struct { + Jobs map[string]struct { + Env map[string]any `yaml:"env"` + } `yaml:"jobs"` + } + if err := yaml.Unmarshal([]byte(yml), &doc); err != nil { + t.Fatalf("output is not valid YAML: %v\n%s", err, yml) + } + j, ok := doc.Jobs[job] + if !ok { + t.Fatalf("job %q not found in:\n%s", job, yml) + } + return j.Env +} + +func TestRenderGHA_PerPhaseEnvScoping(t *testing.T) { + plan, err := Analyze([]string{"testdata/multisite/deploy.yaml"}, Options{ + PhaseConfig: "testdata/multisite/deploy.prereq.yaml", + }) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + files, err := RenderGitHubActions(plan) + if err != nil { + t.Fatalf("render: %v", err) + } + var yml string + for _, c := range files { + yml = c + } + prereqEnv := jobEnv(t, yml, "apply-prereq") // also asserts valid YAML + deployEnv := jobEnv(t, yml, "apply-deploy") + if _, ok := prereqEnv["MULTISITE_DB_URL"]; ok { + t.Errorf("apply-prereq env must NOT contain MULTISITE_DB_URL; got %v", prereqEnv) + } + if _, ok := deployEnv["MULTISITE_DB_URL"]; !ok { + t.Errorf("apply-deploy env must contain MULTISITE_DB_URL; got %v", deployEnv) + } +} + +func TestMigrationsUpCommand_AlwaysFormatJSON(t *testing.T) { + if got := migrationsUpCommand("deploy.yaml", ""); got != "wfctl migrations up --config 'deploy.yaml' --format json" { + t.Errorf("no-env: got %q", got) + } + if got := migrationsUpCommand("deploy.yaml", "prod"); got != "wfctl migrations up --config 'deploy.yaml' --env prod --format json" { + t.Errorf("with-env: got %q", got) + } +} + +// Ensure the _existing_ migration substring assertions still hold after adding --format json. +func TestMigrationsUpCommand_SubstringCompat(t *testing.T) { + cmd := migrationsUpCommand("deploy.yaml", "") + if !strings.Contains(cmd, "wfctl migrations up --config") { + t.Errorf("expected wfctl migrations up --config substring; got %q", cmd) + } + cmd2 := migrationsUpCommand("deploy.yaml", "prod") + if !strings.Contains(cmd2, "--env prod") { + t.Errorf("expected --env prod substring; got %q", cmd2) + } +} From 9f0909a7d14db81a860eb0c4e5ac193e5ca633b2 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 31 May 2026 16:44:42 -0400 Subject: [PATCH 8/8] test(cigen): regen multisite evidence (scoped prereq env + migrations --format json) + on-disk golden test; honest GAP.md --- cigen/multisite_evidence_test.go | 43 ++++++ cigen/testdata/multisite/GAP.md | 136 +++++++++---------- cigen/testdata/multisite/generated-infra.yml | 8 +- cigen/testdata/multisite/plan.json | 88 +++++++++++- 4 files changed, 194 insertions(+), 81 deletions(-) create mode 100644 cigen/multisite_evidence_test.go diff --git a/cigen/multisite_evidence_test.go b/cigen/multisite_evidence_test.go new file mode 100644 index 00000000..bbaafcfb --- /dev/null +++ b/cigen/multisite_evidence_test.go @@ -0,0 +1,43 @@ +package cigen + +import ( + "os" + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +// TestMultisiteEvidence_Honest locks the committed demonstration-fidelity +// artifact. It reads the on-disk generated-infra.yml (NOT a freshly rendered +// plan) and asserts exactly the honest claims in GAP.md: apply-prereq is +// scoped (no deploy-only DB secret), apply-deploy keeps it, and the migrations +// step carries --format json but NOT --env (multisite declares no environments). +func TestMultisiteEvidence_Honest(t *testing.T) { + b, err := os.ReadFile("testdata/multisite/generated-infra.yml") + if err != nil { + t.Fatalf("read committed evidence: %v", err) + } + yml := string(b) + + var doc struct { + Jobs map[string]struct { + Env map[string]any `yaml:"env"` + } `yaml:"jobs"` + } + if err := yaml.Unmarshal(b, &doc); err != nil { + t.Fatalf("committed evidence is not valid YAML: %v", err) + } + if _, ok := doc.Jobs["apply-prereq"].Env["MULTISITE_DB_URL"]; ok { + t.Errorf("apply-prereq must NOT carry the deploy-only MULTISITE_DB_URL (scoping gap #3)") + } + if _, ok := doc.Jobs["apply-deploy"].Env["MULTISITE_DB_URL"]; !ok { + t.Errorf("apply-deploy must carry MULTISITE_DB_URL") + } + if !strings.Contains(yml, "wfctl migrations up --config 'deploy.yaml' --format json") { + t.Errorf("migrations step must emit --format json (gap #4)") + } + if strings.Contains(yml, "migrations up") && strings.Contains(yml, "--env ") { + t.Errorf("multisite declares no ci.migrations.environments → --env must NOT be emitted (honest, design C1)") + } +} diff --git a/cigen/testdata/multisite/GAP.md b/cigen/testdata/multisite/GAP.md index 4b45a079..e36c0c3b 100644 --- a/cigen/testdata/multisite/GAP.md +++ b/cigen/testdata/multisite/GAP.md @@ -15,7 +15,7 @@ paths instead of the testdata-relative paths the binary would otherwise emit ```sh # 1. Real ci plan -wfctl-pr4 ci plan \ +GOWORK=off go run ./cmd/wfctl ci plan \ -c cigen/testdata/multisite/deploy.yaml \ --phase-config cigen/testdata/multisite/deploy.prereq.yaml \ --config-path-alias deploy.yaml \ @@ -23,7 +23,7 @@ wfctl-pr4 ci plan \ --out cigen/testdata/multisite/plan.json # 2. Real ci generate -wfctl-pr4 ci generate \ +GOWORK=off go run ./cmd/wfctl ci generate \ -c cigen/testdata/multisite/deploy.yaml \ --phase-config cigen/testdata/multisite/deploy.prereq.yaml \ --config-path-alias deploy.yaml \ @@ -33,9 +33,11 @@ wfctl-pr4 ci generate \ --write # output: cigen/testdata/multisite/.github/workflows/multisite.yml # renamed to: cigen/testdata/multisite/generated-infra.yml +mv cigen/testdata/multisite/.github/workflows/multisite.yml cigen/testdata/multisite/generated-infra.yml +rm -rf cigen/testdata/multisite/.github -# 3. Real diff -diff -u .../infra.yml cigen/testdata/multisite/generated-infra.yml > /tmp/multisite.diff +# 3. Measured diff +git --no-pager diff cigen/testdata/multisite/generated-infra.yml ``` ## Real plan.json warnings[] array @@ -48,14 +50,23 @@ diff -u .../infra.yml cigen/testdata/multisite/generated-infra.yml > /tmp/multis ] ``` -## Real diff summary +## Real diff summary (this PR) -- Hand-written `infra.yml`: 283 lines -- Generated `generated-infra.yml`: 134 lines -- Diff: 248 lines removed, 99 lines added (384-line diff total) +The measured `git diff cigen/testdata/multisite/generated-infra.yml` shows: -Nearly all lines differ. The generated file is correct in structure and -logically sound, but is much simpler than the production hand-written file. +- `apply-prereq` `env:` block **no longer contains** `MULTISITE_DB_URL`, + `GOOGLE_ANALYTICS_ADMIN_CREDENTIALS_JSON`, `MULTISITE_AUDIT_SIGN_KEY`, + `MULTISITE_INGEST_HMAC_SECRET`, `MULTISITE_JWT_SECRET`, + `MULTISITE_SUPER_ADMIN_BOOTSTRAP_CODE` — only the 10 secrets actually + referenced by `deploy.prereq.yaml` are emitted (per-phase scoping fix #3). +- `apply-deploy` `env:` block is unchanged — it still carries all 16 secrets + from the primary config union, including `MULTISITE_DB_URL`. +- The `Run migrations` step changes from: + `wfctl migrations up --config 'deploy.yaml'` + to: + `wfctl migrations up --config 'deploy.yaml' --format json` + — **no `--env`** flag (multisite declares no `ci.migrations.environments`, + so `--env` is not derivable; this is honest per design C1). --- @@ -76,21 +87,18 @@ intent of the hand-written workflow, as confirmed by the diff: + grep pattern that refuses destructive changes is present in both `apply-prereq` and `apply-deploy`. Derived from `protected: true` on both `multisite-pg` and `gocodealone-multisite` modules. -- **apply-job `env:` block with 1:1 secret name mapping**: all 16 secrets - from the union of both configs are present in the `env:` block on each - apply job using `${{ secrets.NAME }}` syntax. NOTE: this union is GLOBAL - across phases — see "Known limitations: global-union secret over-exposure". +- **Per-phase scoped `env:` block (FIXED #3)**: `apply-prereq` carries only + the 10 secrets referenced by `deploy.prereq.yaml`; `apply-deploy` carries + the full 16-secret union from the primary config. The deploy-only secrets + (`MULTISITE_DB_URL`, `MULTISITE_AUDIT_SIGN_KEY`, etc.) no longer appear in + the prereq job's env block. - **Two-phase plugin install in plan job**: `Install plugins (prereq)` and `Install plugins (deploy)` steps both present. -- **Migrations step in apply-deploy** (FUNCTIONAL — fixed): `Run migrations` - step runs `wfctl migrations up --config 'deploy.yaml'`, the REAL migration - runner, derived from `ci.migrations`. This is the same subcommand the - hand-written workflow uses (`wfctl migrations up --config deploy.yaml --env - prod --format json`); the generated form omits the operational `--env prod - --format json` flags (see "NOT derivable" below). A previous version of the - generator emitted `wfctl ci run --config ... --phase migrate`, which is NOT a - valid `ci run` phase (only build|test|deploy) and would have failed at - runtime; that defect is fixed. +- **Migrations step in apply-deploy with `--format json` (FIXED #4)**: + `Run migrations` step runs `wfctl migrations up --config 'deploy.yaml' + --format json`, derived from `ci.migrations`. The `--format json` flag is + now always appended (machine-readable output). `--env` is correctly omitted + because `deploy.yaml` declares no `ci.migrations[0].environments` entries. - **Smoke job**: derived from `infra.container_service` with `health_check.http_path=/healthz` and `domain: gocodealone.tech`. URL correctly computed as `https://gocodealone.tech/healthz`. @@ -223,7 +231,7 @@ The generator emits plan steps without `continue-on-error`. The hand-written workflow passes `DIGITALOCEAN_TOKEN`, `RELEASES_TOKEN`, `SPACES_access_key`, `SPACES_secret_key` as per-step env on the plan steps -and apply-prereq step (not as job-level env). The generator places the full +and apply-prereq step (not as job-level env). The generator places the scoped secret set in the job-level `env:` block instead, which technically works but differs in structure. This is a correctness delta, not just style. @@ -277,79 +285,63 @@ The hand-written workflow is named `Infrastructure` with global env vars The generator derives the name from the config basename (`multisite`) and emits no global env block. -### 14. Migration `--env prod --format json` flags +### 14. Migration `--env ` flag + +The generated migrations step correctly omits `--env`: -The generated migrations step is now functional and uses the correct -subcommand: ```yaml -run: wfctl migrations up --config 'deploy.yaml' +run: wfctl migrations up --config 'deploy.yaml' --format json ``` -The hand-written form adds operational flags: +The hand-written form adds `--env prod`: ```yaml run: wfctl migrations up --config deploy.yaml --env prod --format json ``` -The `--env prod` flag selects a deploy-environment-scoped migration config; -the `deploy.yaml` config has a single top-level `ci.migrations` block with no -named environments, so the generator has no environment name to emit. (The -generator WILL emit `--env ` when `MigrationsSpec.Env` is set.) `--format -json` is a presentation choice. Neither is derivable from the base config. - ---- - -## Known limitations (this PR's scope) - -### Global-union secret over-exposure across phases - -`deriveSecrets` computes a single GLOBAL union of every secret referenced by -either config, and `RenderGitHubActions` emits that same full union as the -job-level `env:` block on EVERY apply job. As a consequence, the generated -`apply-prereq` job carries secrets it does not use — for example -`MULTISITE_DB_URL` appears in `apply-prereq`'s `env:` even though the prereq -phase runs no migrations and never reads that secret. - -This is honest over-exposure: it does not break the pipeline (the extra -secrets are simply unused in that job), but it widens the blast radius of each -job beyond the minimum it needs. The hand-written `infra.yml` scopes secrets -more tightly (per-step `env:` on the steps that actually use them — see item 8 -above). Per-phase / per-step secret scoping in the generator is future work -and is explicitly NOT in this PR's scope. +The `deploy.yaml` config has a single top-level `ci.migrations` block with no +`environments:` entries, so the generator has no environment name to derive. +The generator WILL emit `--env ` when exactly one +`ci.migrations[0].environments` key is declared (unambiguous single-env case). +When ≥2 environments are declared, `--env` is omitted and a warning is emitted. +For multisite specifically, no `environments:` block exists → `--env` stays +not-derivable (honest; design C1). --- ## What the generator got WRONG (not just incomplete) -None remaining at command level. Both previously-documented defects are FIXED: +None remaining at command level. All previously-documented defects are FIXED: - **Migration step command (FIXED)**: the generator previously emitted `wfctl ci run --config ... --phase migrate`, but `wfctl ci run` only accepts the phases `build|test|deploy` and errors on anything else (`unknown phase: "migrate"`) — the generated step would have failed at runtime. It now emits - `wfctl migrations up --config ''`, the real migration runner. The - DB-url secret is already wired into the apply job's `env:` block via the - secrets union, so the command can read it. -- **paths: filter (FIXED in this artifact)**: the binary uses the `--config` + `wfctl migrations up --config ''`, the real migration runner. +- **Migration `--format json` (FIXED #4)**: the migrations step now always + appends `--format json` for machine-readable output. +- **Per-phase secret over-exposure (FIXED #3)**: `apply-prereq` no longer + carries deploy-only secrets. The scoped env block is derived from the prereq + config's actual secret references, not the full union. +- **paths: filter (FIXED in prior artifact)**: the binary uses the `--config` argument verbatim as the CI trigger `paths:` filter, so running it from the workflow repo root with `cigen/testdata/multisite/deploy.yaml` would emit a - testdata-relative path. This artifact now uses `--config-path-alias deploy.yaml` + testdata-relative path. This artifact uses `--config-path-alias deploy.yaml` / `--phase-config-alias deploy.prereq.yaml` to produce clean repo-relative - paths. Operators generating CI for a real project should either run from the - project root (so `-c deploy.yaml` is already clean) or pass the alias flags. + paths. --- ## Verdict `wfctl ci generate` correctly derives the two-phase plan/apply structure, -plan-guard, secret inventory (by config name), smoke URL, a FUNCTIONAL -migrations step (`wfctl migrations up`), plugin install, and trigger shape. -It correctly warns about the DB hash-suffix and SPACES casing gaps. It does -NOT derive: hash-suffixed secret references, image wait loops, GHCR credential -derivation, phase-selector dispatch inputs, action SHA pinning, apply pipeline -steps, concurrency guards, per-step env scoping, multi-route smoke matrix, the -`always()+skipped` dependency condition, or the migration `--env prod --format -json` operational flags. It also over-exposes secrets via a global union on -every apply job (see "Known limitations"). The generator is a useful starting -scaffold; the 14+ hand-authored additions are each justified by runtime or +plan-guard, per-phase scoped secret inventory, `--format json` on migrations, +smoke URL, a FUNCTIONAL migrations step (`wfctl migrations up`), plugin install, +and trigger shape. It correctly warns about the DB hash-suffix and SPACES casing +gaps. It does NOT derive: hash-suffixed secret references, image wait loops, +GHCR credential derivation, phase-selector dispatch inputs, action SHA pinning, +apply pipeline steps, concurrency guards, per-step env scoping, multi-route +smoke matrix, the `always()+skipped` dependency condition, or the migration +`--env prod` operational flag (multisite declares no `environments:`, so none +is derivable — honest per design C1). The generator is a useful starting +scaffold; the 13+ hand-authored additions are each justified by runtime or operational concerns not encodable in the workflow config format alone. diff --git a/cigen/testdata/multisite/generated-infra.yml b/cigen/testdata/multisite/generated-infra.yml index 8652aa1f..b77e85e6 100644 --- a/cigen/testdata/multisite/generated-infra.yml +++ b/cigen/testdata/multisite/generated-infra.yml @@ -49,17 +49,11 @@ jobs: env: DIGITALOCEAN_TOKEN: ${{ secrets.DIGITALOCEAN_TOKEN }} GHCR_CREDENTIALS: ${{ secrets.GHCR_CREDENTIALS }} - GOOGLE_ANALYTICS_ADMIN_CREDENTIALS_JSON: ${{ secrets.GOOGLE_ANALYTICS_ADMIN_CREDENTIALS_JSON }} - MULTISITE_AUDIT_SIGN_KEY: ${{ secrets.MULTISITE_AUDIT_SIGN_KEY }} MULTISITE_CONTENT_REPO_TOKEN: ${{ secrets.MULTISITE_CONTENT_REPO_TOKEN }} - MULTISITE_DB_URL: ${{ secrets.MULTISITE_DB_URL }} - MULTISITE_INGEST_HMAC_SECRET: ${{ secrets.MULTISITE_INGEST_HMAC_SECRET }} - MULTISITE_JWT_SECRET: ${{ secrets.MULTISITE_JWT_SECRET }} MULTISITE_OAUTH_FACEBOOK_APP_ID: ${{ secrets.MULTISITE_OAUTH_FACEBOOK_APP_ID }} MULTISITE_OAUTH_FACEBOOK_APP_SECRET: ${{ secrets.MULTISITE_OAUTH_FACEBOOK_APP_SECRET }} MULTISITE_OAUTH_GOOGLE_CLIENT_ID: ${{ secrets.MULTISITE_OAUTH_GOOGLE_CLIENT_ID }} MULTISITE_OAUTH_GOOGLE_CLIENT_SECRET: ${{ secrets.MULTISITE_OAUTH_GOOGLE_CLIENT_SECRET }} - MULTISITE_SUPER_ADMIN_BOOTSTRAP_CODE: ${{ secrets.MULTISITE_SUPER_ADMIN_BOOTSTRAP_CODE }} RELEASES_TOKEN: ${{ secrets.RELEASES_TOKEN }} SPACES_access_key: ${{ secrets.SPACES_access_key }} SPACES_secret_key: ${{ secrets.SPACES_secret_key }} @@ -119,7 +113,7 @@ jobs: exit 1 fi - name: Run migrations - run: wfctl migrations up --config 'deploy.yaml' + run: wfctl migrations up --config 'deploy.yaml' --format json - name: Apply deploy run: wfctl infra apply --config 'deploy.yaml' --auto-approve smoke: diff --git a/cigen/testdata/multisite/plan.json b/cigen/testdata/multisite/plan.json index 16903a4f..59322dce 100644 --- a/cigen/testdata/multisite/plan.json +++ b/cigen/testdata/multisite/plan.json @@ -57,11 +57,95 @@ "phases": [ { "name": "prereq", - "config_path": "deploy.prereq.yaml" + "config_path": "deploy.prereq.yaml", + "secrets": [ + { + "name": "DIGITALOCEAN_TOKEN" + }, + { + "name": "GHCR_CREDENTIALS" + }, + { + "name": "MULTISITE_CONTENT_REPO_TOKEN" + }, + { + "name": "MULTISITE_OAUTH_FACEBOOK_APP_ID" + }, + { + "name": "MULTISITE_OAUTH_FACEBOOK_APP_SECRET" + }, + { + "name": "MULTISITE_OAUTH_GOOGLE_CLIENT_ID" + }, + { + "name": "MULTISITE_OAUTH_GOOGLE_CLIENT_SECRET" + }, + { + "name": "RELEASES_TOKEN" + }, + { + "name": "SPACES_access_key" + }, + { + "name": "SPACES_secret_key" + } + ], + "scoped": true }, { "name": "deploy", - "config_path": "deploy.yaml" + "config_path": "deploy.yaml", + "secrets": [ + { + "name": "DIGITALOCEAN_TOKEN" + }, + { + "name": "GHCR_CREDENTIALS" + }, + { + "name": "GOOGLE_ANALYTICS_ADMIN_CREDENTIALS_JSON" + }, + { + "name": "MULTISITE_AUDIT_SIGN_KEY" + }, + { + "name": "MULTISITE_CONTENT_REPO_TOKEN" + }, + { + "name": "MULTISITE_DB_URL" + }, + { + "name": "MULTISITE_INGEST_HMAC_SECRET" + }, + { + "name": "MULTISITE_JWT_SECRET" + }, + { + "name": "MULTISITE_OAUTH_FACEBOOK_APP_ID" + }, + { + "name": "MULTISITE_OAUTH_FACEBOOK_APP_SECRET" + }, + { + "name": "MULTISITE_OAUTH_GOOGLE_CLIENT_ID" + }, + { + "name": "MULTISITE_OAUTH_GOOGLE_CLIENT_SECRET" + }, + { + "name": "MULTISITE_SUPER_ADMIN_BOOTSTRAP_CODE" + }, + { + "name": "RELEASES_TOKEN" + }, + { + "name": "SPACES_access_key" + }, + { + "name": "SPACES_secret_key" + } + ], + "scoped": true } ], "migrations": {