mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
* 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>
719 lines
26 KiB
Python
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}"
|
|
)
|