fix(PL-6895): remove edge case when merging mapping keys to preserve comments and tags

[davidmdm]

Note

Low Risk
Localized change to YAML merge key selection with no auth, security, or external API impact.

Overview
YAML map merges now pick mapping keys through a dedicated mergeKey helper instead of firstNonNil, fixing edge cases when destination and source keys differ (comments, tags, or missing keys).

When the destination key is locked or local, or the source key is absent, the merge keeps the destination key node. Otherwise it uses the source key but copies the destination key’s HeadComment onto it so inline comments on keys survive the merge.

The unused firstNonNil helper is removed.

^{Reviewed by Cursor Bugbot for commit db6dac1. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• Bug Fixes
  • Improved YAML merge behavior by selecting merge keys more reliably when both sides contain mergable values.
  • Preserves destination lock/local semantics so locked/local destination keys remain unchanged.
  • Better handles cases where the source key is nil and preserves head comments during the merge.

[coderabbitai]

📝 Walkthrough

Walkthrough

YAML mapping merges now select keys through a helper that preserves head comments, respects locked destination keys, handles nil nodes, and removes the obsolete firstNonNil helper.

Changes

YAML Mapping Key Merge

Layer / File(s)	Summary
Comment-aware mapping key selection internal/yml/merge.go	mergeMap uses mergeKey to propagate HeadComment values, retain locked or local destination keys, handle nil source keys, and replace the removed firstNonNil logic.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: alexstojda, j-martin, riclima, silphid

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the merge-key fix and its goal of preserving comments and tags.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/PL-6895/support-custom-tags-on-mapping-keys

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/yml/merge.go`:
- Line 78: Update mergeKey in the mergeMap path to return immediately when
either key node is nil, before accessing or copying head comments. Preserve
normal comment propagation when both dst and src keys are present, preventing
nil dereferences for source-only or locked destination-only entries.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d1b86148-60b5-4b6a-9597-0bd3f99b195d

📥 Commits

Reviewing files that changed from the base of the PR and between a0eaf15 and 11510ff.

📒 Files selected for processing (1)

• internal/yml/merge.go

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit db6dac1. Configure here.}

[cursor]

Locked source keys overwrite destination

Medium Severity

mergeKey returns the source key whenever the destination key is unlocked, including when the source key is !lock. That disagrees with merge, which keeps the destination value when the source is locked. During promotion, an existing destination entry can become locked via the key tag while its value stays unchanged, so later merges stop updating it.

 

^{Reviewed by Cursor Bugbot for commit db6dac1. Configure here.}

[cursor]

Destination key comments partially dropped

Low Severity

mergeKey only copies HeadComment from the destination key onto the source key. Destination LineComment and FootComment are discarded whenever both keys exist and the destination key is not locked or local. The previous firstNonNil path kept the full destination key node, so those comments were preserved.

 

^{Reviewed by Cursor Bugbot for commit db6dac1. Configure here.}