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
Reimplements workflow composition from main (c00dc055) with one persisted execution tree and one executor. Replaces #4724 and implements the scoped single-run model approved in #4680 (comment), with the explicit resume extension described below.
Included workflows receive private, strictly bound inputs and return only declared outputs alongside workflow/status/error metadata. Targets must exactly match safe, installed, enabled IDs in the current project. Overlay-resolved definitions are bound at the call site, cycles are path-based, and included depth is limited to 16. The approved clarification supersedes the original issue's child-run and run_id-output proposal.
The previous implementation accumulated separate scope identities, result keys, cursor state, snapshot files, and execution paths. Its review identified a real fan-out isolation bug: nested calls had distinct scope keys but shared the caller-result alias.
Each execution occurrence now owns its result, children, and workflow binding. Authored step IDs are local expression aliases; concurrent items have independent contexts. Execution and replay traverse the same tree, including fan-out. Unused legacy execution adapters were removed; their tests exercise the public engine path.
The current diff from the merge base (c00dc055) is 17 files, 3,706 additions / 594 deletions, including 1,181 production additions / 486 deletions. These replace the smaller initial-PR figures: subsequent review added regression coverage and centralized traversal, projection, validation, and event rules. Local planning documents are not included.
Deliberate deviations
A1 — Exact, frozen resume. Resume continues at the unfinished occurrence. Chosen branches, loop iterations, fan-out items, custom expansions, and workflow bindings stay frozen; completed work is not re-run. This applies to all workflows because they share one executor. It goes beyond the approved single-run contract, which retained resume at the enclosing top-level step. Exact resume preserves completed child work and bound definitions across a pause, avoiding repeated command/LLM effects from restarting the enclosing step.
A2 — Fan-out item internals stay item-local. Internal item results no longer enter the shared root context. Public item results (fan:template:index) and the ordered fan.output.results remain available. This addresses the parallel-item collision in feat(workflows): compose installed workflows via a scoped workflow step #4724.
Architecture and contracts
_execution.py owns tree construction/validation, traversal/replay, control flow, workflow scope entry, projections, and event emission. composition.py owns target resolution, strict binding, declared outputs, and CallError. engine.py keeps public lifecycle, input coercion/merge, and RunState persistence.
YAML strings inside JSON preserve definition/expansion scalar types. Workflow children share the bound definition, fan-out items share the template, and later loop iterations share the first body positionally. There are no separate snapshot files or content-addressed identities.
Ordinary resume retains bound inputs and definitions, including after target changes/deactivation/removal. Explicit root input updates re-evaluate original mappings for reached, incomplete calls and merge them over prior bound inputs. Completed calls remain unchanged; unbound calls validate their targets when reached.
Parent inputs, step results, item/fan-in values, and workflow defaults are not implicitly inherited. Runtime conditions such as inside_fan_out are transitive. Child gates require explicit root-to-child verdict mapping.
continue_on_error can handle reported child failures and initial binding/output contract violations. Step exceptions, expression errors (including call input/output expressions), rebind errors, and checkpoint errors propagate. Pauses, aborts, and unknown step types remain terminal. A resolver RuntimeError is not converted into a call failure.
Output-finalization failures can retry without repeating completed child commands. Rebind failure leaves the call node and child subtree unchanged so corrected inputs can be retried.
Only PAUSED and FAILED runs can resume. The earlier crash-resume claim is withdrawn: RUNNING checkpoints are rejected. No ownership/lease mechanism is added. External effects before their completion checkpoint remain at-least-once.
Binding and chosen expansions are checkpointed before child effects; completion is checkpointed before logs. A checkpoint failure prevents further writes from that instance. Fan-out aliases are reconstructed projections, set under the existing run lock and saved by the next checkpoint.
Nested gate/scope reporting follows the tree. Qualified IDs and private workflow/path attribution are shared by log emission; completed replay emits no step events. Unknown step implementations retain an internal retry record without publishing an item result.
Fan-out item aliases (fan:template:index) are reporting-only, not fan-in.wait_for targets. wait_for again requires declared step IDs (the fan-out step's own id; ordered item results at steps.<id>.output.results), matching main. The fan_out_aliases validation allowance is reverted, and FanInStep.execute() rejects : entries at runtime. This closes a stale-read path: in a loop iteration ≥ 1 the authored item alias was never refreshed, so a per-item join silently read the previous iteration's values.
Evidence
test_concurrent_nested_calls_keep_downstream_aliases_local was run against #4724 commit 7ece7a16 via an isolated import path. It failed with consumed outputs {1: 1, 2: 1} instead of {1: 1, 2: 2} and passes here.
Regression coverage includes call exception propagation, rebind preservation, transitive fan-out restrictions, frozen branch/custom expansion replay, qualified aliases/events, shared snapshots, malformed-tree rejection before writes, checkpoint/log failures, strict targets/inputs/outputs, depth/cycles, and legacy adaptation. The composed-gate CLI test covers run → JSON/human status → resume with explicitly mapped input.
The reporting-only change is covered by test_fan_in_rejects_fan_out_item_alias (validation) and test_fan_in_rejects_item_alias_at_runtime (unvalidated execute), plus test_composed_fan_out_joins_container_results and test_fan_in_container_join_in_loop_sees_current_iteration as container-join controls. A loop with differing per-iteration values previously let a per-item join read iteration 0; the construct is now unexpressible.
The final fan-out fix extends test_unknown_fan_out_template_step_always_fails_despite_continue_on_error: four combinations (sequential/parallel, named/unnamed template) failed before the fix because missing implementations incorrectly published item aliases. All pass afterward, including a same-name inherited parent result and successful resume after re-registering the implementation. Direct comparison with c00dc055 now produces only the fan-out container result, with matching per-item failure events.
Current verification
uv sync --extra test, then this worktree's .venv/bin/python -m pytest tests/test_workflows.py tests/workflows tests/specify_cli/workflows -q -p no:cacheprovider: 1,384 passed, 1 skipped.
The full repository run on the initial rewrite was 8,435 passed, 212 skipped, 4 failed; all four also reproduced on clean c00dc055 (preset-update missing-argument wording and three locale-sensitive checksum expectations). The full repository suite has not been rerun after this cleanup. The current result above is for all workflow suites.
Recorded deterministic measurements against the initial PR show a 400-item fan-out dropping from 806 to 405 saves. With a 4,096-byte template, final state size dropped from 2,084,407 to 418,807 bytes. Sharing removes per-item duplication of definitions; progress still grows with item count. These are save/size measurements, not wall-clock guarantees.
The complete scenario-by-scenario main compatibility matrix remains incomplete. The passing suites and focused comparisons do not establish blanket equivalence beyond the specifically verified cases and documented A1/A2 changes.
Intentionally changed tests
test_checkpoint_failure_never_overwrites_committed_progress became test_checkpoint_failure_leaves_running_run_not_resumable; crash-resume expectations were removed/inverted when RUNNING resume was withdrawn.
test_rebind_failure_has_one_failed_caller_outcome now asserts propagation, run FAILED, and an unchanged call node rather than a recoverable call failure.
test_output_failure_retries_only_finalization injects CallError for a contract violation. test_output_expression_failure_retries_only_finalization separately covers propagating expression errors without repeating children.
Former private fan-out-adapter tests use public execute() while retaining their behavioral coverage.
test_replay_restores_fan_out_aliases_from_completed_if and test_resume_restores_completed_fan_out_item_aliases now join the fan-out container instead of per-item aliases (renamed/enlarged); they still assert the reconstructed item aliases in step_results.
test_private_fan_out_aliases_remain_available_to_child_fan_in became test_composed_fan_out_joins_container_results; test_fan_in_rejects_non_item_fan_out_alias became test_fan_in_rejects_fan_out_item_alias, now covering valid-looking fan:template:0 as well.
Out of scope
Crash recovery/run ownership/leases, a dedicated expression-error type and consistent recoverability policy, a direct occurrence-addressed gate-answer API, implicit input propagation or parent-default inheritance, invalidation of completed dependent work, rejecting unknown root resume inputs, an execution-position value object, and migration of private PR checkpoint formats.
AI disclosure
Implemented and updated on behalf of @markuswondrak using OpenCode in autonomous mode with user-directed scope. This update used gpt-6-astra (github-copilot/gpt-6-astra) for review, the final fan-out fix and regression tests, automated verification, commit/push, and this fully AI-drafted PR description. The reporting-only fan-out alias change (reverting the fan_out_aliases allowance, adding the FanInStep runtime guard, rewriting the affected tests, and the accompanying docs) was implemented with deepseek-v4.1-flash (opencode-go/deepseek-v4.1-flash), including automated verification and commit. Intermediate cleanup commits disclose gpt-5.6-terra and deepseek-v4.1-flash individually in their Assisted-by: trailers. The original rewrite and its AI-assisted #4724 history are retained. Human line-by-line review or manual testing is not attested.
Keep invocation results and workflow bindings on the same execution occurrence. Isolate fan-out contexts and resume persisted expansions through one executor.
Assisted-by: OpenCode (model: gpt-6-astra, autonomous)
Preserve the item traversal's projection decision for missing step types. Cover sequential and parallel execution, inherited aliases, unnamed templates, and resume after reinstalling the implementation.
Assisted-by: OpenCode (model: gpt-6-astra, autonomous)
Updated through 6792bea1aaff941f1767a5c632da84d72f183337.
The cleanup narrows call-boundary recovery, preserves incomplete calls on rebind errors, removes RUNNING resume, unifies live/replay traversal and qualified events, validates inconsistent trees before writes, and shares immutable snapshots. The final fix prevents unknown fan-out step implementations from publishing item results while preserving resume after reinstallation.
Current workflow validation: 1,373 passed, 1 skipped; Ruff and git diff --check pass. The final regression's four sequential/parallel and named/unnamed cases failed before the fix and pass afterward. The PR description now explicitly documents exact frozen resume as an extension of the approved top-level-resume contract, the intentionally changed tests, and the remaining limitation: a complete scenario-by-scenario main comparison is not yet recorded. The full repository-suite numbers are identified as historical.
The error-handling follow-up proposal is explained in the reply to the review thread.
Posted on behalf of @markuswondrak by OpenCode (model: gpt-6-astra / github-copilot/gpt-6-astra, autonomous mode with user-directed scope); review summary and PR update fully AI-drafted, final fix and tests AI-authored and automatically verified. Earlier cleanup commits carry their own model disclosures.
Keep fan-out item aliases in the enclosing workflow context without exposing child workflow internals in root results. Normalize unnamed templates to the item ID for result aliases and lifecycle events, and recognize generated item aliases during fan-in validation.
Assisted-by: OpenCode (model: gpt-5.6-terra, autonomous)
Review round summary for @markuswondrak: addressed both open findings in 6b776ce3 (fix(workflows): preserve composed fan-out aliases).
Fan-out item aliases now remain in the enclosing child workflow context for downstream fan-in, while private child aliases remain excluded from root state.step_results. Validation accepts only numeric item aliases produced by a preceding fan-out and retains rejection coverage for invalid aliases.
Unnamed fan-out templates now normalize to item before traversal, so results, callbacks, and lifecycle events consistently use fan:item:<index>.
Added regression coverage for composed fan-out to fan-in, private root-result isolation, invalid aliases, and unnamed-template result/event/callback correlation.
AI disclosure: Posted on behalf of @markuswondrak. OpenCode, model github-copilot/gpt-5.6-terra, autonomous mode with user-directed scope, implemented the fixes and regression tests, ran the stated automated checks, and drafted this comment. Human manual or line-by-line review is not attested.
RunState.save() now wraps write failures in this exception, but command_resume.py:77-80 still catches OSError around the legacy-origin migration save. Because this currently derives from RuntimeError, an I/O failure in that path escapes the CLI's error handler instead of producing the expected clean resume failure.
A JSON-valid corrupted checkpoint can set binding.definition to a number. yaml.safe_load(42) raises AttributeError, which is not normalized by this validator and bypasses the CLI's malformed-state handlers. Validate that the persisted definition is a string before loading it.
An explicit YAML input: null is converted to {} here, even though the workflow-call contract requires a present input value to be a mapping. This silently falls back to child defaults instead of reporting the malformed call; reject None in both validate_call() and this runtime guard while still defaulting an absent key to {}.
Non-ASCII digits allow invalid fan-in references
src/specify_cli/workflows/engine.py:501
str.isdigit() accepts non-ASCII digits, so a reference such as fan:item:١ passes validation even though runtime item IDs are generated with ASCII decimal indices (...:0, ...:1). The fan-in then silently aggregates an empty result; constrain the suffix to ASCII digits.
Review round update for @markuswondrak: addressed the remaining findings in 01bee4d3 (fix(workflows): validate composition checkpoint boundaries).
Restored the missing execution-tree rejection body and narrowed its sole exception to handled failed workflow calls (continue_on_error: true); malformed completed workflow calls with ready or blocked children are rejected before resume writes.
Reject malformed non-string persisted workflow definitions before YAML loading, so corrupted checkpoints remain clean ValueError failures.
Reject explicit input: null workflow-call mappings while preserving an omitted input as an empty mapping.
Restrict generated fan-out references in fan-in.wait_for to ASCII decimal indexes.
Report CheckpointError from the legacy installed-origin migration through the normal clean CLI resume error path.
Added regression coverage for each case, including the permitted handled-failure state. Verification: .venv/bin/python -m py_compile src/specify_cli/workflows/_execution.py src/specify_cli/workflows/composition.py src/specify_cli/workflows/engine.py src/specify_cli/workflows/command_resume.py; .venv/bin/python -m pytest tests/test_workflows.py tests/workflows tests/specify_cli/workflows -q -p no:cacheprovider (1,382 passed, 1 skipped); uvx ruff@0.15.0 check on the changed files; and git diff --check.
AI disclosure: Posted on behalf of @markuswondrak. OpenCode, model github-copilot/gpt-5.6-terra, autonomous mode with user-directed scope, analyzed the review, implemented the fixes and regression tests, ran the stated automated checks, rebased onto the intervening Copilot autofix commit, and drafted this comment. Human manual or line-by-line review is not attested.
Fan-out item aliases (fan:template:N) are not valid fan-in wait_for targets.
In a loop iteration >= 1 the authored alias is never refreshed, so a fan-in
that waited on it silently joined the previous iteration's values. Joins now
use the fan-out step ID and its ordered output.results instead.
Revert the fan_out_aliases validation allowance so wait_for again requires
declared step IDs (matching main), and reject ':' wait_for entries at runtime
in FanInStep.execute() so unvalidated runs fail loudly rather than joining a
stale or empty item result.
Assisted-by: OpenCode (model: deepseek-v4.1-flash, autonomous)
Preserve missing-step outcomes instead of parsing error text
src/specify_cli/workflows/_execution.py:780
This classifies a terminal missing implementation solely from the error text. A valid custom child step that returns FAILED with an error beginning Unknown step type: is consequently treated as unavailable and has call-level continue_on_error forcibly disabled, unlike the same failure at the root. Carry an internal marker/outcome from the missing-registry branch instead of interpreting step-controlled error strings.
Preserve unavailable implementations through validation
src/specify_cli/workflows/composition.py:123
validate_workflow() reports an unavailable custom step as an ordinary validation error, and this wrapper turns it into CallError. Call-level continue_on_error can therefore mark a child whose implementation is already missing at bind time as handled, while the same implementation becoming unavailable after binding is terminal. Preserve a distinct unavailable-implementation outcome through target validation and bypass caller recovery consistently; add an initial-bind regression case.
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-can-waitVerdict: valid and in-scope but deprioritized; held behind the evidence gate
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-can-wait
Summary
Reimplements workflow composition from main (
c00dc055) with one persisted execution tree and one executor. Replaces #4724 and implements the scoped single-run model approved in #4680 (comment), with the explicit resume extension described below.Closes #4680.
Included workflows receive private, strictly bound inputs and return only declared outputs alongside workflow/status/error metadata. Targets must exactly match safe, installed, enabled IDs in the current project. Overlay-resolved definitions are bound at the call site, cycles are path-based, and included depth is limited to 16. The approved clarification supersedes the original issue's child-run and
run_id-output proposal.Current head:
9375b86e.Why replace #4724
The previous implementation accumulated separate scope identities, result keys, cursor state, snapshot files, and execution paths. Its review identified a real fan-out isolation bug: nested calls had distinct scope keys but shared the caller-result alias.
Each execution occurrence now owns its result, children, and workflow binding. Authored step IDs are local expression aliases; concurrent items have independent contexts. Execution and replay traverse the same tree, including fan-out. Unused legacy execution adapters were removed; their tests exercise the public engine path.
The current diff from the merge base (
c00dc055) is 17 files, 3,706 additions / 594 deletions, including 1,181 production additions / 486 deletions. These replace the smaller initial-PR figures: subsequent review added regression coverage and centralized traversal, projection, validation, and event rules. Local planning documents are not included.Deliberate deviations
fan:template:index) and the orderedfan.output.resultsremain available. This addresses the parallel-item collision in feat(workflows): compose installed workflows via a scoped workflow step #4724.Architecture and contracts
_execution.pyowns tree construction/validation, traversal/replay, control flow, workflow scope entry, projections, and event emission.composition.pyowns target resolution, strict binding, declared outputs, andCallError.engine.pykeeps public lifecycle, input coercion/merge, and RunState persistence.inside_fan_outare transitive. Child gates require explicit root-to-child verdict mapping.continue_on_errorcan handle reported child failures and initial binding/output contract violations. Step exceptions, expression errors (including call input/output expressions), rebind errors, and checkpoint errors propagate. Pauses, aborts, and unknown step types remain terminal. A resolverRuntimeErroris not converted into a call failure.PAUSEDandFAILEDruns can resume. The earlier crash-resume claim is withdrawn:RUNNINGcheckpoints are rejected. No ownership/lease mechanism is added. External effects before their completion checkpoint remain at-least-once.fan:template:index) are reporting-only, notfan-in.wait_fortargets.wait_foragain requires declared step IDs (the fan-out step's ownid; ordered item results atsteps.<id>.output.results), matchingmain. Thefan_out_aliasesvalidation allowance is reverted, andFanInStep.execute()rejects:entries at runtime. This closes a stale-read path: in a loop iteration ≥ 1 the authored item alias was never refreshed, so a per-item join silently read the previous iteration's values.Evidence
test_concurrent_nested_calls_keep_downstream_aliases_localwas run against #4724 commit7ece7a16via an isolated import path. It failed with consumed outputs{1: 1, 2: 1}instead of{1: 1, 2: 2}and passes here.Regression coverage includes call exception propagation, rebind preservation, transitive fan-out restrictions, frozen branch/custom expansion replay, qualified aliases/events, shared snapshots, malformed-tree rejection before writes, checkpoint/log failures, strict targets/inputs/outputs, depth/cycles, and legacy adaptation. The composed-gate CLI test covers run → JSON/human status → resume with explicitly mapped input.
The reporting-only change is covered by
test_fan_in_rejects_fan_out_item_alias(validation) andtest_fan_in_rejects_item_alias_at_runtime(unvalidatedexecute), plustest_composed_fan_out_joins_container_resultsandtest_fan_in_container_join_in_loop_sees_current_iterationas container-join controls. A loop with differing per-iteration values previously let a per-item join read iteration 0; the construct is now unexpressible.The final fan-out fix extends
test_unknown_fan_out_template_step_always_fails_despite_continue_on_error: four combinations (sequential/parallel, named/unnamed template) failed before the fix because missing implementations incorrectly published item aliases. All pass afterward, including a same-name inherited parent result and successful resume after re-registering the implementation. Direct comparison withc00dc055now produces only the fan-out container result, with matching per-item failure events.Current verification
uv sync --extra test, then this worktree's.venv/bin/python -m pytest tests/test_workflows.py tests/workflows tests/specify_cli/workflows -q -p no:cacheprovider: 1,384 passed, 1 skipped.uvx ruff@0.15.0 check src testsandgit diff --check: pass.c00dc055.Historical evidence and remaining limitation
c00dc055(preset-update missing-argument wording and three locale-sensitive checksum expectations). The full repository suite has not been rerun after this cleanup. The current result above is for all workflow suites.Intentionally changed tests
test_checkpoint_failure_never_overwrites_committed_progressbecametest_checkpoint_failure_leaves_running_run_not_resumable; crash-resume expectations were removed/inverted whenRUNNINGresume was withdrawn.test_rebind_failure_has_one_failed_caller_outcomenow asserts propagation, runFAILED, and an unchanged call node rather than a recoverable call failure.test_output_failure_retries_only_finalizationinjectsCallErrorfor a contract violation.test_output_expression_failure_retries_only_finalizationseparately covers propagating expression errors without repeating children.execute()while retaining their behavioral coverage.test_replay_restores_fan_out_aliases_from_completed_ifandtest_resume_restores_completed_fan_out_item_aliasesnow join the fan-out container instead of per-item aliases (renamed/enlarged); they still assert the reconstructed item aliases instep_results.test_private_fan_out_aliases_remain_available_to_child_fan_inbecametest_composed_fan_out_joins_container_results;test_fan_in_rejects_non_item_fan_out_aliasbecametest_fan_in_rejects_fan_out_item_alias, now covering valid-lookingfan:template:0as well.Out of scope
Crash recovery/run ownership/leases, a dedicated expression-error type and consistent recoverability policy, a direct occurrence-addressed gate-answer API, implicit input propagation or parent-default inheritance, invalidation of completed dependent work, rejecting unknown root resume inputs, an execution-position value object, and migration of private PR checkpoint formats.
AI disclosure
Implemented and updated on behalf of @markuswondrak using OpenCode in autonomous mode with user-directed scope. This update used gpt-6-astra (
github-copilot/gpt-6-astra) for review, the final fan-out fix and regression tests, automated verification, commit/push, and this fully AI-drafted PR description. The reporting-only fan-out alias change (reverting thefan_out_aliasesallowance, adding theFanInStepruntime guard, rewriting the affected tests, and the accompanying docs) was implemented with deepseek-v4.1-flash (opencode-go/deepseek-v4.1-flash), including automated verification and commit. Intermediate cleanup commits disclose gpt-5.6-terra and deepseek-v4.1-flash individually in theirAssisted-by:trailers. The original rewrite and its AI-assisted #4724 history are retained. Human line-by-line review or manual testing is not attested.