Skip to content

Commit b890fdd

Browse files
MarkusCopilot
andcommitted
fix(overlays): detect ancestor-conflict anchors in merge_steps
When two overlay edits target anchors that share a parent/descendant relationship (e.g. remove an if-step + insert_after a nested child), merge_steps processed them independently and in dict-insertion order, making the outcome non-deterministic. Add two private helpers to merge.py: - _descendant_ids(step): returns all step IDs nested inside a step dict by delegating to the existing _all_base_step_ids helper on children. - _check_anchor_conflicts(anchors, base_steps): for each targeted anchor finds its descendants and checks whether any other targeted anchor is among them; returns human-readable error strings. Wire _check_anchor_conflicts into merge_steps immediately after edits_by_anchor is built, before any tree mutation occurs. Raises ValueError listing the conflicting anchor pair(s) so overlay authors know exactly what to fix. Add TestMergeStepsAncestorConflicts (6 cases): - remove parent + insert_after child raises ValueError - replace parent + remove child raises ValueError - conflict across multiple overlays raises ValueError - sibling anchors (not ancestor/descendant) pass - single anchor passes - parent targeted but child not targeted passes Closes review comment r3596368746 (PR #3557, round 2, cluster 3). Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ed0daa7 commit b890fdd

2 files changed

Lines changed: 179 additions & 0 deletions

File tree

src/specify_cli/workflows/overlays/merge.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,49 @@ def _all_base_step_ids(steps: list[dict[str, Any]]) -> set[str]:
8181
return ids
8282

8383

84+
def _descendant_ids(step: dict[str, Any]) -> set[str]:
85+
"""Return all step IDs nested inside *step* (not including *step* itself)."""
86+
ids: set[str] = set()
87+
for key in _NESTED_LIST_KEYS:
88+
nested = step.get(key)
89+
if isinstance(nested, list):
90+
ids.update(_all_base_step_ids(nested))
91+
cases = step.get("cases")
92+
if isinstance(cases, dict):
93+
for case_steps in cases.values():
94+
if isinstance(case_steps, list):
95+
ids.update(_all_base_step_ids(case_steps))
96+
return ids
97+
98+
99+
def _check_anchor_conflicts(
100+
anchors: set[str],
101+
base_steps: list[dict[str, Any]],
102+
) -> list[str]:
103+
"""Return error messages for anchor pairs where one is an ancestor of the other.
104+
105+
When two targeted anchors share a parent/descendant relationship the edit
106+
processing order determines success or failure, producing non-deterministic
107+
results. Callers should raise on any returned errors before mutating the
108+
step tree.
109+
"""
110+
errors: list[str] = []
111+
for anchor in sorted(anchors): # sorted for deterministic error order
112+
location = find_step(base_steps, anchor)
113+
if location is None:
114+
continue # missing anchors are reported by validate_edits
115+
parent_list, idx = location
116+
step = parent_list[idx]
117+
conflicting = anchors & _descendant_ids(step)
118+
for child_anchor in sorted(conflicting):
119+
errors.append(
120+
f"Anchor conflict: '{anchor}' is an ancestor of '{child_anchor}'. "
121+
"Targeting both anchors in the same overlay set produces "
122+
"order-dependent results; restructure edits to avoid nesting."
123+
)
124+
return errors
125+
126+
84127
def _init_sources_recursively(
85128
steps: list[dict[str, Any]], sources: dict[str, str]
86129
) -> None:
@@ -252,6 +295,14 @@ def merge_steps(
252295
for edit in layer.overlay.edits:
253296
edits_by_anchor.setdefault(edit.anchor, []).append((layer, edit))
254297

298+
# Reject edits that target anchors with a parent/descendant relationship —
299+
# processing such pairs independently produces order-dependent results.
300+
anchor_conflicts = _check_anchor_conflicts(set(edits_by_anchor.keys()), base_steps)
301+
if anchor_conflicts:
302+
raise ValueError(
303+
"Overlay anchor conflict(s) detected:\n - " + "\n - ".join(anchor_conflicts)
304+
)
305+
255306
for anchor, edits in edits_by_anchor.items():
256307
# Highest-priority edit is last because *overlays* is already sorted.
257308
_, winning_edit = edits[-1]

tests/workflows/test_overlay_merge.py

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -579,3 +579,131 @@ def test_remove_requires_no_step(self):
579579
edits = [OverlayEdit("remove", "a", _step("extra"))]
580580
errors = validate_edits(edits, {"a"})
581581
assert len(errors) > 0
582+
583+
584+
class TestMergeStepsAncestorConflicts:
585+
"""merge_steps raises when two targeted anchors are in a parent/descendant relationship."""
586+
587+
def _if_step(self, parent_id: str, child_id: str) -> dict[str, Any]:
588+
return {
589+
"id": parent_id,
590+
"type": "if",
591+
"condition": "true",
592+
"then": [_step(child_id)],
593+
}
594+
595+
def test_remove_parent_and_insert_after_child_raises(self):
596+
"""Removing a parent while inserting after its nested child is an anchor conflict."""
597+
parent_id = "if-step"
598+
child_id = "then-child"
599+
base = [self._if_step(parent_id, child_id)]
600+
overlay = Overlay(
601+
id="ov",
602+
extends="wf",
603+
priority=10,
604+
edits=[
605+
OverlayEdit("remove", parent_id),
606+
OverlayEdit("insert_after", child_id, _step("new-step")),
607+
],
608+
)
609+
with pytest.raises(ValueError, match="ancestor"):
610+
merge_steps(base, [_layer(overlay, "project:ov")])
611+
612+
def test_replace_parent_and_remove_child_raises(self):
613+
"""Replacing a parent while also removing a nested child is an anchor conflict."""
614+
parent_id = "if-step"
615+
child_id = "then-child"
616+
base = [self._if_step(parent_id, child_id)]
617+
overlay = Overlay(
618+
id="ov",
619+
extends="wf",
620+
priority=10,
621+
edits=[
622+
OverlayEdit("replace", parent_id, _step("new-parent")),
623+
OverlayEdit("remove", child_id),
624+
],
625+
)
626+
with pytest.raises(ValueError, match="ancestor"):
627+
merge_steps(base, [_layer(overlay, "project:ov")])
628+
629+
def test_conflict_across_multiple_overlays_raises(self):
630+
"""Conflict is detected even when conflicting anchors come from different overlays."""
631+
parent_id = "if-step"
632+
child_id = "then-child"
633+
base = [self._if_step(parent_id, child_id)]
634+
overlay_a = Overlay(
635+
id="ov-a",
636+
extends="wf",
637+
priority=5,
638+
edits=[OverlayEdit("remove", parent_id)],
639+
)
640+
overlay_b = Overlay(
641+
id="ov-b",
642+
extends="wf",
643+
priority=10,
644+
edits=[OverlayEdit("insert_after", child_id, _step("new-step"))],
645+
)
646+
with pytest.raises(ValueError, match="ancestor"):
647+
merge_steps(
648+
base,
649+
[_layer(overlay_a, "project:ov-a"), _layer(overlay_b, "project:ov-b")],
650+
)
651+
652+
def test_sibling_anchors_not_conflicting(self):
653+
"""Anchors in sibling branches (not ancestor/descendant) are allowed."""
654+
base = [
655+
{
656+
"id": "if-step",
657+
"type": "if",
658+
"condition": "true",
659+
"then": [_step("then-child")],
660+
"else": [_step("else-child")],
661+
}
662+
]
663+
overlay = Overlay(
664+
id="ov",
665+
extends="wf",
666+
priority=10,
667+
edits=[
668+
OverlayEdit("insert_after", "then-child", _step("after-then")),
669+
OverlayEdit("insert_after", "else-child", _step("after-else")),
670+
],
671+
)
672+
# Should not raise — the two anchors are siblings, not ancestor/descendant.
673+
steps, _ = merge_steps(base, [_layer(overlay, "project:ov")])
674+
step_ids = [s.get("id") for s in steps[0]["then"]] + [s.get("id") for s in steps[0]["else"]]
675+
assert "after-then" in step_ids
676+
assert "after-else" in step_ids
677+
678+
def test_single_anchor_not_conflicting(self):
679+
"""A single anchor is never in conflict with itself."""
680+
parent_id = "if-step"
681+
child_id = "then-child"
682+
base = [self._if_step(parent_id, child_id)]
683+
overlay = Overlay(
684+
id="ov",
685+
extends="wf",
686+
priority=10,
687+
edits=[OverlayEdit("remove", parent_id)],
688+
)
689+
steps, _ = merge_steps(base, [_layer(overlay, "project:ov")])
690+
assert steps == []
691+
692+
def test_child_not_targeted_no_conflict(self):
693+
"""Targeting a parent alone (child not in any edit) is allowed."""
694+
parent_id = "if-step"
695+
child_id = "then-child"
696+
base = [self._if_step(parent_id, child_id), _step("other")]
697+
overlay = Overlay(
698+
id="ov",
699+
extends="wf",
700+
priority=10,
701+
edits=[
702+
OverlayEdit("remove", parent_id),
703+
OverlayEdit("insert_after", "other", _step("new-step")),
704+
],
705+
)
706+
# "other" is not inside "if-step", so no ancestor conflict.
707+
steps, _ = merge_steps(base, [_layer(overlay, "project:ov")])
708+
assert [s["id"] for s in steps] == ["other", "new-step"]
709+

0 commit comments

Comments
 (0)