gabriel / muse public
test_imports_unordered_set_merge.py python
261 lines 11.1 KB
Raw
sha256:5cecc36811c041829d68ecd34bb2d05d9ca81b39d59b14cd47f604f90110cd49 fix: muse code add unconditionally wrote EMPTY_DIR_OID to t… Sonnet 5 patch 3 hours ago
1 """musehub#219 — code domain's imports/variables dimension didn't honor its
2 own "unordered set" merge semantics: a pure reorder on one branch, combined
3 with a pure addition on another, produced a false file-level conflict.
4
5 Root cause, found via live investigation (two bugs stacked):
6
7 1. `_canonical_key` (the symbol tree's dict key) bakes in `node.lineno` for
8 every symbol kind, including imports/variables -- directly contradicting
9 their own dimension schema's declared ``identity="by_content"`` (order
10 is semantically irrelevant). A pure reorder changes every import's key,
11 so `diff_symbol_trees` treated them as "moved" (via its content-match
12 rename-detection pass), emitting a spurious ReplaceOp for a change that
13 should be invisible.
14
15 2. Even after suppressing that spurious op, the actual blob reconstruction
16 still fell back to `three_way_merge_lines` (line-based 3-way diff),
17 which *cannot* reconcile a pure reorder on one side with an insertion on
18 the other -- reordering moves every line, so the edit regions always
19 overlap, regardless of how the structured diff reports it.
20
21 Fix: `_merge_unordered_set_blob` reconstructs the merged file via a real
22 three-way SET merge (survivors = base entries neither side removed, plus
23 either side's new entries) whenever every symbol in the file is an
24 unordered-set kind and the file's content *outside* those symbols' line
25 ranges is byte-identical across base/ours/theirs.
26
27 Verified live against the real ticket repro via muse-dev before writing
28 these tests (base: sys/os/re; ours: alphabetized os/re/sys; theirs:
29 sys/os/re + json) -- confirmed FF+merge succeeds with the correct
30 deduplicated, ordered result and no --force needed.
31 """
32 from __future__ import annotations
33
34 import pathlib
35 from collections.abc import Mapping
36 from typing import TYPE_CHECKING
37
38 import pytest
39
40 from muse.core.types import blob_id
41 from muse.plugins.code.plugin import CodePlugin
42
43 if TYPE_CHECKING:
44 from muse.domain import MergeResult
45
46
47 def _oid(content: bytes) -> str:
48 return blob_id(content)
49
50
51 def _write_blob(root: pathlib.Path, content: bytes) -> str:
52 from muse.core.object_store import write_object
53 oid = _oid(content)
54 write_object(root, oid, content)
55 return oid
56
57
58 def _snap(root: pathlib.Path, files: Mapping[str, bytes]) -> Mapping[str, object]:
59 return {
60 "files": {path: _write_blob(root, content) for path, content in files.items()},
61 "domain": "code",
62 "directories": [],
63 }
64
65
66 def _read_merged_blob(root: pathlib.Path, result: "MergeResult", path: str) -> str:
67 from muse.core.object_store import read_object
68 oid = result.merged["files"][path]
69 raw = read_object(root, oid)
70 assert raw is not None, f"merged blob for {path} not in object store"
71 return raw.decode("utf-8")
72
73
74 class TestReorderPlusAddition:
75 """The ticket's exact scenario: pure reorder on one side, pure addition
76 on the other."""
77
78 def test_reorder_plus_addition_does_not_conflict(self, tmp_path: pathlib.Path) -> None:
79 plugin = CodePlugin()
80 base_src = b"import sys\nimport os\nimport re\n\nprint('hello')\n"
81 ours_src = b"import os\nimport re\nimport sys\n\nprint('hello')\n" # pure reorder
82 theirs_src = b"import sys\nimport os\nimport re\nimport json\n\nprint('hello')\n" # pure add
83
84 base = _snap(tmp_path, {"app.py": base_src})
85 ours = _snap(tmp_path, {"app.py": ours_src})
86 theirs = _snap(tmp_path, {"app.py": theirs_src})
87
88 result = plugin.merge_ops(base, ours, theirs, [], [], repo_root=tmp_path)
89
90 assert "app.py" not in result.conflicts, (
91 "a pure reorder on one side and a pure addition on the other "
92 "must not conflict -- imports are an unordered set"
93 )
94
95 def test_reorder_plus_addition_merged_content_is_correct(
96 self, tmp_path: pathlib.Path
97 ) -> None:
98 plugin = CodePlugin()
99 base_src = b"import sys\nimport os\nimport re\n\nprint('hello')\n"
100 ours_src = b"import os\nimport re\nimport sys\n\nprint('hello')\n"
101 theirs_src = b"import sys\nimport os\nimport re\nimport json\n\nprint('hello')\n"
102
103 base = _snap(tmp_path, {"app.py": base_src})
104 ours = _snap(tmp_path, {"app.py": ours_src})
105 theirs = _snap(tmp_path, {"app.py": theirs_src})
106
107 result = plugin.merge_ops(base, ours, theirs, [], [], repo_root=tmp_path)
108 merged_text = _read_merged_blob(tmp_path, result, "app.py")
109
110 # Ours' reordering is preserved, theirs' new import is appended, no
111 # duplicates, print statement untouched.
112 assert merged_text == "import os\nimport re\nimport sys\nimport json\n\nprint('hello')\n"
113
114 def test_addition_plus_reorder_is_symmetric(self, tmp_path: pathlib.Path) -> None:
115 """Swapping which side reorders vs. adds must behave the same way."""
116 plugin = CodePlugin()
117 base_src = b"import sys\nimport os\nimport re\n\nprint('hello')\n"
118 ours_src = b"import sys\nimport os\nimport re\nimport json\n\nprint('hello')\n" # add
119 theirs_src = b"import os\nimport re\nimport sys\n\nprint('hello')\n" # reorder
120
121 base = _snap(tmp_path, {"app.py": base_src})
122 ours = _snap(tmp_path, {"app.py": ours_src})
123 theirs = _snap(tmp_path, {"app.py": theirs_src})
124
125 result = plugin.merge_ops(base, ours, theirs, [], [], repo_root=tmp_path)
126
127 assert "app.py" not in result.conflicts
128 merged_text = _read_merged_blob(tmp_path, result, "app.py")
129 for expected in ("import sys", "import os", "import re", "import json"):
130 assert expected in merged_text
131 assert merged_text.count("import sys") == 1
132 assert merged_text.count("import os") == 1
133 assert merged_text.count("import re") == 1
134
135
136 class TestReorderPlusRemoval:
137 def test_reorder_plus_removal_does_not_conflict(self, tmp_path: pathlib.Path) -> None:
138 plugin = CodePlugin()
139 base_src = b"import sys\nimport os\nimport re\n\nprint('x')\n"
140 ours_src = b"import os\nimport re\nimport sys\n\nprint('x')\n" # reorder
141 theirs_src = b"import sys\nimport os\n\nprint('x')\n" # removed re
142
143 base = _snap(tmp_path, {"app.py": base_src})
144 ours = _snap(tmp_path, {"app.py": ours_src})
145 theirs = _snap(tmp_path, {"app.py": theirs_src})
146
147 result = plugin.merge_ops(base, ours, theirs, [], [], repo_root=tmp_path)
148
149 assert "app.py" not in result.conflicts
150 merged_text = _read_merged_blob(tmp_path, result, "app.py")
151 assert "import re" not in merged_text, "theirs' explicit removal must be respected"
152 assert "import os" in merged_text
153 assert "import sys" in merged_text
154
155
156 class TestPureReorderIsANoOp:
157 def test_pure_reorder_alone_produces_no_moved_op(self, tmp_path: pathlib.Path) -> None:
158 """A commit that only reorders imports must never show a 'moved' op
159 for each one -- matching the dimension's own declared 'order is
160 semantically irrelevant' contract. build_diff_ops' existing
161 formatting-only convention (a single file-level ReplaceOp labeled
162 'reformatted -- no semantic change') is the correct, accurate
163 signal here -- it is not the misleading per-import 'moved' op this
164 ticket reported."""
165 from muse.plugins.code.symbol_diff import build_diff_ops
166 from muse.plugins.code.ast_parser import parse_symbols
167
168 base_src = b"import sys\nimport os\nimport re\n"
169 target_src = b"import os\nimport re\nimport sys\n"
170
171 base_tree = parse_symbols(base_src, "app.py")
172 target_tree = parse_symbols(target_src, "app.py")
173
174 ops = build_diff_ops(
175 {"app.py": _oid(base_src)},
176 {"app.py": _oid(target_src)},
177 {"app.py": base_tree},
178 {"app.py": target_tree},
179 )
180 assert not any("moved to" in op.get("new_summary", "") for op in ops), (
181 f"a pure import reorder must not produce a 'moved' op per import, got {ops!r}"
182 )
183 assert len(ops) == 1
184 assert "no semantic change" in ops[0]["new_summary"]
185
186
187 class TestVariablesDimensionToo:
188 """musehub#219's own suggested follow-up: check whether this affects the
189 variables dimension too, since it declares the same unordered-set
190 semantics."""
191
192 def test_variable_reorder_plus_addition_does_not_conflict(
193 self, tmp_path: pathlib.Path
194 ) -> None:
195 plugin = CodePlugin()
196 base_src = b"FOO = 1\nBAR = 2\n"
197 ours_src = b"BAR = 2\nFOO = 1\n" # pure reorder
198 theirs_src = b"FOO = 1\nBAR = 2\nBAZ = 3\n" # pure add
199
200 base = _snap(tmp_path, {"config.py": base_src})
201 ours = _snap(tmp_path, {"config.py": ours_src})
202 theirs = _snap(tmp_path, {"config.py": theirs_src})
203
204 result = plugin.merge_ops(base, ours, theirs, [], [], repo_root=tmp_path)
205
206 assert "config.py" not in result.conflicts
207 merged_text = _read_merged_blob(tmp_path, result, "config.py")
208 assert "FOO = 1" in merged_text
209 assert "BAR = 2" in merged_text
210 assert "BAZ = 3" in merged_text
211
212
213 class TestGenuineChangesStillFallThrough:
214 """The reconstruction must never engage when the file has a real,
215 non-import/variable change -- those must still go through the existing
216 merge path untouched."""
217
218 def test_function_body_change_is_not_affected_by_import_reorder(
219 self, tmp_path: pathlib.Path
220 ) -> None:
221 """A genuine same-function conflict must still surface -- the new
222 reconstruction path must decline to engage just because imports
223 also changed."""
224 plugin = CodePlugin()
225 base_src = b"import os\n\ndef f():\n return 1\n"
226 ours_src = b"import os\nimport sys\n\ndef f():\n return 2\n"
227 theirs_src = b"import os\nimport sys\n\ndef f():\n return 3\n"
228
229 base = _snap(tmp_path, {"app.py": base_src})
230 ours = _snap(tmp_path, {"app.py": ours_src})
231 theirs = _snap(tmp_path, {"app.py": theirs_src})
232
233 result = plugin.merge_ops(base, ours, theirs, [], [], repo_root=tmp_path)
234
235 assert "app.py::f" in result.conflicts or "app.py" in result.conflicts, (
236 "a genuine conflicting function-body change must still surface "
237 "as a conflict, not be silently resolved by the new import "
238 "reconstruction path"
239 )
240
241 def test_unparseable_divergent_file_still_surfaces_conflict(
242 self, tmp_path: pathlib.Path
243 ) -> None:
244 """Regression guard for the exact IC_01 scenario this fix could have
245 broken: a genuine SyntaxError on both sides, with divergent content,
246 must never be silently treated as 'zero imports changed'."""
247 plugin = CodePlugin()
248 base_src = b"value = 1\n"
249 ours_src = b"(((\nvalue = 2\n"
250 theirs_src = b")))\nvalue = 3\n"
251
252 base = _snap(tmp_path, {"config.py": base_src})
253 ours = _snap(tmp_path, {"config.py": ours_src})
254 theirs = _snap(tmp_path, {"config.py": theirs_src})
255
256 result = plugin.merge_ops(base, ours, theirs, [], [], repo_root=tmp_path)
257
258 assert "config.py" in result.conflicts, (
259 "divergent unparseable content must surface a conflict, not be "
260 "silently merged as 'no variables changed'"
261 )
File History 1 commit
sha256:5cecc36811c041829d68ecd34bb2d05d9ca81b39d59b14cd47f604f90110cd49 fix: muse code add unconditionally wrote EMPTY_DIR_OID to t… Sonnet 5 patch 3 hours ago