Skip to content

feat(env): structural 3-way merge for YAML/JSON#134

Merged
fentas merged 6 commits into
mainfrom
feat/structural-3way-merge
Apr 7, 2026
Merged

feat(env): structural 3-way merge for YAML/JSON#134
fentas merged 6 commits into
mainfrom
feat/structural-3way-merge

Conversation

@fentas

@fentas fentas commented Apr 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

Why

git merge-file is line-based, so two independent additions on neighbouring lines (e.g. local adds helm:, upstream adds kustomize: to the same binaries: map) collide on overlapping hunks and mark the whole region conflicted. The structural merge treats each map entry as its own decision, so independent adds just merge.

Conflict rules

  • equal on all sides → keep
  • one side unchanged from base → take the other
  • both changed identically → keep
  • both changed and both maps → recurse
  • both changed otherwise → leaf conflict
  • delete + unchanged → drop; delete + modify → conflict
  • add by one side only (not in base) → take it

Unsupported formats (.md, plain text) and any unresolved conflict fall back to the existing text path, so consumers still see familiar conflict markers when they need them.

Test plan

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

🤖 Generated with Claude Code

@fentas @claude
feat(env): structural 3-way merge for YAML/JSON
a6ae101 
Replace the text-only `git merge-file` path with a structural,
map-walking merge for YAML and JSON destinations. The text path
remains as a fallback for unsupported formats and for cases the
structural merge cannot resolve.

The motivating case is "both sides added adjacent map entries":
when local and upstream both insert new keys into the same parent
map, `git merge-file` sees overlapping line ranges and produces a
spurious conflict, even though the changes are semantically
independent. The structural merge walks the union of keys per map
and treats each as an independent decision.

Conflict resolution rules per key:
- equal on all sides → keep
- one side unchanged from base → take the other
- both changed identically → keep (no conflict)
- both changed and both maps → recurse
- both changed otherwise → leaf conflict
- delete + unchanged → drop; delete + modify → conflict
- add by one side only (not in base) → take the addition

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

Introduces a YAML/JSON-aware structural 3-way merge to reduce spurious conflicts during b env sync merges (especially adjacent map-entry insertions) by merging parsed trees instead of line-based git merge-file.

Changes:

  • Added Merge3WayStructural to parse and merge YAML/JSON maps key-by-key.
  • Added unit tests covering adjacent inserts, leaf conflicts, deletes, and JSON behavior.
  • Wired structural merge into doMerge ahead of the existing text-based merge fallback.

Reviewed changes

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

File Description
pkg/env/merge_structural.go Implements structural YAML/JSON 3-way merge + normalization/serialization helpers.
pkg/env/merge_structural_test.go Adds focused tests for the new merge behavior.
pkg/env/env.go Calls Merge3WayStructural from doMerge before falling back to git merge-file.

Comment thread pkg/env/env.go Outdated
Comment thread pkg/env/merge_structural.go
Comment thread pkg/env/merge_structural.go Outdated
Comment thread pkg/env/merge_structural.go
@fentas @claude
fix(env): address copilot review on #134
3ba3027 
- doMerge now tries the text merge FIRST and only falls back to the
  structural merge when text reports conflicts. Clean merges keep
  byte-for-byte git merge-file output, preserving local key order
  and comments. The structural path still resolves the
  adjacent-add case the reviewer flagged as the regression motivator
  — that's exactly when text merge fails, so the structural merge
  takes over.
- detectStructuralFormat no longer sniffs bytes; the destination
  filename extension is authoritative. Sniffing risked routing a
  YAML file with a flow-style root through the JSON path.
- serializeStructural returns nil for an empty map so an
  intentionally blank file doesn't get re-emitted as `{}`.
- mergeMaps doc tightened to acknowledge that the alphabetical
  ordering is squashed by the encoder anyway and that the
  structural path's "messy diff" trade-off only kicks in when
  text merge would otherwise leave conflict markers.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 16:36
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/merge_structural.go Outdated
Comment thread pkg/env/merge_structural.go Outdated
Comment thread pkg/env/env.go Outdated
@fentas @claude
fix(env): address copilot review on #134 round 2
e4c4b1e 
- serializeStructural now propagates yaml.Encoder.Close() errors
  instead of dropping them. yaml.v3 surfaces buffered write
  failures on Close, so a swallowed error could mean a truncated
  doc on disk.
- Extract the doMerge text→structural fallback into
  mergeWithStructuralFallback so the wiring is directly testable
  without spinning up a real git repo.
- New tests:
  - CleanTextMergePreservesBytes proves the clean-text path keeps
    comments verbatim (the structural merge would drop them).
  - AdjacentInsertsResolveStructurally proves the structural
    fallback kicks in for the headline regression case.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas fentas requested a review from Copilot April 7, 2026 16:48
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/merge_structural.go Outdated
Comment thread pkg/env/merge_structural.go Outdated
Comment thread pkg/env/env.go
Comment thread pkg/env/merge_structural.go
Comment thread pkg/env/merge_structural_test.go
@fentas @claude
fix(env): empty-doc handling and type-change conflicts (#134 round 3)
23d9af5 
- Distinguish "every input was empty bytes" from "the merged result
  is an explicit empty map". The previous shortcut returned nil for
  any empty map, which silently dropped a real \`{}\` document. New
  serializeStructuralAlways takes an anyContent flag and emits a
  real document whenever at least one input had real bytes.
- mergeValues no longer recurses into a synthetic empty base map
  when both sides converted a scalar to a map. Treat that as a
  genuine type-change leaf conflict so the user resolves it
  explicitly instead of silently merging unrelated maps.
- Doc comment now says the merged bytes are not byte-for-byte
  identical to local — the structural merge round-trips through
  yaml.v3 / encoding/json so callers shouldn't expect comments,
  key order, or whitespace to survive. The doMerge wiring still
  runs the text merge first so clean merges keep local intact.
- New TestStructural_ExplicitEmptyJSONRoundTrips covers the
  empty-doc round-trip case.

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 2 comments.

Comment thread pkg/env/merge_structural.go Outdated
Comment thread pkg/env/merge_structural_test.go
fentas and others added 2 commits April 7, 2026 21:03
@fentas @claude
fix(env): drop unused serializeStructural wrapper (#134 CI)
c2ac767 
Lint flagged the wrapper as unused. Inline the anyContent flag into
the single function so there's only one entry point.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@fentas @claude
fix(env): allow recurse on base-absent both-add-map; assert text conf…
4018d7b 
…lict (#134 round 4)

- mergeValues now also recurses when base is nil and both local and
  upstream added a map at the same path. That's a "both added
  concurrently" case, not a type change, and previously fell through
  to a leaf conflict. The non-nil non-map base + map local + map
  upstream case (genuine scalar→map type change) still stays a leaf
  conflict.
- TestMergeWithStructuralFallback_AdjacentInsertsResolveStructurally
  now sanity-checks that gitcache.Merge3Way actually flags the input
  as conflicted before exercising the structural path. If a future
  git merge-file resolves this case cleanly the test now skips
  instead of silently passing without exercising the fallback.
- New TestStructural_BothAddSameMapKey covers the base-absent
  recursive add case directly.

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 no new comments.

@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 no new comments.

@fentas fentas merged commit fd787b0 into main Apr 7, 2026
16 checks passed
@fentas fentas deleted the feat/structural-3way-merge branch April 7, 2026 19:43
@buyo-bot buyo-bot mentioned this pull request Apr 7, 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