feat(workflows): WorkflowResolver standalone (PR 1)#3557
Open
markuswondrak wants to merge 4 commits into
Open
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)
Contributor
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.
Collaborator
|
@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 |
added 3 commits
July 16, 2026 14:47
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)
Comment on lines
+44
to
+45
| workflow_overlay_dir = self.overlays_dir / workflow_id | ||
| if workflow_overlay_dir.is_symlink(): |
|
|
||
| Returns *path* if safe. Raises typer.Exit on traversal or symlink. | ||
| """ | ||
| if path.is_symlink(): |
| continue | ||
| if path.is_symlink(): | ||
| raise OverlayLoadError(path, ["Symlinked overlay files are not allowed"]) | ||
| data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} |
Comment on lines
+45
to
+48
| return sorted( | ||
| all_layers, | ||
| key=lambda layer: (-layer.priority, layer.source), | ||
| ) |
Comment on lines
+249
to
+253
| # Group edits by anchor, preserving merge order. | ||
| edits_by_anchor: dict[str, list[tuple[OverlayLayer, OverlayEdit]]] = {} | ||
| for layer in overlays: | ||
| for edit in layer.overlay.edits: | ||
| edits_by_anchor.setdefault(edit.anchor, []).append((layer, edit)) |
Comment on lines
+99
to
+108
| An overlay is a YAML file that declares a set of edit operations against the step list of a base workflow. Overlays are applied in priority order (lowest first, highest last); at the same priority, installed overlays are applied before project overlays, so project overlays win ties. | ||
|
|
||
| Overlay files live in two locations: | ||
|
|
||
| | Location | Purpose | Tier | | ||
| | --- | --- | --- | | ||
| | `.specify/workflows/<id>/overlays/*.yml` | Shipped with the installed workflow | `installed-overlay` | | ||
| | `.specify/workflows/overlays/<id>/*.yml` | Project-local customizations | `project-overlay` | | ||
|
|
||
| Project overlays always take precedence over installed overlays at the same priority. |
| specify workflow overlay remove <workflow-id> <overlay-id> | ||
| ``` | ||
|
|
||
| Removes the project overlay file. Installed overlays shipped with a workflow are removed by `specify workflow remove <workflow-id>`, which deletes the entire workflow directory. |
Comment on lines
+265
to
+269
| ### Interaction with Bundles and Updates | ||
|
|
||
| `specify workflow add <local-directory>` copies `workflow.yml` and an optional `overlays/` subdirectory into `.specify/workflows/<id>/`. Overlays shipped this way are discovered automatically as `installed-overlay` layers. | ||
|
|
||
| When an installed workflow is refreshed or reinstalled, project overlays in `.specify/workflows/overlays/<id>/` are preserved because they live outside the installed workflow directory. |
Comment on lines
+112
to
+115
| _reject_unsafe_dir( | ||
| project_root / ".specify" / "workflows" / "overlays", | ||
| ".specify/workflows/overlays", | ||
| ) |
Comment on lines
+97
to
+98
| """Return *path* only if it resolves inside *root*; otherwise raise typer.Exit.""" | ||
| try: |
mnriem
requested changes
Jul 16, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please address Copilot feedback and resolve conflicts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.