feat: structured observability via two-layer interceptor system#74
Merged
Conversation
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Interceptor and BeforeStep/AfterStep are orthogonal (workflow-level vs step-level) - Remove precomputed Name from StepInfo/AttemptInfo/WorkflowEvent; Step pointer is the identifier - NewStepEventSink returns StepInterceptor (interface); retryNotifier is package-private - Remove retry.go from Files Affected (no changes needed) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Interceptors that need timing should call time.Now() themselves. Start had ambiguous semantics (chain entry vs Do entry) and added no value that a one-liner couldn't provide. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…rs to Workflow Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Also fix wireNotify to use ex.attempt-1 for Retrying events (since runAttempt's defer has already incremented ex.attempt by the time Notify fires) and remove the now-duplicate ex.attempt++ from wireNotify. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- wireNotify: deep-copy RetryOption before mutating Notify to prevent shared-pointer mutation when Workflow.DefaultOption carries a RetryOption - buildAttemptChain: move attempt++ to wrapper around full interceptor chain so counter always advances even if AttemptInterceptor short-circuits, preventing underflow (uint64 wrap) in wireNotify's attempt-1 expression - PrependInterceptors: use make+copy instead of append to avoid aliasing parent's backing array and to ensure idempotency across Reset+Do cycles - event.go: update InterceptorReceiver comment to reflect once-per-step (not per-attempt) injection point Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ase order) main added StepResult.FinishedAt and moved Condition evaluation into tick(). Our branch keeps Condition evaluation in stepExecution.run() (needed for the StepInterceptor TerminalReason path), so only the FinishedAt and unlease-before- signal changes from main are applied here. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a two-layer interceptor system (step-level + attempt-level) to provide structured observability across all workflow steps (including retries and nested sub-workflows), replacing ad-hoc per-step wiring patterns with global hooks on Workflow.
Changes:
- Adds
StepInterceptorandAttemptInterceptorAPIs plus built-in adapters (NewStepEventSink,NewAttemptEventSink) that emit structuredWorkflowEvents. - Refactors step execution to a
stepExecutionrunner and introduces a privatescheduledsentinel to atomically claim steps before spawning goroutines. - Propagates parent interceptors into nested
SubWorkflows and adds integration/unit tests covering ordering, retries, skipped steps, and propagation.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow.go | Adds interceptor fields to Workflow, introduces stepExecution, and updates scheduling/execution flow (incl. retry notify wiring and SubWorkflow propagation). |
| event.go | Defines public interceptor/event types and provides built-in event sink adapters. |
| workflow_test.go | Adds integration tests for interceptor ordering, skip behavior, retry events, and regression coverage. |
| wrap_test.go | Adds tests ensuring interceptor propagation into sub-workflows, child interceptor preservation, and no duplication across retries. |
| event_test.go | Adds unit tests for event constants, function adapters, and event sink adapters. |
| docs/superpowers/specs/2026-05-06-step-interceptor-design.md | Adds design documentation for the interceptor system and event model. |
| openspec/changes/archive/2026-05-06-structured-event-sink/** | Archives spec-driven design artifacts, tasks, and proposal for the change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
385
to
402
| }(ctx, step, state) | ||
| ex := &stepExecution{w: w, step: step, state: state} | ||
| go ex.run(ctx) | ||
| } |
Comment on lines
+310
to
+311
| // scheduled is a private StepStatus sentinel used by tick() to atomically | ||
| // claim a step and prevent double-spawning. Never exposed to users. |
Comment on lines
+313
to
+319
| The solution: `stepExecution.wireNotify()` wraps `RetryOption.Notify` and calls `ex.onRetry` | ||
| directly. `ex.onRetry` is assembled during chain construction by collecting the `sink` function | ||
| from any `*StepEventSinkInterceptor` in `StepInterceptors`. | ||
|
|
||
| ``` | ||
| attempt N fails → backoff.Notify fires → ex.onRetry(Retrying{attempt=N}) → ex.attempt++ | ||
| ``` |
Comment on lines
+338
to
+339
| Custom interceptors that call `next` when `TerminalReason != Pending` will cause a panic (the | ||
| `next` function asserts this precondition). |
…ptInterceptor StepInterceptor should be unaware of retry internals — it sees only Scheduled and a terminal event. Retrying conceptually belongs to the attempt layer alongside Started. - stepEventSink: remove onRetry; no longer implements retryNotifier - attemptEventSink: new concrete type backing NewAttemptEventSink; implements both AttemptInterceptor and retryNotifier (Started + Retrying to same sink) - workflow.go run(): collect retryNotifiers from AttemptInterceptors, not StepInterceptors - event_test.go: assert StepEventSink does NOT implement retryNotifier; assert AttemptEventSink does - spec: update event ownership table, layer descriptions, Retrying section, usage examples, and open questions resolution Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…FromError The private statusFromError was a thin wrapper around the public StatusFromError with identical behavior. Remove it and call StatusFromError directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Scheduled/Started/Retrying had no prefix while EventSucceeded/EventFailed/ EventCanceled/EventSkipped did (to avoid collision with StepStatus constants). Rename to EventScheduled/EventStarted/EventRetrying for uniform naming. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AttemptInterceptor already receives the failure error when InterceptAttempt returns — the only unique information Retrying added was BackoffDuration, which is of limited value (derivable from timestamps, or known from static config). Removing it simplifies the design considerably: - Drop EventRetrying constant and BackoffDuration field from WorkflowEvent - Remove retryNotifier interface, wireNotify, onRetry from stepExecution - NewAttemptEventSink reverts to a simple AttemptInterceptorFunc - No side-channel machinery needed Update spec to reflect clean two-layer model with no side-channels. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ceptor.go Users can implement whatever event system they want on top of the interceptor interfaces — the framework does not need to prescribe WorkflowEvent, EventType constants, or NewStepEventSink/NewAttemptEventSink helpers. - Delete event.go (EventType, WorkflowEvent, NewStepEventSink, NewAttemptEventSink, terminalEventType, terminalStepStatusToEventType, stepEventSink) - Delete event_test.go - Add interceptor.go: StepInfo, AttemptInfo, StepInterceptor, AttemptInterceptor, StepInterceptorFunc, AttemptInterceptorFunc, InterceptorReceiver - Rewrite workflow_test.go and wrap_test.go to use interceptors directly - Update spec to reflect the clean minimal API Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace struct parameters with direct values: - StepInterceptor.InterceptStep(ctx, step Steper, next) - AttemptInterceptor.InterceptAttempt(ctx, step Steper, attempt uint64, next) Skipped/Canceled steps now bypass the interceptor chain entirely instead of entering it with a TerminalReason field. This removes the footgun of calling next on a non-Pending step and simplifies the mental model. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Workflow itself now implements InterceptorReceiver, so any nested *Workflow used as a step inherits parent interceptors (not just SubWorkflow). - SubWorkflow.PrependInterceptors delegates to embedded Workflow. - New Workflow.IsolateInterceptors bool: when true, PrependInterceptors is a no-op and the child runs only its own interceptor stack. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The 2026-05-06-structured-event-sink change was archived without syncing its delta to a main spec, and its delta still described the old EventSink/StepInfo/AttemptInfo/TerminalReason API. Create a fresh main spec that documents the current implementation: - StepInterceptor / AttemptInterceptor with direct Steper / uint64 parameters (no wrapper types). - Skipped/Canceled steps bypass the interceptor chain entirely. - Workflow itself implements InterceptorReceiver; SubWorkflow delegates. - Workflow.IsolateInterceptors opts out of inheritance from a parent. - Attempt counter ownership and short-circuit-safe increment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines
+269
to
+281
| func (w *Workflow) PrependInterceptors(step []StepInterceptor, attempt []AttemptInterceptor) { | ||
| if w.IsolateInterceptors { | ||
| return | ||
| } | ||
| combined := make([]StepInterceptor, len(step)+len(w.StepInterceptors)) | ||
| copy(combined, step) | ||
| copy(combined[len(step):], w.StepInterceptors) | ||
| w.StepInterceptors = combined | ||
|
|
||
| combinedA := make([]AttemptInterceptor, len(attempt)+len(w.AttemptInterceptors)) | ||
| copy(combinedA, attempt) | ||
| copy(combinedA[len(attempt):], w.AttemptInterceptors) | ||
| w.AttemptInterceptors = combinedA |
Comment on lines
+330
to
+332
| // scheduled is a private StepStatus sentinel used by tick() to atomically | ||
| // claim a step and prevent double-spawning. Never exposed to users. | ||
| const scheduled StepStatus = "scheduled" |
Comment on lines
+3
to
+33
| ## Implementation | ||
|
|
||
| - [x] Define public types in `event.go` (`EventType`, `WorkflowEvent`, `StepInterceptor`, `AttemptInterceptor`, `StepInterceptorFunc`, `AttemptInterceptorFunc`, `StepInfo`, `AttemptInfo`, `InterceptorReceiver`, `retryNotifier`) | ||
| - [x] Implement `NewStepEventSink` and `NewAttemptEventSink` in `event.go` | ||
| - [x] Add `StepInterceptors`/`AttemptInterceptors` fields to `Workflow` struct | ||
| - [x] Introduce `stepExecution` struct; simplify `tick()` to only claim step via `scheduled` sentinel | ||
| - [x] Implement `stepExecution.run()`, `executeWithRetry()`, `buildAttemptChain()`, `runAttempt()`, `wireNotify()` | ||
| - [x] Delete `makeDoForStep()` and `runStep()` from `workflow.go` | ||
| - [x] Implement `SubWorkflow.PrependInterceptors` in `wrap.go` | ||
|
|
||
| ## Tests | ||
|
|
||
| - [x] Unit tests for `EventType` constants and `StepInterceptorFunc`/`AttemptInterceptorFunc` adapters | ||
| - [x] Unit tests for `NewStepEventSink` (Succeeded, Failed, Skipped, OnRetry) | ||
| - [x] Unit tests for `NewAttemptEventSink` (Started event) | ||
| - [x] Integration test: basic step success with StepInterceptor | ||
| - [x] Integration test: StepInterceptor chain ordering (A→B→B→A) | ||
| - [x] Integration test: AttemptInterceptor chain ordering (X→Y→Y→X) | ||
| - [x] Integration test: Skipped step enters interceptor chain with TerminalReason | ||
| - [x] Integration test: Retrying events with correct attempt numbers | ||
| - [x] Integration test: SubWorkflow interceptor propagation | ||
| - [x] Integration test: child interceptor preserved alongside parent | ||
| - [x] Integration test: `PrependInterceptors` not duplicated on retry (`TestSubWorkflow_InterceptorNotDuplicatedOnRetry`) | ||
| - [x] Regression test: zero-interceptor workflow unchanged | ||
| - [x] Race detector clean (`go test -race ./...`) | ||
|
|
||
| ## Bug Fixes (found during review) | ||
|
|
||
| - [x] Fix C1: `PrependInterceptors` moved from `runAttempt` (per-attempt) to `executeWithRetry` (once per step) | ||
| - [x] Fix wireNotify timing: `Retrying.Attempt` uses `ex.attempt - 1` (defer in `runAttempt` fires before Notify) | ||
| - [x] Fix `EventType` to be a distinct named type (`type EventType string`), not a type alias |
Previously, PrependInterceptors wrote the combined slice back into the child's StepInterceptors / AttemptInterceptors. The make+copy only avoided backing-array aliasing — it did not prevent permanent mutation of the receiver. As a result, running the same parent multiple times made the parent's interceptors stack up on the child (N+1 invocations on the Nth run). Fix: keep StepInterceptors / AttemptInterceptors immutable after construction. PrependInterceptors writes to private inheritedStep / inheritedAttempt slices instead. Run-time chain construction goes through effectiveStepInterceptors / effectiveAttemptInterceptors which build an ephemeral [inherited..., base...] slice. The inherited slices are cleared after waitGroup.Wait() at the end of each Do() so the next run starts clean. Also add TestSubWorkflow_PrependInterceptorsIdempotentAcrossDo covering 3 sequential runs of the same parent+child pair. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two coupled fixes addressing Copilot review: 1. Skipped/Canceled steps no longer consume a concurrency lease or spawn a worker goroutine. tick() evaluates each runnable step's Condition inline; terminal results are settled directly on State without going through the worker path. Previously, every step — including those about to be Skipped — took a lease, spawned a goroutine, evaluated condition, then released the lease. Under MaxConcurrency=1 with many condition-skipped steps this serialized work that could be settled synchronously. 2. The private "scheduled" StepStatus sentinel is removed. Its sole purpose was preventing tick() from double-spawning a step before the worker set Running. Since tick() now sets Running itself (under statusChange.L, before spawning the worker), no sentinel is needed. As a side effect, StateOf(step).GetStatus() can no longer return the undocumented "scheduled" string — only the public StepStatus values. tick() now re-iterates within a single call when it inline-settles any step, so newly-unblocked downstream steps are picked up without waiting for a signalStatusChange (which is never fired when no worker is spawned). Add TestSkippedStep_DoesNotConsumeLease (MaxConcurrency=1, a→b(skip) →c) asserting b never enters the AttemptInterceptor path while a and c do. Update openspec spec and design doc. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines
+56
to
+58
| // start of each Do() (parent → child) and cleared by reset(). They are never | ||
| // merged into StepInterceptors / AttemptInterceptors so the user-supplied base | ||
| // stays untouched and repeated runs do not accumulate. |
Comment on lines
+336
to
+356
| // next time this workflow runs (under any parent, or standalone) it starts | ||
| // fresh and PrependInterceptors does not accumulate across runs. | ||
| w.inheritedStep = nil | ||
| w.inheritedAttempt = nil |
Comment on lines
+516
to
+526
| stepNext := func(ctx context.Context) error { return ex.executeWithRetry(ctx) } | ||
| stepICs := ex.w.effectiveStepInterceptors() | ||
| for i := len(stepICs) - 1; i >= 0; i-- { | ||
| ic := stepICs[i] | ||
| next := stepNext | ||
| stepNext = func(ctx context.Context) error { | ||
| return ic.InterceptStep(ctx, ex.step, next) | ||
| } | ||
| } | ||
|
|
||
| err := stepNext(ctx) |
Comment on lines
+571
to
+583
| func (ex *stepExecution) buildAttemptChain() func(context.Context) error { | ||
| chain := func(ctx context.Context) error { | ||
| return ex.runAttempt(ctx) | ||
| } | ||
| attemptICs := ex.w.effectiveAttemptInterceptors() | ||
| for i := len(attemptICs) - 1; i >= 0; i-- { | ||
| ic := attemptICs[i] | ||
| next := chain | ||
| icLocal := ic | ||
| chain = func(ctx context.Context) error { | ||
| return icLocal.InterceptAttempt(ctx, ex.step, ex.attempt, next) | ||
| } | ||
| // call After callbacks, will use the ctxStep for After callbacks | ||
| return do(func() error { return state.After(ctxStep, step, err) }) | ||
| } |
Comment on lines
+1
to
+73
| # Step Interceptor 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:** Add a two-layer interceptor system (`StepInterceptor` + `AttemptInterceptor`) to go-workflow, enabling structured global observability with built-in `EventSink` adapters. | ||
|
|
||
| **Architecture:** Introduce `event.go` for public types, refactor `workflow.go` to extract `stepExecution` (replacing the anonymous goroutine in `tick()`), and add `InterceptorReceiver` to `SubWorkflow` for nested propagation. `BeforeStep`/`AfterStep` remain unchanged as step-level configuration; interceptors are workflow-level and orthogonal. | ||
|
|
||
| **Tech Stack:** Go 1.23, `github.com/stretchr/testify`, `github.com/benbjohnson/clock` | ||
|
|
||
| --- | ||
|
|
||
| ## File Map | ||
|
|
||
| | File | Action | Responsibility | | ||
| |------|--------|----------------| | ||
| | `event.go` | **Create** | `EventType`, `WorkflowEvent`, `StepInterceptor`, `AttemptInterceptor`, `StepInterceptorFunc`, `AttemptInterceptorFunc`, `StepInfo`, `AttemptInfo`, `InterceptorReceiver`, `NewStepEventSink`, `NewAttemptEventSink`, private `retryNotifier` | | ||
| | `event_test.go` | **Create** | Tests for `NewStepEventSink` and `NewAttemptEventSink` | | ||
| | `workflow.go` | **Modify** | Add `StepInterceptors`/`AttemptInterceptors` fields; introduce `stepExecution`; simplify `tick()`; add `wireNotify` | | ||
| | `workflow_test.go` | **Modify** | Integration tests for interceptor ordering, SubWorkflow propagation, Retrying events | | ||
| | `wrap.go` | **Modify** | `SubWorkflow` implements `InterceptorReceiver` | | ||
| | `wrap_test.go` | **Modify** | Tests for interceptor propagation through SubWorkflow | | ||
|
|
||
| --- | ||
|
|
||
| ## Task 1: Define public types in `event.go` | ||
|
|
||
| **Files:** | ||
| - Create: `event.go` | ||
| - Create: `event_test.go` | ||
|
|
||
| - [ ] **Step 1: Write the failing test** | ||
|
|
||
| ```go | ||
| // event_test.go | ||
| package flow | ||
|
|
||
| import ( | ||
| "testing" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestEventTypeConstants(t *testing.T) { | ||
| // Verify all constants exist and are distinct | ||
| types := []EventType{Scheduled, Started, Retrying, Succeeded, Failed, Canceled, Skipped} | ||
| seen := map[EventType]bool{} | ||
| for _, et := range types { | ||
| assert.False(t, seen[et], "duplicate EventType: %q", et) | ||
| seen[et] = true | ||
| } | ||
| } | ||
|
|
||
| func TestStepInterceptorFunc(t *testing.T) { | ||
| called := false | ||
| var ic StepInterceptor = StepInterceptorFunc(func(ctx context.Context, info StepInfo, next func(context.Context) error) error { | ||
| called = true | ||
| return next(ctx) | ||
| }) | ||
| _ = ic.InterceptStep(context.Background(), StepInfo{}, func(ctx context.Context) error { return nil }) | ||
| assert.True(t, called) | ||
| } | ||
|
|
||
| func TestAttemptInterceptorFunc(t *testing.T) { | ||
| called := false | ||
| var ic AttemptInterceptor = AttemptInterceptorFunc(func(ctx context.Context, info AttemptInfo, next func(context.Context) error) error { | ||
| called = true | ||
| return next(ctx) | ||
| }) | ||
| _ = ic.InterceptAttempt(context.Background(), AttemptInfo{}, func(ctx context.Context) error { return nil }) | ||
| assert.True(t, called) | ||
| } | ||
| ``` | ||
|
|
Five Copilot review items addressed: 1. inheritedStep / inheritedAttempt only cleared on the success path → Move clearing to a defer at the start of Do() so all exit paths (preflight error, panic, early Empty return) reset per-run state. 2. Comment on inheritedStep field said "cleared by reset()" — wrong (reset cannot clear them without breaking inheritance, since the parent writes them just before child.Do() runs reset). Rewrite the comment to document the actual lifecycle: written by parent before child.Do(), read during child.Do(), cleared by Do()'s defer. 3. DontPanic does not catch panics from user StepInterceptors → wrap each StepInterceptor invocation in catchPanicAsError when DontPanic is true, preventing process crashes / lease leaks / status-signal loss from a faulty interceptor. 4. Same as 3 for AttemptInterceptor. 5. Plan doc still describes the old EventSink/EventType design. Add a prominent OUTDATED banner at the top pointing to the current design doc and synced openspec, kept only as a record of the original direction. Bonus: public Reset() now also clears inheritedStep / inheritedAttempt (internal reset() must NOT, because it runs at the start of Do() — clearing there would wipe the prefix the parent just wrote and break inheritance). Documented this asymmetry on the field and on Reset(). New tests: - TestInterceptorPanic_DontPanic — guards #3 - TestAttemptInterceptorPanic_DontPanic — guards #4 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines
+545
to
+551
| stepNext = func(ctx context.Context) error { | ||
| if ex.w.DontPanic { | ||
| return catchPanicAsError(func() error { | ||
| return ic.InterceptStep(ctx, ex.step, next) | ||
| }) | ||
| } | ||
| return ic.InterceptStep(ctx, ex.step, next) |
Comment on lines
+608
to
+615
| icLocal := ic | ||
| chain = func(ctx context.Context) error { | ||
| if ex.w.DontPanic { | ||
| return catchPanicAsError(func() error { | ||
| return icLocal.InterceptAttempt(ctx, ex.step, ex.attempt, next) | ||
| }) | ||
| } | ||
| return icLocal.InterceptAttempt(ctx, ex.step, ex.attempt, next) |
| // reset both parent and child step states so the workflow is re-runnable | ||
| assert.NoError(t, parent.Reset()) | ||
| assert.NoError(t, parent.Do(context.Background())) | ||
| assert.Equal(t, int32(2), count.Load(), |
Drop the planning artifacts that this PR added under docs/superpowers and openspec/changes/archive — they reflect intermediate / outdated designs and now live only as a historical distraction. The single authoritative spec lives in openspec/specs/step-interceptor/spec.md. Update openspec/specs/step-interceptor/spec.md to fully match the shipped behavior: - Document the per-run scoped inheritance using inheritedStep / inheritedAttempt fields (not "make+copy" on the user-supplied base). - Clarify the Reset() vs reset() asymmetry and why internal reset() must NOT clear inherited interceptors. - New scenario: PrependInterceptors does not accumulate across repeated Do() runs. - New requirement: DontPanic protects interceptor panics (with scenarios for both StepInterceptor and AttemptInterceptor). Pre-existing files under docs/superpowers/plans (e.g. 2026-05-04-test-spec-alignment.md) and openspec/changes/archive (e.g. 2026-05-04-document-existing-behaviors) are preserved. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot reported a suspected loop-variable-capture bug in the chain builders: it worried that `next := stepNext` (and `next := chain`) might be reused across iterations and cause the closure to recurse on itself or call the wrong interceptor. It is NOT a bug. `ic` and `next` are declared inside the loop body with `:=`, so each iteration produces fresh variables and the closure captures each iteration's instance independently. This was already implicitly covered by TestStepExecution_StepInterceptorOrder and TestStepExecution_AttemptInterceptorOrder (both use 2 ICs and verify exact ordering — a capture bug would have failed them). Even so: - Add TestStepExecution_StepInterceptorChain_NoVariableCapture (4 ICs, asserts exact A→B→C→D→D→C→B→A nesting and that the inner step runs exactly once — would catch self-recursion or reordering). - Add TestStepExecution_AttemptInterceptorChain_NoVariableCapture (3 ICs × 3 retried attempts, asserts the full 18-event sequence). - Rename the loop-local `next` to `nextLocal` and add a comment explicitly noting the per-iteration scoping, so future reviewers don't have to re-derive the safety of the closure. - Drop the redundant `icLocal := ic` in buildAttemptChain (ic is already loop-body-local), unifying style with the StepInterceptor chain builder. - Use assert.Equalf in TestSubWorkflow_PrependInterceptorsIdempotentAcrossDo so the printf-style message actually formats. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
docs/superpowers/plans/2026-05-05-errworkflow-execution-order.md and openspec/changes/archive/2026-05-05-errworkflow-execution-order/ were introduced by PR #73 (sort ErrWorkflow by FinishedAt) and merged into main. The previous cleanup commit removed them by mistake — my diff-against-main check listed them as additions on this branch, but they were genuinely on main as well. Restore from 2c88ec2. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces global, structured observability across all workflow steps via a two-layer interceptor system. Users wire whatever event types and sinks they want — the framework provides the interception points, not the event vocabulary.
StepInterceptor— wraps the full lifecycle of a step (all retry attempts). Right place for OTel spans (one span per step) and step-level metrics.AttemptInterceptor— wraps each individual attempt (Before → Do → After). Right place for per-attempt logging and per-attempt tracing. The error returned bynextis the attempt's failure and can be inspected.InterceptorReceiver— implemented onWorkflowitself, so any nested*Workflow(or step embeddingSubWorkflow) inherits its parent's interceptors automatically.Workflow.IsolateInterceptors— opt-out flag for sub-workflows that define their own self-contained observability pipeline.Design notes
Conditionis evaluated inline intick(); terminal results are settled directly without spawning a worker goroutine or consuming aMaxConcurrencylease.StepStatusis unchanged. No new public statuses, no internal sentinels leaking throughStateOf().GetStatus().Runningspans the full retry loop, whileAttemptInterceptorfires per attempt.PrependInterceptorsis run-scoped. Inherited interceptors are stored separately from the user-supplied base and cleared at the end of everyDo(), so repeated runs of the same parent → child pair never accumulate wrappers.EventType/WorkflowEvent/EventSinkwere considered and dropped — they prescribe a vocabulary the framework should not own. Users plugslog, OTel, Prometheus, or anything else directly into the interceptor.Files
interceptor.go— interfaces, function adapters,InterceptorReceiverworkflow.go—StepInterceptors/AttemptInterceptors/IsolateInterceptorsfields; inline-conditiontick();stepExecutionworker; effective-chain helperswrap.go—SubWorkflow.PrependInterceptorsdelegates to embeddedWorkflowopenspec/specs/step-interceptor/spec.md— synced main specdocs/superpowers/specs/2026-05-06-step-interceptor-design.md— full design docTest plan
A:before → B:before → ... → B:after → A:after)AttemptInterceptorfires once per attempt with monotonically increasingattemptMaxConcurrency=1: no lease consumed, no goroutine spawnedPrependInterceptorsonce per step, not per attempt)Do()runs (idempotent inheritance)*Workflowused directly as a step inherits parent interceptorsIsolateInterceptors=trueblocks parent inheritanceMaxConcurrency=1deadlock regression suite still passesgo test -race ./...