feat(cli): plan-json polish — destructive row in error + effectiveDryRun helper#132
Merged
Conversation
…Run helper Two follow-ups from PR #128 reviewer notes (N1, N2): N1. Better breaking-change error messages for safety refusals. The previous "plan contains destructive changes" message was opaque — CI maintainers reading "build broken: b update failed" would have to scroll back through the printed plan to find which file is the problem. Fix: new helper destructiveRefusalError that builds a message with three parts: which gate refused, a count breakdown (e.g. "2 overwrite, 1 conflict"), and the first destructive row's path so users have a concrete file to look at without scrolling. 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 New env.Plan.DestructiveRows() helper returns the rows in source order so callers can pin the first one. N2. Encapsulate the isDryRun derivation. Previously pkg/cli/update.go had `isDryRun := o.DryRun || o.PlanJSON` duplicated as an inline expression, and a future dry-run-like flag would need to be added in multiple places. New o.effectiveDryRun() method centralizes the decision so the rest of the loop stays agnostic. Tests: TestUpdateEnvs_SafetyStrict_RefusesDestructive now also asserts the new "1 overwrite" count and "first: a.yaml" path appear in stderr. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Improves b update UX and maintainability by enhancing safety-gate refusal messages to include destructive action counts + the first offending file, and by centralizing “dry-run effective” logic behind a helper.
Changes:
- Add
(*env.Plan).DestructiveRows()to return destructive plan rows for diagnostics. - Add
(*cli.UpdateOptions).effectiveDryRun()to centralize--dry-run/--plan-jsondry-run semantics. - Improve safety refusal error formatting and extend tests to assert the new message content.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/env/plan.go | Adds DestructiveRows() helper used to build more informative refusal errors. |
| pkg/cli/update.go | Uses DestructiveRows() for safety gating, introduces effectiveDryRun(), and adds destructiveRefusalError() for richer refusal messages. |
| pkg/cli/update_test.go | Updates strict-safety refusal test assertions to match the improved error output. |
- DestructiveRows doc now matches actual ordering: rows come out in Plan.Rows order, which PlanFromResult sorts by destination path, so "first destructive row" pinned for diagnostics is dest-sorted. - destructiveRefusalError hint now includes --safety=auto so the CI recovery guidance is immediately actionable without a config edit. - effectiveDryRun() now actually centralizes the lock-write suppression at the end of updateEnvs (was previously only used for the per-env cfg.DryRun decision). Doc comment updated to describe what the helper covers today. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fentas
pushed a commit
that referenced
this pull request
Apr 8, 2026
🤖 I have created a release *beep* *boop* --- ## [4.14.0](v4.13.0...v4.14.0) (2026-04-08) ### Features * **cli:** b env resolve for merge conflicts ([#125](#125) phase 4) ([#138](#138)) ([b3d00c8](b3d00c8)) * **cli:** plan-json polish — destructive row in error + effectiveDryRun helper ([#132](#132)) ([4ad5dfa](4ad5dfa)) * **cli:** surface conflict markers in env status ([#139](#139)) ([1e54c73](1e54c73)) * **env:** format-preserving byte-level YAML splice fast path ([#133](#133)) ([94cd711](94cd711)) * **env:** JSON splice support for scoped selects ([#135](#135)) ([89ccf9d](89ccf9d)) * **env:** orphan-file delete plumbing ([#125](#125) phase 3) ([#137](#137)) ([5891781](5891781)) * **env:** per-key b.pin annotation ([#125](#125) phase 2) ([#136](#136)) ([9ed59db](9ed59db)) * **env:** structural 3-way merge for YAML/JSON ([#134](#134)) ([fd787b0](fd787b0)) * **env:** wrapKeyFor extracts leading identifier for filter expressions ([#131](#131)) ([f542f28](f542f28)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two small follow-ups from PR #128 reviewer notes (N1, N2).
N1. Better safety-refusal error messages
Before: `strict safety: plan contains destructive changes — use --safety=prompt or --safety=auto to apply`
A CI maintainer reading "build broken: b update failed" had to scroll back through the printed plan to find which file caused the refusal.
After: `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`
The new `destructiveRefusalError` helper builds a message with three parts:
New `env.Plan.DestructiveRows()` helper returns the rows in
Plan.Rowsorder, whichPlanFromResultsorts by destination path, so the first row is a stable, dest-sorted choice.N2. Encapsulate `isDryRun` derivation
`pkg/cli/update.go` had `isDryRun := o.DryRun || o.PlanJSON` duplicated inline. A future dry-run-like flag would need to be added in multiple places. New `o.effectiveDryRun()` method centralizes the decision.
Tests
`TestUpdateEnvs_SafetyStrict_RefusesDestructive` now also asserts the new `1 overwrite` count and `first: a.yaml` path appear in stderr.
🤖 Generated with Claude Code