fix(workflows): keep an overlay's replace when the same overlay also inserts on that anchor (#4140)

* fix(workflows): keep an overlay's replace when it also inserts on that anchor

`_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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* test(workflows): cover insert_before and the conflict-guard use of the fate rule

Addresses two review comments, both correct.

1. `_winning_fate_edit` treats `insert_after` and `insert_before` alike, but
   every same-overlay fate test used only `insert_after` -- the pre-existing
   `insert_before` tests use separate layers and never reach this branch.
   `test_replace_then_insert_same_overlay_keeps_replacement` is now
   parametrized over both, asserting the resulting order in each case
   (`[implement, lint, tail]` vs `[lint, implement, tail]`).

2. The conflict guard's use of `_winning_fate_edit` had no regression test.
   Every existing conflict test ends the parent's edits with `replace` or
   `remove`, so all of them pass under either rule. New parametrized
   `test_replace_parent_with_trailing_insert_still_conflicts` gives the parent
   a `replace` plus a trailing insert alongside a descendant edit: under
   `anchor_edits[-1]` the parent's apparent fate becomes the insert, which
   `_check_anchor_conflicts` deliberately skips, so the conflict would go
   undetected and the subtree would be replaced out from under the descendant.

Mutation-verified: reverting the guard to `anchor_edits[-1]` fails exactly the
two new tests and nothing else.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: jawwad-ali

src/specify_cli/workflows/overlays/merge.py

@@ -230,6 +230,51 @@ 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.
+
+    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:
+        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 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]
+
+
 def _traverse_and_apply(
     steps: list[dict[str, Any]],
     edits_by_anchor: dict[str, list[tuple[OverlayLayer, OverlayEdit]]],
@@ -244,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]] = []
 
@@ -255,7 +303,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 +323,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 +401,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(

tests/workflows/test_overlay_merge.py

@@ -591,6 +591,40 @@ def test_replace_parent_and_remove_child_raises(self):
         with pytest.raises(ValueError, match="ancestor"):
             merge_steps(base, [_layer(overlay, "project:ov")])
 
+    @pytest.mark.parametrize(
+        "insert_op", ["insert_after", "insert_before"], ids=["after", "before"]
+    )
+    def test_replace_parent_with_trailing_insert_still_conflicts(self, insert_op):
+        """A rescued parent `replace` must still be seen by the conflict guard.
+
+        The guard reads the anchor's fate through `_winning_fate_edit`, the same
+        rule `_traverse_and_apply` uses. Were it to read `anchor_edits[-1]`
+        instead, the trailing insert would be the parent's apparent fate --
+        and `_check_anchor_conflicts` skips inserts, because they leave the
+        ancestor intact -- so this replace-plus-descendant conflict would go
+        undetected and the subtree would be replaced out from under the
+        descendant edit.
+
+        Every other conflict test ends the parent's edits with `replace` or
+        `remove`, so they pass under either rule; only a trailing insert tells
+        the two apart.
+        """
+        parent_id = "if-step"
+        child_id = "then-child"
+        base = [self._if_step(parent_id, child_id)]
+        overlay = Overlay(
+            id="ov",
+            extends="wf",
+            priority=10,
+            edits=[
+                OverlayEdit("replace", parent_id, _step("new-parent")),
+                OverlayEdit(insert_op, parent_id, _step("beside-parent")),
+                OverlayEdit("remove", child_id),
+            ],
+        )
+        with pytest.raises(ValueError, match="ancestor"):
+            merge_steps(base, [_layer(overlay, "project:ov")])
+
     def test_conflict_across_multiple_overlays_raises(self):
         """Conflict is detected even when conflicting anchors come from different overlays."""
         parent_id = "if-step"
@@ -733,3 +767,166 @@ 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` 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.
+    """
+
+    @pytest.mark.parametrize(
+        "insert_op,expected_ids",
+        [
+            ("insert_after", ["implement", "lint", "tail"]),
+            ("insert_before", ["lint", "implement", "tail"]),
+        ],
+        ids=["after", "before"],
+    )
+    def test_replace_then_insert_same_overlay_keeps_replacement(
+        self, insert_op, expected_ids
+    ):
+        """The rescue covers both insert operations, not just `insert_after`.
+
+        `_winning_fate_edit` treats `insert_after` and `insert_before` alike, so
+        both are pinned here; the pre-existing `insert_before` tests use
+        separate layers and never reach this same-overlay branch.
+        """
+        base = [_step("implement"), _step("tail")]
+        overlay = Overlay(
+            id="ov",
+            extends="wf",
+            priority=10,
+            edits=[
+                OverlayEdit(
+                    "replace", "implement",
+                    {**_step("implement"), "command": "custom.impl"},
+                ),
+                OverlayEdit(insert_op, "implement", _step("lint")),
+            ],
+        )
+
+        steps, _ = merge_steps(base, [_layer(overlay, "project:ov")])
+
+        assert [s["id"] for s in steps] == expected_ids
+        by_id = {s["id"]: s for s in steps}
+        assert by_id["implement"]["command"] == "custom.impl"
+
+    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"]
+
+    @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."""
+        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