Skip to content

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

Open
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/overlay-replace-lost-to-insert
Open

fix(workflows): keep an overlay's replace when the same overlay also inserts on that anchor#4140
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/overlay-replace-lost-to-insert

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

_traverse_and_apply picks the winning edit for an anchor with edits[-1]. That list is built by iterating overlays in merge order and, within each overlay, its edits in declaration order — so edits[-1] treats the order of lines inside a single YAML file as a precedence signal.

Priority is a per-overlay property (docs/reference/workflows.md), so two edits from one overlay have no priority relation to break. But when an overlay declares a replace and then an insert_after on the same anchor, the trailing insert becomes the "winning edit", the anchor's fate reverts to keep the base step, and that overlay's own replace is silently discarded.

Reproduction on current main (bf88c9f)

A real overlay through the real resolver:

edits:
  - replace: implement
    step: {id: implement, type: shell, run: "make build-hardened"}
  - insert_after: implement
    step: {id: run-lint, type: shell, run: "ruff check src/"}
main:
  implement  run='make build'            <-- the overlay's replace is LOST
  run-lint   run='ruff check src/'
  attribution: [('implement', 'base'), ('run-lint', 'project:my-overlay')]

same edits, reversed declaration order:
  implement  run='make build-hardened'   <-- works

So specify workflow run demo executes make build instead of make build-hardened — no error, and workflow resolve even attributes the untouched step to base. Swapping two lines in the YAML changes what runs.

Fix

When the last edit on an anchor is an insert_*, look back within the same layer for a replace and let that decide the fate.

Deliberately scoped to replace. A replace leaves the anchor in place, so both edits can be honoured and nothing is lost. A remove destroys the anchor, so an insert relative to it cannot also apply — something must be dropped either way, and choosing which is a separate question. That combination keeps its existing behaviour, pinned by test_remove_then_insert_after_same_overlay_is_unchanged.

The ancestor-conflict map uses the same fate rule so the two cannot drift, while still listing every anchor — _check_anchor_conflicts reads its key set to find descendant anchors, so dropping insert-only anchors would stop conflicts being detected against them. (I found that the hard way: my first attempt broke two existing conflict tests, which the regression gate caught.)

Breaking risk: only the replace-then-insert-on-the-same-anchor shape changes, and it changes from silently losing the replacement to applying it. A test pins that a higher-priority insert-only overlay still leaves a lower layer's replace unapplied, so cross-overlay precedence is untouched.

Verification

  • Fail-before / pass-after: 2 new-vs-baseline failures with the source reverted → 43 passed with the fix.
  • Four tests: replace survives its own trailing insert; both declaration orders agree; remove unchanged; cross-overlay precedence unchanged.
  • Scoped regression over tests/workflows: no new failures vs a clean-main baseline captured on bf88c9f9 (10 pre-existing, all Windows symlink-privilege).
  • uvx ruff@0.15.0 check src tests → clean

Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

…t 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>
@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner August 15, 2026 14:28
@mnriem
mnriem requested a balanced review from Copilot August 20, 2026 17:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes workflow overlay merging so a same-overlay trailing insert does not discard its replacement.

Changes:

  • Adds shared anchor-fate selection logic.
  • Adds regression tests for edit ordering and precedence.
  • Preserves existing remove-plus-insert behavior.
Show a summary per file
File Description
src/specify_cli/workflows/overlays/merge.py Resolves replacement fate consistently.
tests/workflows/test_overlay_merge.py Covers same-anchor edit combinations.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

  • Files reviewed: 2/2 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread src/specify_cli/workflows/overlays/merge.py
Comment thread src/specify_cli/workflows/overlays/merge.py
Comment thread tests/workflows/test_overlay_merge.py Outdated
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants