From 3e254b83773d81e1ac4619c3fb3de7bf6a76fe24 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 14:22:54 +0200 Subject: [PATCH 01/13] feat(env): safety tiers + plan output for env update (#125 phase 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the user-consent and visibility layer that #125 calls out: - Per-env safety field: strict | prompt (default) | auto - Plan-style output for every sync: + add / ~ update / = keep / ! overwrite / ⊕ merge / ✗ conflict + summary line - --safety CLI override - --plan-json for machine-readable plan output (PR comment bots, CI summary jobs) - --dry-run always emits a plan and never writes - --yes / -y skips the prompt confirmation in interactive mode Safety semantics: - auto: applies without prompting (legacy default behavior) - prompt (NEW DEFAULT): TTY → asks Apply? [y/N]; non-TTY → falls back to strict (refuses any destructive plan unless --yes) - strict: refuses any plan that contains overwrite or conflict rows, with or without TTY A plan row is "destructive" when it would lose user-owned content (overwrite or conflict). BREAKING CHANGE for non-TTY consumers ===================================== The default safety changed from "always apply" to "prompt → strict on CI". CI pipelines that previously ran `b update` and got silent auto-application of overwrites/conflicts will now need one of: - add `safety: auto` per-env in b.yaml - pass `--safety=auto` on the command line - pass `--yes` / `-y` for one-off runs - pass `--dry-run` to preview without applying This is the consent footgun #125 was opened to fix. The breaking change is intentional and the migration is mechanical (one flag or one yaml line). Performance =========== The plan + apply flow only doubles SyncEnv calls when the safety mode might block (strict, prompt-with-confirm). For SafetyAuto and --yes, the CLI runs SyncEnv exactly once and renders the plan from the apply result. Auto users pay zero overhead vs. the old behavior. Documented in docs/env-sync.mdx. What's NEW ========== - pkg/env/plan.go: PlanAction enum, Plan / PlanRow structs, PlanFromResult mapping from lock.LockFile statuses, RenderPlanText + RenderPlanJSON renderers, IsDestructive / HasDestructive helpers - pkg/env/plan_test.go: status mapping matrix, destructive detection, deterministic ordering, text + JSON renderers - pkg/state/types.go: Safety field on EnvEntry + envEntryRaw, Safety constants, NormalizeSafety(s) - pkg/state/resolve.go: profile composition propagates Safety - pkg/cli/update.go: gateApply / confirmApply helpers, plan-first vs auto-fast-path branching, --safety / --plan-json / --yes flags, plan rendering before apply, lock-write only on real apply - pkg/cli/update_test.go: 8 new tests pinning auto/strict/prompt/ yes/plan-json/dry-run behaviors and SyncEnv call counts - docs/env-sync.mdx: new "Safety tiers" section + plan output example + performance note + plan-json mention in dry-run What's UNCHANGED ================ - Lock format - SyncEnv signature (still accepts DryRun bool) - Existing strategy semantics (replace / client / merge) - Per-file conflict resolution UI for replace strategy on TTY Out of scope (deferred to phase 2-4 of #125): - b.pin: per-key annotations - skipped-delete tracking - in-place YAML conflict markers - b env resolve subcommand Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/env-sync.mdx | 60 +++++++++ pkg/cli/env_test.go | 24 ++-- pkg/cli/update.go | 222 +++++++++++++++++++++++++------ pkg/cli/update_test.go | 295 ++++++++++++++++++++++++++++++++++++++++- pkg/env/plan.go | 202 ++++++++++++++++++++++++++++ pkg/env/plan_test.go | 167 +++++++++++++++++++++++ pkg/state/resolve.go | 3 + pkg/state/types.go | 33 +++++ 8 files changed, 949 insertions(+), 57 deletions(-) create mode 100644 pkg/env/plan.go create mode 100644 pkg/env/plan_test.go diff --git a/docs/env-sync.mdx b/docs/env-sync.mdx index 7192029..77ff7a8 100644 --- a/docs/env-sync.mdx +++ b/docs/env-sync.mdx @@ -287,6 +287,12 @@ b env remove --delete-files github.com/org/infra b update --dry-run ``` +`--dry-run` always prints the plan and never writes. Combine with `--plan-json` to emit a machine-readable plan for PR comment bots and CI summary jobs: + +```bash +b update --plan-json +``` + ### Rollback ```bash @@ -296,6 +302,60 @@ b update --rollback github.com/org/infra --- +## Safety tiers + +Every `b update` produces a **plan** before applying — a per-file table of additions, updates, keeps, overwrites, merges, and conflicts. The `safety` setting controls what `b` does with that plan: + +| Mode | Interactive (TTY) | Non-interactive (CI) | Use when | +|------------|--------------------------------|-----------------------------------|-----------------------------------------| +| `strict` | Refuses any destructive plan | Refuses any destructive plan | Locked-down repos, untrusted upstreams | +| `prompt`* | Shows plan, asks `[y/N]` | Refuses destructive (= strict) | Default — safe everywhere | +| `auto` | Applies without prompting | Applies without prompting | Trusted upstream, fully automated runs | + +\* `prompt` is the default. It is the safe option both interactively and in CI. + +**A plan row is "destructive" when it would lose user-owned content** — currently that means `overwrite` (a non-merge strategy is about to clobber locally modified files) or `conflict` (a 3-way merge produced markers and the file needs manual resolution). + +Configure per-env in `b.yaml`: + +```yaml +envs: + github.com/locked-down/repo: + safety: strict # CI fails on any overwrite or conflict + github.com/trusted/upstream: + safety: auto # apply silently, no prompts + github.com/normal/repo: + # safety omitted → defaults to "prompt" +``` + +Or override on the command line: + +```bash +b update --safety=auto # apply everything without prompting +b update --safety=strict # fail loudly on any destructive change +b update -y # one-time --yes: skip the prompt for this run +b update --plan-json # emit JSON plan, never apply +``` + +### Plan output + +``` + github.com/org/infra abc1234 → def4567 + + add hetzner/control-plane.yaml + ~ update hetzner/load-balancer.yaml + = keep hetzner/firewall.yaml (local changes preserved) + ! overwrite hetzner/legacy.yaml + ⊕ merge hetzner/network.yaml + ✗ conflict hetzner/secrets.yaml + → 1 add, 1 update, 1 keep, 1 overwrite, 1 merge, 1 conflict +``` + +### Performance note + +When `safety` is `auto` (or `--yes` is set), `b` runs `SyncEnv` exactly **once**: it applies and renders the plan from the apply result. When `safety` is `strict` or `prompt`, `b` runs `SyncEnv` twice — once in dry-run to compute the plan for the gate, then once in real apply mode if approved. The second pass benefits from a hot git cache, but it does still re-read and re-merge files. If you need every cycle, use `safety: auto` for fully-automated runs. + +--- + ## Groups Tag envs for selective syncing: diff --git a/pkg/cli/env_test.go b/pkg/cli/env_test.go index f049913..0c456f4 100644 --- a/pkg/cli/env_test.go +++ b/pkg/cli/env_test.go @@ -490,8 +490,12 @@ func TestUpdateEnvs_DryRun_SkipsLockWrite(t *testing.T) { t.Errorf("expected 0 envs in lock after dry-run, got %d", len(lk.Envs)) } - if !strings.Contains(out.String(), "dry-run") { - t.Errorf("output should contain 'dry-run', got: %q", out.String()) + // New plan-based output: dry-run produces a plan summary line ending + // in "→ N add, ..." rather than a literal "dry-run" tag in the + // header. The lock-not-written assertion above is the load-bearing + // behavior contract. + if !strings.Contains(out.String(), "add,") { + t.Errorf("output should contain plan summary, got: %q", out.String()) } } @@ -527,7 +531,7 @@ func TestUpdateEnvs_GroupFilter(t *testing.T) { shared.loadedConfigPath = filepath.Join(tmpDir, "b.yaml") shared.bVersion = "v1.0" - o := &UpdateOptions{SharedOptions: shared, Group: "dev"} + o := &UpdateOptions{SharedOptions: shared, Group: "dev", Yes: true} if err := o.updateEnvs(nil); err != nil { t.Fatalf("updateEnvs error: %v", err) } @@ -580,7 +584,7 @@ func TestUpdateEnvs_Rollback(t *testing.T) { shared.loadedConfigPath = filepath.Join(tmpDir, "b.yaml") shared.bVersion = "v1.0" - o := &UpdateOptions{SharedOptions: shared, Rollback: true} + o := &UpdateOptions{SharedOptions: shared, Rollback: true, Yes: true} if err := o.updateEnvs(nil); err != nil { t.Fatalf("updateEnvs error: %v", err) } @@ -588,9 +592,10 @@ func TestUpdateEnvs_Rollback(t *testing.T) { if forcedCommit != "previous456" { t.Errorf("ForceCommit = %q, want %q", forcedCommit, "previous456") } - if !strings.Contains(out.String(), "rollback") { - t.Errorf("output should contain 'rollback', got: %q", out.String()) - } + // Plan output no longer carries a literal "(rollback)" tag — the + // behavior contract is that ForceCommit got set to the previous + // commit, which is asserted above. + _ = out } func TestUpdateEnvs_Rollback_NoPrevious(t *testing.T) { @@ -663,7 +668,10 @@ func TestUpdateEnvs_ListConflictedFiles(t *testing.T) { shared.loadedConfigPath = filepath.Join(tmpDir, "b.yaml") shared.bVersion = "v1.0" - o := &UpdateOptions{SharedOptions: shared} + // Yes:true short-circuits the safety prompt so the conflict listing + // path is exercised. Without --yes the non-TTY default safety + // (prompt → strict on non-TTY) would refuse to apply. + o := &UpdateOptions{SharedOptions: shared, Yes: true} o.updateEnvs(nil) errStr := errOut.String() diff --git a/pkg/cli/update.go b/pkg/cli/update.go index b5b6d14..c3d2483 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -20,6 +20,7 @@ import ( "github.com/fentas/b/pkg/env" "github.com/fentas/b/pkg/gitcache" "github.com/fentas/b/pkg/lock" + "github.com/fentas/b/pkg/state" ) // Test hooks — production code uses the defaults; tests can override. @@ -40,7 +41,10 @@ type UpdateOptions struct { specifiedBinaries []*binary.Binary // resolved binaries from CLI args specifiedEnvRefs []string // resolved env refs from CLI args Strategy string // strategy flag override: replace, client, merge + Safety string // safety flag override: strict, prompt, auto DryRun bool // show what would change without writing + PlanJSON bool // emit machine-readable plan and exit (implies dry-run) + Yes bool // skip prompt confirmations (treat prompt as auto) Rollback bool // rollback to previous commit from lock Group string // only update envs in this group stdinReader io.Reader // overridden by tests; nil means os.Stdin @@ -89,7 +93,10 @@ func NewUpdateCmd(shared *SharedOptions) *cobra.Command { } cmd.Flags().StringVar(&o.Strategy, "strategy", "", "Conflict strategy: replace (default), client, merge") + cmd.Flags().StringVar(&o.Safety, "safety", "", "Override per-env safety: strict, prompt, auto") cmd.Flags().BoolVar(&o.DryRun, "dry-run", false, "Show what would change without writing files") + cmd.Flags().BoolVar(&o.PlanJSON, "plan-json", false, "Emit a machine-readable plan as JSON (implies --dry-run)") + cmd.Flags().BoolVarP(&o.Yes, "yes", "y", false, "Skip prompt confirmation (treat prompt as auto)") cmd.Flags().BoolVar(&o.Rollback, "rollback", false, "Rollback envs to previous commit from lock") cmd.Flags().StringVar(&o.Group, "group", "", "Only update envs in this group") @@ -255,6 +262,24 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { strategy = o.Strategy } + // Determine safety: CLI flag > config > default (prompt). Defaulted + // via state.NormalizeSafety so unknown values fall back safely. + safety := state.NormalizeSafety(entry.Safety) + if o.Safety != "" { + safety = state.NormalizeSafety(o.Safety) + } + // --plan-json implies --dry-run; the user only wants the plan. + isDryRun := o.DryRun || o.PlanJSON + + // We only need a dry-run "plan" pass when the safety mode might + // reject the apply (strict, or prompt where the user has to be + // asked). For SafetyAuto and for explicit --yes we can go straight + // to the real apply and render the plan from its result — no + // double work, no second clone-cache hit. + needsPlanFirst := isDryRun || + safety == state.SafetyStrict || + (safety == state.SafetyPrompt && !o.Yes) + cfg := env.EnvConfig{ Ref: ref, Label: label, @@ -263,15 +288,18 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { Ignore: entry.Ignore, Strategy: strategy, Files: entry.Files, - DryRun: o.DryRun, + DryRun: needsPlanFirst, OnPreSync: entry.OnPreSync, OnPostSync: entry.OnPostSync, Stdout: o.IO.Out, Stderr: o.IO.ErrOut, } - - // Set up interactive conflict resolver for replace strategy on TTY - if (strategy == "" || strategy == env.StrategyReplace) && isTTYFunc() && !o.DryRun { + // Attach the interactive conflict resolver only when this very + // pass will actually apply to disk (auto / --yes path). The + // resolver is interactive — running it during a dry-run plan + // pass would prompt the user before they've even approved the + // plan. The plan-first path sets it on the second pass instead. + if !needsPlanFirst && (strategy == "" || strategy == env.StrategyReplace) && isTTYFunc() { cfg.ResolveConflict = o.interactiveConflictResolver(ref, lk) } @@ -286,67 +314,177 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { cfg.ForceCommit = lockEntry.PreviousCommit } - result, err := syncEnvFunc(cfg, projectRoot, "", lockEntry) + // First pass — dry-run when we need a plan to gate on, real + // apply when safety is auto / --yes. + firstResult, err := syncEnvFunc(cfg, projectRoot, "", lockEntry) if err != nil { fmt.Fprintf(o.IO.ErrOut, " %-40s ✗ %s\n", entry.Key, firstLine(err.Error())) continue } - if result.Skipped { - fmt.Fprintf(o.IO.Out, " %-40s %s\n", entry.Key, result.Message) + if firstResult.Skipped { + fmt.Fprintf(o.IO.Out, " %-40s %s\n", entry.Key, firstResult.Message) continue // don't overwrite lock entry when up-to-date } - modes := []string{} - if o.DryRun { - modes = append(modes, "dry-run") - } - if o.Rollback { - modes = append(modes, "rollback") + plan := env.PlanFromResult(firstResult) + + // --plan-json: emit JSON and skip everything else for this env. + if o.PlanJSON { + if err := env.RenderPlanJSON(o.IO.Out, plan); err != nil { + fmt.Fprintf(o.IO.ErrOut, " %-40s ✗ %s\n", entry.Key, err) + } + continue } - prefix := "" - if len(modes) > 0 { - prefix = fmt.Sprintf("(%s) ", strings.Join(modes, ", ")) + + // Header line + plan table. + fmt.Fprintf(o.IO.Out, " %-40s %s → %s\n", entry.Key, + shortCommit(firstResult.PreviousCommit), shortCommit(firstResult.Commit)) + env.RenderPlanText(o.IO.Out, plan) + + // If the first pass was already a real apply (auto / --yes path), + // just write the lock and move on. No gate, no second SyncEnv. + if !needsPlanFirst { + if firstResult.Conflicts > 0 { + printConflictHint(o.IO.ErrOut, firstResult, projectRoot) + } + lk.UpsertEnv(lock.EnvEntry{ + Ref: firstResult.Ref, + Label: firstResult.Label, + Version: firstResult.Version, + Commit: firstResult.Commit, + PreviousCommit: firstResult.PreviousCommit, + Files: firstResult.Files, + }) + continue } - fmt.Fprintf(o.IO.Out, " %s%-40s %s → %s (%s)\n", prefix, entry.Key, shortCommit(result.PreviousCommit), shortCommit(result.Commit), result.Message) - for _, f := range result.Files { - o.printFileStatus(f) + // Plan-first path: gate on safety, then apply if approved. + apply, gateErr := o.gateApply(safety, plan, isDryRun) + if gateErr != nil { + fmt.Fprintf(o.IO.ErrOut, " %-40s ✗ %s\n", entry.Key, gateErr) + continue + } + if !apply { + // Dry-run, strict refusal, or user declined. + continue } - if result.Conflicts > 0 { - fmt.Fprintf(o.IO.ErrOut, " ⚠ %d file(s) have merge conflicts — resolve manually:\n", result.Conflicts) - for _, f := range result.Files { - if f.Status == "conflict" || f.Status == "conflict (dry-run)" { - destPath := f.Dest - if !filepath.IsAbs(destPath) { - destPath = filepath.Join(projectRoot, destPath) - } - fmt.Fprintf(o.IO.ErrOut, " %s\n", destPath) - } - } + // Second pass: real apply. The gitcache is hot from the first + // pass, so only the actual file writes hit disk newly. + applyCfg := cfg + applyCfg.DryRun = false + if (strategy == "" || strategy == env.StrategyReplace) && isTTYFunc() { + applyCfg.ResolveConflict = o.interactiveConflictResolver(ref, lk) + } + realResult, err := syncEnvFunc(applyCfg, projectRoot, "", lockEntry) + if err != nil { + fmt.Fprintf(o.IO.ErrOut, " %-40s ✗ %s\n", entry.Key, firstLine(err.Error())) + continue } - // Don't update lock in dry-run mode - if !o.DryRun { - lk.UpsertEnv(lock.EnvEntry{ - Ref: result.Ref, - Label: result.Label, - Version: result.Version, - Commit: result.Commit, - PreviousCommit: result.PreviousCommit, - Files: result.Files, - }) + if realResult.Conflicts > 0 { + printConflictHint(o.IO.ErrOut, realResult, projectRoot) } + + lk.UpsertEnv(lock.EnvEntry{ + Ref: realResult.Ref, + Label: realResult.Label, + Version: realResult.Version, + Commit: realResult.Commit, + PreviousCommit: realResult.PreviousCommit, + Files: realResult.Files, + }) } - if o.DryRun { - return nil // don't write lock in dry-run mode + if o.DryRun || o.PlanJSON { + return nil // don't write lock in dry-run / plan-only mode } return lock.WriteLock(lockDir, lk, o.bVersion) } +// printConflictHint emits the legacy "needs manual resolution" footer for +// any files that came back with conflict status. +func printConflictHint(w io.Writer, result *env.SyncResult, projectRoot string) { + fmt.Fprintf(w, " ⚠ %d file(s) have merge conflicts — resolve manually:\n", result.Conflicts) + for _, f := range result.Files { + status := strings.TrimSuffix(f.Status, " (dry-run)") + if status == "conflict" { + destPath := f.Dest + if !filepath.IsAbs(destPath) { + destPath = filepath.Join(projectRoot, destPath) + } + fmt.Fprintf(w, " %s\n", destPath) + } + } +} + +// gateApply implements the #125 safety + plan flow. It returns +// (applyNow, error). applyNow=false means "do not run the real sync for +// this env, but the loop should continue". +func (o *UpdateOptions) gateApply(safety string, plan *env.Plan, isDryRun bool) (bool, error) { + // Dry-run is the simplest case: never apply, never error. The plan + // has already been printed for the user. + if isDryRun { + return false, nil + } + + destructive := plan.HasDestructive() + + switch safety { + case state.SafetyAuto: + // Trust the upstream and apply silently. + return true, nil + + case state.SafetyStrict: + // Refuse if any destructive row is present. + if destructive { + return false, fmt.Errorf("strict safety: plan contains destructive changes — use --safety=prompt or --safety=auto to apply") + } + return true, nil + + case state.SafetyPrompt: + // --yes overrides the prompt and behaves like auto. + if o.Yes { + return true, nil + } + // On non-TTY, prompt collapses to strict — refuse on destructive. + if !isTTYFunc() { + if destructive { + return false, fmt.Errorf("prompt safety on non-TTY: plan contains destructive changes — re-run with --yes, --safety=auto, or --dry-run") + } + return true, nil + } + // Interactive prompt. + return o.confirmApply(destructive), nil + } + + // Unknown safety value (shouldn't happen — NormalizeSafety covers it, + // but be defensive). + return false, fmt.Errorf("unknown safety value %q", safety) +} + +// confirmApply prompts the user with a y/N question. Default is N (safer). +func (o *UpdateOptions) confirmApply(destructive bool) bool { + r := o.stdinReader + if r == nil { + r = os.Stdin + } + reader := bufio.NewReader(r) + if destructive { + fmt.Fprint(o.IO.ErrOut, " Plan contains destructive changes. Apply? [y/N] ") + } else { + fmt.Fprint(o.IO.ErrOut, " Apply plan? [y/N] ") + } + input, err := reader.ReadString('\n') + if err != nil { + return false + } + input = strings.TrimSpace(strings.ToLower(input)) + return input == "y" || input == "yes" +} + // printFileStatus prints a single file's sync status. func (o *UpdateOptions) printFileStatus(f lock.LockFile) { // Strip dry-run suffix for display matching diff --git a/pkg/cli/update_test.go b/pkg/cli/update_test.go index d99bb9a..81b72f5 100644 --- a/pkg/cli/update_test.go +++ b/pkg/cli/update_test.go @@ -658,20 +658,20 @@ func TestUpdateEnvs_Updated(t *testing.T) { Envs: state.EnvList{{Key: "github.com/org/repo"}}, }, }, + Yes: true, // skip the #125 safety prompt so apply runs in tests } if err := o.updateEnvs(nil); err != nil { t.Fatalf("updateEnvs error = %v", err) } out := outBuf.String() - if !strings.Contains(out, "2 files synced") { - t.Errorf("expected sync message, got: %s", out) + // Plan-format output: "replaced" status maps to action "update", + // "kept" maps to action "keep". Assert the new action labels. + if !strings.Contains(out, "update") { + t.Errorf("expected plan 'update' line, got: %s", out) } - if !strings.Contains(out, "replaced") { - t.Errorf("expected replaced file status, got: %s", out) - } - if !strings.Contains(out, "kept") { - t.Errorf("expected kept file status, got: %s", out) + if !strings.Contains(out, "keep") { + t.Errorf("expected plan 'keep' line, got: %s", out) } // Verify lock was written with the new entry @@ -713,6 +713,10 @@ func TestUpdateEnvs_WithConflicts(t *testing.T) { Envs: state.EnvList{{Key: "github.com/org/repo"}}, }, }, + // Yes:true bypasses the new #125 safety prompt so the conflict + // warning path is reached. Without it, non-TTY default safety + // (prompt → strict on non-TTY) refuses to apply destructive plans. + Yes: true, } if err := o.updateEnvs(nil); err != nil { @@ -820,6 +824,12 @@ func TestUpdateEnvs_TTYConflictResolver(t *testing.T) { }, }, stdinReader: strings.NewReader("r\n"), // mock stdin + // Yes:true makes the first SyncEnv pass the real apply pass, + // so the conflict resolver is attached to the cfg the test + // inspects. Without --yes, the resolver is set on the second + // (apply) pass, which never runs because Skipped:true short- + // circuits the loop above the gate. + Yes: true, } o.updateEnvs(nil) @@ -1692,3 +1702,274 @@ func TestPrintFileStatus_AllStatuses(t *testing.T) { } } } + +// --- Issue #125: safety tiers + plan output --- +// +// These tests cover the new gateApply / plan flow added in #125. They +// pin the public contract: +// +// - default safety is "prompt"; on a non-TTY (CI) it collapses to +// strict and refuses destructive plans +// - --safety=auto applies without prompting (legacy behavior) +// - --safety=strict refuses ANY destructive plan, with or without TTY +// - --yes overrides the prompt and behaves like auto +// - --plan-json emits one JSON document per env and skips writes +// - the dry-run pass never updates the lock and always renders a plan +// - the auto / --yes paths only call SyncEnv ONCE — the plan-first +// paths call it twice (one dry-run for the plan + one apply) + +// makeUpdateOpts is a small helper that wires up an UpdateOptions with +// in-memory streams and a single env. Returns the option struct, an +// in-memory out buffer, and an in-memory err buffer. +func makeUpdateOpts(t *testing.T, entry state.EnvEntry, opts ...func(*UpdateOptions)) (*UpdateOptions, *bytes.Buffer, *bytes.Buffer) { + t.Helper() + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "b.yaml") + if err := os.WriteFile(configPath, []byte("binaries: {}\n"), 0644); err != nil { + t.Fatal(err) + } + if err := lock.WriteLock(tmpDir, &lock.Lock{Version: 1}, "test"); err != nil { + t.Fatal(err) + } + out := &bytes.Buffer{} + errBuf := &bytes.Buffer{} + o := &UpdateOptions{ + SharedOptions: &SharedOptions{ + IO: &streams.IO{Out: out, ErrOut: errBuf}, + ConfigPath: configPath, + loadedConfigPath: configPath, + Config: &state.State{Envs: state.EnvList{&entry}}, + }, + } + for _, opt := range opts { + opt(o) + } + return o, out, errBuf +} + +// makeSyncFunc returns a syncEnvFunc stub that returns a fixed result and +// records every call. The recorded slice lets tests assert how many +// passes ran (1 for auto/--yes, 2 for plan-first). +func makeSyncFunc(result *env.SyncResult) (func(env.EnvConfig, string, string, *lock.EnvEntry) (*env.SyncResult, error), *[]bool) { + calls := &[]bool{} + return func(cfg env.EnvConfig, _ string, _ string, _ *lock.EnvEntry) (*env.SyncResult, error) { + *calls = append(*calls, cfg.DryRun) + // Return a copy so callers can mutate Files between checks. + clone := *result + return &clone, nil + }, calls +} + +// TestUpdateEnvs_SafetyAuto_AppliesAndCallsSyncOnce: when safety=auto the +// CLI should run SyncEnv exactly once (real apply pass) and write the +// lock. No dry-run preview pass. +func TestUpdateEnvs_SafetyAuto_AppliesAndCallsSyncOnce(t *testing.T) { + saveHooks(t) + isTTYFunc = func() bool { return false } + syncFn, calls := makeSyncFunc(&env.SyncResult{ + Ref: "github.com/org/repo", + Commit: "abc", + Files: []lock.LockFile{{Path: "a.yaml", Dest: "a.yaml", Status: "replaced"}}, + }) + syncEnvFunc = syncFn + + o, _, _ := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo", Safety: state.SafetyAuto}) + if err := o.updateEnvs(nil); err != nil { + t.Fatalf("updateEnvs: %v", err) + } + if len(*calls) != 1 { + t.Errorf("expected 1 SyncEnv call (auto path), got %d", len(*calls)) + } + if (*calls)[0] != false { + t.Error("expected the single call to be a real apply (DryRun=false)") + } +} + +// TestUpdateEnvs_SafetyStrict_RefusesDestructive: strict refuses any +// plan that would overwrite local changes or produce a conflict. The +// lock must NOT be updated. +func TestUpdateEnvs_SafetyStrict_RefusesDestructive(t *testing.T) { + saveHooks(t) + isTTYFunc = func() bool { return false } + syncFn, calls := makeSyncFunc(&env.SyncResult{ + Ref: "github.com/org/repo", + Commit: "abc", + Files: []lock.LockFile{ + {Path: "a.yaml", Dest: "a.yaml", Status: "replaced (local changes overwritten)"}, + }, + }) + syncEnvFunc = syncFn + + o, _, errBuf := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo", Safety: state.SafetyStrict}) + if err := o.updateEnvs(nil); err != nil { + t.Fatalf("updateEnvs: %v", err) + } + // Strict refusal: the plan-first dry-run runs (1 call), gate + // rejects, no second apply call. + if len(*calls) != 1 { + t.Errorf("strict refusal should call SyncEnv once (dry-run only), got %d", len(*calls)) + } + if !strings.Contains(errBuf.String(), "strict safety") { + t.Errorf("expected strict refusal message, got: %q", errBuf.String()) + } +} + +// TestUpdateEnvs_SafetyStrict_AppliesNonDestructive: strict ALLOWS plans +// that have only safe actions (add, update, keep, merge). +func TestUpdateEnvs_SafetyStrict_AppliesNonDestructive(t *testing.T) { + saveHooks(t) + isTTYFunc = func() bool { return false } + syncFn, calls := makeSyncFunc(&env.SyncResult{ + Ref: "github.com/org/repo", + Commit: "abc", + Files: []lock.LockFile{ + {Path: "a.yaml", Dest: "a.yaml", Status: "replaced"}, // → update + }, + }) + syncEnvFunc = syncFn + + o, _, errBuf := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo", Safety: state.SafetyStrict}) + if err := o.updateEnvs(nil); err != nil { + t.Fatalf("updateEnvs: %v", err) + } + // Plan-first path: 1 dry-run + 1 apply = 2 calls. + if len(*calls) != 2 { + t.Errorf("strict + non-destructive should call SyncEnv twice (plan + apply), got %d", len(*calls)) + } + if (*calls)[0] != true { + t.Error("first call should be dry-run") + } + if (*calls)[1] != false { + t.Error("second call should be real apply") + } + if strings.Contains(errBuf.String(), "strict safety") { + t.Errorf("strict should not refuse non-destructive plans, got: %q", errBuf.String()) + } +} + +// TestUpdateEnvs_PromptDefault_NonTTY_BlocksDestructive: the new default +// (prompt) collapses to strict on non-TTY, so CI runs that hit a +// destructive plan must be refused unless --yes / --safety=auto is set. +func TestUpdateEnvs_PromptDefault_NonTTY_BlocksDestructive(t *testing.T) { + saveHooks(t) + isTTYFunc = func() bool { return false } + syncFn, calls := makeSyncFunc(&env.SyncResult{ + Ref: "github.com/org/repo", + Commit: "abc", + Files: []lock.LockFile{ + {Path: "a.yaml", Dest: "a.yaml", Status: "conflict"}, + }, + }) + syncEnvFunc = syncFn + + o, _, errBuf := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo"}) // no safety set → default prompt + if err := o.updateEnvs(nil); err != nil { + t.Fatalf("updateEnvs: %v", err) + } + if len(*calls) != 1 { + t.Errorf("non-TTY prompt should refuse without applying, got %d calls", len(*calls)) + } + if !strings.Contains(errBuf.String(), "non-TTY") { + t.Errorf("expected non-TTY refusal message, got: %q", errBuf.String()) + } +} + +// TestUpdateEnvs_Yes_OverridesPrompt: --yes is the CI escape hatch — it +// makes the default prompt safety apply without confirming. +func TestUpdateEnvs_Yes_OverridesPrompt(t *testing.T) { + saveHooks(t) + isTTYFunc = func() bool { return false } + syncFn, calls := makeSyncFunc(&env.SyncResult{ + Ref: "github.com/org/repo", + Files: []lock.LockFile{{Path: "a.yaml", Dest: "a.yaml", Status: "conflict"}}, + }) + syncEnvFunc = syncFn + + o, _, _ := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo"}, + func(o *UpdateOptions) { o.Yes = true }) + if err := o.updateEnvs(nil); err != nil { + t.Fatalf("updateEnvs: %v", err) + } + // --yes takes the auto path: 1 SyncEnv call, real apply, no + // dry-run pass. + if len(*calls) != 1 { + t.Errorf("--yes auto path should call SyncEnv once, got %d", len(*calls)) + } + if (*calls)[0] != false { + t.Error("--yes call should be real apply") + } +} + +// TestUpdateEnvs_PlanJSON_EmitsAndSkipsApply: --plan-json prints a JSON +// plan and never applies. +func TestUpdateEnvs_PlanJSON_EmitsAndSkipsApply(t *testing.T) { + saveHooks(t) + syncFn, calls := makeSyncFunc(&env.SyncResult{ + Ref: "github.com/org/repo", + Commit: "abc", + Files: []lock.LockFile{ + {Path: "a.yaml", Dest: "a.yaml", Status: "replaced"}, + }, + }) + syncEnvFunc = syncFn + + o, out, _ := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo"}, + func(o *UpdateOptions) { o.PlanJSON = true }) + if err := o.updateEnvs(nil); err != nil { + t.Fatalf("updateEnvs: %v", err) + } + if len(*calls) != 1 { + t.Errorf("plan-json should call SyncEnv once (dry-run), got %d", len(*calls)) + } + if (*calls)[0] != true { + t.Error("plan-json call should be dry-run") + } + if !strings.Contains(out.String(), `"action"`) { + t.Errorf("plan-json output should contain JSON 'action' field, got:\n%s", out.String()) + } + if !strings.Contains(out.String(), `"github.com/org/repo"`) { + t.Errorf("plan-json output should contain ref, got:\n%s", out.String()) + } +} + +// TestUpdateEnvs_DryRun_SkipsLockWriteAndApply: --dry-run prints the +// plan and never applies. The lock must remain at its original version. +func TestUpdateEnvs_DryRun_NewBehavior(t *testing.T) { + saveHooks(t) + syncFn, calls := makeSyncFunc(&env.SyncResult{ + Ref: "github.com/org/repo", + Commit: "abc", + Files: []lock.LockFile{ + {Path: "a.yaml", Dest: "a.yaml", Status: "replaced (dry-run)"}, + }, + }) + syncEnvFunc = syncFn + + o, out, _ := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo"}, + func(o *UpdateOptions) { o.DryRun = true }) + if err := o.updateEnvs(nil); err != nil { + t.Fatalf("updateEnvs: %v", err) + } + if len(*calls) != 1 { + t.Errorf("dry-run should call SyncEnv once, got %d", len(*calls)) + } + if !strings.Contains(out.String(), "add,") { + t.Errorf("dry-run plan summary missing, got:\n%s", out.String()) + } +} + +// TestNormalizeSafety covers the centralized safety-value coercion. +func TestNormalizeSafety_DefaultsToPrompt(t *testing.T) { + cases := map[string]string{ + "": state.SafetyPrompt, + "strict": state.SafetyStrict, + "prompt": state.SafetyPrompt, + "auto": state.SafetyAuto, + "nonsense": state.SafetyPrompt, // unknown → safe default + } + for in, want := range cases { + if got := state.NormalizeSafety(in); got != want { + t.Errorf("NormalizeSafety(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/pkg/env/plan.go b/pkg/env/plan.go new file mode 100644 index 0000000..54a69ab --- /dev/null +++ b/pkg/env/plan.go @@ -0,0 +1,202 @@ +package env + +import ( + "encoding/json" + "fmt" + "io" + "sort" + "strings" + + "github.com/fentas/b/pkg/lock" +) + +// PlanAction is the canonical, user-facing classification of what a sync +// will do (or did) to a single file. The set is small so it can be +// rendered uniformly in tables, JSON, and CI summaries. +type PlanAction string + +const ( + PlanAdd PlanAction = "add" // file is new in upstream and will be created locally + PlanUpdate PlanAction = "update" // file exists, content will change + PlanKeep PlanAction = "keep" // file unchanged (or local-only changes preserved by client strategy) + PlanOverwrite PlanAction = "overwrite" // local changes will be replaced by upstream + PlanMerge PlanAction = "merge" // 3-way merge applied (or would be), no conflict + PlanConflict PlanAction = "conflict" // 3-way merge produced conflict markers; needs manual resolution +) + +// IsDestructive reports whether an action would lose user-owned content. +// Strict safety mode refuses to apply a plan whose actions are destructive. +func (a PlanAction) IsDestructive() bool { + switch a { + case PlanOverwrite, PlanConflict: + return true + default: + return false + } +} + +// PlanRow is a single row in a sync plan: one file, one action, plus the +// metadata needed to render it. +type PlanRow struct { + Action PlanAction `json:"action"` + Source string `json:"source"` // path inside the upstream repo + Dest string `json:"dest"` // path on the consumer's disk (relative to projectRoot) + // Note string is rendered alongside the row when non-empty. Currently + // used to surface inline messages like "(merge failed: ...)". + Note string `json:"note,omitempty"` +} + +// Plan is the full per-env plan. +type Plan struct { + Ref string `json:"ref"` + Label string `json:"label,omitempty"` + Version string `json:"version,omitempty"` + Commit string `json:"commit,omitempty"` + Rows []PlanRow `json:"rows"` +} + +// HasDestructive reports whether the plan contains any destructive row. +func (p *Plan) HasDestructive() bool { + for _, r := range p.Rows { + if r.Action.IsDestructive() { + return true + } + } + return false +} + +// CountByAction returns a map[action]count over the plan's rows. Useful +// for summary lines like "12 add, 3 update, 1 conflict". +func (p *Plan) CountByAction() map[PlanAction]int { + out := make(map[PlanAction]int, 6) + for _, r := range p.Rows { + out[r.Action]++ + } + return out +} + +// PlanFromResult builds a Plan from a SyncResult. The result must come +// from a SyncEnv invocation (dry-run or real); the lock files' Status +// field is the ground truth and is mapped to a PlanAction. +// +// The mapping intentionally collapses the dry-run-suffixed statuses +// ("replaced (dry-run)", "kept (dry-run)", etc.) so the renderer doesn't +// have to special-case them. +func PlanFromResult(result *SyncResult) *Plan { + if result == nil { + return &Plan{} + } + p := &Plan{ + Ref: result.Ref, + Label: result.Label, + Version: result.Version, + Commit: result.Commit, + } + for _, f := range result.Files { + p.Rows = append(p.Rows, planRowFromLockFile(f)) + } + // Stable, deterministic ordering by destination path so callers can + // snapshot-test the rendering. + sort.Slice(p.Rows, func(i, j int) bool { + return p.Rows[i].Dest < p.Rows[j].Dest + }) + return p +} + +// planRowFromLockFile maps a lock.LockFile (which carries a free-form +// status string) into a structured PlanRow. +func planRowFromLockFile(f lock.LockFile) PlanRow { + status := strings.TrimSuffix(f.Status, " (dry-run)") + row := PlanRow{ + Source: f.Path, + Dest: f.Dest, + } + switch { + case status == "unchanged": + row.Action = PlanKeep + case status == "kept": + row.Action = PlanKeep + row.Note = "local changes preserved" + case status == "merged": + row.Action = PlanMerge + case status == "conflict": + row.Action = PlanConflict + case strings.Contains(status, "local changes overwritten"): + row.Action = PlanOverwrite + case strings.HasPrefix(status, "replaced (merge failed"): + row.Action = PlanOverwrite + // Extract the parenthetical reason for the user. + row.Note = strings.TrimSuffix(strings.TrimPrefix(status, "replaced "), "") + case status == "replaced": + // "replaced" alone (no "local changes overwritten") means the + // file was new or identical-on-disk; treat as add/update rather + // than destructive. + row.Action = PlanUpdate + default: + row.Action = PlanUpdate + row.Note = status + } + return row +} + +// RenderPlanText writes a human-readable plan table to w. +// +// Format (one line per row): +// +// - add path/to/file +// ~ update path/to/file +// = keep path/to/file (local changes preserved) +// ! overwrite path/to/file +// ⊕ merge path/to/file +// ✗ conflict path/to/file +// +// A summary line is printed at the end: +// +// → 12 add, 3 update, 0 keep, 1 overwrite, 0 merge, 0 conflict +func RenderPlanText(w io.Writer, p *Plan) { + if p == nil || len(p.Rows) == 0 { + fmt.Fprintln(w, " (no files)") + return + } + for _, r := range p.Rows { + marker, label := planMarker(r.Action) + if r.Note != "" { + fmt.Fprintf(w, " %s %-9s %-40s (%s)\n", marker, label, r.Dest, r.Note) + } else { + fmt.Fprintf(w, " %s %-9s %s\n", marker, label, r.Dest) + } + } + counts := p.CountByAction() + fmt.Fprintf(w, " → %d add, %d update, %d keep, %d overwrite, %d merge, %d conflict\n", + counts[PlanAdd], counts[PlanUpdate], counts[PlanKeep], + counts[PlanOverwrite], counts[PlanMerge], counts[PlanConflict]) +} + +// RenderPlanJSON writes the plan as JSON for machine consumers (PR +// comment bots, CI summary jobs, etc). +func RenderPlanJSON(w io.Writer, p *Plan) error { + enc := json.NewEncoder(w) + enc.SetIndent("", " ") + return enc.Encode(p) +} + +// planMarker returns the (glyph, label) pair for a plan action. The glyph +// is single-width to keep the table aligned in the common case. +func planMarker(a PlanAction) (string, string) { + switch a { + case PlanAdd: + return "+", "add" + case PlanUpdate: + return "~", "update" + case PlanKeep: + return "=", "keep" + case PlanOverwrite: + return "!", "overwrite" + case PlanMerge: + return "⊕", "merge" + case PlanConflict: + return "✗", "conflict" + default: + return "?", string(a) + } +} diff --git a/pkg/env/plan_test.go b/pkg/env/plan_test.go new file mode 100644 index 0000000..a44cd96 --- /dev/null +++ b/pkg/env/plan_test.go @@ -0,0 +1,167 @@ +package env + +import ( + "bytes" + "encoding/json" + "strings" + "testing" + + "github.com/fentas/b/pkg/lock" +) + +func TestPlanAction_IsDestructive(t *testing.T) { + cases := map[PlanAction]bool{ + PlanAdd: false, + PlanUpdate: false, + PlanKeep: false, + PlanMerge: false, + PlanOverwrite: true, + PlanConflict: true, + } + for a, want := range cases { + if got := a.IsDestructive(); got != want { + t.Errorf("%q.IsDestructive() = %v, want %v", a, got, want) + } + } +} + +func TestPlanFromResult_StatusMapping(t *testing.T) { + result := &SyncResult{ + Ref: "github.com/org/repo", + Files: []lock.LockFile{ + {Path: "a.yaml", Dest: "a.yaml", Status: "replaced"}, + {Path: "b.yaml", Dest: "b.yaml", Status: "kept"}, + {Path: "c.yaml", Dest: "c.yaml", Status: "merged"}, + {Path: "d.yaml", Dest: "d.yaml", Status: "conflict"}, + {Path: "e.yaml", Dest: "e.yaml", Status: "replaced (local changes overwritten)"}, + {Path: "f.yaml", Dest: "f.yaml", Status: "unchanged"}, + {Path: "g.yaml", Dest: "g.yaml", Status: "replaced (dry-run)"}, + }, + } + plan := PlanFromResult(result) + if len(plan.Rows) != 7 { + t.Fatalf("rows = %d, want 7", len(plan.Rows)) + } + want := map[string]PlanAction{ + "a.yaml": PlanUpdate, + "b.yaml": PlanKeep, + "c.yaml": PlanMerge, + "d.yaml": PlanConflict, + "e.yaml": PlanOverwrite, + "f.yaml": PlanKeep, + "g.yaml": PlanUpdate, // dry-run suffix stripped + } + for _, r := range plan.Rows { + if want[r.Dest] != r.Action { + t.Errorf("%s: action = %q, want %q", r.Dest, r.Action, want[r.Dest]) + } + } +} + +func TestPlanFromResult_DestructiveAndCounts(t *testing.T) { + result := &SyncResult{ + Files: []lock.LockFile{ + {Dest: "a", Status: "replaced"}, + {Dest: "b", Status: "replaced (local changes overwritten)"}, + {Dest: "c", Status: "kept"}, + {Dest: "d", Status: "conflict"}, + }, + } + p := PlanFromResult(result) + if !p.HasDestructive() { + t.Error("plan with overwrite + conflict should be destructive") + } + c := p.CountByAction() + if c[PlanUpdate] != 1 || c[PlanOverwrite] != 1 || c[PlanKeep] != 1 || c[PlanConflict] != 1 { + t.Errorf("counts = %v", c) + } +} + +func TestPlanFromResult_NonDestructive(t *testing.T) { + result := &SyncResult{ + Files: []lock.LockFile{ + {Dest: "a", Status: "replaced"}, + {Dest: "b", Status: "kept"}, + }, + } + if PlanFromResult(result).HasDestructive() { + t.Error("non-destructive plan flagged as destructive") + } +} + +func TestRenderPlanText_Empty(t *testing.T) { + var buf bytes.Buffer + RenderPlanText(&buf, &Plan{}) + if !strings.Contains(buf.String(), "(no files)") { + t.Errorf("empty plan should say (no files), got: %q", buf.String()) + } +} + +func TestRenderPlanText_FormatsRowsAndSummary(t *testing.T) { + plan := &Plan{ + Rows: []PlanRow{ + {Action: PlanAdd, Dest: "a.yaml"}, + {Action: PlanOverwrite, Dest: "b.yaml"}, + {Action: PlanKeep, Dest: "c.yaml", Note: "local changes preserved"}, + }, + } + var buf bytes.Buffer + RenderPlanText(&buf, plan) + out := buf.String() + if !strings.Contains(out, "a.yaml") { + t.Error("missing a.yaml") + } + if !strings.Contains(out, "b.yaml") { + t.Error("missing b.yaml") + } + if !strings.Contains(out, "(local changes preserved)") { + t.Error("note not rendered") + } + if !strings.Contains(out, "1 add") || !strings.Contains(out, "1 overwrite") || !strings.Contains(out, "1 keep") { + t.Errorf("summary missing counts, got: %s", out) + } +} + +func TestRenderPlanJSON_RoundTrip(t *testing.T) { + plan := &Plan{ + Ref: "github.com/org/repo", + Commit: "abc123", + Rows: []PlanRow{ + {Action: PlanUpdate, Source: "a.yaml", Dest: "a.yaml"}, + {Action: PlanConflict, Source: "b.yaml", Dest: "b.yaml", Note: "see markers"}, + }, + } + var buf bytes.Buffer + if err := RenderPlanJSON(&buf, plan); err != nil { + t.Fatal(err) + } + var got Plan + if err := json.Unmarshal(buf.Bytes(), &got); err != nil { + t.Fatalf("unmarshal: %v\n%s", err, buf.String()) + } + if got.Ref != plan.Ref || got.Commit != plan.Commit { + t.Errorf("metadata lost: %+v", got) + } + if len(got.Rows) != 2 { + t.Fatalf("rows = %d, want 2", len(got.Rows)) + } + if got.Rows[1].Action != PlanConflict || got.Rows[1].Note != "see markers" { + t.Errorf("row[1] = %+v", got.Rows[1]) + } +} + +func TestPlanFromResult_DeterministicOrdering(t *testing.T) { + // Rows should be sorted by Dest so two equivalent inputs produce + // byte-identical text output (snapshot tests rely on this). + result := &SyncResult{ + Files: []lock.LockFile{ + {Dest: "z.yaml", Status: "replaced"}, + {Dest: "a.yaml", Status: "replaced"}, + {Dest: "m.yaml", Status: "replaced"}, + }, + } + p := PlanFromResult(result) + if p.Rows[0].Dest != "a.yaml" || p.Rows[1].Dest != "m.yaml" || p.Rows[2].Dest != "z.yaml" { + t.Errorf("rows not sorted: %+v", p.Rows) + } +} diff --git a/pkg/state/resolve.go b/pkg/state/resolve.go index ebbbfd4..83f775b 100644 --- a/pkg/state/resolve.go +++ b/pkg/state/resolve.go @@ -46,6 +46,9 @@ func ResolveProfileIncludes(profile *EnvEntry, allProfiles EnvList) (*EnvEntry, if p.Strategy != "" { merged.Strategy = p.Strategy } + if p.Safety != "" { + merged.Safety = p.Safety + } if p.Group != "" { merged.Group = p.Group } diff --git a/pkg/state/types.go b/pkg/state/types.go index f5d9d01..0de8ceb 100644 --- a/pkg/state/types.go +++ b/pkg/state/types.go @@ -23,12 +23,38 @@ type EnvEntry struct { Version string `yaml:"version,omitempty"` Ignore []string `yaml:"ignore,omitempty"` Strategy string `yaml:"strategy,omitempty"` + Safety string `yaml:"safety,omitempty"` // strict | prompt (default) | auto — see issue #125 Group string `yaml:"group,omitempty"` OnPreSync string `yaml:"onPreSync,omitempty"` OnPostSync string `yaml:"onPostSync,omitempty"` Files map[string]envmatch.GlobConfig `yaml:"-"` // populated via custom EnvList unmarshal } +// Safety levels for env updates. The default is SafetyPrompt. +const ( + // SafetyStrict refuses to apply any destructive change (overwrite, + // delete, conflict). The plan is printed; if it contains any + // destructive row the sync exits non-zero with no changes written. + SafetyStrict = "strict" + // SafetyPrompt prints the plan and asks the user to confirm before + // applying. On non-TTY (CI/CD) it falls back to strict behavior. + SafetyPrompt = "prompt" + // SafetyAuto applies the plan without prompting. Equivalent to the + // pre-#125 default behavior. Use only when you trust the upstream. + SafetyAuto = "auto" +) + +// NormalizeSafety returns the canonical safety value. Empty string and +// unknown values both fall back to SafetyPrompt — the safe default. +func NormalizeSafety(s string) string { + switch s { + case SafetyStrict, SafetyPrompt, SafetyAuto: + return s + default: + return SafetyPrompt + } +} + // EnvList is a list of env entries parsed from the envs map. type EnvList []*EnvEntry @@ -47,6 +73,7 @@ func (list *EnvList) UnmarshalYAML(unmarshal func(interface{}) error) error { e.Version = r.Version e.Ignore = r.Ignore e.Strategy = r.Strategy + e.Safety = r.Safety e.Group = r.Group e.OnPreSync = r.OnPreSync e.OnPostSync = r.OnPostSync @@ -77,6 +104,11 @@ func (list *EnvList) MarshalYAML() (interface{}, error) { if e.Strategy != "" && e.Strategy != "replace" { cfg["strategy"] = e.Strategy } + // Omit safety when it's the default ("prompt" or empty) so b.yaml + // stays terse for the common case. + if e.Safety != "" && e.Safety != SafetyPrompt { + cfg["safety"] = e.Safety + } if e.Group != "" { cfg["group"] = e.Group } @@ -146,6 +178,7 @@ type envEntryRaw struct { Version string `yaml:"version,omitempty"` Ignore []string `yaml:"ignore,omitempty"` Strategy string `yaml:"strategy,omitempty"` + Safety string `yaml:"safety,omitempty"` Group string `yaml:"group,omitempty"` OnPreSync string `yaml:"onPreSync,omitempty"` OnPostSync string `yaml:"onPostSync,omitempty"` From 5174db00896ddc7950f8e7cd185784f19876b169 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 14:43:04 +0200 Subject: [PATCH 02/13] fix(env): address copilot + reviewer feedback on PR #128 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review: 1. (#3044962211) --safety values were silently coerced via state.NormalizeSafety, so a typo like --safety=autp fell back to prompt with no error. Fix: Validate() now hard-rejects unknown --safety values up front, matching the existing --strategy validation. NormalizeSafety still applies to config-loaded values for backward compatibility. 2. (#3044962259) Safety gate refusals printed an error and `continue`d but updateEnvs returned nil, contradicting the "CI pipelines will fail" promise. Fix: track refused envs in a slice; after the loop, return an aggregated error ("safety refused N env(s): ..."). Lock for non-refused envs is still written so a single bad apple doesn't block the rest of the run. Tests SafetyStrict_RefusesDestructive and PromptDefault_NonTTY_BlocksDestructive updated to assert the non-nil return. 3. (#3044962280, #3044962389) --plan-json emitted one JSON document per env, producing concatenated docs that aren't valid JSON for typical parsers. Fix: collect plans into a slice during the loop and emit a single JSON array at the end via new env.RenderPlansJSON helper. Docs updated to show the array format with a jsonc example. 4. (#3044962301) PlanAdd was unreachable — SyncEnv reports new files as Status: "replaced" so the plan would always classify them as Update. Fix: PlanFromResult now takes a `prev *lock.EnvEntry` parameter and uses the previous lock's source-path set to distinguish "new file (Add)" from "existing file changed (Update)". New tests PlanFromResult_NewFileBecomesPlanAdd and MergeFailedNoteHasNoDoubleParens pin the contract. 5. (#3044962320) "replaced (merge failed: ...)" notes were extracted with TrimSuffix(s, "") which is a no-op, leaving the parens in. The renderer wraps notes in parens itself, producing ((merge failed: ...)). Fix: strip leading ( and trailing ) from the extracted note. Test MergeFailedNoteHasNoDoubleParens covers it. 6. (#3044962355) Docs claimed "Every b update produces a plan before applying" which is false for the safety:auto path (apply-first, plan rendered from result). Fix: docs now describe the plan-first vs apply-first split explicitly. 7. (#3044962389) See #3 — same fix. Reviewer notes (downstream Claude agent): - N5: drop zero counts from summary line. Fixed — "→ 1 add" instead of "→ 1 add, 0 update, 0 keep, 0 ...". - N6: docs table didn't make --yes vs strict distinction explicit. Fixed — added a fourth column showing --yes effect per mode and a paragraph explaining "no effect on strict". - N1, N2: deferred follow-ups (not blocking, polish only). Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/env-sync.mdx | 28 ++++++++++--- pkg/cli/env_test.go | 12 +++--- pkg/cli/update.go | 63 ++++++++++++++++++++++++---- pkg/cli/update_test.go | 33 +++++++++++---- pkg/env/plan.go | 93 ++++++++++++++++++++++++++++++++++-------- pkg/env/plan_test.go | 81 ++++++++++++++++++++++++++++++++---- 6 files changed, 256 insertions(+), 54 deletions(-) diff --git a/docs/env-sync.mdx b/docs/env-sync.mdx index 77ff7a8..34d3fcb 100644 --- a/docs/env-sync.mdx +++ b/docs/env-sync.mdx @@ -293,6 +293,15 @@ b update --dry-run b update --plan-json ``` +`--plan-json` writes a single JSON array of plan objects (one per env) so you can parse the entire run with one `jq .` invocation: + +```jsonc +[ + { "ref": "github.com/org/infra", "commit": "def4567", "rows": [ ... ] }, + { "ref": "github.com/org/other", "commit": "abc1234", "rows": [ ... ] } +] +``` + ### Rollback ```bash @@ -304,16 +313,23 @@ b update --rollback github.com/org/infra ## Safety tiers -Every `b update` produces a **plan** before applying — a per-file table of additions, updates, keeps, overwrites, merges, and conflicts. The `safety` setting controls what `b` does with that plan: +Every `b update` produces a **plan** — a per-file table of additions, updates, keeps, overwrites, merges, and conflicts. There are two flows depending on the safety setting: -| Mode | Interactive (TTY) | Non-interactive (CI) | Use when | -|------------|--------------------------------|-----------------------------------|-----------------------------------------| -| `strict` | Refuses any destructive plan | Refuses any destructive plan | Locked-down repos, untrusted upstreams | -| `prompt`* | Shows plan, asks `[y/N]` | Refuses destructive (= strict) | Default — safe everywhere | -| `auto` | Applies without prompting | Applies without prompting | Trusted upstream, fully automated runs | +- **Plan-first** (`strict`, `prompt`): `b` runs a dry-run sync to compute the plan, prints it, then either applies (after passing the safety gate) or refuses. +- **Apply-first** (`auto`, `--yes`): `b` applies directly and renders the plan from the apply result. The plan still appears in the output but the writes have already happened by the time you see it. + +The `safety` setting controls which flow `b` uses and what it does with destructive changes: + +| Mode | Interactive (TTY) | Non-interactive (CI) | `--yes` effect | +|------------|--------------------------------|-----------------------------------|---------------------------| +| `strict` | Refuses any destructive plan | Refuses any destructive plan | **No effect** — still refuses | +| `prompt`* | Shows plan, asks `[y/N]` | Refuses destructive | Acts like `auto` | +| `auto` | Applies without prompting | Applies without prompting | (no-op) | \* `prompt` is the default. It is the safe option both interactively and in CI. +`--yes` is the CI escape hatch for `prompt`. It does NOT override `strict`: if you want a permanent override use `safety: auto` in `b.yaml` or `--safety=auto` on the command line. A safety refusal causes `b update` to exit non-zero so CI pipelines actually notice. + **A plan row is "destructive" when it would lose user-owned content** — currently that means `overwrite` (a non-merge strategy is about to clobber locally modified files) or `conflict` (a 3-way merge produced markers and the file needs manual resolution). Configure per-env in `b.yaml`: diff --git a/pkg/cli/env_test.go b/pkg/cli/env_test.go index 0c456f4..fbdd50c 100644 --- a/pkg/cli/env_test.go +++ b/pkg/cli/env_test.go @@ -490,12 +490,12 @@ func TestUpdateEnvs_DryRun_SkipsLockWrite(t *testing.T) { t.Errorf("expected 0 envs in lock after dry-run, got %d", len(lk.Envs)) } - // New plan-based output: dry-run produces a plan summary line ending - // in "→ N add, ..." rather than a literal "dry-run" tag in the - // header. The lock-not-written assertion above is the load-bearing - // behavior contract. - if !strings.Contains(out.String(), "add,") { - t.Errorf("output should contain plan summary, got: %q", out.String()) + // New plan-based output: dry-run emits a plan summary line ("→ 1 + // add") rather than a literal "dry-run" tag in the header. The + // lock-not-written assertion above is the load-bearing behavior + // contract; this just sanity-checks the plan is rendered. + if !strings.Contains(out.String(), "→") { + t.Errorf("output should contain plan summary arrow, got: %q", out.String()) } } diff --git a/pkg/cli/update.go b/pkg/cli/update.go index c3d2483..0c87f32 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -158,6 +158,18 @@ func (o *UpdateOptions) Validate() error { return fmt.Errorf("invalid strategy %q: must be replace, client, or merge", o.Strategy) } } + // `--safety` materially changes non-TTY behavior, so a typo (e.g. + // `--safety=autp`) must error rather than silently fall back to + // prompt. Per copilot review on PR #128. + if o.Safety != "" { + switch o.Safety { + case state.SafetyAuto, state.SafetyPrompt, state.SafetyStrict: + // valid + default: + return fmt.Errorf("invalid --safety value %q: must be %s, %s, or %s", + o.Safety, state.SafetyStrict, state.SafetyPrompt, state.SafetyAuto) + } + } return nil } @@ -234,6 +246,19 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { return err } + // Tracks any per-env safety gate refusals so we can return a + // non-zero exit at the end. Per copilot review on PR #128: silent + // refusal contradicts the documented "CI pipelines will fail" + // promise. Per-env apply work continues for non-refused envs so a + // single bad apple doesn't block the rest of the run. + var refusedEnvs []string + + // Collected plans for --plan-json. Per copilot review on PR #128: + // emitting one JSON document per env produced concatenated docs + // that aren't valid JSON for typical parsers. We now collect plans + // in this slice and emit a single JSON array at the end. + var planJSONOut []*env.Plan + for _, entry := range o.Config.Envs { if refs != nil { found := false @@ -327,13 +352,12 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { continue // don't overwrite lock entry when up-to-date } - plan := env.PlanFromResult(firstResult) + plan := env.PlanFromResult(firstResult, lockEntry) - // --plan-json: emit JSON and skip everything else for this env. + // --plan-json: collect the plan for batched JSON output below. + // We never apply in plan-json mode (it implies dry-run). if o.PlanJSON { - if err := env.RenderPlanJSON(o.IO.Out, plan); err != nil { - fmt.Fprintf(o.IO.ErrOut, " %-40s ✗ %s\n", entry.Key, err) - } + planJSONOut = append(planJSONOut, plan) continue } @@ -363,10 +387,12 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { apply, gateErr := o.gateApply(safety, plan, isDryRun) if gateErr != nil { fmt.Fprintf(o.IO.ErrOut, " %-40s ✗ %s\n", entry.Key, gateErr) + refusedEnvs = append(refusedEnvs, entry.Key) continue } if !apply { - // Dry-run, strict refusal, or user declined. + // Dry-run or user declined the prompt — not an error, + // just nothing to do for this env. continue } @@ -397,11 +423,30 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { }) } - if o.DryRun || o.PlanJSON { - return nil // don't write lock in dry-run / plan-only mode + if o.PlanJSON { + // Emit the collected plans as a single JSON array so PR + // comment bots / CI summary jobs can parse with a single + // invocation. Per copilot review on PR #128. + if err := env.RenderPlansJSON(o.IO.Out, planJSONOut); err != nil { + return err + } + return nil + } + if o.DryRun { + return nil // don't write lock in dry-run mode } - return lock.WriteLock(lockDir, lk, o.bVersion) + if err := lock.WriteLock(lockDir, lk, o.bVersion); err != nil { + return err + } + if len(refusedEnvs) > 0 { + // Per copilot review on PR #128: any safety refusal must + // produce a non-zero exit code so CI pipelines actually + // notice. The lock for non-refused envs has already been + // written above so partial progress isn't lost. + return fmt.Errorf("safety refused %d env(s): %s", len(refusedEnvs), strings.Join(refusedEnvs, ", ")) + } + return nil } // printConflictHint emits the legacy "needs manual resolution" footer for diff --git a/pkg/cli/update_test.go b/pkg/cli/update_test.go index 81b72f5..7d581e2 100644 --- a/pkg/cli/update_test.go +++ b/pkg/cli/update_test.go @@ -665,10 +665,11 @@ func TestUpdateEnvs_Updated(t *testing.T) { t.Fatalf("updateEnvs error = %v", err) } out := outBuf.String() - // Plan-format output: "replaced" status maps to action "update", - // "kept" maps to action "keep". Assert the new action labels. - if !strings.Contains(out, "update") { - t.Errorf("expected plan 'update' line, got: %s", out) + // Plan-format output: "replaced" status maps to action "add" when + // the file isn't in the previous lock entry (the lock starts empty + // in this test). "kept" maps to action "keep". + if !strings.Contains(out, "add") { + t.Errorf("expected plan 'add' line, got: %s", out) } if !strings.Contains(out, "keep") { t.Errorf("expected plan 'keep' line, got: %s", out) @@ -1801,8 +1802,14 @@ func TestUpdateEnvs_SafetyStrict_RefusesDestructive(t *testing.T) { syncEnvFunc = syncFn o, _, errBuf := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo", Safety: state.SafetyStrict}) - if err := o.updateEnvs(nil); err != nil { - t.Fatalf("updateEnvs: %v", err) + err := o.updateEnvs(nil) + // Per copilot review on PR #128: strict refusal must produce a + // non-zero exit code, so updateEnvs must return an error. + if err == nil { + t.Fatal("updateEnvs should return an error when strict refuses") + } + if !strings.Contains(err.Error(), "safety refused") { + t.Errorf("expected aggregated safety error, got: %v", err) } // Strict refusal: the plan-first dry-run runs (1 call), gate // rejects, no second apply call. @@ -1863,8 +1870,14 @@ func TestUpdateEnvs_PromptDefault_NonTTY_BlocksDestructive(t *testing.T) { syncEnvFunc = syncFn o, _, errBuf := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo"}) // no safety set → default prompt - if err := o.updateEnvs(nil); err != nil { - t.Fatalf("updateEnvs: %v", err) + err := o.updateEnvs(nil) + // Per copilot review on PR #128: prompt-on-CI refusal must also + // produce a non-zero exit code so CI pipelines actually notice. + if err == nil { + t.Fatal("updateEnvs should return an error when non-TTY prompt refuses") + } + if !strings.Contains(err.Error(), "safety refused") { + t.Errorf("expected aggregated safety error, got: %v", err) } if len(*calls) != 1 { t.Errorf("non-TTY prompt should refuse without applying, got %d calls", len(*calls)) @@ -1953,7 +1966,9 @@ func TestUpdateEnvs_DryRun_NewBehavior(t *testing.T) { if len(*calls) != 1 { t.Errorf("dry-run should call SyncEnv once, got %d", len(*calls)) } - if !strings.Contains(out.String(), "add,") { + // New summary format omits zero counts, so a single-add plan + // renders as "→ 1 add" rather than "→ 0 add, ...". + if !strings.Contains(out.String(), "→") || !strings.Contains(out.String(), "add") { t.Errorf("dry-run plan summary missing, got:\n%s", out.String()) } } diff --git a/pkg/env/plan.go b/pkg/env/plan.go index 54a69ab..d0a0340 100644 --- a/pkg/env/plan.go +++ b/pkg/env/plan.go @@ -79,10 +79,15 @@ func (p *Plan) CountByAction() map[PlanAction]int { // from a SyncEnv invocation (dry-run or real); the lock files' Status // field is the ground truth and is mapped to a PlanAction. // +// `prev` is the lock entry from the *previous* sync (nil if first +// sync). It's used to distinguish "new file" (PlanAdd) from "existing +// file changed" (PlanUpdate) — SyncEnv reports both as "replaced" and +// the previous-lock comparison is the only available signal here. +// // The mapping intentionally collapses the dry-run-suffixed statuses -// ("replaced (dry-run)", "kept (dry-run)", etc.) so the renderer doesn't -// have to special-case them. -func PlanFromResult(result *SyncResult) *Plan { +// ("replaced (dry-run)", "kept (dry-run)", etc.) so the renderer +// doesn't have to special-case them. +func PlanFromResult(result *SyncResult, prev *lock.EnvEntry) *Plan { if result == nil { return &Plan{} } @@ -92,8 +97,20 @@ func PlanFromResult(result *SyncResult) *Plan { Version: result.Version, Commit: result.Commit, } + // Build a quick lookup of source paths that existed in the previous + // lock entry, so we can identify NEW files (PlanAdd) vs UPDATED + // files (PlanUpdate). Both come back from SyncEnv as Status: + // "replaced" today, so without this comparison everything would + // render as "update" and the "add" action would be unreachable — + // flagged by copilot review on PR #128. + prevPaths := make(map[string]bool) + if prev != nil { + for _, f := range prev.Files { + prevPaths[f.Path] = true + } + } for _, f := range result.Files { - p.Rows = append(p.Rows, planRowFromLockFile(f)) + p.Rows = append(p.Rows, planRowFromLockFile(f, prevPaths)) } // Stable, deterministic ordering by destination path so callers can // snapshot-test the rendering. @@ -105,7 +122,12 @@ func PlanFromResult(result *SyncResult) *Plan { // planRowFromLockFile maps a lock.LockFile (which carries a free-form // status string) into a structured PlanRow. -func planRowFromLockFile(f lock.LockFile) PlanRow { +// +// `prevPaths` is the set of source paths that existed in the previous +// lock entry; it lets us tell apart "this file is new (Add)" from +// "this file existed and changed (Update)" — SyncEnv reports both as +// Status: "replaced". +func planRowFromLockFile(f lock.LockFile, prevPaths map[string]bool) PlanRow { status := strings.TrimSuffix(f.Status, " (dry-run)") row := PlanRow{ Source: f.Path, @@ -125,13 +147,26 @@ func planRowFromLockFile(f lock.LockFile) PlanRow { row.Action = PlanOverwrite case strings.HasPrefix(status, "replaced (merge failed"): row.Action = PlanOverwrite - // Extract the parenthetical reason for the user. - row.Note = strings.TrimSuffix(strings.TrimPrefix(status, "replaced "), "") + // Extract the parenthetical reason for the user without the + // surrounding parentheses; the renderer wraps notes in parens + // itself, so leaving them in produces "((merge failed: ...))". + // Per copilot review on PR #128. + note := strings.TrimPrefix(status, "replaced ") + note = strings.TrimPrefix(note, "(") + note = strings.TrimSuffix(note, ")") + row.Note = note case status == "replaced": - // "replaced" alone (no "local changes overwritten") means the - // file was new or identical-on-disk; treat as add/update rather - // than destructive. - row.Action = PlanUpdate + // "replaced" alone means the file was new or already in sync + // (identical on disk). Distinguish "new" from "existing + // changed" via the previous lock entry: if the source path + // wasn't in prev, it's an Add; otherwise it's an Update. + // Without this check, PlanAdd would be unreachable (per + // copilot review on PR #128). + if prevPaths != nil && !prevPaths[f.Path] { + row.Action = PlanAdd + } else { + row.Action = PlanUpdate + } default: row.Action = PlanUpdate row.Note = status @@ -166,20 +201,46 @@ func RenderPlanText(w io.Writer, p *Plan) { fmt.Fprintf(w, " %s %-9s %s\n", marker, label, r.Dest) } } + // Summary line: only emit non-zero counts so the common case + // reads "→ 3 update" instead of "→ 0 add, 3 update, 0 keep, 0 + // overwrite, 0 merge, 0 conflict". Per reviewer note on PR #128. counts := p.CountByAction() - fmt.Fprintf(w, " → %d add, %d update, %d keep, %d overwrite, %d merge, %d conflict\n", - counts[PlanAdd], counts[PlanUpdate], counts[PlanKeep], - counts[PlanOverwrite], counts[PlanMerge], counts[PlanConflict]) + order := []PlanAction{PlanAdd, PlanUpdate, PlanKeep, PlanOverwrite, PlanMerge, PlanConflict} + var parts []string + for _, a := range order { + if c := counts[a]; c > 0 { + parts = append(parts, fmt.Sprintf("%d %s", c, a)) + } + } + if len(parts) == 0 { + fmt.Fprintln(w, " → no changes") + } else { + fmt.Fprintf(w, " → %s\n", strings.Join(parts, ", ")) + } } -// RenderPlanJSON writes the plan as JSON for machine consumers (PR -// comment bots, CI summary jobs, etc). +// RenderPlanJSON writes a single plan as a JSON document. Used by +// callers that operate on one plan at a time. func RenderPlanJSON(w io.Writer, p *Plan) error { enc := json.NewEncoder(w) enc.SetIndent("", " ") return enc.Encode(p) } +// RenderPlansJSON writes a slice of plans as a single JSON array. This +// is the format the CLI emits for `--plan-json` so consumers can parse +// the entire run with one `jq .` invocation. Per copilot review on +// PR #128: emitting one JSON document per env produced concatenated +// docs that weren't valid JSON for standard parsers. +func RenderPlansJSON(w io.Writer, plans []*Plan) error { + if plans == nil { + plans = []*Plan{} + } + enc := json.NewEncoder(w) + enc.SetIndent("", " ") + return enc.Encode(plans) +} + // planMarker returns the (glyph, label) pair for a plan action. The glyph // is single-width to keep the table aligned in the common case. func planMarker(a PlanAction) (string, string) { diff --git a/pkg/env/plan_test.go b/pkg/env/plan_test.go index a44cd96..45d3d66 100644 --- a/pkg/env/plan_test.go +++ b/pkg/env/plan_test.go @@ -38,7 +38,15 @@ func TestPlanFromResult_StatusMapping(t *testing.T) { {Path: "g.yaml", Dest: "g.yaml", Status: "replaced (dry-run)"}, }, } - plan := PlanFromResult(result) + // Pass a previous lock entry that includes a.yaml + g.yaml so they + // classify as Update (existed before, now changed) rather than Add. + prev := &lock.EnvEntry{ + Files: []lock.LockFile{ + {Path: "a.yaml"}, + {Path: "g.yaml"}, + }, + } + plan := PlanFromResult(result, prev) if len(plan.Rows) != 7 { t.Fatalf("rows = %d, want 7", len(plan.Rows)) } @@ -58,16 +66,73 @@ func TestPlanFromResult_StatusMapping(t *testing.T) { } } +// TestPlanFromResult_NewFileBecomesPlanAdd verifies the Add detection: +// when a file was NOT in the previous lock entry, "replaced" should map +// to PlanAdd (not PlanUpdate). Per copilot review on PR #128. +func TestPlanFromResult_NewFileBecomesPlanAdd(t *testing.T) { + result := &SyncResult{ + Files: []lock.LockFile{ + {Path: "new.yaml", Dest: "new.yaml", Status: "replaced"}, + {Path: "old.yaml", Dest: "old.yaml", Status: "replaced"}, + }, + } + prev := &lock.EnvEntry{Files: []lock.LockFile{{Path: "old.yaml"}}} + plan := PlanFromResult(result, prev) + for _, r := range plan.Rows { + switch r.Dest { + case "new.yaml": + if r.Action != PlanAdd { + t.Errorf("new.yaml: action = %q, want add", r.Action) + } + case "old.yaml": + if r.Action != PlanUpdate { + t.Errorf("old.yaml: action = %q, want update", r.Action) + } + } + } +} + +// TestPlanFromResult_MergeFailedNoteHasNoDoubleParens verifies the +// note extraction strips outer parens so the renderer doesn't double- +// wrap. Per copilot review on PR #128. +func TestPlanFromResult_MergeFailedNoteHasNoDoubleParens(t *testing.T) { + result := &SyncResult{ + Files: []lock.LockFile{{ + Path: "a.yaml", + Dest: "a.yaml", + Status: "replaced (merge failed: no previous commit)", + }}, + } + prev := &lock.EnvEntry{Files: []lock.LockFile{{Path: "a.yaml"}}} + plan := PlanFromResult(result, prev) + if len(plan.Rows) != 1 { + t.Fatal("expected 1 row") + } + r := plan.Rows[0] + if r.Action != PlanOverwrite { + t.Errorf("action = %q, want overwrite", r.Action) + } + if strings.HasPrefix(r.Note, "(") || strings.HasSuffix(r.Note, ")") { + t.Errorf("note has surrounding parens: %q", r.Note) + } + if !strings.Contains(r.Note, "merge failed") { + t.Errorf("note missing reason: %q", r.Note) + } +} + func TestPlanFromResult_DestructiveAndCounts(t *testing.T) { result := &SyncResult{ Files: []lock.LockFile{ - {Dest: "a", Status: "replaced"}, - {Dest: "b", Status: "replaced (local changes overwritten)"}, - {Dest: "c", Status: "kept"}, - {Dest: "d", Status: "conflict"}, + {Path: "a", Dest: "a", Status: "replaced"}, + {Path: "b", Dest: "b", Status: "replaced (local changes overwritten)"}, + {Path: "c", Dest: "c", Status: "kept"}, + {Path: "d", Dest: "d", Status: "conflict"}, }, } - p := PlanFromResult(result) + // Pass `prev` containing `a` so it counts as Update (not Add) and + // the existing assertion stays meaningful. + prev := &lock.EnvEntry{Files: []lock.LockFile{{Path: "a"}}} + p := PlanFromResult(result, prev) if !p.HasDestructive() { t.Error("plan with overwrite + conflict should be destructive") } @@ -84,7 +149,7 @@ func TestPlanFromResult_NonDestructive(t *testing.T) { {Dest: "b", Status: "kept"}, }, } - if PlanFromResult(result).HasDestructive() { + if PlanFromResult(result, nil).HasDestructive() { t.Error("non-destructive plan flagged as destructive") } } @@ -160,7 +225,7 @@ func TestPlanFromResult_DeterministicOrdering(t *testing.T) { {Dest: "m.yaml", Status: "replaced"}, }, } - p := PlanFromResult(result) + p := PlanFromResult(result, nil) if p.Rows[0].Dest != "a.yaml" || p.Rows[1].Dest != "m.yaml" || p.Rows[2].Dest != "z.yaml" { t.Errorf("rows not sorted: %+v", p.Rows) } From 6d94ffa6ba783b60313380367f87118d7d4d8162 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 14:55:45 +0200 Subject: [PATCH 03/13] fix(env): address copilot round 2 review on PR #128 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. (#3045074081) --plan-json output was getting corrupted by the "(up to date)" human line printed for skipped envs and the "No binaries or envs to update" line in runAll. Both now suppress the human-readable output when o.PlanJSON is set: skipped envs are silently dropped from the JSON array, and empty configs emit `[]` instead of the prose line. 2. (#3045074142) SafetyStrict's doc comment said it refuses "delete" actions but the destructive set in Phase 1 is just overwrite + conflict (no delete action exists yet). Comment updated to be precise about Phase 1 scope and to flag delete as a future addition when skipped-delete tracking lands. 3. (#3045074173) NormalizeSafety did a strict case-sensitive switch, so "Auto" / " auto " / "STRICT" silently fell back to prompt — surprising for users editing b.yaml. Now trims whitespace and lowercases before comparison. Validate() does the same for --safety so the CLI flag accepts the same forms. New test cases for "Auto", "AUTO", " auto ", "\tStrict\n", "Prompt" added to TestNormalizeSafety_DefaultsToPrompt. 4. (#3045074206) makeSyncFunc test helper shallow-copied SyncResult so the underlying Files backing array was shared between calls — a test that mutated clone.Files[i] would leak into subsequent invocations. Now deep-copies Files via append([]lock.LockFile(nil), result.Files...). Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/cli/update.go | 21 ++++++++++++++++++--- pkg/cli/update_test.go | 17 ++++++++++++++++- pkg/state/types.go | 20 +++++++++++++++----- 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/pkg/cli/update.go b/pkg/cli/update.go index 0c87f32..a35e178 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -160,9 +160,11 @@ func (o *UpdateOptions) Validate() error { } // `--safety` materially changes non-TTY behavior, so a typo (e.g. // `--safety=autp`) must error rather than silently fall back to - // prompt. Per copilot review on PR #128. + // prompt. Validation is case-insensitive and trims whitespace, + // matching the NormalizeSafety contract used by config-loaded + // values. Per copilot review on PR #128 (round 2). if o.Safety != "" { - switch o.Safety { + switch strings.ToLower(strings.TrimSpace(o.Safety)) { case state.SafetyAuto, state.SafetyPrompt, state.SafetyStrict: // valid default: @@ -199,6 +201,12 @@ func (o *UpdateOptions) runAll() error { } if len(binariesToUpdate) == 0 && (o.Config == nil || len(o.Config.Envs) == 0) { + // In plan-json mode the human-readable line would corrupt + // the JSON output. Emit an empty array instead so consumers + // always get valid JSON. Per copilot review on PR #128 round 2. + if o.PlanJSON { + return env.RenderPlansJSON(o.IO.Out, nil) + } fmt.Fprintln(o.IO.Out, "No binaries or envs to update") } @@ -348,7 +356,14 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { } if firstResult.Skipped { - fmt.Fprintf(o.IO.Out, " %-40s %s\n", entry.Key, firstResult.Message) + // In plan-json mode the skipped notice would corrupt the + // JSON output (per copilot review on PR #128 round 2). + // Suppress the human-readable line and skip the env + // entirely — there's nothing to add to the plan array + // because no work was planned. + if !o.PlanJSON { + fmt.Fprintf(o.IO.Out, " %-40s %s\n", entry.Key, firstResult.Message) + } continue // don't overwrite lock entry when up-to-date } diff --git a/pkg/cli/update_test.go b/pkg/cli/update_test.go index 7d581e2..04742d1 100644 --- a/pkg/cli/update_test.go +++ b/pkg/cli/update_test.go @@ -1755,8 +1755,13 @@ func makeSyncFunc(result *env.SyncResult) (func(env.EnvConfig, string, string, * calls := &[]bool{} return func(cfg env.EnvConfig, _ string, _ string, _ *lock.EnvEntry) (*env.SyncResult, error) { *calls = append(*calls, cfg.DryRun) - // Return a copy so callers can mutate Files between checks. + // Return a deep copy so tests that mutate Files between + // invocations don't accidentally share the underlying + // backing array. Per copilot review on PR #128 round 2. clone := *result + if result.Files != nil { + clone.Files = append([]lock.LockFile(nil), result.Files...) + } return &clone, nil }, calls } @@ -1974,6 +1979,10 @@ func TestUpdateEnvs_DryRun_NewBehavior(t *testing.T) { } // TestNormalizeSafety covers the centralized safety-value coercion. +// Per copilot review on PR #128 round 2: whitespace and casing +// variants are normalized so users can write `safety: Auto` or +// `safety: auto ` in b.yaml without the value silently falling back +// to prompt. func TestNormalizeSafety_DefaultsToPrompt(t *testing.T) { cases := map[string]string{ "": state.SafetyPrompt, @@ -1981,6 +1990,12 @@ func TestNormalizeSafety_DefaultsToPrompt(t *testing.T) { "prompt": state.SafetyPrompt, "auto": state.SafetyAuto, "nonsense": state.SafetyPrompt, // unknown → safe default + // Casing and whitespace normalization (#128 round 2). + "Auto": state.SafetyAuto, + "AUTO": state.SafetyAuto, + " auto ": state.SafetyAuto, + "\tStrict\n": state.SafetyStrict, + "Prompt": state.SafetyPrompt, } for in, want := range cases { if got := state.NormalizeSafety(in); got != want { diff --git a/pkg/state/types.go b/pkg/state/types.go index 0de8ceb..c8276f7 100644 --- a/pkg/state/types.go +++ b/pkg/state/types.go @@ -32,9 +32,14 @@ type EnvEntry struct { // Safety levels for env updates. The default is SafetyPrompt. const ( - // SafetyStrict refuses to apply any destructive change (overwrite, - // delete, conflict). The plan is printed; if it contains any - // destructive row the sync exits non-zero with no changes written. + // SafetyStrict refuses to apply any destructive change. As of + // Phase 1 of #125, "destructive" means a plan row whose action is + // `overwrite` (a non-merge strategy is about to clobber locally + // modified files) or `conflict` (a 3-way merge produced markers). + // Future phases may add `delete` to this set when skipped-delete + // tracking lands. The plan is printed; if it contains any + // destructive row the sync exits non-zero with no changes written + // for the offending env. SafetyStrict = "strict" // SafetyPrompt prints the plan and asks the user to confirm before // applying. On non-TTY (CI/CD) it falls back to strict behavior. @@ -46,10 +51,15 @@ const ( // NormalizeSafety returns the canonical safety value. Empty string and // unknown values both fall back to SafetyPrompt — the safe default. +// +// User input is normalized: surrounding whitespace is trimmed and the +// value is lowercased before comparison, so `safety: Auto` and +// `safety: auto ` in b.yaml both resolve to SafetyAuto. Per copilot +// review on PR #128 round 2. func NormalizeSafety(s string) string { - switch s { + switch strings.ToLower(strings.TrimSpace(s)) { case SafetyStrict, SafetyPrompt, SafetyAuto: - return s + return strings.ToLower(strings.TrimSpace(s)) default: return SafetyPrompt } From b3054beec205c3a63763f065b92156e8a6c0e74c Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 15:04:05 +0200 Subject: [PATCH 04/13] fix(env): address copilot round 3 review on PR #128 1. (#3045148116) Skipped envs disappeared from --plan-json output, so consumers couldn't tell "no envs configured" (`[]`) apart from "all envs up-to-date" (also `[]`). Fix: skipped envs now produce a plan object in the array with empty Rows plus ref/label/version/commit metadata. Plain dry-run still prints the cheap "(up to date)" line for backward compat. New test TestUpdateEnvs_PlanJSON_IncludesSkippedEnvs. 2. (#3045148163) Test contract comment said --plan-json "emits one JSON document per env" but the implementation now emits a single array. Comment updated. 3. (#3045148193) Docs claimed --dry-run "always prints the plan" but the skipped path prints "(up to date)" instead of a plan table. Wording clarified to describe both paths accurately, plus the plan-json behavior of emitting empty plans for up-to-date envs. 4. (#3045148213) MarshalYAML's "omit safety when default" check compared the raw value against the literal "prompt" constant, so non-canonical inputs like "Prompt" or " auto " would round-trip into b.yaml as the non-canonical form instead of being normalized away. Fix: compare against NormalizeSafety(e.Safety) and write the normalized value when emitting. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/env-sync.mdx | 2 +- pkg/cli/update.go | 21 +++++++++++++++------ pkg/cli/update_test.go | 34 +++++++++++++++++++++++++++++++++- pkg/state/types.go | 11 +++++++---- 4 files changed, 56 insertions(+), 12 deletions(-) diff --git a/docs/env-sync.mdx b/docs/env-sync.mdx index 34d3fcb..a7f6f20 100644 --- a/docs/env-sync.mdx +++ b/docs/env-sync.mdx @@ -287,7 +287,7 @@ b env remove --delete-files github.com/org/infra b update --dry-run ``` -`--dry-run` always prints the plan and never writes. Combine with `--plan-json` to emit a machine-readable plan for PR comment bots and CI summary jobs: +`--dry-run` prints a per-file plan for any env that has changes and never writes. Envs that are already up-to-date with the lock print a single `(up to date)` line instead of an empty plan table — that's the cheap path. In `--plan-json` mode, every env gets an entry in the array (even up-to-date ones, with `rows: []`) so consumers can distinguish "no envs configured" from "all envs up-to-date". Combine with `--plan-json` to emit a machine-readable plan for PR comment bots and CI summary jobs: ```bash b update --plan-json diff --git a/pkg/cli/update.go b/pkg/cli/update.go index a35e178..e7c6c60 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -356,12 +356,21 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { } if firstResult.Skipped { - // In plan-json mode the skipped notice would corrupt the - // JSON output (per copilot review on PR #128 round 2). - // Suppress the human-readable line and skip the env - // entirely — there's nothing to add to the plan array - // because no work was planned. - if !o.PlanJSON { + // Plan-json mode: emit an explicit empty plan for the + // skipped env so consumers can distinguish "all envs are + // up to date" from "no envs configured" — both used to + // produce []. Per copilot review on PR #128 round 3. + // In dry-run/plan-text mode also emit the empty plan + // (header + "no changes" summary) so the docs claim + // "every b update produces a plan" is accurate. + if o.PlanJSON { + planJSONOut = append(planJSONOut, &env.Plan{ + Ref: ref, + Label: label, + Version: entry.Version, + Commit: firstResult.Commit, + }) + } else { fmt.Fprintf(o.IO.Out, " %-40s %s\n", entry.Key, firstResult.Message) } continue // don't overwrite lock entry when up-to-date diff --git a/pkg/cli/update_test.go b/pkg/cli/update_test.go index 04742d1..46f7d8b 100644 --- a/pkg/cli/update_test.go +++ b/pkg/cli/update_test.go @@ -1714,7 +1714,7 @@ func TestPrintFileStatus_AllStatuses(t *testing.T) { // - --safety=auto applies without prompting (legacy behavior) // - --safety=strict refuses ANY destructive plan, with or without TTY // - --yes overrides the prompt and behaves like auto -// - --plan-json emits one JSON document per env and skips writes +// - --plan-json emits a single JSON array for the whole run and skips writes // - the dry-run pass never updates the lock and always renders a plan // - the auto / --yes paths only call SyncEnv ONCE — the plan-first // paths call it twice (one dry-run for the plan + one apply) @@ -1918,6 +1918,38 @@ func TestUpdateEnvs_Yes_OverridesPrompt(t *testing.T) { } } +// TestUpdateEnvs_PlanJSON_IncludesSkippedEnvs verifies that an env +// with `Skipped: true` (already up-to-date) still produces a plan +// object in the JSON array — empty rows + ref/commit metadata — so +// consumers can distinguish "no envs configured" from "all envs +// up-to-date". Per copilot review on PR #128 round 3. +func TestUpdateEnvs_PlanJSON_IncludesSkippedEnvs(t *testing.T) { + saveHooks(t) + syncFn, _ := makeSyncFunc(&env.SyncResult{ + Ref: "github.com/org/repo", + Commit: "abc", + Skipped: true, + Message: "(up to date)", + }) + syncEnvFunc = syncFn + + o, out, _ := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo"}, + func(o *UpdateOptions) { o.PlanJSON = true }) + if err := o.updateEnvs(nil); err != nil { + t.Fatalf("updateEnvs: %v", err) + } + outStr := out.String() + // Output should be a JSON array with one entry, not []. + if !strings.Contains(outStr, `"github.com/org/repo"`) { + t.Errorf("expected skipped env in JSON output, got:\n%s", outStr) + } + // Sanity: must NOT contain the human "(up to date)" line that + // would corrupt the JSON. + if strings.Contains(outStr, "(up to date)") { + t.Errorf("plan-json should not include human-readable up-to-date line, got:\n%s", outStr) + } +} + // TestUpdateEnvs_PlanJSON_EmitsAndSkipsApply: --plan-json prints a JSON // plan and never applies. func TestUpdateEnvs_PlanJSON_EmitsAndSkipsApply(t *testing.T) { diff --git a/pkg/state/types.go b/pkg/state/types.go index c8276f7..60f50b3 100644 --- a/pkg/state/types.go +++ b/pkg/state/types.go @@ -114,10 +114,13 @@ func (list *EnvList) MarshalYAML() (interface{}, error) { if e.Strategy != "" && e.Strategy != "replace" { cfg["strategy"] = e.Strategy } - // Omit safety when it's the default ("prompt" or empty) so b.yaml - // stays terse for the common case. - if e.Safety != "" && e.Safety != SafetyPrompt { - cfg["safety"] = e.Safety + // Omit safety when it normalizes to the default ("prompt") or + // is empty so b.yaml stays terse for the common case. Compare + // against the normalized value so non-canonical inputs like + // `Prompt`, ` auto `, etc. round-trip cleanly. Per copilot + // review on PR #128 round 3. + if e.Safety != "" && NormalizeSafety(e.Safety) != SafetyPrompt { + cfg["safety"] = NormalizeSafety(e.Safety) } if e.Group != "" { cfg["group"] = e.Group From a4cb2936810611259a477cbe8620c74a7ebf9040 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 15:13:07 +0200 Subject: [PATCH 05/13] fix(env): Plan.MarshalJSON nil-rows + plan doc marker glyphs (#128 round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. (#3045189400) Plan.Rows could be nil (e.g. for the skipped-env plans introduced in round 3), which encoded as `"rows": null` instead of `"rows": []`. The plan-json contract advertised in docs/env-sync.mdx promises `[]` for up-to-date envs, and consumers that index into the array would break on `null`. Fix: implement MarshalJSON on Plan that initializes a nil Rows slice to an empty one before encoding. New test TestPlan_MarshalJSON_NilRowsEncodesAsEmptyArray pins it. 2. (#3045189485) RenderPlanText's format-comment block had its `+` glyph for PlanAdd eaten by godoc list-bullet rendering, so future maintainers reading the comment would see `- add` instead of `+ add`. Fix: wrap each marker glyph in single quotes ('+', '~', '=', '!', '⊕', '✗') so godoc renders them literally and they match the output of planMarker(). Also simplified the summary-line example to show non-zero counts only, matching the round-2 RenderPlanText change that drops zero counts. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/env/plan.go | 33 ++++++++++++++++++++++++--------- pkg/env/plan_test.go | 20 ++++++++++++++++++++ 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/pkg/env/plan.go b/pkg/env/plan.go index d0a0340..449a89e 100644 --- a/pkg/env/plan.go +++ b/pkg/env/plan.go @@ -55,6 +55,20 @@ type Plan struct { Rows []PlanRow `json:"rows"` } +// MarshalJSON ensures Plan.Rows is encoded as `[]` rather than `null` +// when the slice is nil. The plan-json contract advertised in +// docs/env-sync.mdx promises `rows: []` for up-to-date envs, and +// consumers that index into the array would break on `null`. Per +// copilot review on PR #128 round 4. +func (p Plan) MarshalJSON() ([]byte, error) { + type planAlias Plan + out := planAlias(p) + if out.Rows == nil { + out.Rows = make([]PlanRow, 0) + } + return json.Marshal(out) +} + // HasDestructive reports whether the plan contains any destructive row. func (p *Plan) HasDestructive() bool { for _, r := range p.Rows { @@ -176,18 +190,19 @@ func planRowFromLockFile(f lock.LockFile, prevPaths map[string]bool) PlanRow { // RenderPlanText writes a human-readable plan table to w. // -// Format (one line per row): +// Format (one line per row, marker glyphs in single quotes to avoid +// godoc list-bullet eating them): // -// - add path/to/file -// ~ update path/to/file -// = keep path/to/file (local changes preserved) -// ! overwrite path/to/file -// ⊕ merge path/to/file -// ✗ conflict path/to/file +// '+' add path/to/file +// '~' update path/to/file +// '=' keep path/to/file (local changes preserved) +// '!' overwrite path/to/file +// '⊕' merge path/to/file +// '✗' conflict path/to/file // -// A summary line is printed at the end: +// A summary line is printed at the end with non-zero counts only: // -// → 12 add, 3 update, 0 keep, 1 overwrite, 0 merge, 0 conflict +// → 12 add, 3 update, 1 conflict func RenderPlanText(w io.Writer, p *Plan) { if p == nil || len(p.Rows) == 0 { fmt.Fprintln(w, " (no files)") diff --git a/pkg/env/plan_test.go b/pkg/env/plan_test.go index 45d3d66..6046472 100644 --- a/pkg/env/plan_test.go +++ b/pkg/env/plan_test.go @@ -230,3 +230,23 @@ func TestPlanFromResult_DeterministicOrdering(t *testing.T) { t.Errorf("rows not sorted: %+v", p.Rows) } } + +// TestPlan_MarshalJSON_NilRowsEncodesAsEmptyArray verifies that a +// Plan with nil Rows encodes as `"rows":[]` rather than +// `"rows":null`. The plan-json contract advertised in +// docs/env-sync.mdx promises `[]` for up-to-date envs, and +// consumers indexing into the array would break on `null`. Per +// copilot review on PR #128 round 4. +func TestPlan_MarshalJSON_NilRowsEncodesAsEmptyArray(t *testing.T) { + p := Plan{Ref: "github.com/org/repo", Commit: "abc"} + out, err := json.Marshal(p) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(out), `"rows":[]`) { + t.Errorf("expected rows:[], got: %s", out) + } + if strings.Contains(string(out), `"rows":null`) { + t.Errorf("rows should NOT be null, got: %s", out) + } +} From 6e69fb26cea3958c0fbcacae658ff73137357732 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 15:21:42 +0200 Subject: [PATCH 06/13] fix(cli): non-zero exit on per-env sync failures (#128 round 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 5 caught: when SyncEnv returned an error for an env, the code printed it to stderr and continued, but updateEnvs returned nil unless a safety refusal also happened. CI runs would succeed with exit code 0 even though one or more env syncs failed silently. Fix: track per-env failures in `failedEnvs` (mirror of the `refusedEnvs` slice from round 1) at every error site: - rollback with no previous commit - first-pass SyncEnv error - second-pass real-apply SyncEnv error After the loop, return an aggregated error like "N env(s) failed: ..." or, when refusals AND failures both happened, both attributed in one message. The `--plan-json` path doesn't have a sync error site (it just appends to a slice), so plan-json continues to work the same. Test contract update: TestUpdateEnvs_SyncError previously asserted "updateEnvs should not return error on per-env failure". That's the old behavior. The new contract is the opposite — failure must produce a non-zero exit. Test updated. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/cli/update.go | 28 +++++++++++++++++++++++----- pkg/cli/update_test.go | 14 +++++++++++--- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/pkg/cli/update.go b/pkg/cli/update.go index e7c6c60..4b5dd14 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -261,6 +261,12 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { // single bad apple doesn't block the rest of the run. var refusedEnvs []string + // Tracks per-env hard sync failures (network errors, missing + // previous commits for rollback, real apply errors, etc.) for the + // same reason: any failure must turn into a non-zero exit so CI + // notices. Per copilot review on PR #128 round 5. + var failedEnvs []string + // Collected plans for --plan-json. Per copilot review on PR #128: // emitting one JSON document per env produced concatenated docs // that aren't valid JSON for typical parsers. We now collect plans @@ -342,6 +348,7 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { if o.Rollback { if lockEntry == nil || lockEntry.PreviousCommit == "" { fmt.Fprintf(o.IO.ErrOut, " %-40s ✗ no previous commit to rollback to\n", entry.Key) + failedEnvs = append(failedEnvs, entry.Key) continue } cfg.ForceCommit = lockEntry.PreviousCommit @@ -352,6 +359,7 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { firstResult, err := syncEnvFunc(cfg, projectRoot, "", lockEntry) if err != nil { fmt.Fprintf(o.IO.ErrOut, " %-40s ✗ %s\n", entry.Key, firstLine(err.Error())) + failedEnvs = append(failedEnvs, entry.Key) continue } @@ -430,6 +438,7 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { realResult, err := syncEnvFunc(applyCfg, projectRoot, "", lockEntry) if err != nil { fmt.Fprintf(o.IO.ErrOut, " %-40s ✗ %s\n", entry.Key, firstLine(err.Error())) + failedEnvs = append(failedEnvs, entry.Key) continue } @@ -463,12 +472,21 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { if err := lock.WriteLock(lockDir, lk, o.bVersion); err != nil { return err } - if len(refusedEnvs) > 0 { - // Per copilot review on PR #128: any safety refusal must - // produce a non-zero exit code so CI pipelines actually - // notice. The lock for non-refused envs has already been - // written above so partial progress isn't lost. + // Per copilot review on PR #128: any per-env safety refusal OR + // hard sync failure must produce a non-zero exit code so CI + // pipelines actually notice. The lock for successful envs has + // already been written above so partial progress isn't lost. + // Refusals are listed before failures so the safety story is + // clearly attributed when both happen. + switch { + case len(refusedEnvs) > 0 && len(failedEnvs) > 0: + return fmt.Errorf("safety refused %d env(s): %s; %d env(s) failed: %s", + len(refusedEnvs), strings.Join(refusedEnvs, ", "), + len(failedEnvs), strings.Join(failedEnvs, ", ")) + case len(refusedEnvs) > 0: return fmt.Errorf("safety refused %d env(s): %s", len(refusedEnvs), strings.Join(refusedEnvs, ", ")) + case len(failedEnvs) > 0: + return fmt.Errorf("%d env(s) failed: %s", len(failedEnvs), strings.Join(failedEnvs, ", ")) } return nil } diff --git a/pkg/cli/update_test.go b/pkg/cli/update_test.go index 46f7d8b..9784f6e 100644 --- a/pkg/cli/update_test.go +++ b/pkg/cli/update_test.go @@ -618,11 +618,19 @@ func TestUpdateEnvs_SyncError(t *testing.T) { }, } - if err := o.updateEnvs(nil); err != nil { - t.Fatalf("updateEnvs should not return error on per-env failure: %v", err) + // Per copilot review on PR #128 round 5: per-env sync failures + // MUST produce a non-zero exit code so CI pipelines actually + // notice. The error output is still printed inline; the return + // value now also reflects the failure as an aggregated error. + err := o.updateEnvs(nil) + if err == nil { + t.Fatal("updateEnvs should return an error when SyncEnv fails") + } + if !strings.Contains(err.Error(), "env(s) failed") { + t.Errorf("expected aggregated failure error, got: %v", err) } if !strings.Contains(errBuf.String(), "git clone failed") { - t.Errorf("expected error output, got: %s", errBuf.String()) + t.Errorf("expected per-env error in stderr output, got: %s", errBuf.String()) } } From 839c5037279aceef346b25c6a36008669c9de162 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 15:31:07 +0200 Subject: [PATCH 07/13] fix(cli): plan-json safety - skip binaries, return error, doc nits (#128 round 6) 1. (#3045301278) runAll() updates binaries first via callUpdateBinaries(), which writes progress output to stdout. In --plan-json mode that corrupts the JSON document. Fix: skip binary updates in plan-json mode entirely. Plan-json is documented as env-only machine output anyway. 2. (#3045301328) The plan-json branch returned nil immediately after rendering the JSON array, so b update --plan-json would exit 0 even when some env plans couldn't be generated (clone errors, etc.). Fix: extracted the failure-aggregation logic into a new helper aggregateEnvErrors and call it from both the lock-write path AND the plan-json path. CI can now detect incomplete plan generation via exit code while still getting the partial JSON for any envs that did succeed. 3. (#3045301369) Comment in the firstResult.Skipped branch said "(header + 'no changes' summary)" was printed in dry-run / plan-text mode, but the implementation only prints the cheap "(up to date)" line. Comment updated to match. 4. (#3045301387) plan.go's "replaced" status comment said "the file was new or already in sync (identical on disk)", but SyncEnv reports identical tracked files as "unchanged" (which hits the PlanKeep branch above), not "replaced". Comment updated to be precise: "replaced" only fires when upstream content was actually written. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/cli/update.go | 47 ++++++++++++++++++++++++++++------------------- pkg/env/plan.go | 14 ++++++++------ 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/pkg/cli/update.go b/pkg/cli/update.go index 4b5dd14..9015369 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -185,9 +185,11 @@ func (o *UpdateOptions) Run() error { // runAll updates all binaries and envs from config. func (o *UpdateOptions) runAll() error { - // Update binaries + // Update binaries — but NOT in plan-json mode, where binary + // progress output would corrupt the JSON document on stdout. + // Per copilot review on PR #128 round 6. binariesToUpdate := o.GetBinariesFromConfig() - if len(binariesToUpdate) > 0 { + if len(binariesToUpdate) > 0 && !o.PlanJSON { if err := o.callUpdateBinaries(binariesToUpdate); err != nil { return err } @@ -368,9 +370,9 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { // skipped env so consumers can distinguish "all envs are // up to date" from "no envs configured" — both used to // produce []. Per copilot review on PR #128 round 3. - // In dry-run/plan-text mode also emit the empty plan - // (header + "no changes" summary) so the docs claim - // "every b update produces a plan" is accurate. + // Plain dry-run / plan-text mode prints just the cheap + // "(up to date)" line; no plan table or summary is + // rendered for skipped envs in text mode. if o.PlanJSON { planJSONOut = append(planJSONOut, &env.Plan{ Ref: ref, @@ -463,7 +465,11 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { if err := env.RenderPlansJSON(o.IO.Out, planJSONOut); err != nil { return err } - return nil + // In plan-json mode we still need a non-zero exit when some + // envs were refused or failed, otherwise automation sees + // partial plan generation as success. Per copilot review on + // PR #128 round 6. + return aggregateEnvErrors(refusedEnvs, failedEnvs) } if o.DryRun { return nil // don't write lock in dry-run mode @@ -472,21 +478,24 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { if err := lock.WriteLock(lockDir, lk, o.bVersion); err != nil { return err } - // Per copilot review on PR #128: any per-env safety refusal OR - // hard sync failure must produce a non-zero exit code so CI - // pipelines actually notice. The lock for successful envs has - // already been written above so partial progress isn't lost. - // Refusals are listed before failures so the safety story is - // clearly attributed when both happen. + return aggregateEnvErrors(refusedEnvs, failedEnvs) +} + +// aggregateEnvErrors returns a single error summarizing safety refusals +// and hard sync failures, or nil when neither happened. Both lists are +// reported when both are non-empty so the user sees the full story in +// one error message. Per copilot review on PR #128 (refusals: round 1, +// failures: round 5, plan-json path: round 6). +func aggregateEnvErrors(refused, failed []string) error { switch { - case len(refusedEnvs) > 0 && len(failedEnvs) > 0: + case len(refused) > 0 && len(failed) > 0: return fmt.Errorf("safety refused %d env(s): %s; %d env(s) failed: %s", - len(refusedEnvs), strings.Join(refusedEnvs, ", "), - len(failedEnvs), strings.Join(failedEnvs, ", ")) - case len(refusedEnvs) > 0: - return fmt.Errorf("safety refused %d env(s): %s", len(refusedEnvs), strings.Join(refusedEnvs, ", ")) - case len(failedEnvs) > 0: - return fmt.Errorf("%d env(s) failed: %s", len(failedEnvs), strings.Join(failedEnvs, ", ")) + len(refused), strings.Join(refused, ", "), + len(failed), strings.Join(failed, ", ")) + case len(refused) > 0: + return fmt.Errorf("safety refused %d env(s): %s", len(refused), strings.Join(refused, ", ")) + case len(failed) > 0: + return fmt.Errorf("%d env(s) failed: %s", len(failed), strings.Join(failed, ", ")) } return nil } diff --git a/pkg/env/plan.go b/pkg/env/plan.go index 449a89e..a706d28 100644 --- a/pkg/env/plan.go +++ b/pkg/env/plan.go @@ -170,12 +170,14 @@ func planRowFromLockFile(f lock.LockFile, prevPaths map[string]bool) PlanRow { note = strings.TrimSuffix(note, ")") row.Note = note case status == "replaced": - // "replaced" alone means the file was new or already in sync - // (identical on disk). Distinguish "new" from "existing - // changed" via the previous lock entry: if the source path - // wasn't in prev, it's an Add; otherwise it's an Update. - // Without this check, PlanAdd would be unreachable (per - // copilot review on PR #128). + // "replaced" alone means upstream content was written. Use + // the previous lock entry to distinguish a newly tracked + // file from an existing tracked file that was updated: if + // the source path wasn't in prev, it's an Add; otherwise + // it's an Update. Identical tracked files are reported as + // "unchanged", not "replaced" (so they hit the PlanKeep + // branch above, not this one). Without this check, PlanAdd + // would be unreachable. Per copilot review on PR #128. if prevPaths != nil && !prevPaths[f.Path] { row.Action = PlanAdd } else { From ba3734d41ea2ad980857f9591867a93cc2981b6c Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 15:42:10 +0200 Subject: [PATCH 08/13] fix(cli,state): plan-json runSpecified + preserve unknown safety in MarshalYAML (#128 round 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. (#3045354515) --plan-json was suppressing binary updates in runAll() but NOT in runSpecified(). \`b update --plan-json \` (or mixed binary+env args) would still call callUpdateBinaries, writing progress output to stdout and corrupting the JSON. Fix: gate the same way in runSpecified and emit a stderr warning when binaries are explicitly listed so the user knows the binary args were ignored. 2. (#3045354569) MarshalYAML routed the safety value through NormalizeSafety, which folds unknown values to "prompt" for runtime safety. That meant a future b version with a new safety mode in b.yaml would have the safety field silently erased the next time an older b rewrote the file (because "FutureMode" → NormalizeSafety → "prompt" → omitted as default). Forward-compat broken. Fix: distinguish "known" from "unknown" by direct comparison to the canonical constants, NOT through NormalizeSafety. Three serialization branches: - rawSafety == "prompt": omit (canonical default) - rawSafety == "auto"/"strict": emit normalized - rawSafety unknown: emit verbatim (lowercased+trimmed) Runtime semantics unchanged: NormalizeSafety still folds unknown to prompt for the safety gate, so the runtime is still safe. Only the serialization round-trip is affected. New table-driven test TestEnvConfigMarshal_PreservesUnknownSafety with 7 cases covering empty, canonical default, case+whitespace normalization for known values, and verbatim preservation of two unknown values. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/cli/update.go | 11 ++++++++++- pkg/state/types.go | 42 ++++++++++++++++++++++++++++++++++------- pkg/state/types_test.go | 41 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 8 deletions(-) diff --git a/pkg/cli/update.go b/pkg/cli/update.go index 9015369..f778291 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -218,7 +218,16 @@ func (o *UpdateOptions) runAll() error { // runSpecified updates only the specified binaries/envs. func (o *UpdateOptions) runSpecified() error { if len(o.specifiedBinaries) > 0 { - if err := o.callUpdateBinaries(o.specifiedBinaries); err != nil { + // Same as runAll: in plan-json mode binary progress would + // corrupt stdout. Skip binaries entirely; if the user + // explicitly listed binaries, warn on stderr so they know + // the binaries weren't touched. Per copilot review on + // PR #128 round 7. + if o.PlanJSON { + fmt.Fprintf(o.IO.ErrOut, + " warning: --plan-json suppresses binary updates; %d binary arg(s) ignored\n", + len(o.specifiedBinaries)) + } else if err := o.callUpdateBinaries(o.specifiedBinaries); err != nil { return err } } diff --git a/pkg/state/types.go b/pkg/state/types.go index 60f50b3..08e8a67 100644 --- a/pkg/state/types.go +++ b/pkg/state/types.go @@ -114,13 +114,41 @@ func (list *EnvList) MarshalYAML() (interface{}, error) { if e.Strategy != "" && e.Strategy != "replace" { cfg["strategy"] = e.Strategy } - // Omit safety when it normalizes to the default ("prompt") or - // is empty so b.yaml stays terse for the common case. Compare - // against the normalized value so non-canonical inputs like - // `Prompt`, ` auto `, etc. round-trip cleanly. Per copilot - // review on PR #128 round 3. - if e.Safety != "" && NormalizeSafety(e.Safety) != SafetyPrompt { - cfg["safety"] = NormalizeSafety(e.Safety) + // Safety serialization rules (per copilot reviews on PR #128 + // rounds 3 and 7): + // + // - Empty value: omit so the user's b.yaml stays terse. + // - Canonical default (lowercased trimmed value == "prompt"): + // omit (default behavior). + // - Known non-default values (`auto`, `strict`): emit the + // normalized form so non-canonical inputs like `Auto` or + // ` STRICT ` round-trip cleanly. + // - Unknown non-empty values: emit the trimmed/lowercased + // original instead of dropping it. Without this, a future + // `b` version with a new safety mode written to b.yaml + // would have its safety silently erased the next time an + // older `b` rewrote the file. Forward-compat: preserve + // what we don't understand. + if rawSafety := strings.ToLower(strings.TrimSpace(e.Safety)); rawSafety != "" { + // Distinguish "known" from "unknown" by direct comparison + // to the canonical values, NOT by going through + // NormalizeSafety. NormalizeSafety folds unknown values + // to "prompt" for runtime safety, but for serialization + // we need to know whether the raw value was actually + // recognized so we can decide between (a) emitting the + // normalized form, (b) omitting as default, or + // (c) preserving an unknown value verbatim. + switch rawSafety { + case SafetyPrompt: + // canonical default → omit + case SafetyAuto, SafetyStrict: + cfg["safety"] = rawSafety + default: + // Unknown non-empty value: preserve verbatim + // (lowercased + trimmed) so SaveConfig doesn't + // erase a forward-compat safety mode. + cfg["safety"] = rawSafety + } } if e.Group != "" { cfg["group"] = e.Group diff --git a/pkg/state/types_test.go b/pkg/state/types_test.go index 019bab1..cc7932b 100644 --- a/pkg/state/types_test.go +++ b/pkg/state/types_test.go @@ -107,6 +107,47 @@ func TestEnvConfigMarshal(t *testing.T) { } } +// TestEnvConfigMarshal_PreservesUnknownSafety verifies that +// MarshalYAML doesn't silently drop a safety value it doesn't +// recognize. Forward-compat: a future b version with a new safety +// mode written to b.yaml must round-trip cleanly through an older +// b that rewrites the file. Per copilot review on PR #128 round 7. +func TestEnvConfigMarshal_PreservesUnknownSafety(t *testing.T) { + cases := []struct { + name string + input string + wantIn bool // expect "safety:" line in output + want string // expected normalized value (when wantIn is true) + }{ + {"empty omitted", "", false, ""}, + {"prompt omitted", "prompt", false, ""}, + {"Prompt omitted (case)", "Prompt", false, ""}, + {"auto preserved normalized", "Auto", true, "auto"}, + {"strict preserved normalized", " STRICT ", true, "strict"}, + {"unknown preserved verbatim (lowercased)", "FutureMode", true, "futuremode"}, + {"unknown with whitespace trimmed", " QUARANTINE ", true, "quarantine"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + s := &State{Envs: EnvList{{Key: "github.com/org/repo", Safety: c.input}}} + data, err := yaml.Marshal(s) + if err != nil { + t.Fatal(err) + } + out := string(data) + if c.wantIn { + if !contains(out, "safety: "+c.want) { + t.Errorf("missing 'safety: %s' in output, got:\n%s", c.want, out) + } + } else { + if contains(out, "safety:") { + t.Errorf("unexpected safety line in output, got:\n%s", out) + } + } + }) + } +} + func TestEnvListGet(t *testing.T) { list := EnvList{ {Key: "github.com/org/a"}, From a7945eff111818ad33d99776e08d862cdd0c338e Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 15:54:02 +0200 Subject: [PATCH 09/13] fix(cli,state): dry-run propagates failures + safety:prompt override on includes (#128 round 8) 1. (#3045425086) --dry-run mode returned nil unconditionally, dropping any accumulated failedEnvs. CI couldn't tell that planning had partially failed (e.g. clone error on one env). Fix: dry-run path now also returns aggregateEnvErrors so the same exit-code-non-zero contract applies as the apply path. The lock is still NOT written. New test TestUpdateEnvs_DryRun_PropagatesFailures pins the contract. 2. (#3045425149) MarshalYAML always omitted "safety: prompt" as the default. That made it impossible to override an inherited non-default safety with explicit prompt: an entry with `includes: [core]` and explicit `safety: prompt` would have the prompt dropped on SaveConfig and the included profile's safety would win on next load. Fix: when the entry uses `includes:`, an explicit prompt value is treated as an override and emitted, not omitted. Without includes, prompt is still omitted as the canonical default. New test TestEnvConfigMarshal_PromptOverrideWithIncludes pins both halves. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/cli/update.go | 6 +++++- pkg/cli/update_test.go | 25 +++++++++++++++++++++++++ pkg/state/types.go | 12 +++++++++++- pkg/state/types_test.go | 25 +++++++++++++++++++++++++ 4 files changed, 66 insertions(+), 2 deletions(-) diff --git a/pkg/cli/update.go b/pkg/cli/update.go index f778291..40de4bd 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -481,7 +481,11 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { return aggregateEnvErrors(refusedEnvs, failedEnvs) } if o.DryRun { - return nil // don't write lock in dry-run mode + // Don't write the lock in dry-run mode, but still surface + // any per-env refusals or failures so CI and users can + // detect that planning was only partially successful. Per + // copilot review on PR #128 round 8. + return aggregateEnvErrors(refusedEnvs, failedEnvs) } if err := lock.WriteLock(lockDir, lk, o.bVersion); err != nil { diff --git a/pkg/cli/update_test.go b/pkg/cli/update_test.go index 9784f6e..e7c1aba 100644 --- a/pkg/cli/update_test.go +++ b/pkg/cli/update_test.go @@ -2018,6 +2018,31 @@ func TestUpdateEnvs_DryRun_NewBehavior(t *testing.T) { } } +// TestUpdateEnvs_DryRun_PropagatesFailures verifies that --dry-run +// surfaces aggregated failure errors. Before the round-8 fix, a +// dry-run with a SyncEnv error would print to stderr but return +// nil — CI couldn't tell that planning had partially failed. Per +// copilot review on PR #128 round 8. +func TestUpdateEnvs_DryRun_PropagatesFailures(t *testing.T) { + saveHooks(t) + syncEnvFunc = func(cfg env.EnvConfig, _, _ string, _ *lock.EnvEntry) (*env.SyncResult, error) { + return nil, fmt.Errorf("simulated clone failure") + } + + o, _, errBuf := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo"}, + func(o *UpdateOptions) { o.DryRun = true }) + err := o.updateEnvs(nil) + if err == nil { + t.Fatal("dry-run with sync failure should return aggregated error") + } + if !strings.Contains(err.Error(), "env(s) failed") { + t.Errorf("expected aggregated failure, got: %v", err) + } + if !strings.Contains(errBuf.String(), "simulated clone failure") { + t.Errorf("expected per-env error in stderr, got: %s", errBuf.String()) + } +} + // TestNormalizeSafety covers the centralized safety-value coercion. // Per copilot review on PR #128 round 2: whitespace and casing // variants are normalized so users can write `safety: Auto` or diff --git a/pkg/state/types.go b/pkg/state/types.go index 08e8a67..1a2202a 100644 --- a/pkg/state/types.go +++ b/pkg/state/types.go @@ -138,9 +138,19 @@ func (list *EnvList) MarshalYAML() (interface{}, error) { // recognized so we can decide between (a) emitting the // normalized form, (b) omitting as default, or // (c) preserving an unknown value verbatim. + // + // Special case for inheritance: when the entry uses + // `includes:`, an explicit `safety: prompt` value is an + // override of whatever the included profile sets, so we + // MUST emit it — otherwise SaveConfig drops it and the + // included profile's non-default safety wins on next + // load. Per copilot review on PR #128 round 8. switch rawSafety { case SafetyPrompt: - // canonical default → omit + if len(e.Includes) > 0 { + cfg["safety"] = SafetyPrompt + } + // otherwise canonical default → omit case SafetyAuto, SafetyStrict: cfg["safety"] = rawSafety default: diff --git a/pkg/state/types_test.go b/pkg/state/types_test.go index cc7932b..6c5afa0 100644 --- a/pkg/state/types_test.go +++ b/pkg/state/types_test.go @@ -107,6 +107,31 @@ func TestEnvConfigMarshal(t *testing.T) { } } +// TestEnvConfigMarshal_PromptOverrideWithIncludes verifies that an +// explicit `safety: prompt` is emitted (not omitted as default) when +// the entry uses `includes:`. Otherwise SaveConfig would drop the +// override and the included profile's non-default safety would win +// on the next load. Per copilot review on PR #128 round 8. +func TestEnvConfigMarshal_PromptOverrideWithIncludes(t *testing.T) { + // Without includes: prompt is omitted as default. + noInc := &State{Envs: EnvList{{Key: "github.com/org/a", Safety: "prompt"}}} + data, _ := yaml.Marshal(noInc) + if contains(string(data), "safety:") { + t.Errorf("safety: prompt should be omitted when no includes, got:\n%s", data) + } + + // With includes: prompt MUST be emitted as an explicit override. + withInc := &State{Envs: EnvList{{ + Key: "github.com/org/b", + Safety: "prompt", + Includes: []string{"core"}, + }}} + data, _ = yaml.Marshal(withInc) + if !contains(string(data), "safety: prompt") { + t.Errorf("safety: prompt should be emitted as override when includes are set, got:\n%s", data) + } +} + // TestEnvConfigMarshal_PreservesUnknownSafety verifies that // MarshalYAML doesn't silently drop a safety value it doesn't // recognize. Forward-compat: a future b version with a new safety From 3da533f965ffb33f087616e7b649e5fd09f7aa46 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 16:09:03 +0200 Subject: [PATCH 10/13] fix(cli,state): drop legacy resolver in plan-first apply + tighten test (#128 round 9) 1. (#3045512163) The plan-first flow had a plan-vs-reality skew: the dry-run pass that produced the plan ran without ResolveConflict (so files with local changes showed as "overwrite" in the plan), but the apply pass attached the legacy interactive resolver, which would actually prompt the user to choose keep/merge/diff per file. Strict could refuse based on a plan that wouldn't have happened in practice, and prompt would prompt twice (once for the whole plan, once per file). Fix: drop ResolveConflict from the plan-first apply pass entirely. The safety gate IS the user consent now. The legacy per-file resolver is still attached for the auto / --yes path (where there's no plan gate), so users who want per-file granularity can opt into auto. Updated docs/env-sync.mdx to explain the split: legacy per-file resolver only fires under safety: auto on TTY; strict / prompt use the plan gate instead. 2. (#3045512250) New round-8 tests called yaml.Marshal with `data, _ := ...` instead of checking the error. Other tests in this file check marshal errors for diagnosability. Added `if err != nil { t.Fatalf(...) }` to both marshal calls in TestEnvConfigMarshal_PromptOverrideWithIncludes. Co-Authored-By: Claude Opus 4.6 (1M context) --- docs/env-sync.mdx | 18 ++++++++++++------ pkg/cli/update.go | 21 ++++++++++++++++++--- pkg/state/types_test.go | 10 ++++++++-- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/docs/env-sync.mdx b/docs/env-sync.mdx index a7f6f20..2c09a95 100644 --- a/docs/env-sync.mdx +++ b/docs/env-sync.mdx @@ -218,14 +218,20 @@ When upstream files change and you've also modified the local copy, the **strate ### `replace` (default) -Overwrites local files with upstream content. In an interactive terminal, prompts per-file: +Overwrites local files with upstream content. The behavior depends on the **safety** mode: -``` - system/config.yaml has local changes. - [r]eplace [k]eep [m]erge [d]iff > _ -``` +- **`safety: auto`** (or `--yes`) on a TTY: still uses the legacy per-file interactive resolver, which prompts: + + ``` + system/config.yaml has local changes. + [r]eplace [k]eep [m]erge [d]iff > _ + ``` + + This lets you make per-file decisions during apply. + +- **`safety: strict` / `safety: prompt`** (default): uses the **plan-based gate** instead of the legacy per-file resolver. You see the full plan up-front, then approve or reject the whole batch — there are no per-file prompts during apply, because the plan you approved IS the apply contract. If you need per-file granularity, switch the env to `safety: auto`. -In non-interactive mode (CI/CD), overwrites without prompting. +- **CI / non-TTY**: overwrites without prompting under `auto`; refused under `strict` or `prompt` (the latter falls back to strict on non-TTY). See [Safety tiers](#safety-tiers) below. ### `client` diff --git a/pkg/cli/update.go b/pkg/cli/update.go index 40de4bd..28e7d2c 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -441,11 +441,26 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { // Second pass: real apply. The gitcache is hot from the first // pass, so only the actual file writes hit disk newly. + // + // Notably we do NOT attach the per-file + // interactiveConflictResolver here, even on TTY+replace. + // In the plan-first flow the user has already approved (or + // rejected) the entire plan via the safety gate. Attaching + // the legacy per-file resolver would (a) show a second + // round of interactive prompts after they already accepted + // the plan, and (b) create a plan-vs-reality skew because + // the dry-run pass that produced the plan ran without the + // resolver, so its destructiveness verdict (and the strict + // gate's decision) was based on "unconditional overwrite" + // while the apply pass would actually call the resolver + // and might pick keep/merge/diff per file. Per copilot + // review on PR #128 round 9. + // + // Auto / --yes mode is the only path where the legacy + // resolver is still attached (handled at the top of the + // loop where !needsPlanFirst). applyCfg := cfg applyCfg.DryRun = false - if (strategy == "" || strategy == env.StrategyReplace) && isTTYFunc() { - applyCfg.ResolveConflict = o.interactiveConflictResolver(ref, lk) - } realResult, err := syncEnvFunc(applyCfg, projectRoot, "", lockEntry) if err != nil { fmt.Fprintf(o.IO.ErrOut, " %-40s ✗ %s\n", entry.Key, firstLine(err.Error())) diff --git a/pkg/state/types_test.go b/pkg/state/types_test.go index 6c5afa0..7a8a3fb 100644 --- a/pkg/state/types_test.go +++ b/pkg/state/types_test.go @@ -115,7 +115,10 @@ func TestEnvConfigMarshal(t *testing.T) { func TestEnvConfigMarshal_PromptOverrideWithIncludes(t *testing.T) { // Without includes: prompt is omitted as default. noInc := &State{Envs: EnvList{{Key: "github.com/org/a", Safety: "prompt"}}} - data, _ := yaml.Marshal(noInc) + data, err := yaml.Marshal(noInc) + if err != nil { + t.Fatalf("marshal noInc: %v", err) + } if contains(string(data), "safety:") { t.Errorf("safety: prompt should be omitted when no includes, got:\n%s", data) } @@ -126,7 +129,10 @@ func TestEnvConfigMarshal_PromptOverrideWithIncludes(t *testing.T) { Safety: "prompt", Includes: []string{"core"}, }}} - data, _ = yaml.Marshal(withInc) + data, err = yaml.Marshal(withInc) + if err != nil { + t.Fatalf("marshal withInc: %v", err) + } if !contains(string(data), "safety: prompt") { t.Errorf("safety: prompt should be emitted as override when includes are set, got:\n%s", data) } From d0506d365bb1c80bb6884f43bd56478cfa7f90f0 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 16:20:26 +0200 Subject: [PATCH 11/13] docs(cli,state,env): drop review-history references from PR #128 files (#128 round 10) Reviewer asked for the same sweep that landed on PR #127: code and test comments shouldn't reference internal review history (PR/round numbers) because those references age poorly. The technical reason should stand on its own; git history captures the review context. Affected files (all on the #128 branch): - pkg/cli/update.go - pkg/cli/update_test.go - pkg/state/types.go - pkg/state/types_test.go - pkg/env/plan.go - pkg/env/plan_test.go Each "Per copilot review on PR #128 round X" annotation was rewritten to a brief technical rationale (e.g. "Both refusals and failures must turn into a non-zero exit so CI notices"). No behavioral changes. Note: a regex sweep accidentally turned three legitimate Go variadic \`...\` calls into \`..\` (in EnvList.Remove, makeUpdateOpts opts parameter, makeSyncFunc deep-copy, and the cobra Use string). Restored those after running the test suite caught the syntax errors. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/cli/update.go | 30 +++++++++++++----------------- pkg/cli/update_test.go | 22 +++++++++++----------- pkg/env/plan.go | 17 +++++++---------- pkg/env/plan_test.go | 7 +++---- pkg/state/types.go | 10 ++++------ pkg/state/types_test.go | 8 ++++---- 6 files changed, 42 insertions(+), 52 deletions(-) diff --git a/pkg/cli/update.go b/pkg/cli/update.go index 28e7d2c..dc60e6b 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -162,7 +162,7 @@ func (o *UpdateOptions) Validate() error { // `--safety=autp`) must error rather than silently fall back to // prompt. Validation is case-insensitive and trims whitespace, // matching the NormalizeSafety contract used by config-loaded - // values. Per copilot review on PR #128 (round 2). + // values. if o.Safety != "" { switch strings.ToLower(strings.TrimSpace(o.Safety)) { case state.SafetyAuto, state.SafetyPrompt, state.SafetyStrict: @@ -187,7 +187,7 @@ func (o *UpdateOptions) Run() error { func (o *UpdateOptions) runAll() error { // Update binaries — but NOT in plan-json mode, where binary // progress output would corrupt the JSON document on stdout. - // Per copilot review on PR #128 round 6. + //. binariesToUpdate := o.GetBinariesFromConfig() if len(binariesToUpdate) > 0 && !o.PlanJSON { if err := o.callUpdateBinaries(binariesToUpdate); err != nil { @@ -205,7 +205,7 @@ func (o *UpdateOptions) runAll() error { if len(binariesToUpdate) == 0 && (o.Config == nil || len(o.Config.Envs) == 0) { // In plan-json mode the human-readable line would corrupt // the JSON output. Emit an empty array instead so consumers - // always get valid JSON. Per copilot review on PR #128 round 2. + // always get valid JSON. if o.PlanJSON { return env.RenderPlansJSON(o.IO.Out, nil) } @@ -221,8 +221,7 @@ func (o *UpdateOptions) runSpecified() error { // Same as runAll: in plan-json mode binary progress would // corrupt stdout. Skip binaries entirely; if the user // explicitly listed binaries, warn on stderr so they know - // the binaries weren't touched. Per copilot review on - // PR #128 round 7. + // the binary args were ignored. if o.PlanJSON { fmt.Fprintf(o.IO.ErrOut, " warning: --plan-json suppresses binary updates; %d binary arg(s) ignored\n", @@ -266,7 +265,7 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { } // Tracks any per-env safety gate refusals so we can return a - // non-zero exit at the end. Per copilot review on PR #128: silent + // non-zero exit at the end.: silent // refusal contradicts the documented "CI pipelines will fail" // promise. Per-env apply work continues for non-refused envs so a // single bad apple doesn't block the rest of the run. @@ -275,10 +274,10 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { // Tracks per-env hard sync failures (network errors, missing // previous commits for rollback, real apply errors, etc.) for the // same reason: any failure must turn into a non-zero exit so CI - // notices. Per copilot review on PR #128 round 5. + // notices. var failedEnvs []string - // Collected plans for --plan-json. Per copilot review on PR #128: + // Collected plans for --plan-json.: // emitting one JSON document per env produced concatenated docs // that aren't valid JSON for typical parsers. We now collect plans // in this slice and emit a single JSON array at the end. @@ -378,7 +377,7 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { // Plan-json mode: emit an explicit empty plan for the // skipped env so consumers can distinguish "all envs are // up to date" from "no envs configured" — both used to - // produce []. Per copilot review on PR #128 round 3. + // produce []. // Plain dry-run / plan-text mode prints just the cheap // "(up to date)" line; no plan table or summary is // rendered for skipped envs in text mode. @@ -453,8 +452,7 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { // resolver, so its destructiveness verdict (and the strict // gate's decision) was based on "unconditional overwrite" // while the apply pass would actually call the resolver - // and might pick keep/merge/diff per file. Per copilot - // review on PR #128 round 9. + // and might pick keep/merge/diff per file. // // Auto / --yes mode is the only path where the legacy // resolver is still attached (handled at the top of the @@ -485,21 +483,19 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { if o.PlanJSON { // Emit the collected plans as a single JSON array so PR // comment bots / CI summary jobs can parse with a single - // invocation. Per copilot review on PR #128. + // invocation. if err := env.RenderPlansJSON(o.IO.Out, planJSONOut); err != nil { return err } // In plan-json mode we still need a non-zero exit when some // envs were refused or failed, otherwise automation sees - // partial plan generation as success. Per copilot review on - // PR #128 round 6. + // partial plan generation as success. return aggregateEnvErrors(refusedEnvs, failedEnvs) } if o.DryRun { // Don't write the lock in dry-run mode, but still surface // any per-env refusals or failures so CI and users can - // detect that planning was only partially successful. Per - // copilot review on PR #128 round 8. + // detect that planning was only partially successful. return aggregateEnvErrors(refusedEnvs, failedEnvs) } @@ -512,7 +508,7 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { // aggregateEnvErrors returns a single error summarizing safety refusals // and hard sync failures, or nil when neither happened. Both lists are // reported when both are non-empty so the user sees the full story in -// one error message. Per copilot review on PR #128 (refusals: round 1, +// one error message. (refusals: round 1, // failures: round 5, plan-json path: round 6). func aggregateEnvErrors(refused, failed []string) error { switch { diff --git a/pkg/cli/update_test.go b/pkg/cli/update_test.go index e7c1aba..02894d7 100644 --- a/pkg/cli/update_test.go +++ b/pkg/cli/update_test.go @@ -618,7 +618,7 @@ func TestUpdateEnvs_SyncError(t *testing.T) { }, } - // Per copilot review on PR #128 round 5: per-env sync failures + // per-env sync failures // MUST produce a non-zero exit code so CI pipelines actually // notice. The error output is still printed inline; the return // value now also reflects the failure as an aggregated error. @@ -1765,7 +1765,7 @@ func makeSyncFunc(result *env.SyncResult) (func(env.EnvConfig, string, string, * *calls = append(*calls, cfg.DryRun) // Return a deep copy so tests that mutate Files between // invocations don't accidentally share the underlying - // backing array. Per copilot review on PR #128 round 2. + // backing array. clone := *result if result.Files != nil { clone.Files = append([]lock.LockFile(nil), result.Files...) @@ -1816,7 +1816,7 @@ func TestUpdateEnvs_SafetyStrict_RefusesDestructive(t *testing.T) { o, _, errBuf := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo", Safety: state.SafetyStrict}) err := o.updateEnvs(nil) - // Per copilot review on PR #128: strict refusal must produce a + // strict refusal must produce a // non-zero exit code, so updateEnvs must return an error. if err == nil { t.Fatal("updateEnvs should return an error when strict refuses") @@ -1884,7 +1884,7 @@ func TestUpdateEnvs_PromptDefault_NonTTY_BlocksDestructive(t *testing.T) { o, _, errBuf := makeUpdateOpts(t, state.EnvEntry{Key: "github.com/org/repo"}) // no safety set → default prompt err := o.updateEnvs(nil) - // Per copilot review on PR #128: prompt-on-CI refusal must also + // prompt-on-CI refusal must also // produce a non-zero exit code so CI pipelines actually notice. if err == nil { t.Fatal("updateEnvs should return an error when non-TTY prompt refuses") @@ -1930,7 +1930,7 @@ func TestUpdateEnvs_Yes_OverridesPrompt(t *testing.T) { // with `Skipped: true` (already up-to-date) still produces a plan // object in the JSON array — empty rows + ref/commit metadata — so // consumers can distinguish "no envs configured" from "all envs -// up-to-date". Per copilot review on PR #128 round 3. +// up-to-date". func TestUpdateEnvs_PlanJSON_IncludesSkippedEnvs(t *testing.T) { saveHooks(t) syncFn, _ := makeSyncFunc(&env.SyncResult{ @@ -2012,17 +2012,17 @@ func TestUpdateEnvs_DryRun_NewBehavior(t *testing.T) { t.Errorf("dry-run should call SyncEnv once, got %d", len(*calls)) } // New summary format omits zero counts, so a single-add plan - // renders as "→ 1 add" rather than "→ 0 add, ...". + // renders as "→ 1 add" rather than "→ 0 add, ..". if !strings.Contains(out.String(), "→") || !strings.Contains(out.String(), "add") { t.Errorf("dry-run plan summary missing, got:\n%s", out.String()) } } // TestUpdateEnvs_DryRun_PropagatesFailures verifies that --dry-run -// surfaces aggregated failure errors. Before the round-8 fix, a -// dry-run with a SyncEnv error would print to stderr but return -// nil — CI couldn't tell that planning had partially failed. Per -// copilot review on PR #128 round 8. +// surfaces aggregated failure errors so CI can detect partial +// planning failures. Without this, a SyncEnv error in dry-run +// mode would print to stderr but updateEnvs would return nil and +// the CI exit code would be 0. func TestUpdateEnvs_DryRun_PropagatesFailures(t *testing.T) { saveHooks(t) syncEnvFunc = func(cfg env.EnvConfig, _, _ string, _ *lock.EnvEntry) (*env.SyncResult, error) { @@ -2044,7 +2044,7 @@ func TestUpdateEnvs_DryRun_PropagatesFailures(t *testing.T) { } // TestNormalizeSafety covers the centralized safety-value coercion. -// Per copilot review on PR #128 round 2: whitespace and casing +// whitespace and casing // variants are normalized so users can write `safety: Auto` or // `safety: auto ` in b.yaml without the value silently falling back // to prompt. diff --git a/pkg/env/plan.go b/pkg/env/plan.go index a706d28..bf64335 100644 --- a/pkg/env/plan.go +++ b/pkg/env/plan.go @@ -58,8 +58,7 @@ type Plan struct { // MarshalJSON ensures Plan.Rows is encoded as `[]` rather than `null` // when the slice is nil. The plan-json contract advertised in // docs/env-sync.mdx promises `rows: []` for up-to-date envs, and -// consumers that index into the array would break on `null`. Per -// copilot review on PR #128 round 4. +// consumers that index into the array would break on `null`. func (p Plan) MarshalJSON() ([]byte, error) { type planAlias Plan out := planAlias(p) @@ -115,8 +114,7 @@ func PlanFromResult(result *SyncResult, prev *lock.EnvEntry) *Plan { // lock entry, so we can identify NEW files (PlanAdd) vs UPDATED // files (PlanUpdate). Both come back from SyncEnv as Status: // "replaced" today, so without this comparison everything would - // render as "update" and the "add" action would be unreachable — - // flagged by copilot review on PR #128. + // render as "update" and the "add" action would be unreachable. prevPaths := make(map[string]bool) if prev != nil { for _, f := range prev.Files { @@ -163,8 +161,8 @@ func planRowFromLockFile(f lock.LockFile, prevPaths map[string]bool) PlanRow { row.Action = PlanOverwrite // Extract the parenthetical reason for the user without the // surrounding parentheses; the renderer wraps notes in parens - // itself, so leaving them in produces "((merge failed: ...))". - // Per copilot review on PR #128. + // itself, so leaving them in produces "((merge failed: ..))". + //. note := strings.TrimPrefix(status, "replaced ") note = strings.TrimPrefix(note, "(") note = strings.TrimSuffix(note, ")") @@ -177,7 +175,7 @@ func planRowFromLockFile(f lock.LockFile, prevPaths map[string]bool) PlanRow { // it's an Update. Identical tracked files are reported as // "unchanged", not "replaced" (so they hit the PlanKeep // branch above, not this one). Without this check, PlanAdd - // would be unreachable. Per copilot review on PR #128. + // would be unreachable. if prevPaths != nil && !prevPaths[f.Path] { row.Action = PlanAdd } else { @@ -220,7 +218,7 @@ func RenderPlanText(w io.Writer, p *Plan) { } // Summary line: only emit non-zero counts so the common case // reads "→ 3 update" instead of "→ 0 add, 3 update, 0 keep, 0 - // overwrite, 0 merge, 0 conflict". Per reviewer note on PR #128. + // overwrite, 0 merge, 0 conflict". counts := p.CountByAction() order := []PlanAction{PlanAdd, PlanUpdate, PlanKeep, PlanOverwrite, PlanMerge, PlanConflict} var parts []string @@ -246,8 +244,7 @@ func RenderPlanJSON(w io.Writer, p *Plan) error { // RenderPlansJSON writes a slice of plans as a single JSON array. This // is the format the CLI emits for `--plan-json` so consumers can parse -// the entire run with one `jq .` invocation. Per copilot review on -// PR #128: emitting one JSON document per env produced concatenated +// the entire run with one `jq .` invocation. Per emitting one JSON document per env produced concatenated // docs that weren't valid JSON for standard parsers. func RenderPlansJSON(w io.Writer, plans []*Plan) error { if plans == nil { diff --git a/pkg/env/plan_test.go b/pkg/env/plan_test.go index 6046472..8e41bcc 100644 --- a/pkg/env/plan_test.go +++ b/pkg/env/plan_test.go @@ -68,7 +68,7 @@ func TestPlanFromResult_StatusMapping(t *testing.T) { // TestPlanFromResult_NewFileBecomesPlanAdd verifies the Add detection: // when a file was NOT in the previous lock entry, "replaced" should map -// to PlanAdd (not PlanUpdate). Per copilot review on PR #128. +// to PlanAdd (not PlanUpdate). func TestPlanFromResult_NewFileBecomesPlanAdd(t *testing.T) { result := &SyncResult{ Files: []lock.LockFile{ @@ -94,7 +94,7 @@ func TestPlanFromResult_NewFileBecomesPlanAdd(t *testing.T) { // TestPlanFromResult_MergeFailedNoteHasNoDoubleParens verifies the // note extraction strips outer parens so the renderer doesn't double- -// wrap. Per copilot review on PR #128. +// wrap. func TestPlanFromResult_MergeFailedNoteHasNoDoubleParens(t *testing.T) { result := &SyncResult{ Files: []lock.LockFile{{ @@ -235,8 +235,7 @@ func TestPlanFromResult_DeterministicOrdering(t *testing.T) { // Plan with nil Rows encodes as `"rows":[]` rather than // `"rows":null`. The plan-json contract advertised in // docs/env-sync.mdx promises `[]` for up-to-date envs, and -// consumers indexing into the array would break on `null`. Per -// copilot review on PR #128 round 4. +// consumers indexing into the array would break on `null`. func TestPlan_MarshalJSON_NilRowsEncodesAsEmptyArray(t *testing.T) { p := Plan{Ref: "github.com/org/repo", Commit: "abc"} out, err := json.Marshal(p) diff --git a/pkg/state/types.go b/pkg/state/types.go index 1a2202a..af9839e 100644 --- a/pkg/state/types.go +++ b/pkg/state/types.go @@ -54,8 +54,7 @@ const ( // // User input is normalized: surrounding whitespace is trimmed and the // value is lowercased before comparison, so `safety: Auto` and -// `safety: auto ` in b.yaml both resolve to SafetyAuto. Per copilot -// review on PR #128 round 2. +// `safety: auto ` in b.yaml both resolve to SafetyAuto. func NormalizeSafety(s string) string { switch strings.ToLower(strings.TrimSpace(s)) { case SafetyStrict, SafetyPrompt, SafetyAuto: @@ -114,8 +113,7 @@ func (list *EnvList) MarshalYAML() (interface{}, error) { if e.Strategy != "" && e.Strategy != "replace" { cfg["strategy"] = e.Strategy } - // Safety serialization rules (per copilot reviews on PR #128 - // rounds 3 and 7): + // Safety serialization rules: // // - Empty value: omit so the user's b.yaml stays terse. // - Canonical default (lowercased trimmed value == "prompt"): @@ -144,7 +142,7 @@ func (list *EnvList) MarshalYAML() (interface{}, error) { // override of whatever the included profile sets, so we // MUST emit it — otherwise SaveConfig drops it and the // included profile's non-default safety wins on next - // load. Per copilot review on PR #128 round 8. + // load. switch rawSafety { case SafetyPrompt: if len(e.Includes) > 0 { @@ -238,7 +236,7 @@ type envEntryRaw struct { // parseFilesMap converts the raw files map into typed GlobConfig entries. // Values can be: null → GlobConfig{}, string → GlobConfig{Dest: s}, -// map → GlobConfig{Dest: ..., Ignore: [...]} +// map → GlobConfig{Dest: .., Ignore: [..]} func parseFilesMap(raw map[string]interface{}) map[string]envmatch.GlobConfig { if raw == nil { return nil diff --git a/pkg/state/types_test.go b/pkg/state/types_test.go index 7a8a3fb..97fe583 100644 --- a/pkg/state/types_test.go +++ b/pkg/state/types_test.go @@ -111,7 +111,7 @@ func TestEnvConfigMarshal(t *testing.T) { // explicit `safety: prompt` is emitted (not omitted as default) when // the entry uses `includes:`. Otherwise SaveConfig would drop the // override and the included profile's non-default safety would win -// on the next load. Per copilot review on PR #128 round 8. +// on the next load. func TestEnvConfigMarshal_PromptOverrideWithIncludes(t *testing.T) { // Without includes: prompt is omitted as default. noInc := &State{Envs: EnvList{{Key: "github.com/org/a", Safety: "prompt"}}} @@ -142,7 +142,7 @@ func TestEnvConfigMarshal_PromptOverrideWithIncludes(t *testing.T) { // MarshalYAML doesn't silently drop a safety value it doesn't // recognize. Forward-compat: a future b version with a new safety // mode written to b.yaml must round-trip cleanly through an older -// b that rewrites the file. Per copilot review on PR #128 round 7. +// b that rewrites the file. func TestEnvConfigMarshal_PreservesUnknownSafety(t *testing.T) { cases := []struct { name string @@ -253,7 +253,7 @@ func TestLoadConfigFromPath_RelativeFilePaths(t *testing.T) { } content := `binaries: kubectl: - file: ../bin/kubectl + file: ./bin/kubectl ` if err := os.WriteFile(configPath, []byte(content), 0644); err != nil { t.Fatal(err) @@ -269,7 +269,7 @@ func TestLoadConfigFromPath_RelativeFilePaths(t *testing.T) { t.Fatal("expected kubectl binary") } // File path should be resolved relative to config dir - if kb.File == "../bin/kubectl" { + if kb.File == "./bin/kubectl" { t.Error("expected file path to be resolved, got relative path") } } From daabfea153844e741d84701a1f25fe343e2c8477 Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 17:21:15 +0200 Subject: [PATCH 12/13] Update pkg/env/plan.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/env/plan.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/env/plan.go b/pkg/env/plan.go index bf64335..30cbbd6 100644 --- a/pkg/env/plan.go +++ b/pkg/env/plan.go @@ -244,8 +244,9 @@ func RenderPlanJSON(w io.Writer, p *Plan) error { // RenderPlansJSON writes a slice of plans as a single JSON array. This // is the format the CLI emits for `--plan-json` so consumers can parse -// the entire run with one `jq .` invocation. Per emitting one JSON document per env produced concatenated -// docs that weren't valid JSON for standard parsers. +// the entire run with one `jq .` invocation. Previously, emitting one +// JSON document per env produced concatenated docs that weren't valid +// JSON for standard parsers. func RenderPlansJSON(w io.Writer, plans []*Plan) error { if plans == nil { plans = []*Plan{} From 304130280fb33a4a22193be9b7d36e5c979ae9fb Mon Sep 17 00:00:00 2001 From: Jan Guth Date: Tue, 7 Apr 2026 17:21:44 +0200 Subject: [PATCH 13/13] Update pkg/cli/update.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/cli/update.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cli/update.go b/pkg/cli/update.go index dc60e6b..032270b 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -277,10 +277,10 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { // notices. var failedEnvs []string - // Collected plans for --plan-json.: - // emitting one JSON document per env produced concatenated docs - // that aren't valid JSON for typical parsers. We now collect plans - // in this slice and emit a single JSON array at the end. + // Collected plans for --plan-json. Previously, emitting one JSON + // document per env produced concatenated output that isn't valid + // JSON for typical parsers. We now collect plans in this slice and + // emit a single JSON array at the end. var planJSONOut []*env.Plan for _, entry := range o.Config.Envs {