Refactor Actor to interface and clean up dependencies#1
Merged
Conversation
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. 📝 WalkthroughWalkthroughRefactors identity to use an Actor interface, updates workflow/module contracts, migrates the test harness to external mdk types, adds a Metadata DB type with tests, and adds CHANGELOG and SECURITY policy. ChangesMDK Type System and Testing Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mdktest/testing.go (2)
178-185:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGoroutine uses detached context, losing cancellation signal.
Executelaunches a goroutine withcontext.Background(), ignoring the caller's context. If the caller expects cancellation to propagate (e.g., test timeout), this won't work. Consider passing the original context or a derived context with the goroutine.Proposed fix
func (twe *TestWorkflowEngine) Execute(ctx context.Context, workflowID string, input map[string]any) (string, error) { val := atomic.AddInt64(&runIDCounter, 1) runID := fmt.Sprintf("wf_run_%d_%d", time.Now().UnixNano(), val) go func() { - _, _ = twe.ExecuteSync(context.Background(), runID, workflowID, input) + _, _ = twe.ExecuteSync(ctx, runID, workflowID, input) }() return runID, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mdktest/testing.go` around lines 178 - 185, The goroutine in TestWorkflowEngine.Execute uses context.Background(), which prevents cancellation from the caller; change the goroutine to pass the caller's ctx (or a derived context like with context.WithCancel/WithTimeout if you need isolation) into ExecuteSync so cancellation/timeouts propagate. Update the anonymous goroutine invocation to call twe.ExecuteSync(ctx, runID, workflowID, input) (or a derivedCtx) instead of context.Background(), keeping runID generation and return behavior unchanged.
129-140: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueFunction pointer comparison has edge cases with closures.
The
reflect.ValueOf(h).Pointer()comparison works correctly for function values that are the same variable instance. However, be aware that two distinct closures with identical code but different captured variables will share the same code pointer, potentially causing unintended matches. This is acceptable for test utilities where the typical usage pattern isunsub, _ := Subscribe(..., handler); unsub().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mdktest/testing.go` around lines 129 - 140, The current removal loop in teb.handlers uses reflect.ValueOf(h).Pointer() to match functions which compares code pointers and can misidentify distinct closures; replace that pointer-based check with a direct function value equality (use if h == handler) inside the loop so closures with different captured state are compared correctly; update the code where reflect.ValueOf(h).Pointer() == reflect.ValueOf(handler).Pointer() is used (inside the unsubscribe closure that locks teb.mu and iterates handlers) to use h == handler instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 7-8: Add a single blank line after the markdown heading "###
Added" so the list item "- Initial modular commerce kernel design and MDK
decoupled integrations." is separated from the heading; update the block
containing the "### Added" heading to insert one empty line immediately
following that heading.
In `@SECURITY.md`:
- Line 5: Update the policy sentence "We currently support and patch security
vulnerabilities on the latest major release branch." to explicitly cover pre-1.0
semantics: replace or append it with a clear statement such as "For pre-1.0
releases we support the latest release or the latest patch series (e.g., 0.1.x),
and for 1.0+ we support the latest major release branch," so readers understand
whether you mean the latest release, latest patch series (0.x), or latest major
branch during the 0.x phase; edit the SECURITY.md line accordingly to include
that clarified scope.
---
Outside diff comments:
In `@mdktest/testing.go`:
- Around line 178-185: The goroutine in TestWorkflowEngine.Execute uses
context.Background(), which prevents cancellation from the caller; change the
goroutine to pass the caller's ctx (or a derived context like with
context.WithCancel/WithTimeout if you need isolation) into ExecuteSync so
cancellation/timeouts propagate. Update the anonymous goroutine invocation to
call twe.ExecuteSync(ctx, runID, workflowID, input) (or a derivedCtx) instead of
context.Background(), keeping runID generation and return behavior unchanged.
- Around line 129-140: The current removal loop in teb.handlers uses
reflect.ValueOf(h).Pointer() to match functions which compares code pointers and
can misidentify distinct closures; replace that pointer-based check with a
direct function value equality (use if h == handler) inside the loop so closures
with different captured state are compared correctly; update the code where
reflect.ValueOf(h).Pointer() == reflect.ValueOf(handler).Pointer() is used
(inside the unsubscribe closure that locks teb.mu and iterates handlers) to use
h == handler instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da9863fc-78ca-404f-88a5-d6e7bb1de333
📒 Files selected for processing (9)
CHANGELOG.mdSECURITY.mdidentity.gomdktest/testing.gomodels.gomodels_test.gomodule.goregistry.goworkflow.go
💤 Files with no reviewable changes (1)
- registry.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mdktest/testing.go (3)
114-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn subscriber failures from
Publish.
Publishdrops every handler error and still returnsnil. That makes this test double report success for flows that should fail, so tests won't catch error-handling regressions aroundmdk.EventBus.Publish.Suggested fix
func (teb *TestEventBus) Publish(ctx context.Context, e mdk.Event) error { teb.mu.Lock() if e.OccurredAt.IsZero() { e.OccurredAt = time.Now() } teb.Published = append(teb.Published, e) teb.mu.Unlock() @@ + var firstErr error for _, h := range handlers { - _ = h(ctx, e) + if err := h(ctx, e); err != nil && firstErr == nil { + firstErr = err + } } for _, h := range wildcardHandlers { - _ = h(ctx, e) + if err := h(ctx, e); err != nil && firstErr == nil { + firstErr = err + } } - return nil + return firstErr }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mdktest/testing.go` around lines 114 - 120, Publish currently ignores errors from subscriber calls in the loops over handlers and wildcardHandlers and always returns nil; update the Publish implementation in testing.go to collect subscriber errors (from handlers and wildcardHandlers) and return an aggregated or first error instead of nil. Specifically, in the function containing the handlers/wildcardHandlers loops, capture each h(ctx, e) error, append to a slice or use an error-aggregator, and at the end return a combined error (or the first non-nil error) so caller/tests receive failure information from subscriber handlers.
178-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
Executecannot start a run.Line 182 discards
ExecuteSync's error inside the goroutine, soExecutereturns(runID, nil)even whenworkflowIDis unknown. Becauseruns[runID]is only initialized insideExecuteSync, an immediateStatuscall can also see""instead of a validmdk.StepStatus. CheckworkflowIDbefore spawning the goroutine and seed the run asmdk.StepPending.Suggested fix
func (twe *TestWorkflowEngine) Execute(ctx context.Context, workflowID string, input map[string]any) (string, error) { + twe.mu.RLock() + _, ok := twe.workflows[workflowID] + twe.mu.RUnlock() + if !ok { + return "", fmt.Errorf("workflow not found: %s", workflowID) + } + val := atomic.AddInt64(&runIDCounter, 1) runID := fmt.Sprintf("wf_run_%d_%d", time.Now().UnixNano(), val) + + twe.mu.Lock() + twe.runs[runID] = mdk.StepPending + twe.mu.Unlock() + go func() { _, _ = twe.ExecuteSync(ctx, runID, workflowID, input) }() return runID, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mdktest/testing.go` around lines 178 - 184, The Execute method currently spawns a goroutine and discards ExecuteSync's error, causing Execute to always return (runID, nil) and leaving runs[runID] uninitialized; fix this by validating the workflowID and seeding the run before launching the goroutine: call the same validation/initialization logic that ExecuteSync uses (or call ExecuteSync synchronously up to the point it seeds runs) to ensure runs[runID] = mdk.StepPending and return an error if validation fails, then start the background goroutine to continue execution; update TestWorkflowEngine.Execute to perform these checks and seeding (referencing ExecuteSync, runs map, runID generation, and mdk.StepPending).
129-139:⚠️ Potential issue | 🟠 MajorReplace
reflect.Value.Pointer()with an explicit subscription ID for unsubscribe.
reflect.Value.Pointer()onFuncvalues returns only the underlying code pointer and is not guaranteed to uniquely identify a specific closure/method value instance (the docs only guarantee it’s zero for a nil func). Using it can cause unsubscribing one subscription to remove a different handler that happens to share the same underlying pointer. Store a generated subscription ID alongside each handler and remove by ID instead.
Filemdktest/testing.golines 129-139.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mdktest/testing.go` around lines 129 - 139, The unsubscribe closure currently matches handlers by reflect.ValueOf(h).Pointer(), which is unsafe; instead, change the handlers storage (teb.handlers) to hold a small struct with a generated subscription ID and the handler function (e.g., {id string; fn HandlerType}) when registering, generate a unique id (UUID or incremental) and return an unsubscribe function that captures that id, and then implement removal by comparing ids (remove the entry whose id equals the captured id) in the code path that currently iterates handlers (the block referencing teb.handlers[key], handler, and the returned func). Ensure locking (teb.mu.Lock/Unlock) stays the same and that the registration API returns the unsubscribe func and nil error as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@mdktest/testing.go`:
- Around line 114-120: Publish currently ignores errors from subscriber calls in
the loops over handlers and wildcardHandlers and always returns nil; update the
Publish implementation in testing.go to collect subscriber errors (from handlers
and wildcardHandlers) and return an aggregated or first error instead of nil.
Specifically, in the function containing the handlers/wildcardHandlers loops,
capture each h(ctx, e) error, append to a slice or use an error-aggregator, and
at the end return a combined error (or the first non-nil error) so caller/tests
receive failure information from subscriber handlers.
- Around line 178-184: The Execute method currently spawns a goroutine and
discards ExecuteSync's error, causing Execute to always return (runID, nil) and
leaving runs[runID] uninitialized; fix this by validating the workflowID and
seeding the run before launching the goroutine: call the same
validation/initialization logic that ExecuteSync uses (or call ExecuteSync
synchronously up to the point it seeds runs) to ensure runs[runID] =
mdk.StepPending and return an error if validation fails, then start the
background goroutine to continue execution; update TestWorkflowEngine.Execute to
perform these checks and seeding (referencing ExecuteSync, runs map, runID
generation, and mdk.StepPending).
- Around line 129-139: The unsubscribe closure currently matches handlers by
reflect.ValueOf(h).Pointer(), which is unsafe; instead, change the handlers
storage (teb.handlers) to hold a small struct with a generated subscription ID
and the handler function (e.g., {id string; fn HandlerType}) when registering,
generate a unique id (UUID or incremental) and return an unsubscribe function
that captures that id, and then implement removal by comparing ids (remove the
entry whose id equals the captured id) in the code path that currently iterates
handlers (the block referencing teb.handlers[key], handler, and the returned
func). Ensure locking (teb.mu.Lock/Unlock) stays the same and that the
registration API returns the unsubscribe func and nil error as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5fa66935-0888-4f25-89e2-14ecd7fd1055
📒 Files selected for processing (3)
CHANGELOG.mdSECURITY.mdmdktest/testing.go
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.
Refactors mdk.Actor to an interface, decouples db model in auth, introduces lightweight local/mem CloudProvider, simplifies HTMLUIProvider, cleans dependencies, and updates tests.
Summary by CodeRabbit
Documentation
New Features
Refactor
Tests