feat: YAML workflow definition format with full step type parity#3
feat: YAML workflow definition format with full step type parity#3
Conversation
Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
…DI extension methods Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds YAML workflow definition support to WorkflowFramework.Extensions.Configuration with the goal of matching the existing JSON loader/builder capabilities, plus DI registration helpers and a comprehensive YAML-focused test suite.
Changes:
- Introduces
YamlWorkflowDefinitionLoaderand expandsWorkflowDefinitionBuilderto recognize 11 step categories (including nested step structures). - Extends the shared workflow definition model (
WorkflowDefinition/StepDefinition) to support richer YAML/JSON authoring (e.g.,description,class, nestedthenSteps/elseSteps, approval fields). - Adds DI extension methods for registering loaders, the step registry, and the definition builder; adds an end-to-end style test file covering supported YAML step types and DI wiring.
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/WorkflowFramework.Tests/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| tests/WorkflowFramework.Tests/Configuration/YamlWorkflowFullTests.cs | New tests covering YAML loading/building for all supported step categories + DI registrations + round-trip checks. |
| tests/WorkflowFramework.Tests.Samples/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| tests/WorkflowFramework.Tests.Integration/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| tests/WorkflowFramework.Tests.E2E/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| tests/WorkflowFramework.Dashboard.UITests/packages.lock.json | Lockfile updates for new/updated transitive dependencies (incl. platform-specific Aspire assets). |
| tests/WorkflowFramework.Dashboard.UITests/Features/SampleWorkflows.feature.cs | Regenerated feature code-behind after dependency/tooling changes. |
| tests/WorkflowFramework.Dashboard.Tests/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| tests/WorkflowFramework.Dashboard.Persistence.Tests/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| tests/WorkflowFramework.Dashboard.Api.Tests/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Extensions.Persistence.SqlServer/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Extensions.Persistence.PostgreSQL/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Extensions.Distributed.Redis/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Extensions.Distributed.PostgreSQL/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Extensions.DataMapping.Schema/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Extensions.DataMapping.Formats/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Extensions.Configuration/packages.lock.json | Lockfile updates reflecting new direct DI abstractions dependency. |
| src/WorkflowFramework.Extensions.Configuration/WorkflowFramework.Extensions.Configuration.csproj | Adds Microsoft.Extensions.DependencyInjection.Abstractions to support DI extension methods. |
| src/WorkflowFramework.Extensions.Configuration/WorkflowDefinition.cs | Extends the definition model with description, class, nested conditional steps, and approval fields. |
| src/WorkflowFramework.Extensions.Configuration/ServiceCollectionExtensions.cs | New DI helper methods to register loaders, registry, and builder. |
| src/WorkflowFramework.Extensions.Configuration/IWorkflowDefinitionLoader.cs | Adds YAML loader and significantly expands WorkflowDefinitionBuilder to handle composite step categories. |
| src/WorkflowFramework.Extensions.Agents.Skills/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Extensions.Agents.Mcp/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Dashboard/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Dashboard.Web/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| src/WorkflowFramework.Dashboard.AppHost/packages.lock.json | Lockfile updates for new/updated transitive dependencies (incl. platform-specific Aspire assets). |
| src/WorkflowFramework.Dashboard.Api/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| samples/WorkflowFramework.Samples/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| samples/WorkflowFramework.Samples.WebApi/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| samples/WorkflowFramework.Samples.VoiceWorkflows/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| samples/WorkflowFramework.Samples.TaskStream/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
| benchmarks/WorkflowFramework.Benchmarks/packages.lock.json | Lockfile updates for new/updated transitive dependencies. |
Files not reviewed (1)
- tests/WorkflowFramework.Dashboard.UITests/Features/SampleWorkflows.feature.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
…ps, foreach IEnumerable, single composite in group, condition doc Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com> Agent-Logs-Url: https://github.com/JerrettDavis/WorkflowFramework/sessions/9d214b01-b342-45fc-9d2a-ddce17697ce0
All five review comments addressed in 835f71c:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- tests/WorkflowFramework.Dashboard.UITests/Features/SampleWorkflows.feature.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
…eaf fix for composite types, regenerate lock files Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com> Agent-Logs-Url: https://github.com/JerrettDavis/WorkflowFramework/sessions/301dc8fa-ec98-4d9f-a94d-853554b5d832
All four review comments addressed in a28c10c:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- tests/WorkflowFramework.Dashboard.UITests/Features/SampleWorkflows.feature.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Prefer nested step definitions; fall back to legacy class-name strings | ||
| var thenSteps = stepDef.ThenSteps; | ||
| var elseSteps = stepDef.ElseSteps; | ||
|
|
||
| IStep thenStep; | ||
| if (thenSteps != null && thenSteps.Count > 0) | ||
| { | ||
| thenStep = BuildStepsAsGroupStep("then", thenSteps); | ||
| } | ||
| else if (stepDef.Then != null) | ||
| { | ||
| thenStep = _stepRegistry.Resolve(stepDef.Then); | ||
| } | ||
| else | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Conditional step '{stepDef.Name ?? "unnamed"}' requires 'then', 'thenSteps', or equivalent."); | ||
| } | ||
|
|
||
| if (elseSteps != null && elseSteps.Count > 0) | ||
| { | ||
| var elseStep = BuildStepsAsGroupStep("else", elseSteps); | ||
| builder.If(predicate).Then(thenStep).Else(elseStep); |
There was a problem hiding this comment.
BuildConditionalStep groups nested branches using hard-coded names ("then"/"else"). Because ConditionalStep.Name is derived from its branch step names, multiple conditionals that use thenSteps/elseSteps will end up with identical step names like If(then/else), which can trip duplicate-step-name validation and makes logs/checkpointing ambiguous. Consider deriving the group step names from the parent conditional step (e.g., include stepDef.Name or an index) instead of constant strings, and apply the same fix to the legacy conditional path that calls BuildStepsAsGroupStep("then"/"else", ...).
| "Aspire.Dashboard.Sdk.linux-x64": { | ||
| "type": "Transitive", | ||
| "resolved": "9.2.1", | ||
| "contentHash": "Ps8SfLO0BfNfKhthJ8o+3Rd6ritF2eCafCcWW9vfdT9wcMvn9pujPWPMJo6XVjk1C3zHrPyva9UMTbOT48HKTg==" | ||
| "contentHash": "Lvl58zxyk/Id2uGrgOW3b7czkaZYa/Psje8Cen9zpgdrLZaMwJIh3g/EajqXeis0AVcLB7Hvq9eHc+t/eQ0aFA==" | ||
| }, |
There was a problem hiding this comment.
This lock file now pins Aspire runtime-specific dependencies to *.linux-x64. Since the associated projects don’t specify a RuntimeIdentifier/RuntimeIdentifiers, restoring on Windows/macOS with --use-lock-file can fail due to RID mismatch. If the intent is to be cross-platform, consider regenerating the lock file in a way that includes the intended RIDs or explicitly setting the runtime identifier(s) in the AppHost/test projects so the lock file matches the expected platform.
| @@ -39,11 +39,11 @@ | |||
| "System.IO.Hashing": "9.0.3" | |||
| } | |||
| }, | |||
| "Aspire.Hosting.Orchestration.win-x64": { | |||
| "Aspire.Hosting.Orchestration.linux-x64": { | |||
| "type": "Direct", | |||
| "requested": "[9.2.1, )", | |||
| "resolved": "9.2.1", | |||
| "contentHash": "zql5k3nKMwCBfpODLt/sy/nKsZM272+LV67e3ehv9Ec2+0mUuPRs2DGx9mzvh7sBYBq53pF7A7pn9E3V6FkOXw==" | |||
| "contentHash": "T27Rae/gvbWGmJN+RbO7Abhdp3WQUei+rCThyCxL3rZQ1eaMZzFFqb53XAiBsvvJ4vIRjNIh0RHDD4EXiOySQg==" | |||
There was a problem hiding this comment.
This lock file now pins Aspire runtime-specific dependencies to *.linux-x64. Since the project file doesn’t specify a RuntimeIdentifier/RuntimeIdentifiers, restoring on non-Linux machines with --use-lock-file can fail due to RID mismatch. Consider explicitly setting the runtime identifier(s) for the AppHost project or regenerating the lock file so it matches the platforms you expect contributors/CI to restore on.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 10 comments.
Files not reviewed (1)
- tests/WorkflowFramework.Dashboard.UITests/Features/SampleWorkflows.feature.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread And regenerate packages.lock.json for all projects to resolve merge conflicts. |
…ching, remove unused using, update docs, regenerate lock files Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com> Agent-Logs-Url: https://github.com/JerrettDavis/WorkflowFramework/sessions/6b9cd6f9-1eb2-4241-934f-e9ae06ef7f4c
All changes from the review thread applied and all 63
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- tests/WorkflowFramework.Dashboard.UITests/Features/SampleWorkflows.feature.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var singleDef = steps[0]; | ||
| var typeCategory = singleDef.Type?.ToLowerInvariant() ?? string.Empty; | ||
|
|
||
| // A leaf step can be resolved directly without creating an extra inline workflow. | ||
| // A step is a leaf if: | ||
| // - Type is empty/unset AND Class is set (bare class reference with no type category), OR | ||
| // - Type is "step" (explicit leaf category), OR | ||
| // - Type is set to a value that is NOT a known composite category (treated as a class name). | ||
| // When Type IS a known composite category (e.g., subworkflow, conditional), Class is | ||
| // interpreted as an identifier within that composite dispatch — NOT as a leaf class name. | ||
| var isLeaf = | ||
| (string.IsNullOrEmpty(singleDef.Type) && !string.IsNullOrEmpty(singleDef.Class)) || | ||
| typeCategory == "step" || | ||
| (!string.IsNullOrEmpty(singleDef.Type) && !KnownCategories.Contains(typeCategory)); | ||
|
|
||
| if (isLeaf) | ||
| return ResolveLeafStep(singleDef); | ||
|
|
||
| // Single composite step (e.g., conditional, retry, saga): wrap in an inline workflow | ||
| // so composite dispatch logic runs correctly. |
There was a problem hiding this comment.
BuildStepsAsGroupStep returns the underlying leaf step directly when the group contains a single leaf. This means the groupName (e.g., {parent}_then / {parent}_else) is ignored, so ConditionalStep.Name will still be derived from the leaf step name(s). That can reintroduce duplicate step-name validation failures when multiple conditionals share the same then/else leaf steps. Consider forcing a wrapper step (e.g., InlineWorkflowStep) for conditional branches even when the branch is a single leaf, or add a flag to BuildStepsAsGroupStep to disable the leaf-optimization when the caller needs the group name to be reflected in the step name.
| var singleDef = steps[0]; | |
| var typeCategory = singleDef.Type?.ToLowerInvariant() ?? string.Empty; | |
| // A leaf step can be resolved directly without creating an extra inline workflow. | |
| // A step is a leaf if: | |
| // - Type is empty/unset AND Class is set (bare class reference with no type category), OR | |
| // - Type is "step" (explicit leaf category), OR | |
| // - Type is set to a value that is NOT a known composite category (treated as a class name). | |
| // When Type IS a known composite category (e.g., subworkflow, conditional), Class is | |
| // interpreted as an identifier within that composite dispatch — NOT as a leaf class name. | |
| var isLeaf = | |
| (string.IsNullOrEmpty(singleDef.Type) && !string.IsNullOrEmpty(singleDef.Class)) || | |
| typeCategory == "step" || | |
| (!string.IsNullOrEmpty(singleDef.Type) && !KnownCategories.Contains(typeCategory)); | |
| if (isLeaf) | |
| return ResolveLeafStep(singleDef); | |
| // Single composite step (e.g., conditional, retry, saga): wrap in an inline workflow | |
| // so composite dispatch logic runs correctly. | |
| // Even for a single-step group, build an inline workflow so that the groupName | |
| // is preserved in the resulting step's Name (e.g., for conditional branches). |
| var resolved = Type.GetType(name) | ||
| ?? Type.GetType($"System.{name}"); | ||
|
|
||
| if (resolved != null) return resolved; |
There was a problem hiding this comment.
ResolveExceptionType can return a non-Exception type when Type.GetType(name) (or the System. prefixed lookup) succeeds for an arbitrary type name. That will cause MakeGenericMethod(exType) for ITryCatchBuilder.Catch<TException> to throw at runtime due to the generic constraint. Ensure the resolved type is assignable to System.Exception (otherwise keep searching / fall back to typeof(Exception)).
| if (resolved != null) return resolved; | |
| if (resolved != null && typeof(Exception).IsAssignableFrom(resolved)) | |
| return resolved; |
| "Polly.Core": { | ||
| "type": "CentralTransitive", | ||
| "requested": "[8.6.0, )", | ||
| "resolved": "8.4.2", | ||
| "contentHash": "BpE2I6HBYYA5tF0Vn4eoQOGYTYIK1BlF5EXVgkWGn3mqUUjbXAr13J6fZVbp7Q3epRR8yshacBMlsHMhpOiV3g==" | ||
| }, | ||
| "System.Diagnostics.DiagnosticSource": { | ||
| "type": "CentralTransitive", | ||
| "requested": "[10.0.3, )", | ||
| "resolved": "10.0.3", | ||
| "contentHash": "IuZXyF3K5X+mCsBKIQ87Cn/V4Nyb39vyCbzfH/AkoneSWNV/ExGQ/I0m4CEaVAeFh9fW6kp2NVObkmevd1Ys7A==" | ||
| }, | ||
| "System.Text.Json": { | ||
| "type": "CentralTransitive", | ||
| "requested": "[10.0.1, )", | ||
| "resolved": "10.0.0", | ||
| "contentHash": "1Dpjwq9peG/Wt5BNbrzIhTpclfOSqBWZsUO28vVr59yQlkvL5jLBWfpfzRmJ1OY+6DciaY0DUcltyzs4fuZHjw==" |
There was a problem hiding this comment.
This lockfile has multiple central transitive pins that don't match their resolved versions (e.g., System.Text.Json requested >= 10.0.1 but resolved 10.0.0, and Polly.Core requested >= 8.6.0 but resolved 8.4.2). Resolved versions below the requested minimums suggest an inconsistent/incorrect lockfile generation and can break deterministic restores. Re-run restore to ensure resolved versions satisfy the requested ranges (or update the central pinning if different versions are intended).
WorkflowDefinition.csand related builder changes (prior commits)ConditionXML doc,BuildForEachStep,BuildParallelStep,BuildStepsAsGroupStepisLeaf,BuildTryStepcatch workflow (prior commits)BuildConditionalStep/ legacy conditional: branch group names now use parent step name (e.g.,PaymentDecision_then,PaymentDecision_else) to prevent duplicate "then"/"else" conflictsThenSteps?.Count > 0instead ofThenSteps != nullBuildApprovalStep: usestepDef.Classas registry key when provided, falling back to "approval"using System.ReflectiondirectiveResolveExceptionType: scan loaded assemblies for user types; cache results inConcurrentDictionaryto avoid repeated scansStepDefinition.ClassXML doc to reflect per-category semantics accuratelypackages.lock.jsonfiles viadotnet restore --force-evaluateto resolve merge conflicts andSystem.Text.Jsonversion mismatchesOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.