feat(workflows): WorkflowResolver standalone (PR 1)#3557
feat(workflows): WorkflowResolver standalone (PR 1)#3557markuswondrak wants to merge 38 commits into
Conversation
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 github#3473 (github#3473) Assisted-by: Kimi (model: opencode-go/kimi-k2.7-code, autonomous)
There was a problem hiding this comment.
Pull request overview
Implements the standalone WorkflowResolver agreed for PR 1 in Discussion #3473, adding upgrade-safe workflow overlays without modifying PresetResolver.
Changes:
- Adds layered workflow composition with recursive step edits and attribution.
- Adds overlay management and resolution CLI commands.
- Supports shipped overlays, documentation, and comprehensive tests.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/overlays/__init__.py |
Defines the resolver facade. |
src/specify_cli/workflows/overlays/composer.py |
Composes and validates workflow layers. |
src/specify_cli/workflows/overlays/layer_sources.py |
Loads project, installed, and base layers. |
src/specify_cli/workflows/overlays/merge.py |
Implements step-list merge operations. |
src/specify_cli/workflows/overlays/schema.py |
Defines and validates overlay manifests. |
src/specify_cli/workflows/overlays/_commands.py |
Implements overlay CLI handlers. |
src/specify_cli/workflows/engine.py |
Resolves overlays when loading workflows. |
src/specify_cli/workflows/_commands.py |
Registers commands and installs shipped overlays. |
docs/reference/workflows.md |
Documents overlay behavior and commands. |
tests/workflows/conftest.py |
Adds shared workflow fixtures. |
tests/workflows/test_overlay_commands.py |
Tests overlay CLI operations. |
tests/workflows/test_overlay_composer.py |
Tests composition validation. |
tests/workflows/test_overlay_merge.py |
Tests merge behavior and attribution. |
tests/workflows/test_overlay_schema.py |
Tests manifest formats and validation. |
tests/workflows/test_overlay_security.py |
Tests path-handling protections. |
tests/workflows/test_resolver_integration.py |
Tests end-to-end resolution. |
Review performed by GitHub Copilot.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@markuswondrak Looks like the right direction to me. Now we'll have to get it to the review cycles. So please address Copilot feedback and resolve conflicts |
Address PR github#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 github#3557 review comments r3594064534, r3594064563 Assisted-by: opencode-go/qwen3.7-max (autonomous)
- 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 github#3557 review discussion Assisted-by: GitHub Copilot (model: qwen3.7-plus, autonomous)
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 github#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)
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback and resolve conflicts
Assisted-by: GitHub Copilot (model: GPT-5.4, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lows.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>
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 github#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>
Resolved conflicts in docs/reference/workflows.md and src/specify_cli/workflows/_commands.py: - docs: kept options table from main + overlay docs from feature branch - _commands.py: adopted main's transactional install refactor Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous)
|
Please address Copilot feedback |
…c 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 two input validation bugs in the overlay layer (Group 2 of copilot review PR github#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>
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>
Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: gpt-5.3-codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
src/specify_cli/workflows/engine.py:749
- A missing YAML path now falls through to
WorkflowResolver, which rejects the dot inmissing.ymlwithValueError. This changesload_workflow()'s documented missing-file contract fromFileNotFoundErrorto “invalid workflow ID,” so callers can no longer distinguish an absent path from malformed workflow content. Treat any YAML-suffixed source as a path and raiseFileNotFoundErrorbefore ID resolution when it does not exist.
# Try as an installed workflow ID, resolving any overlays.
resolver = WorkflowResolver(self.project_root)
try:
return resolver.resolve(str(source))
src/specify_cli/workflows/overlays/layer_sources.py:150
- Duplicate manifest IDs are all appended as active resolver layers, but
_find_overlay_file()manages only the first sorted match. Withaaa.ymlandzzz.ymlboth declaringid: lint,disable,set-priority, orremovecan report success after changingaaa.ymlwhilezzz.ymlremains active. Enforce unique manifest IDs consistently during loading/addition instead of allowing ambiguous layers.
overlay, errors = validate_overlay_yaml(data)
if overlay is None or errors:
raise OverlayLoadError(path, errors)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: GPT-5.3-Codex, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/specify_cli/workflows/overlays/_commands.py:200
- When no manifest ID matches, this blindly chooses
<id>.yml. Since this API explicitly permits filenames to differ from manifest IDs, that filename may already hold a different overlay; the subsequentwrite_textsilently destroys it. After the containment check, detect an existing candidate owned by another manifest and fail (or choose a collision-free filename) instead of overwriting it.
target_path = _ensure_contained_path(
target_dir / f"{overlay.id}.yml", _overlay_root(project_root)
)
src/specify_cli/workflows/overlays/layer_sources.py:169
- Duplicate manifest IDs are accepted as separate layers and both are applied with the same
project:<id>source, while_find_overlay_filereturns only the first sorted match. Consequentlydisable/removecan report success while another overlay with the same ID remains active and cannot be addressed independently. Reject duplicate IDs as ambiguous during collection and make management lookup report the same error rather than codifying first-match behavior.
layers.append(
Layer(
content=overlay,
source=f"project:{overlay.id}",
tier=self.tier,
priority=overlay.priority,
path=path,
)
)
docs/reference/workflows.md:154
- This omits two names rejected by the implementation:
_RESERVED_WORKFLOW_IDSalso reservesrunsandsteps, so manifests documented as valid will fail validation. List all three reserved workflow IDs here.
| `extends` | yes | The workflow id this overlay applies to. Uses the same safe-id format as `id`; `overlays`, `runs`, and `steps` are reserved. |
…ttack 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 github#3557. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, supervised)
…teps 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: github#3557 (comment)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, supervised)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
src/specify_cli/workflows/overlays/layer_sources.py:186
- This containment check trusts
self.workflows_diras the root, so if.specifyor.specify/workflowsitself is a symlink, both sides resolve outside the project and the check passes.BaseWorkflowSource.collect()can then read an external workflow when called directly, unlikeProjectOverlaySource, which validates every ancestor. Validate the base storage root and its ancestors againstproject_rootbefore constructing the per-workflow path.
_validate_workflow_id(workflow_id, self.workflows_dir)
workflow_dir = self.workflows_dir / workflow_id
_ensure_contained_dir(workflow_dir, self.workflows_dir)
src/specify_cli/workflows/overlays/_commands.py:119
- Returning the first manifest-ID match makes overlay identity ambiguous. The resolver still loads every file with that ID, so
removeorset-prioritycan report success while another same-ID layer remains active, andworkflow resolveattributes both as the same source. Detect duplicate manifest IDs and reject the ambiguous overlay set instead of silently selecting the first file.
if data.get("id") == overlay_id:
return path
Assisted-by: GitHub Copilot (model: GPT-5.6 Terra, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assisted-by: GitHub Copilot (model: GPT-5.6 Terra, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/specify_cli/workflows/engine.py:749
load_workflow()documents YAML paths as valid sources andFileNotFoundErrorwhen such a file is missing, but a missingfoo.ymlnow reachesresolver.resolve("foo.yml")and raisesValueError("Invalid workflow ID"). Handle a YAML-suffixed source that is not a file before invoking the ID resolver so the public API keeps distinguishing a missing path from an invalid installed ID; traversal-like IDs without a YAML suffix should still be rejected by the resolver.
# Try as an installed workflow ID, resolving any overlays.
resolver = WorkflowResolver(self.project_root)
try:
return resolver.resolve(str(source))
src/specify_cli/workflows/overlays/_commands.py:208
- Because mismatched filenames are explicitly supported, the fallback filename may already belong to a different manifest ID. For example, an existing
ov1.ymlwithid: lintis not returned by_find_overlay_file(..., "ov1"); adding a newid: ov1then silently overwrites and deletes thelintoverlay. Refuse an occupied fallback path (or select an unused filename) before staging the write.
# Reuse an existing .yaml file so we don't create a duplicate .yml layer.
existing = _find_overlay_file(project_root, overlay.extends, overlay.id)
if existing is not None:
target_path = existing
else:
target_path = _ensure_contained_path(
target_dir / f"{overlay.id}.yml", _overlay_root(project_root)
)
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>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/specify_cli/workflows/overlays/layer_sources.py:105
- This validates symlinks but not other resolving aliases for the overlay root. On Windows, a directory junction can make
overlays_root.resolve()point outsideworkflows_rootwhileis_symlink()remains false;_ensure_contained_dir()then compares the child against that already-escaped root and accepts it, allowing resolver reads outside the project. Apply the same resolved containment check used by_resolve_workflows_root()before returning the overlay root.
overlays_root = workflows_root / "overlays"
if overlays_root.is_symlink():
src/specify_cli/workflows/overlays/_commands.py:54
- The CLI has the same containment gap as the resolver: a directory junction (or another resolving alias not reported by
is_symlink()) can redirect.specify/workflows/overlaysoutside the project. Later checks only ensure targets remain under the already-escaped overlay root, so add/update/remove operations can write outside the project. Verify the resolved root remains under the resolved project root here.
_reject_unsafe_workflow_storage(project_root)
root = project_root / ".specify" / "workflows" / "overlays"
_reject_unsafe_dir(root, ".specify/workflows/overlays")
return root
|
No more copilot findings. |
Summary
This PR implements PR 1 of the workflow-overlays plan: a standalone
WorkflowResolverthat lets downstream projects extend installed workflows without editing the installedworkflow.yml.It follows the sequencing agreed with @mnriem in Discussion #3473: build the concrete workflow resolver first, keep it independent of
PresetResolver, and defer any genericLayerStackResolverabstraction to a later PR.What's included
WorkflowResolver+StepListComposer(standalone, no shared abstraction)insert_after,insert_before,replace,removethen/else/steps/cases.*/default; fan-out templates excluded as discussedworkflow overlay add|set-priority|enable|disable|remove|listworkflow resolve <id>for layer attributiontests/workflows/Architecture simplification
This PR enforces a clean separation of concerns:
workflow addinstalls workflows only (no overlay copying)workflow overlay addinstalls overlays only (project-local)Rationale: If upstream controls both the base workflow and shipped overlays, and both get overwritten on
bundle update, there's no reason to ship overlays separately — just put those steps in the base workflow. Overlays only make sense when someone other than the base author adds them.Review findings resolved
All three review findings from the initial PR review are now resolved:
workflow addno longer copies overlays from all call sites (overlay-copying logic removed entirely)--priorityoverride now applied before validation, not after (fixes timing issue where valid CLI priority couldn't override missing/invalid file priority)Test results
tests/test_presets.pyis untouched and remains green.Scope notes
PresetResolveris not modified.spec/planning folder is intentionally excluded from this PR.COMPONENT_KINDSentry; overlays are project-local only, matching the simplified 2-tier architecture.AI assistance disclosure
This implementation was produced with AI assistance (Kimi / opencode-go/kimi-k2.7-code) and has been verified locally by running the full test suite and linting.