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
Open
Conversation
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_traverse_and_applypicks the winning edit for an anchor withedits[-1]. That list is built by iterating overlays in merge order and, within each overlay, its edits in declaration order — soedits[-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 areplaceand then aninsert_afteron the same anchor, the trailing insert becomes the "winning edit", the anchor's fate reverts to keep the base step, and that overlay's ownreplaceis silently discarded.Reproduction on current
main(bf88c9f)A real overlay through the real resolver:
So
specify workflow run demoexecutesmake buildinstead ofmake build-hardened— no error, andworkflow resolveeven 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 areplaceand let that decide the fate.Deliberately scoped to
replace. Areplaceleaves the anchor in place, so both edits can be honoured and nothing is lost. Aremovedestroys 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 bytest_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_conflictsreads 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
removeunchanged; cross-overlay precedence unchanged.tests/workflows: no new failures vs a clean-mainbaseline captured onbf88c9f9(10 pre-existing, all Windows symlink-privilege).uvx ruff@0.15.0 check src tests→ cleanWritten with assistance from Claude Code. Bug found, reproduced, and verified by me on current
main.