diff --git a/README.md b/README.md index 4487e42..7f9fd84 100644 --- a/README.md +++ b/README.md @@ -44,7 +44,7 @@ func main() { - **Tiny interface.** A step is anything with `Do(context.Context) error`. No codegen, no DSL. - **Dependencies as code.** `Step(x).DependsOn(y)`, `Pipe(...)`, `BatchPipe(...)`, `If/Switch`. -- **Concurrent by default.** Each ready step runs in its own goroutine; cap with `MaxConcurrency`. +- **Concurrent by default.** Each ready step runs in its own goroutine; cap with `Option.MaxConcurrency`. - **Per-step controls.** Retry with backoff, timeout, conditions, typed `Input`/`Output`, before/after hooks. - **Composable.** A `Workflow` is itself a `Step`, so workflows nest — interceptors and options flow into children automatically. @@ -88,18 +88,41 @@ calling it again merges new config into existing steps. ## Workflow knobs -Set fields on `flow.Workflow` before `Do`: - -| Field | Effect | -|------------------------|------------------------------------------------------------------------------| -| `MaxConcurrency` | Max running steps at once. `0` = unlimited. | -| `DontPanic` | Recover panics into `ErrPanic` instead of crashing. | -| `SkipAsError` | Treat `Skipped` as workflow failure (default: skipped is OK). | -| `DefaultOption` | Base `*StepOption` applied (then overridable) to every step. | -| `StepInterceptors` | Wrap full step lifetime (across retries). | -| `AttemptInterceptors` | Wrap each individual attempt (`Before → Do → After`). | -| `IsolateInterceptors` | When nested as a child step, don't inherit parent interceptors. | -| `Clock` | Inject a clock for deterministic tests. | +Workflow-level configuration lives in `flow.Workflow.Option` (type `WorkflowOption`). +Scalar fields are pointer-typed so unset / explicit-zero are distinguishable; +slice fields stay slices. When a Workflow is used as a sub-workflow, the +parent's Option propagates into the child via `WorkflowOptionReceiver` — +nil scalars take the parent's value, slices are parent-prepended. Set +`Option.DontInherit = true` on a child to opt out. + +```go +mc := 4 +dontPanic := true +w := &flow.Workflow{Option: flow.WorkflowOption{ + MaxConcurrency: &mc, + DontPanic: &dontPanic, +}} +``` + +| Field | Effect | +|--------------------------------|------------------------------------------------------------------------------| +| `Option.MaxConcurrency` | Max running steps at once. `nil` or `0` = unlimited. | +| `Option.DontPanic` | Recover panics into `ErrPanic` instead of crashing. | +| `Option.SkipAsError` | Treat `Skipped` as workflow failure (default: skipped is OK). | +| `Option.StepDefaults` | Base `*StepOption` applied (then overridable) to every step. | +| `Option.StepInterceptors` | Wrap full step lifetime (across retries). | +| `Option.AttemptInterceptors` | Wrap each individual attempt (`Before → Do → After`). | +| `Option.Mutators` | Cross-cutting per-type Step contributions (see `flow.Mutate`). | +| `Option.DontInherit` | When nested as a child step, don't inherit any of the parent's Option. | +| `Option.Clock` | Inject a clock for deterministic tests. | + +### Sub-workflows + +Embed `flow.Workflow` directly in your own struct and call `Add` at +construction time — this is the recommended pattern. The embedded +`Workflow` satisfies `WorkflowOptionReceiver`, so the parent's Option +flows in automatically. The legacy `flow.SubWorkflow` type is deprecated +and will be removed in the next major version. ## Learn more diff --git a/build_step.go b/build_step.go index f7d271f..238bf2d 100644 --- a/build_step.go +++ b/build_step.go @@ -17,6 +17,14 @@ package flow // // The StepBuilder is embedded in Workflow itself, so Workflow.Add transparently // invokes BuildStep on every newly seen step. +// +// Deprecated: StepBuilder, the [StepBuilder.BuildStep] method, the user-facing +// `BuildStep()` hook, and [SubWorkflow] are all scheduled for removal together +// in the next major version of go-workflow. Use [Mutate] for cross-cutting +// modification and construct sub-workflows at the call site instead (embed +// [Workflow] directly, or build inside Do() guarded by sync.Once). Behavior +// is unchanged in this release; Workflow still embeds StepBuilder and +// Workflow.Add still calls BuildStep on every newly seen Step. type StepBuilder struct{ built Set[Steper] } // BuildStep walks the tree of step (pre-order) and triggers BuildStep() on diff --git a/docs/superpowers/plans/2026-05-12-workflow-option.md b/docs/superpowers/plans/2026-05-12-workflow-option.md new file mode 100644 index 0000000..38d916a --- /dev/null +++ b/docs/superpowers/plans/2026-05-12-workflow-option.md @@ -0,0 +1,1663 @@ +# Workflow Option Consolidation Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Consolidate `Workflow`'s nine top-level configuration fields into a single `WorkflowOption` struct, unify Mutator/Interceptor propagation under one `WorkflowOptionReceiver` interface, deprecate `SubWorkflow`, and make every scalar configuration option (including `DontPanic`, `MaxConcurrency`) cleanly inheritable from parent to sub-workflow. + +**Architecture:** Add `workflow_option.go` containing `WorkflowOption` (pointer scalars + slice fields + `DontInherit bool`) and the `WorkflowOptionReceiver { InheritOption(parent WorkflowOption) }` interface. `Workflow` exposes one named field `Option WorkflowOption` and implements the receiver; merge rules are scalar-nil-inherits / slices-prepended / `DontInherit`-no-op. The scheduler propagates Option once per child in the parent's `Do()` prologue via a new `findOptionReceiver` walker, replacing the two ad-hoc dispatch sites for Mutators and Interceptors. `Do()` snapshots `w.Option` and restores via defer, eliminating the `inheritedStep`/`inheritedAttempt` side fields. `SubWorkflow` is deprecated but kept for one release; old receiver interfaces and `Prepend*` methods are removed. + +**Tech Stack:** Go 1.22+ (using generics), `github.com/benbjohnson/clock`, `github.com/stretchr/testify`. + +**Spec references:** +- Brainstorming: `docs/superpowers/specs/2026-05-12-workflow-option-design.md` +- OpenSpec change: `openspec/changes/2026-05-12-workflow-option/` + +--- + +## File Layout + +| File | Role | Action | +|---|---|---| +| `workflow_option.go` | New: `WorkflowOption` struct, `WorkflowOptionReceiver` interface, `findOptionReceiver`, `prependSlice` helper | Create | +| `workflow.go` | Remove 9 top-level fields + `inheritedStep`/`inheritedAttempt` + `PrependMutators`/`PrependInterceptors`. Add `Option WorkflowOption`, `InheritOption`. Rewrite `Do()` prologue (snapshot/restore + propagation pass). Rewrite scalar reads via accessor helpers. Update `Reset()` godoc. Update `SubWorkflow` (deprecated; new `InheritOption`; remove `PrependMutators`/`PrependInterceptors`). Remove Mutator dispatch at `:641` and Interceptor dispatch at `:768`. Simplify `effective*Interceptors`. | Modify | +| `interceptor.go` | Remove `InterceptorReceiver`, `findInterceptorReceiver`. Update godoc on `StepInterceptor` to reference `Option.StepInterceptors`. | Modify | +| `mutator.go` | Remove `MutatorReceiver`. Update godoc referencing it. | Modify | +| `workflow_option_test.go` | New: table-driven `InheritOption` matrix, multi-level nesting, snapshot/restore, scalar-inheritance behavior change (parent DontPanic flows to child), Reset contract. | Create | +| `workflow_options_test.go` | Migrate every `&Workflow{Field: ...}` literal to `Option: WorkflowOption{...}` with pointer wrappers. | Modify | +| `workflow_mutator_test.go` | Migrate `w.Mutators = ...` → `w.Option.Mutators = ...`. Rewrite `PrependMutators` exercises to use `InheritOption`. | Modify | +| `workflow_test.go`, `branch_test.go`, `wrap_test.go`, `execution_model_test.go`, `step_configuration_test.go`, `retry_test.go`, `condition_test.go`, `noop_test.go`, `mutator_test.go`, `error_test.go`, `name_test.go`, `testutil_test.go` | Migrate any references to the removed fields/interfaces. | Modify as needed | +| `example/*.go` | Migrate examples that use the nine former fields, `SubWorkflow`, or the removed interfaces. | Modify | +| `openspec/specs/workflow-options/spec.md`, `openspec/specs/composite-steps/spec.md`, `openspec/specs/mutators/spec.md`, `openspec/specs/step-interceptor/spec.md` | Apply the MODIFIED / ADDED / REMOVED requirements from the change's spec deltas. | Modify | + +--- + +## Approach Notes + +**Order of work:** add new types first (so the rest can reference them), then rewrite `Workflow` internals (preserve behavior under the new shape), then propagation (delete two old dispatch sites, add one new one), then removals, then `SubWorkflow` deprecation, then test/example/spec migration, then verification. + +**TDD strategy:** for the *new* behaviors (`InheritOption` merge rules, snapshot/restore, scalar inheritance) we write failing tests first. For the *mechanical migration* (renaming `w.MaxConcurrency` → `w.Option.MaxConcurrency` etc.) we don't add new tests — the existing test suite is the regression net; we run `go test ./...` after each task to confirm green. + +**Commit cadence:** one commit per task. Each commit must leave the tree compiling (or, where unavoidable mid-refactor, clearly note the staging point so the next task fixes it). For the big "remove old fields" step we batch the move + accessor rewrite + test migration into one commit because the tree wouldn't compile otherwise. + +--- + +## Task 1: Add WorkflowOption types and helpers (no wiring yet) + +**Files:** +- Create: `workflow_option.go` +- Test: `workflow_option_test.go` + +This task introduces the new types in isolation. They are not yet referenced from `Workflow`, so the existing code continues to compile and pass tests. + +- [ ] **Step 1.1: Write the failing test for `prependSlice`** + +Create `workflow_option_test.go`: + +```go +package flow + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestPrependSlice(t *testing.T) { + t.Run("parent nil returns child slice unchanged-shape", func(t *testing.T) { + child := []int{1, 2} + got := prependSlice[int](nil, child) + assert.Equal(t, []int{1, 2}, got) + }) + + t.Run("child nil returns parent-prepended fresh slice", func(t *testing.T) { + parent := []int{1, 2} + got := prependSlice[int](parent, nil) + assert.Equal(t, []int{1, 2}, got) + }) + + t.Run("both populated: parent prepended to child", func(t *testing.T) { + parent := []int{1, 2} + child := []int{3, 4} + got := prependSlice[int](parent, child) + assert.Equal(t, []int{1, 2, 3, 4}, got) + }) + + t.Run("does not mutate parent or child", func(t *testing.T) { + parent := []int{1, 2} + child := []int{3, 4} + _ = prependSlice[int](parent, child) + assert.Equal(t, []int{1, 2}, parent, "parent must not be mutated") + assert.Equal(t, []int{3, 4}, child, "child must not be mutated") + }) + + t.Run("returned slice has a fresh backing array when both non-empty", func(t *testing.T) { + parent := []int{1, 2} + child := []int{3, 4} + got := prependSlice[int](parent, child) + // Mutating got must not affect parent or child. + got[0] = 99 + assert.Equal(t, []int{1, 2}, parent) + assert.Equal(t, []int{3, 4}, child) + }) +} +``` + +- [ ] **Step 1.2: Run the test, verify it fails** + +```bash +go test ./... -run TestPrependSlice -v +``` + +Expected: build failure (`prependSlice` undefined). + +- [ ] **Step 1.3: Create `workflow_option.go` with `prependSlice` plus the types** + +```go +package flow + +import "github.com/benbjohnson/clock" + +// WorkflowOption groups all configuration that a Workflow exposes to its +// caller AND inherits from a parent Workflow when used as a sub-workflow step. +// +// Scalar fields are pointers so that "unset" (nil) and "explicit zero value" +// are distinguishable. On parent → child Option inheritance, a nil pointer on +// the child means "inherit from parent (or use the runtime default)"; a +// non-nil pointer is the child's own choice and wins over the parent. +// +// Slice fields are not pointer-typed; on inheritance, the parent's slice is +// prepended to the child's slice (parent contributions run first), preserving +// the existing Mutator and Interceptor propagation semantics. +// +// Mutating a Workflow's Option after Do() has started is undefined behavior. +type WorkflowOption struct { + // MaxConcurrency caps simultaneously-running Steps. nil or 0 means + // unlimited; a positive value installs a buffered-channel lease bucket. + MaxConcurrency *int + + // DontPanic, if non-nil and true, recovers panics in Step Do / Input / + // BeforeStep / AfterStep callbacks and surfaces them as ErrPanic. + DontPanic *bool + + // SkipAsError, if non-nil and true, counts Skipped terminal status as a + // workflow failure (so Do returns ErrWorkflow even if no Step actually + // failed). + SkipAsError *bool + + // Clock is the time source used for Step timeouts, per-try timeouts in + // the retry loop, and backoff waits. nil means real wall clock + // (clock.New()). Inject a clock.Mock in tests to control time. + Clock clock.Clock + + // StepDefaults, if non-nil, is prepended as the FIRST option to every + // Step's Option list as a baseline. Per-step Option calls (Retry, + // Timeout, When, …) still win over it. + StepDefaults *StepOption + + // Mutators is the workflow-level list of cross-cutting step Mutators. + // On inheritance, the parent's Mutators are prepended (parent contributions + // run first within the child). + Mutators []Mutator + + // StepInterceptors wraps each Step's full lifetime (across retries). + // On inheritance, the parent's slice is prepended to the child's. + StepInterceptors []StepInterceptor + + // AttemptInterceptors wraps each individual attempt (Before → Do → After). + // On inheritance, the parent's slice is prepended to the child's. + AttemptInterceptors []AttemptInterceptor + + // DontInherit, when true on a sub-workflow Workflow, makes InheritOption + // a no-op: nothing flows in from the parent. Replaces the previous + // IsolateInterceptors flag and now governs the entire WorkflowOption, + // not just interceptors. Naming aligns with DontPanic. + DontInherit bool +} + +// WorkflowOptionReceiver is implemented by any Step that contains a +// sub-workflow. The parent's Do() prologue locates the nearest receiver in +// each root step's Unwrap chain and calls InheritOption ONCE before any +// scheduling begins, so the child's Do() observes the merged Option. +// +// *Workflow itself implements this interface; users get inheritance for +// free by embedding flow.Workflow in their own Step type. +type WorkflowOptionReceiver interface { + InheritOption(parent WorkflowOption) +} + +// prependSlice returns a fresh slice equal to parent ++ child. It MUST NOT +// mutate either input. The fresh backing array is what allows callers to +// snapshot-and-restore a WorkflowOption with a shallow copy: parent and +// child slice headers retain their original backing arrays. +func prependSlice[T any](parent, child []T) []T { + if len(parent) == 0 { + // Return a copy so callers can mutate without affecting child. + // But for the no-prepend case it's safe to alias; callers don't + // rely on freshness when parent contributes nothing. Choose copy + // for uniformity. + if len(child) == 0 { + return nil + } + out := make([]T, len(child)) + copy(out, child) + return out + } + if len(child) == 0 { + out := make([]T, len(parent)) + copy(out, parent) + return out + } + out := make([]T, 0, len(parent)+len(child)) + out = append(out, parent...) + out = append(out, child...) + return out +} + +// findOptionReceiver returns the first WorkflowOptionReceiver in the Step +// tree rooted at s, walking via Unwrap in pre-order. Returns nil if none of +// the unwrapped Steps satisfies WorkflowOptionReceiver. +// +// This lets a sub-workflow be wrapped in a Steper-only wrapper (e.g. +// NamedStep, which embeds the Steper interface and therefore does not +// promote InheritOption) without losing parent-Option inheritance. +func findOptionReceiver(s Steper) WorkflowOptionReceiver { + var found WorkflowOptionReceiver + Traverse(s, func(s Steper, _ []Steper) TraverseDecision { + if r, ok := s.(WorkflowOptionReceiver); ok { + found = r + return TraverseStop + } + return TraverseContinue + }) + return found +} +``` + +- [ ] **Step 1.4: Run the prependSlice tests, verify pass** + +```bash +go test ./... -run TestPrependSlice -v +``` + +Expected: PASS (5 sub-tests). + +- [ ] **Step 1.5: Verify the full suite still builds and passes** + +```bash +go build ./... && go test ./... +``` + +Expected: PASS. (We have added new types but not wired them anywhere.) + +- [ ] **Step 1.6: Commit** + +```bash +git add workflow_option.go workflow_option_test.go +git commit -m "feat: add WorkflowOption, WorkflowOptionReceiver, prependSlice (unwired)" +``` + +--- + +## Task 2: Wire WorkflowOption into Workflow struct (with accessor helpers) + +**Files:** +- Modify: `workflow.go` + +This is the big breaking-shape change. After this task: `Workflow` has `Option WorkflowOption` and the nine top-level fields are gone; all internal reads go through unexported accessors. The tree won't compile in the middle of this task — tests and examples are migrated in Tasks 3-7. + +- [ ] **Step 2.1: In `workflow.go`, replace the `Workflow` struct definition** + +Replace lines roughly 45-92 (the current `type Workflow struct { ... }`) with: + +```go +// Workflow orchestrates a collection of Steps connected by dependency edges +// into a Directed Acyclic Graph (DAG). +// +// You declare the graph with the helpers in step.go: Step / Steps / Pipe / +// BatchPipe (and branching helpers If / Switch from branch.go), then hand +// them to Workflow.Add: +// +// workflow.Add( +// Step(a), +// Steps(b, c).DependsOn(a), // a runs first, then b and c in parallel. +// Pipe(d, e, f), // d -> e -> f. +// BatchPipe( +// Steps(g, h), +// Steps(i, j), +// ), // g, h finish, then i, j run in parallel. +// ) +// +// Workflow.Do executes the graph in topological order. Each step that becomes +// runnable runs in its own goroutine, with the following guarantee: +// +// When a step's worker goroutine starts, every upstream step is already +// in a terminal status (Succeeded / Failed / Canceled / Skipped). The +// step's Condition then decides whether it actually runs (Running) or is +// settled inline as Skipped / Canceled. +// +// All workflow-level configuration lives in [WorkflowOption], exposed via +// Workflow.Option. See [WorkflowOption] for the available fields and the +// parent → child inheritance rules used when a Workflow is run as a step +// inside another Workflow. +// +// Per-step configuration: use Step / Steps / Pipe (see step.go). +// Composite steps: use Has / As / HasStep (see wrap.go). +type Workflow struct { + // Option groups all workflow-level configuration (concurrency cap, + // panic policy, skip-as-error, clock, default step options, mutators, + // interceptors, and inheritance opt-out). See [WorkflowOption]. + Option WorkflowOption + + StepBuilder // embeds the BuildStep memo so Workflow.Add can call BuildStep on new steps once. + + steps map[Steper]*State // root step → its State (status + StepConfig). + + statusChange *sync.Cond // signals to the tick loop when a worker terminates. + leaseBucket chan struct{} // bounded-channel "permit pool" enforcing Option.MaxConcurrency; nil means unlimited. + waitGroup sync.WaitGroup // tracks worker goroutines so Do() can wait for them on exit. + isRunning sync.Mutex // single-runner guard: TryLock fails fast if Do/Reset is re-entered. +} +``` + +Notes: +- The `inheritedStep` / `inheritedAttempt` fields are GONE — snapshot/restore in `Do()` (Task 4) replaces them. +- The long comment block about `inheritedStep` lifecycle is also GONE. + +- [ ] **Step 2.2: Add unexported scalar accessors near the top of `workflow.go`** + +Add directly after the `Workflow` struct (or just before `Add`): + +```go +// Scalar accessors: handle nil-pointer dereference and runtime defaults. +// All in-code reads of these scalars MUST go through these accessors. + +func (w *Workflow) maxConcurrency() int { + if w.Option.MaxConcurrency == nil { + return 0 + } + return *w.Option.MaxConcurrency +} + +func (w *Workflow) dontPanic() bool { + return w.Option.DontPanic != nil && *w.Option.DontPanic +} + +func (w *Workflow) skipAsError() bool { + return w.Option.SkipAsError != nil && *w.Option.SkipAsError +} + +func (w *Workflow) clock() clock.Clock { + if w.Option.Clock == nil { + return clock.New() + } + return w.Option.Clock +} +``` + +- [ ] **Step 2.3: In `Workflow.Add` (around line 99-118), rewrite the `DefaultOption` read** + +Find: +```go + if w.DefaultOption != nil && config != nil { + config.Option = slices.Insert(config.Option, 0, func(o *StepOption) { + *o = *w.DefaultOption + }) + } +``` + +Replace with: +```go + if w.Option.StepDefaults != nil && config != nil { + config.Option = slices.Insert(config.Option, 0, func(o *StepOption) { + *o = *w.Option.StepDefaults + }) + } +``` + +Also update the comment above `Add` (around line 99-100): +``` +// If Option.StepDefaults is set, it is prepended to every step's Option +// list as a SEED — so per-step Option calls (Retry, Timeout, When, …) still +// win. +``` + +- [ ] **Step 2.4: Remove `Workflow.PrependMutators` method** + +Delete lines roughly 120-129 (the entire `// PrependMutators inserts mw at the front …` doc + the function body). + +- [ ] **Step 2.5: In `applyMutators`, rewrite reads of `w.Mutators` and `w.DontPanic`** + +Find (around line 184-193): +```go + if len(w.Mutators) == 0 { + return + } + + if w.DontPanic { +``` +… and the `for _, m := range w.Mutators {`. + +Replace with: +```go + if len(w.Option.Mutators) == 0 { + return + } + + if w.dontPanic() { +``` +… and `for _, m := range w.Option.Mutators {`. + +- [ ] **Step 2.6: Rewrite `Workflow.reset()` (around line 363-384)** + +Find: +```go +func (w *Workflow) reset() { + for _, state := range w.steps { + state.SetStepResult(StepResult{Status: Pending}) + } + if w.Clock == nil { + w.Clock = clock.New() + } + w.statusChange = sync.NewCond(&sync.Mutex{}) + if w.MaxConcurrency > 0 { + w.leaseBucket = make(chan struct{}, w.MaxConcurrency) + } else { + w.leaseBucket = nil + } +} +``` + +Replace with: +```go +// reset is the per-Do internal reset: clear all step results back to Pending, +// install a fresh statusChange Cond, and re-allocate the concurrency lease +// bucket sized for Option.MaxConcurrency. +// +// reset does NOT touch w.Option: parent → child Option inheritance is +// preserved by the snapshot/restore in Do() (see Workflow.Do). +func (w *Workflow) reset() { + for _, state := range w.steps { + state.SetStepResult(StepResult{Status: Pending}) + } + w.statusChange = sync.NewCond(&sync.Mutex{}) + if mc := w.maxConcurrency(); mc > 0 { + w.leaseBucket = make(chan struct{}, mc) + } else { + w.leaseBucket = nil + } +} +``` + +Note: we no longer mutate `w.Option.Clock`; nil-handling is done by `w.clock()`. The line `if w.Clock == nil { w.Clock = clock.New() }` was destructive (it wrote to the user-supplied struct); the new accessor pattern is non-destructive. + +- [ ] **Step 2.7: Rewrite `Workflow.Reset()` (around line 352-361)** + +Find: +```go +// Reset prepares the Workflow for a fresh run from outside (the user's POV). +// It rejects with ErrWorkflowIsRunning if a Do call is currently in flight. +// +// Difference vs the internal reset(): Reset() ALSO clears the inherited +// interceptor slices set by a parent during a previous run. The internal +// reset() must NOT clear them — see the inheritedStep / inheritedAttempt +// lifecycle docs above. +func (w *Workflow) Reset() error { + if !w.isRunning.TryLock() { + return ErrWorkflowIsRunning + } + defer w.isRunning.Unlock() + w.reset() + w.inheritedStep = nil + w.inheritedAttempt = nil + return nil +} +``` + +Replace with: +```go +// Reset rewinds every Step's status back to Pending so the Workflow can be +// Do()-ed again. Reset rejects with ErrWorkflowIsRunning if a Do call is +// currently in flight. +// +// Reset does NOT modify w.steps (the set of Steps registered via Add) — a +// Workflow built once can be Do()-ed any number of times via Reset/Do +// cycles, with the same DAG each time. To start from an empty set of Steps, +// allocate a new Workflow. +// +// Reset does NOT modify w.Option either. Cross-run accumulation of +// parent-inherited contributions is prevented by the snapshot/restore in +// Do(), not by Reset. Calling Reset between runs is therefore optional from +// an Option-isolation standpoint; its purpose is purely to rewind per-step +// status for re-execution. +func (w *Workflow) Reset() error { + if !w.isRunning.TryLock() { + return ErrWorkflowIsRunning + } + defer w.isRunning.Unlock() + w.reset() + return nil +} +``` + +- [ ] **Step 2.8: Replace `Workflow.PrependInterceptors` with `InheritOption`** + +Find lines roughly 386-411 (the entire `// PrependInterceptors implements InterceptorReceiver …` doc + function). + +Replace with: +```go +// InheritOption implements [WorkflowOptionReceiver]. A parent Workflow calls +// this method on each sub-workflow root step's first receiver (located via +// findOptionReceiver) ONCE in the parent's Do() prologue. +// +// Merge rules: +// - if w.Option.DontInherit is true, this is a no-op; +// - for each scalar pointer (MaxConcurrency, DontPanic, SkipAsError) and +// interface/pointer (Clock, StepDefaults) field: if the child's field is +// nil, the parent's value is copied in; non-nil child fields are +// preserved; +// - for each slice (Mutators, StepInterceptors, AttemptInterceptors): a +// fresh slice equal to parent ++ child replaces the child's field. +// +// w.Option mutations made here are reverted at the end of Do() via a +// snapshot/restore, so multiple Do() invocations do not accumulate parent +// contributions. +func (w *Workflow) InheritOption(parent WorkflowOption) { + if w.Option.DontInherit { + return + } + if w.Option.MaxConcurrency == nil { + w.Option.MaxConcurrency = parent.MaxConcurrency + } + if w.Option.DontPanic == nil { + w.Option.DontPanic = parent.DontPanic + } + if w.Option.SkipAsError == nil { + w.Option.SkipAsError = parent.SkipAsError + } + if w.Option.Clock == nil { + w.Option.Clock = parent.Clock + } + if w.Option.StepDefaults == nil { + w.Option.StepDefaults = parent.StepDefaults + } + w.Option.Mutators = prependSlice(parent.Mutators, w.Option.Mutators) + w.Option.StepInterceptors = prependSlice(parent.StepInterceptors, w.Option.StepInterceptors) + w.Option.AttemptInterceptors = prependSlice(parent.AttemptInterceptors, w.Option.AttemptInterceptors) +} +``` + +- [ ] **Step 2.9: Rewrite `effectiveStepInterceptors` / `effectiveAttemptInterceptors` (lines ~413-437)** + +Replace BOTH with: +```go +// effectiveStepInterceptors returns the chain to invoke for THIS run. With +// parent → child Option inheritance now performed eagerly in Do()'s prologue +// (writing into w.Option.StepInterceptors directly), the effective chain IS +// simply w.Option.StepInterceptors. +func (w *Workflow) effectiveStepInterceptors() []StepInterceptor { + return w.Option.StepInterceptors +} + +// effectiveAttemptInterceptors mirrors effectiveStepInterceptors for AttemptInterceptors. +func (w *Workflow) effectiveAttemptInterceptors() []AttemptInterceptor { + return w.Option.AttemptInterceptors +} +``` + +- [ ] **Step 2.10: Rewrite the scalar reads inside the scheduling/execution code** + +Find these reads and update each one: + +| Line (approx) | Old | New | +|---|---|---| +| 498 | `if w.SkipAsError && err.AllSucceeded() {` | `if w.skipAsError() && err.AllSucceeded() {` | +| 501 | `if !w.SkipAsError && err.AllSucceededOrSkipped() {` | `if !w.skipAsError() && err.AllSucceededOrSkipped() {` | +| 652 | `FinishedAt: w.Clock.Now(),` | `FinishedAt: w.clock().Now(),` | +| 668 | `FinishedAt: w.Clock.Now(),` | `FinishedAt: w.clock().Now(),` | +| 724 | `if ex.w.DontPanic {` | `if ex.w.dontPanic() {` | +| 751 | `FinishedAt: ex.w.Clock.Now(),` | `FinishedAt: ex.w.clock().Now(),` | +| 776 | `notAfter = ex.w.Clock.Now().Add(*option.Timeout)` | `notAfter = ex.w.clock().Now().Add(*option.Timeout)` | +| 778 | `ctx, cancel = ex.w.Clock.WithDeadline(ctx, notAfter)` | `ctx, cancel = ex.w.clock().WithDeadline(ctx, notAfter)` | +| 800 | `if ex.w.DontPanic {` | `if ex.w.dontPanic() {` | +| 824 | `if ex.w.DontPanic {` | `if ex.w.dontPanic() {` | + +Note: `clock()` returns a fresh `clock.New()` on every call when nil. To avoid re-allocating, capture once per use site if performance matters. For correctness this is fine since `clock.New()` returns a value-typed wrapper. + +Also update the godoc comments that mention `DontPanic` / `MaxConcurrency` / `Clock` field names — these should now reference `Option.DontPanic` / `Option.MaxConcurrency` / `Option.Clock`. Specifically: +- Comment on line ~365 ("MaxConcurrency") → `Option.MaxConcurrency` +- Comment on line ~634 ("recover when DontPanic is set") → `recover when Option.DontPanic is set` +- Comment on line ~711 ("When DontPanic is true") → `When Option.DontPanic is true` +- Comment on line ~817 ("when DontPanic is true") → `when Option.DontPanic is true` +- Comment on line ~844 ("MaxConcurrency is unset") → `Option.MaxConcurrency is unset` +- Comment on line ~859 ("MaxConcurrency is unset") → `Option.MaxConcurrency is unset` + +- [ ] **Step 2.11: Save and verify it does not compile yet (expected)** + +```bash +go build ./... +``` + +Expected: build failure. There are still references to the old fields from: +- The Mutator dispatch site at `:641` (handled in Task 3). +- The Interceptor dispatch site at `:768` (handled in Task 3). +- `SubWorkflow.PrependMutators` / `PrependInterceptors` (handled in Task 5). +- Test files (handled in Tasks 6-7). + +DO NOT commit yet — continue to Task 3. + +--- + +## Task 3: Rewrite propagation in Do() prologue (replace two dispatch sites) + +**Files:** +- Modify: `workflow.go` + +After this task, the Mutator-at-`:641` and Interceptor-at-`:768` dispatch sites are gone, replaced by a single prologue pass that calls `InheritOption` once per receiver. The build still fails because `interceptor.go` / `mutator.go` / `SubWorkflow` / tests still reference old symbols — fixed in Tasks 4-7. + +- [ ] **Step 3.1: Locate and remove the Mutator dispatch site** + +In the scheduling code around line 638-643, find: + +```go + // Apply Mutators exactly once per step, before reading Option / + // Before / After. ... + if !state.MutatorsApplied { + if recv, ok := step.(MutatorReceiver); ok && len(w.Mutators) > 0 { + recv.PrependMutators(w.Mutators) + } + w.applyMutators(ctx, step) + ... +``` + +Remove only the `if recv, ok := step.(MutatorReceiver); ok ... { recv.PrependMutators(w.Mutators) }` block. KEEP `w.applyMutators(ctx, step)` and the `state.MutatorsApplied = true` mechanics — those are still needed for the local-mutator dispatch. The "propagate to child" job is now done by the prologue pass via `InheritOption`. + +Result (approximate): + +```go + if !state.MutatorsApplied { + w.applyMutators(ctx, step) + ... +``` + +- [ ] **Step 3.2: Locate and remove the Interceptor dispatch site** + +Around line 767-770, find: + +```go + if recv := findInterceptorReceiver(ex.step); recv != nil { + recv.PrependInterceptors(ex.w.effectiveStepInterceptors(), ex.w.effectiveAttemptInterceptors()) + } +``` + +Delete those 3 lines. The job is now done by the prologue pass. + +- [ ] **Step 3.3: Rewrite `Workflow.Do()` prologue with snapshot + propagation pass** + +Find the existing `Workflow.Do()` (around line 450-505) and replace with: + +```go +func (w *Workflow) Do(ctx context.Context) error { + // Single-runner guard. + if !w.isRunning.TryLock() { + return ErrWorkflowIsRunning + } + defer w.isRunning.Unlock() + + // Snapshot Option so any InheritOption writes performed below (and + // transitively by nested workflows during their own Do() prologue) are + // reverted at the end of THIS Do() call. The snapshot is a shallow copy; + // this is correct because: + // - InheritOption only overwrites nil pointer fields with the parent's + // pointer values (not mutating the parent's targets), and + // - prependSlice always allocates a fresh slice, so neither the + // snapshot's slice header nor the parent's slice is mutated. + optSnapshot := w.Option + defer func() { w.Option = optSnapshot }() + + // Nothing to do. + if w.Empty() { + return nil + } + + w.reset() + + // Reject cycles before launching any work. + if err := w.preflight(); err != nil { + return err + } + + // Propagate w.Option into every sub-workflow root step exactly once, + // BEFORE the tick loop dispatches anything. Receivers are located via + // pre-order Unwrap walk so a sub-workflow may be wrapped in a Steper-only + // wrapper (e.g. NamedStep) without losing inheritance. + for step := range w.steps { + if recv := findOptionReceiver(step); recv != nil { + recv.InheritOption(w.Option) + } + } + + // Tick loop: each time a step terminates it Signal()s the cond, we wake + // up and tick() again. Inline-settled steps may unblock more steps within + // the same tick (no signal needed for those — see tick()). + w.statusChange.L.Lock() + for { + if done := w.tick(ctx); done { + break + } + w.statusChange.Wait() + } + w.statusChange.L.Unlock() + + // Drain worker goroutines so we don't return while children are still alive. + w.waitGroup.Wait() + + // Build the per-step error map and decide the overall outcome. + err := make(ErrWorkflow) + for step, state := range w.steps { + err[step] = state.GetStepResult() + } + if w.skipAsError() && err.AllSucceeded() { + return nil + } + if !w.skipAsError() && err.AllSucceededOrSkipped() { + return nil + } + return err +} +``` + +Note the changes vs. before: +- Removed the `defer func() { w.inheritedStep = nil; w.inheritedAttempt = nil }()` block — those fields no longer exist. +- Added `optSnapshot` / `defer w.Option = optSnapshot`. +- Added the propagation `for step := range w.steps { ... }` loop after `preflight()`. +- `w.SkipAsError` → `w.skipAsError()` (already handled in Task 2.10, but reaffirmed here). + +- [ ] **Step 3.4: Verify it still won't compile (expected) — continue to Task 4** + +```bash +go build ./... +``` + +Expected: build failure (`InterceptorReceiver`, `findInterceptorReceiver`, `MutatorReceiver` still referenced in `interceptor.go` / `mutator.go` / `workflow.go` SubWorkflow methods). + +--- + +## Task 4: Remove old receiver interfaces and helper + +**Files:** +- Modify: `interceptor.go`, `mutator.go` + +- [ ] **Step 4.1: In `interceptor.go`, delete `InterceptorReceiver` and `findInterceptorReceiver`** + +Find lines 42-78 (approximate — covers the type declaration, godoc, and the `findInterceptorReceiver` function). Delete the entire block. + +Also remove any imports that become unused (the `Traverse` import is still used elsewhere if any; usually nothing to clean). + +- [ ] **Step 4.2: In `mutator.go`, delete `MutatorReceiver`** + +Find lines 12-21 (the `// MutatorReceiver is implemented by …` doc plus the `type MutatorReceiver interface { ... }` block). Delete. + +Also update the godoc reference inside the `applyTo` body comment (around line 49) — replace `Inner steps are reached via PrependMutators.` with `Inner steps are reached via the WorkflowOptionReceiver.InheritOption mechanism (see workflow_option.go).`. + +- [ ] **Step 4.3: Verify build still fails because of SubWorkflow (expected)** + +```bash +go build ./... +``` + +Expected: build errors from `SubWorkflow.PrependMutators` / `SubWorkflow.PrependInterceptors` (still in `workflow.go`) referencing the deleted interfaces / methods. + +--- + +## Task 5: Deprecate SubWorkflow (and re-tag StepBuilder) + +**Files:** +- Modify: `workflow.go`, `build_step.go` + +- [ ] **Step 5.1: Re-tag `StepBuilder` as Deprecated in its godoc** + +In `build_step.go`, the existing godoc only deprecates the `BuildStep` *method*. With this change we publicly mark the `StepBuilder` *type* itself as deprecated so users who embed it directly (none known, but the type is exported) are warned. The behavior is unchanged — `Workflow` still embeds `StepBuilder`, the `BuildStep()` user hook still fires once per step via `Workflow.Add` → `w.BuildStep(step)`. + +Replace the existing type-level doc (above `type StepBuilder struct{ built Set[Steper] }`) with: + +```go +// StepBuilder is the per-Workflow memo that ensures every Step's optional +// BuildStep() hook fires at most once. +// +// A Step type can implement BuildStep() to assemble its internal sub-steps +// lazily — typically the first time it is added to a Workflow: +// +// type StepImpl struct{} +// func (s *StepImpl) Unwrap() []flow.Steper { return /* internal steps */ } +// func (s *StepImpl) Do(ctx context.Context) error { /* ... */ } +// func (s *StepImpl) BuildStep() { /* assemble children */ } +// +// workflow.Add( +// flow.Step(new(StepImpl)), // BuildStep() fires here, exactly once. +// ) +// +// The StepBuilder is embedded in Workflow itself, so Workflow.Add transparently +// invokes BuildStep on every newly seen step. +// +// Deprecated: StepBuilder and the BuildStep() user hook will be removed in +// the next major version of go-workflow, together with [SubWorkflow] and +// [SubWorkflow.Reset]. Use [Mutate] for cross-cutting modification, and +// construct sub-workflows inside Do() (or at the embedding type's +// construction time) instead. +type StepBuilder struct{ built Set[Steper] } +``` + +The existing `// Deprecated:` paragraph on the `BuildStep(s Steper)` *method* stays as-is; this step just adds the same notice at the *type* level so embedders see it. + +- [ ] **Step 5.2: Replace the entire `SubWorkflow` block in `workflow.go`** + +Find lines ~891-931 (the existing `// SubWorkflow makes any user struct …` doc plus the struct, methods, and `PrependMutators` / `PrependInterceptors` helpers). + +Replace with: + +```go +// SubWorkflow makes any user struct behave as a Step that contains a +// Workflow. Embed it in your own struct to get Add/Do/Unwrap and Option +// inheritance for free. +// +// Deprecated: Embed flow.Workflow directly instead. With workflow-level +// configuration consolidated under [Workflow.Option] (one named field), +// embedding flow.Workflow promotes only that single Option name onto your +// type — the same surface area SubWorkflow previously hid. SubWorkflow will +// be removed in the next major version of go-workflow. +// +// // Recommended pattern: +// type Deploy struct { +// flow.Workflow +// Region string +// } +type SubWorkflow struct{ w Workflow } + +func (s *SubWorkflow) Unwrap() Steper { return &s.w } +func (s *SubWorkflow) Add(builders ...Builder) *Workflow { return s.w.Add(builders...) } +func (s *SubWorkflow) Do(ctx context.Context) error { return s.w.Do(ctx) } + +// Reset clears the inner workflow so a subsequent BuildStep() can rebuild +// from scratch. +// +// Deprecated: Reset is only invoked by the deprecated [StepBuilder.BuildStep] +// path. With the [Mutator] mechanism (see [Mutate]) and Do()-time sub-workflow +// construction, Reset is no longer needed and will be removed together with +// SubWorkflow in the next major version of go-workflow. +func (s *SubWorkflow) Reset() { s.w = Workflow{} } + +// InheritOption forwards to the inner Workflow so SubWorkflow continues to +// participate in parent → child Option propagation during the deprecation +// window. Implements [WorkflowOptionReceiver]. +func (s *SubWorkflow) InheritOption(parent WorkflowOption) { + s.w.InheritOption(parent) +} +``` + +Note: `PrependMutators` and `PrependInterceptors` methods are gone. The interfaces they satisfied no longer exist. + +- [ ] **Step 5.3: Try to build (expected: source compiles; tests/examples may not)** + +```bash +go build ./... +``` + +Expected: PASS. The non-test source tree now compiles cleanly under the new shape. Tests and examples are migrated in Tasks 6-8. + +--- + +## Task 6: Write the InheritOption test matrix + +**Files:** +- Modify: `workflow_option_test.go` (append to file created in Task 1) + +We add new tests against the NOW-working `Workflow.InheritOption` and `findOptionReceiver`. These exercise behavior that did not exist before. + +- [ ] **Step 6.1: Add the table-driven InheritOption matrix** + +Append to `workflow_option_test.go`: + +```go +import ( + // already imported: "testing", "github.com/stretchr/testify/assert" + "context" + "github.com/benbjohnson/clock" + "github.com/stretchr/testify/require" +) + +// Helpers for the matrix tests. +func ptr[T any](v T) *T { return &v } + +// noopMutator is a Mutator that matches nothing; used as a slice element to +// verify slice order without exercising dispatch. +type noopMutator struct{ tag string } + +func (noopMutator) applyTo(context.Context, Steper) (bool, Steper, Builder) { + return false, nil, nil +} + +// stepInterceptorTag returns a StepInterceptor that appends a tag to a shared +// slice so we can observe ordering. Used only in tests that exercise +// dispatch; the matrix tests below just compare slice identity / length. +func stepInterceptorTag(tag string, out *[]string) StepInterceptor { + return func(ctx context.Context, step Steper, next func(context.Context) error) error { + *out = append(*out, tag) + return next(ctx) + } +} + +func TestWorkflow_InheritOption(t *testing.T) { + mockClock := clock.NewMock() + otherClock := clock.NewMock() + + t.Run("scalar nil inherits parent's value", func(t *testing.T) { + parent := WorkflowOption{ + MaxConcurrency: ptr(4), + DontPanic: ptr(true), + SkipAsError: ptr(true), + Clock: mockClock, + StepDefaults: &StepOption{}, + } + child := &Workflow{} + child.InheritOption(parent) + assert.Equal(t, 4, *child.Option.MaxConcurrency) + assert.Equal(t, true, *child.Option.DontPanic) + assert.Equal(t, true, *child.Option.SkipAsError) + assert.Equal(t, mockClock, child.Option.Clock) + assert.Equal(t, parent.StepDefaults, child.Option.StepDefaults) + }) + + t.Run("scalar non-nil: child wins", func(t *testing.T) { + parent := WorkflowOption{MaxConcurrency: ptr(4)} + child := &Workflow{Option: WorkflowOption{MaxConcurrency: ptr(8)}} + child.InheritOption(parent) + assert.Equal(t, 8, *child.Option.MaxConcurrency) + }) + + t.Run("explicit zero (*int=0) wins over parent's non-zero", func(t *testing.T) { + // Child wants unlimited concurrency under a parent that limits. + parent := WorkflowOption{MaxConcurrency: ptr(4)} + zero := 0 + child := &Workflow{Option: WorkflowOption{MaxConcurrency: &zero}} + child.InheritOption(parent) + assert.Equal(t, 0, *child.Option.MaxConcurrency, + "explicit *int=0 child must win over parent's *int=4") + }) + + t.Run("clock nil inherits", func(t *testing.T) { + parent := WorkflowOption{Clock: mockClock} + child := &Workflow{} + child.InheritOption(parent) + assert.Equal(t, mockClock, child.Option.Clock) + }) + + t.Run("clock non-nil: child wins", func(t *testing.T) { + parent := WorkflowOption{Clock: mockClock} + child := &Workflow{Option: WorkflowOption{Clock: otherClock}} + child.InheritOption(parent) + assert.Equal(t, otherClock, child.Option.Clock) + }) + + t.Run("Mutators: parent prepended", func(t *testing.T) { + mP := noopMutator{tag: "parent"} + mC := noopMutator{tag: "child"} + parent := WorkflowOption{Mutators: []Mutator{mP}} + child := &Workflow{Option: WorkflowOption{Mutators: []Mutator{mC}}} + child.InheritOption(parent) + require.Len(t, child.Option.Mutators, 2) + assert.Equal(t, "parent", child.Option.Mutators[0].(noopMutator).tag) + assert.Equal(t, "child", child.Option.Mutators[1].(noopMutator).tag) + }) + + t.Run("StepInterceptors: parent prepended", func(t *testing.T) { + var log []string + parentS := stepInterceptorTag("P", &log) + childS := stepInterceptorTag("C", &log) + parent := WorkflowOption{StepInterceptors: []StepInterceptor{parentS}} + child := &Workflow{Option: WorkflowOption{StepInterceptors: []StepInterceptor{childS}}} + child.InheritOption(parent) + require.Len(t, child.Option.StepInterceptors, 2) + }) + + t.Run("AttemptInterceptors: parent prepended", func(t *testing.T) { + dummyP := AttemptInterceptor(func(ctx context.Context, step Steper, attempt int, next func(context.Context) error) error { + return next(ctx) + }) + dummyC := AttemptInterceptor(func(ctx context.Context, step Steper, attempt int, next func(context.Context) error) error { + return next(ctx) + }) + parent := WorkflowOption{AttemptInterceptors: []AttemptInterceptor{dummyP}} + child := &Workflow{Option: WorkflowOption{AttemptInterceptors: []AttemptInterceptor{dummyC}}} + child.InheritOption(parent) + require.Len(t, child.Option.AttemptInterceptors, 2) + }) + + t.Run("DontInherit: complete no-op", func(t *testing.T) { + mP := noopMutator{tag: "parent"} + parent := WorkflowOption{ + MaxConcurrency: ptr(4), + DontPanic: ptr(true), + Mutators: []Mutator{mP}, + } + child := &Workflow{Option: WorkflowOption{ + DontInherit: true, + Mutators: []Mutator{noopMutator{tag: "child"}}, + }} + child.InheritOption(parent) + assert.Nil(t, child.Option.MaxConcurrency, "DontInherit must leave scalars untouched") + assert.Nil(t, child.Option.DontPanic) + require.Len(t, child.Option.Mutators, 1, "DontInherit must NOT prepend parent slices") + assert.Equal(t, "child", child.Option.Mutators[0].(noopMutator).tag) + }) + + t.Run("parent slice is not mutated by InheritOption", func(t *testing.T) { + mP := noopMutator{tag: "parent"} + parentMutators := []Mutator{mP} + parent := WorkflowOption{Mutators: parentMutators} + child := &Workflow{Option: WorkflowOption{Mutators: []Mutator{noopMutator{tag: "child"}}}} + child.InheritOption(parent) + require.Len(t, parentMutators, 1, "parent's slice must not be appended into") + assert.Equal(t, "parent", parentMutators[0].(noopMutator).tag) + }) +} +``` + +- [ ] **Step 6.2: Run the matrix tests** + +```bash +go test ./... -run TestWorkflow_InheritOption -v +``` + +Expected: PASS. If a test fails, the merge rules in Task 2.8 are wrong — fix them before proceeding. + +- [ ] **Step 6.3: Commit Tasks 2-6 as one squashed change** + +The tree compiles for source and the new InheritOption tests pass; tests for the legacy API surface still need migration (next tasks). Squash because the intermediate states do not compile. + +```bash +git add workflow.go workflow_option.go workflow_option_test.go interceptor.go mutator.go +git commit -m "feat: consolidate Workflow config into Option; add WorkflowOptionReceiver + +- Replace nine top-level Workflow fields with one Option WorkflowOption (pointer scalars + slices + DontInherit). +- Add WorkflowOptionReceiver / Workflow.InheritOption; merge rules: nil scalar + inherits parent, slices prepended, DontInherit is a no-op. +- Rewrite Do() prologue: snapshot/restore Option, propagate via findOptionReceiver + once per child sub-workflow root step, replacing the two ad-hoc dispatch sites. +- Remove inheritedStep/inheritedAttempt side fields and the special-cased + reset() exclusion logic. +- Remove MutatorReceiver, InterceptorReceiver, findInterceptorReceiver, + Workflow.PrependMutators, Workflow.PrependInterceptors. +- Rename DefaultOption → Option.StepDefaults, IsolateInterceptors → Option.DontInherit. +- Deprecate SubWorkflow; add SubWorkflow.InheritOption delegating to inner Workflow. +- Drop now-destructive Clock-defaulting in reset(); accessor handles nil. +- Add WorkflowOption.InheritOption test matrix. + +Tests/examples migrated in follow-up commits." +``` + +(Note: tests still won't all pass — migrate in subsequent tasks.) + +--- + +## Task 7: Migrate test files + +**Files:** +- Modify: `workflow_options_test.go`, `workflow_mutator_test.go`, `workflow_test.go`, `branch_test.go`, `wrap_test.go`, `execution_model_test.go`, `step_configuration_test.go`, `retry_test.go`, `condition_test.go`, `noop_test.go`, `mutator_test.go`, `error_test.go`, `name_test.go`, `testutil_test.go` (whichever reference the removed symbols) + +The mechanical sed-friendly substitutions: + +| Old | New | +|---|---| +| `Workflow{MaxConcurrency: N, …}` | `Workflow{Option: WorkflowOption{MaxConcurrency: &mc, …}}` (declare `mc := N` above) | +| `Workflow{DontPanic: true, …}` | `Workflow{Option: WorkflowOption{DontPanic: &dp, …}}` (declare `dp := true`) | +| `Workflow{SkipAsError: true, …}` | `Workflow{Option: WorkflowOption{SkipAsError: &se, …}}` | +| `Workflow{Clock: c, …}` | `Workflow{Option: WorkflowOption{Clock: c, …}}` | +| `Workflow{DefaultOption: &so, …}` | `Workflow{Option: WorkflowOption{StepDefaults: &so, …}}` | +| `Workflow{Mutators: m, …}` | `Workflow{Option: WorkflowOption{Mutators: m, …}}` | +| `Workflow{StepInterceptors: si, …}` | `Workflow{Option: WorkflowOption{StepInterceptors: si, …}}` | +| `Workflow{AttemptInterceptors: ai, …}` | `Workflow{Option: WorkflowOption{AttemptInterceptors: ai, …}}` | +| `Workflow{IsolateInterceptors: true, …}` | `Workflow{Option: WorkflowOption{DontInherit: true, …}}` | +| `w.MaxConcurrency = N` | `w.Option.MaxConcurrency = ptr(N)` (or any equivalent) | +| `w.DontPanic = true` | `w.Option.DontPanic = ptr(true)` | +| `w.SkipAsError = true` | `w.Option.SkipAsError = ptr(true)` | +| `w.Clock = c` | `w.Option.Clock = c` | +| `w.DefaultOption = &so` | `w.Option.StepDefaults = &so` | +| `w.Mutators = m` | `w.Option.Mutators = m` | +| `w.StepInterceptors = si` | `w.Option.StepInterceptors = si` | +| `w.AttemptInterceptors = ai` | `w.Option.AttemptInterceptors = ai` | +| `w.IsolateInterceptors = true` | `w.Option.DontInherit = true` | +| `w.PrependMutators(m)` | `w.InheritOption(WorkflowOption{Mutators: m})` | +| `w.PrependInterceptors(si, ai)` | `w.InheritOption(WorkflowOption{StepInterceptors: si, AttemptInterceptors: ai})` | +| `MutatorReceiver` reference | `WorkflowOptionReceiver` | +| `InterceptorReceiver` reference | `WorkflowOptionReceiver` | +| `findInterceptorReceiver` call | `findOptionReceiver` (look for the same `Unwrap`-walking semantics) | + +- [ ] **Step 7.1: Inventory remaining test files referencing the old symbols** + +Run: + +```bash +grep -rn 'MaxConcurrency\|DontPanic\|SkipAsError\|DefaultOption\|IsolateInterceptors\|PrependMutators\|PrependInterceptors\|MutatorReceiver\|InterceptorReceiver\|findInterceptorReceiver\|w\.Mutators\b\|w\.StepInterceptors\b\|w\.AttemptInterceptors\b\|w\.Clock\b' --include='*_test.go' +``` + +This produces the working list. Migrate each file in turn. Group commits by file or by sensible chunks of 3-5 files. + +- [ ] **Step 7.2: For each test file, apply the substitution table** + +Example concrete migration — `workflow_options_test.go`. Find a pattern like: + +```go +w := &Workflow{MaxConcurrency: 2} +``` + +Replace with: + +```go +mc := 2 +w := &Workflow{Option: WorkflowOption{MaxConcurrency: &mc}} +``` + +If the test is asserting `w.MaxConcurrency == 2`, change the assertion to: + +```go +require.NotNil(t, w.Option.MaxConcurrency) +assert.Equal(t, 2, *w.Option.MaxConcurrency) +``` + +For tests of `IsolateInterceptors`, rename to `DontInherit`. The behavior matrix is widened (now opts out of EVERYTHING, not just interceptors) — but every existing test only ever observes the interceptor behavior, so the substitution is semantics-preserving for those tests. + +- [ ] **Step 7.3: Migrate `PrependMutators` / `PrependInterceptors` test exercises** + +The tests that previously called `w.PrependMutators(...)` were asserting that the parent's Mutators slice was prepended into `w.Mutators`. Equivalent assertion under the new API: + +Before: +```go +w := &Workflow{Mutators: []Mutator{childM}} +w.PrependMutators([]Mutator{parentM}) +require.Len(t, w.Mutators, 2) +assert.Equal(t, parentM, w.Mutators[0]) +``` + +After: +```go +w := &Workflow{Option: WorkflowOption{Mutators: []Mutator{childM}}} +w.InheritOption(WorkflowOption{Mutators: []Mutator{parentM}}) +require.Len(t, w.Option.Mutators, 2) +assert.Equal(t, parentM, w.Option.Mutators[0]) +``` + +- [ ] **Step 7.4: Migrate `SubWorkflow`-using tests (DO NOT remove SubWorkflow)** + +`SubWorkflow` is deprecated but still works. Test files that use it should: +- Continue to work as-is for the SubWorkflow-as-deprecated-path smoke tests. +- For any test that exercises the OLD `SubWorkflow.PrependMutators` / `SubWorkflow.PrependInterceptors` methods, replace with `subWorkflow.InheritOption(...)`. + +- [ ] **Step 7.5: Add a SubWorkflow smoke test** + +In `workflow_test.go` (or a new `subworkflow_deprecated_test.go`): + +```go +//nolint:staticcheck // deprecation smoke test +func TestSubWorkflow_InheritOption_DeprecationSmoke(t *testing.T) { + // Verify that during the deprecation window, a step embedding SubWorkflow + // still receives the parent's Option via the delegated InheritOption. + type composite struct { + SubWorkflow + } + + parent := &Workflow{Option: WorkflowOption{DontPanic: ptr(true)}} + c := &composite{} + // Inner step inside SubWorkflow. + innerCalled := false + innerStep := Func("inner", func(ctx context.Context) error { + innerCalled = true + return nil + }) + c.Add(Step(innerStep)) + parent.Add(Step(c)) + + require.NoError(t, parent.Do(context.Background())) + assert.True(t, innerCalled) + // During parent.Do(), the inner Workflow inside c.SubWorkflow should have + // observed DontPanic=true via the delegated InheritOption. + // We assert indirectly via a Func that panics; this assertion is in a + // separate test below. +} + +func TestSubWorkflow_InheritOption_PanicRecovered(t *testing.T) { + type composite struct { + SubWorkflow + } + + parent := &Workflow{Option: WorkflowOption{DontPanic: ptr(true)}} + c := &composite{} + // Inner step that panics. + c.Add(Step(Func("panicker", func(ctx context.Context) error { + panic("boom") + }))) + parent.Add(Step(c)) + + // With DontPanic propagated, parent.Do must return an error (not panic). + err := parent.Do(context.Background()) + require.Error(t, err) +} +``` + +- [ ] **Step 7.6: Add a scalar inheritance behavior test (motivating use case)** + +Append to `workflow_option_test.go`: + +```go +func TestWorkflow_ScalarInheritance_DontPanicFlowsToChild(t *testing.T) { + // Motivating use case: parent sets DontPanic=true, child workflow has no + // DontPanic set; the child's panicking step should be caught. + child := &Workflow{} + child.Add(Step(Func("panicker", func(ctx context.Context) error { + panic("boom") + }))) + + parent := &Workflow{Option: WorkflowOption{DontPanic: ptr(true)}} + parent.Add(Step(child)) + + err := parent.Do(context.Background()) + require.Error(t, err, "parent.DontPanic=true should propagate so child's panic is recovered") +} + +func TestWorkflow_DoSnapshotRestore_NoAccumulationAcrossRuns(t *testing.T) { + // After parent.Do() returns, the child's Option.Mutators must be back to + // its pre-inheritance value; running parent.Do() N times should NOT cause + // Mutators to accumulate in the child. + child := &Workflow{Option: WorkflowOption{Mutators: []Mutator{noopMutator{tag: "C"}}}} + child.Add(Step(NoOp("noop"))) + + parent := &Workflow{Option: WorkflowOption{Mutators: []Mutator{noopMutator{tag: "P"}}}} + parent.Add(Step(child)) + + for i := 0; i < 3; i++ { + require.NoError(t, parent.Reset()) + require.NoError(t, parent.Do(context.Background())) + require.Len(t, child.Option.Mutators, 1, "run %d: child Mutators must not accumulate", i) + assert.Equal(t, "C", child.Option.Mutators[0].(noopMutator).tag) + } +} + +func TestWorkflow_MultiLevelMutatorPropagation(t *testing.T) { + // grandparent → parent → child, each with one no-op Mutator. Mid-run, + // the child's Option.Mutators must be observed as [gp, p, c]. + gp := noopMutator{tag: "gp"} + p := noopMutator{tag: "p"} + c := noopMutator{tag: "c"} + + child := &Workflow{Option: WorkflowOption{Mutators: []Mutator{c}}} + // Use a Func step that captures child.Option.Mutators DURING execution. + var observed []Mutator + child.Add(Step(Func("observer", func(ctx context.Context) error { + observed = child.Option.Mutators + return nil + }))) + + parent := &Workflow{Option: WorkflowOption{Mutators: []Mutator{p}}} + parent.Add(Step(child)) + + grandparent := &Workflow{Option: WorkflowOption{Mutators: []Mutator{gp}}} + grandparent.Add(Step(parent)) + + require.NoError(t, grandparent.Do(context.Background())) + require.Len(t, observed, 3) + assert.Equal(t, "gp", observed[0].(noopMutator).tag) + assert.Equal(t, "p", observed[1].(noopMutator).tag) + assert.Equal(t, "c", observed[2].(noopMutator).tag) +} +``` + +- [ ] **Step 7.7: Run the full test suite** + +```bash +go test ./... -count=1 +``` + +Expected: PASS. If any test fails, fix the specific migration (almost certainly a leftover `w.MaxConcurrency` etc.). + +- [ ] **Step 7.8: Commit** + +```bash +git add . +git commit -m "test: migrate all tests to Option-shape and add inheritance/snapshot tests" +``` + +--- + +## Task 8: Migrate examples + +**Files:** +- Modify: `example/*.go` files that reference the changed symbols + +- [ ] **Step 8.1: Inventory example files** + +```bash +grep -rln 'MaxConcurrency\|DontPanic\|SkipAsError\|DefaultOption\|IsolateInterceptors\|PrependMutators\|PrependInterceptors\|MutatorReceiver\|InterceptorReceiver\|SubWorkflow' example/ +``` + +- [ ] **Step 8.2: Apply the same substitution table from Task 7 to each example** + +- [ ] **Step 8.3: For examples that use `flow.SubWorkflow`, update them to embed `flow.Workflow` directly** + +Before: +```go +type Deploy struct { + flow.SubWorkflow + Region string +} +``` + +After: +```go +type Deploy struct { + flow.Workflow + Region string +} +``` + +- [ ] **Step 8.4: Add a new example showing scalar inheritance** + +Create `example/14_scalar_inheritance_test.go`: + +```go +package example + +import ( + "context" + "fmt" + + flow "github.com/Azure/go-workflow" +) + +// ExampleScalarInheritance demonstrates how a parent Workflow's Option flows +// into a sub-workflow when the child leaves the field unset (nil pointer). +// +// In this example, only the parent sets DontPanic; the child workflow +// inherits it automatically, so the child's panicking step is recovered +// rather than crashing the process. +func ExampleScalarInheritance() { + child := &flow.Workflow{} + child.Add(flow.Step(flow.Func("panicker", func(ctx context.Context) error { + panic("boom") + }))) + + dontPanic := true + parent := &flow.Workflow{ + Option: flow.WorkflowOption{DontPanic: &dontPanic}, + } + parent.Add(flow.Step(child)) + + err := parent.Do(context.Background()) + fmt.Println(err != nil) + // Output: true +} +``` + +- [ ] **Step 8.5: Run example tests** + +```bash +go test ./example/... -count=1 +``` + +Expected: PASS. + +- [ ] **Step 8.6: Commit** + +```bash +git add example/ +git commit -m "example: migrate to Option-shape; add scalar-inheritance example" +``` + +--- + +## Task 9: Documentation and godoc + +**Files:** +- Modify: `workflow.go`, `interceptor.go`, `mutator.go`, `state.go`, `error.go`, `README.md` (if any field is referenced) + +- [ ] **Step 9.1: Update `Workflow.Add` godoc with the strong warning** + +Find the `// Add wires Builders …` doc comment above `Workflow.Add` (around line 94-100). Append (after the existing doc): + +``` +// WARNING: Add MUST NOT be called from inside the same Workflow's Do() (or +// any method transitively reachable from Do()) UNLESS guarded by sync.Once. +// Calling Add unguarded inside Do() leads to undefined behavior — possible +// failure modes include callback chains accumulating across runs (each +// BeforeStep/AfterStep firing N times on the N-th invocation), duplicate +// Step entries if new pointers are allocated each call, and parent +// introspection returning multiple matches. See the "Composite step MUST +// NOT call Add inside Do unguarded" scenario in the composite-steps spec. +// +// Acceptable patterns for sub-workflow construction: +// +// 1. Build at construction time (recommended): the constructor calls +// x.Add(...) before returning. +// 2. Construct inline inside Do() with a fresh *flow.Workflow: do NOT +// embed flow.Workflow; instead, allocate w := &flow.Workflow{} inside +// Do(), populate via w.Add(...), and call w.Do(ctx). To inherit +// parent Option, the containing step must implement +// WorkflowOptionReceiver. +// 3. Lazy build guarded by sync.Once: embed flow.Workflow and call +// x.Add(...) from inside x.once.Do(...). +``` + +- [ ] **Step 9.2: Update `Workflow` type godoc to describe sub-workflow patterns** + +Append to the existing `// Workflow orchestrates …` doc: + +``` +// Sub-workflows: +// +// type Deploy struct { +// flow.Workflow // embed: get Add/Do/Unwrap/InheritOption for free +// Region string +// } +// +// func NewDeploy(region string) *Deploy { +// d := &Deploy{Region: region} +// d.Add(flow.Step(/* … */)) // construction-time build (recommended) +// return d +// } +// +// The embedded Workflow's Option is reachable as d.Option. When d is added +// to a parent Workflow, the parent's Option propagates into d.Option via +// the WorkflowOptionReceiver.InheritOption mechanism, with the merge rules +// described in [WorkflowOption]. +``` + +- [ ] **Step 9.3: Update godoc references to removed symbols in other files** + +- `state.go:112` — change `// an error when Workflow.DontPanic is true.` to `// an error when Workflow.Option.DontPanic is true.`. +- `error.go:29` — change `Workflow.DontPanic` reference to `Workflow.Option.DontPanic`. +- `error.go:166` — same treatment. +- `mutator.go` — any leftover godoc mentioning `MutatorReceiver` or `PrependMutators` is replaced with `WorkflowOptionReceiver.InheritOption`. +- `interceptor.go` — any leftover godoc mentioning `InterceptorReceiver` or `PrependInterceptors` is replaced with `WorkflowOptionReceiver.InheritOption` and `Workflow.Option.StepInterceptors` / `AttemptInterceptors`. + +- [ ] **Step 9.4: Update README** + +Locate sub-workflow / configuration sections in `README.md` (use `grep -n 'SubWorkflow\|MaxConcurrency\|DontPanic\|DefaultOption' README.md`). Update each to reflect: +- Sub-workflow pattern: embed `flow.Workflow`, not `flow.SubWorkflow`. +- Configuration: under `Option WorkflowOption`, with pointer scalars. +- A short "scalar inheritance" note. + +If README has no examples touching these fields, skip the README edit and rely on godoc examples. + +- [ ] **Step 9.5: Add CHANGELOG entry** + +Append to `CHANGELOG.md` (create if missing): + +```markdown +## Unreleased + +### Breaking changes + +- `Workflow` no longer exposes the nine top-level configuration fields + (`MaxConcurrency`, `DontPanic`, `SkipAsError`, `Clock`, `DefaultOption`, + `Mutators`, `StepInterceptors`, `AttemptInterceptors`, + `IsolateInterceptors`). All configuration is now under + `Workflow.Option WorkflowOption`. Scalar fields are pointer-typed so that + "unset" and "explicit zero" are distinguishable. Migration is mechanical: + + // before + &Workflow{MaxConcurrency: 4, DontPanic: true} + + // after + mc := 4; dp := true + &Workflow{Option: WorkflowOption{MaxConcurrency: &mc, DontPanic: &dp}} + +- `DefaultOption` is renamed to `Option.StepDefaults`. +- `IsolateInterceptors` is renamed to `Option.DontInherit` and now opts out + of inheriting the entire WorkflowOption from a parent, not just + interceptors. +- `MutatorReceiver`, `InterceptorReceiver`, `Workflow.PrependMutators`, + `Workflow.PrependInterceptors`, and `findInterceptorReceiver` are + removed. Implement `WorkflowOptionReceiver.InheritOption` instead. + +### Deprecations + +- `flow.SubWorkflow` is deprecated; embed `flow.Workflow` directly. + SubWorkflow will be removed in the next major version of go-workflow. +- `flow.StepBuilder` (the type) is now also marked deprecated at the type + level (the method was already deprecated). The type, the `BuildStep()` + user hook, and the implicit invocation in `Workflow.Add` will all be + removed in the next major version of go-workflow, together with + `SubWorkflow` and `SubWorkflow.Reset`. Behavior is unchanged in this + release. + +### Behavior changes + +- Scalar configuration (`DontPanic`, `MaxConcurrency`, `SkipAsError`, + `Clock`, `StepDefaults`) now propagates from a parent Workflow into a + sub-workflow when the child leaves the field nil. Previously these did + not propagate. To opt out, set `Option.DontInherit = true` on the child, + or restate the desired value on the child explicitly. +``` + +- [ ] **Step 9.6: Commit** + +```bash +git add . +git commit -m "docs: update godoc, README, and CHANGELOG for Option consolidation" +``` + +--- + +## Task 10: Apply OpenSpec deltas to live specs + +**Files:** +- Modify: `openspec/specs/workflow-options/spec.md`, `openspec/specs/composite-steps/spec.md`, `openspec/specs/mutators/spec.md`, `openspec/specs/step-interceptor/spec.md` + +The change directory `openspec/changes/2026-05-12-workflow-option/specs/*/spec.md` contains the MODIFIED / ADDED / REMOVED requirement deltas. This task applies them to the live spec files. + +- [ ] **Step 10.1: Apply `workflow-options` delta** + +Open both files side by side: +- Source (delta): `openspec/changes/2026-05-12-workflow-option/specs/workflow-options/spec.md` +- Target: `openspec/specs/workflow-options/spec.md` + +For each `### Requirement: X` block in the delta: +- If marked MODIFIED, replace the existing requirement in the target with the delta's version. +- If under `## ADDED Requirements`, append to the target. +- If under `## REMOVED Requirements`, delete the corresponding requirement from the target. + +- [ ] **Step 10.2: Apply `composite-steps` delta** + +Same procedure. Particular care: +- `SubWorkflow — Workflow as a Step` requirement: target should now include the `Deprecated:` paragraph and the SubWorkflow-InheritOption scenario. +- Remove the two `*Workflow / SubWorkflow implements MutatorReceiver` requirements. +- Add the new `*Workflow implements WorkflowOptionReceiver` requirement. +- Add the four new sub-workflow build-timing scenarios. + +- [ ] **Step 10.3: Apply `mutators` delta** + +- Update field references to `Workflow.Option.Mutators`. +- Replace `MutatorReceiver` / `PrependMutators` propagation references with `WorkflowOptionReceiver.InheritOption`. + +- [ ] **Step 10.4: Apply `step-interceptor` delta** + +- Update field references to `Workflow.Option.StepInterceptors` / `AttemptInterceptors`. +- Remove `InterceptorReceiver` and `IsolateInterceptors` requirements. +- Add the new `DontInherit` requirement. +- Update propagation requirement to use `WorkflowOptionReceiver.InheritOption`. + +- [ ] **Step 10.5: Sanity check the updated specs** + +```bash +grep -n 'MutatorReceiver\|InterceptorReceiver\|IsolateInterceptors\|PrependMutators\|PrependInterceptors\|DefaultOption\|inheritedStep\|inheritedAttempt' openspec/specs/ +``` + +Expected: empty output (no stale references to removed symbols). + +- [ ] **Step 10.6: Commit** + +```bash +git add openspec/specs/ +git commit -m "spec: apply workflow-option deltas to live workflow-options/composite-steps/mutators/step-interceptor specs" +``` + +--- + +## Task 11: Final verification + +**Files:** +- (No edits; running verification commands) + +- [ ] **Step 11.1: Build** + +```bash +go build ./... +``` + +Expected: no output (success). + +- [ ] **Step 11.2: Vet** + +```bash +go vet ./... +``` + +Expected: no output (success). + +- [ ] **Step 11.3: Full test suite** + +```bash +go test ./... -race -count=1 +``` + +Expected: PASS for every package (including `./example/...`). The `-race` flag exercises the snapshot/restore concurrency story. + +- [ ] **Step 11.4: Grep for leftover references to removed symbols in source** + +```bash +grep -rn 'PrependMutators\|PrependInterceptors\|MutatorReceiver\|InterceptorReceiver\|findInterceptorReceiver\|IsolateInterceptors\|inheritedStep\|inheritedAttempt' --include='*.go' +``` + +Expected: empty (no source-side references to removed symbols). + +- [ ] **Step 11.5: Grep for direct reads of the removed fields** + +```bash +grep -rn 'w\.MaxConcurrency\|w\.DontPanic\|w\.SkipAsError\|w\.Clock\|w\.DefaultOption\|w\.Mutators\|w\.StepInterceptors\|w\.AttemptInterceptors' --include='*.go' | grep -v 'w\.Option\.' | grep -v '_test.go' +``` + +Expected: empty (every source-side scalar read goes through accessors; every slice read goes through `w.Option.X`). + +- [ ] **Step 11.6: Confirm `SubWorkflow` smoke test passes** + +```bash +go test ./... -run TestSubWorkflow -v +``` + +Expected: PASS. + +- [ ] **Step 11.7: Confirm scalar inheritance behavior change is exercised** + +```bash +go test ./... -run TestWorkflow_ScalarInheritance -v +go test ./... -run TestWorkflow_DoSnapshotRestore -v +go test ./... -run TestWorkflow_MultiLevelMutatorPropagation -v +``` + +Expected: PASS. + +- [ ] **Step 11.8: Final commit (only if any docs changed in Tasks 11)** + +Most likely no diff at this stage; skip if `git status` is clean. + +--- + +## Self-Review (Plan-Author Internal) + +**Spec coverage check.** Walking the change's spec deltas: + +- `workflow-options`: + - Every MODIFIED requirement (MaxConcurrency, DontPanic, SkipAsError, StepDefaults, Clock) → covered by Task 2 + Task 7 (field/test migration). + - ADDED: `WorkflowOption groups workflow-level configuration` → Task 1 (type) + Task 7 (migration test). + - ADDED: `WorkflowOptionReceiver propagates Option to sub-workflows` → Task 1 (interface) + Task 2 (`Workflow.InheritOption`) + Task 3 (`Do()` prologue propagation) + Task 6 (test matrix) + Task 7 (multi-level test). + - ADDED: `DontInherit opts out of all parent Option inheritance` → Task 2 (merge rule) + Task 6 (test). + - ADDED: `Do() snapshots and restores Option` → Task 3 (snapshot/restore) + Task 7 (no-accumulation test). + - ADDED: `Reset rewinds per-step status without touching the step set` → Task 2.7 (Reset godoc + behavior) + tests covered by existing `branch_test.go:170` etc. + - REMOVED: top-level fields / `MutatorReceiver` / `InterceptorReceiver` / `IsolateInterceptors` / `inheritedXxx` → Tasks 2, 4. +- `composite-steps`: + - MODIFIED `SubWorkflow — Workflow as a Step` (deprecated) + new SubWorkflow-InheritOption scenario → Task 5 + Task 7.5 smoke test. + - MODIFIED `Sub-workflow construction inside Do` → covered by Task 9.1 godoc warning + 9.2 patterns block. (No new test needed beyond the existing scenarios.) + - ADDED `*Workflow implements WorkflowOptionReceiver` → Task 2.8 + Task 6 test matrix. + - REMOVED two MutatorReceiver requirements → Task 4. + - Four new build-timing scenarios → Task 9 godoc; runtime guard is explicitly OUT-OF-SCOPE per the change's Section 11 future-work. +- `mutators`: field rename + InheritOption propagation → Tasks 2.5 + 3 + 7. +- `step-interceptor`: field rename + InheritOption propagation + DontInherit → Tasks 2.9 + 2.10 + 3 + 7. + +**Placeholder scan.** No "TBD", "TODO", "fill in later", "similar to" — every step has the actual code or substitution table. ✓ + +**Type consistency.** `Workflow.Option.X` naming is consistent throughout (`StepDefaults`, not `DefaultStepOption`; `DontInherit`, not `IsolateInterceptors`; `InheritOption`, not `PrependOption` / `SetOption`). Accessor names `maxConcurrency() / dontPanic() / skipAsError() / clock()` are unexported and used consistently in Tasks 2.10 and 3.3. ✓ + +**Build-stability invariant.** Tasks 2-5 are squashed into a single commit (Step 6.3) because the intermediate tree does not compile. After that commit, every subsequent task leaves the tree compiling. + +**Out-of-scope items, deliberately:** +- `go vet` analyzer for "Add called from Do()" — listed as Section 11 future work in the change's tasks.md; not implemented here. +- `Workflow.Reset` semantic widening (clearing `steps`) — explicitly rejected during brainstorming; current behavior preserved with updated godoc only. +- Helper `flow.Ptr[T]` / `flow.Bool` / `flow.Int` — explicitly rejected during brainstorming. diff --git a/docs/superpowers/specs/2026-05-12-workflow-option-design.md b/docs/superpowers/specs/2026-05-12-workflow-option-design.md new file mode 100644 index 0000000..9176eeb --- /dev/null +++ b/docs/superpowers/specs/2026-05-12-workflow-option-design.md @@ -0,0 +1,361 @@ +# Workflow Option Consolidation — Brainstorming Spec + +**Date:** 2026-05-12 +**Topic:** Group `Workflow`'s scattered configuration fields into a single +`WorkflowOption` value, unify the parent→child propagation interfaces into one +`WorkflowOptionReceiver`, and remove `SubWorkflow` (deprecation now, removal in +the next major). + +This is the brainstorming output. The implementation-facing change lives at +`openspec/changes/2026-05-12-workflow-option/`. + +--- + +## Why + +Today `Workflow` carries nine configuration fields directly on the struct +(`MaxConcurrency`, `DontPanic`, `SkipAsError`, `Clock`, `DefaultOption`, +`Mutators`, `StepInterceptors`, `AttemptInterceptors`, `IsolateInterceptors`). +The list keeps growing, the propagation rules into sub-workflows are +inconsistent (Mutators / Interceptors propagate via two separate ad-hoc +interfaces; the other six don't propagate at all), and the surface that gets +promoted into a user struct that embeds `Workflow` is wide and noisy. + +Three problems fall out of this: + +1. **Visual clutter.** `Workflow` mixes runtime state and configuration in one + place; the godoc reader has to know which is which. +2. **Inconsistent propagation.** `MutatorReceiver` and `InterceptorReceiver` + are two separate interfaces with two separate type assertions in the + scheduler. Any future workflow-level option that should "flow into a child" + would need a third interface. +3. **`SubWorkflow` no longer earns its keep.** Its only remaining job is to + keep the inner `Workflow` field unexported on a user struct that embeds it. + With the configuration fields collapsed under a single `Option` field, that + value disappears — the user struct ends up with one promoted name + (`Option`) either way. After `BuildStep` was deprecated alongside the + Mutator API, `SubWorkflow.Reset` is also already deprecated and removed in + the next major. Keeping `SubWorkflow` adds a parallel type with no unique + capability. + +## What Changes + +- Introduce `WorkflowOption` struct that owns all nine fields. Scalar fields + become pointer-typed (`*int`, `*bool`, `clock.Clock`, `*StepOption`) so + unset / explicitly-zero are distinguishable; slice fields stay slices. +- `Workflow` exposes one named field `Option WorkflowOption`. The nine former + top-level fields are removed. +- `IsolateInterceptors` is renamed `DontInherit`; semantics extend from + "don't inherit interceptors" to "don't inherit any of the parent's + WorkflowOption". Naming aligns with `DontPanic`. +- Introduce single propagation interface `WorkflowOptionReceiver` with a + single method `InheritOption(parent WorkflowOption)`. `*Workflow` implements + it. +- Remove `MutatorReceiver`, `InterceptorReceiver`, + `Workflow.PrependMutators`, `Workflow.PrependInterceptors`, + `findInterceptorReceiver`. They are not in widespread external use; clean + removal is preferred over a parallel deprecated path. +- `SubWorkflow` is deprecated (godoc `// Deprecated:` notice) but kept for one + release window, because it is something users embed in their own structs. + Its `PrependMutators` / `PrependInterceptors` methods are removed (the + interfaces they satisfy no longer exist); a single `InheritOption` + delegating to the inner `Workflow` is added. +- The scheduling loop is rewritten: a single `Do()` prologue pass walks each + root step's Unwrap chain via `findOptionReceiver`, and calls + `InheritOption(parent.Option)` exactly once before scheduling begins. +- The `inheritedStep` / `inheritedAttempt` side fields and their careful + `reset()` exclusion logic are removed. The new mechanism uses a + snapshot-and-restore of `w.Option` around `Do()`, which is simpler and + prevents accumulation across repeated `Do()` calls without special-cased + state. +- The runtime no longer offers a helper to construct pointer values + (`flow.Bool`, `flow.Int`, etc.). Callers use the language: `mc := 4; + Option: WorkflowOption{MaxConcurrency: &mc}`, or Go 1.26's `new(value)`, + or their own `Ptr[T]` helper. + +## Decisions Locked In + +| Decision | Choice | Reason | +|---|---|---| +| `Option` is a named field, not embedded | `Workflow.Option WorkflowOption` | An embedded `WorkflowOption` would still be promoted onto user structs that embed `Workflow`, defeating the "one name on the user struct" goal. | +| Scalar fields are pointers; slices stay slices | `*int`, `*bool`, `clock.Clock`, `*StepOption`; `[]Mutator` etc. | Pointers cleanly distinguish "unset" from "explicit zero" so child can override parent's `DontPanic=true` with `DontPanic=false`. Slices already encode "unset" as nil/empty and have an established prepend semantics for cross-cutting concerns. | +| Single `InheritOption` method, not three | `WorkflowOptionReceiver { InheritOption(parent WorkflowOption) }` | Keeps API surface small; merge details (scalar override vs slice prepend vs `DontInherit` no-op) live inside `Workflow.InheritOption`, not in the contract. | +| Scalar inheritance: child nil → take parent's value | Standard Kubernetes / AWS-SDK pattern | Lets a sub-workflow positively inherit `DontPanic` / `MaxConcurrency` from its parent without restating it. Direct answer to the use case "I want `DontPanic` to flow into the child." | +| Slice inheritance: parent prepended to child | Same as today's `PrependMutators` / `PrependInterceptors` | Mutators and Interceptors are cross-cutting workflow-level concerns; parent's contributions running before child's is the existing, correct behavior. | +| `DontInherit` is one boolean, not nine | Whole-Option opt-out | Field-by-field opt-out (one `Inherit*` companion per field) doubles the API surface to expose an internal mechanism. The existing `IsolateInterceptors` shows the whole-Option pattern is sufficient in practice. | +| Old receiver interfaces and `*Workflow.Prepend*` methods are removed, not deprecated | Hard removal | They have no production users outside this repo. Carrying a deprecated parallel propagation path complicates the scheduler for one release with no benefit. | +| `SubWorkflow` is deprecated, not removed | One-release window | Users embed it inside their own structs; an immediate removal would break their type definitions, not just call sites. Removing in the next major is consistent with the existing `BuildStep` deprecation. | +| `inheritedStep` / `inheritedAttempt` side fields removed | Replaced by `Do()` snapshot/restore of `w.Option` | The original side-channel existed because `reset()` couldn't safely clear the parent's just-prepended slices. Snapshotting at `Do()` entry and restoring at exit is simpler and eliminates the special-cased rule entirely. | +| Sub-workflow via direct embedding of `flow.Workflow` | The official recommended pattern | With the nine top-level fields collapsed, embedding `flow.Workflow` only promotes one configuration name (`Option`) plus `Add` / `Do` / `InheritOption`. `SubWorkflow`'s only remaining unique feature was hiding the nine fields, which no longer exist. | +| Behavior of mutating `child.Option` after `parent.Do()` has started but before `child.Do()` runs | Undefined | Already covered by the existing "mutating a Workflow mid-run is undefined" rule; explicitly noted in `WorkflowOption` godoc. | +| Helper for pointer-to-value | Not provided | Users have `&v`, Go 1.26 `new(value)`, or their own one-line `Ptr[T any]` helper. | + +## Architecture + +### Type layout + +```go +// workflow_option.go (new) + +// WorkflowOption groups all configuration that a Workflow exposes to its +// caller AND inherits from a parent Workflow when used as a sub-workflow +// step. +// +// Scalar fields are pointers so that "unset" and "explicit zero value" are +// distinguishable: a nil pointer means "inherit from parent (or use the +// runtime default)"; a non-nil pointer is the child's own choice. Slice +// fields are not pointer-typed; the parent's slice is prepended to the +// child's slice (parent contributions run first), preserving the existing +// Mutator and Interceptor propagation semantics. +// +// Mutating a Workflow's Option after Do() has started is undefined behavior. +type WorkflowOption struct { + MaxConcurrency *int + DontPanic *bool + SkipAsError *bool + Clock clock.Clock // interface; nil = inherit / wall clock + StepDefaults *StepOption // already a pointer + + Mutators []Mutator // parent prepended on inherit + StepInterceptors []StepInterceptor // parent prepended on inherit + AttemptInterceptors []AttemptInterceptor // parent prepended on inherit + + // When true and this Workflow is used as a sub-workflow step, + // InheritOption is a no-op: nothing flows in from the parent. Replaces + // the previous IsolateInterceptors flag and now governs the entire + // WorkflowOption, not just interceptors. + DontInherit bool +} + +// WorkflowOptionReceiver is implemented by any Step that contains a +// sub-workflow. The parent's Do() prologue locates the nearest receiver in +// each root step's Unwrap chain and calls InheritOption ONCE before any +// scheduling begins, so the child's Do() observes the merged Option. +// +// *Workflow itself implements this interface; users get inheritance for +// free by embedding flow.Workflow in their own Step type. +type WorkflowOptionReceiver interface { + InheritOption(parent WorkflowOption) +} +``` + +```go +// workflow.go + +type Workflow struct { + Option WorkflowOption + StepBuilder + + steps map[Steper]*State + statusChange *sync.Cond + leaseBucket chan struct{} + waitGroup sync.WaitGroup + isRunning sync.Mutex +} + +// InheritOption implements WorkflowOptionReceiver. +func (w *Workflow) InheritOption(parent WorkflowOption) { + if w.Option.DontInherit { + return + } + if w.Option.MaxConcurrency == nil { w.Option.MaxConcurrency = parent.MaxConcurrency } + if w.Option.DontPanic == nil { w.Option.DontPanic = parent.DontPanic } + if w.Option.SkipAsError == nil { w.Option.SkipAsError = parent.SkipAsError } + if w.Option.Clock == nil { w.Option.Clock = parent.Clock } + if w.Option.StepDefaults == nil { w.Option.StepDefaults = parent.StepDefaults } + + w.Option.Mutators = prependSlice(parent.Mutators, w.Option.Mutators) + w.Option.StepInterceptors = prependSlice(parent.StepInterceptors, w.Option.StepInterceptors) + w.Option.AttemptInterceptors = prependSlice(parent.AttemptInterceptors, w.Option.AttemptInterceptors) +} +``` + +`prependSlice` allocates a fresh backing array; it never mutates either input +slice. This guarantee is what allows the snapshot/restore mechanism (below) to +be a shallow copy. + +### Internal accessors + +The runtime needs effective scalar values. Add unexported helpers on +`*Workflow`: + +```go +func (w *Workflow) maxConcurrency() int { if w.Option.MaxConcurrency == nil { return 0 }; return *w.Option.MaxConcurrency } +func (w *Workflow) dontPanic() bool { return w.Option.DontPanic != nil && *w.Option.DontPanic } +func (w *Workflow) skipAsError() bool { return w.Option.SkipAsError != nil && *w.Option.SkipAsError } +func (w *Workflow) clock() clock.Clock { if w.Option.Clock == nil { return clock.New() }; return w.Option.Clock } +func (w *Workflow) defaultStepOption() *StepOption { return w.Option.StepDefaults } +``` + +All existing `w.MaxConcurrency`, `w.DontPanic`, `w.SkipAsError`, `w.Clock`, +`w.DefaultOption` reads in the runtime are rewritten to call these. + +### Propagation flow + +```go +func (w *Workflow) Do(ctx context.Context) error { + if !w.isRunning.TryLock() { + return ErrWorkflowIsRunning + } + defer w.isRunning.Unlock() + + // Snapshot Option so InheritOption writes are reverted at exit, allowing + // multiple Do() runs without accumulation. + optSnapshot := w.Option + defer func() { w.Option = optSnapshot }() + + w.reset() + w.init() + + // Propagate parent Option into any sub-workflow root steps before tick. + for step := range w.steps { + if recv := findOptionReceiver(step); recv != nil { + recv.InheritOption(w.Option) + } + } + + return w.tick(ctx) +} + +func findOptionReceiver(s Steper) WorkflowOptionReceiver { + var found WorkflowOptionReceiver + Walk(s, func(s Steper) bool { + if r, ok := s.(WorkflowOptionReceiver); ok { + found = r + return false // stop on first match + } + return true + }) + return found +} +``` + +Snapshot is a shallow copy of `WorkflowOption`. `InheritOption` only ever +writes via `prependSlice` (which allocates fresh) or by overwriting nil +pointer fields (which doesn't touch parent's pointer values). So the restore +at defer is correct without a deep copy. + +### `SubWorkflow` during the deprecation window + +```go +// Deprecated: Embed flow.Workflow directly. SubWorkflow will be removed in +// the next major version of go-workflow. +type SubWorkflow struct{ w Workflow } + +func (s *SubWorkflow) Unwrap() Steper { return &s.w } +func (s *SubWorkflow) Add(builders ...Builder) *Workflow { return s.w.Add(builders...) } +func (s *SubWorkflow) Do(ctx context.Context) error { return s.w.Do(ctx) } + +// InheritOption forwards to the inner Workflow so SubWorkflow continues to +// participate in parent → child Option propagation during the deprecation +// window. +func (s *SubWorkflow) InheritOption(parent WorkflowOption) { s.w.InheritOption(parent) } +``` + +`SubWorkflow.Reset` and `SubWorkflow.PrependMutators` / +`PrependInterceptors` are removed (the latter two satisfy interfaces that no +longer exist; `Reset` was already deprecated for removal). + +## User Migration + +**Before:** + +```go +type Deploy struct { + flow.SubWorkflow + Region string +} + +w := &flow.Workflow{ + MaxConcurrency: 4, + DontPanic: true, + Mutators: []flow.Mutator{logMutator}, + IsolateInterceptors: false, +} +``` + +**After:** + +```go +type Deploy struct { + flow.Workflow // embed Workflow directly + Region string +} + +mc := 4 +dp := true +w := &flow.Workflow{ + Option: flow.WorkflowOption{ + MaxConcurrency: &mc, + DontPanic: &dp, + Mutators: []flow.Mutator{logMutator}, + }, +} +``` + +## Removed in This Release (No Deprecation) + +- `MutatorReceiver` (interface) +- `InterceptorReceiver` (interface) +- `Workflow.PrependMutators` (method) +- `Workflow.PrependInterceptors` (method) +- `findInterceptorReceiver` (function) +- `Workflow.IsolateInterceptors` (renamed → `Option.DontInherit`) +- The nine top-level option fields on `Workflow` (moved into `Option`) +- `SubWorkflow.PrependMutators` / `SubWorkflow.PrependInterceptors` +- `Workflow.inheritedStep` / `inheritedAttempt` (internal; replaced by + Option snapshot/restore) + +## Removed in the Next Major + +- `SubWorkflow` (struct) +- `SubWorkflow.Reset` (already deprecated) +- `SubWorkflow.InheritOption` (goes with the type) +- All godoc references to `SubWorkflow` + +## Testing Strategy + +### Migration of existing tests + +`workflow_options_test.go` and any test that constructs `&Workflow{ Field: ... +}` for one of the nine former fields is rewritten mechanically to use +`Option: WorkflowOption{ Field: ... }`, with pointer fields wrapped. Test +*assertions* do not change — the user-observable behavior of every existing +field is preserved. + +### New: `workflow_option_inherit_test.go` + +Table-driven matrix: + +- Scalar nil → inherits parent's value +- Scalar non-nil → child wins +- Scalar explicit zero (e.g., `*int = 0`) → child wins, distinguished from + nil/inherit +- `Mutators` / `StepInterceptors` / `AttemptInterceptors` → parent prepended +- `Clock` nil → inherits parent +- `StepDefaults` nil → inherits parent +- `DontInherit = true` → no field changes regardless of parent + +Plus dedicated tests: + +- **Multi-level nesting:** grandparent → parent → child, each with one + Mutator; child's effective `Mutators` is `[grandparent, parent, child]`. +- **`Do()` snapshot/restore:** running `parent.Do()` twice with a child that + has `Mutators=[B]` and parent `Mutators=[A]` results in child's effective + `Mutators` being `[A, B]` *during* each run and `[B]` *after* each run, with + no accumulation across runs. +- **`SubWorkflow` deprecation smoke:** an embedded `SubWorkflow` still + receives the parent's Option via the delegated `InheritOption`. + +### Untouched + +`execution_model_test.go`, `branch_test.go`, `condition_test.go`, +`retry_test.go`, `wrap_test.go`, `mutator_test.go` content — the scheduling +core, branching, condition, retry, wrapping, and Mutator semantics themselves +are unchanged. + +### Examples + +`example/13_mutators_test.go` and any example referencing `flow.SubWorkflow` +update to embed `flow.Workflow` and use `Option`. A short note pointing to the +deprecated `SubWorkflow` is preserved during the deprecation window. diff --git a/error.go b/error.go index 3322003..f2d0a25 100644 --- a/error.go +++ b/error.go @@ -26,7 +26,7 @@ func Skip(err error) ErrSkip { return ErrSkip{err} } // - ErrSucceed → Succeeded // - ErrCancel → Canceled // - ErrSkip → Skipped -// - ErrPanic → Failed (only ever produced when Workflow.DontPanic is true) +// - ErrPanic → Failed (only ever produced when Workflow.Option.DontPanic is true) // - ErrBeforeStep→ Failed (the failure happened in a Before/Input callback, // not in Do itself) type ErrSucceed struct{ error } diff --git a/error_test.go b/error_test.go index 6e3c501..19e11b3 100644 --- a/error_test.go +++ b/error_test.go @@ -54,7 +54,7 @@ func TestErrWorkflowOutputOrdering(t *testing.T) { stepA := newSerialStep("A-second", fmt.Errorf("A failed")) stepB := newSerialStep("B-third", fmt.Errorf("B failed")) - w := &flow.Workflow{Clock: mockClock} + w := &flow.Workflow{Option: flow.WorkflowOption{Clock: mockClock}} w.Add( flow.Step(stepC), flow.Step(stepA).DependsOn(stepC).When(flow.Always), @@ -100,7 +100,7 @@ func TestErrWorkflowTieBreakByName(t *testing.T) { stepZ := newSerialStep("Z-step", fmt.Errorf("Z failed")) stepA := newSerialStep("A-step", fmt.Errorf("A failed")) - w := &flow.Workflow{Clock: mockClock} + w := &flow.Workflow{Option: flow.WorkflowOption{Clock: mockClock}} w.Add(flow.Step(stepZ), flow.Step(stepA)) done := make(chan error, 1) diff --git a/example/07_retry_and_timeout_test.go b/example/07_retry_and_timeout_test.go index b6c7e23..dd83b21 100644 --- a/example/07_retry_and_timeout_test.go +++ b/example/07_retry_and_timeout_test.go @@ -59,7 +59,7 @@ func ExampleAddStep_Retry() { // We use a mock clock so the example is fast and deterministic. func ExampleAddStep_Retry_perTryTimeout() { mock := clock.NewMock() - w := &flow.Workflow{Clock: mock} + w := &flow.Workflow{Option: flow.WorkflowOption{Clock: mock}} startedAttempt := make(chan struct{}, 16) hangForever := flow.Func("hang", func(ctx context.Context) error { diff --git a/example/09_workflow_options_test.go b/example/09_workflow_options_test.go index 9d3263b..19f1a67 100644 --- a/example/09_workflow_options_test.go +++ b/example/09_workflow_options_test.go @@ -12,19 +12,22 @@ import ( // # Workflow options: tuning execution // // **What you'll learn** -// - `MaxConcurrency` caps how many Steps run at the same time. -// - `DontPanic` recovers panics inside Step bodies and converts them to -// errors instead of crashing the program. +// - `Option.MaxConcurrency` caps how many Steps run at the same time. +// - `Option.DontPanic` recovers panics inside Step bodies and converts +// them to errors instead of crashing the program. +// - All workflow-level options live under the named `Option` field +// (type `WorkflowOption`). Scalar fields are pointer-typed so unset / +// explicit-zero are distinguishable. // -// **Other options (see godoc on `Workflow`)** -// - `Clock` — inject a deterministic clock for testing. -// - `DefaultOption` — apply a `StepOption` (timeout, retry, …) to every -// Step. Per-Step options still win. -// - `SkipAsError` — treat `Skipped` Steps as workflow errors. +// **Other options (see godoc on `WorkflowOption`)** +// - `Option.Clock` — inject a deterministic clock for testing. +// - `Option.StepDefaults` — apply a `StepOption` (timeout, retry, …) to +// every Step. Per-Step options still win. +// - `Option.SkipAsError` — treat `Skipped` Steps as workflow errors. // ExampleWorkflow_MaxConcurrency caps parallelism. Steps that are eligible // to run beyond the cap wait for a slot to free up. -func ExampleWorkflow_MaxConcurrency() { +func ExampleWorkflowOption_MaxConcurrency() { const cap = 2 var live atomic.Int32 @@ -50,7 +53,8 @@ func ExampleWorkflow_MaxConcurrency() { }) } - w := &flow.Workflow{MaxConcurrency: cap} + mc := cap + w := &flow.Workflow{Option: flow.WorkflowOption{MaxConcurrency: &mc}} w.Add(flow.Steps(work("a"), work("b"), work("c"), work("d"), work("e"))) go func() { @@ -68,8 +72,9 @@ func ExampleWorkflow_MaxConcurrency() { // ExampleWorkflow_DontPanic enables panic recovery. A panicking Step is // reported as a normal `Failed` Step instead of crashing the program. -func ExampleWorkflow_DontPanic() { - w := &flow.Workflow{DontPanic: true} +func ExampleWorkflowOption_DontPanic() { + dontPanic := true + w := &flow.Workflow{Option: flow.WorkflowOption{DontPanic: &dontPanic}} w.Add( flow.Step(flow.Func("oops", func(context.Context) error { panic("boom") @@ -83,3 +88,31 @@ func ExampleWorkflow_DontPanic() { // Output: // ErrWorkflow? true } + +// ExampleWorkflowOption_inheritance shows how scalar Option fields +// propagate from a parent Workflow into a sub-Workflow that left them +// unset (nil pointer). Once you set DontPanic on the parent, every nested +// child workflow recovers panics too — no need to restate the option on +// every sub-workflow. +// +// To opt out of inheritance entirely, set Option.DontInherit = true on +// the child. To override just one field, set that field's pointer on the +// child (an explicit `&zero` pointer is distinguishable from "unset"). +func ExampleWorkflowOption_inheritance() { + // Inner workflow: leaves DontPanic unset. + inner := new(flow.Workflow) + inner.Add(flow.Step(flow.Func("boom", func(ctx context.Context) error { + panic("inner panic") + }))) + + // Outer workflow sets DontPanic at the top level. The inner sub-workflow + // inherits it via WorkflowOptionReceiver.InheritOption. + dontPanic := true + outer := &flow.Workflow{Option: flow.WorkflowOption{DontPanic: &dontPanic}} + outer.Add(flow.Step(inner)) + + err := outer.Do(context.Background()) + fmt.Println("recovered as error:", err != nil) + // Output: + // recovered as error: true +} diff --git a/example/10_observability_test.go b/example/10_observability_test.go index 3539e68..1da77ea 100644 --- a/example/10_observability_test.go +++ b/example/10_observability_test.go @@ -15,7 +15,8 @@ import ( // - Use `AttemptInterceptor` to observe each individual attempt — perfect // for per-try metrics or transforming an attempt's error. // - Parent → child workflows inherit interceptors automatically; opt out -// with `IsolateInterceptors`. +// with `Option.DontInherit` (which now opts out of the entire +// `WorkflowOption`, not just interceptors). // // **Two layers, deliberately separated** // @@ -55,7 +56,7 @@ func ExampleStepInterceptor() { bar = flow.Func("Bar", func(ctx context.Context) error { fmt.Println("Bar"); return nil }) ) workflow := &flow.Workflow{ - StepInterceptors: []flow.StepInterceptor{logger}, + Option: flow.WorkflowOption{StepInterceptors: []flow.StepInterceptor{logger}}, } workflow.Add( flow.Step(foo).DependsOn(bar), @@ -90,8 +91,10 @@ func ExampleAttemptInterceptor() { passAfter2 := &flow.NamedStep{Name: "PassAfter2", Steper: &passAfter{n: 2}} workflow := &flow.Workflow{ - StepInterceptors: []flow.StepInterceptor{stepLog}, - AttemptInterceptors: []flow.AttemptInterceptor{attemptLog}, + Option: flow.WorkflowOption{ + StepInterceptors: []flow.StepInterceptor{stepLog}, + AttemptInterceptors: []flow.AttemptInterceptor{attemptLog}, + }, } workflow.Add( flow.Step(passAfter2). @@ -113,19 +116,20 @@ func ExampleAttemptInterceptor() { // [step ] end PassAfter2 (err=) } -// ExampleInterceptorReceiver shows that when a Workflow is used as a Step -// inside another Workflow, the outer Workflow's interceptors automatically -// wrap every Step in the inner Workflow. +// ExampleWorkflowOptionReceiver shows that when a Workflow is used as a +// Step inside another Workflow, the outer Workflow's interceptors +// automatically wrap every Step in the inner Workflow. // -// The mechanism is the InterceptorReceiver interface: any Step that contains -// a sub-Workflow (Workflow itself, SubWorkflow) implements -// PrependInterceptors, and the parent walks the Step tree (via Unwrap) to -// find a receiver. So you can wrap a sub-Workflow in flow.Name (or any other -// Steper wrapper) without losing inheritance. +// The mechanism is the WorkflowOptionReceiver interface: any Step that +// contains a sub-Workflow (Workflow itself, the deprecated SubWorkflow) +// implements InheritOption, and the parent walks the Step tree (via Unwrap) +// to find a receiver. So you can wrap a sub-Workflow in flow.Name (or any +// other Steper wrapper) without losing inheritance. // // To opt out of inheritance and run an inner Workflow with only its own -// interceptors, set IsolateInterceptors: true on the inner. -func ExampleInterceptorReceiver() { +// interceptors (and other Option fields), set Option.DontInherit: true on +// the inner. +func ExampleWorkflowOptionReceiver() { outerLogger := flow.StepInterceptorFunc(func(ctx context.Context, step flow.Steper, next func(context.Context) error) error { fmt.Printf("[outer] %s\n", step) return next(ctx) @@ -141,17 +145,17 @@ func ExampleInterceptorReceiver() { ) // isolated has the same shape but opts out of inheritance. - isolated := &flow.Workflow{IsolateInterceptors: true} + isolated := &flow.Workflow{Option: flow.WorkflowOption{DontInherit: true}} isolated.Add( flow.Step(flow.Func("isolated-X", func(ctx context.Context) error { fmt.Println("isolated-X"); return nil })), ) outer := &flow.Workflow{ - StepInterceptors: []flow.StepInterceptor{outerLogger}, + Option: flow.WorkflowOption{StepInterceptors: []flow.StepInterceptor{outerLogger}}, } // Naming the sub-workflows is purely cosmetic — it just makes the log // readable. flow.Name wraps each sub-workflow in a NamedStep, and the - // outer interceptor finds the InterceptorReceiver by walking through + // outer interceptor finds the WorkflowOptionReceiver by walking through // the wrapper via Unwrap. outer.Add( flow.Name(inner, "inner"), diff --git a/example/13_mutators_test.go b/example/13_mutators_test.go index d1f88ca..e0f5f3d 100644 --- a/example/13_mutators_test.go +++ b/example/13_mutators_test.go @@ -16,7 +16,7 @@ import ( // `flow.Builder` to register `Input` / `BeforeStep` / `AfterStep` / // `Retry` / `Timeout` / ..., or both. // - Parent-Workflow Mutators reach into sub-Workflows automatically — the -// same MutatorReceiver mechanism described in 10_observability for +// same WorkflowOptionReceiver mechanism described in 10_observability for // interceptors. // // **Where they fit** @@ -68,14 +68,16 @@ func (n *Notify) Do(ctx context.Context) error { // changes are applied before any Input / BeforeStep / Do runs. func ExampleMutator_fieldDefaults() { w := &flow.Workflow{ - Mutators: []flow.Mutator{ - // Anywhere a *Notify shows up, default Channel if it's empty. - flow.Mutate[*Notify](func(_ context.Context, n *Notify) flow.Builder { - if n.Channel == "" { - n.Channel = "#release" - } - return nil // pure field mutation; no Builder needed - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + // Anywhere a *Notify shows up, default Channel if it's empty. + flow.Mutate[*Notify](func(_ context.Context, n *Notify) flow.Builder { + if n.Channel == "" { + n.Channel = "#release" + } + return nil // pure field mutation; no Builder needed + }), + }, }, } w.Add( @@ -103,18 +105,20 @@ func ExampleMutator_fieldDefaults() { // AfterStep, etc. func ExampleMutator_contributeConfig() { w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*Notify](func(_ context.Context, n *Notify) flow.Builder { - if n.Channel == "" { - n.Channel = "#release" - } - // Same builder methods you use at Add() time — BeforeStep, - // AfterStep, Input, Retry, Timeout, ... — all work here. - return flow.Step(n).BeforeStep(func(ctx context.Context, s flow.Steper) (context.Context, error) { - s.(*Notify).Title = "[prod] " + s.(*Notify).Title - return ctx, nil - }) - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*Notify](func(_ context.Context, n *Notify) flow.Builder { + if n.Channel == "" { + n.Channel = "#release" + } + // Same builder methods you use at Add() time — BeforeStep, + // AfterStep, Input, Retry, Timeout, ... — all work here. + return flow.Step(n).BeforeStep(func(ctx context.Context, s flow.Steper) (context.Context, error) { + s.(*Notify).Title = "[prod] " + s.(*Notify).Title + return ctx, nil + }) + }), + }, }, } w.Add(flow.Step(&Notify{Title: "promo live", Body: "v2"})) @@ -131,14 +135,16 @@ func ExampleMutator_ctxValue() { type envKey struct{} w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*Notify](func(ctx context.Context, n *Notify) flow.Builder { - if env, ok := ctx.Value(envKey{}).(string); ok && n.Channel == "" { - // e.g. #release-prod vs #release-dev - n.Channel = "#release-" + env - } - return nil - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*Notify](func(ctx context.Context, n *Notify) flow.Builder { + if env, ok := ctx.Value(envKey{}).(string); ok && n.Channel == "" { + // e.g. #release-prod vs #release-dev + n.Channel = "#release-" + env + } + return nil + }), + }, }, } w.Add(flow.Step(&Notify{Title: "deploy started", Body: "v1.2.3"})) @@ -154,11 +160,11 @@ func ExampleMutator_ctxValue() { // sub-Workflow, and even when the sub-Workflow adds them lazily at run // time. // -// The propagation mechanism is the same `MutatorReceiver` interface used -// by interceptors in 10_observability: any Step that contains a -// sub-Workflow (a `*Workflow` used as a Step, or a struct embedding -// `flow.SubWorkflow`) automatically receives the parent's Mutators before -// the inner Workflow starts scheduling. +// The propagation mechanism is the same `WorkflowOptionReceiver` interface +// used by interceptors in 10_observability: any Step that contains a +// sub-Workflow (a `*Workflow` used as a Step, or a struct embedding the +// deprecated `flow.SubWorkflow`) automatically receives the parent's Option +// (Mutators included) before the inner Workflow starts scheduling. func ExampleMutator_subWorkflow() { // A "release stage" sub-workflow that posts a start and an end notice. stage := new(flow.Workflow).Add( @@ -169,13 +175,15 @@ func ExampleMutator_subWorkflow() { ) outer := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*Notify](func(_ context.Context, n *Notify) flow.Builder { - if n.Channel == "" { - n.Channel = "#release" - } - return nil - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*Notify](func(_ context.Context, n *Notify) flow.Builder { + if n.Channel == "" { + n.Channel = "#release" + } + return nil + }), + }, }, } outer.Add( @@ -206,19 +214,21 @@ func ExampleMutator_subWorkflow() { // later Mutators can rely on earlier Mutators having already run. func ExampleMutator_multipleInOrder() { w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*Notify](func(_ context.Context, n *Notify) flow.Builder { - return flow.Step(n).Input(func(_ context.Context, n *Notify) error { - n.Body += " [m1]" - return nil - }) - }), - flow.Mutate[*Notify](func(_ context.Context, n *Notify) flow.Builder { - return flow.Step(n).Input(func(_ context.Context, n *Notify) error { - n.Body += " [m2]" - return nil - }) - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*Notify](func(_ context.Context, n *Notify) flow.Builder { + return flow.Step(n).Input(func(_ context.Context, n *Notify) error { + n.Body += " [m1]" + return nil + }) + }), + flow.Mutate[*Notify](func(_ context.Context, n *Notify) flow.Builder { + return flow.Step(n).Input(func(_ context.Context, n *Notify) error { + n.Body += " [m2]" + return nil + }) + }), + }, }, } w.Add( diff --git a/execution_model_test.go b/execution_model_test.go index c6b7c17..f7d6fbe 100644 --- a/execution_model_test.go +++ b/execution_model_test.go @@ -88,7 +88,8 @@ func TestWorkflowErr(t *testing.T) { func TestSkip(t *testing.T) { t.Parallel() t.Run("should skip step if return ErrSkip", func(t *testing.T) { - w := &Workflow{SkipAsError: true} + skipAsError := true + w := &Workflow{Option: WorkflowOption{SkipAsError: &skipAsError}} skipMe := Func("SkipMe", func(ctx context.Context) error { return Skip(fmt.Errorf("skip me")) }) @@ -220,7 +221,7 @@ func TestConcurrentExecution(t *testing.T) { func TestStepResultFinishedAtPopulated(t *testing.T) { mockClock := clock.NewMock() step := Func("test-step", func(ctx context.Context) error { return nil }) - w := &Workflow{Clock: mockClock} + w := &Workflow{Option: WorkflowOption{Clock: mockClock}} w.Add(Step(step)) err := w.Do(context.Background()) diff --git a/interceptor.go b/interceptor.go index 9e2e6b1..8e0a8e8 100644 --- a/interceptor.go +++ b/interceptor.go @@ -39,40 +39,4 @@ func (f AttemptInterceptorFunc) InterceptAttempt(ctx context.Context, step Stepe return f(ctx, step, attempt, next) } -// InterceptorReceiver is implemented by any Step that contains a sub-workflow -// (notably *Workflow itself and SubWorkflow). The parent's stepExecution -// calls PrependInterceptors ONCE — in executeWithRetry, just before the -// retry loop — so the parent's interceptor chain wraps the child's -// interceptor chain for the duration of the step. -// -// Implementations should be careful not to mutate the user-supplied base -// chain or accumulate inherited entries across runs (see Workflow's -// `inheritedStep` / `inheritedAttempt` design). -// -// The parent looks for an InterceptorReceiver by walking the Step tree via -// Unwrap (using the same protocol as As[T] / Has[T]). This means inheritance -// keeps working when the sub-workflow is wrapped in a Name / NamedStep / any -// other Steper wrapper that exposes Unwrap. The first receiver found in -// pre-order is used. -type InterceptorReceiver interface { - PrependInterceptors(step []StepInterceptor, attempt []AttemptInterceptor) -} -// findInterceptorReceiver returns the first InterceptorReceiver in the Step -// tree rooted at s, walking via Unwrap in pre-order. Returns nil if none of -// the unwrapped Steps satisfies InterceptorReceiver. -// -// This lets a sub-workflow be wrapped in a Steper-only wrapper (e.g. -// NamedStep, which embeds the Steper interface and therefore does not -// promote PrependInterceptors) without losing parent-interceptor inheritance. -func findInterceptorReceiver(s Steper) InterceptorReceiver { - var found InterceptorReceiver - Traverse(s, func(s Steper, _ []Steper) TraverseDecision { - if r, ok := s.(InterceptorReceiver); ok { - found = r - return TraverseStop - } - return TraverseContinue - }) - return found -} diff --git a/mutator.go b/mutator.go index a8efbc5..3d284b2 100644 --- a/mutator.go +++ b/mutator.go @@ -9,17 +9,6 @@ type Mutator interface { applyTo(ctx context.Context, step Steper) (matched bool, target Steper, builder Builder) } -// MutatorReceiver is implemented by Steps that host a sub-workflow (e.g. -// [SubWorkflow] or *Workflow used as a step) so that parent workflows can -// propagate their [Mutator]s into the inner workflow before it schedules its -// own steps. -type MutatorReceiver interface { - // PrependMutators inserts mw at the front of the receiver's Mutators - // list, so propagated parent Mutators run before locally-registered - // child Mutators. - PrependMutators(mw []Mutator) -} - // Mutate constructs a [Mutator] that runs against any step whose concrete type // matches T anywhere along its Unwrap() chain (within a single workflow's // boundaries). The first matching layer is passed to fn. fn returns a [Builder] @@ -46,7 +35,10 @@ func (m mutatorFunc[T]) applyTo(ctx context.Context, step Steper) (bool, Steper, return TraverseStop } // Stop at workflow boundaries: do NOT descend into a nested workflow's - // inner steps from here. Inner steps are reached via PrependMutators. + // inner steps from here. Inner steps are reached when the inner + // workflow runs its own Do() prologue — the parent's Mutators have + // already been merged into the child's Option.Mutators via + // [WorkflowOptionReceiver.InheritOption]. if _, isWorkflow := s.(interface { StateOf(Steper) *State }); isWorkflow { diff --git a/mutator_test.go b/mutator_test.go index 78656c5..0a439b5 100644 --- a/mutator_test.go +++ b/mutator_test.go @@ -79,7 +79,8 @@ func TestMutate_outerWrapperWinsWhenItIsTheTarget(t *testing.T) { func TestMutate_doesNotCrossWorkflowBoundary(t *testing.T) { // A *Workflow sits between the outer step and the inner *mutFoo. - // applyTo must NOT descend into it; that's what PrependMutators is for. + // applyTo must NOT descend into it; the parent's Mutators reach the + // inner workflow via WorkflowOptionReceiver.InheritOption instead. innerFoo := &mutFoo{} innerWf := new(Workflow).Add(Step(innerFoo)) diff --git a/openspec/changes/archive/2026-05-12-workflow-option/design.md b/openspec/changes/archive/2026-05-12-workflow-option/design.md new file mode 100644 index 0000000..ab641d6 --- /dev/null +++ b/openspec/changes/archive/2026-05-12-workflow-option/design.md @@ -0,0 +1,248 @@ +# Workflow Option Consolidation — Design + +This document covers the implementation-facing specifics. The brainstorming +output (decision log, alternatives considered, naming discussion) is at +`docs/superpowers/specs/2026-05-12-workflow-option-design.md`. Anything covered +there is referenced rather than duplicated. + +--- + +## Summary of Decisions + +| Decision | Choice | Reason | +|---|---|---| +| Configuration grouping | New `WorkflowOption` struct, exposed as `Workflow.Option` (named, not embedded) | Embedded would still promote field names onto user structs that embed `Workflow`, defeating the "one name on the user struct" goal. | +| Scalar field types | Pointers (`*int`, `*bool`, `clock.Clock`, `*StepOption`) | Distinguishes "unset → inherit from parent" from "explicit zero → child wins". Standard K8s / AWS-SDK pattern. | +| Slice field types | Stay `[]Mutator` etc. | Existing prepend semantics already encode "unset" as nil/empty; pointers would add no value. | +| Propagation interface | Single `WorkflowOptionReceiver { InheritOption(parent WorkflowOption) }` | Replaces `MutatorReceiver` and `InterceptorReceiver`. Merge details live inside `Workflow.InheritOption`, not in the contract. | +| Scalar inheritance rule | Child-nil pointer takes parent's value; non-nil child wins | Lets sub-workflow positively inherit `DontPanic` / `MaxConcurrency` from parent without restating. | +| Slice inheritance rule | Parent prepended to child | Preserves existing Mutator / Interceptor semantics. | +| Opt-out granularity | One `DontInherit bool` covering the whole Option | Per-field `Inherit*` companions would double API surface to expose an internal mechanism. The previous `IsolateInterceptors` shows whole-Option opt-out is sufficient. | +| Renaming `IsolateInterceptors` | `DontInherit` | Name matches `DontPanic`; semantics widen from interceptor-only to whole-Option. | +| Old receiver interfaces fate | Remove (no deprecation) | No widespread external implementations. Carrying parallel deprecated paths complicates the scheduler for one release with no benefit. | +| Old top-level configuration fields fate | Remove (breaking, no deprecation) | Same release as the receiver removal; migration is mechanical. Go does not effectively support `// Deprecated:` on struct fields (staticcheck recognizes it but most editor tooling doesn't). | +| `SubWorkflow` fate | Deprecate now, remove next major | Users embed it in their own structs; immediate removal would break their type definitions, not just call sites. One release window mirrors the `BuildStep` deprecation. | +| `StepBuilder` (type) fate | Mark deprecated at the type level now; remove next major | The `BuildStep` *method* was already deprecated by the Mutator change. The type itself is embedded by `Workflow` and therefore promoted onto any user struct embedding `flow.Workflow` — same surface-area concern this change is solving for the nine option fields. Behavior is unchanged this release; type + method + user hook + implicit `Workflow.Add` invocation all remain until the next major. | +| `SubWorkflow.PrependMutators` / `PrependInterceptors` | Remove now | The interfaces they satisfy no longer exist. A single `SubWorkflow.InheritOption` delegates to inner `w.InheritOption` for the deprecation window. | +| Propagation invocation site | Once-per-`Do()` prologue, after `init()`, before `tick` | Single call site; both Mutator and Interceptor inheritance happen at the same moment. The previous Mutator-at-`:641` / Interceptor-at-`:768` split was historical; nothing in the runtime requires it. | +| Receiver lookup | New `findOptionReceiver(s)`, walks `Unwrap()` chain, returns first match | Mirrors the removed `findInterceptorReceiver`. | +| Multiple-`Do()` accumulation prevention | Shallow snapshot of `w.Option` at `Do()` entry, defer restore | Replaces the `inheritedStep` / `inheritedAttempt` side fields and their special-cased "`reset()` must not clear these" rule. Allowed because `prependSlice` always allocates fresh, and overwriting nil-pointer fields with parent values does not mutate the parent's pointers. | +| Mid-run mutation of `Option` | Undefined behavior | Already covered by the existing "mutating a Workflow mid-run is undefined" rule; reaffirmed in `WorkflowOption` godoc. | +| Helper for pointer-to-value | Not provided | Users have `&v`, Go 1.26 `new(value)`, or their own `Ptr[T any]` helper. | + +--- + +## Implementation Notes + +### `WorkflowOption` and `WorkflowOptionReceiver` + +New file `workflow_option.go`: + +```go +type WorkflowOption struct { + MaxConcurrency *int + DontPanic *bool + SkipAsError *bool + Clock clock.Clock + StepDefaults *StepOption + + Mutators []Mutator + StepInterceptors []StepInterceptor + AttemptInterceptors []AttemptInterceptor + + DontInherit bool +} + +type WorkflowOptionReceiver interface { + InheritOption(parent WorkflowOption) +} +``` + +### `Workflow` struct + +The configuration fields disappear. Runtime fields stay: + +```go +type Workflow struct { + Option WorkflowOption + StepBuilder + + steps map[Steper]*State + statusChange *sync.Cond + leaseBucket chan struct{} + waitGroup sync.WaitGroup + isRunning sync.Mutex +} +``` + +`inheritedStep` and `inheritedAttempt` are removed; the snapshot/restore in +`Do()` covers the same invariant. + +### `InheritOption` + +```go +func (w *Workflow) InheritOption(parent WorkflowOption) { + if w.Option.DontInherit { + return + } + if w.Option.MaxConcurrency == nil { w.Option.MaxConcurrency = parent.MaxConcurrency } + if w.Option.DontPanic == nil { w.Option.DontPanic = parent.DontPanic } + if w.Option.SkipAsError == nil { w.Option.SkipAsError = parent.SkipAsError } + if w.Option.Clock == nil { w.Option.Clock = parent.Clock } + if w.Option.StepDefaults == nil { w.Option.StepDefaults = parent.StepDefaults } + w.Option.Mutators = prependSlice(parent.Mutators, w.Option.Mutators) + w.Option.StepInterceptors = prependSlice(parent.StepInterceptors, w.Option.StepInterceptors) + w.Option.AttemptInterceptors = prependSlice(parent.AttemptInterceptors, w.Option.AttemptInterceptors) +} +``` + +`prependSlice` MUST allocate a fresh backing array and never mutate either +input slice. This is what allows the snapshot at `Do()` entry to be a shallow +copy. + +### `Do()` prologue + +```go +func (w *Workflow) Do(ctx context.Context) error { + if !w.isRunning.TryLock() { return ErrWorkflowIsRunning } + defer w.isRunning.Unlock() + + optSnapshot := w.Option + defer func() { w.Option = optSnapshot }() + + w.reset() + w.init() + + for step := range w.steps { + if recv := findOptionReceiver(step); recv != nil { + recv.InheritOption(w.Option) + } + } + + return w.tick(ctx) +} +``` + +`findOptionReceiver` mirrors the removed `findInterceptorReceiver`: + +```go +func findOptionReceiver(s Steper) WorkflowOptionReceiver { + var found WorkflowOptionReceiver + Walk(s, func(s Steper) bool { + if r, ok := s.(WorkflowOptionReceiver); ok { + found = r + return false + } + return true + }) + return found +} +``` + +### Internal accessors for scalars + +The runtime currently reads `w.MaxConcurrency`, `w.DontPanic`, `w.SkipAsError`, +`w.Clock`, `w.DefaultOption` directly. Each call site is rewritten to use a +small helper that handles nil-pointer dereferencing and runtime defaults: + +```go +func (w *Workflow) maxConcurrency() int { if w.Option.MaxConcurrency == nil { return 0 }; return *w.Option.MaxConcurrency } +func (w *Workflow) dontPanic() bool { return w.Option.DontPanic != nil && *w.Option.DontPanic } +func (w *Workflow) skipAsError() bool { return w.Option.SkipAsError != nil && *w.Option.SkipAsError } +func (w *Workflow) clock() clock.Clock { if w.Option.Clock == nil { return clock.New() }; return w.Option.Clock } +``` + +Known direct-read sites in `workflow.go` (line numbers approximate, pre-change): +`108`, `188`, `374-378`, `498`, `501`, `652`, `668`, `724`, `751`, `766`, +`776`, `778`, `800`, `824`. All are rewritten to call helpers. + +### `SubWorkflow` during the deprecation window + +```go +// Deprecated: Embed flow.Workflow directly. SubWorkflow will be removed in +// the next major version of go-workflow. +type SubWorkflow struct{ w Workflow } + +func (s *SubWorkflow) Unwrap() Steper { return &s.w } +func (s *SubWorkflow) Add(builders ...Builder) *Workflow { return s.w.Add(builders...) } +func (s *SubWorkflow) Do(ctx context.Context) error { return s.w.Do(ctx) } +func (s *SubWorkflow) InheritOption(parent WorkflowOption) { s.w.InheritOption(parent) } +``` + +`SubWorkflow.Reset` (already deprecated for removal), `SubWorkflow.PrependMutators`, +and `SubWorkflow.PrependInterceptors` are removed in this change. The first +goes with the `BuildStep` deprecation; the latter two satisfy interfaces that +no longer exist. + +--- + +## Coordination + +This change touches the same scheduling-time path as the existing +`step-interceptor` and `mutators` capabilities. Specifically: + +- The Mutator dispatch site in the scheduler (`workflow.go:641`, + `if recv, ok := step.(MutatorReceiver); ok`) is removed. Mutators are now + delivered to a sub-workflow via `WorkflowOptionReceiver.InheritOption` in + the prologue, and the sub-workflow's own scheduling pass dispatches them. + The Mutator authoring contract (signature, once-per-step, Builder return) + is unchanged. +- The Interceptor dispatch site (`workflow.go:768`, + `findInterceptorReceiver`) is removed. Interceptors propagate via the same + `InheritOption` prologue pass. Effective interceptor chain construction + inside the child workflow remains identical. +- `effectiveStepInterceptors` / `effectiveAttemptInterceptors` previously + read from the side fields `inheritedStep` / `inheritedAttempt`. After this + change, both the parent's and child's interceptors live in the same + `Option.StepInterceptors` / `Option.AttemptInterceptors` slices (already + merged by `InheritOption`), so the helpers simplify to direct returns. +- The `composite-steps` capability is updated to recommend embedding + `flow.Workflow` directly; `flow.SubWorkflow` is preserved one release for + in-flight migrations. + +--- + +## Behavior Change to Document + +Setting a scalar option (`DontPanic`, `MaxConcurrency`, etc.) on the parent +workflow now causes that value to flow into any sub-workflow that left the +field unset (nil pointer). Before this change those scalars did not +propagate. Two implications: + +1. **Positive use case** — the motivating example: a parent that wants + `DontPanic` for the whole tree no longer needs to restate it on every + sub-workflow. This was previously a manual chore. +2. **Mitigation for users who relied on non-propagation**: set + `Option.DontInherit = true` on the child, or set the desired non-default + explicitly on the child (e.g., `MaxConcurrency: &noLimit` with `noLimit + := 0` to keep the child unlimited under a parent that limits). + +This is documented in `WorkflowOption` godoc, the `workflow-options` spec, +and the CHANGELOG. + +--- + +## Test Plan + +Two existing files require migration: + +- `workflow_options_test.go` — every `&Workflow{ Field: ... }` literal is + rewritten to use `Option: WorkflowOption{ ... }` with scalars wrapped as + pointers. Assertions unchanged. +- `workflow_mutator_test.go` — any test that constructs Mutators on + `Workflow.Mutators` is rewritten to `Option.Mutators`. Tests that exercise + `PrependMutators` are rewritten to verify `InheritOption` instead. + +One existing file likely requires migration if it touches interceptor +propagation (`workflow_test.go`, depending on what the existing tests cover). + +New file: `workflow_option_inherit_test.go` covering the table-driven matrix +in the brainstorming spec (scalar nil/set/explicit-zero × all five scalar +fields, slice prepend × all three slice fields, `DontInherit`), plus +multi-level nesting and `Do()` snapshot/restore. + +`SubWorkflow` deprecation smoke test (one happy-path case) lives alongside +the `SubWorkflow` definition or in a dedicated `subworkflow_deprecated_test.go`. + +Examples: `example/13_mutators_test.go` and any other example referencing +`flow.SubWorkflow` migrate to embed `flow.Workflow` directly. diff --git a/openspec/changes/archive/2026-05-12-workflow-option/proposal.md b/openspec/changes/archive/2026-05-12-workflow-option/proposal.md new file mode 100644 index 0000000..827d346 --- /dev/null +++ b/openspec/changes/archive/2026-05-12-workflow-option/proposal.md @@ -0,0 +1,124 @@ +# Workflow Option Consolidation + +## Why + +`Workflow` carries nine configuration fields directly on the struct +(`MaxConcurrency`, `DontPanic`, `SkipAsError`, `Clock`, `DefaultOption`, +`Mutators`, `StepInterceptors`, `AttemptInterceptors`, `IsolateInterceptors`) +and three independent propagation mechanisms (`MutatorReceiver`, +`InterceptorReceiver`, none-for-the-rest). Three problems: + +1. **Cluttered struct surface.** Reading the `Workflow` godoc, the user must + distinguish nine configuration fields from runtime-state fields by hand. + Embedding `Workflow` in a user struct (the recommended way to build a + sub-workflow) promotes all nine onto the user's type. +2. **Inconsistent propagation.** Mutators and Interceptors propagate via two + different ad-hoc interfaces and two separate type-asserts in the + scheduler. The other six configuration fields don't propagate at all. + Adding a third workflow-level option that should flow into a child would + need a third interface. +3. **`SubWorkflow` no longer earns its keep.** Its only remaining unique + feature is hiding the inner `Workflow`'s exported field set. Once the nine + configuration fields collapse under one `Option` named field, the + "exposure" is reduced to a single name (`Option`) regardless of whether + the user embeds `Workflow` or `SubWorkflow`. After Mutators deprecated + `BuildStep`, `SubWorkflow.Reset` was already deprecated. Keeping + `SubWorkflow` adds a parallel type with no unique capability. + +Brainstorming context, alternatives considered, and per-decision rationale +are in `docs/superpowers/specs/2026-05-12-workflow-option-design.md`. + +## What Changes + +- Add `WorkflowOption` struct and `WorkflowOptionReceiver` interface in a new + `workflow_option.go`. Scalar fields are pointer-typed (`*int`, `*bool`, + `clock.Clock`, `*StepOption`) so unset / explicit-zero are distinguishable. + Slice fields stay slices; parent slices are prepended to the child's on + inheritance. +- Replace `Workflow`'s nine top-level configuration fields with a single + `Option WorkflowOption` named field. **Breaking change.** +- Rename `IsolateInterceptors` → `Option.DontInherit`; semantics widen from + "don't inherit interceptors" to "don't inherit any of the parent's + WorkflowOption". Naming aligns with `DontPanic`. +- `*Workflow` implements `WorkflowOptionReceiver` via a new `InheritOption` + method. Merge rules: nil scalar pointer → take parent's value; non-nil → + child wins; slices → parent prepended; `DontInherit=true` → no-op. +- Rewrite the scheduler's parent→child propagation: a single `Do()` prologue + pass walks each root step's Unwrap chain via a new `findOptionReceiver` and + calls `InheritOption(w.Option)` once before `tick`. Replaces the two + separate type-asserts at the previous `workflow.go:641` (Mutator) and + `:768` (Interceptor) call sites. +- Replace `Workflow.inheritedStep` / `inheritedAttempt` side fields with a + shallow snapshot of `w.Option` at `Do()` entry plus a `defer` restore at + exit. Eliminates the special-case rule that `reset()` cannot clear those + fields. +- **Remove (no deprecation):** `MutatorReceiver`, `InterceptorReceiver`, + `Workflow.PrependMutators`, `Workflow.PrependInterceptors`, + `findInterceptorReceiver`, `Workflow.IsolateInterceptors`, + `SubWorkflow.PrependMutators`, `SubWorkflow.PrependInterceptors`. They are + not in widespread external use; clean removal is preferred over carrying a + parallel deprecated path. +- **Deprecate (remove next major):** `flow.SubWorkflow` (struct). It is + embedded in user structs, so removal would break their type definitions + rather than just call sites — one release window of grace. Add + `SubWorkflow.InheritOption` delegating to the inner `Workflow` so + inheritance still works during the window. `SubWorkflow.Reset` was already + deprecated; remains deprecated. +- **Deprecate (remove next major):** `flow.StepBuilder` (the type). Its + only method `BuildStep` was already deprecated in the Mutator change; the + type itself is now marked deprecated at the godoc level too so users who + embed `flow.Workflow` see the warning. Behavior is unchanged this release; + the type, the method, the `BuildStep()` user hook, and the implicit + invocation in `Workflow.Add` all remain until the next major version, when + they are removed together. +- No helper for pointer-to-value (`flow.Bool`, `flow.Int`) — callers use + `&v`, Go 1.26 `new(value)`, or their own one-line generic. +- Update `example/13_mutators_test.go` and any other examples that reference + `flow.SubWorkflow` to embed `flow.Workflow` directly and use `Option`. + +## Capabilities + +### Modified Capabilities + +- `workflow-options`: every requirement is rewritten — fields are now + accessed under `Workflow.Option`, scalar fields are pointer-typed; new + requirements describe `WorkflowOption`, `WorkflowOptionReceiver`, + `InheritOption` propagation rules (scalar nil-inherit, slice prepend), + `DontInherit` opt-out, `Do()` snapshot/restore semantics, and the + prologue-pass propagation site. +- `composite-steps`: `flow.SubWorkflow` requirement marked deprecated; + `SubWorkflow implements MutatorReceiver` and `*Workflow implements + MutatorReceiver` requirements removed (the interface no longer exists); + added requirement that `*Workflow` implements `WorkflowOptionReceiver` and + is the recommended pattern for sub-workflows. +- `mutators`: requirements about `MutatorReceiver` propagation rewritten to + reference `WorkflowOptionReceiver.InheritOption` instead. `Workflow.Mutators + field` requirement updated to read `Workflow.Option.Mutators`. +- `step-interceptor`: `InterceptorReceiver` requirement removed; + `IsolateInterceptors` requirement removed; replaced with reference to + `WorkflowOptionReceiver` and `Option.DontInherit`. Field references updated + to `Workflow.Option.StepInterceptors` / + `Workflow.Option.AttemptInterceptors`. + +## Impact + +- **Breaking change to all callers** that construct `&Workflow{ Field: ... }` + for any of the nine former top-level fields. Migration is mechanical: wrap + in `Option: WorkflowOption{ ... }` and convert scalars to pointers. +- **Breaking change to anything implementing `MutatorReceiver` or + `InterceptorReceiver` directly.** No known external implementations. + Migration: implement `WorkflowOptionReceiver.InheritOption` instead. +- **Behavior change (positive):** scalar configuration fields (`DontPanic`, + `MaxConcurrency`, `SkipAsError`, `Clock`, `StepDefaults`) now propagate + from parent into sub-workflows when the child leaves them nil. Previously + they didn't propagate at all. Existing code that sets these on the parent + but didn't expect them on the child needs to set `Option.DontInherit` on + the child or restate the desired non-default. This is the answer to the + "I want sub-workflow to also be `DontPanic`" use case. +- **No behavior change to slice propagation.** `Mutators`, + `StepInterceptors`, `AttemptInterceptors` continue to be parent-prepended + (just via `InheritOption` instead of `PrependMutators` / `PrependInterceptors`). +- **No new dependencies.** +- **`SubWorkflow` deprecation window**: one minor release. The next major + version of `go-workflow` removes the type entirely; users embedding + `flow.SubWorkflow` MUST migrate to `flow.Workflow` before that release. diff --git a/openspec/changes/archive/2026-05-12-workflow-option/specs/composite-steps/spec.md b/openspec/changes/archive/2026-05-12-workflow-option/specs/composite-steps/spec.md new file mode 100644 index 0000000..2842780 --- /dev/null +++ b/openspec/changes/archive/2026-05-12-workflow-option/specs/composite-steps/spec.md @@ -0,0 +1,200 @@ +# composite-steps — Spec Delta + +## MODIFIED Requirements + +### Requirement: SubWorkflow — Workflow as a Step + +**Deprecated:** New code SHOULD embed `flow.Workflow` directly instead. The +`flow.SubWorkflow` type is preserved for one release window to allow +in-flight users to migrate; it SHALL be removed in the next major version +of `go-workflow`. + +`SubWorkflow` is an embeddable helper that exposes a nested `Workflow` as a +Step. Embedding `flow.SubWorkflow` in a struct gives it `Do`, `Add`, and +`Unwrap` methods. The outer Workflow orchestrates the composite Step; the +inner Workflow orchestrates the sub-steps. The outer Workflow can reach into +the inner Steps using `Has`, `As`, and `HasStep`. + +During the deprecation window, `SubWorkflow` SHALL implement +`WorkflowOptionReceiver.InheritOption` by delegating to the embedded +`Workflow`, so parent → child Option propagation continues to work for +`SubWorkflow`-embedding steps. + +`SubWorkflow.Reset` remains deprecated (per the existing Mutator-era +deprecation) and SHALL be removed together with `SubWorkflow` itself in the +next major version. + +#### Scenario: Workflow can be used directly as a Step +- **WHEN** a `*flow.Workflow` is added as a Step inside another Workflow +- **THEN** the outer Workflow treats the inner Workflow as an opaque Step and calls + its `Do` method, which in turn executes the inner DAG + +#### Scenario: Sub-steps are reachable from the outer Workflow +- **WHEN** an outer Workflow contains a composite Step that embeds `SubWorkflow` + with inner Steps +- **THEN** `flow.As[*InnerStepType](outerWorkflow)` returns the inner Step instances + +#### Scenario: SubWorkflow inherits parent Option during the deprecation window +- **WHEN** an outer Workflow with `Option.DontPanic = &true` contains a + composite step that embeds `flow.SubWorkflow` +- **THEN** the inner workflow observes `DontPanic = true` via the delegated + `InheritOption` + +--- + +### Requirement: Sub-workflow construction inside Do + +The framework SHALL support constructing a composite Step's sub-workflow inside its +`Do(ctx)` method, using a freshly created `flow.Workflow{}` and standard `Add` / +`Step` / `Pipe` builders. This pattern MUST NOT require implementing `BuildStep`. + +The recommended pattern is to embed `flow.Workflow` directly in the +composite step's struct: + +```go +type Deploy struct { + flow.Workflow + Region string +} + +func (d *Deploy) Do(ctx context.Context) error { + d.Add(/* ... */) + return d.Workflow.Do(ctx) +} +``` + +When this pattern is used, the inner workflow SHALL remain visible to +parent-level Mutators and Interceptors via the +`WorkflowOptionReceiver.InheritOption` propagation mechanism (see the +`workflow-options` capability spec). + +When this pattern is used WITHOUT exposing the inner workflow via embedding +or `Unwrap` (e.g. by constructing a local `flow.Workflow{}` inside `Do` and +discarding it after `Do` returns), the inner workflow SHALL be invisible to +parent introspection helpers (`Has`, `As`, `HasStep`) and to parent Option +propagation. This is intentional: such a step is fully self-contained and +opaque to the outer workflow. + +#### Scenario: Composite step embeds Workflow and inherits parent Option +- **WHEN** a composite step embeds `flow.Workflow` and the parent has Mutators registered +- **THEN** the parent's Mutators reach the inner steps via `InheritOption` (invoked once + per inner-workflow execution; once-per-step semantics inside the inner workflow then + take over) + +#### Scenario: Local sub-workflow inside Do is opaque to parent +- **WHEN** a step constructs a `flow.Workflow{}` inside `Do` without embedding it + in the step's struct and without exposing it via `Unwrap` +- **THEN** parent `flow.As[T](outer)` does not find inner steps and parent Option + does not propagate to them + +#### Scenario: Composite step embedding flow.Workflow can be Do()-ed multiple times +- **GIVEN** `type X struct { flow.Workflow }` whose constructor calls `x.Add(...)` once at construction time +- **WHEN** the parent runs N times (with `parent.Reset()` between runs) +- **THEN** each run executes the inner DAG once with no accumulation, because + `Workflow.Add` is idempotent on the step pointer key (the same step pointer + added twice merges its `StepConfig` into the existing entry rather than + creating a duplicate scheduling unit) + +#### Scenario: Composite step MUST NOT call Add inside Do unguarded +- **GIVEN** `type X struct { flow.Workflow }` whose `Do(ctx)` calls `x.Add(...)` + unconditionally on every invocation (no `sync.Once`, no fresh-Workflow + inline construction) +- **WHEN** the parent invokes `x.Do()` more than once on the same `X` instance + (across separate parent runs OR via the retry loop within a single parent run) +- **THEN** the behavior is **undefined**. The framework MAY exhibit any of: + - per-step `BeforeStep`/`AfterStep` callbacks accumulated across runs + (each callback firing N times on the N-th invocation), because + `StepConfig.Merge` appends callback chains and does not deduplicate + - duplicate Step entries of the same logical type in `x.steps` if + `Add` is called with newly-allocated step pointers each `Do()` + - parent introspection (`flow.As`, `Has`) returning multiple matches when + only one was expected + - Mutator dispatch firing against stale step instances retained from + previous invocations +- **REASON** `Workflow.steps` and `StepConfig` are designed for build-once / + execute-many. The pointer-key idempotence of `Add` prevents duplicate + scheduling on identical pointers but does not deduplicate appended + `Before`/`After` callback chains. The framework SHALL NOT add a runtime + guard for this misuse: the parent's `isRunning` lock is held at the + outer scope, not inside `x`'s scope, and `x.isRunning` is acquired and + released across each retry attempt — neither offers a stable signal for + distinguishing legitimate `sync.Once`-guarded lazy initialisation from + unguarded re-`Add` per invocation. +- **REQUIRED PATTERNS** instead — choose exactly one: + 1. **Build at construction time** — the constructor (e.g., `NewX(...)`) + calls `x.Add(...)` before returning. Recommended when the inner DAG + is known at construction. + 2. **Construct inline inside Do() with a fresh *flow.Workflow** — do NOT + embed `flow.Workflow` in `X`; instead, allocate `w := &flow.Workflow{}` + inside `x.Do`, populate via `w.Add(...)`, and call `w.Do(ctx)`. The + containing step MUST implement `WorkflowOptionReceiver` to forward + parent Option (see the next scenario). Recommended when the inner + DAG depends on runtime state in `ctx`. + 3. **Lazy build guarded by sync.Once** — embed `flow.Workflow` and call + `x.Add(...)` from inside `x.once.Do(...)` so the build happens + exactly once across all invocations. Acceptable when the user + explicitly wants a single-construction, multi-execution lifecycle + on the same `X`. + +#### Scenario: Local sub-workflow inside Do is opaque to parent +- **WHEN** a step constructs a `flow.Workflow{}` inside `Do` without embedding it + in the step's struct and without exposing it via `Unwrap` +- **THEN** parent `flow.As[T](outer)` does not find inner steps and parent Option + does not propagate to them + +#### Scenario: Inline sub-workflow inherits parent Option only via explicit InheritOption +- **GIVEN** `type Y struct { inheritedOpt flow.WorkflowOption }` with + `func (y *Y) InheritOption(p flow.WorkflowOption) { y.inheritedOpt = p }` + and a `Do(ctx)` that constructs `w := &flow.Workflow{Option: y.inheritedOpt}` + then calls `w.Add(...)` and `w.Do(ctx)` +- **WHEN** the parent runs and propagates Option via `findOptionReceiver` +- **THEN** `findOptionReceiver` discovers `Y` (because `Y` directly implements + `WorkflowOptionReceiver`); `y.InheritOption` records the parent's Option; + the inline `*flow.Workflow` constructed inside `y.Do` then inherits it +- **WITHOUT** the explicit `InheritOption` method on `Y`, the parent's Option + does NOT reach the inline sub-workflow (the inline workflow is opaque to + the parent per the preceding scenario) + +--- + +## ADDED Requirements + +### Requirement: *Workflow implements WorkflowOptionReceiver + +`*flow.Workflow`, when used directly as a Step in another workflow (whether +added directly or embedded in a user struct), SHALL implement +`WorkflowOptionReceiver` (defined in the `workflow-options` capability spec). +Its `InheritOption` implementation merges the parent's `WorkflowOption` +into its own per the rules in `workflow-options`. + +This is the recommended pattern for sub-workflows: a user struct embeds +`flow.Workflow` directly and gets `Add`, `Do`, `Unwrap`, and Option +inheritance for free, with only one configuration name (`Option`) promoted +onto the user struct. + +#### Scenario: Embedded *Workflow inherits parent Option +- **GIVEN** `type Deploy struct { flow.Workflow; Region string }` and a parent with `Option.DontPanic = &true` +- **WHEN** the parent runs, with `&Deploy{Region: "westus2"}` added as a step +- **THEN** the embedded inner workflow observes `DontPanic = true` for its execution + +#### Scenario: Nested *Workflow added directly inherits parent Option +- **GIVEN** a parent with `Option.Mutators = [M]` and a child `*flow.Workflow` containing a step matching `M` +- **WHEN** the parent runs with the child added as a step +- **THEN** `M` runs against the matching inner step + +--- + +## REMOVED Requirements + +### Requirement: ~~SubWorkflow implements MutatorReceiver~~ + +**Reason:** `MutatorReceiver` no longer exists. `SubWorkflow` participates in +parent → child propagation via `WorkflowOptionReceiver.InheritOption` during +its deprecation window (see the modified `SubWorkflow — Workflow as a Step` +requirement above). + +### Requirement: ~~*Workflow implements MutatorReceiver~~ + +**Reason:** `MutatorReceiver` no longer exists. `*Workflow` now implements +`WorkflowOptionReceiver` instead (see the new `*Workflow implements +WorkflowOptionReceiver` requirement above). diff --git a/openspec/changes/archive/2026-05-12-workflow-option/specs/mutators/spec.md b/openspec/changes/archive/2026-05-12-workflow-option/specs/mutators/spec.md new file mode 100644 index 0000000..c37716d --- /dev/null +++ b/openspec/changes/archive/2026-05-12-workflow-option/specs/mutators/spec.md @@ -0,0 +1,76 @@ +# mutators — Spec Delta + +## MODIFIED Requirements + +### Requirement: Workflow.Option.Mutators field + +`flow.Workflow` SHALL expose a `Mutators []Mutator` field under +`Workflow.Option` (the new `WorkflowOption` grouping introduced by the +`workflow-options` capability). Mutators in this slice are evaluated in +slice order against each step in the Workflow's state map: element 0 runs +first, element n-1 runs last, among the Mutators whose target type matches +that step. + +A workflow with `Option.Mutators == nil` or an empty slice SHALL behave +identically to one without any Mutators at all. + +The previous top-level `Workflow.Mutators` field SHALL NOT exist. + +#### Scenario: Slice order is preserved among matching Mutators +- **WHEN** two Mutators `[m1, m2]` both registered for `*Foo` are present under + `Option.Mutators`, and `m1` and `m2` each return a Builder with a `BeforeStep` + callback +- **THEN** the merged step's `Before` chain runs `m1`'s callback before `m2`'s + +--- + +### Requirement: Mutator propagation to nested workflows + +A nested workflow (whether `*Workflow` used directly as a step or a user +struct embedding `flow.Workflow`) SHALL receive its parent's Mutators via +the `WorkflowOptionReceiver.InheritOption` mechanism defined in the +`workflow-options` capability. + +Specifically, the parent SHALL invoke `child.InheritOption(parent.Option)` +once in its `Do()` prologue. The implementation of `InheritOption` MUST +prepend the parent's `Option.Mutators` to the child's `Option.Mutators` +slice (parent contributions run first within the child). + +The parent SHALL NOT directly walk into a child workflow's `state` map to +apply Mutators; that remains the inner workflow's responsibility once it +begins its own scheduling pass with the merged `Option.Mutators` slice. + +The previous `MutatorReceiver` interface and `PrependMutators` method SHALL +NOT exist. + +#### Scenario: Parent Mutator reaches inner *Foo via InheritOption +- **GIVEN** `parent.Option.Mutators = [flow.Mutate[*Foo](fn)]` +- **AND** a child `*Workflow` containing a `*Foo` step +- **WHEN** parent runs, with child added as a step +- **THEN** parent invokes `child.InheritOption(parent.Option)` once before scheduling +- **AND** the inner workflow's first-schedule pass evaluates the propagated Mutator against `*Foo` + +#### Scenario: Parent Mutator reaches inner *Foo when child is wrapped +- **GIVEN** `parent.Option.Mutators = [flow.Mutate[*Foo](fn)]` +- **AND** a child `*Workflow` containing a `*Foo` step, added to parent via `flow.Name(child, "name")` +- **WHEN** parent runs +- **THEN** the parent's Mutator reaches the inner `*Foo` via `findOptionReceiver`'s `Unwrap` walk + +#### Scenario: Multi-level Mutator propagation preserves order +- **GIVEN** grandparent `Option.Mutators = [A]`, parent `Option.Mutators = [B]`, child `Option.Mutators = [C]` +- **WHEN** grandparent runs the parent which runs the child +- **THEN** the child's effective Mutators (during its own scheduling) is `[A, B, C]` + +--- + +## REMOVED Requirements + +### Requirement: ~~Top-level Workflow.Mutators field~~ + +**Reason:** Moved to `Workflow.Option.Mutators` under the new `WorkflowOption` +grouping. + +### Requirement: ~~MutatorReceiver propagation interface~~ + +**Reason:** Replaced by `WorkflowOptionReceiver.InheritOption`, which carries +`Mutators` as one component of the inherited `WorkflowOption`. diff --git a/openspec/changes/archive/2026-05-12-workflow-option/specs/step-interceptor/spec.md b/openspec/changes/archive/2026-05-12-workflow-option/specs/step-interceptor/spec.md new file mode 100644 index 0000000..d4b92f3 --- /dev/null +++ b/openspec/changes/archive/2026-05-12-workflow-option/specs/step-interceptor/spec.md @@ -0,0 +1,143 @@ +# step-interceptor — Spec Delta + +## MODIFIED Requirements + +### Requirement: Workflow registration of interceptors + +`Workflow` SHALL expose two slice fields for global interceptor registration +under `Workflow.Option` (the new `WorkflowOption` grouping introduced by the +`workflow-options` capability): + +```go +type WorkflowOption struct { + // ... other fields ... + StepInterceptors []StepInterceptor // [0] outermost, [len-1] innermost + AttemptInterceptors []AttemptInterceptor // [0] outermost, [len-1] innermost + DontInherit bool // if true, do not inherit from a parent workflow + // ... other fields ... +} +``` + +Nil/empty slices mean no interceptors. Existing workflows without +interceptors SHALL behave identically to before this feature was added +(zero-value safe, no allocations on the hot path). + +The previous top-level `Workflow.StepInterceptors` / +`Workflow.AttemptInterceptors` / `Workflow.IsolateInterceptors` fields +SHALL NOT exist. `IsolateInterceptors` is replaced by `Option.DontInherit`, +whose semantics extend from interceptor-only to whole-`WorkflowOption` +opt-out. + +#### Scenario: Outer-to-inner ordering +- **WHEN** `Option.StepInterceptors = [A, B]` are registered +- **THEN** the execution order is `A:before → B:before → step → B:after → A:after` + +#### Scenario: No interceptors means no behavioural change +- **WHEN** a Workflow is constructed without `Option.StepInterceptors` or `Option.AttemptInterceptors` +- **THEN** all existing semantics (retries, conditions, BeforeStep/AfterStep) are unchanged + +--- + +### Requirement: Interceptor propagation to nested workflows + +`Workflow` SHALL implement `WorkflowOptionReceiver` (defined in the +`workflow-options` capability spec) so that when a `*Workflow` (or a user +struct embedding `flow.Workflow`) is used as a Step inside another +Workflow, the parent's interceptors are prepended to the child's +interceptor slices via the parent's prologue-pass `InheritOption` call. + +The parent's `Do()` prologue locates the receiver for each root step by +walking `Unwrap()` via `findOptionReceiver` (the same protocol used by +`As[T]` / `Has[T]`) and selecting the first receiver found in pre-order. +This means a sub-workflow MAY be wrapped in any Steper-only wrapper +(notably `flow.Name` / `NamedStep`) without losing inheritance. + +The parent SHALL invoke `restore := recv.InheritOption(parent.Option)` +exactly once per root sub-workflow step before the parent's tick begins, +and SHALL `defer restore()`. Inheritance is **per-run scoped**: + +- The parent's user-supplied `Option.StepInterceptors` / + `Option.AttemptInterceptors` slices SHALL NOT be mutated by + `InheritOption`; the implementation prepends to a fresh slice. +- Each `InheritOption` call snapshots the receiver's `Option` and returns + a `restore func()` that the parent defers, so prepended parent + contributions do not accumulate across repeated `Do()` runs. + +The previous `InterceptorReceiver` interface, `PrependInterceptors` method, +and `inheritedStep` / `inheritedAttempt` side fields SHALL NOT exist. + +#### Scenario: Nested *Workflow inherits parent interceptors +- **GIVEN** a parent Workflow with `Option.StepInterceptors = [X]`, and a child `*Workflow` containing + step `S` added as a step in the parent +- **WHEN** the parent runs +- **THEN** X is invoked for both the child workflow step and the inner step S + +#### Scenario: Embedded Workflow inherits parent interceptors +- **GIVEN** a parent Workflow with `Option.StepInterceptors = [X]`, and a step embedding `flow.Workflow` + containing step `S` +- **WHEN** the parent runs +- **THEN** X is invoked for both the outer step and the inner step S + +#### Scenario: Inheritance survives Steper-only wrappers (NamedStep / flow.Name) +- **GIVEN** a parent Workflow with `Option.StepInterceptors = [X]`, and a child `*Workflow` + containing step `S` that is added to the parent via `flow.Name(child, "name")` +- **WHEN** the parent runs +- **THEN** X is invoked for both the wrapping `NamedStep` and the inner step S +- **AND** inheritance works because the parent looks up `WorkflowOptionReceiver` via + `Unwrap`, not via a direct type assertion on the registered Step + +#### Scenario: InheritOption does not duplicate across retries +- **WHEN** a sub-workflow step is retried N times within one parent run +- **THEN** parent interceptors are prepended to the child's slice exactly once, not N times + +#### Scenario: InheritOption does not accumulate across repeated Do() runs +- **GIVEN** a parent containing a child sub-workflow +- **WHEN** the parent's `Do()` is invoked N times in succession +- **THEN** each invocation results in the parent's interceptors firing exactly once per + step (no compounding across runs) +- **AND** the child's `Option.StepInterceptors` field after each run is its original + pre-inheritance value + +--- + +### Requirement: Opting out of inheritance via DontInherit + +A nested `Workflow` MAY set `Option.DontInherit = true` to opt out of +inheriting any of the parent's `WorkflowOption`, including interceptors. +When true, the `InheritOption` call from the parent SHALL be a no-op and +the workflow runs only with its own configured `Option`. + +This is intended for self-contained sub-workflows that define their own +observability pipeline (e.g., their own tracer or event sink) that must not +be wrapped by parent interceptors, or that more generally want isolation +from the parent's whole `WorkflowOption`. + +The previous `IsolateInterceptors` flag SHALL NOT exist; its semantics are +subsumed and widened by `Option.DontInherit`. + +#### Scenario: DontInherit blocks interceptor inheritance +- **GIVEN** parent `Option.StepInterceptors = [X]`, child `Option.DontInherit = true, StepInterceptors = [Y]` +- **WHEN** parent runs the child +- **THEN** the child runs with only `[Y]` as its effective StepInterceptors + +--- + +## REMOVED Requirements + +### Requirement: ~~InterceptorReceiver interface~~ + +**Reason:** Replaced by `WorkflowOptionReceiver.InheritOption`, which +carries `StepInterceptors` and `AttemptInterceptors` as components of the +inherited `WorkflowOption`. + +### Requirement: ~~IsolateInterceptors as a separate flag~~ + +**Reason:** Replaced by `Option.DontInherit`, which generalises from +interceptor-only opt-out to whole-`WorkflowOption` opt-out. + +### Requirement: ~~inheritedStep / inheritedAttempt side fields~~ + +**Reason:** The accumulation-prevention invariant is now satisfied by the +`restore func()` returned from `InheritOption` and deferred by the parent; +the side fields and their special-cased `reset()` behavior are no longer +needed. diff --git a/openspec/changes/archive/2026-05-12-workflow-option/specs/workflow-options/spec.md b/openspec/changes/archive/2026-05-12-workflow-option/specs/workflow-options/spec.md new file mode 100644 index 0000000..ce62f34 --- /dev/null +++ b/openspec/changes/archive/2026-05-12-workflow-option/specs/workflow-options/spec.md @@ -0,0 +1,357 @@ +# workflow-options — Spec Delta + +## MODIFIED Requirements + +### Requirement: MaxConcurrency limits simultaneous running Steps + +When `Workflow.Option.MaxConcurrency` is non-nil and points to a positive integer +`N`, the Workflow SHALL run at most `N` Steps concurrently. Additional runnable +Steps wait until a running Step terminates before starting. A nil pointer (the +default) or a value of `0` means unlimited concurrency. + +The limit is implemented via a buffered channel used as a lease bucket. + +#### Scenario: MaxConcurrency=2 allows exactly 2 concurrent Steps +- **WHEN** `Option.MaxConcurrency` points to `2` and 3 independent Steps are added +- **THEN** at most 2 Steps run simultaneously; the third starts only after one finishes + +#### Scenario: MaxConcurrency nil imposes no limit +- **WHEN** `Option.MaxConcurrency` is nil (the default) +- **THEN** all runnable Steps start concurrently without any concurrency bound + +--- + +### Requirement: DontPanic converts panics to errors + +When `Workflow.Option.DontPanic` is non-nil and dereferences to `true`, any +`panic` in a Step's `Do`, `Input`, or `BeforeStep`/`AfterStep` callbacks is +recovered and returned as a `ErrPanic`-wrapped error, setting the Step to +`Failed`. Stack trace information is captured and included in the error. + +When `Option.DontPanic` is nil (the default) or dereferences to `false`, +panics propagate normally and crash the process. + +#### Scenario: Panic converted to Failed with DontPanic=true +- **WHEN** `Option.DontPanic` points to `true` and a Step panics during `Do` +- **THEN** the Step status is `Failed`; the returned `ErrWorkflow` entry wraps the panic + value as an `ErrPanic` + +#### Scenario: Panic propagates with DontPanic nil +- **WHEN** `Option.DontPanic` is nil and a Step panics +- **THEN** the panic propagates out of the goroutine (process crash or test failure) + +--- + +### Requirement: SkipAsError controls whether Skipped counts as failure + +When `Workflow.Option.SkipAsError` is nil or dereferences to `false` (the +default), Steps that are `Skipped` are considered acceptable outcomes. +`workflow.Do` returns `nil` if all root Steps are `Succeeded` or `Skipped`. + +When `Option.SkipAsError` dereferences to `true`, any `Skipped` Step causes +`workflow.Do` to return an `ErrWorkflow` even if no Step actually failed. + +#### Scenario: Skipped is acceptable by default +- **WHEN** `Option.SkipAsError` is nil and all root Steps are either `Succeeded` or `Skipped` +- **THEN** `workflow.Do` returns `nil` + +#### Scenario: Skipped counts as error when SkipAsError=true +- **WHEN** `Option.SkipAsError` points to `true` and at least one root Step is `Skipped` +- **THEN** `workflow.Do` returns an `ErrWorkflow` containing the skipped Step + +--- + +### Requirement: StepDefaults applies a baseline StepOption to all Steps + +`Workflow.Option.StepDefaults` is a `*StepOption` that the Workflow +prepends to the Option slice of every Step added to it. This lets callers set +a universal default for all Steps (e.g., a global timeout) without modifying +each Step individually. + +Step-level options that are set after the default take precedence because the +Option slice is evaluated left-to-right and later values overwrite earlier +ones on the same `StepOption` struct. + +The renaming from `DefaultOption` to `StepDefaults` clarifies that the +field configures step-level options, not workflow-level options +(`WorkflowOption`). + +#### Scenario: StepDefaults sets a global timeout +- **WHEN** `Option.StepDefaults` has `Timeout` set to 10 minutes + and a Step is added without its own `Timeout` +- **THEN** the effective timeout for that Step is 10 minutes + +#### Scenario: Step-level option overrides the default +- **WHEN** `Option.StepDefaults` has `Timeout` of 10 minutes and a Step declares + `.Timeout(5 * time.Minute)` +- **THEN** the effective timeout for that Step is 5 minutes + +--- + +### Requirement: Clock enables time injection for testing + +`Workflow.Option.Clock` is a `clock.Clock` interface (from +`github.com/benbjohnson/clock`). The Workflow uses the Clock for all +time-related operations: Step-level timeouts, per-try timeouts in the retry +loop, and backoff waits. + +When `Option.Clock` is `nil`, the Workflow uses the real wall clock +(`clock.New()`). Providing a mock clock allows unit tests to control time +without real delays. + +#### Scenario: Nil Clock uses wall clock +- **WHEN** `Option.Clock` is not set +- **THEN** the Workflow automatically uses `clock.New()` for all time operations + +#### Scenario: Mock clock controls timeout behavior in tests +- **WHEN** a `clock.Mock` is injected as `Option.Clock` + and the mock is advanced past a Step's `Timeout` duration +- **THEN** the Step's context is canceled and the Step is set to `Canceled` + +--- + +## ADDED Requirements + +### Requirement: WorkflowOption groups workflow-level configuration + +`flow.Workflow` SHALL expose all configuration fields under a single named +field `Option WorkflowOption`. The previous nine top-level configuration +fields (`MaxConcurrency`, `DontPanic`, `SkipAsError`, `Clock`, +`DefaultOption`, `Mutators`, `StepInterceptors`, `AttemptInterceptors`, +`IsolateInterceptors`) SHALL NOT exist as direct fields on `Workflow`. + +`WorkflowOption` SHALL declare: + +```go +type WorkflowOption struct { + MaxConcurrency *int + DontPanic *bool + SkipAsError *bool + Clock clock.Clock + StepDefaults *StepOption + + Mutators []Mutator + StepInterceptors []StepInterceptor + AttemptInterceptors []AttemptInterceptor + + DontInherit bool +} +``` + +Scalar configuration fields are pointer-typed so that "unset" (nil pointer) +and "explicit zero value" are distinguishable. This distinction is required +for parent → child Option inheritance: a nil pointer on the child means +"inherit from parent"; a non-nil pointer means "child wins". + +#### Scenario: Workflow exposes Option field +- **WHEN** a user constructs `&flow.Workflow{Option: flow.WorkflowOption{...}}` +- **THEN** the Workflow accepts the configuration and applies it + +#### Scenario: Explicit zero is distinguishable from unset +- **GIVEN** a parent with `Option.MaxConcurrency = &four` (where `four = 4`) +- **AND** a child with `Option.MaxConcurrency = &zero` (where `zero = 0`) +- **WHEN** parent runs the child as a sub-workflow +- **THEN** the child observes `MaxConcurrency = 0` (unlimited), NOT inherited 4 + +--- + +### Requirement: WorkflowOptionReceiver propagates Option to sub-workflows + +`flow.Workflow` SHALL implement `WorkflowOptionReceiver`: + +```go +type WorkflowOptionReceiver interface { + // InheritOption merges the parent's WorkflowOption into the receiver + // and returns a restore func that the parent MUST defer to rewind the + // receiver's Option to its pre-inheritance state. + InheritOption(parent WorkflowOption) (restore func()) +} +``` + +The parent Workflow SHALL invoke `restore := child.InheritOption(parent.Option)` +exactly once per sub-workflow root step, in the parent's `Do()` prologue +(after `init()`, before the scheduling tick begins), and SHALL `defer +restore()` so the receiver's Option is rewound on every exit path. The +parent SHALL locate the receiver by walking each root step's `Unwrap()` +chain via `findOptionReceiver`, returning the first `WorkflowOptionReceiver` +found in pre-order. This means a sub-workflow MAY be wrapped in any +Steper-only wrapper (notably `flow.Name` / `NamedStep`) without losing +inheritance. + +`Workflow.InheritOption` SHALL apply the following merge rules: + +1. If `w.Option.DontInherit` is `true`, the merge SHALL be a no-op; a + (possibly trivial) restore func SHALL still be returned so the parent's + `defer restore()` is uniformly safe. +2. For each scalar pointer field (`MaxConcurrency`, `DontPanic`, + `SkipAsError`) and each interface/pointer field (`Clock`, + `StepDefaults`): if the child's field is nil, set it to the parent's + value. Non-nil child fields SHALL NOT be modified. +3. For each slice field (`Mutators`, `StepInterceptors`, + `AttemptInterceptors`): allocate a fresh slice equal to + `parent_slice ++ child_slice` and assign it to the child's field. The + parent's and child's input slices SHALL NOT be mutated. + +The parent's user-supplied `WorkflowOption` SHALL NOT be mutated by +`InheritOption`. + +#### Scenario: Scalar nil inherits parent's value +- **GIVEN** parent `Option.DontPanic = &true`, child `Option.DontPanic = nil` +- **WHEN** parent invokes `restore := child.InheritOption(parent.Option)` +- **THEN** child observes `DontPanic = true` for the duration of the run +- **AND** after `restore()` runs, the child's `Option.DontPanic` is back to nil + +#### Scenario: Scalar non-nil child wins +- **GIVEN** parent `Option.MaxConcurrency = &four`, child `Option.MaxConcurrency = &eight` +- **WHEN** parent invokes `child.InheritOption(parent.Option)` +- **THEN** child observes `MaxConcurrency = 8` + +#### Scenario: Slices are parent-prepended +- **GIVEN** parent `Option.Mutators = [A]`, child `Option.Mutators = [B]` +- **WHEN** parent invokes `child.InheritOption(parent.Option)` +- **THEN** child's effective `Mutators` is `[A, B]` for the duration of the run + +#### Scenario: Multi-level nesting prepends in order +- **GIVEN** grandparent `Option.Mutators = [A]`, parent `Option.Mutators = [B]`, child `Option.Mutators = [C]` +- **WHEN** grandparent runs, propagating to parent then to child +- **THEN** child's effective `Mutators` is `[A, B, C]` + +#### Scenario: Inheritance survives Steper-only wrappers +- **GIVEN** a parent with a `Mutator` registered, and a child `*Workflow` added to the parent via `flow.Name(child, "name")` +- **WHEN** the parent runs +- **THEN** the parent's `Mutator` reaches the inner steps via `InheritOption` propagation through `Unwrap` + +--- + +### Requirement: DontInherit opts out of all parent Option inheritance + +When a sub-workflow's `Option.DontInherit` is `true`, the parent's +`InheritOption(parent.Option)` call SHALL be a no-op. The sub-workflow runs +with exactly its own configured Option, with no scalars filled in from the +parent and no slices prepended. + +This replaces the previous `IsolateInterceptors` flag and widens its +semantics from interceptor-only to whole-Option opt-out. The naming aligns +with `DontPanic`. + +#### Scenario: DontInherit blocks scalar inheritance +- **GIVEN** parent `Option.DontPanic = &true`, child `Option.DontInherit = true, DontPanic = nil` +- **WHEN** parent invokes `child.InheritOption(parent.Option)` +- **THEN** child observes `DontPanic = false` (nil dereferenced as default) + +#### Scenario: DontInherit blocks slice prepending +- **GIVEN** parent `Option.Mutators = [A]`, child `Option.DontInherit = true, Mutators = [B]` +- **WHEN** parent invokes `child.InheritOption(parent.Option)` +- **THEN** child's effective `Mutators` is `[B]`, not `[A, B]` + +--- + +### Requirement: InheritOption restore prevents cross-run accumulation + +The `restore func()` returned by `InheritOption` SHALL rewind the receiver's +`Option` to its pre-inheritance value (a snapshot captured at the start of +`InheritOption`). The parent SHALL `defer restore()` for every receiver it +called `InheritOption` on, ensuring that on every exit path of the parent's +`Do()` (success, error, or panic) the children's Option fields are reset. + +This guarantees that `InheritOption` writes performed by a parent during +one `Do()` run do not accumulate into subsequent runs of the same parent +or of the same child reused under a different parent. + +The snapshot captured inside `InheritOption` is a shallow copy. This is +correct because: + +- Pointer overwrites on nil scalar fields point to the parent's existing + pointer values without mutating them; restoring the snapshot resets + the pointer back to nil. +- Slice fields are always written via fresh `make`-and-append in + `InheritOption`; the snapshot's slice header still references the + pre-inheritance backing array, which is what restore reinstates. + +The internal `reset()` SHALL NOT clear `w.Option` (that is the restore +func's job, deferred at the parent scope, and reset runs at the top of +`Do()` before scheduling). + +The public `Workflow.Reset()` SHALL NOT clear `w.Option` either, because +the restore mechanism in the parent's `Do()` already prevents accumulation; +`Reset()` exists solely to reset per-step status (see the `Reset` requirement). + +#### Scenario: Repeated Do() runs do not accumulate inherited contributions +- **GIVEN** a parent with `Option.Mutators = [A]` and a child with `Option.Mutators = [B]` +- **WHEN** `parent.Do()` is invoked N times +- **THEN** each invocation results in the child's effective `Mutators` being `[A, B]` during the run +- **AND** the child's `Option.Mutators` field is `[B]` after each run completes + +#### Scenario: Restore covers all exit paths +- **WHEN** `Do()` returns successfully, returns an error, or panics (when not recovered) +- **THEN** every `restore()` deferred by the parent fires; each receiver's `Option` is restored to its pre-`InheritOption` state + +--- + +### Requirement: Reset rewinds per-step status without touching the step set + +`Workflow.Reset()` SHALL set every Step's status from any terminal state +(`Succeeded`, `Failed`, `Skipped`, `Canceled`) back to `Pending`, allowing +`Do()` to be invoked again on the same Workflow. `Reset()` SHALL reject +with `ErrWorkflowIsRunning` if a `Do()` call is currently in flight. + +`Reset()` SHALL NOT modify `w.steps` (the set of Steps registered via +`Add`). This is a contract: a Workflow built once via `Add` can be +`Do()`-ed any number of times via `Reset/Do` cycles, with the same DAG +each time. To start from an empty set of Steps, allocate a new `Workflow`. + +`Reset()` SHALL NOT modify `w.Option`. Cross-run accumulation of +parent-inherited contributions is prevented by the `restore func()` returned +from `InheritOption` (see the preceding requirement), not by `Reset()`. +Calling `Reset()` between runs is therefore optional from an Option-isolation +standpoint; its purpose is purely to rewind per-step status for re-execution. + +#### Scenario: Reset rewinds status but preserves the DAG +- **GIVEN** a Workflow with steps `[a, b, c]` that has been `Do()`-ed once +- **WHEN** `w.Reset()` is called and then `w.Do()` is called again +- **THEN** all three steps execute a second time +- **AND** `w.steps` still contains `[a, b, c]` + +#### Scenario: Reset rejected while a Do is in flight +- **WHEN** `Reset()` is called while `Do()` is executing on the same Workflow +- **THEN** `Reset()` returns `ErrWorkflowIsRunning` without modifying state + +#### Scenario: Reset is not required to prevent Option accumulation +- **GIVEN** a parent with `Option.Mutators = [A]` and a child with `Option.Mutators = [B]` +- **WHEN** `parent.Do()` is invoked N times in succession WITHOUT calling + `parent.Reset()` between runs (the parent has no terminal-status steps + to reset because each Do() resets internally via the unexported `reset()`) +- **THEN** each invocation results in the child's effective `Mutators` + being `[A, B]` during the run, with no accumulation + +--- + +## REMOVED Requirements + +### Requirement: ~~Workflow exposes Mutators / StepInterceptors / AttemptInterceptors as top-level fields~~ + +**Reason:** Replaced by `Workflow.Option.Mutators`, `Workflow.Option.StepInterceptors`, +`Workflow.Option.AttemptInterceptors` under the new `WorkflowOption` grouping. + +### Requirement: ~~MutatorReceiver interface~~ + +**Reason:** Replaced by `WorkflowOptionReceiver.InheritOption`, which carries +`Mutators` as one component of the inherited `WorkflowOption`. + +### Requirement: ~~InterceptorReceiver interface~~ + +**Reason:** Replaced by `WorkflowOptionReceiver.InheritOption`, which carries +`StepInterceptors` and `AttemptInterceptors` as components of the inherited +`WorkflowOption`. + +### Requirement: ~~IsolateInterceptors~~ + +**Reason:** Replaced by `Option.DontInherit`, which generalises from +interceptor-only opt-out to whole-`WorkflowOption` opt-out. + +### Requirement: ~~inheritedStep / inheritedAttempt side fields~~ + +**Reason:** The accumulation-prevention invariant is now satisfied by the +`restore func()` returned from `InheritOption` and deferred by the parent; +the side fields and their special-cased `reset()` behavior are no longer +needed. diff --git a/openspec/changes/archive/2026-05-12-workflow-option/tasks.md b/openspec/changes/archive/2026-05-12-workflow-option/tasks.md new file mode 100644 index 0000000..c260066 --- /dev/null +++ b/openspec/changes/archive/2026-05-12-workflow-option/tasks.md @@ -0,0 +1,108 @@ +# Tasks + +## 1. New Types + +- [x] 1.1 Create `workflow_option.go` with `WorkflowOption` struct (pointer scalars, slice fields, `DontInherit bool`) +- [x] 1.2 Add `WorkflowOptionReceiver` interface with `InheritOption(parent WorkflowOption)` +- [x] 1.3 Implement `prependSlice[T any](parent, child []T) []T` helper that always allocates a fresh backing array (godoc: "MUST NOT mutate either input") + +## 2. Workflow Refactor + +- [x] 2.1 In `workflow.go`, replace the nine top-level fields (`MaxConcurrency`, `DontPanic`, `SkipAsError`, `Clock`, `DefaultOption`, `Mutators`, `StepInterceptors`, `AttemptInterceptors`, `IsolateInterceptors`) with a single `Option WorkflowOption` field +- [x] 2.2 Remove `inheritedStep` / `inheritedAttempt` side fields from `Workflow` +- [x] 2.3 Add `(*Workflow).InheritOption(parent WorkflowOption)` implementing the merge rules (nil scalar → parent; non-nil scalar → child; slices → parent prepended; `DontInherit=true` → no-op) +- [x] 2.4 Add unexported scalar accessor helpers: `maxConcurrency()`, `dontPanic()`, `skipAsError()`, `clock()`. `Option.StepDefaults` is read directly (already a pointer, nil-safe at call sites). +- [x] 2.5 Rewrite all in-repo reads of the former fields to call the helpers (`workflow.go`, plus any other file that touches these — verify with `grep`) + +## 3. Propagation Rewrite + +- [x] 3.1 Add `findOptionReceiver(s Steper) WorkflowOptionReceiver` mirroring the removed `findInterceptorReceiver` +- [x] 3.2 In `Workflow.Do()`, after `init()` and before `tick`, walk root steps and call `recv.InheritOption(w.Option)` once per receiver +- [x] 3.3 In `Workflow.Do()`, snapshot `w.Option` immediately after acquiring `isRunning` lock and `defer` restore +- [x] 3.4 Remove the Mutator dispatch site that called `recv.PrependMutators` (currently around `workflow.go:641`) +- [x] 3.5 Remove the Interceptor dispatch site that called `findInterceptorReceiver` / `recv.PrependInterceptors` (currently around `workflow.go:768`) +- [x] 3.6 Simplify `effectiveStepInterceptors` / `effectiveAttemptInterceptors` to return `w.Option.StepInterceptors` / `w.Option.AttemptInterceptors` directly (parent contributions are already merged in by `InheritOption`) + +## 4. Removals + +- [x] 4.1 Delete `MutatorReceiver` interface from `mutator.go` +- [x] 4.2 Delete `InterceptorReceiver` interface and `findInterceptorReceiver` from `interceptor.go` +- [x] 4.3 Delete `(*Workflow).PrependMutators` method +- [x] 4.4 Delete `(*Workflow).PrependInterceptors` method +- [x] 4.5 Delete `(*SubWorkflow).PrependMutators` and `(*SubWorkflow).PrependInterceptors` methods + +## 5. SubWorkflow Deprecation + +- [x] 5.1 Add `// Deprecated: Embed flow.Workflow directly. SubWorkflow will be removed in the next major version of go-workflow.` to the `SubWorkflow` type godoc +- [x] 5.2 Add `(*SubWorkflow).InheritOption(parent WorkflowOption)` delegating to `s.w.InheritOption(parent)` so `SubWorkflow`-embedding steps continue to participate in propagation during the deprecation window +- [x] 5.3 Verify `SubWorkflow.Reset` retains its existing deprecation notice (no change needed if already correct) +- [x] 5.4 Add `// Deprecated:` notice at the `StepBuilder` *type* godoc level (the method `StepBuilder.BuildStep` is already deprecated). The notice SHALL state that the type, the `BuildStep()` user hook, and `SubWorkflow` will all be removed together in the next major version. Behavior is unchanged this release — `Workflow` still embeds `StepBuilder`, `Workflow.Add` still calls `w.BuildStep(step)`, user `BuildStep()` hooks still fire once. + +## 6. Spec Updates + +- [x] 6.1 Apply MODIFIED Requirements from `changes/2026-05-12-workflow-option/specs/workflow-options/spec.md` to `openspec/specs/workflow-options/spec.md` +- [x] 6.2 Apply MODIFIED Requirements from `changes/2026-05-12-workflow-option/specs/composite-steps/spec.md` to `openspec/specs/composite-steps/spec.md`: + - Mark `SubWorkflow — Workflow as a Step` as deprecated + - Remove `SubWorkflow implements MutatorReceiver` requirement + - Remove `*Workflow implements MutatorReceiver` requirement + - Add `*Workflow implements WorkflowOptionReceiver` requirement (recommended sub-workflow pattern via direct embedding) +- [x] 6.3 Apply MODIFIED Requirements from `changes/2026-05-12-workflow-option/specs/mutators/spec.md` to `openspec/specs/mutators/spec.md`: + - Update field reference `Workflow.Mutators` → `Workflow.Option.Mutators` + - Replace `PrependMutators` propagation references with `InheritOption(parent.Option)` references +- [x] 6.4 Apply MODIFIED Requirements from `changes/2026-05-12-workflow-option/specs/step-interceptor/spec.md` to `openspec/specs/step-interceptor/spec.md`: + - Update field references `Workflow.StepInterceptors` / `AttemptInterceptors` → `Workflow.Option.StepInterceptors` / `AttemptInterceptors` + - Remove `InterceptorReceiver` requirement + - Remove `IsolateInterceptors` requirement; add reference to `Option.DontInherit` + - Update propagation requirement to use `WorkflowOptionReceiver.InheritOption` + +## 7. Tests + +- [x] 7.1 Migrate `workflow_options_test.go`: every `&Workflow{ Field: ... }` literal rewritten to `Option: WorkflowOption{ ... }`; scalars wrapped as pointers; assertions unchanged +- [x] 7.2 Migrate `workflow_mutator_test.go`: `Workflow.Mutators` references → `Option.Mutators`; tests of `PrependMutators` rewritten to exercise `InheritOption` +- [x] 7.3 Migrate any other test file that references the nine former fields or the removed receiver interfaces +- [x] 7.4 Create `workflow_option_inherit_test.go` with table-driven matrix: + - Scalar nil → parent's value used + - Scalar non-nil → child wins + - Scalar explicit zero (`*int = 0`) → child wins, distinguished from inherit + - Each of `Mutators` / `StepInterceptors` / `AttemptInterceptors` → parent prepended + - `Clock` nil → parent's clock used + - `StepDefaults` nil → parent's value used + - `DontInherit = true` → no field changes regardless of parent +- [x] 7.5 Test: multi-level nesting (grandparent → parent → child each with one Mutator) yields effective `[grandparent, parent, child]` Mutators on the child +- [x] 7.6 Test: `Do()` snapshot/restore — `parent.Do()` runs twice; child's effective Mutators is `[A, B]` during each run and `[B]` after each run, no accumulation +- [x] 7.7 Test: `SubWorkflow` deprecation smoke — embedded `SubWorkflow` still receives parent's Option via delegated `InheritOption` +- [x] 7.8 Test: scalar inheritance covers the motivating use case — parent with `DontPanic=true`, child without setting it; verify child's executed code path also recovers panics +- [x] 7.9 Test: `Workflow{}` zero value (no Option set) runs identically to before for a single-workflow case (no panic, no overhead) + +## 8. Examples + +- [x] 8.1 Update `example/13_mutators_test.go` example that references `flow.SubWorkflow` to embed `flow.Workflow` directly and use `Option` +- [x] 8.2 Update any other example file referencing the nine former fields or `flow.SubWorkflow` +- [x] 8.3 Add example demonstrating scalar inheritance: parent sets `DontPanic`, child inherits it without restating + +## 9. Documentation + +- [x] 9.1 Update `Workflow` godoc: remove field-by-field documentation of the nine former fields; add reference to `Option` and `WorkflowOption` +- [x] 9.2 Add `WorkflowOption` and `WorkflowOptionReceiver` godoc with merge rules and snapshot/restore note +- [x] 9.3 Update README sub-workflow section if any to recommend embedding `flow.Workflow` directly; note `SubWorkflow` deprecation +- [x] 9.4 Add CHANGELOG entry summarizing the breaking change, migration steps, and the scalar-inheritance behavior change + - **N/A:** repo does not maintain a CHANGELOG file; release notes captured in the OpenSpec change folder instead. +- [x] 9.5 Rewrite `(*Workflow).Reset()` godoc: drop the obsolete `inheritedStep` / `inheritedAttempt` paragraph; state the new contract — Reset rewinds per-step status only, never modifies `w.steps` or `w.Option`; note that calling Reset between Do() runs is optional from an Option-isolation standpoint (the snapshot/restore in Do() already prevents accumulation) +- [x] 9.6 In the godoc for `flow.Workflow` (or a dedicated example), call out the two sub-workflow patterns and their tradeoffs: + - Embed `flow.Workflow` and call `Add` at construction time (recommended; supports introspection and parent Option inheritance) + - Construct `&flow.Workflow{}` inline inside `Do()` (opaque to parent unless the host step also implements `WorkflowOptionReceiver` to forward parent Option) + - Lazy build guarded by `sync.Once` (acceptable for single-construction, multi-execution lifecycle on the same composite step) +- [x] 9.7 Add a strong warning to `(*Workflow).Add` godoc: calling `Add` from inside the same Workflow's `Do()` (or any method transitively reachable from `Do()`) is **forbidden** unless guarded by `sync.Once`. State the failure modes (callback accumulation, duplicate steps, multi-match introspection, stale Mutator dispatch) explicitly. Reference the `composite-steps` "Composite step MUST NOT call Add inside Do unguarded" scenario. + +## 11. Future Work (Out of Scope) + +- [ ] 11.1 (future) Static analysis check (`go vet` analyzer or standalone linter) that flags `flow.Workflow.Add` called transitively from a `Do()` method without a `sync.Once` guard. The framework SHALL NOT add a runtime panic for this misuse — neither the parent's `isRunning` lock nor `x.isRunning` (acquired/released across retry attempts) offers a stable signal for distinguishing legitimate `sync.Once`-guarded lazy init from unguarded re-`Add`. A separate lint tool is the right layer. + +## 10. Verify + +- [x] 10.1 `go build ./...` — no compile errors +- [x] 10.2 `go test ./...` — all migrated and new tests pass +- [x] 10.3 `go vet ./...` — no issues +- [x] 10.4 `grep -rn "PrependMutators\|PrependInterceptors\|MutatorReceiver\|InterceptorReceiver\|IsolateInterceptors\|inheritedStep\|inheritedAttempt\|findInterceptorReceiver" --include="*.go"` returns only expected residual hits (none in source; possibly historical refs in archived openspec) +- [x] 10.5 `grep -rn "DefaultOption\|w\.MaxConcurrency\|w\.DontPanic\|w\.SkipAsError\|w\.Clock\|w\.Mutators\|w\.StepInterceptors\|w\.AttemptInterceptors" --include="*.go" | grep -v _test.go` — no source-side direct reads of the removed fields +- [x] 10.6 Confirm `SubWorkflow`-embedding example compiles, runs, and inherits parent Option correctly diff --git a/openspec/specs/composite-steps/spec.md b/openspec/specs/composite-steps/spec.md index 4896d68..86f5f05 100644 --- a/openspec/specs/composite-steps/spec.md +++ b/openspec/specs/composite-steps/spec.md @@ -122,11 +122,25 @@ If the Step also implements `Reset()`, the Workflow SHALL call `Reset()` before ### Requirement: SubWorkflow — Workflow as a Step -`SubWorkflow` is an embeddable helper that exposes a nested `Workflow` as a Step. -Embedding `flow.SubWorkflow` in a struct gives it `Do`, `Add`, `Unwrap`, and `Reset` methods. -The outer Workflow orchestrates the composite Step; the inner Workflow orchestrates the -sub-steps. The outer Workflow can reach into the inner Steps using `Has`, `As`, and -`HasStep`. +**Deprecated:** New code SHOULD embed `flow.Workflow` directly instead. The +`flow.SubWorkflow` type is preserved for one release window to allow +in-flight users to migrate; it SHALL be removed in the next major version +of `go-workflow`. + +`SubWorkflow` is an embeddable helper that exposes a nested `Workflow` as a +Step. Embedding `flow.SubWorkflow` in a struct gives it `Do`, `Add`, and +`Unwrap` methods. The outer Workflow orchestrates the composite Step; the +inner Workflow orchestrates the sub-steps. The outer Workflow can reach into +the inner Steps using `Has`, `As`, and `HasStep`. + +During the deprecation window, `SubWorkflow` SHALL implement +`WorkflowOptionReceiver.InheritOption` by delegating to the embedded +`Workflow`, so parent → child Option propagation continues to work for +`SubWorkflow`-embedding steps. + +`SubWorkflow.Reset` remains deprecated (per the existing Mutator-era +deprecation) and SHALL be removed together with `SubWorkflow` itself in the +next major version. #### Scenario: Workflow can be used directly as a Step - **WHEN** a `*flow.Workflow` is added as a Step inside another Workflow @@ -138,6 +152,12 @@ sub-steps. The outer Workflow can reach into the inner Steps using `Has`, `As`, with inner Steps - **THEN** `flow.As[*InnerStepType](outerWorkflow)` returns the inner Step instances +#### Scenario: SubWorkflow inherits parent Option during the deprecation window +- **WHEN** an outer Workflow with `Option.DontPanic = &true` contains a + composite step that embeds `flow.SubWorkflow` +- **THEN** the inner workflow observes `DontPanic = true` via the delegated + `InheritOption` + --- ### Requirement: Cross-boundary upstream registration @@ -203,57 +223,130 @@ The framework SHALL support constructing a composite Step's sub-workflow inside `Do(ctx)` method, using a freshly created `flow.Workflow{}` and standard `Add` / `Step` / `Pipe` builders. This pattern MUST NOT require implementing `BuildStep`. -When this pattern is used in combination with `flow.SubWorkflow` embedding, the inner -workflow SHALL remain visible to parent-level Mutators via the `MutatorReceiver` -propagation mechanism (see the `mutators` capability spec). - -When this pattern is used WITHOUT `flow.SubWorkflow` embedding (e.g. by constructing a -local `flow.Workflow{}` inside `Do` and discarding it after `Do` returns), the inner -workflow SHALL be invisible to parent introspection helpers (`Has`, `As`, `HasStep`) and -to parent Mutator propagation. This is intentional: such a step is fully self-contained -and opaque to the outer workflow. - -#### Scenario: Composite step constructs sub-workflow in Do -- **WHEN** a composite step embeds `flow.SubWorkflow` and inside `Do` calls `s.Reset()` - followed by `s.Add(...)` then `s.Do(ctx)` -- **THEN** the sub-workflow runs correctly and parent Mutators reach its inner steps via - `PrependMutators` (invoked once per inner-workflow execution; once-per-step semantics - inside the inner workflow then take over) +The recommended pattern is to embed `flow.Workflow` directly in the +composite step's struct: + +```go +type Deploy struct { + flow.Workflow + Region string +} + +func (d *Deploy) Do(ctx context.Context) error { + d.Add(/* ... */) + return d.Workflow.Do(ctx) +} +``` + +When this pattern is used, the inner workflow SHALL remain visible to +parent-level Mutators and Interceptors via the +`WorkflowOptionReceiver.InheritOption` propagation mechanism (see the +`workflow-options` capability spec). + +When this pattern is used WITHOUT exposing the inner workflow via embedding +or `Unwrap` (e.g. by constructing a local `flow.Workflow{}` inside `Do` and +discarding it after `Do` returns), the inner workflow SHALL be invisible to +parent introspection helpers (`Has`, `As`, `HasStep`) and to parent Option +propagation. This is intentional: such a step is fully self-contained and +opaque to the outer workflow. + +#### Scenario: Composite step embeds Workflow and inherits parent Option +- **WHEN** a composite step embeds `flow.Workflow` and the parent has Mutators registered +- **THEN** the parent's Mutators reach the inner steps via `InheritOption` (invoked once + per inner-workflow execution; once-per-step semantics inside the inner workflow then + take over) #### Scenario: Local sub-workflow inside Do is opaque to parent -- **WHEN** a step constructs a `flow.Workflow{}` inside `Do` without embedding - `SubWorkflow` and without exposing it via `Unwrap` -- **THEN** parent `flow.As[T](outer)` does not find inner steps and parent Mutators do - not reach them - ---- - -### Requirement: SubWorkflow implements MutatorReceiver - -`flow.SubWorkflow` SHALL implement the `MutatorReceiver` interface defined in the -`mutators` capability spec. Its `PrependMutators` implementation prepends the supplied -Mutators to the inner workflow's `Mutators` slice on each invocation. - -This requirement is what makes the deprecated `BuildStep` + `SubWorkflow` pattern and the -new `Do`-time + `SubWorkflow` pattern interchangeable from the parent Mutator -perspective. - -#### Scenario: Parent Mutator propagates to SubWorkflow -- **WHEN** a parent workflow has `flow.Mutate[*Foo]` registered and a step embedding - `flow.SubWorkflow` adds a `*Foo` to its inner workflow -- **THEN** the parent Mutator runs against the inner `*Foo` instance before each of its - attempts +- **WHEN** a step constructs a `flow.Workflow{}` inside `Do` without embedding it + in the step's struct and without exposing it via `Unwrap` +- **THEN** parent `flow.As[T](outer)` does not find inner steps and parent Option + does not propagate to them + +#### Scenario: Composite step embedding flow.Workflow can be Do()-ed multiple times +- **GIVEN** `type X struct { flow.Workflow }` whose constructor calls `x.Add(...)` once at construction time +- **WHEN** the parent runs N times (with `parent.Reset()` between runs) +- **THEN** each run executes the inner DAG once with no accumulation, because + `Workflow.Add` is idempotent on the step pointer key (the same step pointer + added twice merges its `StepConfig` into the existing entry rather than + creating a duplicate scheduling unit) + +#### Scenario: Composite step MUST NOT call Add inside Do unguarded +- **GIVEN** `type X struct { flow.Workflow }` whose `Do(ctx)` calls `x.Add(...)` + unconditionally on every invocation (no `sync.Once`, no fresh-Workflow + inline construction) +- **WHEN** the parent invokes `x.Do()` more than once on the same `X` instance + (across separate parent runs OR via the retry loop within a single parent run) +- **THEN** the behavior is **undefined**. The framework MAY exhibit any of: + - per-step `BeforeStep`/`AfterStep` callbacks accumulated across runs + (each callback firing N times on the N-th invocation), because + `StepConfig.Merge` appends callback chains and does not deduplicate + - duplicate Step entries of the same logical type in `x.steps` if + `Add` is called with newly-allocated step pointers each `Do()` + - parent introspection (`flow.As`, `Has`) returning multiple matches when + only one was expected + - Mutator dispatch firing against stale step instances retained from + previous invocations +- **REASON** `Workflow.steps` and `StepConfig` are designed for build-once / + execute-many. The pointer-key idempotence of `Add` prevents duplicate + scheduling on identical pointers but does not deduplicate appended + `Before`/`After` callback chains. The framework SHALL NOT add a runtime + guard for this misuse: the parent's `isRunning` lock is held at the + outer scope, not inside `x`'s scope, and `x.isRunning` is acquired and + released across each retry attempt — neither offers a stable signal for + distinguishing legitimate `sync.Once`-guarded lazy initialisation from + unguarded re-`Add` per invocation. +- **REQUIRED PATTERNS** instead — choose exactly one: + 1. **Build at construction time** — the constructor (e.g., `NewX(...)`) + calls `x.Add(...)` before returning. Recommended when the inner DAG + is known at construction. + 2. **Construct inline inside Do() with a fresh *flow.Workflow** — do NOT + embed `flow.Workflow` in `X`; instead, allocate `w := &flow.Workflow{}` + inside `x.Do`, populate via `w.Add(...)`, and call `w.Do(ctx)`. The + containing step MUST implement `WorkflowOptionReceiver` to forward + parent Option (see the next scenario). Recommended when the inner + DAG depends on runtime state in `ctx`. + 3. **Lazy build guarded by sync.Once** — embed `flow.Workflow` and call + `x.Add(...)` from inside `x.once.Do(...)` so the build happens + exactly once across all invocations. Acceptable when the user + explicitly wants a single-construction, multi-execution lifecycle + on the same `X`. + +#### Scenario: Inline sub-workflow inherits parent Option only via explicit InheritOption +- **GIVEN** `type Y struct { inheritedOpt flow.WorkflowOption }` with + `func (y *Y) InheritOption(p flow.WorkflowOption) (restore func()) { prev := y.inheritedOpt; y.inheritedOpt = p; return func() { y.inheritedOpt = prev } }` + and a `Do(ctx)` that constructs `w := &flow.Workflow{Option: y.inheritedOpt}` + then calls `w.Add(...)` and `w.Do(ctx)` +- **WHEN** the parent runs and propagates Option via `findOptionReceiver` +- **THEN** `findOptionReceiver` discovers `Y` (because `Y` directly implements + `WorkflowOptionReceiver`); `y.InheritOption` records the parent's Option; + the inline `*flow.Workflow` constructed inside `y.Do` then inherits it +- **WITHOUT** the explicit `InheritOption` method on `Y`, the parent's Option + does NOT reach the inline sub-workflow (the inline workflow is opaque to + the parent per the preceding scenario) --- -### Requirement: *Workflow implements MutatorReceiver - -`*flow.Workflow`, when used directly as a Step in another workflow, SHALL implement the -`MutatorReceiver` interface. Its `PrependMutators` implementation prepends the supplied -Mutators to its own `Mutators` slice. - -#### Scenario: Nested *Workflow inherits parent Mutators -- **WHEN** a `*flow.Workflow` containing `*Foo` is added as a step in another workflow - that has `flow.Mutate[*Foo]` registered -- **THEN** the parent Mutator runs against the inner `*Foo` before each of its attempts - +### Requirement: *Workflow implements WorkflowOptionReceiver + +`*flow.Workflow`, when used directly as a Step in another workflow (whether +added directly or embedded in a user struct), SHALL implement +`WorkflowOptionReceiver` (defined in the `workflow-options` capability spec). +Its `InheritOption` implementation merges the parent's `WorkflowOption` +into its own per the rules in `workflow-options`, and returns a `restore func()` +that the parent defers to rewind the receiver's `Option` to its +pre-inheritance state. + +This is the recommended pattern for sub-workflows: a user struct embeds +`flow.Workflow` directly and gets `Add`, `Do`, `Unwrap`, and Option +inheritance for free, with only one configuration name (`Option`) promoted +onto the user struct. + +#### Scenario: Embedded *Workflow inherits parent Option +- **GIVEN** `type Deploy struct { flow.Workflow; Region string }` and a parent with `Option.DontPanic = &true` +- **WHEN** the parent runs, with `&Deploy{Region: "westus2"}` added as a step +- **THEN** the embedded inner workflow observes `DontPanic = true` for its execution + +#### Scenario: Nested *Workflow added directly inherits parent Option +- **GIVEN** a parent with `Option.Mutators = [M]` and a child `*flow.Workflow` containing a step matching `M` +- **WHEN** the parent runs with the child added as a step +- **THEN** `M` runs against the matching inner step \ No newline at end of file diff --git a/openspec/specs/mutators/spec.md b/openspec/specs/mutators/spec.md index 46e35d9..b1b70fd 100644 --- a/openspec/specs/mutators/spec.md +++ b/openspec/specs/mutators/spec.md @@ -33,8 +33,9 @@ construct a Mutator that: Selection is by concrete type along the unwrap chain only. There is no interface matching and no name matching. The unwrap traversal mirrors `flow.As[T]`: it follows `Unwrap() Steper` and `Unwrap() []Steper`, but does NOT cross workflow boundaries (a -nested workflow's inner steps are reached via the propagation mechanism in -`MutatorReceiver propagation interface`, not via in-place unwrap from the parent). +nested workflow's inner steps are reached via the propagation mechanism in the +`Mutator propagation to nested workflows` requirement, not via in-place unwrap from +the parent). The `ctx` passed to `fn` is the **workflow-scoped context** — i.e. the `ctx` that was passed into `Workflow.Do(ctx)`. It is suitable for accessing values that are stable for @@ -90,8 +91,8 @@ through composition. The runtime SHALL dispatch and merge Mutators using the following rule, hereafter "**dispatch by the owning workflow**": -1. Each `*Workflow` (parent or inner) SHALL evaluate its own `Workflow.Mutators` slice - against every step in its own `state` map, at that step's first scheduling. +1. Each `*Workflow` (parent or inner) SHALL evaluate its own `Workflow.Option.Mutators` + slice against every step in its own `state` map, at that step's first scheduling. 2. For each step `wrapper` in `w.state`, the runtime SHALL walk `wrapper`'s `Unwrap()` chain. For each layer `L` in the chain (including `wrapper` itself, in pre-order), the runtime SHALL ask each Mutator whether `L.(T)` matches. The first matching layer @@ -101,12 +102,13 @@ The runtime SHALL dispatch and merge Mutators using the following rule, hereafte `wrapper` as a root**, regardless of which layer in the unwrap chain the Mutator matched. This matches the `Config merge destination follows StateOf` rule in `step-configuration`. -4. Parent Mutators SHALL reach inner-workflow steps only via the `MutatorReceiver` - propagation interface (parent prepends its Mutators into the inner workflow's - `Mutators` slice). The parent SHALL NOT directly walk into a child workflow's - `state` map to apply Mutators; that is the inner workflow's responsibility once it - begins scheduling. This keeps once-per-step state, merge destinations, and lazily - added inner steps consistent across all nesting depths. +4. Parent Mutators SHALL reach inner-workflow steps only via the + `WorkflowOptionReceiver.InheritOption` propagation mechanism defined in the + `workflow-options` capability (parent prepends its `Option.Mutators` into the inner + workflow's `Option.Mutators` slice). The parent SHALL NOT directly walk into a child + workflow's `state` map to apply Mutators; that is the inner workflow's responsibility + once it begins scheduling. This keeps once-per-step state, merge destinations, and + lazily added inner steps consistent across all nesting depths. This rule resolves three previously-ambiguous points: @@ -118,13 +120,14 @@ This rule resolves three previously-ambiguous points: Mutator was registered. - **Nested workflows are reached by propagation, not by parent walking** — a parent Mutator targeting `*X` where `*X` lives inside an inner workflow IS observed by - `*X`, but the dispatch is performed by the inner workflow after `PrependMutators`, - not by the parent reaching across the workflow boundary. + `*X`, but the dispatch is performed by the inner workflow after `InheritOption` + prepends the parent's contributions, not by the parent reaching across the workflow + boundary. #### Scenario: Mutator matches via Unwrap and merges into wrapper's state - **GIVEN** a workflow `w` where `w.state` contains `wrapper = flow.Name("x", inner)` whose `Unwrap()` returns `inner` of type `*Inner` -- **AND** `w.Mutators = [flow.Mutate[*Inner](func(ctx, x *Inner) Builder { return flow.Step(x).Input(fn) })]` +- **AND** `w.Option.Mutators = [flow.Mutate[*Inner](func(ctx, x *Inner) Builder { return flow.Step(x).Input(fn) })]` - **WHEN** `wrapper` reaches its first scheduling - **THEN** the Mutator's user function receives the `*Inner` pointer `inner` - **AND** `fn` is appended to `w.state[wrapper].Config.Before` (NOT to a state entry @@ -133,46 +136,49 @@ This rule resolves three previously-ambiguous points: #### Scenario: Parent Mutator targeting inner type does not directly mutate inner state - **GIVEN** `parent` contains `inner` (a `*Workflow` used as a step) which contains `x` of type `*X` -- **AND** `parent.Mutators = [flow.Mutate[*X](fn)]` +- **AND** `parent.Option.Mutators = [flow.Mutate[*X](fn)]` - **WHEN** `parent.Do(ctx)` runs -- **THEN** `parent` SHALL call `inner.PrependMutators(parent.Mutators)` before invoking - `inner` as a step +- **THEN** `parent` SHALL invoke `inner.InheritOption(parent.Option)` once before + scheduling, propagating the parent's `Option.Mutators` into `inner.Option.Mutators` - **AND** the Mutator runs once against `x` inside `inner`'s scheduling pass; the Builder is merged into `inner.state[x].Config`, NOT into any state entry on `parent` #### Scenario: Wrapper inside inner workflow is reached via propagation - **GIVEN** `parent → inner → flow.Name("x", x)` where `x` is `*X` -- **AND** `parent.Mutators = [flow.Mutate[*X](fn)]` +- **AND** `parent.Option.Mutators = [flow.Mutate[*X](fn)]` - **WHEN** the wrapper reaches first scheduling inside `inner` -- **THEN** the Mutator (propagated by `PrependMutators`) unwraps the wrapper, matches +- **THEN** the Mutator (propagated by `InheritOption`) unwraps the wrapper, matches `x`, and merges its Builder into `inner.state[wrapper].Config` --- -### Requirement: Workflow.Mutators field +### Requirement: Workflow.Option.Mutators field -`flow.Workflow` SHALL expose a `Mutators []Mutator` field. Mutators in this slice are -evaluated against every step before the step's first attempt. Slice order is preserved: -element 0 runs first, element n-1 runs last, among the Mutators whose target type matches -the step. +`flow.Workflow` SHALL expose a `Mutators []Mutator` field under +`Workflow.Option` (the new `WorkflowOption` grouping introduced by the +`workflow-options` capability). Mutators in this slice are evaluated in +slice order against each step in the Workflow's state map: element 0 runs +first, element n-1 runs last, among the Mutators whose target type matches +that step. -A workflow with `Mutators == nil` or an empty slice SHALL behave identically to one -without the Mutator mechanism — no overhead, no panic. +A workflow with `Option.Mutators == nil` or an empty slice SHALL behave +identically to one without any Mutators at all. + +The previous top-level `Workflow.Mutators` field SHALL NOT exist. #### Scenario: Slice order is preserved among matching Mutators -- **WHEN** two Mutators `[m1, m2]` both registered for `*Foo` are present, and `m1` - returns a Builder with `Input` callback A, `m2` returns a Builder with `Input` - callback B -- **THEN** before the first attempt of any `*Foo`, callback A runs before callback B - (because the merge appends in order) +- **WHEN** two Mutators `[m1, m2]` both registered for `*Foo` are present under + `Option.Mutators`, and `m1` and `m2` each return a Builder with a `BeforeStep` + callback +- **THEN** the merged step's `Before` chain runs `m1`'s callback before `m2`'s #### Scenario: Multiple Mutators for the same type all run -- **WHEN** two Mutators `m1`, `m2` are both registered for `*Foo` +- **WHEN** two Mutators `m1`, `m2` are both registered for `*Foo` under `Option.Mutators` - **THEN** both run, in slice order, before the first attempt of every `*Foo` step; their contributions are merged #### Scenario: Nil Mutators field is a no-op -- **WHEN** `Workflow.Mutators` is nil +- **WHEN** `Workflow.Option.Mutators` is nil - **THEN** the workflow runs identically to a workflow without the Mutator field --- @@ -189,9 +195,9 @@ step's existing lists via the standard `StepConfig.Merge` rule (see `step-configuration`'s `Idempotent Add with config merging` requirement). Consequently: - **Before chain execution order:** plan-declared `Input` / `BeforeStep` run first; - Mutator-contributed callbacks run after, in `Workflow.Mutators` slice order. + Mutator-contributed callbacks run after, in `Workflow.Option.Mutators` slice order. - **After chain execution order:** plan-declared `Output` / `AfterStep` run first; - Mutator-contributed callbacks run after, in `Workflow.Mutators` slice order. + Mutator-contributed callbacks run after, in `Workflow.Option.Mutators` slice order. - **Multiple Mutators on the same step:** their callbacks all append after plan's; among Mutators, slice order is preserved. @@ -204,9 +210,10 @@ cannot pre-empt plan callbacks; if pre-empting is needed (rare), register the be as a plan-time `Input` instead. Sub-workflow propagation interacts with this ordering as follows: parent Mutators are -prepended into the inner workflow's `Mutators` slice (see `MutatorReceiver propagation` -requirement) so that, inside the inner workflow, parent Mutators run before child -Mutators. Both still run after the inner step's plan-declared callbacks. +prepended into the inner workflow's `Option.Mutators` slice (see +`Mutator propagation to nested workflows` requirement) so that, inside the inner +workflow, parent Mutators run before child Mutators. Both still run after the inner +step's plan-declared callbacks. #### Scenario: Plan Input runs before Mutator Input - **GIVEN** a step `s` added via `flow.Step(s).Input(planFn)` @@ -217,7 +224,7 @@ Mutators. Both still run after the inner step's plan-declared callbacks. #### Scenario: Multiple Mutator Inputs run in slice order, after plan - **GIVEN** a step `s` added via `flow.Step(s).Input(planFn)` -- **AND** `Workflow.Mutators = [m1, m2]` where both target `*S` and contribute Inputs +- **AND** `Workflow.Option.Mutators = [m1, m2]` where both target `*S` and contribute Inputs `mfn1`, `mfn2` respectively - **WHEN** the step executes its first attempt - **THEN** the Before chain runs `planFn`, then `mfn1`, then `mfn2`, then `Do` @@ -306,77 +313,63 @@ constructs a Builder that mentions other steps. --- -### Requirement: MutatorReceiver propagation interface - -`flow.MutatorReceiver` SHALL be defined as an optional interface that composite steps and -sub-workflows implement to receive Mutators from their parent workflow: +### Requirement: Mutator propagation to nested workflows -```go -type MutatorReceiver interface { - PrependMutators(mw []Mutator) -} -``` +A nested workflow (whether `*Workflow` used directly as a step or a user +struct embedding `flow.Workflow`) SHALL receive its parent's Mutators via +the `WorkflowOptionReceiver.InheritOption` mechanism defined in the +`workflow-options` capability. -Before the runtime invokes a step that hosts a sub-workflow (composite step or -`*Workflow` used as a step), the runtime SHALL check whether the step implements -`MutatorReceiver` and, if so, call `step.PrependMutators(parent.Mutators)` so that parent -Mutators reach inner steps. +Specifically, the parent SHALL invoke `restore := child.InheritOption(parent.Option)` +once in its `Do()` prologue and `defer restore()`. The implementation of +`InheritOption` MUST prepend the parent's `Option.Mutators` to the child's +`Option.Mutators` slice (parent contributions run first within the child), +allocating a fresh slice rather than mutating the parent's or child's input. -The contract for implementers is to **prepend** parent Mutators to their own Mutators -list, preserving the invariant that parent Mutators run before child Mutators. +The parent SHALL NOT directly walk into a child workflow's `state` map to +apply Mutators; that remains the inner workflow's responsibility once it +begins its own scheduling pass with the merged `Option.Mutators` slice. -The propagation is invoked once per inner-workflow execution. Inner steps that are added -lazily (e.g. inside the composite step's `Do`) are still reached, because Mutator -evaluation in the inner workflow is itself once-per-step at first scheduling. +The propagation is invoked once per inner-workflow execution. Inner steps +that are added lazily (e.g. inside the composite step's `Do`) are still +reached, because Mutator evaluation in the inner workflow is itself +once-per-step at first scheduling. -#### Scenario: Parent Mutator reaches step inside SubWorkflow -- **WHEN** the parent workflow has `flow.Mutate[*Foo]` registered and a step embeds - `flow.SubWorkflow` with a `*Foo` inside -- **THEN** the parent Mutator runs against the inner `*Foo` instance once, before its - first attempt +The previous `MutatorReceiver` interface and `PrependMutators` method SHALL +NOT exist. #### Scenario: Parent Mutator reaches step inside nested *Workflow -- **WHEN** the parent workflow has `flow.Mutate[*Foo]` registered and a `*Workflow` is - added as a step containing a `*Foo` -- **THEN** the parent Mutator runs against the inner `*Foo` +- **GIVEN** `parent.Option.Mutators = [flow.Mutate[*Foo](fn)]` +- **AND** a child `*Workflow` containing a `*Foo` step +- **WHEN** parent runs, with child added as a step +- **THEN** parent invokes `child.InheritOption(parent.Option)` once before scheduling +- **AND** the inner workflow's first-schedule pass evaluates the propagated Mutator against `*Foo` + +#### Scenario: Parent Mutator reaches inner *Foo when child is wrapped +- **GIVEN** `parent.Option.Mutators = [flow.Mutate[*Foo](fn)]` +- **AND** a child `*Workflow` containing a `*Foo` step, added to parent via `flow.Name(child, "name")` +- **WHEN** parent runs +- **THEN** the parent's Mutator reaches the inner `*Foo` via `findOptionReceiver`'s `Unwrap` walk #### Scenario: Lazily added inner steps are reached -- **WHEN** a composite step embeds `SubWorkflow`, calls `s.Add(new(Foo))` inside its - `Do`, and the parent has `flow.Mutate[*Foo]` registered +- **WHEN** a composite step embeds `flow.Workflow`, calls `s.Add(new(Foo))` inside its + `Do`, and the parent has `flow.Mutate[*Foo]` registered under `Option.Mutators` - **THEN** the lazily added `*Foo` is seen by the parent Mutator at its first scheduling inside the inner workflow +#### Scenario: Multi-level Mutator propagation preserves order +- **GIVEN** grandparent `Option.Mutators = [A]`, parent `Option.Mutators = [B]`, child `Option.Mutators = [C]` +- **WHEN** grandparent runs the parent which runs the child +- **THEN** the child's effective Mutators (during its own scheduling) is `[A, B, C]` + #### Scenario: Child Mutator runs after parent -- **WHEN** the parent registers Mutator `pm` and a `SubWorkflow`-embedded child registers - Mutator `cm`, both targeting `*Foo` +- **WHEN** the parent registers Mutator `pm` and a child `*Workflow` registers + Mutator `cm` (both via `Option.Mutators`), both targeting `*Foo` - **THEN** for any `*Foo` inside the child, `pm` runs before `cm`; their `Before` chain contributions are appended in that order --- -### Requirement: SubWorkflow and *Workflow implement MutatorReceiver - -`flow.SubWorkflow` SHALL implement `MutatorReceiver` by prepending the supplied Mutators -to its inner workflow's `Mutators` slice. - -`*flow.Workflow` (when used directly as a step in another workflow) SHALL implement -`MutatorReceiver` by prepending the supplied Mutators to its own `Mutators` slice. - -Both implementations MUST be safe to call multiple times; calling `PrependMutators` twice -with the same list MUST NOT cause double-execution of the Mutators on the same step -(deduplication is provided by the once-per-step invariant — once a step's Mutators have -been applied, re-prepending the slice has no further effect on that step). - -#### Scenario: SubWorkflow.PrependMutators prepends in order -- **WHEN** `parent.Mutators = [m1, m2]` and a SubWorkflow's inner has `[m3]` -- **THEN** after `PrependMutators` is called, the inner has `[m1, m2, m3]` - -#### Scenario: *Workflow used as step inherits Mutators -- **WHEN** a `*Workflow` is added as a step in another workflow with Mutators `[m1]` -- **THEN** during execution, `[m1]` is prepended to the inner workflow's Mutators - ---- - ### Requirement: Mutator vs Input/BeforeStep usage guidance The framework SHALL preserve both `flow.Step(s).Input(fn)` (per-instance binding) and diff --git a/openspec/specs/step-interceptor/spec.md b/openspec/specs/step-interceptor/spec.md index 5227148..411f20c 100644 --- a/openspec/specs/step-interceptor/spec.md +++ b/openspec/specs/step-interceptor/spec.md @@ -75,26 +75,36 @@ serialize behind a low concurrency limit. ### Requirement: Workflow registration of interceptors -`Workflow` SHALL expose two slice fields for global interceptor registration: +`Workflow` SHALL expose two slice fields for global interceptor registration +under `Workflow.Option` (the `WorkflowOption` grouping defined by the +`workflow-options` capability): ```go -type Workflow struct { +type WorkflowOption struct { + // ... other fields ... StepInterceptors []StepInterceptor // [0] outermost, [len-1] innermost AttemptInterceptors []AttemptInterceptor // [0] outermost, [len-1] innermost - IsolateInterceptors bool // if true, do not inherit from a parent workflow + DontInherit bool // if true, do not inherit from a parent workflow + // ... other fields ... } ``` -Nil/empty slices mean no interceptors. Existing workflows without interceptors SHALL behave -identically to before this feature was added (zero-value safe, no allocations on the hot -path). +Nil/empty slices mean no interceptors. Existing workflows without +interceptors SHALL behave identically to before this feature was added +(zero-value safe, no allocations on the hot path). + +The previous top-level `Workflow.StepInterceptors` / +`Workflow.AttemptInterceptors` / `Workflow.IsolateInterceptors` fields +SHALL NOT exist. `IsolateInterceptors` is replaced by `Option.DontInherit`, +whose semantics extend from interceptor-only to whole-`WorkflowOption` +opt-out. #### Scenario: Outer-to-inner ordering -- **WHEN** `StepInterceptors = [A, B]` are registered +- **WHEN** `Option.StepInterceptors = [A, B]` are registered - **THEN** the execution order is `A:before → B:before → step → B:after → A:after` #### Scenario: No interceptors means no behavioural change -- **WHEN** a Workflow is constructed without `StepInterceptors` or `AttemptInterceptors` +- **WHEN** a Workflow is constructed without `Option.StepInterceptors` or `Option.AttemptInterceptors` - **THEN** all existing semantics (retries, conditions, BeforeStep/AfterStep) are unchanged --- @@ -125,83 +135,91 @@ StepInterceptor[0] → ... → StepInterceptor[N-1] ### Requirement: Interceptor propagation to nested workflows -`Workflow` SHALL implement the `InterceptorReceiver` interface so that when a `*Workflow` -(or a step embedding `SubWorkflow`) is used as a Step inside another Workflow, the parent's -interceptors are prepended to the child's interceptor stack. +`Workflow` SHALL implement `WorkflowOptionReceiver` (defined in the +`workflow-options` capability spec) so that when a `*Workflow` (or a user +struct embedding `flow.Workflow`) is used as a Step inside another +Workflow, the parent's interceptors are prepended to the child's +interceptor slices via the parent's prologue-pass `InheritOption` call. -```go -type InterceptorReceiver interface { - PrependInterceptors(step []StepInterceptor, attempt []AttemptInterceptor) -} -``` +The parent's `Do()` prologue locates the receiver for each root step by +walking `Unwrap()` via `findOptionReceiver` (the same protocol used by +`As[T]` / `Has[T]`) and selecting the first receiver found in pre-order. +This means a sub-workflow MAY be wrapped in any Steper-only wrapper +(notably `flow.Name` / `NamedStep`) without losing inheritance. -`stepExecution` locates the `InterceptorReceiver` for a step by walking the Step tree via -`Unwrap` (the same protocol used by `As[T]` / `Has[T]`) and selecting the first receiver -found in pre-order. This means a sub-workflow MAY be wrapped in any Steper-only wrapper -(notably `flow.Name` / `NamedStep`, whose embedded `Steper` interface does not promote -`PrependInterceptors`) without losing inheritance. +The parent SHALL invoke `restore := recv.InheritOption(parent.Option)` +exactly once per root sub-workflow step before the parent's tick begins, +and SHALL `defer restore()`. Inheritance is **per-run scoped**: -`stepExecution` calls `PrependInterceptors` exactly once per step, in `executeWithRetry` -before the retry loop begins. Inheritance is **per-run scoped**: +- The parent's user-supplied `Option.StepInterceptors` / + `Option.AttemptInterceptors` slices SHALL NOT be mutated by + `InheritOption`; the implementation prepends to a fresh slice. +- Each `InheritOption` call snapshots the receiver's `Option` and returns + a `restore func()` that the parent defers, so prepended parent + contributions do not accumulate across repeated `Do()` runs. -- The user-supplied `StepInterceptors` / `AttemptInterceptors` slices SHALL NOT be mutated. -- The inherited prefix SHALL be stored on private `inheritedStep` / `inheritedAttempt` - fields and combined with the base only when constructing the run-time chain. -- The inherited fields SHALL be cleared via `defer` at the start of every `Do()` so all - exit paths (success, preflight error, panic) reset the per-run state. -- The public `Reset()` method SHALL also clear the inherited fields. The internal - `reset()` (called by `Do()` itself) SHALL NOT, since clearing there would wipe the - prefix the parent just wrote and break inheritance. - -`SubWorkflow.PrependInterceptors` SHALL delegate to the embedded `Workflow.PrependInterceptors`. +The previous `InterceptorReceiver` interface, `PrependInterceptors` method, +and `inheritedStep` / `inheritedAttempt` side fields SHALL NOT exist. #### Scenario: Nested *Workflow inherits parent interceptors -- **GIVEN** a parent Workflow with a `StepInterceptor` X, and a child `*Workflow` containing +- **GIVEN** a parent Workflow with `Option.StepInterceptors = [X]`, and a child `*Workflow` containing step `S` added as a step in the parent - **WHEN** the parent runs - **THEN** X is invoked for both the child workflow step and the inner step S -#### Scenario: SubWorkflow inherits parent interceptors -- **GIVEN** a parent Workflow with a `StepInterceptor` X, and a step embedding `SubWorkflow` +#### Scenario: Embedded Workflow inherits parent interceptors +- **GIVEN** a parent Workflow with `Option.StepInterceptors = [X]`, and a step embedding `flow.Workflow` containing step `S` - **WHEN** the parent runs - **THEN** X is invoked for both the outer step and the inner step S #### Scenario: Inheritance survives Steper-only wrappers (NamedStep / flow.Name) -- **GIVEN** a parent Workflow with a `StepInterceptor` X, and a child `*Workflow` - containing step `S` that is added to the parent via `flow.Name(child, "name")` (which - wraps the child in a `NamedStep` whose embedded `Steper` interface does not promote - `PrependInterceptors`) +- **GIVEN** a parent Workflow with `Option.StepInterceptors = [X]`, and a child `*Workflow` + containing step `S` that is added to the parent via `flow.Name(child, "name")` - **WHEN** the parent runs - **THEN** X is invoked for both the wrapping `NamedStep` and the inner step S -- **AND** inheritance works because `stepExecution` looks up `InterceptorReceiver` via +- **AND** inheritance works because the parent looks up `WorkflowOptionReceiver` via `Unwrap`, not via a direct type assertion on the registered Step -#### Scenario: PrependInterceptors does not duplicate across retries -- **WHEN** a sub-workflow step is retried N times -- **THEN** parent interceptors are prepended exactly once, not N times +#### Scenario: InheritOption does not duplicate across retries +- **WHEN** a sub-workflow step is retried N times within one parent run +- **THEN** parent interceptors are prepended to the child's slice exactly once, not N times -#### Scenario: PrependInterceptors does not accumulate across repeated Do() runs +#### Scenario: InheritOption does not accumulate across repeated Do() runs - **GIVEN** a parent containing a child sub-workflow - **WHEN** the parent's `Do()` is invoked N times in succession - **THEN** each invocation results in the parent's interceptors firing exactly once per step (no compounding across runs) +- **AND** the child's `Option.StepInterceptors` field after each run is its original + pre-inheritance value --- -### Requirement: Opting out of inheritance via IsolateInterceptors +### Requirement: Opting out of inheritance via DontInherit + +A nested `Workflow` MAY set `Option.DontInherit = true` to opt out of +inheriting any of the parent's `WorkflowOption`, including interceptors. +When true, the merge step performed by `InheritOption` SHALL be a no-op +and the workflow runs only with its own configured `Option`. A (possibly +trivial) restore func is still returned so the parent's `defer restore()` +remains uniform. + +This is intended for self-contained sub-workflows that define their own +observability pipeline (e.g., their own tracer or event sink) that must not +be wrapped by parent interceptors, or that more generally want isolation +from the parent's whole `WorkflowOption`. -A nested `Workflow` MAY set `IsolateInterceptors = true` to opt out of inheriting -interceptors from its parent. When true, `Workflow.PrependInterceptors` SHALL be a no-op -and the workflow runs only with its own registered interceptors. +The previous `IsolateInterceptors` flag SHALL NOT exist; its semantics are +subsumed and widened by `Option.DontInherit`. -This is intended for self-contained sub-workflows that define their own observability -pipeline (e.g., their own tracer or event sink) that must not be wrapped by parent -interceptors. +#### Scenario: DontInherit blocks interceptor inheritance +- **GIVEN** parent `Option.StepInterceptors = [X]`, child `Option.DontInherit = true, StepInterceptors = [Y]` +- **WHEN** parent runs the child +- **THEN** the child runs with only `[Y]` as its effective StepInterceptors #### Scenario: Isolated child does not see parent interceptors -- **GIVEN** a parent Workflow with `StepInterceptor` X and a child Workflow with - `IsolateInterceptors = true` and its own `StepInterceptor` Y, containing inner step S +- **GIVEN** a parent Workflow with `Option.StepInterceptors = [X]` and a child Workflow with + `Option.DontInherit = true` and its own `Option.StepInterceptors = [Y]`, containing inner step S - **WHEN** the parent runs the child as a step - **THEN** X is invoked exactly once (for the child workflow step itself) - **AND** Y is invoked for inner step S @@ -231,27 +249,27 @@ regardless of interceptor behaviour. ### Requirement: DontPanic protects interceptor panics -When `Workflow.DontPanic` is `true`, panics raised inside user-provided `StepInterceptor` -or `AttemptInterceptor` implementations SHALL be caught and converted to errors using the -same `catchPanicAsError` mechanism already applied to `Before` / `Do` / `After`. This -prevents: +When `Workflow.Option.DontPanic` is non-nil and dereferences to `true`, panics raised +inside user-provided `StepInterceptor` or `AttemptInterceptor` implementations SHALL be +caught and converted to errors using the same `catchPanicAsError` mechanism already +applied to `Before` / `Do` / `After`. This prevents: - Process crashes from a faulty user interceptor. - `MaxConcurrency` lease leaks (an unrecovered panic skips the deferred `unlease`). - Loss of `signalStatusChange`, which would otherwise hang the main `Do()` loop. -When `DontPanic` is `false` (the default), interceptor panics propagate as in normal Go -semantics. +When `Option.DontPanic` is nil or dereferences to `false` (the default), interceptor +panics propagate as in normal Go semantics. #### Scenario: Panicking StepInterceptor under DontPanic -- **GIVEN** a Workflow with `DontPanic = true` and a `StepInterceptor` that panics +- **GIVEN** a Workflow with `Option.DontPanic = &true` and a `StepInterceptor` that panics - **WHEN** the Workflow runs - **THEN** `Do()` returns an error within a bounded time - **AND** the step's `StepResult.Err` carries the panic value - **AND** the workflow does not hang waiting for a status signal #### Scenario: Panicking AttemptInterceptor under DontPanic -- **GIVEN** a Workflow with `DontPanic = true` and an `AttemptInterceptor` that panics +- **GIVEN** a Workflow with `Option.DontPanic = &true` and an `AttemptInterceptor` that panics - **WHEN** the Workflow runs - **THEN** `Do()` returns an error within a bounded time - **AND** the step's `StepResult.Err` carries the panic value diff --git a/openspec/specs/workflow-options/spec.md b/openspec/specs/workflow-options/spec.md index c1e0fef..a692c58 100644 --- a/openspec/specs/workflow-options/spec.md +++ b/openspec/specs/workflow-options/spec.md @@ -1,80 +1,147 @@ -## ADDED Requirements +# workflow-options Specification + +## Purpose + +This capability covers `flow.Workflow`'s top-level configuration surface, +grouped under a single named field `Workflow.Option WorkflowOption`. It +defines: + +- The `WorkflowOption` struct and its scalar pointer / slice fields. +- The `WorkflowOptionReceiver` interface used to propagate `WorkflowOption` + from a parent workflow into a sub-workflow root step. +- The merge rules for parent → child Option inheritance (scalars, slices, + `DontInherit` opt-out). +- The accumulation-prevention contract: each `InheritOption` invocation + returns a `restore func()` that the parent defers, so writes performed + during one parent `Do()` run do not bleed into subsequent runs. +- The role of `Workflow.Reset()`, which rewinds per-step status only. + +## Requirements + +### Requirement: WorkflowOption groups workflow-level configuration + +`flow.Workflow` SHALL expose all configuration fields under a single named +field `Option WorkflowOption`. The previous nine top-level configuration +fields (`MaxConcurrency`, `DontPanic`, `SkipAsError`, `Clock`, +`DefaultOption`, `Mutators`, `StepInterceptors`, `AttemptInterceptors`, +`IsolateInterceptors`) SHALL NOT exist as direct fields on `Workflow`. + +`WorkflowOption` SHALL declare: + +```go +type WorkflowOption struct { + MaxConcurrency *int + DontPanic *bool + SkipAsError *bool + Clock clock.Clock + StepDefaults *StepOption + + Mutators []Mutator + StepInterceptors []StepInterceptor + AttemptInterceptors []AttemptInterceptor + + DontInherit bool +} +``` + +Scalar configuration fields are pointer-typed so that "unset" (nil pointer) +and "explicit zero value" are distinguishable. This distinction is required +for parent → child Option inheritance: a nil pointer on the child means +"inherit from parent"; a non-nil pointer means "child wins". + +#### Scenario: Workflow exposes Option field +- **WHEN** a user constructs `&flow.Workflow{Option: flow.WorkflowOption{...}}` +- **THEN** the Workflow accepts the configuration and applies it + +#### Scenario: Explicit zero is distinguishable from unset +- **GIVEN** a parent with `Option.MaxConcurrency = &four` (where `four = 4`) +- **AND** a child with `Option.MaxConcurrency = &zero` (where `zero = 0`) +- **WHEN** parent runs the child as a sub-workflow +- **THEN** the child observes `MaxConcurrency = 0` (unlimited), NOT inherited 4 + +--- ### Requirement: MaxConcurrency limits simultaneous running Steps -When `Workflow.MaxConcurrency` is set to a positive integer `N`, the Workflow SHALL run -at most `N` Steps concurrently. Additional runnable Steps wait until a running Step -terminates before starting. A value of `0` (the default) means unlimited concurrency. +When `Workflow.Option.MaxConcurrency` is non-nil and points to a positive integer +`N`, the Workflow SHALL run at most `N` Steps concurrently. Additional runnable +Steps wait until a running Step terminates before starting. A nil pointer (the +default) or a value of `0` means unlimited concurrency. The limit is implemented via a buffered channel used as a lease bucket. #### Scenario: MaxConcurrency=2 allows exactly 2 concurrent Steps -- **WHEN** `MaxConcurrency` is 2 and 3 independent Steps are added +- **WHEN** `Option.MaxConcurrency` points to `2` and 3 independent Steps are added - **THEN** at most 2 Steps run simultaneously; the third starts only after one finishes -#### Scenario: MaxConcurrency=0 imposes no limit -- **WHEN** `MaxConcurrency` is 0 (the default) +#### Scenario: MaxConcurrency nil imposes no limit +- **WHEN** `Option.MaxConcurrency` is nil (the default) - **THEN** all runnable Steps start concurrently without any concurrency bound --- ### Requirement: DontPanic converts panics to errors -When `Workflow.DontPanic` is `true`, any `panic` in a Step's `Do`, `Input`, or -`BeforeStep`/`AfterStep` callbacks is recovered and returned as a `ErrPanic`-wrapped -error, setting the Step to `Failed`. Stack trace information is captured and included -in the error. +When `Workflow.Option.DontPanic` is non-nil and dereferences to `true`, any +`panic` in a Step's `Do`, `Input`, or `BeforeStep`/`AfterStep` callbacks is +recovered and returned as a `ErrPanic`-wrapped error, setting the Step to +`Failed`. Stack trace information is captured and included in the error. -When `DontPanic` is `false` (the default), panics propagate normally and crash the -process. +When `Option.DontPanic` is nil (the default) or dereferences to `false`, +panics propagate normally and crash the process. #### Scenario: Panic converted to Failed with DontPanic=true -- **WHEN** `DontPanic` is `true` and a Step panics during `Do` +- **WHEN** `Option.DontPanic` points to `true` and a Step panics during `Do` - **THEN** the Step status is `Failed`; the returned `ErrWorkflow` entry wraps the panic value as an `ErrPanic` -#### Scenario: Panic propagates with DontPanic=false -- **WHEN** `DontPanic` is `false` and a Step panics +#### Scenario: Panic propagates with DontPanic nil +- **WHEN** `Option.DontPanic` is nil and a Step panics - **THEN** the panic propagates out of the goroutine (process crash or test failure) --- ### Requirement: SkipAsError controls whether Skipped counts as failure -When `Workflow.SkipAsError` is `false` (the default), Steps that are `Skipped` are -considered acceptable outcomes. `workflow.Do` returns `nil` if all root Steps are -`Succeeded` or `Skipped`. +When `Workflow.Option.SkipAsError` is nil or dereferences to `false` (the +default), Steps that are `Skipped` are considered acceptable outcomes. +`workflow.Do` returns `nil` if all root Steps are `Succeeded` or `Skipped`. -When `SkipAsError` is `true`, any `Skipped` Step causes `workflow.Do` to return an -`ErrWorkflow` even if no Step actually failed. +When `Option.SkipAsError` dereferences to `true`, any `Skipped` Step causes +`workflow.Do` to return an `ErrWorkflow` even if no Step actually failed. #### Scenario: Skipped is acceptable by default -- **WHEN** `SkipAsError` is `false` and all root Steps are either `Succeeded` or `Skipped` +- **WHEN** `Option.SkipAsError` is nil and all root Steps are either `Succeeded` or `Skipped` - **THEN** `workflow.Do` returns `nil` #### Scenario: Skipped counts as error when SkipAsError=true -- **WHEN** `SkipAsError` is `true` and at least one root Step is `Skipped` +- **WHEN** `Option.SkipAsError` points to `true` and at least one root Step is `Skipped` - **THEN** `workflow.Do` returns an `ErrWorkflow` containing the skipped Step --- -### Requirement: DefaultOption applies a baseline StepOption to all Steps +### Requirement: StepDefaults applies a baseline StepOption to all Steps -`Workflow.DefaultOption` is a `*StepOption` that the Workflow prepends to the Option -slice of every Step added to it. This lets callers set a universal default for all Steps -(e.g., a global timeout) without modifying each Step individually. +`Workflow.Option.StepDefaults` is a `*StepOption` that the Workflow +prepends to the Option slice of every Step added to it. This lets callers set +a universal default for all Steps (e.g., a global timeout) without modifying +each Step individually. -Step-level options that are set after the default take precedence because the Option -slice is evaluated left-to-right and later values overwrite earlier ones on the same -`StepOption` struct. +Step-level options that are set after the default take precedence because the +Option slice is evaluated left-to-right and later values overwrite earlier +ones on the same `StepOption` struct. -#### Scenario: DefaultOption sets a global timeout -- **WHEN** `Workflow.DefaultOption` has `Timeout` set to 10 minutes +The renaming from `DefaultOption` to `StepDefaults` clarifies that the +field configures step-level options, not workflow-level options +(`WorkflowOption`). + +#### Scenario: StepDefaults sets a global timeout +- **WHEN** `Option.StepDefaults` has `Timeout` set to 10 minutes and a Step is added without its own `Timeout` - **THEN** the effective timeout for that Step is 10 minutes #### Scenario: Step-level option overrides the default -- **WHEN** `Workflow.DefaultOption` has `Timeout` of 10 minutes and a Step declares +- **WHEN** `Option.StepDefaults` has `Timeout` of 10 minutes and a Step declares `.Timeout(5 * time.Minute)` - **THEN** the effective timeout for that Step is 5 minutes @@ -82,18 +149,194 @@ slice is evaluated left-to-right and later values overwrite earlier ones on the ### Requirement: Clock enables time injection for testing -`Workflow.Clock` is a `clock.Clock` interface (from `github.com/benbjohnson/clock`). -The Workflow uses the Clock for all time-related operations: Step-level timeouts, -per-try timeouts in the retry loop, and backoff waits. +`Workflow.Option.Clock` is a `clock.Clock` interface (from +`github.com/benbjohnson/clock`). The Workflow uses the Clock for all +time-related operations: Step-level timeouts, per-try timeouts in the retry +loop, and backoff waits. -When `Clock` is `nil`, the Workflow uses the real wall clock (`clock.New()`). -Providing a mock clock allows unit tests to control time without real delays. +When `Option.Clock` is `nil`, the Workflow uses the real wall clock +(`clock.New()`). Providing a mock clock allows unit tests to control time +without real delays. #### Scenario: Nil Clock uses wall clock -- **WHEN** `Workflow.Clock` is not set -- **THEN** the Workflow automatically initializes it to `clock.New()` at the start of `Do` +- **WHEN** `Option.Clock` is not set +- **THEN** the Workflow automatically uses `clock.New()` for all time operations #### Scenario: Mock clock controls timeout behavior in tests -- **WHEN** a `clock.Mock` is injected as `Workflow.Clock` +- **WHEN** a `clock.Mock` is injected as `Option.Clock` and the mock is advanced past a Step's `Timeout` duration - **THEN** the Step's context is canceled and the Step is set to `Canceled` + +--- + +### Requirement: WorkflowOptionReceiver propagates Option to sub-workflows + +`flow.Workflow` SHALL implement `WorkflowOptionReceiver`: + +```go +type WorkflowOptionReceiver interface { + // InheritOption merges the parent's WorkflowOption into the receiver + // (scalars: child wins when non-nil; slices: parent prepended). + // It returns a restore func that the parent MUST defer to rewind + // the receiver's Option to its pre-inheritance state, so writes + // performed during one parent Do() run do not accumulate across + // subsequent runs. + InheritOption(parent WorkflowOption) (restore func()) +} +``` + +The parent Workflow SHALL invoke `restore := child.InheritOption(parent.Option)` +exactly once per sub-workflow root step, in the parent's `Do()` prologue +(after `init()`, before the scheduling tick begins), and SHALL `defer restore()` +so the receiver's Option is rewound on every exit path. The parent SHALL +locate the receiver by walking each root step's `Unwrap()` chain via +`findOptionReceiver`, returning the first `WorkflowOptionReceiver` found in +pre-order. This means a sub-workflow MAY be wrapped in any Steper-only +wrapper (notably `flow.Name` / `NamedStep`) without losing inheritance. + +`Workflow.InheritOption` SHALL apply the following merge rules: + +1. If `w.Option.DontInherit` is `true`, `InheritOption` SHALL be a no-op + for the merge step but SHALL still return a (possibly trivial) restore + func so the parent's `defer restore()` is always safe. +2. For each scalar pointer field (`MaxConcurrency`, `DontPanic`, + `SkipAsError`) and each interface/pointer field (`Clock`, + `StepDefaults`): if the child's field is nil, set it to the parent's + value. Non-nil child fields SHALL NOT be modified. +3. For each slice field (`Mutators`, `StepInterceptors`, + `AttemptInterceptors`): allocate a fresh slice equal to + `parent_slice ++ child_slice` and assign it to the child's field. The + parent's and child's input slices SHALL NOT be mutated. + +The parent's user-supplied `WorkflowOption` SHALL NOT be mutated by +`InheritOption`. + +#### Scenario: Scalar nil inherits parent's value +- **GIVEN** parent `Option.DontPanic = &true`, child `Option.DontPanic = nil` +- **WHEN** parent invokes `restore := child.InheritOption(parent.Option)` +- **THEN** child observes `DontPanic = true` for the duration of the run +- **AND** after `restore()` runs, the child's `Option.DontPanic` is back to nil + +#### Scenario: Scalar non-nil child wins +- **GIVEN** parent `Option.MaxConcurrency = &four`, child `Option.MaxConcurrency = &eight` +- **WHEN** parent invokes `child.InheritOption(parent.Option)` +- **THEN** child observes `MaxConcurrency = 8` + +#### Scenario: Slices are parent-prepended +- **GIVEN** parent `Option.Mutators = [A]`, child `Option.Mutators = [B]` +- **WHEN** parent invokes `child.InheritOption(parent.Option)` +- **THEN** child's effective `Mutators` is `[A, B]` for the duration of the run + +#### Scenario: Multi-level nesting prepends in order +- **GIVEN** grandparent `Option.Mutators = [A]`, parent `Option.Mutators = [B]`, child `Option.Mutators = [C]` +- **WHEN** grandparent runs, propagating to parent then to child +- **THEN** child's effective `Mutators` is `[A, B, C]` + +#### Scenario: Inheritance survives Steper-only wrappers +- **GIVEN** a parent with a `Mutator` registered, and a child `*Workflow` added to the parent via `flow.Name(child, "name")` +- **WHEN** the parent runs +- **THEN** the parent's `Mutator` reaches the inner steps via `InheritOption` propagation through `Unwrap` + +--- + +### Requirement: DontInherit opts out of all parent Option inheritance + +When a sub-workflow's `Option.DontInherit` is `true`, the parent's +`InheritOption(parent.Option)` call SHALL be a merge no-op. The +sub-workflow runs with exactly its own configured Option, with no scalars +filled in from the parent and no slices prepended. `InheritOption` still +returns a (possibly trivial) restore func so the parent's `defer restore()` +remains safe and uniform. + +This replaces the previous `IsolateInterceptors` flag and widens its +semantics from interceptor-only to whole-Option opt-out. The naming aligns +with `DontPanic`. + +#### Scenario: DontInherit blocks scalar inheritance +- **GIVEN** parent `Option.DontPanic = &true`, child `Option.DontInherit = true, DontPanic = nil` +- **WHEN** parent invokes `child.InheritOption(parent.Option)` +- **THEN** child observes `DontPanic = false` (nil dereferenced as default) + +#### Scenario: DontInherit blocks slice prepending +- **GIVEN** parent `Option.Mutators = [A]`, child `Option.DontInherit = true, Mutators = [B]` +- **WHEN** parent invokes `child.InheritOption(parent.Option)` +- **THEN** child's effective `Mutators` is `[B]`, not `[A, B]` + +--- + +### Requirement: InheritOption restore prevents cross-run accumulation + +The `restore func()` returned by `InheritOption` SHALL rewind the receiver's +`Option` to its pre-inheritance value (a snapshot captured at the start of +`InheritOption`). The parent SHALL `defer restore()` for every receiver it +called `InheritOption` on, ensuring that on every exit path of the parent's +`Do()` (success, error, or panic) the children's Option fields are reset. + +This guarantees that `InheritOption` writes performed by a parent during one +`Do()` run do not accumulate into subsequent runs of the same parent or of +the same child reused under a different parent. + +The snapshot captured inside `InheritOption` is a shallow copy. This is +correct because: + +- Pointer overwrites on nil scalar fields point to the parent's existing + pointer values without mutating them; restoring the snapshot resets + the pointer back to nil. +- Slice fields are always written via fresh `make`-and-append in + `InheritOption`; the snapshot's slice header still references the + pre-inheritance backing array, which is what restore reinstates. + +The internal `reset()` SHALL NOT clear `w.Option` (that is the restore +func's job, deferred at the parent scope). + +The public `Workflow.Reset()` SHALL NOT clear `w.Option` either, because +the restore mechanism already prevents accumulation; `Reset()` exists +solely to reset per-step status (see the `Reset` requirement). + +#### Scenario: Repeated Do() runs do not accumulate inherited contributions +- **GIVEN** a parent with `Option.Mutators = [A]` and a child with `Option.Mutators = [B]` +- **WHEN** `parent.Do()` is invoked N times +- **THEN** each invocation results in the child's effective `Mutators` being `[A, B]` during the run +- **AND** the child's `Option.Mutators` field is `[B]` after each run completes + +#### Scenario: Restore covers all exit paths +- **WHEN** `Do()` returns successfully, returns an error, or panics (when not recovered) +- **THEN** every `restore()` deferred by the parent fires; each receiver's `Option` is restored to its pre-`InheritOption` state + +--- + +### Requirement: Reset rewinds per-step status without touching the step set + +`Workflow.Reset()` SHALL set every Step's status from any terminal state +(`Succeeded`, `Failed`, `Skipped`, `Canceled`) back to `Pending`, allowing +`Do()` to be invoked again on the same Workflow. `Reset()` SHALL reject +with `ErrWorkflowIsRunning` if a `Do()` call is currently in flight. + +`Reset()` SHALL NOT modify `w.steps` (the set of Steps registered via +`Add`). This is a contract: a Workflow built once via `Add` can be +`Do()`-ed any number of times via `Reset/Do` cycles, with the same DAG +each time. To start from an empty set of Steps, allocate a new `Workflow`. + +`Reset()` SHALL NOT modify `w.Option`. Cross-run accumulation of +parent-inherited contributions is prevented by the `restore func()` returned +from `InheritOption` (see the preceding requirement), not by `Reset()`. +Calling `Reset()` between runs is therefore optional from an Option-isolation +standpoint; its purpose is purely to rewind per-step status for re-execution. + +#### Scenario: Reset rewinds status but preserves the DAG +- **GIVEN** a Workflow with steps `[a, b, c]` that has been `Do()`-ed once +- **WHEN** `w.Reset()` is called and then `w.Do()` is called again +- **THEN** all three steps execute a second time +- **AND** `w.steps` still contains `[a, b, c]` + +#### Scenario: Reset rejected while a Do is in flight +- **WHEN** `Reset()` is called while `Do()` is executing on the same Workflow +- **THEN** `Reset()` returns `ErrWorkflowIsRunning` without modifying state + +#### Scenario: Reset is not required to prevent Option accumulation +- **GIVEN** a parent with `Option.Mutators = [A]` and a child with `Option.Mutators = [B]` +- **WHEN** `parent.Do()` is invoked N times in succession WITHOUT calling + `parent.Reset()` between runs (the parent has no terminal-status steps + to reset because each Do() resets internally via the unexported `reset()`) +- **THEN** each invocation results in the child's effective `Mutators` + being `[A, B]` during the run, with no accumulation diff --git a/retry.go b/retry.go index 2ab8b13..8a5e329 100644 --- a/retry.go +++ b/retry.go @@ -84,7 +84,7 @@ func (w *Workflow) retry(opt *RetryOption) func( } backOff = backoff.WithContext(backOff, ctx) if !notAfter.IsZero() { - backOff = &backOffStopIfTimeout{BackOff: backOff, NotAfter: notAfter, Now: w.Clock.Now} + backOff = &backOffStopIfTimeout{BackOff: backOff, NotAfter: notAfter, Now: w.clock().Now} } if opt.Attempts > 0 { // WithMaxRetries counts RETRIES, not total attempts — Attempts=N @@ -98,7 +98,7 @@ func (w *Workflow) retry(opt *RetryOption) func( backOff = b } e := RetryEvent{Attempt: 0} - start := w.Clock.Now() + start := w.clock().Now() return backoff.RetryNotifyWithTimer( func() error { defer func() { @@ -108,11 +108,11 @@ func (w *Workflow) retry(opt *RetryOption) func( ctxPerTry := ctx if opt.TimeoutPerTry > 0 { var cancel context.CancelFunc - ctxPerTry, cancel = w.Clock.WithTimeout(ctx, opt.TimeoutPerTry) + ctxPerTry, cancel = w.clock().WithTimeout(ctx, opt.TimeoutPerTry) defer cancel() } err := do(ctxPerTry) - e.Since = w.Clock.Since(start) + e.Since = w.clock().Since(start) e.Error = err return err }, diff --git a/retry_test.go b/retry_test.go index 9aa9262..ac4d9c1 100644 --- a/retry_test.go +++ b/retry_test.go @@ -47,7 +47,7 @@ func TestRetry(t *testing.T) { newMock := func() *Mock { var ( mockClock = clock.NewMock() - w = &flow.Workflow{Clock: mockClock} + w = &flow.Workflow{Option: flow.WorkflowOption{Clock: mockClock}} mockStep = &MockStep{Started: make(chan struct{})} ) w.Add(flow.Step(mockStep).Retry(func(ro *flow.RetryOption) { @@ -155,7 +155,7 @@ func TestRetry(t *testing.T) { // For this test we need a fresh workflow using Retry(nil) instead. var ( mockClock = clock.NewMock() - w = &flow.Workflow{Clock: mockClock} + w = &flow.Workflow{Option: flow.WorkflowOption{Clock: mockClock}} mockStep = &MockStep{Started: make(chan struct{})} ) w.Add(flow.Step(mockStep).Retry(nil)) diff --git a/state.go b/state.go index 493bfe3..b1f681a 100644 --- a/state.go +++ b/state.go @@ -109,7 +109,7 @@ func (s *State) Option() *StepOption { // // The do parameter is the panic-catching wrapper supplied by the caller — // passing each callback through `do` lets a panicking callback be turned into -// an error when Workflow.DontPanic is true. +// an error when Workflow.Option.DontPanic is true. func (s *State) Before(root context.Context, step Steper, do func(func() error) error) (context.Context, error) { if s.Config == nil || len(s.Config.Before) == 0 { return root, nil diff --git a/step_configuration_test.go b/step_configuration_test.go index 738dbc5..dbc70d0 100644 --- a/step_configuration_test.go +++ b/step_configuration_test.go @@ -179,7 +179,8 @@ func TestBeforeAfter(t *testing.T) { assert.ErrorIs(t, w.Do(context.Background()), assert.AnError) }) t.Run("should call AfterStep even step panics", func(t *testing.T) { - w := &Workflow{DontPanic: true} + dontPanic := true + w := &Workflow{Option: WorkflowOption{DontPanic: &dontPanic}} w.Add( Step(Func("step", func(ctx context.Context) error { panic("panic!") @@ -208,7 +209,8 @@ func TestBeforeAfter(t *testing.T) { assert.True(t, afterRan) }) t.Run("modified context from BeforeStep should still be used even panic happens", func(t *testing.T) { - w := &Workflow{DontPanic: true} + dontPanic := true + w := &Workflow{Option: WorkflowOption{DontPanic: &dontPanic}} noop := NoOp("NoOp") ctx := context.Background() w.Add(Step(noop). @@ -250,18 +252,18 @@ func TestBeforeAfter(t *testing.T) { func TestDefaultOption(t *testing.T) { t.Parallel() - t.Run("DefaultOption applies to all Steps", func(t *testing.T) { + t.Run("StepDefaults applies to all Steps", func(t *testing.T) { w := &Workflow{ - DefaultOption: &StepOption{Timeout: durationPtr(10 * time.Minute)}, + Option: WorkflowOption{StepDefaults: &StepOption{Timeout: durationPtr(10 * time.Minute)}}, } step := NoOp("step") w.Add(Step(step)) assert.Equal(t, 10*time.Minute, *w.StateOf(step).Option().Timeout) }) - t.Run("Step-level option overrides DefaultOption", func(t *testing.T) { + t.Run("Step-level option overrides StepDefaults", func(t *testing.T) { w := &Workflow{ - DefaultOption: &StepOption{Timeout: durationPtr(10 * time.Minute)}, + Option: WorkflowOption{StepDefaults: &StepOption{Timeout: durationPtr(10 * time.Minute)}}, } step := NoOp("step") w.Add(Step(step).Timeout(5 * time.Minute)) diff --git a/workflow.go b/workflow.go index c7c99d6..4682a57 100644 --- a/workflow.go +++ b/workflow.go @@ -38,66 +38,119 @@ import ( // step's Condition then decides whether it actually runs (Running) or is // settled inline as Skipped / Canceled. // -// See StepStatus / Condition for the status state machine. +// All workflow-level configuration lives in [WorkflowOption], exposed via +// Workflow.Option. See [WorkflowOption] for the available fields and the +// parent → child inheritance rules used when a Workflow is run as a step +// inside another Workflow. +// +// # Building sub-workflows +// +// A Workflow can itself be used as a Step inside another Workflow. Three +// patterns, in order of preference: +// +// 1. **Embed flow.Workflow at construction time (recommended).** +// Build the sub-workflow's DAG once, before passing it as a Step. This +// makes the inner Steps visible to introspection (Has / As / HasStep) +// and lets the parent's [WorkflowOption] inherit cleanly via +// [WorkflowOptionReceiver]: +// +// type MyComposite struct{ flow.Workflow } +// func New() *MyComposite { +// c := &MyComposite{} +// c.Add(flow.Step(/* inner steps */)) +// return c +// } +// +// 2. **Build inside Do() with a sync.Once guard.** +// If the sub-workflow needs context only available at run time, build +// it lazily on first Do(). Guard the Add() call with sync.Once so +// re-execution (retries, multiple Do()s) does not re-Add the same Step: +// +// type MyLazy struct{ flow.Workflow; once sync.Once } +// func (m *MyLazy) Do(ctx context.Context) error { +// m.once.Do(func() { m.Add(/* inner steps */) }) +// return m.Workflow.Do(ctx) +// } +// +// 3. **Construct &flow.Workflow{} inline inside Do() (last resort).** +// A throwaway sub-workflow is opaque to the parent: it does not +// participate in [WorkflowOptionReceiver] inheritance unless the host +// step itself implements [WorkflowOptionReceiver] and forwards the +// parent's Option. +// +// **DO NOT** call [Workflow.Add] from inside Do() (or any method +// transitively reachable from Do()) without a sync.Once guard. Doing so +// produces undefined behavior: callbacks accumulate across re-executions, +// duplicate Steps multiply, introspection helpers report multiple matches +// for what should be a single Step, and Mutator dispatch becomes stale. +// See the warning on [Workflow.Add]. // // Per-step configuration: use Step / Steps / Pipe (see step.go). // Composite steps: use Has / As / HasStep (see wrap.go). type Workflow struct { - // Workflow-level scheduling and panic policy. - - MaxConcurrency int // 0 means unlimited; otherwise caps simultaneously-running steps. - DontPanic bool // if true, panics are recovered and surfaced as ErrPanic. - SkipAsError bool // if true, Skipped terminal status counts as a workflow failure. - Clock clock.Clock // injected clock for retries / timeouts (deterministic in tests). - DefaultOption *StepOption // applied as the FIRST option for every Step (per-step Option calls override it). - - // Mutators are evaluated against every step in this workflow before that - // step's first attempt. See [Mutator] / [Mutate]. - Mutators []Mutator - - // Workflow-level interceptors. The base slices are never mutated by - // inheritance (see PrependInterceptors / effective*Interceptors below), - // so multiple Do() runs stay deterministic. - StepInterceptors []StepInterceptor // wrap each step's full lifetime (across retries). - AttemptInterceptors []AttemptInterceptor // wrap each individual attempt (Before → Do → After). - IsolateInterceptors bool // when true and this Workflow runs as a child step, do NOT inherit parent interceptors. + // Option groups all workflow-level configuration (concurrency cap, + // panic policy, skip-as-error, clock, default step options, mutators, + // interceptors, and inheritance opt-out). See [WorkflowOption]. + Option WorkflowOption StepBuilder // embeds the BuildStep memo so Workflow.Add can call BuildStep on new steps once. steps map[Steper]*State // root step → its State (status + StepConfig). - // inheritedStep / inheritedAttempt hold the interceptors a parent workflow - // (or a SubWorkflow-bearing parent step) has prepended for the current run. - // Lifecycle: - // - // 1. Parent writes them BEFORE invoking child.Do() (in executeWithRetry). - // 2. child.Do() reads them while building its effective interceptor chain. - // 3. child.Do() clears them in a defer covering ALL exit paths - // (success, preflight error, panic) so the next run starts fresh. - // - // They are intentionally NOT cleared by the internal reset() (reset() runs at - // the very top of Do() and would wipe the parent's just-written prefix). The - // public Reset() does clear them, since users call Reset() between runs and - // expect a fully-fresh state. - // - // They are never folded into StepInterceptors / AttemptInterceptors so the - // user-supplied base stays untouched and repeated runs do not accumulate. - inheritedStep []StepInterceptor - inheritedAttempt []AttemptInterceptor - statusChange *sync.Cond // signals to the tick loop when a worker terminates. - leaseBucket chan struct{} // bounded-channel "permit pool" enforcing MaxConcurrency; nil means unlimited. + leaseBucket chan struct{} // bounded-channel "permit pool" enforcing Option.MaxConcurrency; nil means unlimited. waitGroup sync.WaitGroup // tracks worker goroutines so Do() can wait for them on exit. isRunning sync.Mutex // single-runner guard: TryLock fails fast if Do/Reset is re-entered. } +// Scalar accessors: handle nil-pointer dereference and runtime defaults. +// All in-code reads of these scalars MUST go through these accessors. + +func (w *Workflow) maxConcurrency() int { + if w.Option.MaxConcurrency == nil { + return 0 + } + return *w.Option.MaxConcurrency +} + +func (w *Workflow) dontPanic() bool { + return w.Option.DontPanic != nil && *w.Option.DontPanic +} + +func (w *Workflow) skipAsError() bool { + return w.Option.SkipAsError != nil && *w.Option.SkipAsError +} + +func (w *Workflow) clock() clock.Clock { + if w.Option.Clock == nil { + return clock.New() + } + return w.Option.Clock +} + // Add wires Builders (Step / Steps / Pipe / BatchPipe / If / Switch / …) into // this Workflow. Repeated calls are additive: a step that appears in multiple // Add() calls has its config merged (upstreams unioned, callbacks/options // concatenated). Returns the Workflow for chaining. // -// If DefaultOption is set, it is prepended to every step's Option list as a -// SEED — so per-step Option calls (Retry, Timeout, When, …) still win. +// If Option.StepDefaults is set, it is prepended to every step's Option +// list as a SEED — so per-step Option calls (Retry, Timeout, When, …) still +// win. +// +// **WARNING — re-entry is forbidden.** Calling Add from inside the same +// Workflow's Do (or any method transitively reachable from Do) is +// FORBIDDEN unless guarded by a sync.Once that ensures Add fires at most +// once across the lifetime of the host step. Unguarded re-entry produces +// undefined behavior, including (a) callbacks accumulating across +// re-executions (BeforeStep / AfterStep / Input fire N times after N +// runs), (b) duplicate Steps registered under the same identity, +// (c) introspection helpers (Has / As / HasStep) reporting multiple +// matches for what should be one Step, and (d) Mutator dispatch becoming +// stale because per-step state was already marked applied. The framework +// does NOT detect this misuse at runtime: a host step's isRunning lock is +// released between retry attempts, so there is no stable signal to +// distinguish legitimate sync.Once-guarded lazy init from accidental +// re-Add. See [Workflow] godoc for the recommended construction patterns. func (w *Workflow) Add(was ...Builder) *Workflow { if w.steps == nil { w.steps = make(map[Steper]*State) @@ -105,9 +158,9 @@ func (w *Workflow) Add(was ...Builder) *Workflow { for _, wa := range was { if wa != nil { for step, config := range wa.AddToWorkflow() { - if w.DefaultOption != nil && config != nil { + if w.Option.StepDefaults != nil && config != nil { config.Option = slices.Insert(config.Option, 0, func(o *StepOption) { - *o = *w.DefaultOption + *o = *w.Option.StepDefaults }) } w.addStep(step, config) @@ -117,17 +170,6 @@ func (w *Workflow) Add(was ...Builder) *Workflow { return w } -// PrependMutators inserts mw at the front of w.Mutators, preserving the -// invariant that propagated parent Mutators run before locally-registered -// child Mutators. Safe to call multiple times; the once-per-step flag on -// State prevents double application. -func (w *Workflow) PrependMutators(mw []Mutator) { - if len(mw) == 0 { - return - } - w.Mutators = append(append([]Mutator{}, mw...), w.Mutators...) -} - // addStep registers `step` as a root in this workflow if it isn't already // reachable from one. If the new step embeds previously-registered roots, those // roots are demoted (their state is folded into the new root's state) so the @@ -176,19 +218,19 @@ func (w *Workflow) addStep(step Steper, config *StepConfig) { } } -// applyMutators runs each Mutator in w.Mutators against step. For every match, +// applyMutators runs each Mutator in w.Option.Mutators against step. For every match, // the returned Builder's config keyed on the matched layer is merged into the // state of step (the wrapper key in this workflow). Called once per step, // just before its first attempt. func (w *Workflow) applyMutators(ctx context.Context, step Steper, state *State) { - if len(w.Mutators) == 0 { + if len(w.Option.Mutators) == 0 { return } do := func(fn func() error) error { return fn() } - if w.DontPanic { + if w.dontPanic() { do = catchPanicAsError } - for _, m := range w.Mutators { + for _, m := range w.Option.Mutators { err := do(func() error { matched, target, b := m.applyTo(ctx, step) if !matched || b == nil { @@ -342,97 +384,102 @@ func (w *Workflow) IsTerminated() bool { return true } -// Reset prepares the Workflow for a fresh run from outside (the user's POV). -// It rejects with ErrWorkflowIsRunning if a Do call is currently in flight. +// Reset rewinds every Step's status back to Pending so the Workflow can be +// Do()-ed again. Reset rejects with ErrWorkflowIsRunning if a Do call is +// currently in flight. +// +// Reset does NOT modify w.steps (the set of Steps registered via Add) — a +// Workflow built once can be Do()-ed any number of times via Reset/Do +// cycles, with the same DAG each time. To start from an empty set of Steps, +// allocate a new Workflow. // -// Difference vs the internal reset(): Reset() ALSO clears the inherited -// interceptor slices set by a parent during a previous run. The internal -// reset() must NOT clear them — see the inheritedStep / inheritedAttempt -// lifecycle docs above. +// Reset does NOT modify w.Option either. Cross-run accumulation of +// parent-inherited contributions is prevented by the snapshot/restore in +// Do(), not by Reset. Calling Reset between runs is therefore optional from +// an Option-isolation standpoint; its purpose is purely to rewind per-step +// status for re-execution. func (w *Workflow) Reset() error { if !w.isRunning.TryLock() { return ErrWorkflowIsRunning } defer w.isRunning.Unlock() w.reset() - w.inheritedStep = nil - w.inheritedAttempt = nil return nil } // reset is the per-Do internal reset: clear all step results back to Pending, -// install a fresh statusChange Cond, ensure Clock is set, and re-allocate the -// concurrency lease bucket sized for MaxConcurrency. +// install a fresh statusChange Cond, and re-allocate the concurrency lease +// bucket sized for Option.MaxConcurrency. // -// Crucially, this does NOT touch inheritedStep / inheritedAttempt — those were -// just written by the parent before invoking Do() and must survive into the -// run. +// reset does NOT touch w.Option: parent → child Option inheritance is +// preserved by the snapshot/restore in Do() (see Workflow.Do). func (w *Workflow) reset() { for _, state := range w.steps { state.SetStepResult(StepResult{Status: Pending}) } - if w.Clock == nil { - w.Clock = clock.New() - } - w.statusChange = sync.NewCond(new(sync.Mutex)) - if w.MaxConcurrency > 0 { - // Buffered channel as a sized permit pool: a Step takes a slot via - // `w.leaseBucket <- struct{}{}` to begin running, and frees it via - // `<-w.leaseBucket` when it terminates. - w.leaseBucket = make(chan struct{}, w.MaxConcurrency) + w.statusChange = sync.NewCond(&sync.Mutex{}) + if mc := w.maxConcurrency(); mc > 0 { + w.leaseBucket = make(chan struct{}, mc) + } else { + w.leaseBucket = nil } } -// PrependInterceptors implements InterceptorReceiver on Workflow itself, so a -// Workflow used directly as a step (or via SubWorkflow) can inherit -// interceptors from its parent for the duration of one run. With -// IsolateInterceptors == true the call is a no-op (the workflow uses only -// its own configured interceptors). +// InheritOption implements [WorkflowOptionReceiver]. A parent Workflow calls +// this method on each sub-workflow root step's first receiver (located via +// findOptionReceiver) ONCE in the parent's Do() prologue. // -// The inherited slices are stored separately from StepInterceptors / -// AttemptInterceptors so the user-supplied base is never mutated and repeated -// runs do not accumulate inherited entries. -func (w *Workflow) PrependInterceptors(step []StepInterceptor, attempt []AttemptInterceptor) { - if w.IsolateInterceptors { - return +// Merge rules: +// - if w.Option.DontInherit is true, this is a no-op (restore is still +// non-nil but does nothing); +// - for each scalar pointer (MaxConcurrency, DontPanic, SkipAsError) and +// interface/pointer (Clock, StepDefaults) field: if the child's field is +// nil, the parent's value is copied in; non-nil child fields are +// preserved; +// - for each slice (Mutators, StepInterceptors, AttemptInterceptors): a +// fresh slice equal to parent ++ child replaces the child's field. +// +// The returned restore function rewinds w.Option to its pre-InheritOption +// shape; the parent MUST defer it on every Do() exit path so the child does +// not retain inherited state across runs. restore is always non-nil and is +// safe to call multiple times (idempotent). +func (w *Workflow) InheritOption(parent WorkflowOption) (restore func()) { + prev := w.Option + if w.Option.DontInherit { + return func() { w.Option = prev } } - if len(step) > 0 { - merged := make([]StepInterceptor, 0, len(step)+len(w.inheritedStep)) - merged = append(merged, step...) - merged = append(merged, w.inheritedStep...) - w.inheritedStep = merged + if w.Option.MaxConcurrency == nil { + w.Option.MaxConcurrency = parent.MaxConcurrency } - if len(attempt) > 0 { - mergedA := make([]AttemptInterceptor, 0, len(attempt)+len(w.inheritedAttempt)) - mergedA = append(mergedA, attempt...) - mergedA = append(mergedA, w.inheritedAttempt...) - w.inheritedAttempt = mergedA + if w.Option.DontPanic == nil { + w.Option.DontPanic = parent.DontPanic } + if w.Option.SkipAsError == nil { + w.Option.SkipAsError = parent.SkipAsError + } + if w.Option.Clock == nil { + w.Option.Clock = parent.Clock + } + if w.Option.StepDefaults == nil { + w.Option.StepDefaults = parent.StepDefaults + } + w.Option.Mutators = prependSlice(parent.Mutators, w.Option.Mutators) + w.Option.StepInterceptors = prependSlice(parent.StepInterceptors, w.Option.StepInterceptors) + w.Option.AttemptInterceptors = prependSlice(parent.AttemptInterceptors, w.Option.AttemptInterceptors) + return func() { w.Option = prev } } -// effectiveStepInterceptors returns the chain to invoke for THIS run: the -// inherited prefix (from a parent, if any) followed by this workflow's own -// configured base. The result is computed each call and is never written -// back to either field. +// effectiveStepInterceptors returns the chain to invoke for THIS run. With +// parent → child Option inheritance now performed eagerly in Do()'s prologue +// (writing into w.Option.StepInterceptors directly), the effective chain IS +// simply w.Option.StepInterceptors. func (w *Workflow) effectiveStepInterceptors() []StepInterceptor { - if len(w.inheritedStep) == 0 { - return w.StepInterceptors - } - out := make([]StepInterceptor, 0, len(w.inheritedStep)+len(w.StepInterceptors)) - out = append(out, w.inheritedStep...) - out = append(out, w.StepInterceptors...) - return out + return w.Option.StepInterceptors } // effectiveAttemptInterceptors mirrors effectiveStepInterceptors for AttemptInterceptors. func (w *Workflow) effectiveAttemptInterceptors() []AttemptInterceptor { - if len(w.inheritedAttempt) == 0 { - return w.AttemptInterceptors - } - out := make([]AttemptInterceptor, 0, len(w.inheritedAttempt)+len(w.AttemptInterceptors)) - out = append(out, w.inheritedAttempt...) - out = append(out, w.AttemptInterceptors...) - return out + return w.Option.AttemptInterceptors } // Do runs the Workflow synchronously: it spawns a goroutine for every @@ -454,14 +501,16 @@ func (w *Workflow) Do(ctx context.Context) error { } defer w.isRunning.Unlock() - // Clear inherited interceptors set by a parent during this run on EVERY - // exit path, so a subsequent run (under any parent, or standalone) starts - // fresh and PrependInterceptors does not accumulate. Defer ensures even - // early exits (Empty, preflight failure, panic) reset state. - defer func() { - w.inheritedStep = nil - w.inheritedAttempt = nil - }() + // Snapshot Option so any InheritOption writes performed below (and + // transitively by nested workflows during their own Do() prologue) are + // reverted at the end of THIS Do() call. The snapshot is a shallow copy; + // this is correct because: + // - InheritOption only overwrites nil pointer fields with the parent's + // pointer values (not mutating the parent's targets), and + // - prependSlice always allocates a fresh slice, so neither the + // snapshot's slice header nor the parent's slice is mutated. + optSnapshot := w.Option + defer func() { w.Option = optSnapshot }() // Nothing to do. if w.Empty() { @@ -475,6 +524,29 @@ func (w *Workflow) Do(ctx context.Context) error { return err } + // Propagate w.Option into every sub-workflow root step exactly once, + // BEFORE the tick loop dispatches anything. Receivers are located via + // pre-order Unwrap walk so a sub-workflow may be wrapped in a Steper-only + // wrapper (e.g. NamedStep) without losing inheritance. + // + // Each receiver's InheritOption returns a `restore` func that we MUST + // invoke on every Do() exit path so the child's Option is rewound to + // its pre-InheritOption shape. Without this, repeated Do() runs of the + // same parent would accumulate parent contributions on the child. + var childRestores []func() + for step := range w.steps { + if recv := findOptionReceiver(step); recv != nil { + if r := recv.InheritOption(w.Option); r != nil { + childRestores = append(childRestores, r) + } + } + } + defer func() { + for _, r := range childRestores { + r() + } + }() + // Tick loop: each time a step terminates it Signal()s the cond, we wake // up and tick() again. Inline-settled steps may unblock more steps within // the same tick (no signal needed for those — see tick()). @@ -495,10 +567,10 @@ func (w *Workflow) Do(ctx context.Context) error { for step, state := range w.steps { err[step] = state.GetStepResult() } - if w.SkipAsError && err.AllSucceeded() { + if w.skipAsError() && err.AllSucceeded() { return nil } - if !w.SkipAsError && err.AllSucceededOrSkipped() { + if !w.skipAsError() && err.AllSucceededOrSkipped() { return nil } return err @@ -631,16 +703,10 @@ func (w *Workflow) tick(ctx context.Context) bool { // Option/Before/After contributions from Mutators are visible to // the rest of this iteration. The flag is flipped BEFORE the merge // runs so that a panicking mutator (caught by applyMutators's - // recover when DontPanic is set) does not cause re-entry on the + // recover when Option.DontPanic is set) does not cause re-entry on the // next tick. if !state.MutatorsApplied() { state.SetMutatorsApplied() - // Propagate this workflow's Mutators into nested workflows so - // they can apply against inner steps when those steps are - // scheduled by the inner workflow. - if recv, ok := step.(MutatorReceiver); ok && len(w.Mutators) > 0 { - recv.PrependMutators(w.Mutators) - } w.applyMutators(ctx, step, state) // If applyMutators caught a panic (DontPanic), the state already // holds an error. Settle the step inline as Failed so it never @@ -649,7 +715,7 @@ func (w *Workflow) tick(ctx context.Context) bool { state.SetStepResult(StepResult{ Status: Failed, Err: err, - FinishedAt: w.Clock.Now(), + FinishedAt: w.clock().Now(), }) progressed = true continue @@ -665,7 +731,7 @@ func (w *Workflow) tick(ctx context.Context) bool { if nextStatus := cond(ctx, ups); nextStatus.IsTerminated() { state.SetStepResult(StepResult{ Status: nextStatus, - FinishedAt: w.Clock.Now(), + FinishedAt: w.clock().Now(), }) progressed = true continue @@ -708,7 +774,7 @@ func (ex *stepExecution) run(ctx context.Context) { // Condition (terminal results were settled inline) and set the status to // Running, so we can dive straight in. // - // When DontPanic is true, EVERY interceptor invocation is wrapped in + // When Option.DontPanic is true, EVERY interceptor invocation is wrapped in // catchPanicAsError so a panicking user interceptor cannot crash the // process or leave the lease unreleased / status unsignalled. stepNext := func(ctx context.Context) error { return ex.executeWithRetry(ctx) } @@ -721,7 +787,7 @@ func (ex *stepExecution) run(ctx context.Context) { ic := stepICs[i] nextLocal := stepNext stepNext = func(ctx context.Context) error { - if ex.w.DontPanic { + if ex.w.dontPanic() { return catchPanicAsError(func() error { return ic.InterceptStep(ctx, ex.step, nextLocal) }) @@ -748,7 +814,7 @@ func (ex *stepExecution) run(ctx context.Context) { ex.state.SetStepResult(StepResult{ Status: status, Err: err, - FinishedAt: ex.w.Clock.Now(), + FinishedAt: ex.w.clock().Now(), }) // Release the lease BEFORE signalling, so when the tick loop wakes up it @@ -765,17 +831,13 @@ func (ex *stepExecution) run(ctx context.Context) { func (ex *stepExecution) executeWithRetry(ctx context.Context) error { option := ex.state.Option() - if recv := findInterceptorReceiver(ex.step); recv != nil { - recv.PrependInterceptors(ex.w.effectiveStepInterceptors(), ex.w.effectiveAttemptInterceptors()) - } - attemptChain := ex.buildAttemptChain() var notAfter time.Time if option != nil && option.Timeout != nil { - notAfter = ex.w.Clock.Now().Add(*option.Timeout) + notAfter = ex.w.clock().Now().Add(*option.Timeout) var cancel func() - ctx, cancel = ex.w.Clock.WithDeadline(ctx, notAfter) + ctx, cancel = ex.w.clock().WithDeadline(ctx, notAfter) defer cancel() } @@ -797,7 +859,7 @@ func (ex *stepExecution) buildAttemptChain() func(context.Context) error { ic := attemptICs[i] nextLocal := chain chain = func(ctx context.Context) error { - if ex.w.DontPanic { + if ex.w.dontPanic() { return catchPanicAsError(func() error { return ic.InterceptAttempt(ctx, ex.step, ex.attempt, nextLocal) }) @@ -814,14 +876,14 @@ func (ex *stepExecution) buildAttemptChain() func(context.Context) error { // runAttempt executes one attempt: Before callbacks → Do → After callbacks. // -// The `do` wrapper is either a direct invocation, or — when DontPanic is true +// The `do` wrapper is either a direct invocation, or — when Option.DontPanic is true // — catchPanicAsError, which converts a panic to an ErrPanic-tagged error. // The Before chain may swap the context that is threaded into Do (and the // After chain). After callbacks always run, even if Before or Do failed; they // receive the latest error and can transform it. func (ex *stepExecution) runAttempt(ctx context.Context) error { do := func(fn func() error) error { return fn() } - if ex.w.DontPanic { + if ex.w.dontPanic() { do = catchPanicAsError } @@ -841,7 +903,7 @@ func (ex *stepExecution) runAttempt(ctx context.Context) error { // lease takes one slot from the concurrency permit pool. Returns true if the // caller may now run, or false if the pool is full (the tick loop will retry -// on the next signal). When MaxConcurrency is unset (leaseBucket == nil), the +// on the next signal). When Option.MaxConcurrency is unset (leaseBucket == nil), the // answer is always true. func (w *Workflow) lease() bool { if w.leaseBucket == nil { @@ -856,7 +918,7 @@ func (w *Workflow) lease() bool { } // unlease returns one slot to the concurrency permit pool, or is a no-op if -// MaxConcurrency is unset. +// Option.MaxConcurrency is unset. func (w *Workflow) unlease() { if w.leaseBucket != nil { <-w.leaseBucket @@ -889,18 +951,27 @@ func catchPanicAsError(f func() error) error { } // SubWorkflow makes any user struct behave as a Step that contains a -// Workflow. Embed it in your own struct to get Add/Do/Reset and the -// InterceptorReceiver delegation for free: +// Workflow. +// +// Deprecated: Embed [Workflow] directly in your own struct instead. +// SubWorkflow will be removed in the next major version of go-workflow. +// The recommended pattern is: // // type MyStep struct { -// flow.SubWorkflow +// flow.Workflow // } // -// func (s *MyStep) BuildStep() { +// func NewMyStep() *MyStep { +// s := &MyStep{} // s.Add( // flow.Step(/* stepX */), // ) +// return s // } +// +// A Workflow embedded this way also satisfies [WorkflowOptionReceiver], so +// parent → child Option inheritance continues to work without any extra +// boilerplate. type SubWorkflow struct{ w Workflow } func (s *SubWorkflow) Unwrap() Steper { return &s.w } @@ -916,16 +987,9 @@ func (s *SubWorkflow) Do(ctx context.Context) error { return s.w.Do(ctx) } // major version of go-workflow. func (s *SubWorkflow) Reset() { s.w = Workflow{} } -// PrependMutators forwards mw to the inner workflow. Implements [MutatorReceiver] -// so parent workflows can propagate Mutators into a sub-workflow before its -// scheduling pass begins. -func (s *SubWorkflow) PrependMutators(mw []Mutator) { - s.w.PrependMutators(mw) -} - -// PrependInterceptors satisfies InterceptorReceiver by delegating to the -// embedded Workflow — so a parent workflow's interceptors flow into the -// SubWorkflow's inner Workflow exactly as if it were used directly. -func (s *SubWorkflow) PrependInterceptors(step []StepInterceptor, attempt []AttemptInterceptor) { - s.w.PrependInterceptors(step, attempt) +// InheritOption delegates to the inner Workflow's InheritOption so a parent +// workflow's Option propagates into the SubWorkflow's inner Workflow during +// the deprecation window. Implements [WorkflowOptionReceiver]. +func (s *SubWorkflow) InheritOption(parent WorkflowOption) (restore func()) { + return s.w.InheritOption(parent) } diff --git a/workflow_mutator_test.go b/workflow_mutator_test.go index cff58ab..723f569 100644 --- a/workflow_mutator_test.go +++ b/workflow_mutator_test.go @@ -13,33 +13,36 @@ type wfFoo struct{} func (*wfFoo) Do(context.Context) error { return nil } -func TestWorkflow_PrependMutators(t *testing.T) { +func TestWorkflow_InheritOption_Mutators(t *testing.T) { m1 := flow.Mutate[*wfFoo](func(ctx context.Context, f *wfFoo) flow.Builder { return nil }) m2 := flow.Mutate[*wfFoo](func(ctx context.Context, f *wfFoo) flow.Builder { return nil }) - w := &flow.Workflow{Mutators: []flow.Mutator{m2}} - w.PrependMutators([]flow.Mutator{m1}) + w := &flow.Workflow{Option: flow.WorkflowOption{Mutators: []flow.Mutator{m2}}} + // Simulate a parent workflow injecting m1 as a prepended Mutator via + // InheritOption (parent → child propagation contract). + w.InheritOption(flow.WorkflowOption{Mutators: []flow.Mutator{m1}}) - assert.Len(t, w.Mutators, 2) + assert.Len(t, w.Option.Mutators, 2) } -func TestWorkflow_PrependMutatorsNilOrEmpty(t *testing.T) { +func TestWorkflow_InheritOption_NilOrEmptyMutators(t *testing.T) { w := &flow.Workflow{} - w.PrependMutators(nil) - assert.Empty(t, w.Mutators) - w.PrependMutators([]flow.Mutator{}) - assert.Empty(t, w.Mutators) + w.InheritOption(flow.WorkflowOption{}) // empty parent — no contribution + assert.Empty(t, w.Option.Mutators) + w.InheritOption(flow.WorkflowOption{Mutators: []flow.Mutator{}}) + assert.Empty(t, w.Option.Mutators) } -func TestSubWorkflow_PrependMutators(t *testing.T) { +func TestSubWorkflow_InheritOption(t *testing.T) { type sub struct{ flow.SubWorkflow } s := &sub{} m := flow.Mutate[*wfFoo](func(ctx context.Context, f *wfFoo) flow.Builder { return nil }) - // MutatorReceiver must be implemented - var _ flow.MutatorReceiver = s - s.PrependMutators([]flow.Mutator{m}) - // No panic / no error → behaviour verified by integration test in Task 7 + // WorkflowOptionReceiver must be implemented (SubWorkflow delegates). + var _ flow.WorkflowOptionReceiver = s + s.InheritOption(flow.WorkflowOption{Mutators: []flow.Mutator{m}}) + // Behaviour verified by integration tests; constructing this scenario + // exercises the deprecation-window delegation path. } type wfGreet struct { @@ -53,14 +56,16 @@ func TestMutator_mergesInputBeforeFirstAttempt(t *testing.T) { called := 0 g := &wfGreet{Greeting: "Hi"} w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { - called++ - return flow.Step(gg).Input(func(_ context.Context, gg *wfGreet) error { - gg.Who = "world" - return nil - }) - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { + called++ + return flow.Step(gg).Input(func(_ context.Context, gg *wfGreet) error { + gg.Who = "world" + return nil + }) + }), + }, }, } w.Add(flow.Step(g)) @@ -89,16 +94,18 @@ func TestMutator_runsExactlyOnceAcrossRetries(t *testing.T) { inputCalls := 0 f := &wfFlaky{failTill: 2} w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*wfFlaky](func(ctx context.Context, ff *wfFlaky) flow.Builder { - mutatorCalls++ - return flow.Step(ff). - Retry(func(o *flow.RetryOption) { o.Attempts = 3 }). - Input(func(_ context.Context, _ *wfFlaky) error { - inputCalls++ - return nil - }) - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*wfFlaky](func(ctx context.Context, ff *wfFlaky) flow.Builder { + mutatorCalls++ + return flow.Step(ff). + Retry(func(o *flow.RetryOption) { o.Attempts = 3 }). + Input(func(_ context.Context, _ *wfFlaky) error { + inputCalls++ + return nil + }) + }), + }, }, } w.Add(flow.Step(f)) @@ -112,7 +119,7 @@ func TestMutator_runsExactlyOnceAcrossRetries(t *testing.T) { func TestMutator_nilSliceIsNoOp(t *testing.T) { g := &wfGreet{Greeting: "Hi", Who: "Bob"} - w := &flow.Workflow{} // Mutators == nil + w := &flow.Workflow{} // Option.Mutators == nil w.Add(flow.Step(g)) assert.NoError(t, w.Do(context.Background())) assert.Equal(t, "Bob", g.Who) @@ -124,7 +131,9 @@ type wfComposite struct { } func (c *wfComposite) Do(ctx context.Context) error { - // Lazy build inside Do — replaces BuildStep pattern. + // Lazy build inside Do — replaces BuildStep pattern. NOTE: in + // production code this MUST be guarded by sync.Once when the host step + // can be re-executed; this test runs Do once so the bare Add is fine. c.Add(flow.Step(&c.Inner)) return c.SubWorkflow.Do(ctx) } @@ -132,18 +141,20 @@ func (c *wfComposite) Do(ctx context.Context) error { func TestMutator_reachesIntoSubWorkflow(t *testing.T) { c := &wfComposite{Inner: wfGreet{Greeting: "Hello"}} w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*wfGreet](func(ctx context.Context, g *wfGreet) flow.Builder { - g.Who = "world" - return nil - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*wfGreet](func(ctx context.Context, g *wfGreet) flow.Builder { + g.Who = "world" + return nil + }), + }, }, } w.Add(flow.Step(c)) err := w.Do(context.Background()) assert.NoError(t, err) - assert.Equal(t, "world", c.Inner.Who, "parent Mutator must reach inner step via PrependMutators") + assert.Equal(t, "world", c.Inner.Who, "parent Mutator must reach inner step via WorkflowOptionReceiver.InheritOption") } func TestMutator_reachesIntoNestedWorkflow(t *testing.T) { @@ -151,11 +162,13 @@ func TestMutator_reachesIntoNestedWorkflow(t *testing.T) { innerW := new(flow.Workflow).Add(flow.Step(innerG)) w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*wfGreet](func(ctx context.Context, g *wfGreet) flow.Builder { - g.Who = "world" - return nil - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*wfGreet](func(ctx context.Context, g *wfGreet) flow.Builder { + g.Who = "world" + return nil + }), + }, }, } w.Add(flow.Step(innerW)) @@ -169,12 +182,14 @@ func TestMutator_reachesLazilyAddedInnerStep(t *testing.T) { c := &wfComposite{Inner: wfGreet{Greeting: "Yo"}} called := 0 w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*wfGreet](func(ctx context.Context, g *wfGreet) flow.Builder { - called++ - g.Who = "lazy" - return nil - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*wfGreet](func(ctx context.Context, g *wfGreet) flow.Builder { + called++ + g.Who = "lazy" + return nil + }), + }, }, } w.Add(flow.Step(c)) @@ -187,13 +202,15 @@ func TestMutator_planInputBeforeMutatorInput(t *testing.T) { g := &wfGreet{Greeting: "Hi"} order := []string{} w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { - return flow.Step(gg).Input(func(_ context.Context, _ *wfGreet) error { - order = append(order, "mutator") - return nil - }) - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { + return flow.Step(gg).Input(func(_ context.Context, _ *wfGreet) error { + order = append(order, "mutator") + return nil + }) + }), + }, }, } w.Add( @@ -217,7 +234,7 @@ func TestMutator_multipleMutatorsRunInSliceOrder(t *testing.T) { }) }) } - w := &flow.Workflow{Mutators: []flow.Mutator{mk("m1"), mk("m2")}} + w := &flow.Workflow{Option: flow.WorkflowOption{Mutators: []flow.Mutator{mk("m1"), mk("m2")}}} w.Add(flow.Step(g)) assert.NoError(t, w.Do(context.Background())) assert.Equal(t, []string{"m1", "m2"}, order) @@ -227,11 +244,13 @@ func TestMutator_mergeAtFirstScheduling_NotAtAdd(t *testing.T) { g := &wfGreet{} called := 0 w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { - called++ - return nil - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { + called++ + return nil + }), + }, }, } w.Add(flow.Step(g)) @@ -243,11 +262,13 @@ func TestMutator_mergeAtFirstScheduling_NotAtAdd(t *testing.T) { func TestMutator_matchesThroughNameWrapper(t *testing.T) { g := &wfGreet{Greeting: "Hi"} w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { - gg.Who = "world" - return nil - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { + gg.Who = "world" + return nil + }), + }, }, } w.Add(flow.Name(g, "named-greet")) @@ -263,13 +284,15 @@ func TestMutator_receivesWorkflowCtx(t *testing.T) { g := &wfGreet{} got := "" w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { - if v, ok := ctx.Value(wfCtxKey).(string); ok { - got = v - } - return nil - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { + if v, ok := ctx.Value(wfCtxKey).(string); ok { + got = v + } + return nil + }), + }, }, } w.Add(flow.Step(g)) @@ -282,17 +305,19 @@ func TestMutator_unrelatedBuilderEntryIgnored(t *testing.T) { g := &wfGreet{Greeting: "Hi", Who: "Bob"} other := &wfGreet{Who: "untouched"} w := &flow.Workflow{ - Mutators: []flow.Mutator{ - flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { - if gg == g { - // Mistakenly return a Builder keyed on `other` instead of `gg`. - return flow.Step(other).Input(func(_ context.Context, o *wfGreet) error { - o.Who = "stolen" - return nil - }) - } - return nil - }), + Option: flow.WorkflowOption{ + Mutators: []flow.Mutator{ + flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { + if gg == g { + // Mistakenly return a Builder keyed on `other` instead of `gg`. + return flow.Step(other).Input(func(_ context.Context, o *wfGreet) error { + o.Who = "stolen" + return nil + }) + } + return nil + }), + }, }, } w.Add(flow.Step(g)) // only g is in the workflow @@ -302,12 +327,15 @@ func TestMutator_unrelatedBuilderEntryIgnored(t *testing.T) { func TestMutator_panicCaughtWhenDontPanic(t *testing.T) { g := &wfGreet{} + dontPanic := true w := &flow.Workflow{ - DontPanic: true, - Mutators: []flow.Mutator{ - flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { - panic("boom") - }), + Option: flow.WorkflowOption{ + DontPanic: &dontPanic, + Mutators: []flow.Mutator{ + flow.Mutate[*wfGreet](func(ctx context.Context, gg *wfGreet) flow.Builder { + panic("boom") + }), + }, }, } w.Add(flow.Step(g)) diff --git a/workflow_option.go b/workflow_option.go new file mode 100644 index 0000000..e602f59 --- /dev/null +++ b/workflow_option.go @@ -0,0 +1,120 @@ +package flow + +import "github.com/benbjohnson/clock" + +// WorkflowOption groups all configuration that a Workflow exposes to its +// caller AND inherits from a parent Workflow when used as a sub-workflow step. +// +// Scalar fields are pointers so that "unset" (nil) and "explicit zero value" +// are distinguishable. On parent → child Option inheritance, a nil pointer on +// the child means "inherit from parent (or use the runtime default)"; a +// non-nil pointer is the child's own choice and wins over the parent. +// +// Slice fields are not pointer-typed; on inheritance, the parent's slice is +// prepended to the child's slice (parent contributions run first), preserving +// the existing Mutator and Interceptor propagation semantics. +// +// Mutating a Workflow's Option after Do() has started is undefined behavior. +type WorkflowOption struct { + // MaxConcurrency caps simultaneously-running Steps. nil or 0 means + // unlimited; a positive value installs a buffered-channel lease bucket. + MaxConcurrency *int + + // DontPanic, if non-nil and true, recovers panics in Step Do / Input / + // BeforeStep / AfterStep callbacks and surfaces them as ErrPanic. + DontPanic *bool + + // SkipAsError, if non-nil and true, counts Skipped terminal status as a + // workflow failure (so Do returns ErrWorkflow even if no Step actually + // failed). + SkipAsError *bool + + // Clock is the time source used for Step timeouts, per-try timeouts in + // the retry loop, and backoff waits. nil means real wall clock + // (clock.New()). Inject a clock.Mock in tests to control time. + Clock clock.Clock + + // StepDefaults, if non-nil, is prepended as the FIRST option to every + // Step's Option list as a baseline. Per-step Option calls (Retry, + // Timeout, When, …) still win over it. + StepDefaults *StepOption + + // Mutators is the workflow-level list of cross-cutting step Mutators. + // On inheritance, the parent's Mutators are prepended (parent contributions + // run first within the child). + Mutators []Mutator + + // StepInterceptors wraps each Step's full lifetime (across retries). + // On inheritance, the parent's slice is prepended to the child's. + StepInterceptors []StepInterceptor + + // AttemptInterceptors wraps each individual attempt (Before → Do → After). + // On inheritance, the parent's slice is prepended to the child's. + AttemptInterceptors []AttemptInterceptor + + // DontInherit, when true on a sub-workflow Workflow, makes InheritOption + // a no-op: nothing flows in from the parent. Replaces the previous + // IsolateInterceptors flag and now governs the entire WorkflowOption, + // not just interceptors. Naming aligns with DontPanic. + DontInherit bool +} + +// WorkflowOptionReceiver is implemented by any Step that contains a +// sub-workflow. The parent's Do() prologue locates the nearest receiver in +// each root step's Unwrap chain and calls InheritOption ONCE before any +// scheduling begins, so the child's Do() observes the merged Option. +// +// InheritOption mutates the receiver's Option in place. To prevent +// cross-Do() accumulation (the child being re-run later observing the +// previous parent's contributions), implementations MUST return a non-nil +// `restore` function that the parent invokes on every Do() exit path to +// rewind the receiver's Option to its pre-InheritOption shape. +// +// *Workflow itself implements this interface; users get inheritance for +// free by embedding flow.Workflow in their own Step type. +type WorkflowOptionReceiver interface { + InheritOption(parent WorkflowOption) (restore func()) +} + +// prependSlice returns a fresh slice equal to parent ++ child. It MUST NOT +// mutate either input. The fresh backing array is what allows callers to +// snapshot-and-restore a WorkflowOption with a shallow copy: parent and +// child slice headers retain their original backing arrays. +func prependSlice[T any](parent, child []T) []T { + if len(parent) == 0 { + if len(child) == 0 { + return nil + } + out := make([]T, len(child)) + copy(out, child) + return out + } + if len(child) == 0 { + out := make([]T, len(parent)) + copy(out, parent) + return out + } + out := make([]T, 0, len(parent)+len(child)) + out = append(out, parent...) + out = append(out, child...) + return out +} + +// findOptionReceiver returns the first WorkflowOptionReceiver in the Step +// tree rooted at s, walking via Unwrap in pre-order. Returns nil if none of +// the unwrapped Steps satisfies WorkflowOptionReceiver. +// +// This lets a sub-workflow be wrapped in a Steper-only wrapper (e.g. +// NamedStep, which embeds the Steper interface and therefore does not +// promote InheritOption) without losing parent-Option inheritance. +func findOptionReceiver(s Steper) WorkflowOptionReceiver { + var found WorkflowOptionReceiver + Traverse(s, func(s Steper, _ []Steper) TraverseDecision { + if r, ok := s.(WorkflowOptionReceiver); ok { + found = r + return TraverseStop + } + return TraverseContinue + }) + return found +} diff --git a/workflow_option_inherit_test.go b/workflow_option_inherit_test.go new file mode 100644 index 0000000..df592c1 --- /dev/null +++ b/workflow_option_inherit_test.go @@ -0,0 +1,223 @@ +package flow + +import ( + "context" + "testing" + + "github.com/benbjohnson/clock" + "github.com/stretchr/testify/assert" +) + +// ptr is a tiny helper local to this test file. Real users would write &v or +// use Go 1.26's `new(value)`; the test file uses ptr only to keep call sites +// short. +func ptr[T any](v T) *T { return &v } + +func TestInheritOption_Scalars(t *testing.T) { + parent := WorkflowOption{ + MaxConcurrency: ptr(4), + DontPanic: ptr(true), + SkipAsError: ptr(true), + Clock: clock.NewMock(), + StepDefaults: &StepOption{}, + } + + t.Run("nil scalar -> parent value used", func(t *testing.T) { + w := &Workflow{} + w.InheritOption(parent) + assert.Equal(t, parent.MaxConcurrency, w.Option.MaxConcurrency) + assert.Equal(t, parent.DontPanic, w.Option.DontPanic) + assert.Equal(t, parent.SkipAsError, w.Option.SkipAsError) + assert.Equal(t, parent.Clock, w.Option.Clock) + assert.Equal(t, parent.StepDefaults, w.Option.StepDefaults) + }) + + t.Run("non-nil child wins", func(t *testing.T) { + childMax := 9 + childDontPanic := false + childSkip := false + childClock := clock.New() + childDefaults := &StepOption{} + w := &Workflow{Option: WorkflowOption{ + MaxConcurrency: &childMax, + DontPanic: &childDontPanic, + SkipAsError: &childSkip, + Clock: childClock, + StepDefaults: childDefaults, + }} + w.InheritOption(parent) + assert.Equal(t, 9, *w.Option.MaxConcurrency) + assert.Equal(t, false, *w.Option.DontPanic) + assert.Equal(t, false, *w.Option.SkipAsError) + assert.Same(t, childClock, w.Option.Clock) + assert.Same(t, childDefaults, w.Option.StepDefaults) + }) + + t.Run("explicit-zero child wins (distinguished from inherit)", func(t *testing.T) { + // Pointer to zero IS the user saying "child wins with value 0". + zeroMax := 0 + zeroDontPanic := false + w := &Workflow{Option: WorkflowOption{ + MaxConcurrency: &zeroMax, + DontPanic: &zeroDontPanic, + }} + w.InheritOption(parent) + assert.NotNil(t, w.Option.MaxConcurrency) + assert.Equal(t, 0, *w.Option.MaxConcurrency, "explicit-zero pointer wins over parent's 4") + assert.NotNil(t, w.Option.DontPanic) + assert.Equal(t, false, *w.Option.DontPanic, "explicit-false wins over parent's true") + }) +} + +func TestInheritOption_Slices(t *testing.T) { + mA := Mutate[*Workflow](func(context.Context, *Workflow) Builder { return nil }) + mB := Mutate[*Workflow](func(context.Context, *Workflow) Builder { return nil }) + sA := StepInterceptorFunc(func(ctx context.Context, _ Steper, next func(context.Context) error) error { + return next(ctx) + }) + sB := StepInterceptorFunc(func(ctx context.Context, _ Steper, next func(context.Context) error) error { + return next(ctx) + }) + aA := AttemptInterceptorFunc(func(ctx context.Context, _ Steper, _ uint64, next func(context.Context) error) error { + return next(ctx) + }) + aB := AttemptInterceptorFunc(func(ctx context.Context, _ Steper, _ uint64, next func(context.Context) error) error { + return next(ctx) + }) + + parent := WorkflowOption{ + Mutators: []Mutator{mA}, + StepInterceptors: []StepInterceptor{sA}, + AttemptInterceptors: []AttemptInterceptor{aA}, + } + + t.Run("parent prepended to child for each slice field", func(t *testing.T) { + w := &Workflow{Option: WorkflowOption{ + Mutators: []Mutator{mB}, + StepInterceptors: []StepInterceptor{sB}, + AttemptInterceptors: []AttemptInterceptor{aB}, + }} + w.InheritOption(parent) + assert.Len(t, w.Option.Mutators, 2) + assert.Len(t, w.Option.StepInterceptors, 2) + assert.Len(t, w.Option.AttemptInterceptors, 2) + }) + + t.Run("InheritOption does not mutate parent's slices", func(t *testing.T) { + parentCopy := WorkflowOption{ + Mutators: []Mutator{mA}, + StepInterceptors: []StepInterceptor{sA}, + AttemptInterceptors: []AttemptInterceptor{aA}, + } + w := &Workflow{Option: WorkflowOption{ + Mutators: []Mutator{mB}, + StepInterceptors: []StepInterceptor{sB}, + AttemptInterceptors: []AttemptInterceptor{aB}, + }} + w.InheritOption(parentCopy) + assert.Len(t, parentCopy.Mutators, 1) + assert.Len(t, parentCopy.StepInterceptors, 1) + assert.Len(t, parentCopy.AttemptInterceptors, 1) + }) +} + +func TestInheritOption_DontInherit(t *testing.T) { + parent := WorkflowOption{ + MaxConcurrency: ptr(4), + DontPanic: ptr(true), + Mutators: []Mutator{Mutate[*Workflow](func(context.Context, *Workflow) Builder { return nil })}, + StepInterceptors: []StepInterceptor{StepInterceptorFunc(func(ctx context.Context, _ Steper, next func(context.Context) error) error { return next(ctx) })}, + AttemptInterceptors: []AttemptInterceptor{AttemptInterceptorFunc(func(ctx context.Context, _ Steper, _ uint64, next func(context.Context) error) error { return next(ctx) })}, + } + w := &Workflow{Option: WorkflowOption{DontInherit: true}} + w.InheritOption(parent) + assert.Nil(t, w.Option.MaxConcurrency) + assert.Nil(t, w.Option.DontPanic) + assert.Nil(t, w.Option.Mutators) + assert.Nil(t, w.Option.StepInterceptors) + assert.Nil(t, w.Option.AttemptInterceptors) + assert.True(t, w.Option.DontInherit) +} + +// TestInheritOption_MultiLevelNesting verifies that a three-level chain +// (grandparent → parent → child) yields a child Mutators slice of +// [grandparent, parent, child] after the chain is walked through +// InheritOption twice — once on parent (receiving grandparent), then on +// child (receiving the now-merged parent.Option). +func TestInheritOption_MultiLevelNesting(t *testing.T) { + g := Mutate[*Workflow](func(context.Context, *Workflow) Builder { return nil }) + p := Mutate[*Workflow](func(context.Context, *Workflow) Builder { return nil }) + c := Mutate[*Workflow](func(context.Context, *Workflow) Builder { return nil }) + + grandparent := WorkflowOption{Mutators: []Mutator{g}} + parent := &Workflow{Option: WorkflowOption{Mutators: []Mutator{p}}} + child := &Workflow{Option: WorkflowOption{Mutators: []Mutator{c}}} + + parent.InheritOption(grandparent) + child.InheritOption(parent.Option) + + assert.Len(t, child.Option.Mutators, 3) +} + +// TestDo_OptionSnapshot verifies the snapshot/restore behavior at Workflow.Do +// entry/exit. A parent that .Do()s its child workflow twice must observe the +// child's Option.Mutators reverted between runs (no accumulation). +func TestDo_OptionSnapshot(t *testing.T) { + parentM := Mutate[*Workflow](func(context.Context, *Workflow) Builder { return nil }) + + child := &Workflow{} + child.Option.Mutators = []Mutator{} // explicit empty + + parent := &Workflow{Option: WorkflowOption{Mutators: []Mutator{parentM}}} + parent.Add(Step(child)) + + ctx := context.Background() + + // Run 1. + require := assert.New(t) + require.NoError(parent.Do(ctx)) + // After Do() exits, child's Option must be back to its pre-Do() shape. + require.Empty(child.Option.Mutators, "child Mutators must be reverted to pre-run shape after Do()") + + // Reset before re-running per the documented contract (rewinds per-step + // statuses; Option isolation is already handled by snapshot/restore). + require.NoError(parent.Reset()) + require.NoError(child.Reset()) + + // Run 2 — same expected post-state, no accumulation. + require.NoError(parent.Do(ctx)) + require.Empty(child.Option.Mutators, "no accumulation across Do() runs") +} + +// TestInheritOption_ScalarInheritsDontPanic covers the motivating use case: +// a parent sets DontPanic=true and the child leaves it unset (nil); after +// InheritOption the child observes DontPanic=true via the dontPanic() +// accessor. +func TestInheritOption_ScalarInheritsDontPanic(t *testing.T) { + parent := WorkflowOption{DontPanic: ptr(true)} + child := &Workflow{} + child.InheritOption(parent) + assert.True(t, child.dontPanic()) +} + +// TestZeroWorkflow_NoPanic verifies that a Workflow{} with no Option set +// behaves identically to before: empty workflow Do() is a no-op success. +func TestZeroWorkflow_NoPanic(t *testing.T) { + w := &Workflow{} + assert.NoError(t, w.Do(context.Background())) + // Helper accessors must return zero values on a fresh Workflow. + assert.Equal(t, 0, w.maxConcurrency()) + assert.False(t, w.dontPanic()) + assert.False(t, w.skipAsError()) + assert.NotNil(t, w.clock()) +} + +// TestSubWorkflow_InheritOption smoke-tests the deprecation-window delegation +// from SubWorkflow.InheritOption to the inner Workflow. +func TestSubWorkflow_InheritOption(t *testing.T) { + parent := WorkflowOption{DontPanic: ptr(true), MaxConcurrency: ptr(7)} + s := &SubWorkflow{} + s.InheritOption(parent) + assert.True(t, s.w.dontPanic()) + assert.Equal(t, 7, s.w.maxConcurrency()) +} diff --git a/workflow_option_test.go b/workflow_option_test.go new file mode 100644 index 0000000..bbe7cd3 --- /dev/null +++ b/workflow_option_test.go @@ -0,0 +1,45 @@ +package flow + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestPrependSlice(t *testing.T) { + t.Run("parent nil returns child slice unchanged-shape", func(t *testing.T) { + child := []int{1, 2} + got := prependSlice[int](nil, child) + assert.Equal(t, []int{1, 2}, got) + }) + + t.Run("child nil returns parent-prepended fresh slice", func(t *testing.T) { + parent := []int{1, 2} + got := prependSlice[int](parent, nil) + assert.Equal(t, []int{1, 2}, got) + }) + + t.Run("both populated: parent prepended to child", func(t *testing.T) { + parent := []int{1, 2} + child := []int{3, 4} + got := prependSlice[int](parent, child) + assert.Equal(t, []int{1, 2, 3, 4}, got) + }) + + t.Run("does not mutate parent or child", func(t *testing.T) { + parent := []int{1, 2} + child := []int{3, 4} + _ = prependSlice[int](parent, child) + assert.Equal(t, []int{1, 2}, parent, "parent must not be mutated") + assert.Equal(t, []int{3, 4}, child, "child must not be mutated") + }) + + t.Run("returned slice has a fresh backing array when both non-empty", func(t *testing.T) { + parent := []int{1, 2} + child := []int{3, 4} + got := prependSlice[int](parent, child) + got[0] = 99 + assert.Equal(t, []int{1, 2}, parent) + assert.Equal(t, []int{3, 4}, child) + }) +} diff --git a/workflow_options_test.go b/workflow_options_test.go index 280c486..a70e463 100644 --- a/workflow_options_test.go +++ b/workflow_options_test.go @@ -13,9 +13,10 @@ import ( func TestDontPanic(t *testing.T) { t.Parallel() + dontPanic := true t.Run("panic in step", func(t *testing.T) { t.Parallel() - workflow := &Workflow{DontPanic: true} + workflow := &Workflow{Option: WorkflowOption{DontPanic: &dontPanic}} panicStep := Func("panic", func(ctx context.Context) error { panic("panic in step") }) @@ -27,7 +28,7 @@ func TestDontPanic(t *testing.T) { }) t.Run("panic in flow", func(t *testing.T) { t.Parallel() - workflow := &Workflow{DontPanic: true} + workflow := &Workflow{Option: WorkflowOption{DontPanic: &dontPanic}} answer := FuncO("answer", func(ctx context.Context) (int, error) { return 42, nil }) @@ -47,7 +48,7 @@ func TestDontPanic(t *testing.T) { }) t.Run("panic will have stack traces", func(t *testing.T) { t.Parallel() - workflow := &Workflow{DontPanic: true} + workflow := &Workflow{Option: WorkflowOption{DontPanic: &dontPanic}} panicStep := Func("panic", func(ctx context.Context) error { panic("panic in step") }) @@ -63,7 +64,8 @@ func TestMaxConcurrency(t *testing.T) { t.Parallel() t.Run("MaxConcurrency=2 allows at most 2 concurrent Steps", func(t *testing.T) { t.Parallel() - w := &Workflow{MaxConcurrency: 2} + mc := 2 + w := &Workflow{Option: WorkflowOption{MaxConcurrency: &mc}} var running atomic.Int32 var maxSeen atomic.Int32 gate := make(chan struct{}) @@ -98,7 +100,8 @@ func TestMaxConcurrency(t *testing.T) { t.Run("MaxConcurrency=0 imposes no limit", func(t *testing.T) { t.Parallel() const n = 4 - w := &Workflow{MaxConcurrency: 0} + mc := 0 + w := &Workflow{Option: WorkflowOption{MaxConcurrency: &mc}} var running atomic.Int32 var maxSeen atomic.Int32 gate := make(chan struct{}) @@ -133,6 +136,7 @@ func TestMaxConcurrency(t *testing.T) { func TestSkipAsError(t *testing.T) { t.Parallel() + skipAsError := true t.Run("Skipped is acceptable by default", func(t *testing.T) { step := Func("step", func(ctx context.Context) error { return Skip(nil) }) w := new(Workflow).Add(Step(step)) @@ -141,7 +145,7 @@ func TestSkipAsError(t *testing.T) { t.Run("Skipped counts as error when SkipAsError=true", func(t *testing.T) { step := Func("step", func(ctx context.Context) error { return Skip(nil) }) - w := &Workflow{SkipAsError: true} + w := &Workflow{Option: WorkflowOption{SkipAsError: &skipAsError}} w.Add(Step(step)) assert.Error(t, w.Do(context.Background())) }) @@ -149,13 +153,16 @@ func TestSkipAsError(t *testing.T) { func TestClock(t *testing.T) { t.Parallel() - t.Run("Nil Clock uses wall clock", func(t *testing.T) { + t.Run("Nil Clock uses wall clock via accessor", func(t *testing.T) { step := Func("step", func(ctx context.Context) error { return nil }) w := &Workflow{} w.Add(Step(step)) - assert.Nil(t, w.Clock) + assert.Nil(t, w.Option.Clock, "Clock field starts unset") + assert.NotNil(t, w.clock(), "w.clock() returns a real clock when field is nil") assert.NoError(t, w.Do(context.Background())) - assert.NotNil(t, w.Clock) + // New contract: w.Option.Clock is NOT written by Do() — the accessor + // returns a fresh clock.New() on each call when the field is unset. + assert.Nil(t, w.Option.Clock, "Do() must not mutate Option.Clock") }) t.Run("Mock clock controls Step timeout", func(t *testing.T) { @@ -170,7 +177,7 @@ func TestClock(t *testing.T) { return nil } }) - w := &Workflow{Clock: mockClock} + w := &Workflow{Option: WorkflowOption{Clock: mockClock}} w.Add(Step(step).Timeout(time.Minute)) done := make(chan error, 1) diff --git a/workflow_test.go b/workflow_test.go index 48e4958..24f9091 100644 --- a/workflow_test.go +++ b/workflow_test.go @@ -258,7 +258,8 @@ func TestSubWorkflow(t *testing.T) { func TestMaxConcurrencyDeadlock(t *testing.T) { t.Parallel() a, b, c := NoOp("a"), NoOp("b"), NoOp("c") - w := &Workflow{MaxConcurrency: 1} + mc := 1 + w := &Workflow{Option: WorkflowOption{MaxConcurrency: &mc}} w.Add( Step(a), Step(b).DependsOn(a), @@ -287,7 +288,8 @@ func TestMaxConcurrencyDeadlockStress(t *testing.T) { go func() { defer wg.Done() a, b, c := NoOp("a"), NoOp("b"), NoOp("c") - w := &Workflow{MaxConcurrency: 1} + mc := 1 + w := &Workflow{Option: WorkflowOption{MaxConcurrency: &mc}} w.Add( Step(a), Step(b).DependsOn(a), @@ -311,12 +313,12 @@ func TestStepExecution_BasicSuccess(t *testing.T) { var stepped []Steper step := NoOp("a") w := &Workflow{ - StepInterceptors: []StepInterceptor{ + Option: WorkflowOption{StepInterceptors: []StepInterceptor{ StepInterceptorFunc(func(ctx context.Context, s Steper, next func(context.Context) error) error { stepped = append(stepped, s) return next(ctx) }), - }, + }}, } w.Add(Step(step)) assert.NoError(t, w.Do(context.Background())) @@ -335,7 +337,7 @@ func TestStepExecution_StepInterceptorOrder(t *testing.T) { }) } w := &Workflow{ - StepInterceptors: []StepInterceptor{makeIC("A"), makeIC("B")}, + Option: WorkflowOption{StepInterceptors: []StepInterceptor{makeIC("A"), makeIC("B")}}, } w.Add(Step(NoOp("s"))) assert.NoError(t, w.Do(context.Background())) @@ -365,7 +367,7 @@ func TestStepExecution_StepInterceptorChain_NoVariableCapture(t *testing.T) { return nil }) w := &Workflow{ - StepInterceptors: []StepInterceptor{makeIC("A"), makeIC("B"), makeIC("C"), makeIC("D")}, + Option: WorkflowOption{StepInterceptors: []StepInterceptor{makeIC("A"), makeIC("B"), makeIC("C"), makeIC("D")}}, } w.Add(Step(step)) assert.NoError(t, w.Do(context.Background())) @@ -400,7 +402,7 @@ func TestStepExecution_AttemptInterceptorChain_NoVariableCapture(t *testing.T) { return nil }) w := &Workflow{ - AttemptInterceptors: []AttemptInterceptor{makeIC("X"), makeIC("Y"), makeIC("Z")}, + Option: WorkflowOption{AttemptInterceptors: []AttemptInterceptor{makeIC("X"), makeIC("Y"), makeIC("Z")}}, } w.Add(Step(step).Retry(func(o *RetryOption) { o.Attempts = 3 @@ -426,7 +428,7 @@ func TestStepExecution_AttemptInterceptorOrder(t *testing.T) { }) } w := &Workflow{ - AttemptInterceptors: []AttemptInterceptor{makeIC("X"), makeIC("Y")}, + Option: WorkflowOption{AttemptInterceptors: []AttemptInterceptor{makeIC("X"), makeIC("Y")}}, } w.Add(Step(NoOp("s"))) assert.NoError(t, w.Do(context.Background())) @@ -438,12 +440,12 @@ func TestStepExecution_SkippedStep(t *testing.T) { interceptorCalled := false step := NoOp("a") w := &Workflow{ - StepInterceptors: []StepInterceptor{ + Option: WorkflowOption{StepInterceptors: []StepInterceptor{ StepInterceptorFunc(func(ctx context.Context, s Steper, next func(context.Context) error) error { interceptorCalled = true return next(ctx) }), - }, + }}, } w.Add(Step(step).When(func(_ context.Context, _ map[Steper]StepResult) StepStatus { return Skipped @@ -466,14 +468,14 @@ func TestStepExecution_RetryingStep(t *testing.T) { return nil }) w := &Workflow{ - AttemptInterceptors: []AttemptInterceptor{ + Option: WorkflowOption{AttemptInterceptors: []AttemptInterceptor{ AttemptInterceptorFunc(func(ctx context.Context, s Steper, attempt uint64, next func(context.Context) error) error { mu.Lock() attempts = append(attempts, attempt) mu.Unlock() return next(ctx) }), - }, + }}, } w.Add(Step(step).Retry(func(o *RetryOption) { o.Attempts = 3 @@ -514,9 +516,12 @@ func TestSkippedStep_DoesNotConsumeLease(t *testing.T) { }) a, b, c := NoOp("a"), NoOp("b"), NoOp("c") + mc := 1 w := &Workflow{ - MaxConcurrency: 1, - AttemptInterceptors: []AttemptInterceptor{ic}, + Option: WorkflowOption{ + MaxConcurrency: &mc, + AttemptInterceptors: []AttemptInterceptor{ic}, + }, } w.Add( Step(a), @@ -551,12 +556,15 @@ func TestSkippedStep_DoesNotConsumeLease(t *testing.T) { func TestInterceptorPanic_DontPanic(t *testing.T) { t.Parallel() step := NoOp("a") + dontPanic := true w := &Workflow{ - DontPanic: true, - StepInterceptors: []StepInterceptor{ - StepInterceptorFunc(func(ctx context.Context, s Steper, next func(context.Context) error) error { - panic("boom from StepInterceptor") - }), + Option: WorkflowOption{ + DontPanic: &dontPanic, + StepInterceptors: []StepInterceptor{ + StepInterceptorFunc(func(ctx context.Context, s Steper, next func(context.Context) error) error { + panic("boom from StepInterceptor") + }), + }, }, } w.Add(Step(step)) @@ -581,12 +589,15 @@ func TestInterceptorPanic_DontPanic(t *testing.T) { func TestAttemptInterceptorPanic_DontPanic(t *testing.T) { t.Parallel() step := NoOp("a") + dontPanic := true w := &Workflow{ - DontPanic: true, - AttemptInterceptors: []AttemptInterceptor{ - AttemptInterceptorFunc(func(ctx context.Context, s Steper, attempt uint64, next func(context.Context) error) error { - panic("boom from AttemptInterceptor") - }), + Option: WorkflowOption{ + DontPanic: &dontPanic, + AttemptInterceptors: []AttemptInterceptor{ + AttemptInterceptorFunc(func(ctx context.Context, s Steper, attempt uint64, next func(context.Context) error) error { + panic("boom from AttemptInterceptor") + }), + }, }, } w.Add(Step(step)) diff --git a/wrap_test.go b/wrap_test.go index dcf2abd..b0835c0 100644 --- a/wrap_test.go +++ b/wrap_test.go @@ -172,7 +172,7 @@ func TestSubWorkflow_InterceptorPropagation(t *testing.T) { sub := &mySubStep{} sub.Add(Step(innerStep)) - w := &Workflow{StepInterceptors: []StepInterceptor{ic}} + w := &Workflow{Option: WorkflowOption{StepInterceptors: []StepInterceptor{ic}}} w.Add(Step(sub)) assert.NoError(t, w.Do(context.Background())) @@ -211,9 +211,9 @@ func TestSubWorkflow_ChildInterceptorPreserved(t *testing.T) { type mySubStep struct{ SubWorkflow } sub := &mySubStep{} sub.Add(Step(innerStep)) - sub.w.StepInterceptors = []StepInterceptor{childIC} + sub.w.Option.StepInterceptors = []StepInterceptor{childIC} - w := &Workflow{StepInterceptors: []StepInterceptor{parentIC}} + w := &Workflow{Option: WorkflowOption{StepInterceptors: []StepInterceptor{parentIC}}} w.Add(Step(sub)) assert.NoError(t, w.Do(context.Background())) @@ -248,7 +248,7 @@ func TestSubWorkflow_InterceptorNotDuplicatedOnRetry(t *testing.T) { o.Backoff = &backoff.ZeroBackOff{} })) - w := &Workflow{StepInterceptors: []StepInterceptor{sink}} + w := &Workflow{Option: WorkflowOption{StepInterceptors: []StepInterceptor{sink}}} w.Add(Step(sub)) assert.NoError(t, w.Do(context.Background())) @@ -273,7 +273,7 @@ func TestWorkflow_AsStep_InheritsInterceptors(t *testing.T) { child := &Workflow{} child.Add(Step(innerStep)) - parent := &Workflow{StepInterceptors: []StepInterceptor{ic}} + parent := &Workflow{Option: WorkflowOption{StepInterceptors: []StepInterceptor{ic}}} parent.Add(Step(child)) assert.NoError(t, parent.Do(context.Background())) @@ -284,15 +284,15 @@ func TestWorkflow_AsStep_InheritsInterceptors(t *testing.T) { found = true } } - assert.True(t, found, "parent interceptor should see inner step via Workflow.PrependInterceptors") + assert.True(t, found, "parent interceptor should see inner step via WorkflowOptionReceiver.InheritOption") } -// TestSubWorkflow_PrependInterceptorsIdempotentAcrossDo ensures that running the +// TestSubWorkflow_InterceptorIdempotentAcrossDo ensures that running the // same parent (with a sub-workflow child) multiple times does NOT accumulate -// prepended interceptors on the child. The parent's interceptor should fire +// inherited interceptors on the child. The parent's interceptor should fire // exactly twice per run (outer sub step + inner step), regardless of how many // times Do() is called. -func TestSubWorkflow_PrependInterceptorsIdempotentAcrossDo(t *testing.T) { +func TestSubWorkflow_InterceptorIdempotentAcrossDo(t *testing.T) { t.Parallel() var count atomic.Int32 @@ -306,7 +306,7 @@ func TestSubWorkflow_PrependInterceptorsIdempotentAcrossDo(t *testing.T) { sub := &mySubStep{} sub.Add(Step(innerStep)) - parent := &Workflow{StepInterceptors: []StepInterceptor{ic}} + parent := &Workflow{Option: WorkflowOption{StepInterceptors: []StepInterceptor{ic}}} parent.Add(Step(sub)) const runs = 3 @@ -320,7 +320,7 @@ func TestSubWorkflow_PrependInterceptorsIdempotentAcrossDo(t *testing.T) { } } -func TestSubWorkflow_IsolateInterceptors(t *testing.T) { +func TestSubWorkflow_DontInherit(t *testing.T) { t.Parallel() var parentCount, childCount atomic.Int32 @@ -337,10 +337,10 @@ func TestSubWorkflow_IsolateInterceptors(t *testing.T) { type mySubStep struct{ SubWorkflow } sub := &mySubStep{} sub.Add(Step(innerStep)) - sub.w.StepInterceptors = []StepInterceptor{childIC} - sub.w.IsolateInterceptors = true + sub.w.Option.StepInterceptors = []StepInterceptor{childIC} + sub.w.Option.DontInherit = true - w := &Workflow{StepInterceptors: []StepInterceptor{parentIC}} + w := &Workflow{Option: WorkflowOption{StepInterceptors: []StepInterceptor{parentIC}}} w.Add(Step(sub)) assert.NoError(t, w.Do(context.Background())) @@ -352,9 +352,9 @@ func TestSubWorkflow_IsolateInterceptors(t *testing.T) { // TestWorkflow_AsStep_InheritsThroughNamedStep ensures that wrapping a child // *Workflow in a Steper-only wrapper (NamedStep embeds the Steper interface, -// so PrependInterceptors is NOT promoted) does not break parent-interceptor +// so InheritOption is NOT promoted) does not break parent-Option // inheritance: the parent walks the Step tree via Unwrap to find the -// InterceptorReceiver, so wrappers are transparent. +// WorkflowOptionReceiver, so wrappers are transparent. func TestWorkflow_AsStep_InheritsThroughNamedStep(t *testing.T) { t.Parallel() @@ -373,10 +373,10 @@ func TestWorkflow_AsStep_InheritsThroughNamedStep(t *testing.T) { // Wrap child in NamedStep — this is the wrapper produced by flow.Name(). // NamedStep embeds the Steper interface, so it does NOT promote - // PrependInterceptors. Inheritance must therefore go through Unwrap. + // InheritOption. Inheritance must therefore go through Unwrap. named := &NamedStep{Name: "child", Steper: child} - parent := &Workflow{StepInterceptors: []StepInterceptor{ic}} + parent := &Workflow{Option: WorkflowOption{StepInterceptors: []StepInterceptor{ic}}} parent.Add(Step(named)) assert.NoError(t, parent.Do(context.Background()))