fix: address CodeRabbit review feedback from PR #34 - #35
Merged
Conversation
- Propagate ExecutionResult in CLI and all examples instead of only
checking error (critical: Execute can return nil err with failed status)
- Remove stale "wait" activity from CLI help text
- Fix stale "Script activities" text in branching example
- Add Outputs declaration to README quick-start example
- Make ActivityRegistry zero value safe (nil receiver + nil map guard)
- Guard against nil Checkpointer in pause/unpause helpers
- Propagate failed child workflow executions as errors from ExecuteSync
- Reject pre-v1 checkpoints (SchemaVersion < 1) in all checkpoint readers
- Reject malformed ${...} templates instead of silently treating as literals
- Clarify Runner is recommended but not required in production checklist
- Remove dead variablesMap() method from BranchLocalState
- Replace require assertions in activity goroutine with error returns
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
myzie
added a commit
that referenced
this pull request
Apr 12, 2026
* Add v1 implementation plan and combined API review Capture the planning documents that drive the v1 cleanup so the branch has the source of truth for sequencing and decisions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR1: rename Path → Branch (#15) Mechanical rename of execution-thread terminology from "path" to "branch" across the public API, internal engine, tests, and examples: - Types: Path→Branch, PathState→BranchState, PathLocalState→ BranchLocalState, PathSnapshot→BranchSnapshot, PathOptions→ BranchOptions, PathSpec→BranchSpec, PathExecutionEvent→ BranchExecutionEvent. - IDs and methods: PathID→BranchID, PausePath→PauseBranch, UnpausePath→UnpauseBranch, GetPathID→GetBranchID, PausePathInCheckpoint→PauseBranchInCheckpoint (and unpause). - Errors: ErrPathNotFound→ErrBranchNotFound. - Fields: Edge.Path→Edge.BranchName, JoinConfig.Paths→ JoinConfig.Branches, JoinConfig.PathMappings→ JoinConfig.BranchMappings, Output.Path→Output.Branch, Checkpoint.PathStates→BranchStates, PathCounter→BranchCounter. - Callbacks: BeforePathExecution→BeforeBranchExecution and friends. - Deletes Workflow.Path(), Workflow.path field, Options.Path. These were never load-bearing. - Renames files path*.go → branch*.go and examples/join_paths → examples/join_branches. The word "path" survives only as English prose for state dot-notation (state.foo.bar) and genuine filesystem paths (filepath, checkpointer_file.go, file_activity). Tests + go vet green. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR2: surface shrink (#16) Hide orchestration plumbing types that consumers should never see. Opaque Branch, unexported internal snapshot/request types, deleted the undocumented WorkflowFormatter. Unexport: - Branch (struct), branchOptions, branchSpec, branchSnapshot - activityExecutor (internal side interface) - waitRequest, joinRequest, pauseRequest - executionState, executionAdapter - patch, patchOptions, newPatch, generatePatches, applyPatches - newSignalWait, newSleepWait, isWaitUnwind (asWaitUnwind added as the *waitUnwindError extractor for internal error handling) - newBranch Keep exported (required by the checkpoint wire format or by consumer tests until PR6 lands FakeContext): - BranchState, JoinState, WaitState, WaitKind - BranchLocalState, NewBranchLocalState - NewContext, ExecutionContextOptions Delete WorkflowFormatter entirely: no consumers, undocumented, and ExecutionCallbacks covers the same use case. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR3: validation phase 1 + step kinds + StartAt (#22) workflow.New now runs structural validation eagerly and fails fast with a *ValidationError containing every problem found. Validation is independent of the activity registry and script compiler — binding-level checks ship in PR5. New: - Options.StartAt names the initial step (default: Steps[0]). - Retry / catch modifier rejection on non-activity, non-wait_signal steps. - RetryConfig sanity (MaxRetries >= 0, BaseDelay <= MaxDelay, BackoffRate >= 0). - Duplicate branch name detection now surfaces as a structured ValidationProblem instead of an ad-hoc fmt error. - ValidationProblem carries an Err sentinel; *ValidationError implements Is so errors.Is(err, ErrDuplicateStepName) etc. works. Removed: - Unreachable-step check. Per plan: warn-but-don't-error is better v1 posture. We'll reintroduce it as a soft warning later if consumers ask. New sentinels: ErrDuplicateStepName, ErrEmptyStepName, ErrUnknownStartStep, ErrUnknownEdgeTarget, ErrUnknownCatchTarget, ErrUnknownJoinBranch, ErrInvalidStepKind, ErrInvalidModifier, ErrInvalidRetryConfig, ErrInvalidSleepConfig, ErrInvalidWaitConfig, ErrReservedBranchName, ErrDuplicateBranchName. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR4: ActivityRegistry, functional options, single Execute (#23) The biggest reshape in the v1 cleanup. Every public entry point into the engine now looks like post-v1 code. NewExecution signature: NewExecution(wf *Workflow, reg *ActivityRegistry, opts ...ExecutionOption) (*Execution, error) Activity registration: reg := workflow.NewActivityRegistry() reg.MustRegister(myActivity) reg.MustRegister(anotherActivity) Execution options (functional): WithInputs, WithCheckpointer, WithSignalStore, WithLogger, WithExecutionID, WithExecutionCallbacks, WithStepProgressStore, WithActivityLogger, WithScriptCompiler. Run method collapse: Execution now exposes exactly one run method, with resume as an option: exec.Execute(ctx) exec.Execute(ctx, workflow.ResumeFrom(priorID)) Deleted Run, Resume, RunOrResume, ExecuteOrResume. The Execute(ctx, ResumeFrom(id)) form silently falls back to a fresh run when no checkpoint is found, matching the old RunOrResume semantics. Runner options also move to functional form: runner := workflow.NewRunner( workflow.WithRunnerLogger(l), workflow.WithDefaultTimeout(5*time.Minute), ) result, err := runner.Run(ctx, exec, workflow.WithHeartbeat(hb), workflow.WithCompletionHook(hook), workflow.WithRunTimeout(30*time.Second), workflow.WithResumeFrom(priorID), ) Activity function renames (http.HandlerFunc style): NewActivityFunction → ActivityFunc NewTypedActivityFunction → TypedActivityFunc Internal struct types activityFunc / typedActivityFunc are now unexported. ActivityRegistry is an opaque struct with Register / MustRegister / Get / Names — the old type alias `map[string]Activity` is gone. Register returns ErrDuplicateActivity on repeat names. Also fixes the runner's completion-hook behavior: FollowUps are attached even when the hook returns an error (logged separately). Every example, test, and consumer site is updated to the new shape. Old tests that exercised the Run/Resume/RunOrResume error return contract were rewritten to match the new (*ExecutionResult, error) shape. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR5: binding validation in NewExecution (#24) Adds phase-2 validation that runs after NewExecution binds the ActivityRegistry and script.Compiler: - Unknown activity references - Parameter templates ("${...}") and $() expressions compile - Edge condition expressions compile - WaitSignalConfig.Topic templates compile - Store fields (Step.Store, WaitSignalConfig.Store, CatchConfig.Store, Output.Variable) reject "state." prefix - Warn (do not error) when WaitSignal is used without a SignalStore New sentinels: ErrUnknownActivity, ErrInvalidTemplate, ErrInvalidExpression, ErrInvalidStorePath. All problems collected into *ValidationError so errors.Is works. Updates tests and examples to use bare variable names for Store fields. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR6: Context becomes idiomatic Go (#25) Fold VariableContainer and the SignalAware / ActivityHistoryAware / ProgressReporter side-interfaces into Context, and rename the GetFoo/ListFoo accessors to property-style. Context now exposes: Inputs() Inputs Set/Get/Delete/Keys (variables) Logger, Compiler, BranchID, StepName Wait(topic, timeout) History() *History ReportProgress(detail) Package-level helpers workflow.Wait, workflow.ActivityHistory, workflow.ReportProgress, InputsFromContext, and VariablesFromContext are gone. Activity code calls the Context methods directly. Inputs is a named struct (map wrapper) with Get/Keys/Len/ToMap so we can grow typed accessors later without reopening the interface. New workflowtest.FakeContext lets consumer tests unit-test activities without spinning up a real Execution. NewFakeContext takes a FakeContextOptions and returns a fully usable workflow.Context. Two small helper constructors (workflow.NewInputsForTest, workflow.NewHistoryForTest) exist so FakeContext can build these values without reaching into package internals. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR7: checkpoint stable wire format (#26) Add SchemaVersion to Checkpoint with a defined forward-compatibility contract: readers reject checkpoints with a version newer than the library, so rolling back after a wire-format bump fails loudly instead of silently dropping fields. Change Status from bare string to the ExecutionStatus typed constant so the JSON shape is enforced by the type system. Rename lingering "path_states" / "path_counter" JSON keys to "branch_states" / "branch_counter" to match the PR1 rename. Add an AtomicCheckpointer optional side interface: backends with transactional primitives (Postgres row-lock, Redis CAS) can implement AtomicUpdate to close the load-modify-write race that bare PauseBranchInCheckpoint / UnpauseBranchInCheckpoint would otherwise open when a host process is concurrently writing the same execution. MemoryCheckpointer implements it; mutatePauseInCheckpoint prefers it when available and falls back to load-modify-write otherwise. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR8: single template syntax (#27) One template syntax, contextual type inference. Delete $(...) parsing entirely; ${...} is the only form. When a parameter value is a single ${...} covering the whole trimmed string, the result preserves its native type; otherwise it is interpolated as a string. Conditions and each.Items are raw expressions. - script/eval.go: parse only ${...}; single-expression templates preserve typed values, interpolated templates return strings. - script/eval_test.go: cover int/bool/whitespace single-expression and EvalString stringification. - branch.go: drop $(...) handling; conditions and each.Items are raw expressions; parameter templates unified on ${...}. - validate.go: drop $(...) branches in parameter/condition/topic validation. - branch_test.go, coverage_test.go, validate_test.go: rewrite to use ${...} templates. - examples/{child_workflows,branching,simple,structured_result}: migrate to ${...}. - step.go, README.md, llms.txt, CLAUDE.md, examples/join_branches/ README.md: doc/comment cleanup removing $(...) references. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR9: activities tier split + naming (#28) Reorganise the activities/ tree by guarantee level. Core safe activities stay in activities/. HTTP moves to activities/httpx/. Host-touching activities (shell, file) move to activities/contrib/. Delete the in-process wait activity — durable Sleep replaces it. - activities/contrib/{shell,file}_activity.go: moved + repackaged - activities/httpx/http_activity.go: moved + repackaged - activities/{contrib,httpx}/helpers_test.go: per-package newTestContext - activities/wait_activity{,_test}.go: deleted - activities/print_activity.go: PrintActivity now wraps an io.Writer. NewPrintActivity() defaults to os.Stdout; NewPrintActivityTo(w) injects a custom writer for tests and embedded uses. - activities/contrib/shell_activity.go: ShellInput.Timeout becomes time.Duration (was float64 seconds). - activities/httpx/http_activity.go: HTTPInput.Timeout becomes time.Duration; default of 30s preserved. - activities/child_workflow_activity.go: ChildWorkflowInput.Timeout becomes time.Duration; the float→Duration conversion in Execute is gone. - cmd/workflow/main.go: import contrib + httpx, drop the wait activity from the registry. - examples/simple/main.go: drop the wait step (it was just a sleep between loop iterations) and the wait activity registration. - README.md, llms.txt: doc updates for the new sub-packages, PrintActivity io.Writer story, and rename of NewActivityFunction → ActivityFunc / NewTypedActivityFunction → TypedActivityFunc that PR4 made but the docs still referenced. activity_functions_test.go and typed_activity_example_test.go: matching test/comment renames. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR10: child workflow fixes (#29) Per decision 1.15: keep child workflows in the public API and fix the concrete bugs. - ChildWorkflowSpec.Sync deleted. The ExecuteSync/ExecuteAsync split on the executor interface is the source of truth; flagging on the spec was redundant and confusing. - ChildWorkflowResult.Error dropped. The execution error is the second return value of ExecuteSync/GetResult — duplicating it on the struct led to two sources of truth. - ChildWorkflowExecutorOptions.CleanupTimeout added. Default 1h (was a hardcoded 5min); zero means use default; negative disables eviction entirely. Replaces the magic 5-minute sleep. - ExecuteAsync godoc spells out the async-vs-checkpoint contract: in-process only, dies with the process, parent loses the handle on restart. TODO(v1.1) for durable async-child handles. - activities/child_workflow_activity.go: drop the Sync field from ChildWorkflowInput and the executeSync/executeAsync branching. The bundled `workflow.child` activity is sync-only; consumers needing fire-and-forget build their own activity wrapping executor.ExecuteAsync. - activities/child_workflow_activity_test.go: drop the async path test, drop "sync": true from the remaining cases. - examples/child_workflows/main.go: drop "sync": true, switch timeouts to time.Duration literals. - examples/child_workflows/README.md: rewrite to document the wait-for-completion pattern (which is just `workflow.child` with Step.Store) and the async durability caveat. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR11: error model + completion hook godoc (#30) - ClassifyError no longer substring-matches "timeout" in error messages, and no longer routes context.Canceled into the timeout bucket. Real timeouts must wrap context.DeadlineExceeded or workflow.ErrWaitTimeout. Substring matching of "timeout" was a classic surprise (any error string containing the word would unexpectedly route through timeout catch handlers). - ErrorTypeAll godoc now spells out the fatal-error escape valve: ErrorTypeAll matches everything except ErrorTypeFatal, which is matchable only by an explicit ErrorTypeFatal pattern. - ErrorTypeTimeout godoc spells out the new "real timeouts only" classification rule. - WorkflowError.Details godoc spells out the non-roundtrip contract: Details is any so consumers can attach structure, but Checkpoint.Error is a flat string and Details is dropped on resume. Consumers needing persistent structured details should wrap a custom error type instead. - WorkflowError.Error() now prefixes "workflow: " for consistency with the rest of the package's error strings. - All root-package error sentinels (ErrNoCheckpoint, ErrAlreadyStarted, ErrNilExecution, ErrInvalidHeartbeatInterval, ErrNilHeartbeatFunc) and workflow.New's name/steps fmt.Errorf calls now use the "workflow: " prefix. - errors_test.go and workflow_test.go updated for the new error strings. Note: the runner.go FollowUps-on-hook-error fix and the completion_hook.go godoc were already in place from earlier work, so this PR is godoc + classification only on those files. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR12: ExecutionResult helpers (#31) Add convenience accessors that consumers will actually reach for so they don't have to write the same map-lookup-and-type-assert boilerplate at every call site. Outputs: - ExecutionResult.Output(key) — raw lookup with presence flag. - ExecutionResult.OutputString(key) - ExecutionResult.OutputInt(key) — accepts int/int32/int64/float32/ float64 so JSON-decoded numbers (which arrive as float64) work. - ExecutionResult.OutputBool(key) - workflow.OutputAs[T](r, key) — package-level generic for arbitrary types, including custom structs. Suspension: - ExecutionResult.WaitReason() — dominant SuspensionReason or "". - ExecutionResult.Topics() — union of waited-on signal topics or nil. - ExecutionResult.NextWakeAt() — earliest wall-clock deadline + ok. All accessors are nil-safe on the receiver. Tests cover happy paths, type mismatches, missing keys, and nil receivers. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * PR13: documentation rewrite (#32) The library should be recommendable to someone reading the README, the MIGRATION doc, and the suspension model doc cold. Update the docs to reflect the v1 surface and contracts. Long-form docs: - README.md — quick example uses Runner and the v1 functional-options constructors. Adds a "Going to production" section linking to the new docs. - MIGRATION.md (new) — every breaking change between pre-v1 and v1 with before/after snippets, organised by PR. - docs/suspension.md (new) — the consolidated suspension model. The three reasons table, lifecycle diagram, replay-safety contract, RecordOrReplay shape, scheduling-resume recipe, dominant-reason precedence rule. - docs/production_checklist.md (new) — punch list for taking the library to production: storage, execution, activity authoring, suspension/resume, observability, worker hygiene. - llms.txt — reflects v1 API throughout. Replaces every stale Path/path reference with Branch, every ExecutionOptions struct example with the functional-options constructor, every execution.Run/Resume/RunOrResume with exec.Execute, every GetVariable/SetVariable with Get/Set, every RunnerConfig/RunOptions with NewRunner/Run options, and adds the new ExecutionResult helpers (OutputString/OutputInt/OutputBool/OutputAs/WaitReason/ Topics/NextWakeAt) to the Execution section. Godoc: - checkpoint.go — Checkpoint godoc gets a "Load-bearing fields" section that explicitly lists BranchState.Variables / Wait / PauseRequested / ActivityHistory as round-trip-required. - step.go — Step godoc documents the now-validated "exactly one kind" rule and the modifier-field restrictions. - context.go — Context.Wait godoc spells out the behavior, the replay-safety contract, the deadline rules, and the custom-Context-implementer contract. Includes the canonical RecordOrReplay shape. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add suspend/resume examples, fix stale Context API references Add runnable end-to-end examples for the three suspension primitives (signal wait, durable sleep, operator pause) so consumers have a working template for the full suspend + resume cycle. Fix stale references in llms.txt, docs/suspension.md, and context.go godoc that still showed the pre-fold package-level helpers (workflow.Wait, workflow.ActivityHistory, workflow.ReportProgress, workflow.RecordOrReplay). All four were folded into Context methods in v1; the docs now match the real API. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: sweep leftover Path→Branch identifiers Finish the PR1 rename in the places it missed: JSON field waiting_path_id → waiting_branch_id, the Go field WaitingPathID → WaitingBranchID, the JSON tag path_id → branch_id on ActivityLogEntry.BranchID, and the slog keys path_id/waiting_path → branch_id/waiting_branch across execution.go and branch.go. Also renames local vars and comments that still said "path". Wire format impact: ActivityLogEntry and JoinState JSON both change key names. This matches the pre-v1 MIGRATION guidance that checkpoints written before v1 cannot be loaded — re-run any in-flight executions rather than trying to convert old data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: remove examples/expr expr is now the default ScriptCompiler, so the integration example shows redundant wiring and the rest are pure expr-language demos that belong in the expr repo. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: fix gofmt formatting across 16 files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address CodeRabbit review feedback from PR #34 (#35) - Propagate ExecutionResult in CLI and all examples instead of only checking error (critical: Execute can return nil err with failed status) - Remove stale "wait" activity from CLI help text - Fix stale "Script activities" text in branching example - Add Outputs declaration to README quick-start example - Make ActivityRegistry zero value safe (nil receiver + nil map guard) - Guard against nil Checkpointer in pause/unpause helpers - Propagate failed child workflow executions as errors from ExecuteSync - Reject pre-v1 checkpoints (SchemaVersion < 1) in all checkpoint readers - Reject malformed ${...} templates instead of silently treating as literals - Clarify Runner is recommended but not required in production checklist - Remove dead variablesMap() method from BranchLocalState - Replace require assertions in activity goroutine with error returns Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <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
1 participant
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
Addresses the actionable feedback from CodeRabbit's review of the v1 PR (#34). Changes across 20 files:
Critical: ExecutionResult propagation
cmd/workflow/main.go) now checksresult.Failed()instead of onlyerrjoin_branches,retry_simple,error_handling,branching) updated to useExecutionResultOutputssoOutputString("result")worksCode safety
ActivityRegistry.Registerhandles nil receiver and uninitialized mapmutatePauseInCheckpointguards against nilCheckpointerExecuteSyncpropagates failed child workflow status as an errorSchemaVersion < 1(pre-v1 checkpoints)NewTemplaterejects malformed${...}expressions instead of treating them as literalsCleanup
variablesMap()fromBranchLocalStatewaitactivity from CLI help textRunneris recommended but optional in production checklistTest fixes
requireassertions inside activity goroutines with error returns inbranch_join_test.goTestCheckpointOlderSchemaVersionIsRejectedtestSchemaVersionNot addressed (disagreed or out of scope):
time.DurationJSON serialization — params flow through the engine's binding system, not raw JSONEach.Itemsexpression behavior — needs deeper investigationrequirein goroutines inexecution_test.go— pervasive pattern, needs systematic sweepTest plan
go test ./...passesgo vet ./...passesgo build ./...succeeds (all examples compile)🤖 Generated with Claude Code