You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Adds a built-in type: workflow step that runs an already-installed workflow as a scoped subtree of the current run. Composition is an engine facility, like fan-out: there is one RunState, one run directory, and one process.
Follow-up to #4680 (maintainer-approved follow-up to discussion #4647). Per the design discussion, this implements the one composed run model rather than the separate-child-run/deterministic-run_id model in the original issue text: a workflow boundary behaves like a function call, with values crossing only through declared inputs and outputs.
The target must resolve to a value the engine captures (a declared input or a
preceding step's captured output such as a shell step's stdout); a prompt step streams its output and returns an empty stdout.
What changed
Composition module (src/specify_cli/workflows/composition.py): reserved output names, path-based cycle + depth (16) checks, strict JSON-safe input binding and declared-output evaluation, and the ExecutionScope data model / persistence helpers.
WorkflowStep registered as a built-in step, plus WorkflowDefinition.outputs validated by validate_workflow.
Scoped execution: _execute_steps operates on an ExecutionScope; the root run is the root scope, nested scopes persist in state.json under workflow_scopes, and each child's immutable definition is stored as a YAML snapshot under snapshots/ (YAML round-trips native scalars JSON cannot encode). Legacy states with an embedded definition still load.
Atomic completion handoff: a completed child's status and its caller-step result are committed in one locked, atomic state write, so a persisted completed child can never lack its caller result (including under concurrent fan-out).
Resume: reuses the bound target and composed definition snapshot; explicit root --input updates rebind reached, incomplete calls through their authored mapping, while an ordinary resume retains persisted child inputs. A failed rebind atomically transitions the paused child and caller step to failed.
Failure handling: nested resolution, binding, cycle/depth, and output-evaluation errors become failed workflow-step results, so the caller's normal continue_on_error applies (and never overrides an abort or bypasses a pause); dynamic-target failures preserve the resolved workflow ID in their output.
CLI reporting: nested scopes in workflow status and the --json payload (existing payload keys stay stable when no scopes exist).
Docs: step table entry and a composition section covering scope isolation, declared outputs, resume, and limits.
workflow step type registered, documented, and tested.
Resolves installed IDs only; rejects unknown, path-like, and disabled workflows.
input: supports expressions and validates against the target schema (strict: undeclared names rejected).
Output exposed and readable downstream via the workflow step's output.
Resume reuses the bound snapshot; completed calls are reused, paused/failed calls retry at their local index.
Path-based cycle detection plus a max-depth backstop (16).
Failed/aborted child surfaces as a failed/aborted parent step.
Note: output shape is the declared outputs merged onto {workflow, status} (plus aborted on abort), not the {status, run_id, steps} shape in the original issue, because there is no child run — this follows the approved design decision.
Test evidence
New tests/workflows/test_workflow_composition.py (113 tests) covers helpers, literal/runtime targets, scope isolation, output shapes, continue_on_error at both boundaries, resolution/cycle/depth failures, persistence + atomic handoff (including a forced concurrent fan-out save during the handoff window), YAML definition snapshots, JSON-safe declared outputs and input mappings, resume rebinding, input propagation through nested calls, repeated calls, and CLI reporting.
Full-suite baseline before the final output-safety regressions: 8426 passed, 212 skipped, 4 pre-existing environment-specific failures
uvx ruff@0.15.0 check src tests → All checks passed
Full-suite failures are unrelated and environment-specific (a Typer/Click argument-error wording mismatch and a checksum-digest command test), none touching workflow code.
AI disclosure
Implemented with opencode (model: deepseek-v4.1-flash, human-supervised) — code generation, tests, and documentation, on behalf of @markuswondrak. The design and implementation plan were reviewed with the maintainer in discussion #4647.
Add a built-in `type: workflow` step that runs an installed workflow as a
scoped subtree of the current run. Composition is an engine facility: one
RunState, one run directory, and one process.
- Add `composition` helpers: reserved output names, path-based cycle and
depth checks, strict input binding, and declared-output evaluation.
- Add `WorkflowStep`, registered in the built-in step registry, and
`WorkflowDefinition.outputs` validated by `validate_workflow`.
- Refactor `_execute_steps` to operate on an `ExecutionScope`; the root run
is the root scope and nested scopes persist in `state.json` under
`workflow_scopes` with backward-compatible load.
- Commit a completed child's status and its caller-step result in one
locked, atomic state write.
- On resume, reuse the bound target and composed definition snapshot;
explicit root input updates rebind reached, incomplete calls through
their authored mapping.
- Turn nested resolution, binding, cycle/depth, and output-evaluation
errors into failed workflow-step results so `continue_on_error` applies.
- Report nested scopes in `workflow status` and the `--json` payload.
- Document the step type, scope isolation, outputs, and composition limits.
Closesgithub#4680
Assisted-by: opencode (model: deepseek-v4.1-flash, supervised)
- Escape authored scope IDs before rendering `workflow status` so a valid
step ID containing Rich markup (e.g. `[/red]`) no longer raises
MarkupError or misformats the scope tree.
- Bound-check a nested scope's persisted `current_step_index` against its
snapshot's step count, mirroring the root resume check, so a malformed
state cannot silently complete a child with an empty step slice.
- Replace the composition doc example that selected a workflow from a
`prompt` step (whose stdout is always empty) with a declared workflow
input, and note why.
- Restore the process-global step registry after the custom-step scope test
via monkeypatch.setitem instead of leaking `scope-probe`.
Assisted-by: opencode (model: deepseek-v4.1-flash, supervised)
Addressed all four review findings in commit 2c4add6:
command_status._render_scopes now escapes the scope key and workflow ID with _escape_markup, matching workflow run, so a bracketed authored step ID no longer raises MarkupError or misformats the scope tree.
composition._validate_scope_record now bound-checks a nested scope's persisted current_step_index against its snapshot's step count, mirroring the root resume check in engine.resume, closing the silent-completion path for a malformed state.
The composition docs example no longer selects its target from a prompt step (whose stdout is always empty); it now uses a declared workflow input, with a note on which outputs are capturable. The equivalent example in the PR description was corrected too.
The custom-step scope test registers scope-probe via monkeypatch.setitem, so the process-global step registry is restored instead of leaking.
Regression tests were added for the escaping and the nested-index bound.
The declared output is only inspected directly from the final state, so this does not verify the acceptance criterion that callers can consume it in a downstream expression. Add a parent step after call that references {{ steps.call.output.echoed }} and assert its resolved output; this would catch failures to publish the workflow-step result into the caller context before continuing.
Update workflow guides for the 13th built-in
src/specify_cli/workflows/__init__.py:73
Registering the 13th built-in leaves the repository's other workflow guides stale: workflows/README.md:88 and workflows/ARCHITECTURE.md:82 still state that there are 12 built-ins and their step tables omit workflow. Update those inventories so users and maintainers see the same supported step set everywhere.
Remove or add missing design decisions reference
src/specify_cli/workflows/composition.py:4
This module points readers to spec/workflow_composition/design_decisions.md, but that path does not exist in the repository. Remove the stale reference or add the referenced design document so the implementation notes are navigable.
Remove or add missing implementation plan reference
tests/workflows/test_workflow_composition.py:5
This test module references spec/workflow_composition/implementation_plan.md, but that file is absent from the repository. Remove the broken reference or include the plan in this PR.
- Add a downstream consumption test: a parent step after the workflow call
evaluates `{{ steps.call.output.echoed }}`, so a failure to publish the
completed child's output into the caller context before continuing is now
caught (final-state inspection alone could not).
- Update the built-in step inventories from 12 to 13 and add the `workflow`
step to workflows/README.md and workflows/ARCHITECTURE.md.
- Drop the two dangling references to spec/workflow_composition/ (the design
decisions and implementation plan are not part of this change).
Assisted-by: opencode (model: deepseek-v4.1-flash, supervised)
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Malformed workflow-step input can be silently discarded when the engine executes an unvalidated definition.
Review effort: Balanced Findings: None
Previously missed (1)
In code that hasn't changed since last review
Reject malformed workflow input instead of silently using defaults
src/specify_cli/workflows/composition.py:189
WorkflowEngine.execute() may receive unvalidated definitions, but this helper converts every malformed input value (for example, a list) to an empty mapping. The call then runs with defaults instead of failing, even though the same configuration is rejected by validate_workflow_call_config. Preserve the omitted/null case, but raise ValueError for other non-mappings so WorkflowStep.execute and the resume path produce a failed workflow-step result.
…lper
`evaluate_input_mapping` previously coerced any non-mapping `input` (for
example a list) to an empty mapping. Because `WorkflowEngine.execute` accepts
unvalidated definitions, a malformed `input:` ran the child with defaults
instead of failing, even though `validate_workflow_call_config` rejects the
same shape at definition time.
Preserve the omitted/null case, raise `ValueError` otherwise. All three call
sites (`WorkflowStep.execute`, and the initial and resume binding paths) map
the error to a failed workflow-step result.
Assisted-by: opencode (model: deepseek-v4.1-flash, supervised)
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Dynamic shell targets, custom-step state loading, and nested gate JSON reporting have unresolved correctness issues.
Review effort: Balanced Findings: None
Previously missed (3)
In code that hasn't changed since last review
Expose nested gate details when composed workflows pause
src/specify_cli/workflows/_commands.py:942
A pause inside a composed workflow loses the existing machine-readable gate contract. The root current_step_id points to the workflow call, so _gate_outcome(state) returns None, while this scope summary exposes only IDs/status and omits the child's gate message, options, and choice. Consequently workflow run/status --json can report paused without enough information for an orchestrator to drive the nested gate, unlike the same workflow run directly. Preserve and surface the active nested gate details (including its scope path) in the JSON payload.
Avoid registry-dependent validation when loading persisted state
src/specify_cli/workflows/composition.py:590
Loading persisted state now semantically revalidates every nested definition against the process-global step registry. workflow run loads installed custom steps, so a composed child containing one can be persisted successfully, but workflow status <run-id> does not call load_custom_steps; in a fresh process this validation reports the custom type as invalid and status cannot load the run. Persisted-state loading should validate the snapshot's structural safety without depending on currently registered step implementations (or every load path must register custom steps first).
Normalize shell output before validating dynamic targets
Dynamic targets taken from shell.output.stdout reject the trailing newline produced by normal commands such as echo child. ShellStep preserves proc.stdout verbatim, and _ID_PATTERN.fullmatch() rejects "child\n", so the documented shell-driven selection only works when authors know to use printf (as this test suite does). Normalize a resolved string before validating it, and add a runtime-target test using echo child.
`ShellStep` returns captured stdout verbatim, so `echo child` yields
`"child\n"` and `_ID_PATTERN.fullmatch` rejected it, forcing authors to use
`printf` for dynamic workflow selection. Trim surrounding whitespace on the
resolved target before validating it; literal targets are already
pattern-safe, so this is a no-op for them.
Assisted-by: opencode (model: deepseek-v4.1-flash, supervised)
`RunState.load` re-ran `validate_workflow` on every persisted nested
definition, which rejects any step type absent from the process-global
`STEP_REGISTRY`. `workflow run`/`resume` call `load_custom_steps`, but
`workflow status` does not, so a run whose composed child used a project
custom step could be persisted yet not reloaded in a fresh process.
Replace the semantic pass with a structural check covering only the shapes
needed to slice and deserialize a scope (mapping snapshot sections, a list of
step mappings each with a non-empty id). Step types are no longer restricted
to the currently registered implementations.
Assisted-by: opencode (model: deepseek-v4.1-flash, supervised)
A pause inside a composed workflow leaves the root resting on the `workflow`
call step, so `_gate_outcome` returned None and `scopes` carried only
ids/status. `workflow run/status --json` therefore reported `paused` without
the gate message/options/choice, unlike the same workflow run directly.
Search the serialized scope tree for the active gate when the root is not one,
returning its detail plus the `scope_path` of invocation ids. Also expose each
scope's `current_step_id` in the summary.
Assisted-by: opencode (model: deepseek-v4.1-flash, supervised)
Addressed the three findings from this review in commits 415e8e3, be4f2a8, and 2a6d2ae:
415e8e38 trims a dynamically resolved workflow target before ID validation, so shell-driven selection works with echo child (trailing newline), not only printf child. The docs note the trimming.
be4f2a81 replaces the registry-dependent validate_workflow pass on persisted nested definitions with structural validation (mapping sections, a list of steps each with a non-empty id). workflow status no longer needs the project's custom steps registered to load a run whose composed child used one; the fail-closed shape checks are preserved.
2a6d2ae8 surfaces the active nested gate (message/options/choice plus scope_path) in the run/resume/status--json payload, and adds current_step_id to each scope summary. Root-level gate payloads are unchanged.
This example references inputs.report and inputs.slug, but the shown caller declares only target. The engine builds the caller scope solely from declared inputs, so these expressions resolve to None and commonly fail the child's strict string input binding. Declare report and slug in this example (or replace them with values that are actually in scope) so the documented workflow is runnable.
A scope paused inside a nested control-flow body (if/switch/loop) keeps the
enclosing step's index while `current_step_id` points at the inner gate, so
deriving the id from the snapshot index surfaced the enclosing step and hid
the gate from JSON clients.
Persist `current_step_id` in `ExecutionScope._serialize`, restore and
structurally validate it in `deserialize_scope`/`validate_serialized_scopes`,
and prefer it in `_scope_current_step_id` with the index derivation kept as a
fallback for older states.
Assisted-by: opencode (model: deepseek-v4.1-flash, supervised)
When a dynamic target resolves successfully but registry resolution, input evaluation, or binding fails, this catch reports the authored expression (for example {{ steps.pick.output.stdout }}) as output.workflow instead of the resolved workflow ID. Other failure paths and successful calls expose the resolved target, so continued callers cannot reliably inspect which workflow was selected. Preserve the evaluated target in this failure payload and cover a failing dynamic-target call.
Corrected the stale _gate_details() documentation in d8ce4717270b697c3aad0796780838fc68bda9fe. The helper remains defensive for unvalidated or legacy records; its docstring no longer incorrectly claims that GateStep never coerces message values.
The scope cache uses the workflow step’s ID, but loop/fan-out namespacing only reaches the directly expanded step. A workflow call beneath an if therefore keeps the same ID across iterations. Later iterations reuse the first completed scope rather than executing their own call and binding their inputs.
A local reproduction with three iterations of while → if → workflow executes the child only once. A fan-out over ["a", "b", "c"] with if → workflow processes only "a", while reporting completion.
Please include the enclosing execution path in invocation identity, keeping it stable when replaying the same invocation on resume.
Only completed scopes receive terminal-state handling. An existing ABORTED scope is rebound, reset to RUNNING, and executed again.
This is reachable through concurrent fan-out: the first child pauses, while the second child aborts at a rejected gate. Item-order handling leaves the parent paused. Resuming with verdict=approve then restarts the aborted child, executes its post-gate command, and completes the run.
Please preserve aborted scopes as terminal and propagate their existing abort result instead of rebinding or re-executing them.
Runtime expression errors in the child’s body escape directly to the root engine rather than becoming failed workflow-step results.
For example, a child that captures non-JSON stdout and evaluates it through from_json raises ValueError. Even with continue_on_error: true on the caller, the fallback step never executes, no caller result is recorded, and the persisted child remains running despite the root run failing.
Please handle these runtime failures at the composition boundary on both initial execution and resume, recording the failed child and caller result so normal caller recovery applies. Interruption and explicit-abort behavior should remain intact.
Drafted on behalf of @mnriem by GitHub Copilot (model: GPT-6 Astra, autonomous); review fully AI-drafted.
Implemented the three workflow-composition findings in 3f3fdaa4:
Invocation identity now includes the enclosing control-flow path, so calls beneath if execute separately per loop iteration and fan-out item while retaining the same identity on replay.
Aborted child scopes are terminal on resume and are propagated without rebinding or executing later child steps.
Runtime failures inside a composed child are isolated at the workflow-call boundary, persisted atomically with the caller result, and honor continue_on_error on execution and resume.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The extensive concurrency, persistence, and resume changes require final human validation, and one publishing-guide inconsistency remains.
Review effort: Balanced Findings: None
Previously missed (1)
In code that hasn't changed since last review
Document the new workflow step type in the publishing guide
docs/reference/workflows.md:542
The publishing guide still lists only the previous 12 valid step types (workflows/PUBLISHING.md:93) and omits workflow. That guide is used by workflow authors to validate publishable definitions, so this new built-in is documented inconsistently; please add workflow there as part of this change.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Nested gate reporting can select an inactive aborted sibling, and long valid workflow IDs can exceed snapshot filename limits.
Review effort: Balanced Findings: None
Previously missed (2)
In code that hasn't changed since last review
Match resumed scope selection to the root run outcome
src/specify_cli/workflows/_commands.py:1050
This treats both paused and aborted scopes as candidates without matching the root run's outcome. Concurrent fan-out can leave one composed child paused and a sibling aborted (the new test_aborted_child_is_not_restarted_when_resuming_fan_out already demonstrates that mixed state); because scope insertion order is thread-dependent, a paused run can then report the aborted sibling's rejected gate in its JSON payload instead of the gate that actually paused the run. Pass the root outcome through this recursion and only select scopes with the corresponding status (and add a mixed-sibling reporting regression test).
Prevent snapshot filenames from exceeding filesystem name limits
src/specify_cli/workflows/composition.py:421
The workflow ID has no length bound, but this adds 17 characters to it for the snapshot filename. An installed ID of 239–255 ASCII characters is valid and fits as its directory name, yet composition fails here with ENAMETOOLONG on common 255-byte-component filesystems. Use a digest-only filename or truncate the readable prefix so every accepted workflow ID remains composable.
Keep the established workflow ID contract and bound composition snapshot filenames with a full digest instead. Existing readable snapshot references remain loadable.
Assisted-by: GitHub Copilot (model: gpt-5.6-sol, autonomous)
Addressed the workflow-ID compatibility regression in commit 7c5f9f5c.
The 200-character global workflow-ID cap has been removed. New composition snapshots now use a fixed-length full SHA-256 basename derived from the invocation path and target workflow ID, so snapshot path safety no longer changes the established workflow-ID contract. Existing readable <workflow-id>-<digest>.yml snapshot references remain loadable. Regression coverage now verifies composition with a 239-character installed ID and loading persisted 255-character IDs.
Avoid returning stale paused gates after branch-switch resume
src/specify_cli/workflows/_commands.py:1052
Filtering scopes only by status can return an abandoned gate after resume. For example, if an if initially selects workflow A and pauses there, then resume --input changes the condition so workflow B is selected and pauses, both A and B remain paused in workflow_scopes; insertion-order traversal returns A's stale gate even though the run now rests on B. Persist or derive the active invocation path (and add a branch-switch resume regression test) rather than treating every same-status scope as active.
Sequential resume now persists the selected expansion and nested invocation path, retries the active blocker, and preserves completed prefixes. Updating an input while paused inside a selected branch no longer switches that branch and leaves an abandoned paused child behind. Long expansions retain loadable scope indices, and cursor progress is committed atomically with results and child/caller handoffs.
The local review follow-up also preserves the last-top-level-step retry boundary after child output-evaluation errors, prevents failed or paused results from activating returned child steps, and restores failure/abort/continue-on-error log events after the state commit. Legacy runs and loop/fan-out execution retain their existing coarse-resume behavior.
Validation: .venv/bin/python -m pytest tests/test_workflows.py tests/workflows tests/specify_cli/workflows -q passed with 1,405 passed and 1 skipped. Changed Python files pass Ruff 0.15.0, and git diff --check passes. Repository-wide Ruff reports the existing unused re import in src/specify_cli/workflows/_commands.py:11.
Posted on behalf of @markuswondrak by OpenCode (model: gpt-6-astra, autonomous); this review-round comment is fully AI-drafted. OpenCode reviewed the local changes, authored the follow-up fixes and regression tests, ran the reported checks, and committed and pushed this update. Human line-by-line review is not attested.
The replacement reimplements the agreed composition contract from main with one execution tree and one executor. Results and workflow bindings belong to the same occurrence, with item-local expression contexts for concurrent fan-out. The nested output-collision regression fails on this branch and passes in the replacement. The diff is reduced from 5,990 added lines to 1,655 added / 434 removed; implementation and test evidence are in the new PR.
Closing this implementation in favor of commit 7aa424d in #4764. The original branch remains available for comparison.
Posted on behalf of @markuswondrak by OpenCode (model: gpt-6-astra, autonomous); this replacement/closure comment is fully AI-drafted.
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
triage-nice-to-haveVerdict: evidence-backed fix or greenlit feature — land after review
3 participants
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.
mnriem
added
the
triage-nice-to-have
Summary
Adds a built-in
type: workflowstep that runs an already-installed workflow as a scoped subtree of the current run. Composition is an engine facility, likefan-out: there is oneRunState, one run directory, and one process.Follow-up to #4680 (maintainer-approved follow-up to discussion #4647). Per the design discussion, this implements the one composed run model rather than the separate-child-run/deterministic-
run_idmodel in the original issue text: a workflow boundary behaves like a function call, with values crossing only through declaredinputsandoutputs.What changed
src/specify_cli/workflows/composition.py): reserved output names, path-based cycle + depth (16) checks, strict JSON-safe input binding and declared-output evaluation, and theExecutionScopedata model / persistence helpers.WorkflowStepregistered as a built-in step, plusWorkflowDefinition.outputsvalidated byvalidate_workflow._execute_stepsoperates on anExecutionScope; the root run is the root scope, nested scopes persist instate.jsonunderworkflow_scopes, and each child's immutable definition is stored as a YAML snapshot undersnapshots/(YAML round-trips native scalars JSON cannot encode). Legacy states with an embedded definition still load.completedchild can never lack its caller result (including under concurrentfan-out).--inputupdates rebind reached, incomplete calls through their authored mapping, while an ordinary resume retains persisted child inputs. A failed rebind atomically transitions the paused child and caller step tofailed.continue_on_errorapplies (and never overrides an abort or bypasses a pause); dynamic-target failures preserve the resolved workflow ID in their output.workflow statusand the--jsonpayload (existing payload keys stay stable when no scopes exist).Acceptance criteria from #4680
workflowstep type registered, documented, and tested.input:supports expressions and validates against the target schema (strict: undeclared names rejected).Test evidence
New
tests/workflows/test_workflow_composition.py(113 tests) covers helpers, literal/runtime targets, scope isolation, output shapes,continue_on_errorat both boundaries, resolution/cycle/depth failures, persistence + atomic handoff (including a forced concurrentfan-outsave during the handoff window), YAML definition snapshots, JSON-safe declared outputs and input mappings, resume rebinding, input propagation through nested calls, repeated calls, and CLI reporting..venv/bin/python -m pytest tests/workflows/test_workflow_composition.py→ 113 passed.venv/bin/python -m pytest tests/test_workflows.py tests/specify_cli/workflows tests/workflows tests/specify_cli/bundles/test_references.py→ 1375 passed, 1 skippeduvx ruff@0.15.0 check src tests→ All checks passedFull-suite failures are unrelated and environment-specific (a Typer/Click argument-error wording mismatch and a checksum-digest command test), none touching workflow code.
AI disclosure
Implemented with opencode (model: deepseek-v4.1-flash, human-supervised) — code generation, tests, and documentation, on behalf of @markuswondrak. The design and implementation plan were reviewed with the maintainer in discussion #4647.