Skip to content

feat(env): per-key b.pin annotation (#125 phase 2)#136

Merged
fentas merged 10 commits into
mainfrom
feat/pin-annotations
Apr 8, 2026
Merged

feat(env): per-key b.pin annotation (#125 phase 2)#136
fentas merged 10 commits into
mainfrom
feat/pin-annotations

Conversation

@fentas

@fentas fentas commented Apr 7, 2026

Copy link
Copy Markdown
Owner

Summary

Why

Phase 2 of issue #125: scalpel-level overrides for individual keys without flipping the whole file to `strategy: client`. Lets a consumer pin `kubectl: v1.30` for a specific reason while still letting `kustomize` flow through normal sync.

Behavior

```yaml
binaries:
kubectl:
version: v1.30.0
b.pin: true # ignore upstream for this entry
kustomize:
version: v5.0.0 # no annotation → updated from upstream
```

  • Truthy: `true`, `yes`, `on` (case-insensitive). `b.pin: false` is a no-op so users can toggle it.
  • Annotation itself is preserved across syncs.
  • Pinned key absent in upstream → re-added from local.
  • YAML only; JSON pinning is a follow-up.

Test plan

  • `go test ./pkg/env/... -run Pin`
  • full `go test ./...`
  • `golangci-lint run ./pkg/env/...`

Refs #125

🤖 Generated with Claude Code

@fentas @claude
feat(env): per-key b.pin annotation for scalpel overrides (#125 phase 2)
9c89969 
Adds the `b.pin: true` annotation from #125 phase 2: any map node in
the consumer's local YAML that carries the annotation is preserved
verbatim across syncs, regardless of strategy or safety mode. The
annotation itself is preserved alongside the data so subsequent syncs
keep honoring it.

Implementation:
- New `pkg/env/pin.go` with `applyPinsYAML(local, pending, path)`
- Walks local with yaml.v3 to collect annotated paths
- Substitutes the local subtree into the pending document tree at
  each path; re-inserts the subtree if upstream had deleted it
- Truthy values: `true`, `yes`, `on` (case-insensitive). `false` is
  treated as no pin so users can leave the annotation in place and
  toggle it without deleting the key

Wired into `SyncEnv` after every merge/splice path (no-local-changes,
merge-failed, structural merge, structural splice, replace) so the
behavior is uniform across strategies. Non-YAML files pass through
unchanged; JSON pinning is left for a follow-up.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 16:28
Copilot AI reviewed Apr 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces per-key pinning for env sync by honoring a local-only b.pin: true YAML annotation, ensuring consumer-owned map entries are preserved verbatim across syncs (even if upstream changes or deletes them).

Changes:

  • Added YAML pin discovery and application logic (b.pin) that restores pinned map nodes from the local file into the merged/spliced output.
  • Wired pin application into SyncEnv so it runs after merge/splice across strategies.
  • Added unit tests for pin path collection and pin application behavior.

Reviewed changes

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

File Description
pkg/env/pin.go Implements YAML pin annotation scanning and substitution into pending output.
pkg/env/pin_test.go Adds unit tests validating pin detection, restoration, and re-add behavior.
pkg/env/env.go Integrates pin application into the sync pipeline after merge/splice and before writing.

Comment thread pkg/env/env.go Outdated
Comment thread pkg/env/pin.go Outdated
Comment thread pkg/env/pin.go Outdated
Comment thread pkg/env/env.go
@fentas @claude
fix(env): address copilot review on #136
31fe133 
- applyPins now distinguishes os.IsNotExist from other read errors.
  Permission failures or transient I/O previously got swallowed and
  silently skipped pin restoration, which would have hidden a real
  problem and surprised users whose pinned keys appeared to vanish.
- yaml encoder Close() error is now checked. A flush failure was
  silently dropped before, which could have produced truncated
  output for large pinned documents.
- Doc comment about pinning `kubectl.version` was misleading because
  scalars cannot carry the b.pin annotation. Reworded to clarify
  that pinning is per-map-node.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 16:38
Copilot AI reviewed Apr 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread pkg/env/pin.go Outdated
Comment thread pkg/env/pin.go
Comment thread pkg/env/pin.go Outdated
Comment thread pkg/env/env.go
Comment thread pkg/env/pin_test.go Outdated
@fentas @claude
fix(env): address copilot review on #136 round 2
fe90e63 
- applyPins now short-circuits non-YAML destinations BEFORE the
  os.ReadFile call so JSON / text files don't have to survive a
  read error to land in the no-op branch. This avoids an
  unnecessary I/O hop and removes a class of failures (permission
  errors on a path the helper would have ignored anyway).
- Rename TestApplyPinsYAML_PinFalseIsNotApin → ...IsNotAPin so
  the test name reads as English.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 16:50
Copilot AI reviewed Apr 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread pkg/env/env.go
Comment thread pkg/env/env.go
fentas and others added 2 commits April 7, 2026 19:07
@fentas @claude
fix(env): handle empty/header-only pending in applyPinsYAML (#136 rou…
4d43a79 
…nd 3)

- Drop the early return for empty pending. A brand-new file
  (first sync) should still get the pinned keys carried over from
  local; previously they were silently dropped.
- Coerce zero-Kind / empty-content yaml.v3 results into a synthetic
  mapping document so setPath / addPath can walk into header-only
  files without crashing on Content[0]. yaml.v3 sometimes returns
  Kind == 0 for comment-only inputs and would otherwise panic.
- Pass-through when pending's root is a non-mapping node (sequence,
  scalar) — pinning doesn't apply there.
- New tests cover the empty-pending, header-only, and non-mapping
  cases.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas @claude
fix(env): cheap pin pre-check + skip-write on identical pinned target (…
1f09195 
…#136 round 3.5)

- Add a bytes.Contains pre-check on local for the b.pin substring
  before yaml.Unmarshal-ing it. The common no-pin case now skips
  the YAML parse entirely; the structural collectPinnedPaths walk
  is still the source of truth on the slow path.
- Skip the writeFile call when the pin-restored target already
  matches the on-disk file. Pin restoration intentionally makes
  the local file diverge from the upstream hash, so the existing
  unchanged-fast-path (which compares against upstreamHash) never
  fires for pinned files. Without the new check we churn mtime
  and file watchers on every sync.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 17:20
Copilot AI reviewed Apr 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread pkg/env/pin.go
Comment thread pkg/env/env.go
Comment thread pkg/env/pin.go
@fentas @claude
fix(env): doc clarification, chmod on skip-write, truthy-variant tests (
2197981 
#136 round 4)

- applyPinsYAML doc no longer claims it's a no-op when pending
  parses cleanly but the pinned path isn't present. The
  implementation explicitly addPath-reinserts pinned-but-deleted
  paths; the comment now describes that.
- The skip-write-on-identical-pinned-target branch now still
  applies the upstream file mode via os.Chmod when it differs.
  Otherwise an upstream 644→755 flip on a pinned file would never
  reach disk.
- New TestApplyPinsYAML_TruthyValueVariants covers true / yes /
  on plus mixed-case (True / YES / On) so a future parser change
  can't silently drop yes/on without breaking tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 19:13
Copilot AI reviewed Apr 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread pkg/env/env.go
Comment thread pkg/env/pin.go Outdated
@fentas @claude
fix(env): preserve splice bytes when pins already match (#136 round 5)
023e77e 
The previous applyPinsYAML always re-encoded the entire pending
document through yaml.v3 whenever any pin existed, which defeated
spliceYAMLByteLevel's byte-preservation guarantees and reformatted
comments/whitespace across the whole file.

New behavior: track whether any pinned subtree actually needs
substitution. If every pin already matches what the splice
produced, return the splice's bytes verbatim. Only when at least
one pin diverges do we round-trip through the encoder. This means
the common no-drift case keeps splice's byte-preservation
guarantees, and the docstring is honest about the formatting
trade-off when restoration does kick in.

Equality check uses a small encode-and-compare helper so style /
comment differences (which the pin restoration explicitly doesn't
care about) don't trip the equality check.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 19:25
Copilot AI reviewed Apr 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread pkg/env/env.go
@fentas @claude
fix(env): set status=unchanged when pinned target matches on-disk (#136…
bdec1ce 
… round 6)

When the no-local-changes path skips writeFile because the
pin-restored target already matches what's on disk, the SyncResult
status was still "replaced". The plan would then report an update
even though the on-disk file was untouched. Common when upstream
changes only affected pinned subtrees.

Set status to "unchanged" in that branch so the plan reflects
reality. The chmod-on-skip path stays so a permission flip still
reaches disk.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 19:49
Copilot AI reviewed Apr 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread pkg/env/pin.go Outdated
@fentas @claude
fix(env): handle Encoder.Close error in yamlNodesStructurallyEqual (#136
6bd2a78 
 round 7)

Encoder.Close can surface a buffered write/flush error. Ignoring
it could leave the encoded bytes incomplete, which would make the
equality check return a wrong answer (skipping a needed
substitution or doing an unnecessary one).

Return nil from the inner encoder helper on Close error so the
outer comparison falls through to the "not equal" branch and the
substitution path runs — the safer of the two outcomes — and
require both encoded sides to be non-nil for the equality verdict.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 19:58
Copilot AI reviewed Apr 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread pkg/env/pin.go
@fentas @claude
feat(env): support root-level b.pin (#136 round 8)
6423e61 
A b.pin: true on the document root was previously silently
ignored — collectPinnedPaths only recorded a pin when len(prefix)
was > 0. Make it a real feature instead: a root pin means "pin
the entire document", equivalent to setting strategy: client for
that one file. applyPinsYAML special-cases an empty pinned path
and returns local verbatim, so the splice's bytes are preserved
exactly.

New TestApplyPinsYAML_RootPinPreservesLocalVerbatim covers it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 20:14
Copilot AI reviewed Apr 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@fentas fentas merged commit 9ed59db into main Apr 8, 2026
12 checks passed
@fentas fentas deleted the feat/pin-annotations branch April 8, 2026 06:23
@buyo-bot buyo-bot mentioned this pull request Apr 8, 2026
Merged
fentas pushed a commit that referenced this pull request Apr 8, 2026
@buyo-bot
chore(main): release 4.14.0 (#140)
fce396e 
🤖 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fentas