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 1 commit 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 1 commit 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
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.

1 participant