feat(workflows): WorkflowResolver standalone (PR 1)#3557
Open
markuswondrak wants to merge 15 commits into
Open
feat(workflows): WorkflowResolver standalone (PR 1)#3557markuswondrak wants to merge 15 commits into
markuswondrak wants to merge 15 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)
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)
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
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)
Collaborator
|
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>
| """Read and parse an overlay YAML file, returning (data, errors).""" | ||
| try: | ||
| content = path.read_text(encoding="utf-8") | ||
| except OSError as exc: |
| if not workflow_overlay_dir.is_dir(): | ||
| return [] | ||
| layers: list[Layer] = [] | ||
| for path in sorted(workflow_overlay_dir.iterdir()): |
Comment on lines
+44
to
+46
| all_layers: list[Layer] = [] | ||
| for source in self._sources: | ||
| all_layers.extend(source.collect(workflow_id)) |
Comment on lines
+190
to
+192
| step_id = step.get("id") | ||
| if isinstance(step_id, str): | ||
| sources.pop(step_id, None) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+79
to
+80
| self.overlays_dir = _resolve_project_overlay_root(self.project_root) | ||
| workflow_overlay_dir = self.overlays_dir / workflow_id |
Comment on lines
+138
to
+141
| try: | ||
| content = path.read_text(encoding="utf-8") | ||
| except OSError as exc: | ||
| return None, [f"Failed to read {path}: {exc}"] |
Comment on lines
+105
to
+107
| overlay, errors = validate_overlay_yaml(data) | ||
| if overlay is None or errors: | ||
| raise OverlayLoadError(path, errors) |
| if not isinstance(id_value, str) or not id_value: | ||
| err_console.print(f"[red]Error:[/red] {label} is required and must be a non-empty string.") | ||
| raise typer.Exit(1) | ||
| if not _SAFE_ID_PATTERN.match(id_value): |
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.