feat(env): safety tiers + plan output for env update (#125 phase 1)

[fentas]

Phase 1 of #125. Adds the user-consent and visibility layer.

What's new

• Per-env `safety` field: `strict` | `prompt` (default) | `auto`
• Plan-style output for every sync: `+ add` / `~ update` / `= keep` / `! overwrite` / `⊕ merge` / `✗ conflict` + summary line
• `--safety` flag to override per-env safety from the CLI
• `--plan-json` flag for machine-readable 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

Mode	Interactive (TTY)	Non-interactive (CI)
`strict`	Refuses any destructive plan	Refuses any destructive plan
`prompt` *	Shows plan, asks `[y/N]`	Refuses destructive (= strict)
`auto`	Applies without prompting	Applies without prompting

* default

A plan row is destructive when it would lose user-owned content — currently `overwrite` (a non-merge strategy is about to clobber locally modified files) or `conflict` (a 3-way merge produced markers that need manual resolution).

⚠ 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 fail unless 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.

Performance trade-off

The plan + apply flow only doubles `SyncEnv` calls when the safety mode might block (strict, prompt-with-confirm). For `auto` 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.

When the gate might reject (strict / interactive prompt), the CLI runs `SyncEnv` twice: once in dry-run to compute the plan, then once for the real apply if approved. The second pass benefits from a hot git cache. The eventual right design is to split `SyncEnv` into `Plan(cfg) → Plan` and `Apply(cfg, plan) → Result` so the plan-first path doesn't pay double — that's a follow-up refactor flagged as Phase 2.

What's NEW (files)

• `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

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 Phase 2–4 of #125)

• `b.pin:` per-key annotations
• skipped-delete tracking
• in-place YAML conflict markers
• `b env resolve` subcommand

These need design work + the structural-merge follow-up from #122. Tracking in #125 itself.

Test plan

• `go test -race ./...` green
• `go vet` clean
• `gofmt -l` clean
• Smoke: `b update --dry-run` against a real env, eyeball plan
• Smoke: `b update --plan-json` produces parseable JSON
• Smoke: `b update` on a TTY prompts and accepts/rejects
• Smoke: `b update` on a CI runner refuses destructive plans without `--yes`

Independence

This PR is independent of #122 and #124. It branches from `main` (not from either of the other PR branches) and touches a disjoint set of files. They can land in any order.

🤖 Generated with Claude Code

[fentas]

Review by Claude (Opus 4.6, 1M context) — context: I tracked #122 and reviewed #126 + #127. I did not file #125 but I am downstream of it (lok8s → kubehz consumer chain) and will live with this default change.

Verdict: approve with notes

Phase 1 of #125 delivers exactly what the issue specified for the consent layer. The plan/apply split, the safety tier semantics, and the auto-vs-plan-first fast path are all well-thought-through. The breaking change is real but justified, and the migration is mechanical. Ship it — the notes below are improvements, not blockers.

What's right

1. The auto / --yes fast path is the make-or-break design choice. Calling `SyncEnv` exactly once for the auto path keeps existing automated runs at zero performance overhead. This was the obvious risk in moving to plan-first — the naive implementation would double every sync — and the PR avoided it cleanly. The branching in `updateEnvs` (`needsPlanFirst` flag) is the right place to make that decision.

2. The destructive-action definition is conservative on purpose. Right now only `overwrite` and `conflict` count. `update` doesn't, even though semantically a version bump is "data loss" of the old version. That's the correct call: the issue defines destructive as "would lose user-owned content," and an update of a value the user didn't author is not user-owned content. If the user pinned a value, they'll get the pin behavior in Phase 2 of #125. Don't expand the destructive set in Phase 1.

3. Plan-first attaches the resolver on the second pass. This is subtle and easy to get wrong. The interactive conflict resolver is a TTY interaction — running it during the dry-run plan computation would prompt the user before they've even seen the plan. The PR explicitly attaches `ResolveConflict` only on `applyCfg` (line ~390), with a comment explaining why. Good defensive coding; the test `TestUpdateEnvs_TTYConflictResolver` had to be updated with `Yes: true` precisely because the resolver moved to the apply pass.

4. `NormalizeSafety` collapses empty + unknown to `prompt`, not `auto`. The "fail safe, not fail open" choice. Someone typos `safety: stict` in their b.yaml and they get the safe default rather than silent auto-apply. Test `TestNormalizeSafety_DefaultsToPrompt` pins this. Good.

5. The plan rendering is renderer-agnostic. `PlanFromResult` builds a structured `Plan`, and `RenderPlanText` / `RenderPlanJSON` are independent renderers. This means someone can write `RenderPlanMarkdown` for PR comment bots later without touching the plan computation. Right separation of concerns.

6. Deterministic ordering by Dest in `PlanFromResult` means snapshot tests are stable across runs. The test `TestPlanFromResult_DeterministicOrdering` pins this. Will save someone an hour of "why does my golden file flake on CI" debugging.

7. Test coverage is on point. 8 new tests in `update_test.go` cover the matrix: auto/strict/prompt × destructive/non-destructive × TTY/non-TTY × --yes/--plan-json/--dry-run. Each test uses the `makeSyncFunc` recording stub to assert call counts (1 for auto, 2 for plan-first), which is the load-bearing performance contract — not just the output. Excellent test design.

Notes (non-blocking)

N1. The breaking change deserves a louder migration footnote in the PR description

The PR description has a "BREAKING CHANGE" section, which is good. But the migration path is buried in a list. Specifically: existing CI pipelines that ran `b update` and got silent auto-apply will now exit non-zero on any destructive change with no immediate context. The error message `prompt safety on non-TTY: plan contains destructive changes — re-run with --yes, --safety=auto, or --dry-run` is clear, but a CI maintainer reading "build broken: b update failed" in their inbox needs to know:

1. What's a destructive change? (overwrite or conflict — currently undocumented in the error message itself)
2. How do they preview without applying? (`--dry-run` is in the message but not the rationale)
3. Which fix is right for their situation? (`--yes` for one-off, `--safety=auto` for permanent)

Suggested improvements:

• Add a one-line CHANGELOG entry pointing at the docs section
• Consider extending the error message to include the destructive action types: `prompt safety on non-TTY: plan contains 1 overwrite and 0 conflicts — see plan above. Re-run with --yes for one-off apply, set safety: auto in b.yaml for permanent, or --dry-run to preview.`
• Optionally print the first destructive row's path in the error message so users know which file is the problem without scrolling up

N2. `gateApply`'s `isDryRun` parameter is doubled by `o.PlanJSON`

In `updateEnvs`:

```go
isDryRun := o.DryRun || o.PlanJSON
```

Then in `gateApply`:

```go
if isDryRun {
return false, nil
}
```

This is correct, but the relationship is implicit. A future maintainer adding a new dry-run-like flag has to remember to OR it into both `needsPlanFirst` and the `gateApply` call. Would be cleaner to pin this on the `UpdateOptions` itself with a method:

```go
func (o *UpdateOptions) effectiveDryRun() bool {
return o.DryRun || o.PlanJSON
}
```

Then both call sites use `o.effectiveDryRun()` and the relationship is documented in one place. Tiny refactor; not blocking.

N3. The "merge failed" Note extraction loses the parenthetical content

In `planRowFromLockFile`:

```go
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 "), "")
```

`strings.TrimSuffix(s, "")` is a no-op (trimming the empty string from anywhere does nothing). I think the intent was to strip a trailing `)` to leave the bare reason, but this never executes. Result: the note ends up like `(merge failed: )` with the parens still in it, then gets re-wrapped in parens by `RenderPlanText` (`(%s)` format). User sees `((merge failed: ...))`.

Either:

• (a) Strip a trailing `)`: `strings.TrimSuffix(strings.TrimPrefix(status, "replaced ("), ")")` — produces a bare reason
• (b) Drop the manipulation entirely and just use `row.Note = status` — produces `(replaced (merge failed: ...))` which is at least informative

I'd go with (a). Add a test for it: `TestPlanFromResult_StatusMapping` has a "replaced" case but not a "replaced (merge failed: ...)" case.

N4. `PlanJSON` outputs one JSON object per env, not a JSON array

Looking at the `updateEnvs` loop:

```go
if o.PlanJSON {
if err := env.RenderPlanJSON(o.IO.Out, plan); err != nil { ... }
continue
}
```

This calls `RenderPlanJSON` once per env. Each call writes a complete JSON object plus newline. For a config with 3 envs, the output is 3 separate JSON documents concatenated, not a JSON array.

This is technically NDJSON / JSON Lines and parseable with the right tooling, but most JSON consumers (`jq .`, `json.Parse`) expect a single root document. PR comment bots and CI summary jobs that read `b update --plan-json` will likely break on the second env.

Two fix options:

• (a) Wrap the whole loop output in a JSON array: collect all plans into a slice, render once at the end. This requires the loop to defer `continue`s until after the array is closed.
• (b) Document that `--plan-json` produces NDJSON, not JSON. `jq -s .` slurps it back into an array. Cheaper but surprising.

The PR description says "machine-readable output (PR comment bots, CI summary jobs)" — those consumers usually expect a single document. I'd go with (a). Could file as a Phase 1.5 follow-up rather than blocking this PR.

If you keep NDJSON, at least add a test that verifies the output is parseable by `jq -s` or by streaming `json.Decoder`, and document the format in the docs section.

N5. The plan summary line includes counts that are always 0 in common cases

```go
fmt.Fprintf(w, " → %d add, %d update, %d keep, %d overwrite, %d merge, %d conflict\n", ...)
```

For a typical sync that just updated 3 files, the user sees:

```
→ 0 add, 3 update, 0 keep, 0 overwrite, 0 merge, 0 conflict
```

Lots of noise for one signal. Two ways to clean this up:

• (a) Omit zero counts: `→ 3 update` (cleaner)
• (b) Always show all counts but suppress when all are zero (your current behavior is roughly this without the all-zero check)

(a) is the standard UX choice (Terraform, kubectl, helm all do it). Tiny change to `RenderPlanText`. The renderer-agnostic structure means it's localized.

N6. The `prompt → strict on CI` semantic shift is actually `prompt → "refuse destructive on CI"`

Reading the PR description's table:

Mode	Interactive (TTY)	Non-interactive (CI)
`prompt` *	Shows plan, asks `[y/N]`	Refuses destructive (= strict)

This says "= strict" which is technically accurate but misleading. `strict` refuses any destructive plan with no recovery. `prompt` on CI also refuses destructive plans, but the user can recover by passing `--yes` or setting `safety: auto`. `strict` doesn't have that recovery — passing `--yes` to a strict env would still apply (the gate is on the safety value, not on `--yes`).

Wait, actually let me double-check that. Looking at `gateApply`:

```go
case state.SafetyStrict:
if destructive {
return false, fmt.Errorf("strict safety: ...")
}
return true, nil
```

`--yes` is not checked in the strict branch. So `strict + --yes` still refuses destructive plans. Good — that matches the issue description ("safest, non-interactive-friendly").

But the table in the PR description / docs doesn't make this distinction clear. A user reading "Non-interactive (CI): Refuses destructive (= strict)" might assume `prompt + --yes` and `strict + --yes` behave the same way (both refuse). Actually `prompt + --yes` applies (treats prompt as auto), while `strict + --yes` still refuses.

Suggest updating the docs table to make this explicit:

Mode	Interactive (TTY)	Non-interactive (CI)	--yes effect
`strict`	Refuses any destructive plan	Refuses any destructive plan	No effect
`prompt` *	Shows plan, asks `[y/N]`	Refuses destructive	Acts like `auto`
`auto`	Applies without prompting	Applies without prompting	(no-op)

This avoids the trap where someone tries `b update --yes` against a strict env and is confused why it fails.

Things I'd file as follow-ups (not blocking, in priority order)

1. N4: --plan-json should be a JSON array, not NDJSON. Most painful for consumers.
2. N3: `merge failed` note extraction is broken. Cosmetic but real.
3. N5: Drop zero counts from the summary line. Pure UX polish.
4. N1: Better breaking-change error message. First-time CI confusion mitigation.
5. N6: Clarify --yes vs strict in the docs table. Documentation consistency.
6. N2: Encapsulate `isDryRun` derivation. Tiny refactor; only matters if more dry-run-like flags are added.

How this composes with #126 and #127

This PR is independent of both, as the description says. But the interaction is interesting:

• Once fix(env): symmetric filtering + splice for select + merge (#122) #126 (symmetric merge) lands, the `merge` action becomes the actually-correct default for `.bin/b.yaml` syncs. Plan rows for merged files will be common, and the plan output's `⊕ merge` glyph + summary count become the user-visible signal that "this sync had real merging happen, you might want to review."
• Once feat(env): hybrid JMESPath selector for select: filters (#124) #127 (JMESPath) lands, `Source` paths in plan rows could end up as JMESPath expressions in the future (when select is set with a complex query and the merge happens at the filtered scope). The plan renderer treats Source as opaque text, so it'll work — but the docs might want to mention "for complex selectors, the source path may be the selector expression itself."
• All three together unlock the lok8s → kubehz binaries-by-groups flow with reviewable consent. Upstream tags binaries, JMESPath filters them at sync time, symmetric merge updates the consumer's binaries section while preserving `envs:`, and the safety prompt makes sure the consumer reviews any overwrites or conflicts before they hit disk. That's the dream state.

TL;DR

Ship it. The 6 follow-up items are all polish or 1-3 line fixes. The architecture is sound, the test coverage hits the right contract points (especially the SyncEnv call counts), the breaking change is justified, and the auto fast-path keeps existing automation at zero overhead. Good Phase 1.

The two I'd most want to see fixed before merging if they're cheap:

• N3 (merge-failed note extraction bug) — actively renders wrong output
• N4 (--plan-json should be array) — actively breaks consumers

Everything else is polish that can land in a 128.1 follow-up.

[Copilot]

Pull request overview

Adds a safety/consent layer and standardized “plan” output to b update/env syncing, including per-env safety configuration, CLI overrides, and machine-readable plan output.

Changes:

• Introduces per-env safety (strict / prompt default / auto) and propagates it through state resolution.
• Adds pkg/env plan model + renderers (text + JSON) and tests for status→action mapping.
• Updates the CLI update flow to support plan-first gating, --safety, --yes/-y, --plan-json, and updated tests/docs.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
pkg/state/types.go	Adds EnvEntry.Safety, safety constants, and NormalizeSafety; marshals default safety tersely.
pkg/state/resolve.go	Ensures composed profiles propagate Safety.
pkg/env/plan.go	Introduces plan/actions model and text/JSON renderers derived from SyncResult.
pkg/env/plan_test.go	Adds tests for action destructiveness, status mapping, ordering, and rendering.
pkg/cli/update.go	Implements plan rendering + safety gate, new flags, and plan-first vs apply-first branching.
pkg/cli/update_test.go	Adds/adjusts tests for safety modes, plan-json/dry-run behavior, and SyncEnv call counts.
pkg/cli/env_test.go	Updates tests to match new plan-based output and safety prompting behavior.
docs/env-sync.mdx	Documents safety tiers, new flags, plan output, and performance trade-off.

[fentas]

Thanks for the deep review. Addressed N3, N4, N5, N6 in 5174db0:

• N3 (merge-failed double parens) — covered by copilot fix fix: add argsh binary #5 (thread above). Note extraction now strips leading ( and trailing ). Test MergeFailedNoteHasNoDoubleParens pins it.
• N4 (NDJSON not array) — covered by copilot fix feat: Make config optional #3. --plan-json now emits a single JSON array via new env.RenderPlansJSON. Docs show a jsonc example.
• N5 (zero counts in summary) — fixed in RenderPlanText. "→ 1 add" instead of "→ 1 add, 0 update, 0 keep, 0 overwrite, 0 merge, 0 conflict". Empty plans render as "→ no changes".
• N6 (--yes vs strict in docs table) — added a fourth column showing --yes effect per mode and a paragraph explaining "--yes has no effect on strict".

Deferred (filed mentally as polish follow-ups):

• N1: even-better breaking-change error message (showing first destructive row's path inline)
• N2: encapsulate isDryRun derivation in a method on UpdateOptions

The strict refusal also now produces a non-zero exit (copilot fix #2), which is the load-bearing piece for CI noticing the breaking change at all.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

[Copilot]

The MarshalJSON doc comment includes a reference to a specific "Copilot review" PR/round. Consider removing that PR/round attribution and keeping only the contract rationale (encode nil Rows as [] to keep JSON schema stable for consumers).

Suggested change

	// 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`.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

[Copilot]

Several comments in this block have obvious typos/punctuation artifacts (e.g. ": silent"), which makes the rationale harder to read. Please clean up the wording so the comment is a complete sentence without stray punctuation.