diff --git a/pkg/cli/update.go b/pkg/cli/update.go index 032270b..35597ba 100644 --- a/pkg/cli/update.go +++ b/pkg/cli/update.go @@ -183,6 +183,22 @@ func (o *UpdateOptions) Run() error { return o.runAll() } +// effectiveDryRun reports whether this update invocation should be +// treated as dry-run by callers that route through this helper. +// `--dry-run` is the obvious case. `--plan-json` is also dry-run-like +// because the user only wants plan output and side effects such as +// file writes, hooks, or lock writes would be surprising. +// +// Today the helper centralizes: +// - the per-env `cfg.DryRun` decision in `updateEnvs` +// - the lock-write suppression at the end of `updateEnvs` +// +// New dry-run-like flags should be added here so future code paths +// that rely on this helper interpret them consistently. +func (o *UpdateOptions) effectiveDryRun() bool { + return o.DryRun || o.PlanJSON +} + // runAll updates all binaries and envs from config. func (o *UpdateOptions) runAll() error { // Update binaries — but NOT in plan-json mode, where binary @@ -317,8 +333,10 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { if o.Safety != "" { safety = state.NormalizeSafety(o.Safety) } - // --plan-json implies --dry-run; the user only wants the plan. - isDryRun := o.DryRun || o.PlanJSON + // `--plan-json` implies `--dry-run` — the helper hides this + // dependency from the rest of the loop so future dry-run-like + // flags only need to be added in one place. + isDryRun := o.effectiveDryRun() // 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 @@ -492,10 +510,12 @@ func (o *UpdateOptions) updateEnvs(refs []string) error { // 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. + if o.effectiveDryRun() { + // Don't write the lock in any dry-run-like mode, but still + // surface any per-env refusals or failures so CI and users + // can detect that planning was only partially successful. + // Routed through effectiveDryRun() so future dry-run-like + // flags get the same lock-write suppression for free. return aggregateEnvErrors(refusedEnvs, failedEnvs) } @@ -550,7 +570,8 @@ func (o *UpdateOptions) gateApply(safety string, plan *env.Plan, isDryRun bool) return false, nil } - destructive := plan.HasDestructive() + destructiveRows := plan.DestructiveRows() + destructive := len(destructiveRows) > 0 switch safety { case state.SafetyAuto: @@ -560,7 +581,8 @@ func (o *UpdateOptions) gateApply(safety string, plan *env.Plan, isDryRun bool) 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 false, destructiveRefusalError("strict safety", destructiveRows, + "use --safety=prompt or --safety=auto to apply, or --dry-run to preview") } return true, nil @@ -572,7 +594,8 @@ func (o *UpdateOptions) gateApply(safety string, plan *env.Plan, isDryRun bool) // 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 false, destructiveRefusalError("prompt safety on non-TTY", destructiveRows, + "re-run with --yes or --safety=auto, set safety: auto in b.yaml, or --dry-run to preview") } return true, nil } @@ -585,6 +608,49 @@ func (o *UpdateOptions) gateApply(safety string, plan *env.Plan, isDryRun bool) return false, fmt.Errorf("unknown safety value %q", safety) } +// destructiveRefusalError builds the user-facing error string for a +// safety gate refusal. The message has three parts to make CI failure +// triage fast: +// +// 1. Which gate refused (so users know what to flip). +// 2. A breakdown of WHAT is destructive: count by action type, plus +// the first row's path so the user has a concrete file to look at +// without scrolling back through the printed plan. +// 3. The recovery hint (which flag/setting to use). +// +// Example: +// +// strict safety: plan contains 2 overwrite, 1 conflict +// (first: hetzner/legacy.yaml) — use --safety=prompt or +// --safety=auto to apply, or --dry-run to preview +// +// Per N1 from PR #128 reviewer feedback. +func destructiveRefusalError(gate string, rows []env.PlanRow, recovery string) error { + if len(rows) == 0 { + return fmt.Errorf("%s: plan contains destructive changes — %s", gate, recovery) + } + // Count by action. + var overwrite, conflict int + for _, r := range rows { + switch r.Action { + case env.PlanOverwrite: + overwrite++ + case env.PlanConflict: + conflict++ + } + } + var parts []string + if overwrite > 0 { + parts = append(parts, fmt.Sprintf("%d overwrite", overwrite)) + } + if conflict > 0 { + parts = append(parts, fmt.Sprintf("%d conflict", conflict)) + } + breakdown := strings.Join(parts, ", ") + first := rows[0].Dest + return fmt.Errorf("%s: plan contains %s (first: %s) — %s", gate, breakdown, first, recovery) +} + // confirmApply prompts the user with a y/N question. Default is N (safer). func (o *UpdateOptions) confirmApply(destructive bool) bool { r := o.stdinReader diff --git a/pkg/cli/update_test.go b/pkg/cli/update_test.go index 02894d7..03dd3d5 100644 --- a/pkg/cli/update_test.go +++ b/pkg/cli/update_test.go @@ -1832,6 +1832,17 @@ func TestUpdateEnvs_SafetyStrict_RefusesDestructive(t *testing.T) { if !strings.Contains(errBuf.String(), "strict safety") { t.Errorf("expected strict refusal message, got: %q", errBuf.String()) } + // The improved error message includes a count of destructive + // rows AND the first destructive row's path so users can find + // the offending file without scrolling. The fixture above uses + // "a.yaml" with "replaced (local changes overwritten)" which + // maps to PlanOverwrite. Per N1 from PR #128 reviewer feedback. + if !strings.Contains(errBuf.String(), "1 overwrite") { + t.Errorf("expected destructive count in error message, got: %q", errBuf.String()) + } + if !strings.Contains(errBuf.String(), "first: a.yaml") { + t.Errorf("expected first destructive path in error message, got: %q", errBuf.String()) + } } // TestUpdateEnvs_SafetyStrict_AppliesNonDestructive: strict ALLOWS plans diff --git a/pkg/env/plan.go b/pkg/env/plan.go index 30cbbd6..765bc2e 100644 --- a/pkg/env/plan.go +++ b/pkg/env/plan.go @@ -78,6 +78,21 @@ func (p *Plan) HasDestructive() bool { return false } +// DestructiveRows returns every plan row whose action is destructive +// (currently `overwrite` or `conflict`). Rows are returned in the +// same order they appear in Plan.Rows, which PlanFromResult sorts by +// destination path — so callers that pin "the first destructive row" +// for diagnostic messages get a stable, dest-sorted choice. +func (p *Plan) DestructiveRows() []PlanRow { + var out []PlanRow + for _, r := range p.Rows { + if r.Action.IsDestructive() { + out = append(out, r) + } + } + return out +} + // 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 {