From 693ad3d8f3e00b13605f9b54b6c89d458dd9a237 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 15 Aug 2026 19:28:31 +0500 Subject: [PATCH 1/2] fix(workflows): keep an overlay's replace when it also inserts on that anchor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_traverse_and_apply` decided an anchor's fate with `edits[-1]`, which treats declaration order *inside a single overlay file* as a precedence signal. Priority is a per-overlay property, so two edits from one overlay have no priority relation to break — yet a trailing `insert_after` reverted the anchor to the base step and silently discarded that same overlay's `replace`. Measured through the real resolver, one overlay declaring both edits: replace-then-insert (main): implement run='make build' <-- LOST attribution: ('implement', 'base') insert-then-replace (main): implement run='make build-hardened' either order (fixed): implement run='make build-hardened' attribution: ('implement', 'project:my-overlay') So `specify workflow run demo` executed `make build` instead of `make build-hardened`, with no error, and `workflow resolve` attributed the untouched step to "base". Scoped to `replace` only. A replace leaves the anchor in place so both edits can be honoured; `remove` destroys it, so an insert relative to it cannot also apply and choosing between them is a separate question — that combination keeps its existing behaviour, pinned by a test. The ancestor-conflict map uses the same fate rule so the guard cannot drift, while still listing every anchor: `_check_anchor_conflicts` reads its key set to find descendant anchors. Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/workflows/overlays/merge.py | 55 ++++++++++-- tests/workflows/test_overlay_merge.py | 98 +++++++++++++++++++++ 2 files changed, 147 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/workflows/overlays/merge.py b/src/specify_cli/workflows/overlays/merge.py index bf28a1f133..e4b19c4cc4 100644 --- a/src/specify_cli/workflows/overlays/merge.py +++ b/src/specify_cli/workflows/overlays/merge.py @@ -230,6 +230,38 @@ def _build_attribution( return result +def _winning_fate_edit( + edits: list[tuple[OverlayLayer, OverlayEdit]], +) -> tuple[OverlayLayer, OverlayEdit] | None: + """Return the edit that decides an anchor's fate. + + Normally that is simply the last edit in merge order. The exception this + helper exists for: when the last edit is an ``insert_*``, the same overlay + may *also* have declared a ``replace`` on the anchor earlier in the file. + Declaration order inside one overlay is not a precedence signal -- priority + is a per-overlay property -- so the trailing insert must not cancel that + overlay's own replacement, which previously reverted the anchor to the base + step and discarded the replacement silently. + + A ``replace`` leaves the anchor in place, so both edits can be honoured. + ``remove`` is deliberately NOT rescued here: it destroys the anchor, so an + insert relative to it cannot also apply, and choosing between them is a + separate question. That combination keeps its existing behaviour. + + Returns ``None`` only when there are no edits. + """ + if not edits: + return None + winning_layer, last_edit = edits[-1] + if last_edit.operation not in ("insert_after", "insert_before"): + return edits[-1] + replacement: tuple[OverlayLayer, OverlayEdit] | None = None + for layer, edit in edits: + if layer is winning_layer and edit.operation == "replace": + replacement = (layer, edit) + return replacement if replacement is not None else edits[-1] + + def _traverse_and_apply( steps: list[dict[str, Any]], edits_by_anchor: dict[str, list[tuple[OverlayLayer, OverlayEdit]]], @@ -255,7 +287,8 @@ def _traverse_and_apply( step_id = step.get("id") edits = edits_by_anchor.get(step_id, []) if isinstance(step_id, str) else [] - winning_edit = edits[-1][1] if edits else None + fate = _winning_fate_edit(edits) + winning_edit = fate[1] if fate is not None else None if winning_edit is not None and winning_edit.operation == "remove": # Winning edit removes this step; ignore all other edits on this anchor. @@ -274,7 +307,7 @@ def _traverse_and_apply( result.append(new_step) if winning_edit is not None and winning_edit.operation == "replace": - winning_layer = edits[-1][0] + winning_layer = fate[0] new_step = copy.deepcopy(winning_edit.step) _remove_sources_recursively(step, sources) _record_sources_recursively(new_step, winning_layer.source, sources) @@ -352,10 +385,20 @@ def merge_steps( # the ancestor edit replaces or removes its subtree — those produce # order-dependent results. Pure insert edits on an ancestor are safe because # the ancestor step (and its descendants) remain intact. - anchor_winning_ops = { - anchor: anchor_edits[-1][1].operation - for anchor, anchor_edits in edits_by_anchor.items() - } + # Must agree with ``_traverse_and_apply``: use the same fate rule, or the + # conflict guard stops firing for a subtree that is in fact replaced. + # Every anchor stays in the mapping even when its fate is a pure insert -- + # ``_check_anchor_conflicts`` reads the key set to find *descendant* + # anchors, so dropping insert-only anchors would stop conflicts being + # detected against them. + anchor_winning_ops = {} + for anchor, anchor_edits in edits_by_anchor.items(): + anchor_fate = _winning_fate_edit(anchor_edits) + anchor_winning_ops[anchor] = ( + anchor_fate[1].operation + if anchor_fate is not None + else anchor_edits[-1][1].operation + ) anchor_conflicts = _check_anchor_conflicts(anchor_winning_ops, base_steps) if anchor_conflicts: raise ValueError( diff --git a/tests/workflows/test_overlay_merge.py b/tests/workflows/test_overlay_merge.py index c924d1c271..a83ed7c498 100644 --- a/tests/workflows/test_overlay_merge.py +++ b/tests/workflows/test_overlay_merge.py @@ -733,3 +733,101 @@ def test_replace_with_reused_id_does_not_affect_original(self): assert sources.get("b") == "project:ov", ( f"expected 'project:ov' but got {sources.get('b')!r}" ) + + +class TestMergeStepsSameOverlayFateEdits: + """An overlay's replace/remove must survive its own trailing insert. + + `_traverse_and_apply` decided an anchor's fate with `edits[-1]`, which + treats declaration order *inside one overlay file* as a precedence signal. + Priority is a per-overlay property, so two edits from the same overlay have + no priority relation to break — yet a trailing `insert_after` reverted the + anchor to the base step and discarded that overlay's own `replace`. + """ + + def test_replace_then_insert_after_same_overlay_keeps_replacement(self): + base = [_step("implement"), _step("tail")] + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[ + OverlayEdit( + "replace", "implement", + {**_step("implement"), "command": "custom.impl"}, + ), + OverlayEdit("insert_after", "implement", _step("lint")), + ], + ) + + steps, _ = merge_steps(base, [_layer(overlay, "project:ov")]) + + by_id = {s["id"]: s for s in steps} + assert by_id["implement"]["command"] == "custom.impl" + assert "lint" in by_id + + def test_replace_and_insert_order_inside_one_overlay_is_irrelevant(self): + """Both declaration orders must produce the same result.""" + base = [_step("implement"), _step("tail")] + replace_edit = OverlayEdit( + "replace", "implement", {**_step("implement"), "command": "custom.impl"} + ) + insert_edit = OverlayEdit("insert_after", "implement", _step("lint")) + + first, _ = merge_steps( + base, + [_layer(Overlay(id="ov", extends="wf", priority=10, edits=[replace_edit, insert_edit]), "project:ov")], + ) + second, _ = merge_steps( + base, + [_layer(Overlay(id="ov", extends="wf", priority=10, edits=[insert_edit, replace_edit]), "project:ov")], + ) + + assert [(s["id"], s.get("command")) for s in first] == [ + (s["id"], s.get("command")) for s in second + ] + + def test_remove_then_insert_after_same_overlay_is_unchanged(self): + """`remove` is deliberately not rescued: it destroys the anchor, so an + insert relative to it cannot also apply. That combination keeps its + existing behaviour; only `replace` is rescued.""" + base = [_step("implement"), _step("tail")] + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[ + OverlayEdit("remove", "implement"), + OverlayEdit("insert_after", "implement", _step("lint")), + ], + ) + + steps, _ = merge_steps(base, [_layer(overlay, "project:ov")]) + + assert [s["id"] for s in steps] == ["implement", "lint", "tail"] + + def test_higher_priority_insert_only_overlay_keeps_base_step(self): + """A later layer that only inserts must NOT resurrect a lower layer's + replace — the fate still comes from the winning layer.""" + base = [_step("implement")] + replacer = Overlay( + id="low", extends="wf", priority=5, + edits=[OverlayEdit( + "replace", "implement", + {**_step("implement"), "command": "low.impl"}, + )], + ) + inserter = Overlay( + id="high", extends="wf", priority=10, + edits=[OverlayEdit("insert_after", "implement", _step("lint"))], + ) + + steps, _ = merge_steps( + base, + [_layer(replacer, "project:low"), _layer(inserter, "project:high")], + ) + + by_id = {s["id"]: s for s in steps} + # The insert-only layer wins the anchor, so the base step survives. + assert by_id["implement"]["command"] == "speckit.specify" + assert "lint" in by_id From ca033c0061596324d0b45a19a53dcaaeef158636 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Thu, 27 Aug 2026 23:26:44 +0500 Subject: [PATCH 2/2] fix(workflows): scope the fate rescue to unambiguous replace-plus-insert Addresses review feedback on the same-overlay fate rescue. 1. `_winning_fate_edit` also rescued the `replace` when the winning layer declared `replace`, `remove` AND a trailing insert on one anchor. That changed behaviour for a combination this PR deliberately scoped out. The rescue now bails out when the winning layer has a `remove` on the anchor, so such layers stay byte-identical to their pre-rescue outcome: one overlay's edits upstream/main before now replace, remove, insert base kept replaced base kept remove, replace, insert base kept replaced base kept replace, insert (target) base kept replaced replaced remove, insert base kept base kept base kept Only the intended case now differs from main. 2. `_traverse_and_apply`'s docstring still said the winning edit "is `edits[-1]`", which stopped being true on this path. It now points at `_winning_fate_edit` so future changes do not bypass it. 3. The test class docstring said an overlay's "replace/remove" must survive a trailing insert; only `replace` is rescued. Limited to `replace` and made the `remove` exclusion explicit. New parametrized regression test pins the ambiguous layer in both declaration orders, so the rescue cannot start honouring whichever of replace/remove happens to come first. Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/workflows/overlays/merge.py | 20 +++++++- tests/workflows/test_overlay_merge.py | 51 ++++++++++++++++++++- 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/workflows/overlays/merge.py b/src/specify_cli/workflows/overlays/merge.py index e4b19c4cc4..6fdff1504b 100644 --- a/src/specify_cli/workflows/overlays/merge.py +++ b/src/specify_cli/workflows/overlays/merge.py @@ -248,6 +248,13 @@ def _winning_fate_edit( insert relative to it cannot also apply, and choosing between them is a separate question. That combination keeps its existing behaviour. + For the same reason the rescue applies only to an *unambiguous* + replace-plus-insert layer. If the winning layer also declared a ``remove`` + on this anchor, the layer is asking for two incompatible fates and picking + one is that same separate question -- so the previous trailing-insert + outcome is preserved and such layers stay byte-identical to their + pre-rescue behaviour. + Returns ``None`` only when there are no edits. """ if not edits: @@ -257,7 +264,13 @@ def _winning_fate_edit( return edits[-1] replacement: tuple[OverlayLayer, OverlayEdit] | None = None for layer, edit in edits: - if layer is winning_layer and edit.operation == "replace": + if layer is not winning_layer: + continue + if edit.operation == "remove": + # Ambiguous layer (replace *and* remove on one anchor): leave the + # pre-existing trailing-insert fate untouched. + return edits[-1] + if edit.operation == "replace": replacement = (layer, edit) return replacement if replacement is not None else edits[-1] @@ -276,7 +289,10 @@ def _traverse_and_apply( steps). *edits* are expected to be in merge order (lowest priority first, highest - priority last); the winning edit for each anchor is ``edits[-1]``. + priority last). The winning edit for each anchor is chosen by + ``_winning_fate_edit`` -- normally ``edits[-1]``, except that a trailing + ``insert_*`` does not cancel a ``replace`` declared earlier by that same + overlay. Go through that helper rather than reading ``edits[-1]`` directly. """ result: list[dict[str, Any]] = [] diff --git a/tests/workflows/test_overlay_merge.py b/tests/workflows/test_overlay_merge.py index a83ed7c498..928022cddd 100644 --- a/tests/workflows/test_overlay_merge.py +++ b/tests/workflows/test_overlay_merge.py @@ -736,13 +736,19 @@ def test_replace_with_reused_id_does_not_affect_original(self): class TestMergeStepsSameOverlayFateEdits: - """An overlay's replace/remove must survive its own trailing insert. + """An overlay's `replace` must survive its own trailing insert. `_traverse_and_apply` decided an anchor's fate with `edits[-1]`, which treats declaration order *inside one overlay file* as a precedence signal. Priority is a per-overlay property, so two edits from the same overlay have no priority relation to break — yet a trailing `insert_after` reverted the anchor to the base step and discarded that overlay's own `replace`. + + `remove` is explicitly out of scope: it destroys the anchor, so an insert + relative to it cannot also apply. Layers combining `remove` with a trailing + insert — with or without a `replace` alongside — keep their pre-existing + behaviour, as `test_remove_then_insert_after_same_overlay_is_unchanged` and + `test_replace_and_remove_with_trailing_insert_is_unchanged` pin. """ def test_replace_then_insert_after_same_overlay_keeps_replacement(self): @@ -806,6 +812,49 @@ def test_remove_then_insert_after_same_overlay_is_unchanged(self): assert [s["id"] for s in steps] == ["implement", "lint", "tail"] + @pytest.mark.parametrize( + "order", + ["replace_first", "remove_first"], + ) + def test_replace_and_remove_with_trailing_insert_is_unchanged(self, order): + """A layer asking for two incompatible fates keeps the old outcome. + + The rescue applies only to an unambiguous replace-plus-insert layer. If + the winning layer also declared a `remove` on the anchor it is asking + for two incompatible fates, and choosing one is the separate question + this change deliberately does not answer — so the trailing-insert + outcome is preserved and the base step survives, exactly as it did + before the rescue existed. Pinned in both declaration orders so the + rescue cannot start honouring whichever of the two happens to come + first. + """ + base = [_step("implement"), _step("tail")] + replace_edit = OverlayEdit( + "replace", "implement", {**_step("implement"), "command": "custom.impl"} + ) + remove_edit = OverlayEdit("remove", "implement") + fate_edits = ( + [replace_edit, remove_edit] + if order == "replace_first" + else [remove_edit, replace_edit] + ) + overlay = Overlay( + id="ov", + extends="wf", + priority=10, + edits=[ + *fate_edits, + OverlayEdit("insert_after", "implement", _step("lint")), + ], + ) + + steps, _ = merge_steps(base, [_layer(overlay, "project:ov")]) + + assert [s["id"] for s in steps] == ["implement", "lint", "tail"] + # The base step, not the replacement: the ambiguous layer is left alone. + by_id = {s["id"]: s for s in steps} + assert by_id["implement"].get("command") != "custom.impl" + def test_higher_priority_insert_only_overlay_keeps_base_step(self): """A later layer that only inserts must NOT resurrect a lower layer's replace — the fate still comes from the winning layer."""