fix(xcstrings): keep conflict resolutions valid JSON

[teamleaderleo]

Summary

Resolving a delete-versus-modify catalog conflict could leave a comma outside the selected side, producing invalid JSON. The driver now includes separators only on non-empty conflict sides. For the last key in an object, it consumes the preceding separator and places it inside those sides too.

This follows up on #15858 from main. The diff changes only scripts/merge-xcstrings.py and its tests. It adds resolution coverage for both sides and the last-key case, pins the ours text preference in the conflict skeleton, corrects the conflict-region assertion, and documents the deliberate line-check asymmetry. The overlap guard remains in place.

I traced every refusal return in main() with Git's arguments supplied: it writes conflict markers or attempts to blank %A on failure. The existing warning when even blanking fails is preserved.

Testing

• TMPDIR=/home/leo/tmp python3 tests/test_merge_xcstrings.py: 33 tests passed on 39a7ce21a02.
• python3 scripts/verify-local.py: 15 selected checks passed; Swift syntax was skipped with zero selected Swift files.
• Setting comma = "" made test_delete_versus_modify_resolves_to_each_side_as_valid_json fail. Swapping the conflict branch to prefer theirs made test_conflicting_key_skeleton_uses_ours_text fail. Both mutations were restored, and the full runner passed afterward.
• A deterministic run of 128 conflicted random merges (seed 15858) produced 61 invalid ours resolutions and 65 invalid theirs resolutions before the fix, compared with 0 and 0 after it. The last-key deletion case is included in the fix and regression coverage.
• tests/test_ci_catch_up_pr.py is absent on current main, so that earlier validation command could not run on this follow-up branch.

Changelog

Fixed: Resolving delete-versus-modify string catalog conflicts keeps JSON separators on the selected side.

Generated with Claude Code.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Fixes delete-versus-modify catalog conflicts in scripts/merge-xcstrings.py so resolving either side keeps valid JSON instead of leaving a comma outside the selected side.

• Separators now attach only to non-empty conflict sides; for the last key in an object, the preceding separator is consumed and, if non-empty, placed inside both sides.
• Adds regression coverage for both sides and the last-key case, pins the ours text in the conflict skeleton, and tightens the conflict-region test so each region contains only its reported key.

^{Written for commit f44612e. Summary will update on new commits.}

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 7 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e0cc547a-7d7b-46fd-96ca-246344dfe191

📥 Commits

Reviewing files that changed from the base of the PR and between 5606649 and f44612e.

📒 Files selected for processing (2)

• scripts/merge-xcstrings.py
• tests/test_merge_xcstrings.py

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[teamleaderleo]

Reviewed this independently of the summary, by reading the diff and running it rather than trusting the report. The fix is correct.

What the defect was

materialize_catalog_conflicts emitted the member separator outside the conflict markers, so resolving to a side that deletes the key left the comma behind. Middle key gave {"a":…,,"c":…}; last key gave {…,"b":…,}. Either way the resolution was invalid JSON, which is the one thing a merge driver for a JSON catalog must not produce.

Why the fix holds

The comma now travels with the content. with_comma appends it only to non-empty blocks, so an empty side contributes nothing at all. For a last key, which has no trailing comma to move, the code walks back to the previous member's end, takes the separator's comma, and re-emits each non-empty block with a leading one.

Two things I checked because they are where this shape usually goes wrong:

• Conflict markers still start at column 0. The new path replaces from previous_end rather than line_start, so I expected the key's indentation to end up in front of <<<<<<<. It does not: the separator runs from the previous member's end to line_start, and the indentation lives between line_start and the key, so the re-emitted prefix ends at a newline. Verified on the last-key case: zero indented markers.
• It fails closed, not open. if comma_index < 0 or separator[:comma_index].strip() raises rather than guessing, and the pre-existing shares-a-line refusal is untouched, so compacted input still falls back to one lossless whole-file conflict.

The if member_index: guard correctly skips a first-or-only member, where deleting leaves {} and there is no preceding comma to consume.

Tests

resolve_conflict is the right test to have written. It picks a side, drops the markers, and json.loads the result, which asserts the property that actually matters instead of pattern-matching the output. Both paths are covered, middle key and last key. 33 tests pass at 39a7ce21a02.

One nit, not blocking

test_each_reported_key_is_inside_a_conflict_region was renamed to test_each_conflict_region_stays_within_its_reported_key_span and its assertion replaced. The new property, that no unrelated key is swallowed into a region, is a good one to have. But the old assertion, that every reported key appears in some region, did not need to be given up for it: I restored it against this branch's production code and all 33 tests still pass. The two are independent and both cheap. Worth keeping both rather than trading one for the other.

Context

This is the follow-up to the defect that went in with #15858, which was squash-merged at 6d5645c487a while the fix was in flight. Based on origin/main, single commit, two files, no protections weakened.

I am not touching the merge button on this one; handing that back.

[teamleaderleo]

Re-checked at 39a7ce21a02. The nit is addressed and the new coverage holds up.

• test_conflicting_key_skeleton_uses_ours_text restores the assertion the rename had traded away, and pins the skeleton as "a":{"v":"ours"} rather than a reformatted "a": {.
• All 33 merge-driver tests pass here under plain python3 tests/test_merge_xcstrings.py.
• Independent mutation check rather than taking the summary for it: disabling only the last-key branch (if not comma: to if False:) makes test_delete_versus_modify_last_key_resolves_to_each_side_as_valid_json fail with Illegal trailing comma before end of object, which is precisely the defect this PR fixes. Restoring the line returns 33/33.

Nothing further from me on this one. The merge button is yours.

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[github-actions]

CI failure attribution

CI passes on f44612e3b9 (run 36769962151 attempt 1).

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's.

[github-actions]

Merge receipt for f44612e3b9: every check was green at merge (10 verified; 14 skipped by policy). Full suite runs on main after merge.