Files
github-spec-kit/tests/workflows/test_overlay_merge.py
Markus Wondrak d6fa0460ed feat(workflows): WorkflowResolver standalone (PR 1) (#3557)
* feat(workflows): add standalone WorkflowResolver and overlay subsystem

Implement PR 1 of the workflow-overlays plan: a concrete, standalone
WorkflowResolver for downstream workflow extensibility without touching the
Preset subsystem.

- Add overlay manifest schema (Overlay, OverlayEdit, validate_overlay_yaml)
- Add pure-function merge engine (find_step, apply_edit, merge_steps,
  validate_edits) with recursive anchor search and higher-wins semantics
- Add StepListComposer and tiered layer sources (project, installed, base)
- Add WorkflowResolver facade with inline HIGHER_WINS priority sorting
- Add CLI verbs: workflow overlay add/set-priority/enable/disable/remove/list
  and workflow resolve <id>
- Wire WorkflowEngine.load_workflow through WorkflowResolver
- Extend workflow add to copy optional overlays/ subdirectory from local
  workflow directories
- Add comprehensive unit, integration, and security tests

Refs: discussion #3473 (https://github.com/github/spec-kit/discussions/3473)

Assisted-by: Kimi (model: opencode-go/kimi-k2.7-code, autonomous)

* fix(workflows): reject symlinked overlay directories in layer sources

Address PR #3557 review comments r3594064534 and r3594064563:

- ProjectOverlaySource.collect now rejects symlinked per-workflow overlay
  directories (.specify/workflows/overlays/<id>) before iterating
- InstalledOverlaySource.collect now rejects symlinked installed overlay
  directories (.specify/workflows/<id>/overlays) before iterating
- workflow_overlay_list catches ValueError from resolver and exits with
  code 1 instead of crashing on unhandled exceptions
- Added .specify/workflows/overlays to _reject_unsafe_workflow_storage
  chokepoint for defense-in-depth

These guards prevent symlinked overlay directories from redirecting
auto-loaded overlay YAML to attacker-controlled content outside the
project, which could inject executable shell steps into trusted workflows.

Refs: PR #3557 review comments r3594064534, r3594064563

Assisted-by: opencode-go/qwen3.7-max (autonomous)

* fix(workflows): address Copilot review findings in merge engine

- Apply inserts before winning replace to prevent anchor-not-found errors
  when replace changes step ID (r3594064604)
- Track attribution recursively for nested steps in composite inserts/replaces
  so workflow resolve attributes all child steps correctly (r3594064638)
- Add regression tests for both fixes

Refs: PR #3557 review discussion

Assisted-by: GitHub Copilot (model: qwen3.7-plus, autonomous)

* refactor(workflows): simplify overlay architecture to 2-tier

Remove installed overlays tier to enforce clean separation of concerns:
- workflow add installs workflows only (no overlay copying)
- workflow overlay add installs overlays only (project-local)

Changes:
- Remove InstalledOverlaySource class and all references
- Remove overlay-copying logic from _validate_and_install_local()
- Update WorkflowResolver to 2-tier: project overlays + base workflow
- Fix --priority override timing: apply before validation, not after
- Remove tests for installed overlays (no longer applicable)

Rationale: If upstream controls both base workflow and shipped overlays,
and both get overwritten on bundle update, there's no reason to ship
overlays separately. Overlays only make sense when someone other than
the base author adds them.

Resolves all three review findings from PR #3557:
- r3594064677: workflow add no longer copies overlays from all call sites
- r3594064705: --priority override now applied before validation
- r3594064726: no stale installed overlays (tier removed entirely)

Assisted-by: Claude (model: claude-opus-4-7, autonomous)

* fix(workflows): harden overlay symlink handling

Assisted-by: GitHub Copilot (model: GPT-5.4, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* docs(workflows): remove stale installed-overlay references from workflows.md

The 2-tier refactor (cc28185) removed the installed-overlay tier entirely,
but docs/reference/workflows.md was not updated. This commit addresses all
four Cluster 2 findings from the PR review:

- workflow add: remove sentence about copying overlays/ subdirectory
- How Overlays Work: drop installed-overlay table row and precedence prose;
  rewrite to 2-tier model (project overlays only, source-order tie-break)
- overlay remove: drop trailing sentence about installed overlays
- Interaction with Bundles: rewrite to say workflow add installs only
  workflow.yml; remove installed-overlay discovery language

Fixes: r3596368791, r3596368831, r3596368873, r3596368919

Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(overlays): detect ancestor-conflict anchors in merge_steps

When two overlay edits target anchors that share a parent/descendant
relationship (e.g. remove an if-step + insert_after a nested child),
merge_steps processed them independently and in dict-insertion order,
making the outcome non-deterministic.

Add two private helpers to merge.py:
- _descendant_ids(step): returns all step IDs nested inside a step dict
  by delegating to the existing _all_base_step_ids helper on children.
- _check_anchor_conflicts(anchors, base_steps): for each targeted anchor
  finds its descendants and checks whether any other targeted anchor is
  among them; returns human-readable error strings.

Wire _check_anchor_conflicts into merge_steps immediately after
edits_by_anchor is built, before any tree mutation occurs. Raises
ValueError listing the conflicting anchor pair(s) so overlay authors
know exactly what to fix.

Add TestMergeStepsAncestorConflicts (6 cases):
- remove parent + insert_after child raises ValueError
- replace parent + remove child raises ValueError
- conflict across multiple overlays raises ValueError
- sibling anchors (not ancestor/descendant) pass
- single anchor passes
- parent targeted but child not targeted passes

Closes review comment r3596368746 (PR #3557, round 2, cluster 3).

Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(overlays): fix over-broad conflict detection and non-deterministic ID collision

Finding 1.1 — _check_anchor_conflicts was rejecting any ancestor/descendant
anchor pair, including insert-only edits that are perfectly safe. Only
replace/remove on an ancestor can destroy its subtree and make a descendant
anchor unresolvable. Change the signature to accept a dict[str, str]
(anchor → winning operation) and skip the check for insert_after/insert_before.

Finding 1.2 — merge_steps was calling find_step on the already-mutated tree,
so a replacement step that reused a base step ID could be accidentally targeted
by a later edit group (non-deterministic result depending on dict iteration
order). Replace the anchor-group loop with a single-pass _traverse_and_apply
that walks the original tree structure and applies edits as each step is
encountered. Anchors are never re-looked up in a mutated tree.

Design invariant enforced: overlays always apply to the original base tree and
cannot target steps introduced by other overlays. Non-remove edits on non-base
anchors now raise ValueError early.

Also removes apply_edit (no production callers, only tested in isolation) and
its test class — the new traversal inlines the same mechanics without the
find_step round-trip.

Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(overlays): reject ID trailing newlines and reuse existing .yaml path

Fix two input validation bugs in the overlay layer (Group 2 of copilot
review PR #3557):

1. _validate_safe_id in schema.py used re.match() which anchors only at
   the start of the string, so IDs like 'overlay\n' passed validation
   and could produce newline-containing file paths. Changed to fullmatch()
   so the entire string must satisfy the pattern.

2. workflow_overlay_add always wrote <id>.yml without checking whether
   <id>.yaml already existed. Since the resolver loads both extensions,
   this created two active layers whose edits applied twice. Now uses
   the existing _find_overlay_file() to detect a pre-existing file and
   reuse its path, falling back to .yml only for new overlays.

Tests added for both fixes.

Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(overlays): fix display order inversion and wrap file-read errors

Finding group 3 from copilot-review-v2.md:

3.1 — Precedence display inverted (overlays/__init__.py)
collect_all_layers used a single-pass sort by (-priority, source_asc),
which placed the *losing* equal-priority source first in the display
while claiming "highest first". Fix: two-pass stable sort — source
descending then priority descending — so the actual winner (last applied
by the composer) rises to the top of the display.

3.2 — Unwrapped file-read errors (overlays/layer_sources.py)
Only yaml.YAMLError was caught around path.read_text(), so an
unreadable or non-UTF-8 overlay produced a raw traceback. Fix: widen
the except clause to (yaml.YAMLError, OSError, UnicodeDecodeError),
matching the pattern used throughout catalog.py.

Tests:
- test_workflow_resolve_equal_priority_winner_shown_first: verifies
  project:zzz (the winner) appears before project:aaa in workflow resolve
  output when both overlays share the same priority.
- tests/workflows/test_overlay_layer_sources.py (new): OSError and
  non-UTF-8 bytes both produce OverlayLoadError, not raw tracebacks.

Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: rename misleading overlay test

Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: remove EOF blank line in overlay resolver

Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* fix: handle overlay read and enumeration errors

Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(overlays): validate resolver workflow IDs

Reject unsafe and reserved workflow IDs before overlay or base sources construct paths, preventing traversal through resolver and engine fallback paths.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* fix(overlays): drop _remove_sources_recursively from remove branch

In _traverse_and_apply, the remove branch called _remove_sources_recursively
to clean up attribution entries for the deleted step.  This was inherited from
the old apply_edit loop (c70a5d6) where it was needed because the sources dict
was queried exhaustively.

In the current single-pass design, _build_attribution only traverses the result
list, so stale sources entries for removed steps are never read.  The cleanup
call is therefore unnecessary — and actively harmful when another overlay has
replaced a different step with a new step that reuses the same ID: the pop
clobbers the replacement's attribution entry, causing workflow resolve to report
the surviving step as 'unknown'.

Fix: simply remove the _remove_sources_recursively call from the remove branch.
Add an attribution assertion to the existing reused-ID regression test to catch
this case.

Fixes: r3604242050 (Copilot review finding)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)

* Fix CLI overlay ID validation anchoring

Use fullmatch for CLI workflow/overlay ID validation so trailing newlines are rejected consistently with manifest validation.

Add regression coverage for newline-suffixed workflow and overlay IDs in overlay set-priority.

Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(workflows): validate workflow_id in layer sources before path construction

ProjectOverlaySource.collect() and BaseWorkflowSource.collect() joined
workflow_id directly onto storage paths without validation, enabling
path traversal (e.g. '../../outside') when called outside the
WorkflowResolver.

Add _validate_workflow_id() to layer_sources.py — mirrors the same
_SAFE_ID_PATTERN / _RESERVED_WORKFLOW_IDS check used by WorkflowResolver
in overlays/__init__.py — and call it at the top of both collect()
methods before any path is constructed.

Adds parametrised tests covering unsafe IDs and verifying no filesystem
access occurs for an invalid ID.

Closes review finding r3604772700.

Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(workflows): align layer source validation with _safe_workflow_id_dir

Plan §4.1 requires that Workflow-ID-Validierung, Symlink-/Containment-
Prüfungen and Fehlerübersetzung must not diverge between workflow
management and the overlay resolver.

My previous fix added ID pattern + reserved-name validation to both
collect() methods but was missing the containment step and the
BaseWorkflowSource directory/file checks that _safe_workflow_id_dir
performs.

Changes:
- Add _ensure_contained_dir(path, root) to layer_sources.py — pure
  domain mirror of overlays/_commands.py::_ensure_contained_dir that
  raises OverlayLoadError instead of typer.Exit
- ProjectOverlaySource.collect(): replace two inline symlink/dir checks
  with _ensure_contained_dir(workflow_overlay_dir, self.overlays_dir),
  adding the missing resolve().relative_to() containment step
- BaseWorkflowSource.collect(): add _ensure_contained_dir on the
  workflow directory, and add workflow.yml symlink check before is_file()

The same logic now lives in three places (workflow CLI, overlay CLI,
layer sources). The DRY extraction to workflows/_validation.py is
deferred to PR 3 per plan §4.1.

Tests: add containment and symlink tests for both sources.

Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* fix(overlays): resolve identity from manifest field, not filename

Align overlay identity resolution with the project-wide convention:
presets use preset.id, extensions use extension.id, workflows use
workflow.id, and workflow steps use step.type_key. Overlays must
derive identity from the manifest id field, not the filename.

Rewrite _find_overlay_file() to scan all YAML files in the overlay
directory and match on the manifest id field, fixing the bug where
enable/disable/remove/set-priority failed when filename != manifest id.

Closes: PR #3557 discussion r3605010197

Assisted-by: opencode-go/qwen3.7-max (autonomous)

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* fix(workflows): make validation behavior consistent across YAML loading paths

Address PR #3557 review finding r3607632921:

- Wrap yaml.YAMLError → ValueError in from_yaml() and from_string() so
  malformed YAML matches the documented exception contract
- Add except ValueError to workflow_info to handle composition errors
  cleanly instead of crashing with a raw traceback
- Remove validate_workflow() from compose() so the resolver path is
  parse-only like all other YAML loading mechanisms; callers validate
  explicitly via engine.validate()
- Update test to reflect new behavior: resolve() returns composed
  definition, caller validates separately

Assisted-by: opencode-go/qwen3.7-max (autonomous)

* fix(overlays): list disabled overlays in management view

Keep disabled overlays visible in workflow overlay list while leaving resolution behavior unchanged.

- add an include_disabled opt-in to overlay source/resolver collection
- use include_disabled=True for workflow overlay list
- add regression tests for list visibility and default filtering

Assisted-by: GitHub Copilot (model: GPT-5.4, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* docs: align overlay extends and resolver contract

Assisted-by: GitHub Copilot (model: GPT-5.3-Codex, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: use atomic write for overlay file updates to prevent hard-link attack

Replace in-place write_text() calls in workflow_overlay_add() and
_update_overlay_field() with the same mkstemp → write → os.replace()
pattern used by the workflow installer (_stage_workflow_file /
_commit_workflow_file / _discard_staged_workflow_file).

The prior code rejected symlinks and validated path containment, but a
hard-linked destination file passes both checks while sharing an inode
with an external file. write_text() would then truncate and overwrite
that external inode. The atomic staging approach never opens the
existing destination for writing, eliminating the hard-link vector.

Fixes findings r3608669512 and r3608669517 on PR #3557.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, supervised)

* fix(composer): preserve invalid base definition instead of coercing steps to []

When 'steps' is not a list, returning early with the unmodified
WorkflowDefinition lets validate_workflow surface the proper error
("'steps' must be a list.") to the caller. The previous silent
coercion to [] masked the validation error entirely.

Fixes: https://github.com/github/spec-kit/pull/3557#discussion_r3608669506

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, supervised)

* fix: align workflow overlay priority semantics

Assisted-by: GitHub Copilot (model: GPT-5.6 Terra, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: validate overlay priority presentation

Assisted-by: GitHub Copilot (model: GPT-5.6 Terra, autonomous)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: catch OverflowError in normalize_priority for float infinity values

YAML values like `priority: .inf` parse to float('inf'), causing
int() to raise OverflowError. This broke validate_overlay_yaml()'s
'validation never raises' contract. Adding OverflowError to the
except clause makes it fall back to the default priority (10),
consistent with other invalid value handling.

Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Markus <markus@example.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2026-07-21 08:35:25 -05:00

719 lines
26 KiB
Python

"""Tests for the workflow overlay merge engine."""
from __future__ import annotations
import copy
from typing import Any
import pytest
from specify_cli.workflows.overlays.merge import (
ComposedStep,
OverlayLayer,
find_step,
merge_steps,
validate_edits,
)
from specify_cli.workflows.overlays.schema import Overlay, OverlayEdit
def _step(id: str, **kwargs: Any) -> dict[str, Any]: # noqa: A002
"""Build a minimal step dict with the given id."""
return {"id": id, "type": "command", "command": "speckit.specify", **kwargs}
def _layer(overlay: Overlay, source: str) -> OverlayLayer:
"""Build an OverlayLayer for merge_steps."""
return OverlayLayer(overlay, source)
class TestFindStep:
"""Recursive anchor lookup across nested step lists."""
def test_find_step_flat(self):
steps = [_step("a"), _step("b"), _step("c")]
result = find_step(steps, "b")
assert result is not None
assert result[0] is steps
assert result[1] == 1
def test_find_step_missing(self):
steps = [_step("a"), _step("b")]
assert find_step(steps, "missing") is None
def test_find_step_in_then(self):
steps = [
{
"id": "if-1",
"type": "if",
"condition": "true",
"then": [_step("then-a")],
"else": [_step("else-b")],
},
]
result = find_step(steps, "then-a")
assert result is not None
assert result[0] is steps[0]["then"]
assert result[1] == 0
def test_find_step_in_else(self):
steps = [
{
"id": "if-1",
"type": "if",
"condition": "true",
"then": [_step("then-a")],
"else": [_step("else-b")],
},
]
result = find_step(steps, "else-b")
assert result is not None
assert result[0] is steps[0]["else"]
assert result[1] == 0
def test_find_step_in_nested_steps(self):
steps = [
{
"id": "while-1",
"type": "while",
"condition": "true",
"steps": [_step("inner-a"), _step("inner-b")],
},
]
result = find_step(steps, "inner-b")
assert result is not None
assert result[0] is steps[0]["steps"]
assert result[1] == 1
def test_find_step_in_switch_cases(self):
steps = [
{
"id": "switch-1",
"type": "switch",
"expression": "{{ inputs.x }}",
"cases": {
"one": [_step("case-a")],
"two": [_step("case-b")],
},
"default": [_step("default-c")],
},
]
assert find_step(steps, "case-b")[1] == 0
assert find_step(steps, "default-c")[1] == 0
def test_find_step_not_in_fan_out_template(self):
steps = [
{
"id": "fan-1",
"type": "fan-out",
"items": "{{ inputs.items }}",
"step": {"id": "template-x", "type": "command", "command": "echo"},
},
]
assert find_step(steps, "template-x") is None
class TestMergeSteps:
"""Composition of multiple overlays in merge order."""
def test_merge_steps_no_overlays(self):
base = [_step("a"), _step("b")]
steps, attribution = merge_steps(base, [])
assert [s["id"] for s in steps] == ["a", "b"]
assert attribution == [ComposedStep("a", "base"), ComposedStep("b", "base")]
def test_merge_steps_single_overlay(self):
base = [_step("a"), _step("b")]
overlay = Overlay(
id="ov1",
extends="wf",
priority=10,
edits=[OverlayEdit("insert_after", "a", _step("new"))],
)
steps, attribution = merge_steps(base, [_layer(overlay, "project:ov1")])
assert [s["id"] for s in steps] == ["a", "new", "b"]
assert attribution == [
ComposedStep("a", "base"),
ComposedStep("new", "project:ov1"),
ComposedStep("b", "base"),
]
def test_merge_steps_higher_priority_wins(self):
base = [_step("a")]
low = Overlay(
id="low",
extends="wf",
priority=5,
edits=[OverlayEdit("insert_after", "a", _step("low-step"))],
)
high = Overlay(
id="high",
extends="wf",
priority=10,
edits=[OverlayEdit("insert_after", "a", _step("high-step"))],
)
steps, attribution = merge_steps(base, [_layer(low, "project:low"), _layer(high, "project:high")])
# low applied first, then high; both insert after 'a', so high-step ends
# closer to the anchor (higher priority wins the conflict).
assert [s["id"] for s in steps] == ["a", "high-step", "low-step"]
assert attribution == [
ComposedStep("a", "base"),
ComposedStep("high-step", "project:high"),
ComposedStep("low-step", "project:low"),
]
def test_merge_steps_replace_wins_over_insert(self):
"""Overlays apply to the original tree only; targeting an overlay-introduced step raises."""
base = [_step("a")]
insert = Overlay(
id="insert",
extends="wf",
priority=5,
edits=[OverlayEdit("insert_after", "a", _step("inserted"))],
)
replace = Overlay(
id="replace",
extends="wf",
priority=10,
edits=[OverlayEdit("replace", "inserted", _step("replaced"))],
)
# "inserted" is not a base step — overlays cannot target each other's steps.
with pytest.raises(ValueError, match="Anchor 'inserted' not found"):
merge_steps(base, [_layer(insert, "project:insert"), _layer(replace, "project:replace")])
def test_merge_steps_does_not_mutate_base(self):
base = [_step("a")]
overlay = Overlay(
id="ov1",
extends="wf",
priority=10,
edits=[OverlayEdit("insert_after", "a", _step("new"))],
)
original = copy.deepcopy(base)
merge_steps(base, [_layer(overlay, "project:ov1")])
assert base == original
def test_merge_steps_attribution_uses_source_not_overlay_id(self):
base = [_step("a")]
overlay = Overlay(
id="same-id",
extends="wf",
priority=10,
edits=[OverlayEdit("insert_after", "a", _step("new"))],
)
steps, attribution = merge_steps(base, [_layer(overlay, "installed:same-id")])
assert [s["id"] for s in steps] == ["a", "new"]
assert attribution == [
ComposedStep("a", "base"),
ComposedStep("new", "installed:same-id"),
]
def test_merge_steps_nested_base_attribution(self):
base = [
{
"id": "if-1",
"type": "if",
"condition": "true",
"then": [_step("then-a")],
"else": [_step("else-b")],
},
]
steps, attribution = merge_steps(base, [])
assert attribution == [
ComposedStep("if-1", "base"),
ComposedStep("then-a", "base"),
ComposedStep("else-b", "base"),
]
def test_merge_steps_higher_replace_wins_lower_replace_same_anchor(self):
base = [_step("implement")]
low = Overlay(
id="low",
extends="wf",
priority=5,
edits=[OverlayEdit("replace", "implement", _step("low-implement"))],
)
high = Overlay(
id="high",
extends="wf",
priority=10,
edits=[OverlayEdit("replace", "implement", _step("high-implement"))],
)
steps, attribution = merge_steps(base, [_layer(low, "project:low"), _layer(high, "project:high")])
assert [s["id"] for s in steps] == ["high-implement"]
assert any(
composed.step_id == "high-implement" and composed.source == "project:high"
for composed in attribution
)
def test_merge_steps_higher_replace_wins_after_lower_remove_same_anchor(self):
base = [_step("implement")]
low = Overlay(
id="low",
extends="wf",
priority=5,
edits=[OverlayEdit("remove", "implement")],
)
high = Overlay(
id="high",
extends="wf",
priority=10,
edits=[OverlayEdit("replace", "implement", _step("high-implement"))],
)
steps, attribution = merge_steps(base, [_layer(low, "project:low"), _layer(high, "project:high")])
assert [s["id"] for s in steps] == ["high-implement"]
def test_merge_steps_higher_insert_wins_after_lower_remove_same_anchor(self):
base = [_step("implement")]
low = Overlay(
id="low",
extends="wf",
priority=5,
edits=[OverlayEdit("remove", "implement")],
)
high = Overlay(
id="high",
extends="wf",
priority=10,
edits=[OverlayEdit("insert_after", "implement", _step("high-after"))],
)
steps, attribution = merge_steps(base, [_layer(low, "project:low"), _layer(high, "project:high")])
assert [s["id"] for s in steps] == ["implement", "high-after"]
assert attribution == [
ComposedStep("implement", "base"),
ComposedStep("high-after", "project:high"),
]
def test_merge_steps_later_overlay_wins_tie_same_anchor(self):
"""When two overlays have the same priority, the one applied later wins."""
base = [_step("a")]
first = Overlay(
id="first",
extends="wf",
priority=10,
edits=[OverlayEdit("replace", "a", _step("first-replace"))],
)
second = Overlay(
id="second",
extends="wf",
priority=10,
edits=[OverlayEdit("replace", "a", _step("second-replace"))],
)
# Merge order: first applied, then second wins tie.
steps, attribution = merge_steps(
base,
[
_layer(first, "overlay:first"),
_layer(second, "overlay:second"),
],
)
assert [s["id"] for s in steps] == ["second-replace"]
assert any(
composed.step_id == "second-replace" and composed.source == "overlay:second"
for composed in attribution
)
def test_merge_steps_insert_after_then_replace_same_anchor_id_change(self):
"""Inserts must be applied before the winning replace so the anchor still exists.
Regression: when a replace changes the step ID, applying it before inserts
causes ``find_step`` to fail on the now-gone original anchor.
"""
base = [_step("build")]
low = Overlay(
id="low",
extends="wf",
priority=5,
edits=[OverlayEdit("insert_after", "build", _step("test"))],
)
high = Overlay(
id="high",
extends="wf",
priority=10,
edits=[OverlayEdit("replace", "build", _step("compile"))],
)
steps, attribution = merge_steps(
base, [_layer(low, "project:low"), _layer(high, "project:high")]
)
# The insert should land after the original anchor position, then the
# anchor is replaced. Final order: ["compile", "test"].
assert [s["id"] for s in steps] == ["compile", "test"]
assert attribution == [
ComposedStep("compile", "project:high"),
ComposedStep("test", "project:low"),
]
def test_merge_steps_insert_before_then_replace_same_anchor_id_change(self):
"""Same as above but with insert_before — anchor must still be findable."""
base = [_step("build")]
low = Overlay(
id="low",
extends="wf",
priority=5,
edits=[OverlayEdit("insert_before", "build", _step("lint"))],
)
high = Overlay(
id="high",
extends="wf",
priority=10,
edits=[OverlayEdit("replace", "build", _step("compile"))],
)
steps, attribution = merge_steps(
base, [_layer(low, "project:low"), _layer(high, "project:high")]
)
assert [s["id"] for s in steps] == ["lint", "compile"]
assert attribution == [
ComposedStep("lint", "project:low"),
ComposedStep("compile", "project:high"),
]
def test_merge_steps_unknown_anchor_still_raises(self):
base = [_step("a")]
overlay = Overlay(
id="ov",
extends="wf",
priority=10,
edits=[OverlayEdit("replace", "missing", _step("new"))],
)
with pytest.raises(ValueError, match="Anchor 'missing' not found"):
merge_steps(base, [_layer(overlay, "project:ov")])
# ── composite step attribution ───────────────────────────────────────
def test_merge_insert_composite_if_attribution(self):
"""Nested then/else children of an inserted 'if' step get the overlay source."""
base = [_step("a")]
composite = {
"id": "if-1",
"type": "if",
"condition": "true",
"then": [_step("then-a")],
"else": [_step("else-b")],
}
overlay = Overlay(
id="ov", extends="wf", priority=10,
edits=[OverlayEdit("insert_after", "a", composite)],
)
_steps, attribution = merge_steps(
base, [_layer(overlay, "project:ov")]
)
assert attribution == [
ComposedStep("a", "base"),
ComposedStep("if-1", "project:ov"),
ComposedStep("then-a", "project:ov"),
ComposedStep("else-b", "project:ov"),
]
def test_merge_insert_composite_switch_attribution(self):
"""Nested cases/default children of an inserted 'switch' step get the overlay source."""
base = [_step("a")]
composite = {
"id": "switch-1",
"type": "switch",
"expression": "{{inputs.x}}",
"cases": {"one": [_step("case-one")], "two": [_step("case-two")]},
"default": [_step("default-z")],
}
overlay = Overlay(
id="ov", extends="wf", priority=10,
edits=[OverlayEdit("insert_before", "a", composite)],
)
_steps, attribution = merge_steps(
base, [_layer(overlay, "project:ov")]
)
assert attribution == [
ComposedStep("switch-1", "project:ov"),
ComposedStep("default-z", "project:ov"),
ComposedStep("case-one", "project:ov"),
ComposedStep("case-two", "project:ov"),
ComposedStep("a", "base"),
]
def test_merge_replace_flat_with_composite_attribution(self):
"""Replacing a flat step with a composite step attributes all nested children."""
base = [_step("a")]
composite = {
"id": "if-1",
"type": "if",
"condition": "true",
"then": [_step("inner-x"), _step("inner-y")],
}
overlay = Overlay(
id="ov", extends="wf", priority=10,
edits=[OverlayEdit("replace", "a", composite)],
)
_steps, attribution = merge_steps(
base, [_layer(overlay, "project:ov")]
)
assert attribution == [
ComposedStep("if-1", "project:ov"),
ComposedStep("inner-x", "project:ov"),
ComposedStep("inner-y", "project:ov"),
]
def test_merge_remove_composite_step_cleans_nested_sources(self):
"""Removing a composite step also cleans its nested children from sources."""
base = [
{
"id": "if-1",
"type": "if",
"condition": "true",
"then": [_step("then-a")],
"else": [_step("else-b")],
},
_step("a"),
]
overlay = Overlay(
id="ov", extends="wf", priority=10,
edits=[OverlayEdit("remove", "if-1")],
)
steps, attribution = merge_steps(
base, [_layer(overlay, "project:ov")]
)
assert [s["id"] for s in steps] == ["a"]
assert attribution == [ComposedStep("a", "base")]
def test_merge_insert_deeply_nested_composite_attribution(self):
"""Deep nesting (if inside while) gets the overlay source at every level."""
base = [_step("a")]
inner_if = {
"id": "inner-if",
"type": "if",
"condition": "true",
"then": [_step("deep-x")],
}
composite = {
"id": "while-1",
"type": "while",
"condition": "true",
"steps": [inner_if],
}
overlay = Overlay(
id="ov", extends="wf", priority=10,
edits=[OverlayEdit("insert_after", "a", composite)],
)
_steps, attribution = merge_steps(
base, [_layer(overlay, "project:ov")]
)
assert attribution == [
ComposedStep("a", "base"),
ComposedStep("while-1", "project:ov"),
ComposedStep("inner-if", "project:ov"),
ComposedStep("deep-x", "project:ov"),
]
class TestValidateEdits:
"""Edit validation against known base step IDs."""
def test_valid_edits(self):
edits = [
OverlayEdit("insert_after", "a", _step("new")),
OverlayEdit("remove", "b"),
]
assert validate_edits(edits, {"a", "b"}) == []
def test_invalid_anchor(self):
edits = [OverlayEdit("insert_after", "missing", _step("new"))]
errors = validate_edits(edits, {"a"})
assert any("missing" in e for e in errors)
def test_step_id_contains_colon(self):
edits = [OverlayEdit("insert_after", "a", _step("bad:id"))]
errors = validate_edits(edits, {"a"})
assert any("':'" in e for e in errors)
def test_remove_requires_no_step(self):
edits = [OverlayEdit("remove", "a", _step("extra"))]
errors = validate_edits(edits, {"a"})
assert len(errors) > 0
class TestMergeStepsAncestorConflicts:
"""merge_steps raises when two targeted anchors are in a parent/descendant relationship."""
def _if_step(self, parent_id: str, child_id: str) -> dict[str, Any]:
return {
"id": parent_id,
"type": "if",
"condition": "true",
"then": [_step(child_id)],
}
def test_remove_parent_and_insert_after_child_raises(self):
"""Removing a parent while inserting after its nested child is an anchor conflict."""
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("remove", parent_id),
OverlayEdit("insert_after", child_id, _step("new-step")),
],
)
with pytest.raises(ValueError, match="ancestor"):
merge_steps(base, [_layer(overlay, "project:ov")])
def test_replace_parent_and_remove_child_raises(self):
"""Replacing a parent while also removing a nested child is an anchor conflict."""
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("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"
child_id = "then-child"
base = [self._if_step(parent_id, child_id)]
overlay_a = Overlay(
id="ov-a",
extends="wf",
priority=5,
edits=[OverlayEdit("remove", parent_id)],
)
overlay_b = Overlay(
id="ov-b",
extends="wf",
priority=10,
edits=[OverlayEdit("insert_after", child_id, _step("new-step"))],
)
with pytest.raises(ValueError, match="ancestor"):
merge_steps(
base,
[_layer(overlay_a, "project:ov-a"), _layer(overlay_b, "project:ov-b")],
)
def test_sibling_anchors_not_conflicting(self):
"""Anchors in sibling branches (not ancestor/descendant) are allowed."""
base = [
{
"id": "if-step",
"type": "if",
"condition": "true",
"then": [_step("then-child")],
"else": [_step("else-child")],
}
]
overlay = Overlay(
id="ov",
extends="wf",
priority=10,
edits=[
OverlayEdit("insert_after", "then-child", _step("after-then")),
OverlayEdit("insert_after", "else-child", _step("after-else")),
],
)
# Should not raise — the two anchors are siblings, not ancestor/descendant.
steps, _ = merge_steps(base, [_layer(overlay, "project:ov")])
step_ids = [s.get("id") for s in steps[0]["then"]] + [s.get("id") for s in steps[0]["else"]]
assert "after-then" in step_ids
assert "after-else" in step_ids
def test_single_anchor_not_conflicting(self):
"""A single anchor is never in conflict with itself."""
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("remove", parent_id)],
)
steps, _ = merge_steps(base, [_layer(overlay, "project:ov")])
assert steps == []
def test_child_not_targeted_no_conflict(self):
"""Targeting a parent alone (child not in any edit) is allowed."""
parent_id = "if-step"
child_id = "then-child"
base = [self._if_step(parent_id, child_id), _step("other")]
overlay = Overlay(
id="ov",
extends="wf",
priority=10,
edits=[
OverlayEdit("remove", parent_id),
OverlayEdit("insert_after", "other", _step("new-step")),
],
)
# "other" is not inside "if-step", so no ancestor conflict.
steps, _ = merge_steps(base, [_layer(overlay, "project:ov")])
assert [s["id"] for s in steps] == ["other", "new-step"]
def test_insert_only_on_ancestor_and_descendant_not_conflicting(self):
"""insert_after on both a parent and its nested child is valid and order-independent."""
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("insert_after", parent_id, _step("after-parent")),
OverlayEdit("insert_after", child_id, _step("after-child")),
],
)
# Should not raise — inserts leave the ancestor intact.
steps, attribution = merge_steps(base, [_layer(overlay, "project:ov")])
# "after-parent" is inserted at the top level after the if-step.
assert [s["id"] for s in steps] == [parent_id, "after-parent"]
# "after-child" is inserted inside the then list.
then_ids = [s["id"] for s in steps[0]["then"]]
assert then_ids == [child_id, "after-child"]
class TestMergeStepsIdCollision:
"""merge_steps is deterministic when a replacement reuses a base step ID."""
def test_replace_with_reused_id_does_not_affect_original(self):
"""Replacing A with new_step(id=B) must not interfere with editing original B.
Before the fix, the remove-B anchor group would find the replacement step
(which now has id='b') instead of the original 'b' step, producing a
different result depending on dict iteration order.
"""
base = [_step("a"), _step("b"), _step("c")]
overlay = Overlay(
id="ov",
extends="wf",
priority=10,
edits=[
# Replace "a" with a new step that reuses id "b".
OverlayEdit("replace", "a", {**_step("b"), "command": "speckit.replaced"}),
# Remove the original "b".
OverlayEdit("remove", "b"),
],
)
steps, attribution = merge_steps(base, [_layer(overlay, "project:ov")])
# The original "b" is removed; the replacement (also id="b") survives.
# "c" is untouched.
assert len(steps) == 2
remaining_ids = [s["id"] for s in steps]
assert remaining_ids == ["b", "c"]
# The surviving "b" step is the replacement (has the custom command).
assert steps[0]["command"] == "speckit.replaced"
# Attribution for the surviving replacement "b" must not be "unknown".
# Previously, removing original "b" would pop sources["b"], erasing the
# attribution recorded for the replacement step (regression guard for the
# _remove_sources_recursively-in-remove-branch bug).
sources = {cs.step_id: cs.source for cs in attribution}
assert sources.get("b") == "project:ov", (
f"expected 'project:ov' but got {sources.get('b')!r}"
)