start making this into a php extension #160
Conversation
Signed-off-by: Robert Landers <[email protected]>
WalkthroughAdds a FrankenPHP CGO-based PHP extension and Go worker with NATS JetStream feeders, in-process local PHP clients, helper/state abstractions, a new xcaddy/frankenphp Makefile target, wide Go module/import path changes, and refactors Glue to use the Worker interface for in-process request handling and state/event IO. Changes
Sequence Diagram(s)sequenceDiagram
participant PHP as PHP App / Glue
participant Ext as PHP Extension (C)
participant Go as Go Worker
participant NATS as NATS JetStream
participant FP as FrankenPHP runtime
PHP->>Ext: Bottledcode\DurablePhp\Ext\emit_event(user,event,from)
Ext->>Go: cgo call -> emit_event
Go->>NATS: Publish event (JetStream)
NATS-->>Go: Ack
Go-->>Ext: return ack
Ext-->>PHP: return ack
Note over Go,NATS: Background feeders deliver messages to Go
NATS->>Go: Deliver message
Go->>FP: ProvideRequest (/worker, headers, body)
FP->>PHP: frankenphp_handle_request -> Glue->process()
PHP-->>FP: Response
FP-->>Go: Request complete
Go->>NATS: Ack/Nak based on outcome
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #160 +/- ##
=====================================
Coverage ? 45.56%
Complexity ? 1359
=====================================
Files ? 106
Lines ? 3814
Branches ? 0
=====================================
Hits ? 1738
Misses ? 2076
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (3)
cli/lib/api.go (1)
257-263:ctx.Value("bootstrap").(string)can panic when the key is absent
bootstrap := ctx.Value("bootstrap").(string)assumes the key is always set.
If a caller forgets to decorate the root context with"bootstrap", the server will crash the moment it receives the first request.- bootstrap := ctx.Value("bootstrap").(string) + raw := ctx.Value("bootstrap") + if raw == nil { + logger.Fatal("missing bootstrap value in context") + } + bootstrap, _ := raw.(string)Alternatively, pass the bootstrap string explicitly instead of relying on a magic context key.
cli/glue/glue.go (1)
268-273: State file is closed before the caller can use itBoth branches of
GetStateFilecalldefer stateFile.Close()immediately afteros.CreateTemp.
The deferred close executes before the function returns, so the closed file descriptor is returned to the caller, leading to silent I/O errors later.- stateFile, err := os.CreateTemp("", "state") - defer stateFile.Close() + stateFile, err := os.CreateTemp("", "state") ... - defer stateFile.Close()Move
stateFile.Close()into the cleanup function you already return, or let the caller deal with it.Also applies to: 322-328
cli/test.php (1)
11-11: Fix syntax error: incomplete closing PHP tagThe file ends with an incomplete closing tag.
Apply this diff to fix the syntax error:
-} +} +?>
🧹 Nitpick comments (8)
frankenphp-ext/ext/client.go (2)
14-17: Empty moduleInit function needs implementationThe
moduleInitfunction is currently empty. Consider adding necessary initialization logic or documenting why it's intentionally empty.// export_php:module init func moduleInit() { - // this is during init + // Module initialization - currently no setup required + // TODO: Add any necessary initialization logic here }
19-34: Function implementation looks solid with minor concernsThe
repeat_thisfunction implementation is generally good, but there are a few considerations:
- The
idStrparameter is unused - consider removing it or documenting its purpose- The string reversal logic is correct and handles Unicode properly using runes
- Memory management with
frankenphp.PHPString(result, false)appears appropriate-// export_php:function repeat_this(string $id, string $str, int $count, bool $reverse): string -func repeat_this(idStr *C.zend_string, s *C.zend_string, count int, reverse bool) unsafe.Pointer { +// export_php:function repeat_this(string $str, int $count, bool $reverse): string +func repeat_this(s *C.zend_string, count int, reverse bool) unsafe.Pointer {If the
idStrparameter is required for the PHP signature, add a comment explaining its purpose.cli/test.http (1)
1-1: Consider expanding test coverage and documentationThe single HTTP test is a good start but could be enhanced:
- Add more test cases to cover different scenarios
- Consider adding request headers if needed
- Document expected responses
+# Test the PHP extension functionality GET localhost:8080/test.php + +### +# Add more test cases as needed +# GET localhost:8080/test.php?param=valuecli/lib/api.go (1)
30-37: Use cryptographically-secure randomness for correlation IDs
generateCorrelationIdrelies onmath/rand, which is deterministic by default and not suitable when IDs are exposed outside the system (e.g., in headers).
Considercrypto/randor at least seedingmath/randonce at startup.cli/glue/response_writer.go (1)
66-75: Potential memory growth when logging large bodies
c.Data += string(b)repeatedly copies and appends the entire byte slice, causing quadratic memory usage for large responses.
If this writer is ever used for sizable payloads, switch to abytes.Bufferor stream the data.cli/test.php (1)
1-10: Consider adding proper test assertionsThis appears to be a manual test script rather than an automated test. Consider using PHPUnit or another testing framework to add proper assertions and test coverage.
Would you like me to help generate a proper PHPUnit test suite for the extension?
cli/Makefile (1)
25-26: Add standard Makefile targetsConsider adding standard targets for better developer experience.
Add these standard targets:
-.PHONY: frankenphp +.PHONY: all clean test frankenphp + +all: frankenphp + +clean: + rm -f frankenphp + rm -rf ext/build/ + +test: frankenphp + ./frankenphp php-cli test.phpcli/ext/ext.go (1)
164-164: Use of zero duration in billing initializationThe expression
0*time.Minuteis redundant and could be confusing.Simplify the initialization:
-billings.Store("a", 0*time.Minute) +billings.Store("a", time.Duration(0))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
cli/Makefile(1 hunks)cli/auth/keys.go(1 hunks)cli/auth/keys_test.go(1 hunks)cli/auth/resource.go(1 hunks)cli/auth/resourceManager.go(1 hunks)cli/auth/resource_test.go(1 hunks)cli/auth/user.go(1 hunks)cli/cli.go(2 hunks)cli/export.go(1 hunks)cli/ext/ext.go(1 hunks)cli/ext/helpers/helpers.go(1 hunks)cli/glue/glue.go(1 hunks)cli/glue/response_writer.go(1 hunks)cli/glue/state.go(1 hunks)cli/go.mod(2 hunks)cli/lib/api.go(1 hunks)cli/lib/billing.go(1 hunks)cli/lib/consumer.go(1 hunks)cli/lib/index.go(1 hunks)cli/lib/indexer.go(1 hunks)cli/lib/locks.go(1 hunks)cli/test.http(1 hunks)cli/test.php(1 hunks)frankenphp-ext/ext/client.go(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
cli/Makefile
[warning] 11-11: Target body for "frankenphp" exceeds allowed length of 5 (8).
(maxbodylength)
[warning] 25-25: Missing required phony target "all"
(minphony)
[warning] 25-25: Missing required phony target "clean"
(minphony)
[warning] 25-25: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (18)
frankenphp-ext/ext/client.go (1)
1-8: LGTM - Clean package structure and importsThe package declaration and imports are well-organized. The use of
unsafeis appropriate for PHP extension development, and all necessary dependencies are included.cli/lib/index.go (1)
5-5: LGTM - Import path refactoring is consistentThe import path update from relative to fully qualified module path aligns with the broader refactoring mentioned in the AI summary. This improves module clarity and supports the new build system.
cli/glue/state.go (1)
5-5: LGTM - Consistent import path modernizationThe import path update for the
idspackage follows the same pattern as other files in this PR, maintaining consistency across the codebase refactoring.cli/lib/locks.go (1)
6-6: LGTM - Completing the import path consistencyThe import path update for the
idspackage completes the consistent refactoring pattern seen across all files in this PR.cli/auth/resource_test.go (1)
6-6: LGTM: Import path correctly updated to fully qualified module path.The change from relative import to fully qualified module path aligns with the broader refactoring effort and follows Go best practices.
cli/auth/user.go (1)
5-5: LGTM: Import path correctly updated to fully qualified module path.The import path change is consistent with the module restructuring and maintains all existing functionality.
cli/auth/resourceManager.go (1)
6-8: LGTM: Import paths correctly updated to fully qualified module paths.All three import path changes are consistent with the module restructuring effort and follow the same pattern as other files in the refactoring.
cli/auth/resource.go (1)
8-10: LGTM: Import paths correctly updated to fully qualified module paths.The import path changes are consistent with the broader module restructuring and maintain all existing functionality.
cli/lib/billing.go (1)
7-9: LGTM: Import paths correctly updated to fully qualified module paths.The import path changes are consistent with the overall refactoring effort and preserve all billing functionality.
cli/export.go (1)
1-5: Build-tag stub looks correctThe file cleanly separates the build for
nodphpby providing only a side-effect import. No issues spotted.cli/auth/keys_test.go (1)
5-7: Import path update looks goodImports now match the new module namespace; tests still compile. No further action needed.
cli/lib/indexer.go (1)
6-8: Import path refactoring looks good.The import paths have been correctly updated to use fully qualified module paths, which aligns with the broader module restructuring effort for the PHP extension development.
cli/auth/keys.go (1)
8-9: Import path updates are correct.The import paths have been properly updated to use the new fully qualified module path structure.
cli/lib/consumer.go (1)
7-11: All import path updates are consistent and correct.The five internal package imports have been properly updated to use the new fully qualified module path structure, maintaining consistency across the codebase.
cli/cli.go (2)
1-2: Good use of build constraint for PHP extension development.The
!nodphpbuild constraint appropriately excludes this CLI main function when building the PHP extension, allowing for different build targets.
31-36: Import path refactoring is consistent and correct.All internal package imports have been updated to use the new fully qualified module paths, maintaining consistency with the broader module restructuring.
cli/go.mod (2)
1-1: Module path update is essential for the refactoring.The module path change to
github.com/bottledcode/durable-php/cliproperly establishes the Go module identity and enables the import path standardization across all files.
9-9: Please review the changelogs for the updated dependencies to confirm no breaking changesI attempted to fetch the GitHub release notes for each new version but the API returned empty bodies. To be safe, please manually verify the CHANGELOGs or migration guides for:
- github.com/nats-io/nats-server/v2 v2.11.7
- github.com/oapi-codegen/runtime v1.1.2
- github.com/prometheus/client_golang v1.23.0
You may also run your test suite (
go test ./...) and/or a vulnerability scanner (e.g.govulncheck) to catch any unexpected issues.
cli/ext/ext.go
Outdated
| if !billings.CompareAndSwap(key, old, old.(int)+1) { | ||
| incrementInt(key, amount) |
There was a problem hiding this comment.
Infinite recursion risk in increment functions
The recursive retry on CAS failure could cause stack overflow under high contention.
Use a loop instead of recursion:
incrementInt = func(key string, amount int) {
- var old interface{}
- old, _ = billings.Load(key)
- if !billings.CompareAndSwap(key, old, old.(int)+1) {
- incrementInt(key, amount)
+ for {
+ old, _ := billings.Load(key)
+ if billings.CompareAndSwap(key, old, old.(int)+amount) {
+ break
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !billings.CompareAndSwap(key, old, old.(int)+1) { | |
| incrementInt(key, amount) | |
| incrementInt = func(key string, amount int) { | |
| for { | |
| old, _ := billings.Load(key) | |
| if billings.CompareAndSwap(key, old, old.(int)+amount) { | |
| break | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In cli/ext/ext.go around lines 171 to 172, the incrementInt function uses
recursion to retry on CompareAndSwap failure, which risks infinite recursion and
stack overflow under high contention. Replace the recursive call with a loop
that repeatedly attempts the CompareAndSwap operation until it succeeds,
ensuring safe and efficient retry without growing the call stack.
cli/ext/ext.go
Outdated
| incrementInt("e", 1) | ||
| msg.Ack() | ||
| }) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| defer consume.Drain() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential resource leak with consumers
The consumer drains are deferred but may not execute if the init panics.
Consider storing consumers globally and cleaning them up in shutdown:
var (
activeConsumers []jetstream.ConsumeContext
consumerMu sync.Mutex
)
// In init, after creating each consumer:
consumerMu.Lock()
activeConsumers = append(activeConsumers, consume)
consumerMu.Unlock()
// In shutdown:
consumerMu.Lock()
for _, c := range activeConsumers {
c.Drain()
}
consumerMu.Unlock()Also applies to: 273-279, 290-302
🤖 Prompt for AI Agents
In cli/ext/ext.go around lines 256 to 262, the deferred calls to consume.Drain()
may not run if the init function panics, causing a resource leak. To fix this,
store all created consumers in a global slice protected by a mutex during
initialization, and then in the shutdown function, iterate over this slice to
call Drain() on each consumer, ensuring proper cleanup even if init panics.
Apply the same pattern to the consumer creation blocks at lines 273-279 and
290-302.
| var Logger *zap.Logger | ||
| var NatsState string | ||
| var Js jetstream.JetStream | ||
| var Config *config.Config |
There was a problem hiding this comment.
Consider protecting global state from concurrent access
These global variables could be accessed concurrently from multiple PHP requests, potentially causing race conditions.
Consider using sync mechanisms or request-scoped storage:
var (
loggerMu sync.RWMutex
Logger *zap.Logger
// ... other globals with appropriate synchronization
)🤖 Prompt for AI Agents
In cli/ext/helpers/helpers.go around lines 40 to 43, the global variables
Logger, NatsState, Js, and Config are vulnerable to concurrent access issues. To
fix this, introduce synchronization primitives like sync.RWMutex for each global
variable or group them under a mutex to protect read and write operations. Wrap
all accesses and modifications of these globals with the appropriate lock and
unlock calls to prevent race conditions during concurrent use.
cli/ext/helpers/helpers.go
Outdated
| case "targetType": | ||
| ev.TargetType = v.(string) | ||
| case "scheduleAt": | ||
| ev.ScheduleAt, err = time.Parse(time.RFC3339, v.(string)) |
There was a problem hiding this comment.
Validate time format parsing
Time parsing can fail with invalid input, but the error is not checked immediately.
Check the error immediately:
case "scheduleAt":
ev.ScheduleAt, err = time.Parse(time.RFC3339, v.(string))
+ if err != nil {
+ return nil, fmt.Errorf("invalid scheduleAt format: %w", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ev.ScheduleAt, err = time.Parse(time.RFC3339, v.(string)) | |
| case "scheduleAt": | |
| ev.ScheduleAt, err = time.Parse(time.RFC3339, v.(string)) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid scheduleAt format: %w", err) | |
| } |
🤖 Prompt for AI Agents
In cli/ext/helpers/helpers.go at line 73, the error returned by time.Parse when
parsing ev.ScheduleAt is not checked immediately. Modify the code to check the
error right after calling time.Parse, and handle it appropriately (e.g., return
the error or log it) before proceeding further.
| func GetUserContext(arr *frankenphp.Array) *auth.User { | ||
| user := &auth.User{} | ||
|
|
||
| for i := uint32(0); i < arr.Len(); i++ { | ||
| k, v := arr.At(i) | ||
| if k.Type == frankenphp.PHPStringKey { | ||
| if k.Str == "userId" { | ||
| user.UserId = auth.UserId(v.(string)) | ||
| } | ||
| if k.Str == "roles" { | ||
| for j := uint32(0); j < v.(*frankenphp.Array).Len(); j++ { | ||
| _, n := v.(*frankenphp.Array).At(j) | ||
| user.Roles = append(user.Roles, auth.Role(n.(string))) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return user | ||
| } |
There was a problem hiding this comment.
Add nil checks to prevent panics
The function doesn't validate that the input array is non-nil.
Add validation at the beginning:
func GetUserContext(arr *frankenphp.Array) *auth.User {
+ if arr == nil {
+ return &auth.User{}
+ }
user := &auth.User{}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func GetUserContext(arr *frankenphp.Array) *auth.User { | |
| user := &auth.User{} | |
| for i := uint32(0); i < arr.Len(); i++ { | |
| k, v := arr.At(i) | |
| if k.Type == frankenphp.PHPStringKey { | |
| if k.Str == "userId" { | |
| user.UserId = auth.UserId(v.(string)) | |
| } | |
| if k.Str == "roles" { | |
| for j := uint32(0); j < v.(*frankenphp.Array).Len(); j++ { | |
| _, n := v.(*frankenphp.Array).At(j) | |
| user.Roles = append(user.Roles, auth.Role(n.(string))) | |
| } | |
| } | |
| } | |
| } | |
| return user | |
| } | |
| func GetUserContext(arr *frankenphp.Array) *auth.User { | |
| if arr == nil { | |
| return &auth.User{} | |
| } | |
| user := &auth.User{} | |
| for i := uint32(0); i < arr.Len(); i++ { | |
| k, v := arr.At(i) | |
| if k.Type == frankenphp.PHPStringKey { | |
| if k.Str == "userId" { | |
| user.UserId = auth.UserId(v.(string)) | |
| } | |
| if k.Str == "roles" { | |
| for j := uint32(0); j < v.(*frankenphp.Array).Len(); j++ { | |
| _, n := v.(*frankenphp.Array).At(j) | |
| user.Roles = append(user.Roles, auth.Role(n.(string))) | |
| } | |
| } | |
| } | |
| } | |
| return user | |
| } |
🤖 Prompt for AI Agents
In cli/ext/helpers/helpers.go around lines 81 to 100, the GetUserContext
function lacks a nil check for the input array, which can cause panics if a nil
pointer is passed. Add a check at the start of the function to return nil or an
empty user if the input arr is nil, preventing dereferencing a nil pointer.
| ../dist: ${BIN_PATH}/${TARGET} | ||
| docker create --name builder builder | ||
| docker cp ${DOCKER_IMAGE}:${BUILD_PATH} ../dist | ||
| LOCAL_MODULE := /home/withinboredom/code/durable-php/cli |
There was a problem hiding this comment.
Remove hardcoded absolute paths for portability
The hardcoded paths will break when run on different machines or environments.
Use relative paths or environment variables instead:
-LOCAL_MODULE := /home/withinboredom/code/durable-php/cli
+LOCAL_MODULE := $(shell pwd)
# ... other code ...
- GEN_STUB_SCRIPT=/home/withinboredom/code/php-src/build/gen_stub.php \
+ GEN_STUB_SCRIPT=$${GEN_STUB_SCRIPT:-$(shell which gen_stub.php)} \Also applies to: 22-22
🤖 Prompt for AI Agents
In cli/Makefile at lines 8 and 22, replace the hardcoded absolute paths with
relative paths or environment variables to improve portability across different
machines and environments. Identify the base directory relative to the Makefile
location or use environment variables to dynamically set the path, ensuring the
Makefile works correctly regardless of the user's file system structure.
| func init() { | ||
| frankenphp.RegisterExtension(unsafe.Pointer(&C.ext_module_entry)) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify extension registration pattern
The extension registration using unsafe.Pointer(&C.ext_module_entry) follows the FrankenPHP pattern, but ensure that ext_module_entry is properly defined in the corresponding C header file.
🏁 Script executed:
#!/bin/bash
# Description: Check for C header files and ext_module_entry definition
# Expected: Find ext_module_entry definition in C header files
fd -e h -e c . --exec grep -l "ext_module_entry" {}Length of output: 53
🏁 Script executed:
#!/usr/bin/env bash
# Display the CGo preamble in client.go to see where ext_module_entry is declared
sed -n '1,200p' frankenphp-ext/ext/client.go
# Search for ext_module_entry in the Go file itself (CGo block)
grep -n "ext_module_entry" frankenphp-ext/ext/client.goLength of output: 910
Missing C module entry (ext_module_entry) definition
I couldn’t find a zend_module_entry ext_module_entry symbol in any C source or header under frankenphp-ext/ext. The call to unsafe.Pointer(&C.ext_module_entry) in client.go will fail to link unless the extension’s module entry is defined and exposed to CGo.
Please:
-
Add a C source (e.g.
ext.c) defining the module entry:#include <php.h> zend_module_entry ext_module_entry = { STANDARD_MODULE_HEADER, "my_ext", // extension name NULL, // functions moduleInit, // MINIT NULL, // MSHUTDOWN NULL, // RINIT NULL, // RSHUTDOWN NULL, // MINFO "0.1.0", // version STANDARD_MODULE_PROPERTIES };
-
Declare it in a header (e.g.
ext.h):extern zend_module_entry ext_module_entry;
-
Include that header in your Go file’s CGo preamble (before
import "C"):/* #include "ext.h" */ import "C"
Once the symbol is defined and the header is imported,
frankenphp.RegisterExtension(unsafe.Pointer(&C.ext_module_entry))
will correctly register the extension.
🤖 Prompt for AI Agents
In frankenphp-ext/ext/client.go around lines 10 to 12, the code references
C.ext_module_entry which is not defined anywhere, causing linking failures. To
fix this, create a C source file (e.g., ext.c) defining the zend_module_entry
ext_module_entry with appropriate fields like extension name, version, and
lifecycle callbacks. Then declare this symbol in a header file (e.g., ext.h)
with 'extern zend_module_entry ext_module_entry;'. Finally, include this header
in the Go file's CGo preamble before 'import "C"' so that the symbol is visible
to Go and the extension registers correctly.
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
cli/ext/ext.go (3)
170-177: Infinite recursion risk in increment functionsThe recursive retry on CAS failure could cause stack overflow under high contention.
Use a loop instead of recursion:
incrementInt = func(key string, amount int) { - var old interface{} - old, _ = billings.Load(key) - if !billings.CompareAndSwap(key, old, old.(int)+1) { - incrementInt(key, amount) + for { + old, _ := billings.Load(key) + if billings.CompareAndSwap(key, old, old.(int)+amount) { + break + } } }
258-305: Potential resource leak with consumersThe consumer drains are commented out and not handled in shutdown, which may cause resource leaks.
Store consumers globally and clean them up in shutdown:
var ( activeConsumers []jetstream.ConsumeContext consumerMu sync.Mutex ) // In init, after creating each consumer: consumerMu.Lock() activeConsumers = append(activeConsumers, consume) consumerMu.Unlock() // In shutdown: consumerMu.Lock() for _, c := range activeConsumers { c.Drain() } consumerMu.Unlock()
387-390: Missing return statement after errorAfter throwing the PHP exception, the function should return to avoid further execution.
Add return statement:
if !authd { helpers.ThrowPHPException("Resource not found") + return 0 }
🧹 Nitpick comments (1)
cli/ext/ext.go (1)
42-313: Consider breaking down the init functionThis function is very long and handles multiple responsibilities. Consider extracting helper functions for:
- NATS server setup (lines 55-74)
- Stream and consumer creation (lines 103-140)
- Extension initialization (lines 142-159)
- Billing setup (lines 161-312)
This would improve readability, testability, and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/ext/ext.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (5)
cli/ext/ext.go (5)
1-26: LGTM!The package declaration and imports are well-organized and appropriate for a PHP extension with NATS JetStream integration.
34-40: LGTM!The shutdown function properly handles cleanup of the NATS server and temporary state directory, addressing previous resource leak concerns.
315-341: LGTM!The authorization function properly handles resource discovery, permission checking, and error cases with appropriate PHP exception throwing.
343-346: LGTM!Simple test function with correct FrankenPHP API usage.
391-441: LGTM with minor fix needed!The event emission logic is well-structured with proper message construction, header handling, and error management. Just needs the return statement fix mentioned above.
Signed-off-by: Robert Landers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
cli/Makefile (1)
13-13: Remove hardcoded absolute paths for portability.These hardcoded paths will break when run on different machines or environments. Use relative paths or environment variables instead.
For
LOCAL_MODULE, use a relative path or environment variable:-LOCAL_MODULE := /home/withinboredom/code/durable-php/cli +LOCAL_MODULE := $(shell pwd)For
GEN_STUB_SCRIPT, make it configurable:- GEN_STUB_SCRIPT=/home/withinboredom/code/php-src/build/gen_stub.php \ + GEN_STUB_SCRIPT=$${GEN_STUB_SCRIPT:-$(shell which gen_stub.php)} \Also applies to: 27-27
cli/ext/helpers/helpers.go (4)
50-55: Consider protecting global state from concurrent access.These global variables could be accessed concurrently from multiple PHP requests, potentially causing race conditions.
Consider using sync mechanisms or request-scoped storage:
var ( loggerMu sync.RWMutex Logger *zap.Logger // ... other globals with appropriate synchronization )
78-94: Add safe type assertions to prevent panics.Direct type assertions without checking can cause panics if the value is not the expected type. Note that line 96 correctly uses safe type assertion - please apply this pattern consistently.
Use safe type assertions:
case "eventId": - ev.EventId = v.(string) + if str, ok := v.(string); ok { + ev.EventId = str + } else { + return nil, errors.New("eventId must be a string") + }
96-98: Check time parsing error immediately.The error from
time.Parseshould be checked immediately to prevent silent failures.case "scheduleAt": if str, ok := v.(string); ok { ev.ScheduleAt, err = time.Parse(time.RFC3339, str) + if err != nil { + return nil, fmt.Errorf("invalid scheduleAt format: %w", err) + } }
107-126: Add input validation and safe type assertions.The function needs both nil checking and safe type assertions to prevent panics.
func GetUserContext(arr *frankenphp.Array) *auth.User { + if arr == nil { + return &auth.User{} + } user := &auth.User{} for i := uint32(0); i < arr.Len(); i++ { k, v := arr.At(i) if k.Type == frankenphp.PHPStringKey { if k.Str == "userId" { - user.UserId = auth.UserId(v.(string)) + if str, ok := v.(string); ok { + user.UserId = auth.UserId(str) + } } if k.Str == "roles" { - for j := uint32(0); j < v.(*frankenphp.Array).Len(); j++ { - _, n := v.(*frankenphp.Array).At(j) - user.Roles = append(user.Roles, auth.Role(n.(string))) + if roles, ok := v.(*frankenphp.Array); ok { + for j := uint32(0); j < roles.Len(); j++ { + _, n := roles.At(j) + if role, ok := n.(string); ok { + user.Roles = append(user.Roles, auth.Role(role)) + } + } } } } }cli/lib/consumer.go (1)
395-578: Critical: Same authorization code duplication as noted above.This authorization logic (lines 395-578) is nearly identical to the logic in
ProcessMessage(lines 175-350). This is the same critical code duplication issue that needs immediate attention through the refactoring suggested in the previous comment.cli/ext/ext.go (1)
175-191: Critical: Infinite recursion risk still unresolved.The recursive retry pattern in
incrementIntandincrementDurfunctions still poses a stack overflow risk under high contention, as mentioned in the previous review.
🧹 Nitpick comments (3)
cli/Makefile (1)
1-31: Consider adding conventional Makefile targets.The build logic is solid, but consider adding conventional phony targets for better Makefile practices:
+all: frankenphp + +clean: + rm -f frankenphp ext/build/ext.go + +test: + php cli/test.php + -.PHONY: frankenphp +.PHONY: all clean test frankenphpcli/lib/consumer.go (2)
21-35: Consider making batch size configurable and improve error handling.The hardcoded batch size of 10 and panic-based error handling have some concerns:
- Hardcoded batch size: The value
10should be configurable or at least documented why this specific value was chosen- Panic usage: While panics may be acceptable in this context, consider if graceful error handling would be more appropriate
Consider making the batch size configurable:
-func StartConsumer(ctx context.Context, config *config.Config, stream jetstream.Stream, logger *zap.Logger, kind ids.IdKind) jetstream.MessagesContext { +func StartConsumer(ctx context.Context, config *config.Config, stream jetstream.Stream, logger *zap.Logger, kind ids.IdKind, batchSize int) jetstream.MessagesContext { logger.Debug("Starting consumer", zap.String("stream", config.Stream), zap.String("kind", string(kind))) consumer, err := stream.Consumer(ctx, config.Stream+"-"+string(kind)) if err != nil { panic(err) } - iter, err := consumer.Messages(jetstream.PullMaxMessages(10), jetstream.WithMessagesErrOnMissingHeartbeat(false)) + iter, err := consumer.Messages(jetstream.PullMaxMessages(batchSize), jetstream.WithMessagesErrOnMissingHeartbeat(false))
368-390: Consider using the getUserFromHeader helper.While the refactoring to use
getStateIdandlockStateIdis good, there's a missed opportunity to also use thegetUserFromHeaderhelper function instead of duplicating the user unmarshaling logic inline.Replace lines 379-390 with:
- // configure the current user - currentUser := &auth.User{} - b := msg.Headers().Get(string(glue.HeaderProvenance)) - err = json.Unmarshal([]byte(b), currentUser) - if err != nil { - logger.Warn("Failed to unmarshal event provenance", - zap.Any("Provenance", msg.Headers().Get(string(glue.HeaderProvenance))), - zap.Error(err), - ) - currentUser = nil - } else { - ctx = auth.DecorateContextWithUser(ctx, currentUser) - } + // configure the current user + currentUser, err := getUserFromHeader(msg) + if err != nil { + logger.Warn("Failed to unmarshal event provenance", + zap.Any("Provenance", msg.Headers().Get(string(glue.HeaderProvenance))), + zap.Error(err), + ) + currentUser = nil + } else { + ctx = auth.DecorateContextWithUser(ctx, currentUser) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cli/Makefile(1 hunks)cli/ext/ext.go(1 hunks)cli/ext/helpers/helpers.go(1 hunks)cli/glue/glue.go(2 hunks)cli/go.mod(2 hunks)cli/lib/api.go(18 hunks)cli/lib/consumer.go(3 hunks)cli/test.php(1 hunks)src/Events/EventDescription.php(4 hunks)src/State/Ids/StateId.php(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/State/Ids/StateId.php
- cli/lib/api.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/go.mod
🧰 Additional context used
🧬 Code Graph Analysis (3)
cli/glue/glue.go (2)
cli/ids/id.go (2)
StateId(188-191)Orchestration(14-14)cli/glue/state.go (1)
GetObjectStore(9-16)
src/Events/EventDescription.php (1)
src/SerializedArray.php (1)
toArray(60-72)
cli/ext/helpers/helpers.go (2)
cli/glue/glue.go (1)
Array(257-259)cli/auth/user.go (3)
User(51-54)UserId(13-13)Role(14-14)
🪛 checkmake (0.2.2)
cli/Makefile
[warning] 30-30: Missing required phony target "all"
(minphony)
[warning] 30-30: Missing required phony target "clean"
(minphony)
[warning] 30-30: Missing required phony target "test"
(minphony)
[warning] 16-16: Target body for "frankenphp" exceeds allowed length of 5 (8).
(maxbodylength)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (15)
cli/glue/glue.go (2)
16-17: LGTM! Import paths updated for module restructuring.The import paths have been correctly updated to fully qualified module paths, supporting the transition to a PHP extension architecture.
287-393: Excellent state management implementation!The
StateArraystruct and its methods provide a clean abstraction over JetStream storage with proper optimistic concurrency control. The implementation correctly handles the distinction between orchestrations (KV store) and other entities (object store), with appropriate error handling throughout.cli/test.php (1)
24-24: Function call parameters have been corrected.The
emit_eventcall now includes all three required parameters as mentioned in previous review comments. The implementation properly demonstrates usage of the new PHP extension with correctly structured event data.src/Events/EventDescription.php (2)
32-32: Excellent serialization attribute improvements!The added
#[Field]attributes provide better control over serialization behavior, particularly theomitIfNullconfiguration for nullable properties and theserializedNamemapping forscheduledAt. This enhances consistency and null handling in the serialization process.Also applies to: 39-39, 42-42, 45-45, 48-48
188-214: Well-designed API separation of concerns.The new
toArray()method provides a clean separation between array construction and JSON encoding, enhancing the API's flexibility. This design allows the PHP extension to work directly with array data while maintaining backward compatibility withtoStream().cli/lib/consumer.go (2)
84-84: LGTM!The change from
getCorrelationIdtoGetCorrelationIdcorrectly uses the exported function, which aligns with the refactoring mentioned in the summary.
113-137: LGTM! Well-structured helper functions.These helper functions improve code organization and readability:
getStateId- Clean wrapper for state ID extractionlockStateId- Good conditional logic to only lock Entity typesgetUserFromHeader- Proper JSON unmarshaling with error handlingcli/ext/ext.go (8)
320-346: LGTM! Clear authorization logic with good error handling.The authorization function properly:
- Checks if authorization is enabled
- Discovers resources with proper error handling
- Validates permissions before allowing operations
- Throws appropriate PHP exceptions for errors
The return tuple
(bool, error)is well-documented in the comments.
387-413: LGTM! Well-structured Worker class with proper validation.The Worker struct and constructor are well-designed:
- Appropriate fields for managing consumer state
- Proper validation of kind parameter with clear error messages
- Good encapsulation of consumer lifecycle management
415-486: LGTM! Comprehensive message processing with good error handling.The
getNextEventmethod properly handles:
- User context extraction and validation
- Delayed message processing with proper NAK
- Delete operations
- Integration with the
ProcessMessagefunction- Consistent PHP exception throwing for errors
The recursive call on line 459 for delayed messages is acceptable since it's only one level deep.
488-535: LGTM! Clean getter methods with proper PHP interop.All the getter methods (
queryState,getUser,getSource,getCurrentId,getCorrelationId,getState) are well-implemented:
- Proper error handling with PHP exceptions
- Good use of frankenphp utilities for type conversion
- Clean access to worker internal state
537-548: LGTM! Simple and correct state management methods.Both
updateStateandemitEventmethods are appropriately implemented:
- Proper PHP/Go array conversion
- Clean state mutation logic
- Event buffering approach is reasonable
551-587: LGTM! Proper user context handling and validation.The function correctly:
- Handles optional user context parameter
- Validates required fields (userId, roles)
- Decorates context with user information
- Determines creation prevention based on operation type
589-641: LGTM! Proper authorization and message construction.The function correctly:
- Uses the
Authorizefunction for permission validation- Constructs comprehensive NATS message headers
- Handles event scheduling with proper time formatting
- Maintains good error handling throughout
642-648: LGTM! Clean message publishing with proper return value.The final section properly:
- Publishes the constructed message to JetStream
- Returns the sequence number for tracking
- Maintains consistent error handling
cli/ext/ext.go
Outdated
| func go_init_module() { | ||
| cfg, err := config.GetProjectConfig() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| helpers.Config = cfg | ||
|
|
||
| helpers.Logger = helpers.GetLogger(zap.DebugLevel) | ||
| logger := helpers.Logger | ||
|
|
||
| logger.Info("Starting Durable PHP") | ||
|
|
||
| helpers.Ctx = context.WithValue(context.Background(), "bootstrap", cfg.Bootstrap) | ||
|
|
||
| boostrapNats := cfg.Nat.Bootstrap | ||
| if cfg.Nat.Internal { | ||
| logger.Warn("Running in dev mode, all data will be deleted at the end of this") | ||
| helpers.NatsState, err = os.MkdirTemp("", "nats-state-*") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| helpers.NatServer = test.RunServer(&server.Options{ | ||
| Host: "localhost", | ||
| Port: 4222, | ||
| NoLog: true, | ||
| NoSigs: true, | ||
| JetStream: true, | ||
| MaxControlLine: 2048, | ||
| StoreDir: helpers.NatsState, | ||
| HTTPPort: 8222, | ||
| }) | ||
| boostrapNats = true | ||
| } | ||
|
|
||
| nopts := []nats.Option{ | ||
| nats.Compression(true), | ||
| nats.RetryOnFailedConnect(true), | ||
| } | ||
|
|
||
| if cfg.Nat.Jwt != "" && cfg.Nat.Nkey != "" { | ||
| nopts = append(nopts, nats.UserCredentials(cfg.Nat.Jwt, cfg.Nat.Nkey)) | ||
| } | ||
|
|
||
| if cfg.Nat.Tls.Ca != "" { | ||
| nopts = append(nopts, nats.RootCAs(strings.Split(cfg.Nat.Tls.Ca, ",")...)) | ||
| } | ||
|
|
||
| if cfg.Nat.Tls.KeyFile != "" { | ||
| nopts = append(nopts, nats.ClientCert(cfg.Nat.Tls.ClientCert, cfg.Nat.Tls.KeyFile)) | ||
| } | ||
|
|
||
| ns, err := nats.Connect(cfg.Nat.Url, nopts...) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| helpers.Js, err = jetstream.New(ns) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ctx := context.WithValue(context.Background(), "bootstrap", cfg.Bootstrap) | ||
|
|
||
| if boostrapNats { | ||
| stream, _ := helpers.Js.CreateStream(ctx, jetstream.StreamConfig{ | ||
| Name: cfg.Stream, | ||
| Description: "Handles durable-php events", | ||
| Subjects: []string{cfg.Stream + ".>"}, | ||
| Retention: jetstream.WorkQueuePolicy, | ||
| Storage: jetstream.FileStorage, | ||
| AllowRollup: false, | ||
| DenyDelete: true, | ||
| DenyPurge: true, | ||
| }) | ||
| _, _ = helpers.Js.CreateStream(ctx, jetstream.StreamConfig{ | ||
| Name: cfg.Stream + "_history", | ||
| Description: "The history of the stream", | ||
| Mirror: &jetstream.StreamSource{ | ||
| Name: cfg.Stream, | ||
| }, | ||
| Retention: jetstream.LimitsPolicy, | ||
| AllowRollup: true, | ||
| MaxAge: 7 * 24 * time.Hour, | ||
| Discard: jetstream.DiscardOld, | ||
| }) | ||
|
|
||
| consumers := []string{ | ||
| string(ids.Activity), | ||
| string(ids.Entity), | ||
| string(ids.Orchestration), | ||
| } | ||
|
|
||
| for _, kind := range consumers { | ||
| _, _ = stream.CreateConsumer(ctx, jetstream.ConsumerConfig{ | ||
| Durable: cfg.Stream + "-" + kind, | ||
| FilterSubject: cfg.Stream + "." + kind + ".>", | ||
| AckPolicy: jetstream.AckExplicitPolicy, | ||
| AckWait: 5 * time.Minute, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| if len(cfg.Extensions.Search.Collections) > 0 { | ||
| for _, collection := range cfg.Extensions.Search.Collections { | ||
| switch collection { | ||
| case "entities": | ||
| err := lib.IndexerListen(ctx, cfg, ids.Entity, helpers.Js, logger) | ||
| if err != nil { | ||
| cfg.Extensions.Search.Collections = []string{} | ||
| logger.Warn("Disabling search extension due to failing to connect to typesense") | ||
| } | ||
| case "orchestrations": | ||
| err := lib.IndexerListen(ctx, cfg, ids.Orchestration, helpers.Js, logger) | ||
| if err != nil { | ||
| cfg.Extensions.Search.Collections = []string{} | ||
| logger.Warn("Disabling search extension due to failing to connect to typesense") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if cfg.Extensions.Billing.Enabled { | ||
| if cfg.Extensions.Billing.Listen { | ||
|
|
||
| billings := sync.Map{} | ||
| billings.Store("e", 0) | ||
| billings.Store("o", 0) | ||
| billings.Store("a", 0*time.Minute) | ||
| billings.Store("ac", 0) | ||
|
|
||
| var incrementInt func(key string, amount int) | ||
| incrementInt = func(key string, amount int) { | ||
| var old interface{} | ||
| old, _ = billings.Load(key) | ||
| if !billings.CompareAndSwap(key, old, old.(int)+1) { | ||
| incrementInt(key, amount) | ||
| } | ||
| } | ||
|
|
||
| var incrementDur func(key string, amount time.Duration) | ||
| incrementDur = func(key string, amount time.Duration) { | ||
| var old interface{} | ||
| old, _ = billings.Load(key) | ||
| if !billings.CompareAndSwap(key, old, old.(time.Duration)+amount) { | ||
| incrementDur(key, amount) | ||
| } | ||
| } | ||
|
|
||
| /* | ||
| outputBillingStatus := func() { | ||
| costC := func(num interface{}, basis int) float64 { | ||
| return float64(num.(int)) * float64(basis) / 10_000_000 | ||
| } | ||
| costA := func(dur interface{}, basis int) float64 { | ||
| duration := dur.(time.Duration) | ||
| seconds := duration.Seconds() | ||
| return float64(basis) * seconds / 100_000 | ||
| } | ||
| avg := func(dur interface{}, count interface{}) time.Duration { | ||
| seconds := dur.(time.Duration).Seconds() | ||
| return time.Duration(seconds/float64(count.(int))) * time.Second | ||
| } | ||
| e, _ := billings.Load("e") | ||
| o, _ := billings.Load("o") | ||
| ac, _ := billings.Load("ac") | ||
| a, _ := billings.Load("a") | ||
| ecost := costC(e, cfg.Extensions.Billing.Costs.Entities.Cost) | ||
| ocost := costC(o, cfg.Extensions.Billing.Costs.Orchestrations.Cost) | ||
| acost := costA(a, cfg.Extensions.Billing.Costs.Activities.Cost) | ||
| logger.Warn("Billing estimate", | ||
| zap.Any("launched entities", e), | ||
| zap.String("entity cost", fmt.Sprintf("$%.2f", ecost)), | ||
| zap.Any("launched orchestrations", o), | ||
| zap.String("orchestration cost", fmt.Sprintf("$%.2f", ocost)), | ||
| zap.Any("activity time", a), | ||
| zap.Any("activities launced", ac), | ||
| zap.Any("average activity time", avg(a, ac)), | ||
| zap.String("activity cost", fmt.Sprintf("$%.2f", acost)), | ||
| zap.String("total estimate", fmt.Sprintf("$%.2f", ecost+ocost+acost)), | ||
| ) | ||
| } | ||
| go func() { | ||
| ticker := time.NewTicker(3 * time.Second) | ||
| for range ticker.C { | ||
| outputBillingStatus() | ||
| } | ||
| }() | ||
| */ | ||
|
|
||
| billingStream, err := helpers.Js.CreateOrUpdateStream(ctx, jetstream.StreamConfig{ | ||
| Name: "billing", | ||
| Subjects: []string{ | ||
| "billing." + cfg.Stream + ".>", | ||
| }, | ||
| Storage: jetstream.FileStorage, | ||
| Retention: jetstream.LimitsPolicy, | ||
| MaxAge: 7 * 24 * time.Hour, | ||
| }) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| entityConsumer, err := billingStream.CreateOrUpdateConsumer(ctx, jetstream.ConsumerConfig{ | ||
| Durable: "entityAggregator", | ||
| FilterSubjects: []string{ | ||
| "billing." + cfg.Stream + ".entities.>", | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| _, err = entityConsumer.Consume(func(msg jetstream.Msg) { | ||
| incrementInt("e", 1) | ||
| msg.Ack() | ||
| }) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| //defer consume.Drain() | ||
|
|
||
| orchestrationConsumer, err := billingStream.CreateOrUpdateConsumer(ctx, jetstream.ConsumerConfig{ | ||
| Durable: "orchestrationAggregator", | ||
| FilterSubject: "billing." + cfg.Stream + ".orchestrations.>", | ||
| }) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| _, err = orchestrationConsumer.Consume(func(msg jetstream.Msg) { | ||
| incrementInt("o", 1) | ||
| msg.Ack() | ||
| }) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| //defer consume.Drain() | ||
|
|
||
| activityConsumer, err := billingStream.CreateOrUpdateConsumer(ctx, jetstream.ConsumerConfig{ | ||
| Durable: "activityAggregator", | ||
| FilterSubject: "billing." + cfg.Stream + ".activities.>", | ||
| }) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| _, err = activityConsumer.Consume(func(msg jetstream.Msg) { | ||
| incrementInt("ac", 1) | ||
| var ev lib.BillingEvent | ||
| err := json.Unmarshal(msg.Data(), &ev) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| incrementDur("a", ev.Duration) | ||
| msg.Ack() | ||
| }) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| //defer consume.Drain() | ||
| } | ||
|
|
||
| err := lib.StartBillingProcessor(ctx, cfg, helpers.Js, logger) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extremely large initialization function violates Single Responsibility Principle.
The go_init_module function is 270+ lines long and handles multiple distinct responsibilities:
- Configuration loading
- Logging setup
- NATS server bootstrapping
- Stream and consumer creation
- Extension initialization (search, billing)
This makes the code difficult to maintain, test, and debug.
Break down into smaller, focused functions:
+func initializeConfig() (*config.Config, error) {
+ // Lines 47-51
+}
+
+func initializeNATS(cfg *config.Config) error {
+ // Lines 60-107
+}
+
+func setupStreamsAndConsumers(cfg *config.Config) error {
+ // Lines 108-145
+}
+
+func initializeExtensions(cfg *config.Config) error {
+ // Lines 147-317
+}
func go_init_module() {
+ cfg, err := initializeConfig()
+ if err != nil {
+ panic(err)
+ }
+
+ if err := initializeNATS(cfg); err != nil {
+ panic(err)
+ }
+ // ... continue with other initialization steps
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cli/ext/ext.go from lines 46 to 318, the go_init_module function is
excessively large and handles multiple responsibilities such as configuration
loading, logging setup, NATS server bootstrapping, stream and consumer creation,
and extension initialization. To fix this, refactor the code by extracting each
major responsibility into its own smaller, focused function. For example, create
separate functions for loading configuration, setting up logging, initializing
the NATS server, creating streams and consumers, and initializing extensions
like search and billing. Then call these smaller functions sequentially from
go_init_module to improve readability, maintainability, and testability.
| type Array struct { | ||
| *frankenphp.Array | ||
| } | ||
|
|
||
| func (a *Array) GetStringKey(key string) interface{} { | ||
| for i := uint32(0); i < a.Len(); i++ { | ||
| k, v := a.At(i) | ||
| if k.Type == frankenphp.PHPStringKey && k.Str == key { | ||
| return v | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (a *Array) Unmarshall(j []byte) error { | ||
| result := Array{} | ||
| err := json.Unmarshal(j, &result.Array) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| a.Array = result.Array | ||
| return nil | ||
| } | ||
|
|
||
| func (a *Array) Marshal() ([]byte, error) { | ||
| data, err := json.Marshal(a.Array) | ||
| return data, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix method naming inconsistency.
The method name Unmarshall is inconsistent with Go conventions and the corresponding Marshal method. It should be Unmarshal (single 'l').
-func (a *Array) Unmarshall(j []byte) error {
+func (a *Array) Unmarshal(j []byte) error {Also update the usage in line 310 and 336:
- err = result.Unmarshall(get.Value())
+ err = result.Unmarshal(get.Value())- err = result.Data.Unmarshall(res)
+ err = result.Data.Unmarshal(res)🤖 Prompt for AI Agents
In cli/glue/glue.go from lines 257 to 285, rename the method Unmarshall to
Unmarshal to follow Go naming conventions and match the Marshal method. Also
update all calls to this method accordingly, including those at lines 310 and
336, to use Unmarshal with a single 'l'.
| // ProcessMessage takes a message and some references and returns: | ||
| // 1. an auth context | ||
| // 2. the destination id | ||
| // 3. the state | ||
| // 4. or an error | ||
| func ProcessMessage( | ||
| ctx context.Context, | ||
| logger *zap.Logger, | ||
| msg jetstream.Msg, | ||
| rm *auth.ResourceManager, | ||
| config *config.Config, | ||
| js jetstream.JetStream, | ||
| ) ([]byte, *ids.StateId, *glue.StateArray, error) { | ||
| logger.Debug("Processing message", zap.Any("msg", msg)) | ||
|
|
||
| id := getStateId(msg) | ||
| unlocker, err := lockStateId(ctx, id, nil, logger) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| defer unlocker() | ||
|
|
||
| ctx, cancelCtx := context.WithCancel(ctx) | ||
| defer cancelCtx() | ||
|
|
||
| currentUser, err := getUserFromHeader(msg) | ||
| if err != nil { | ||
| logger.Warn("Failed to unmarshal event provenance", | ||
| zap.Any("Provenance", msg.Headers().Get(string(glue.HeaderProvenance))), | ||
| zap.Error(err), | ||
| ) | ||
| currentUser = nil | ||
| } else { | ||
| ctx = auth.DecorateContextWithUser(ctx, currentUser) | ||
| } | ||
|
|
||
| // retrieve the source | ||
| sourceId := ids.ParseStateId(msg.Headers().Get(string(glue.HeaderEmittedBy))) | ||
| var authContext []byte | ||
|
|
||
| if config.Extensions.Authz.Enabled { | ||
| // extract the source operations | ||
| sourceOps := strings.Split(msg.Headers().Get(string(glue.HeaderSourceOps)), ",") | ||
|
|
||
| // extract the target operations | ||
| targetOps := strings.Split(msg.Headers().Get(string(glue.HeaderTargetOps)), ",") | ||
| preventCreation := true | ||
| for _, op := range targetOps { | ||
| switch auth.Operation(op) { | ||
| case auth.Signal: | ||
| fallthrough | ||
| case auth.Call: | ||
| fallthrough | ||
| case auth.Lock: | ||
| fallthrough | ||
| case auth.Output: | ||
| preventCreation = false | ||
| } | ||
| } | ||
|
|
||
| resource, err := rm.DiscoverResource(ctx, id, sourceId, logger, preventCreation) | ||
| if err != nil { | ||
| logger.Warn("User attempted to perform an unauthorized operation", zap.String("operation", "create"), zap.String("From", sourceId.Id), zap.String("To", id.Id), zap.String("User", string(currentUser.UserId))) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| if resource == nil { | ||
| logger.Warn("User accessed missing object", zap.Any("operation", sourceOps), zap.String("from", sourceId.Id), zap.String("to", id.Id), zap.String("user", string(currentUser.UserId))) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| } | ||
|
|
||
| authContext, err = rm.ToAuthContext(ctx, resource) | ||
| if err != nil { | ||
| logger.Warn("Failed to retrieve auth context", zap.Error(err)) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, err | ||
| } | ||
|
|
||
| m := msg.Headers().Get(string(glue.HeaderMeta)) | ||
| var meta map[string]interface{} | ||
| if m != "[]" { | ||
| err = json.Unmarshal([]byte(m), &meta) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
|
|
||
| switch msg.Headers().Get(string(glue.HeaderEventType)) { | ||
| case "RevokeRole": | ||
| if !resource.WantTo(auth.ShareMinus, ctx) { | ||
| logger.Warn("User attempted to perform an unauthorized operation", zap.String("operation", "revokeRole"), zap.String("From", sourceId.Id), zap.String("To", id.Id), zap.String("User", string(currentUser.UserId))) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| } | ||
| role := meta["role"].(string) | ||
|
|
||
| err := resource.RevokeRole(auth.Role(role), ctx) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| err = resource.Update(ctx, logger) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| case "RevokeUser": | ||
| if !resource.WantTo(auth.ShareMinus, ctx) { | ||
| logger.Warn("User attempted to perform an unauthorized operation", zap.String("operation", "revokeUser"), zap.String("From", sourceId.Id), zap.String("To", id.Id), zap.String("User", string(currentUser.UserId))) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| } | ||
| user := meta["userId"].(string) | ||
| err := resource.RevokeUser(auth.UserId(user), ctx) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| err = resource.Update(ctx, logger) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| case "ShareWithRole": | ||
| if !resource.WantTo(auth.SharePlus, ctx) { | ||
| logger.Warn("User attempted to perform an unauthorized operation", zap.String("operation", "shareWithRole"), zap.String("From", sourceId.Id), zap.String("To", id.Id), zap.String("User", string(currentUser.UserId))) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| } | ||
| role := meta["role"].(auth.Role) | ||
| operations := meta["allowedOperations"].([]auth.Operation) | ||
|
|
||
| for _, op := range operations { | ||
| err := resource.GrantRole(role, op, ctx) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| } | ||
| err = resource.Update(ctx, logger) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| case "ShareWithUser": | ||
| if !resource.WantTo(auth.SharePlus, ctx) { | ||
| logger.Warn("User attempted to perform an unauthorized operation", zap.String("operation", "shareWithUser"), zap.String("From", sourceId.Id), zap.String("To", id.Id), zap.String("User", string(currentUser.UserId))) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| } | ||
| role := meta["userId"].(auth.UserId) | ||
| operations := meta["allowedOperations"].([]auth.Operation) | ||
|
|
||
| for _, op := range operations { | ||
| err := resource.GrantUser(role, op, ctx) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| } | ||
| err = resource.Update(ctx, logger) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| case "ShareOwnership": | ||
| if !resource.WantTo(auth.Owner, ctx) { | ||
| logger.Warn("User attempted to perform an unauthorized operation", zap.String("operation", "shareOwnership"), zap.String("From", sourceId.Id), zap.String("To", id.Id), zap.String("User", string(currentUser.UserId))) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| } | ||
| userId := meta["userId"].(auth.UserId) | ||
| user := ctx.Value(appcontext.CurrentUserKey).(*auth.User) | ||
| err := resource.ShareOwnership(userId, user, true) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| err = resource.Update(ctx, logger) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| case "GiveOwnership": | ||
| if !resource.WantTo(auth.Owner, ctx) { | ||
| logger.Warn("User attempted to perform an unauthorized operation", zap.String("operation", "giveOwnership"), zap.String("From", sourceId.Id), zap.String("To", id.Id), zap.String("User", string(currentUser.UserId))) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| } | ||
| userId := meta["userId"].(auth.UserId) | ||
| user := ctx.Value(appcontext.CurrentUserKey).(*auth.User) | ||
| err := resource.ShareOwnership(userId, user, true) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| err = resource.Update(ctx, logger) | ||
| if err != nil { | ||
| return []byte{}, nil, nil, err | ||
| } | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| } | ||
| } | ||
|
|
||
| for _, op := range targetOps { | ||
| if !resource.WantTo(auth.Operation(op), ctx) { | ||
| logger.Warn("User attempted to perform an unauthorized operation", zap.String("operation", op), zap.String("From", sourceId.Id), zap.String("To", id.Id), zap.String("User", string(currentUser.UserId))) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| state, err := glue.GetStateArray(id, js, ctx, logger) | ||
| if err != nil { | ||
| logger.Warn("Failed to retrieve state", zap.Error(err)) | ||
| msg.Ack() | ||
| return []byte{}, nil, nil, err | ||
| } | ||
|
|
||
| return authContext, id, state, nil | ||
| } |
There was a problem hiding this comment.
Significant code duplication and complexity issues.
This function has several serious maintainability concerns:
-
Massive code duplication: Lines 175-350 are nearly identical to the authorization logic in
processMsg(lines 395-578). This violates the DRY principle and creates maintenance overhead. -
Single Responsibility Principle violation: The function handles user extraction, authorization, resource discovery, and multiple event types in a single large function (220+ lines).
-
Inconsistent error handling: Some paths call
msg.Ack()before returning errors, others don't.
Immediate action needed: Refactor to eliminate code duplication by extracting shared authorization logic into separate functions:
+func handleAuthorizationEvents(ctx context.Context, msg jetstream.Msg, resource *auth.Resource, meta map[string]interface{}, logger *zap.Logger, currentUser *auth.User, sourceId, id *ids.StateId) error {
+ // Move lines 226-340 here
+ // Return error instead of calling msg.Ack() and returning nil
+}
+func processAuthorization(ctx context.Context, msg jetstream.Msg, rm *auth.ResourceManager, config *config.Config, logger *zap.Logger, currentUser *auth.User, sourceId, id *ids.StateId) (*auth.Resource, error) {
+ // Move lines 179-225 here
+}Then both ProcessMessage and processMsg can use these shared functions instead of duplicating the logic.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cli/lib/consumer.go from lines 139 to 360, the ProcessMessage function
contains duplicated authorization logic also present in processMsg, violating
DRY and single responsibility principles, and has inconsistent error handling
with msg.Ack calls. Refactor by extracting the shared authorization and resource
handling logic into separate helper functions that handle user extraction,
authorization checks, resource discovery, and event-specific operations. Then
update ProcessMessage and processMsg to call these shared functions, ensuring
consistent error handling and msg.Ack usage across all code paths.
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
This reverts commit 3a0475c.
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
src/DurableClient.php (1)
84-92: Missing user context handling in remote methodThe
remote()method doesn't accept or handle user context like the other factory methods, which could lead to inconsistent behavior.Apply this diff to add user context support:
- public static function remote(string $apiHost = 'http://localhost:8080'): self + public static function remote(string $apiHost = 'http://localhost:8080', ?Provenance $userContext = null): self { $builder = new HttpClientBuilder(); $builder->retry(3); $httpClient = $builder->build(); + $entityClient = new RemoteEntityClient($apiHost, $httpClient); + $orchestrationClient = new RemoteOrchestrationClient($apiHost, $httpClient); + + if ($userContext !== null) { + // Note: Remote clients may need auth token instead of direct user context + // This might require a different approach for remote authentication + } - return new self(new RemoteEntityClient($apiHost, $httpClient), new RemoteOrchestrationClient($apiHost, $httpClient)); + return new self($entityClient, $orchestrationClient); }
♻️ Duplicate comments (5)
cli/Makefile (1)
13-13: Remove hardcoded absolute paths for portabilityThe hardcoded paths will break when run on different machines or environments.
Use relative paths or environment variables instead:
-LOCAL_MODULE := /home/withinboredom/code/durable-php/cli +LOCAL_MODULE := $(shell pwd)Also applies to lines 14-16.
cli/lib/consumer.go (1)
139-360: Significant code duplication between ProcessMessage and processMsg.The ProcessMessage function (lines 175-350) duplicates almost all of the authorization logic from processMsg (lines 395-578). This creates a maintenance burden and increases the risk of bugs when one is updated but not the other.
Extract the shared authorization logic into reusable functions:
+func handleAuthorizationEvent(ctx context.Context, msg jetstream.Msg, resource *auth.Resource, meta map[string]interface{}, logger *zap.Logger, currentUser *auth.User, sourceId, id *ids.StateId) error { + eventType := msg.Headers().Get(string(glue.HeaderEventType)) + + switch eventType { + case "RevokeRole": + if !resource.WantTo(auth.ShareMinus, ctx) { + logger.Warn("User attempted to perform an unauthorized operation", + zap.String("operation", "revokeRole"), + zap.String("From", sourceId.Id), + zap.String("To", id.Id), + zap.String("User", string(currentUser.UserId))) + msg.Ack() + return nil + } + // ... rest of the logic + // ... other cases + } + return nil +} func ProcessMessage(...) (...) { // ... existing setup code ... if config.Extensions.Authz.Enabled { // ... existing validation ... if meta != nil { - switch msg.Headers().Get(string(glue.HeaderEventType)) { - // Remove all the duplicated cases - } + if err := handleAuthorizationEvent(ctx, msg, resource, meta, logger, currentUser, sourceId, id); err != nil { + return []byte{}, nil, nil, err + } } } // ... rest of the function }cli/ext/helpers/helpers.go (3)
54-59: Global variables need synchronization for concurrent access.These global variables could be accessed concurrently from multiple PHP requests, potentially causing race conditions.
+import "sync" + +var ( + globalMu sync.RWMutex +) + var Logger *zap.Logger var NatsState string var Js jetstream.JetStream var Config *config.Config var NatServer *server.Server var Ctx context.Context + +// SetLogger safely sets the global logger +func SetLogger(l *zap.Logger) { + globalMu.Lock() + defer globalMu.Unlock() + Logger = l +} + +// GetLoggerSafe safely retrieves the global logger +func GetLoggerSafe() *zap.Logger { + globalMu.RLock() + defer globalMu.RUnlock() + return Logger +}
72-109: Add safe type assertions to prevent panics.Direct type assertions without checking can cause panics if the value is not the expected type.
switch k.Str { case "eventId": - ev.EventId = v.(string) + if str, ok := v.(string); ok { + ev.EventId = str + } else { + return nil, fmt.Errorf("eventId must be a string") + } case "event": - ev.Event = v.(string) + if str, ok := v.(string); ok { + ev.Event = str + } else { + return nil, fmt.Errorf("event must be a string") + }
111-130: Add nil check for input array.The function should validate that the input array is non-nil to prevent panics.
func GetUserContext(arr *frankenphp.Array) *auth.User { + if arr == nil { + return &auth.User{} + } user := &auth.User{}
🧹 Nitpick comments (8)
src/Glue/worker.php (1)
32-32: Global variable usage creates tight couplingUsing
global $loggermakes the code tightly coupled and harder to test. The logger should be passed as a parameter or resolved from a container.Consider refactoring to pass the logger through a more structured approach:
-frankenphp_handle_request(static function (): void { - global $logger; +frankenphp_handle_request(static function () use ($logger): void {cli/ext/build/ext.h (1)
10-11: Consider adding documentation for the worker handle functionsWhile the function names are self-explanatory, adding brief comments about the purpose and thread-safety guarantees would improve maintainability.
Apply this diff to add documentation:
/* Functions for managing current worker */ +/* Set the current worker handle for this thread - not thread-safe */ void set_current_worker_handle(uintptr_t handle); +/* Clear the current worker handle for this thread - not thread-safe */ void clear_current_worker(void);cli/auth/resource.go (1)
149-184: Improve error handling consistency in the local message pathThe local message path has good error handling but could be more consistent with the fallback path's error handling patterns.
Apply this diff to improve error handling consistency:
if SendLocalMessage != nil { // Use the new local message system contextData := map[string]interface{}{ "bootstrap": ctx.Value("bootstrap"), } response, err := SendLocalMessage("getPermissions", id.String(), contextData) if err != nil { - return perms, err + return perms, fmt.Errorf("local message failed: %w", err) } // The response should be JSON string if responseStr, ok := response.(string); ok { if err = json.Unmarshal([]byte(responseStr), &perms); err != nil { - return perms, err + return perms, fmt.Errorf("failed to unmarshal local response: %w", err) } } else { - return perms, errors.New("invalid response format from local message") + return perms, fmt.Errorf("invalid response format from local message: expected string, got %T", response) }cli/lib/consumer.go (1)
117-127: Consider adding error context to lockStateId.The function correctly handles non-entity types by returning a no-op unlocker, but when lockSubject fails, it returns a no-op unlocker instead of propagating the error context.
func lockStateId(ctx context.Context, id *ids.StateId, js jetstream.JetStream, logger *zap.Logger) (func() error, error) { if id.Kind != ids.Entity { return func() error { return nil }, nil } unlocker, err := lockSubject(ctx, id.ToSubject(), js, logger) if err != nil { - return func() error { return nil }, err + return func() error { return nil }, fmt.Errorf("failed to lock state %s: %w", id.Id, err) } return unlocker, nil }src/LocalOrchestrationClient.php (2)
49-51: Remove unused constructor parameterThe
$spyProxyparameter is never used in this class and can be removed.- public function __construct( - private SpyProxy $spyProxy = new SpyProxy(), - ) {} + public function __construct() {}
145-168: Consider implementing share/grant/revoke operations locallyThese authorization operations could potentially be implemented locally by emitting appropriate events to the authorization system, similar to how
raiseEventandstartNewwork. This would provide a more complete local client implementation.Would you like me to help implement these authorization operations using the local event emission pattern?
cli/ext/build/ext.go (2)
165-170: Remove debug logging from production codeDebug logging statements should be removed or controlled by a debug flag to avoid cluttering production logs.
- // Set the current worker for PHP access BEFORE processing/authorization - helpers.Logger.Info("About to call SetCurrentWorker") SetCurrentWorker(worker) - helpers.Logger.Info("SetCurrentWorker completed")
685-694: Improve user validation error handlingThe validation check for userId and roles should provide more specific error messages and might want to allow empty roles in some cases.
if userVal != nil { user = helpers.GetUserContext(userVal) - if user.UserId == "" || len(user.Roles) == 0 { - helpers.LogError("User context is missing userId or roles") + if user.UserId == "" { + helpers.LogError("User context is missing userId") + return 0 + } + if len(user.Roles) == 0 { + helpers.Logger.Warn("User context has no roles assigned") return 0 } ctx = auth.DecorateContextWithUser(ctx, user) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.idea/codeStyles/Project.xml(0 hunks)cli/Makefile(1 hunks)cli/auth/resource.go(2 hunks)cli/ext/build/README.md(1 hunks)cli/ext/build/ext.c(1 hunks)cli/ext/build/ext.go(1 hunks)cli/ext/build/ext.h(1 hunks)cli/ext/build/ext.stub.php(1 hunks)cli/ext/build/ext_arginfo.h(1 hunks)cli/ext/helpers/helpers.go(1 hunks)cli/go.mod(3 hunks)cli/lib/consumer.go(3 hunks)composer.json(1 hunks)src/DurableClient.php(2 hunks)src/Glue/Glue.php(6 hunks)src/Glue/autoload.php(1 hunks)src/Glue/worker.php(1 hunks)src/LocalEntityClient.php(1 hunks)src/LocalOrchestrationClient.php(1 hunks)
💤 Files with no reviewable changes (1)
- .idea/codeStyles/Project.xml
🧰 Additional context used
🧬 Code Graph Analysis (13)
src/Glue/worker.php (2)
src/Glue/Glue.php (2)
Glue(65-315)process(121-124)src/OrchestrationContext.php (1)
info(537-544)
cli/ext/build/ext.h (1)
cli/ext/build/ext.c (2)
set_current_worker_handle(304-308)clear_current_worker(316-320)
src/DurableClient.php (3)
src/Glue/Provenance.php (1)
Provenance(30-33)src/LocalEntityClient.php (1)
LocalEntityClient(48-158)src/LocalOrchestrationClient.php (1)
LocalOrchestrationClient(45-169)
cli/ext/build/ext.stub.php (1)
cli/ext/build/ext.go (1)
Worker(759-769)
cli/ext/helpers/helpers.go (2)
cli/glue/glue.go (1)
Array(257-259)cli/auth/user.go (3)
User(51-54)UserId(13-13)Role(14-14)
src/LocalOrchestrationClient.php (12)
src/Events/EventDescription.php (1)
EventDescription(37-223)cli/auth/user.go (1)
Operation(11-11)cli/glue/glue.go (1)
StartOrchestration(29-29)src/Events/WithOrchestration.php (1)
WithOrchestration(30-59)src/Glue/Provenance.php (1)
Provenance(30-33)cli/ids/id.go (2)
StateId(188-191)OrchestrationId(175-178)src/State/Serializer.php (2)
Serializer(31-82)serialize(35-48)src/LocalEntityClient.php (2)
__construct(52-54)setUserContext(123-126)cli/ext/build/ext.stub.php (1)
emit_event(7-7)src/SerializedArray.php (1)
SerializedArray(30-85)src/Events/With.php (1)
id(32-39)src/State/Ids/StateId.php (1)
toOrchestrationInstance(90-101)
cli/ext/build/ext_arginfo.h (3)
cli/ext/build/ext.stub.php (15)
Worker(9-40)__construct(11-11)GetCurrent(13-13)startEventLoop(15-15)drainEventLoop(17-17)__destruct(19-19)queryState(22-22)getUser(24-24)getSource(26-26)getCurrentId(28-28)getCorrelationId(30-30)getState(32-32)updateState(34-34)emitEvent(36-36)delete(38-38)cli/ext/build/ext.go (1)
Worker(759-769)cli/ext/build/ext.c (1)
Bottledcode_DurablePhp_Ext_emit_event(357-369)
src/LocalEntityClient.php (10)
src/Events/EventDescription.php (1)
EventDescription(37-223)src/Events/WithEntity.php (1)
WithEntity(29-59)src/Glue/Provenance.php (1)
Provenance(30-33)src/Proxy/SpyException.php (1)
SpyException(5-5)src/Proxy/SpyProxy.php (1)
SpyProxy(31-126)src/Search/EntityFilter.php (1)
EntityFilter(27-30)src/State/Serializer.php (2)
Serializer(31-82)serialize(35-48)cli/ext/build/ext.stub.php (1)
emit_event(7-7)src/State/Ids/StateId.php (1)
fromEntityId(69-72)src/SerializedArray.php (1)
SerializedArray(30-85)
cli/ext/build/ext.c (4)
cli/ext/build/ext.stub.php (14)
__construct(11-11)startEventLoop(15-15)drainEventLoop(17-17)__destruct(19-19)queryState(22-22)getUser(24-24)getSource(26-26)getCurrentId(28-28)getCorrelationId(30-30)getState(32-32)updateState(34-34)emitEvent(36-36)delete(38-38)GetCurrent(13-13)src/Glue/Glue.php (2)
__construct(81-119)queryState(126-134)src/DurableClient.php (1)
__construct(42-45)cli/ext/build/ext_arginfo.h (1)
register_class_Bottledcode_DurablePhp_Ext_Worker(90-98)
src/Glue/Glue.php (4)
cli/ext/build/ext.stub.php (4)
GetCurrent(13-13)getSource(26-26)getUser(24-24)emitEvent(36-36)src/State/Ids/StateId.php (1)
fromString(85-88)src/Glue/Provenance.php (1)
Provenance(30-33)src/State/Serializer.php (2)
Serializer(31-82)deserialize(61-76)
cli/ext/build/ext.go (10)
cli/glue/glue.go (4)
DeleteState(225-255)StateArray(287-291)Array(257-259)GetStateArray(293-342)cli/ids/id.go (3)
StateId(188-191)IdKind(9-9)Activity(12-12)cli/ext/helpers/helpers.go (7)
Logger(54-54)LogError(35-37)ParseStateId(61-70)Config(57-57)Consumer(48-52)GetUserContext(111-130)ParseEvent(72-109)cli/glue/response_writer.go (10)
HeaderProvenance(26-26)HeaderDelay(21-21)HeaderStateId(20-20)HeaderEventType(22-22)HeaderEmittedBy(24-24)HeaderTargetType(23-23)HeaderEmittedAt(25-25)HeaderTargetOps(27-27)HeaderSourceOps(28-28)HeaderMeta(29-29)cli/auth/keys.go (1)
DecorateContextWithUser(30-36)cli/lib/api.go (1)
GetCorrelationId(39-53)cli/auth/resourceManager.go (1)
GetResourceManager(29-46)cli/lib/consumer.go (1)
StartConsumer(21-35)cli/auth/user.go (2)
Operation(11-11)UserId(13-13)cli/config/config.go (1)
GetProjectConfig(74-92)
cli/auth/resource.go (3)
cli/ext/build/ext.go (1)
SendLocalMessage(290-309)cli/glue/glue.go (2)
NewGlue(68-75)GetPermissions(32-32)cli/ids/id.go (1)
SystemSource(24-27)
cli/lib/consumer.go (9)
cli/ext/helpers/helpers.go (3)
Config(57-57)Logger(54-54)ParseStateId(61-70)cli/config/config.go (1)
Config(66-72)cli/ids/id.go (4)
IdKind(9-9)StateId(188-191)ParseStateId(130-142)Entity(13-13)cli/lib/api.go (1)
GetCorrelationId(39-53)cli/glue/response_writer.go (7)
HeaderStateId(20-20)HeaderProvenance(26-26)HeaderEmittedBy(24-24)HeaderSourceOps(28-28)HeaderTargetOps(27-27)HeaderMeta(29-29)HeaderEventType(22-22)cli/auth/user.go (7)
User(51-54)Operation(11-11)Signal(27-27)UserId(13-13)Role(14-14)SharePlus(42-42)Owner(48-48)cli/glue/glue.go (1)
ProcessMessage(27-27)cli/auth/resourceManager.go (1)
ResourceManager(19-22)cli/auth/keys.go (1)
DecorateContextWithUser(30-36)
🪛 PHPMD (2.15.0)
cli/ext/build/ext.stub.php
7-7: Avoid unused parameters such as '$userContext'. (Unused Code Rules)
(UnusedFormalParameter)
7-7: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
7-7: Avoid unused parameters such as '$from'. (Unused Code Rules)
(UnusedFormalParameter)
15-15: Avoid unused parameters such as '$kind'. (Unused Code Rules)
(UnusedFormalParameter)
22-22: Avoid unused parameters such as '$stateId'. (Unused Code Rules)
(UnusedFormalParameter)
34-34: Avoid unused parameters such as '$state'. (Unused Code Rules)
(UnusedFormalParameter)
36-36: Avoid unused parameters such as '$eventDescription'. (Unused Code Rules)
(UnusedFormalParameter)
src/LocalOrchestrationClient.php
60-60: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
66-66: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
86-86: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
92-92: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
92-92: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
111-111: Avoid unused local variables such as '$sequence'. (Unused Code Rules)
(UnusedLocalVariable)
117-117: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
117-117: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
123-123: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
123-123: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
129-129: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
135-135: Avoid unused parameters such as '$token'. (Unused Code Rules)
(UnusedFormalParameter)
145-145: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
145-145: Avoid unused parameters such as '$with'. (Unused Code Rules)
(UnusedFormalParameter)
150-150: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
150-150: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
150-150: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
155-155: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
155-155: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
155-155: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
165-165: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
165-165: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
src/LocalEntityClient.php
60-60: Avoid unused parameters such as '$filter'. (Unused Code Rules)
(UnusedFormalParameter)
60-60: Avoid unused parameters such as '$page'. (Unused Code Rules)
(UnusedFormalParameter)
112-112: Avoid unused parameters such as '$entityId'. (Unused Code Rules)
(UnusedFormalParameter)
118-118: Avoid unused parameters such as '$token'. (Unused Code Rules)
(UnusedFormalParameter)
129-129: Avoid unused parameters such as '$entityId'. (Unused Code Rules)
(UnusedFormalParameter)
134-134: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
134-134: Avoid unused parameters such as '$with'. (Unused Code Rules)
(UnusedFormalParameter)
139-139: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
139-139: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
139-139: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
149-149: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
149-149: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
154-154: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
154-154: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 checkmake (0.2.2)
cli/Makefile
[warning] 19-19: Target body for "frankenphp" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 LanguageTool
cli/ext/build/README.md
[grammar] ~27-~27: There might be a mistake here.
Context: ...Worker Properties: - kind: mixed - started: bool - consumer: mixed (nullable) - ...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...es:** - kind: mixed - started: bool - consumer: mixed (nullable) - activeId: mixed (...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...ed: bool - consumer: mixed (nullable) - activeId: mixed (nullable) - state`: mixed (nul...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...nullable) - activeId: mixed (nullable) - state: mixed (nullable) - pendingEvents: ar...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...d (nullable) - state: mixed (nullable) - pendingEvents: array - authContext: array - `curren...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...ixed (nullable) - pendingEvents: array - authContext: array - currentCtx: mixed - `current...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ingEvents: array - authContext: array - currentCtx: mixed - currentMsg`: mixed
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...uthContext: array - currentCtx: mixed - currentMsg`: mixed
(QB_NEW_EN)
🔇 Additional comments (43)
composer.json (1)
43-44: LGTM: FrankenPHP extension dependency added correctly.The addition of the
ext-frankenphpruntime dependency aligns with the PR's objective to integrate FrankenPHP as a PHP extension. The dependency specification using"*"is appropriate for extensions where version constraints are typically managed by the PHP installation.src/Glue/autoload.php (1)
41-41: Minor formatting improvement.The additional blank line improves readability after the error message.
cli/go.mod (2)
1-1: Module path updated to align with project structure.The module path change from
durable_phptogithub.com/bottledcode/durable-php/clifollows Go module naming conventions and aligns with the broader import path updates across the CLI codebase.
11-11: Dependency updates look reasonable.The version bumps for nats-server, oapi-codegen runtime, prometheus client, and protobuf are minor updates that should be compatible with existing functionality.
Also applies to: 44-44, 46-46, 55-55
cli/ext/build/README.md (1)
1-38: Documentation accurately reflects the extension API.The README provides clear documentation for the auto-generated PHP extension, correctly documenting:
- The
emit_eventfunction signature matching the stub implementation- The
Workerclass properties that align with the Go bridge implementationThe documentation structure and formatting are appropriate for developer reference.
src/LocalEntityClient.php (7)
52-54: Constructor design is clean and follows dependency injection principles.The constructor properly initializes the SpyProxy with a sensible default while allowing for dependency injection in tests.
66-91: Signal method implementation is well-designed with proper error handling.The method correctly:
- Uses reflection to extract the interface name from the closure parameter
- Validates interface existence
- Uses SpyProxy to capture operation details
- Handles SpyException gracefully
- Guards against incomplete operation capture
The logic flow is sound and handles edge cases appropriately.
100-109: Event emission implementation follows the established patterns.The method correctly:
- Creates a WithEntity wrapper around the RaiseEvent
- Serializes input parameters appropriately
- Handles user context serialization
- Uses the proper StateId for targeting
The integration with the extension's
emit_eventfunction is properly implemented.
60-63: Clear separation of local vs remote capabilities with helpful error messages.The implementation clearly delineates which operations are supported locally versus requiring the RemoteEntityClient. The error messages provide clear guidance to developers on which client to use for unsupported operations.
Also applies to: 112-115, 118-121, 129-132, 134-157
123-126: User context management is appropriately designed.The
setUserContextmethod provides a clean way to manage authentication context for local operations, which is more appropriate than the token-basedwithAuthfor in-process scenarios.
46-46: emit_event stub and documentation verified
- Stub definition found in
cli/ext/build/ext.stub.phpwith signature:namespace Bottledcode\DurablePhp\Ext; function emit_event(?array $userContext, array $event, string $from): int {}- Documentation present in
cli/ext/build/README.mdunder the emit_event section, matching the stub signature and parameter list.No further action required.
87-87: No issue with EntityId construction
The expressionEntityId($interfaceName, $entityId)invokes the global helper insrc/functions.php(line 13), which is declared as:function EntityId(string $name, string $id): EntityIdThat helper delegates to the class’s
from(string $name, string $id)factory. There is no public constructor to call directly, so this usage is correct and requires no change.Likely an incorrect or invalid review comment.
src/Glue/worker.php (1)
34-35: Glue constructor correctly accepts a DurableLogger instanceVerified that in src/Glue/Glue.php (line 81) the constructor is declared as:
public function __construct(private DurableLogger $logger)which matches the
$glue = new Glue($logger);usage in worker.php. No changes are needed.cli/ext/build/ext.h (1)
1-13: LGTM! Clean and minimal header designThe header follows good C practices with proper header guards, minimal includes, and clear function declarations. The separation of concerns between worker handle management functions is well-structured.
cli/auth/resource.go (3)
14-16: LGTM! Import path migration completed correctlyThe module path updates from
durable_php/...togithub.com/bottledcode/durable-php/cli/...are consistent and align with the broader codebase migration.
21-25: LGTM! Well-designed local message sender interfaceThe
LocalMessageSendertype provides a clean abstraction for local messaging with appropriate error handling. The global variable approach is reasonable for extension integration.
169-184: Verify fallback glue path functionalityAlthough the fallback code in
cli/auth/resource.go(lines 169–184) is unchanged, please:
- Confirm that
cli/glue/glue.godeclares aGetPermissionsconstant of typeMethodand that its value matches the intended glue command.- Ensure the signatures of
NewGlue(bootstrap, GetPermissions, …)(around line 68 incli/glue/glue.go) and(*Glue).Execute(...)(around line 116) still align with the call site inresource.go.- Exercise the fallback path in an environment without the local message sender to verify it correctly returns the
Permissionsheader.cli/Makefile (2)
7-11: LGTM! Proper debug flag handlingThe conditional logic for debug vs release builds is well-structured, with appropriate compiler flags for each scenario.
30-31: Confirm stub generation paths in cli/MakefilePlease verify that the stub generator script and target stub file are correctly wired up:
- Ensure
build/gen_stub.phpexists in your PHP source root and is executable (this is whatGEN_STUBpoints to on line 14 ofcli/Makefile).- From the
cli/directory, run:and confirm it completes without errors, producingmake ext/build/ext_arginfo.h ext/build/ext.stub.phpcli/ext/build/ext.stub.phpas expected.Key locations to check:
- cli/Makefile, line 14:
GEN_STUB := …/build/gen_stub.php- cli/Makefile, line 31: the stub target invoking
$(GEN_STUB)src/DurableClient.php (4)
29-29: LGTM! Clean import additionThe addition of the Provenance import is necessary for the new user context functionality.
47-69: Excellent architectural improvement with local/remote abstractionThe enhanced
get()method provides intelligent runtime detection between local and remote execution modes, with proper fallback behavior. The user context propagation is well-implemented.
71-82: LGTM! Clean local factory methodThe
local()method provides a clear way to explicitly request local execution with proper user context handling.
49-49: emit_event is correctly defined and namespaced
The stub in cli/ext/build/ext.stub.php declaresfunction emit_eventunderBottledcode\DurablePhp\Ext, and both LocalOrchestrationClient and LocalEntityClient import it viause function Bottledcode\DurablePhp\Ext\emit_event;Since
function_exists('Bottledcode\DurablePhp\Ext\emit_event')will accurately detect it, no changes are needed.cli/ext/build/ext.stub.php (3)
1-7: LGTM! Properly documented PHP extension stub file.The file header and namespace declaration follow PHP standards correctly, and the
@generate-class-entriesdocblock indicates this is used for automatic code generation.
7-7: Stub function parameters are intentionally unused.The static analysis warnings about unused parameters are expected for PHP stub files. These are function signatures used for code generation - the actual implementation is in the C/Go extension code.
9-40: Thread safety of Worker instance management verified
- The PHP extension stores the current Worker handle in C-level thread‐local storage, isolating each thread’s Worker instance.
- The Go side uses cgo.NewHandle/getGoObject (cgo.Handle), which is managed by the Go runtime with internal synchronization and is safe for concurrent use.
- No additional mutexes or manual locking around Worker registration or retrieval are required.
cli/lib/consumer.go (2)
21-35: LGTM! Clean consumer initialization with proper error handling.The function correctly initializes a JetStream consumer with appropriate error handling using panic for unrecoverable errors during startup.
113-115: LGTM! Clean helper function for state ID parsing.Simple and effective extraction of state ID parsing logic.
cli/ext/build/ext.c (5)
11-17: LGTM! Proper error handling macro for Go handle validation.The macro correctly validates the Go handle and throws a PHP error with proper control flow.
56-64: LGTM! Proper cleanup of Go objects.The destructor correctly removes the Go object from the registry before deallocating the PHP object.
74-74: Thread-local storage usage is appropriate for worker context.The use of
__threadforcurrent_go_worker_handleensures each PHP request thread has its own worker context, which is the correct approach for FrankenPHP's threading model.
76-111: Constructor implementation handles thread-local worker correctly.The constructor properly checks for an existing handle to make it idempotent and uses the thread-local handle when available.
357-369: LGTM! Proper implementation of the emit_event function.The function correctly parses parameters and delegates to the Go implementation.
src/Glue/Glue.php (5)
79-119: Worker integration properly handles context extraction.The constructor correctly attempts to get the current worker and extracts all necessary context (target, source, user, payload) from it. Error handling for JSON decode is appropriate.
126-134: queryState properly delegates to worker.The method correctly uses the worker's queryState method and deserializes the result.
152-155: outputDelete correctly delegates to worker.Simple and effective delegation to the worker's delete method.
165-168: outputEvent properly converts and delegates to worker.The method correctly converts the EventDescription to an array before passing to the worker.
199-209: entityDecoder correctly retrieves state from worker.The method properly retrieves state from the worker and deserializes it as EntityHistory.
src/LocalOrchestrationClient.php (2)
72-83: LGTM! Event raising implementation is correctThe implementation properly wraps the event with orchestration context, serializes the data, and emits it through the extension.
140-143: LGTM! User context management is properly implementedThe implementation follows the same pattern as
LocalEntityClientfor managing user context.cli/ext/build/ext_arginfo.h (2)
1-2: Generated file - no review neededThis is an auto-generated file as indicated by the comment. Any necessary changes should be made to the
.stub.phpfile instead.
4-8: Parameter order verified
The arginfo declaration (userContext,event,from) matches the C implementation’sZEND_PARSE_PARAMETERSorder and the call toemit_event(userContext, event, from). No changes required.cli/ext/build/ext.go (1)
131-134: Potential stack overflow from recursive channel insertionPutting the message back into the channel and returning nil creates a recursive pattern that could lead to stack overflow if many messages need delaying.
Consider using a separate delay queue or continue to the next message instead:
- // Recursively handle delayed message by putting it back in channel - globalWorkerInstance.requestChan <- msg - return nil + // Re-queue the message for later processing + go func() { + time.Sleep(delay) + globalWorkerInstance.requestChan <- msg + }() + return nilLikely an incorrect or invalid review comment.
| static zend_object *ext_create_object(zend_class_entry *ce) { | ||
| fprintf(stderr, "[C DEBUG] ext_create_object called!\n"); | ||
| fflush(stderr); | ||
|
|
||
| /* Always allocate without properties due to corrupted class entry */ | ||
| ext_object *intern = ecalloc(1, sizeof(ext_object)); | ||
|
|
||
| fprintf(stderr, "[C DEBUG] About to call zend_object_std_init\n"); | ||
| fflush(stderr); | ||
|
|
||
| zend_object_std_init(&intern->std, ce); | ||
|
|
||
| fprintf(stderr, "[C DEBUG] zend_object_std_init completed\n"); | ||
| fflush(stderr); | ||
|
|
||
| /* Skip object_properties_init due to corrupted class entry */ | ||
|
|
||
| intern->std.handlers = &object_handlers_ext; | ||
| intern->go_handle = 0; /* will be set in __construct */ | ||
|
|
||
| fprintf(stderr, "[C DEBUG] ext_create_object returning\n"); | ||
| fflush(stderr); | ||
|
|
||
| return &intern->std; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or conditionally compile debug output in production.
The extensive debug fprintf statements should be removed or controlled by a debug flag for production builds.
+#ifdef DEBUG_EXTENSION
static zend_object *ext_create_object(zend_class_entry *ce) {
fprintf(stderr, "[C DEBUG] ext_create_object called!\n");
fflush(stderr);
+#else
+static zend_object *ext_create_object(zend_class_entry *ce) {
+#endif
/* Always allocate without properties due to corrupted class entry */
ext_object *intern = ecalloc(1, sizeof(ext_object));
+#ifdef DEBUG_EXTENSION
fprintf(stderr, "[C DEBUG] About to call zend_object_std_init\n");
fflush(stderr);
+#endifCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cli/ext/build/ext.c around lines 30 to 54, several fprintf(stderr, ...) debug
prints are present; remove them or guard them behind a compile-time debug macro
so production builds don’t emit debug output. Replace direct fprintf calls with
either no-ops for release builds or wrap them in an #ifdef EXT_DEBUG / #endif
(or use a project-wide DEBUG macro) so the messages are only compiled in when
debugging is enabled, or alternatively route them through the PHP/zend logging
facility (e.g., zend_error) if persistent runtime logging is required.
| func (w *localResponseWriter) Header() http.Header { | ||
| return make(http.Header) | ||
| } | ||
|
|
||
| func (w *localResponseWriter) Write(data []byte) (int, error) { | ||
| // Accumulate response data in buffer | ||
| w.buffer = append(w.buffer, data...) | ||
|
|
||
| // Start a goroutine to wait for completion if not already done | ||
| if !w.sent { | ||
| w.sent = true | ||
| go w.waitForCompletion() | ||
| } | ||
|
|
||
| return len(data), nil | ||
| } | ||
|
|
||
| func (w *localResponseWriter) WriteHeader(statusCode int) { | ||
| // For local requests, we don't need to handle status codes | ||
| } | ||
|
|
||
| func (w *localResponseWriter) waitForCompletion() { | ||
| // Wait for the request to complete | ||
| <-w.doneCh | ||
|
|
||
| // Send the complete response | ||
| w.responseCh <- string(w.buffer) | ||
| close(w.responseCh) | ||
| } |
There was a problem hiding this comment.
Race condition in localResponseWriter
The waitForCompletion goroutine is started on first write, but the channel might be closed before all data is written if multiple writes occur.
func (w *localResponseWriter) Write(data []byte) (int, error) {
// Accumulate response data in buffer
w.buffer = append(w.buffer, data...)
-
- // Start a goroutine to wait for completion if not already done
- if !w.sent {
- w.sent = true
- go w.waitForCompletion()
- }
-
return len(data), nil
}
-func (w *localResponseWriter) waitForCompletion() {
- // Wait for the request to complete
- <-w.doneCh
-
- // Send the complete response
- w.responseCh <- string(w.buffer)
- close(w.responseCh)
+// Call this when the request is done
+func (w *localResponseWriter) sendResponse() {
+ if !w.sent {
+ w.sent = true
+ w.responseCh <- string(w.buffer)
+ close(w.responseCh)
+ }
}Then ensure sendResponse() is called when the request completes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (w *localResponseWriter) Header() http.Header { | |
| return make(http.Header) | |
| } | |
| func (w *localResponseWriter) Write(data []byte) (int, error) { | |
| // Accumulate response data in buffer | |
| w.buffer = append(w.buffer, data...) | |
| // Start a goroutine to wait for completion if not already done | |
| if !w.sent { | |
| w.sent = true | |
| go w.waitForCompletion() | |
| } | |
| return len(data), nil | |
| } | |
| func (w *localResponseWriter) WriteHeader(statusCode int) { | |
| // For local requests, we don't need to handle status codes | |
| } | |
| func (w *localResponseWriter) waitForCompletion() { | |
| // Wait for the request to complete | |
| <-w.doneCh | |
| // Send the complete response | |
| w.responseCh <- string(w.buffer) | |
| close(w.responseCh) | |
| } | |
| func (w *localResponseWriter) Header() http.Header { | |
| return make(http.Header) | |
| } | |
| func (w *localResponseWriter) Write(data []byte) (int, error) { | |
| // Accumulate response data in buffer | |
| w.buffer = append(w.buffer, data...) | |
| return len(data), nil | |
| } | |
| func (w *localResponseWriter) WriteHeader(statusCode int) { | |
| // For local requests, we don't need to handle status codes | |
| } | |
| // Call this when the request is done | |
| func (w *localResponseWriter) sendResponse() { | |
| if !w.sent { | |
| w.sent = true | |
| w.responseCh <- string(w.buffer) | |
| close(w.responseCh) | |
| } | |
| } |
🤖 Prompt for AI Agents
In cli/ext/build/ext.go around lines 245 to 273, the waitForCompletion goroutine
can race with concurrent Write calls and close responseCh before all writes
finish; change Write to be goroutine-safe (use a mutex or atomic to append to
buffer and to set/sync the sent flag or use sync.Once to start waitForCompletion
exactly once), ensure waitForCompletion reads the doneCh only after all writers
have finished, send the accumulated response via a single sendResponse() call
into responseCh and close responseCh after sending, and make sure sendResponse()
is invoked when the request finishes (i.e., when doneCh is closed) rather than
starting the goroutine on the first write.
|
|
||
| require github.com/dunglas/frankenphp v1.9.0 | ||
|
|
||
| replace github.com/dunglas/frankenphp v1.9.0 => /home/withinboredom/code/frankenphp |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the local FrankenPHP replace directive for production readiness.
The replace directive points to a local development path /home/withinboredom/code/frankenphp which is specific to your development environment. This will cause build failures in other environments.
Run the following script to check if this replace directive is used elsewhere and should be addressed before merging:
🏁 Script executed:
#!/bin/bash
# Description: Check for hardcoded local paths in go.mod files and related build configurations
# Search for similar replace directives with local paths
fd -e mod -x cat {} \; | grep -n "replace.*=>.*/home/"
# Search for any references to this local path in build scripts
rg "/home/withinboredom" --type makefile --type shell --type goLength of output: 242
Local FrankenPHP replace directive detected
A local path replace remains in cli/go.mod that will break builds outside your machine:
- cli/go.mod (line 7):
replace github.com/dunglas/frankenphp v1.9.0 => /home/withinboredom/code/frankenphp
Please remove this directive or update it to use the published v1.9.0 module (e.g. point to the GitHub tag) before merging.
🤖 Prompt for AI Agents
In cli/go.mod around line 7 there is a local replace directive pointing to
/home/withinboredom/code/frankenphp which will break CI and other developers'
builds; remove this local replace or change it to the published module reference
for v1.9.0 (for example: remove the replace line entirely or replace it with the
module path pointing to the GitHub tag/release) and run go mod tidy to update
go.sum so the project builds for others.
| frankenphp: ext/build/ext.go ext/build/ext_arginfo.h | ||
| CGO_ENABLED=1 \ | ||
| XCADDY_GO_BUILD_FLAGS="$(XCADDY_FLAGS)" \ | ||
| CGO_CFLAGS="$(PHP_INCLUDES)" \ | ||
| CGO_LDFLAGS="$(PHP_LDFLAGS) $(PHP_LIBS)" \ | ||
| xcaddy build \ | ||
| --output frankenphp \ | ||
| --with github.com/dunglas/frankenphp/caddy=$(LOCAL_FRANKENPHP_CADDY) \ | ||
| --with github.com/dunglas/frankenphp=$(LOCAL_FRANKENPHP) \ | ||
| --with github.com/bottledcode/durable-php/cli=$(LOCAL_MODULE) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add phony targets and improve build organization
The Makefile is missing standard phony targets and could benefit from better organization.
Apply this diff to add missing phony targets and improve structure:
+.PHONY: all clean test frankenphp
+
+all: frankenphp
+
+clean:
+ rm -f frankenphp
+ rm -f ext/build/ext_arginfo.h
+
+test:
+ @echo "No tests defined"
+
# Targets
frankenphp: ext/build/ext.go ext/build/ext_arginfo.h📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| frankenphp: ext/build/ext.go ext/build/ext_arginfo.h | |
| CGO_ENABLED=1 \ | |
| XCADDY_GO_BUILD_FLAGS="$(XCADDY_FLAGS)" \ | |
| CGO_CFLAGS="$(PHP_INCLUDES)" \ | |
| CGO_LDFLAGS="$(PHP_LDFLAGS) $(PHP_LIBS)" \ | |
| xcaddy build \ | |
| --output frankenphp \ | |
| --with github.com/dunglas/frankenphp/caddy=$(LOCAL_FRANKENPHP_CADDY) \ | |
| --with github.com/dunglas/frankenphp=$(LOCAL_FRANKENPHP) \ | |
| --with github.com/bottledcode/durable-php/cli=$(LOCAL_MODULE) | |
| .PHONY: all clean test frankenphp | |
| all: frankenphp | |
| clean: | |
| rm -f frankenphp | |
| rm -f ext/build/ext_arginfo.h | |
| test: | |
| @echo "No tests defined" | |
| # Targets | |
| frankenphp: ext/build/ext.go ext/build/ext_arginfo.h | |
| CGO_ENABLED=1 \ | |
| XCADDY_GO_BUILD_FLAGS="$(XCADDY_FLAGS)" \ | |
| CGO_CFLAGS="$(PHP_INCLUDES)" \ | |
| CGO_LDFLAGS="$(PHP_LDFLAGS) $(PHP_LIBS)" \ | |
| xcaddy build \ | |
| --output frankenphp \ | |
| --with github.com/dunglas/frankenphp/caddy=$(LOCAL_FRANKENPHP_CADDY) \ | |
| --with github.com/dunglas/frankenphp=$(LOCAL_FRANKENPHP) \ | |
| --with github.com/bottledcode/durable-php/cli=$(LOCAL_MODULE) |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 19-19: Target body for "frankenphp" exceeds allowed length of 5 (9).
(maxbodylength)
🤖 Prompt for AI Agents
In cli/Makefile around lines 19 to 28, the build recipe for the frankenphp
target lacks standard phony declarations and top-level organization; add a
.PHONY line listing targets (e.g., .PHONY: all frankenphp build clean install)
near the top, introduce a simple top-level target like all that depends on
frankenphp, and keep the existing recipe under the frankenphp target; optionally
add a clean target to remove the built binary and a build alias that forwards to
frankenphp to make invocation and maintenance clearer.
|
|
||
| namespace Bottledcode\DurablePhp\Glue; | ||
|
|
||
| require_once __DIR__ . '/autoload.php'; |
There was a problem hiding this comment.
Fix the require path for autoload.php
The require statement uses __DIR__ . '/autoload.php' but there's no evidence that an autoload.php file exists in the same directory as worker.php. This will likely cause a fatal error when the worker starts.
Verify the correct path to the autoloader file:
#!/bin/bash
# Description: Check for autoload.php files in the codebase
fd "autoload.php" --type f🤖 Prompt for AI Agents
In src/Glue/worker.php around line 27, the require_once uses __DIR__ .
'/autoload.php' but that file likely doesn't exist there; search the repo for
the real autoload.php (e.g. vendor/autoload.php) and replace the require_once
with the correct relative path to the discovered autoloader. Ensure the new
require_once points to the actual file (commonly __DIR__ .
'/../../vendor/autoload.php' or similar) and wrap it with a file_exists check
that throws/ logs a clear fatal error if the autoloader cannot be found so the
worker fails fast with a helpful message.
|
|
||
| require_once __DIR__ . '/autoload.php'; | ||
|
|
||
| $logger->info('Starting worker'); |
There was a problem hiding this comment.
Fix undefined variable error
The $logger variable is used without being defined, which will cause a fatal error.
Apply this diff to fix the undefined variable:
+use Psr\Log\LoggerInterface;
+
require_once __DIR__ . '/autoload.php';
-$logger->info('Starting worker');
+// Get logger from autoload or create a simple one
+$logger = $logger ?? new \Psr\Log\NullLogger();
+$logger->info('Starting worker');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $logger->info('Starting worker'); | |
| use Psr\Log\LoggerInterface; | |
| require_once __DIR__ . '/autoload.php'; | |
| // Get logger from autoload or create a simple one | |
| $logger = $logger ?? new \Psr\Log\NullLogger(); | |
| $logger->info('Starting worker'); |
🤖 Prompt for AI Agents
In src/Glue/worker.php around line 29, the variable $logger is referenced but
not defined; initialize or obtain a logger instance before using it (e.g.,
retrieve the logger from the DI/container or instantiate and configure a logger
and assign it to $logger) and then call $logger->info('Starting worker'); so
$logger is defined in the current scope prior to line 29.
| frankenphp_handle_request(static function (): void { | ||
| global $logger; | ||
|
|
||
| $glue = new Glue($logger); | ||
| $glue->process(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the FrankenPHP request handler
The callback function lacks error handling, which could cause the worker to crash silently on exceptions.
Apply this diff to add proper error handling:
frankenphp_handle_request(static function (): void {
- global $logger;
+ global $logger;
+
+ try {
+ $glue = new Glue($logger);
+ $glue->process();
+ } catch (\Throwable $e) {
+ $logger->error('Worker request processing failed', [
+ 'error' => $e->getMessage(),
+ 'trace' => $e->getTraceAsString()
+ ]);
+
+ // Send error response
+ http_response_code(500);
+ echo json_encode(['error' => 'Internal server error']);
+ }
- $glue = new Glue($logger);
- $glue->process();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| frankenphp_handle_request(static function (): void { | |
| global $logger; | |
| $glue = new Glue($logger); | |
| $glue->process(); | |
| }); | |
| frankenphp_handle_request(static function (): void { | |
| global $logger; | |
| try { | |
| $glue = new Glue($logger); | |
| $glue->process(); | |
| } catch (\Throwable $e) { | |
| $logger->error('Worker request processing failed', [ | |
| 'error' => $e->getMessage(), | |
| 'trace' => $e->getTraceAsString() | |
| ]); | |
| // Send error response | |
| http_response_code(500); | |
| echo json_encode(['error' => 'Internal server error']); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In src/Glue/worker.php around lines 31–36, the FrankenPHP request handler
callback is missing error handling; wrap the callback body in a try/catch that
catches Throwable, log the exception with $logger->error including message and
stack trace, send a safe 500 response (set appropriate status header and a
generic message) and avoid rethrowing so the worker doesn't crash silently;
optionally include a finally block to perform any cleanup or flush logs.
| public function startNew(string $name, array $args = [], ?string $id = null): OrchestrationInstance | ||
| { | ||
| $orchestrationId = OrchestrationId($name, $id ?? Uuid::uuid4()->toString()); | ||
| $stateId = StateId::fromOrchestrationId($orchestrationId); | ||
|
|
||
| $event = WithOrchestration::forInstance( | ||
| $stateId, | ||
| StartOrchestration::fromArray(SerializedArray::fromArray($args)), | ||
| ); | ||
|
|
||
| $eventDescription = new EventDescription($event); | ||
| $userArray = $this->userContext ? Serializer::serialize($this->userContext) : null; | ||
|
|
||
| $sequence = emit_event($userArray, $eventDescription->toArray(), $stateId->id); | ||
|
|
||
| return $orchestrationId->toOrchestrationInstance(); | ||
| } |
There was a problem hiding this comment.
Fix unused variable and improve ID generation
The $sequence variable is assigned but never used. Additionally, using Uuid::uuid4() instead of uuid7() breaks consistency with the rest of the codebase which uses UUIDv7 for time-ordered identifiers.
public function startNew(string $name, array $args = [], ?string $id = null): OrchestrationInstance
{
- $orchestrationId = OrchestrationId($name, $id ?? Uuid::uuid4()->toString());
+ $orchestrationId = OrchestrationId($name, $id ?? Uuid::uuid7()->toString());
$stateId = StateId::fromOrchestrationId($orchestrationId);
$event = WithOrchestration::forInstance(
$stateId,
StartOrchestration::fromArray(SerializedArray::fromArray($args)),
);
$eventDescription = new EventDescription($event);
$userArray = $this->userContext ? Serializer::serialize($this->userContext) : null;
- $sequence = emit_event($userArray, $eventDescription->toArray(), $stateId->id);
+ emit_event($userArray, $eventDescription->toArray(), $stateId->id);
return $orchestrationId->toOrchestrationInstance();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function startNew(string $name, array $args = [], ?string $id = null): OrchestrationInstance | |
| { | |
| $orchestrationId = OrchestrationId($name, $id ?? Uuid::uuid4()->toString()); | |
| $stateId = StateId::fromOrchestrationId($orchestrationId); | |
| $event = WithOrchestration::forInstance( | |
| $stateId, | |
| StartOrchestration::fromArray(SerializedArray::fromArray($args)), | |
| ); | |
| $eventDescription = new EventDescription($event); | |
| $userArray = $this->userContext ? Serializer::serialize($this->userContext) : null; | |
| $sequence = emit_event($userArray, $eventDescription->toArray(), $stateId->id); | |
| return $orchestrationId->toOrchestrationInstance(); | |
| } | |
| public function startNew(string $name, array $args = [], ?string $id = null): OrchestrationInstance | |
| { | |
| $orchestrationId = OrchestrationId($name, $id ?? Uuid::uuid7()->toString()); | |
| $stateId = StateId::fromOrchestrationId($orchestrationId); | |
| $event = WithOrchestration::forInstance( | |
| $stateId, | |
| StartOrchestration::fromArray(SerializedArray::fromArray($args)), | |
| ); | |
| $eventDescription = new EventDescription($event); | |
| $userArray = $this->userContext ? Serializer::serialize($this->userContext) : null; | |
| emit_event($userArray, $eventDescription->toArray(), $stateId->id); | |
| return $orchestrationId->toOrchestrationInstance(); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
111-111: Avoid unused local variables such as '$sequence'. (Unused Code Rules)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In src/LocalOrchestrationClient.php around lines 98 to 114, the code assigns
$sequence but never uses it and generates the orchestration ID with
Uuid::uuid4(), which is inconsistent with the project's UUIDv7 usage; replace
Uuid::uuid4()->toString() with Uuid::uuid7()->toString() when building the
OrchestrationId and remove the unused $sequence assignment (or if the sequence
value is required elsewhere, use it appropriately) so there are no unused
variables and IDs are time-ordered.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
src/RemoteEntityClient.php (1)
119-121: Trailing spaces in URI will break requestsThe entity signal endpoint has three trailing spaces in the path, causing a 404/mis-route.
- $req = new Request("{$this->apiHost}/entity/{$name}/{$id} ", 'PUT', json_encode($input, JSON_THROW_ON_ERROR)); + $req = new Request("{$this->apiHost}/entity/{$name}/{$id}", 'PUT', json_encode($input, JSON_THROW_ON_ERROR));src/RemoteOrchestrationClient.php (1)
145-159: Guard against missing X-Id header when starting orchestrationIf the server fails to return X-Id, StateId::fromString(null) will likely throw. Add a defensive check and surface a clear error.
- return StateId::fromString($result->getHeader('X-Id'))->toOrchestrationInstance(); + $idHeader = $result->getHeader('X-Id'); + if ($idHeader === null || $idHeader === '') { + throw new RuntimeException('Missing X-Id header starting orchestration'); + } + return StateId::fromString($idHeader)->toOrchestrationInstance();
♻️ Duplicate comments (1)
src/LocalOrchestrationClient.php (1)
98-114: Use UUIDv7 and remove unused $sequence (duplicate of prior review)Keep IDs time-ordered and address the unused variable.
- $orchestrationId = OrchestrationId($name, $id ?? Uuid::uuid4()->toString()); + $orchestrationId = OrchestrationId($name, $id ?? Uuid::uuid7()->toString()); @@ - $sequence = emit_event($userArray, $eventDescription->toArray(), $stateId->id); + emit_event($userArray, $eventDescription->toArray(), $stateId->id);
🧹 Nitpick comments (12)
src/EntityClientInterface.php (2)
28-28: Avoid leaking Glue types into top-level client interfacesImporting Glue\Provenance couples the public client interface to a lower layer. Consider defining a top-level AuthContext/Provenance value object in the root namespace (or an interface) to avoid cross-layer dependencies.
38-38: Widened union type is fine; document semantics and expected handlingPlease document what each variant means (string as bearer token, Provenance for local context propagation, null to clear auth). This helps implementers keep behavior consistent.
Apply a short docblock:
interface EntityClientInterface { - public function withAuth(Provenance|string|null $token): void; + /** + * Set the calling user context for subsequent requests. + * - string: Bearer token to be sent to the remote API. + * - Provenance: Local provenance context (not a bearer token). + * - null: Clear any existing auth/context. + */ + public function withAuth(Provenance|string|null $token): void;Please confirm all implementers were updated to handle all 3 variants consistently.
src/OrchestrationClientInterface.php (2)
28-28: Same layering concern: interface now depends on Glue\ProvenanceTo keep package boundaries clean, prefer a root-level AuthContext (or interface) that both glue and clients can rely on, rather than importing from Glue.
35-35: Add minimal docs for withAuth union to ensure consistent implementationsExplicitly describe string vs Provenance vs null semantics to reduce ambiguity across implementations.
interface OrchestrationClientInterface { - public function withAuth(Provenance|string|null $token): void; + /** + * Set the calling user context for subsequent requests. + * - string: Bearer token for remote calls. + * - Provenance: Local provenance context (not a bearer token). + * - null: Clear any existing auth/context. + */ + public function withAuth(Provenance|string|null $token): void;Ensure all implementing classes were updated accordingly (local and remote).
src/RemoteEntityClient.php (1)
63-73: DRY header application across requestsMultiple places manually set Authorization headers. After introducing applyAuth(), replace these if-blocks with a single call to reduce duplication and future drift.
Example change at each request site:
- if ($this->userToken) { - $req->setHeader('Authorization', 'Bearer ' . $this->userToken); - } + $this->applyAuth($req);Also applies to: 135-147, 158-167, 171-179, 183-191, 195-203, 207-215, 219-227
src/RemoteOrchestrationClient.php (1)
112-121: Centralize auth header injectionMultiple duplicate header blocks will drift as behavior evolves (e.g., provenance support). Replace them with $this->applyAuth($req) for maintainability.
Also applies to: 150-156, 176-184, 206-214, 218-226, 230-238, 242-250, 254-262
src/DurableClient.php (1)
47-69: De-duplicate remote/local construction by delegating to the new factoriesget() now duplicates logic already present in local()/remote(). Route through those helpers to keep a single source of truth.
- public static function get(string $apiHost = 'http://localhost:8080', ?Provenance $userContext = null): self - { - if (function_exists('Bottledcode\DurablePhp\Ext\emit_event')) { - $entityClient = new LocalEntityClient(); - $orchestrationClient = new LocalOrchestrationClient(); - - if ($userContext !== null) { - $entityClient->setUserContext($userContext); - $orchestrationClient->setUserContext($userContext); - } - - return new self($entityClient, $orchestrationClient); - } - - $builder = new HttpClientBuilder(); - $builder->retry(3); - - $httpClient = $builder->build(); - $entityClient = new RemoteEntityClient($apiHost, $httpClient); - $orchestrationClient = new RemoteOrchestrationClient($apiHost, $httpClient); - - return new self($entityClient, $orchestrationClient); - } + public static function get(string $apiHost = 'http://localhost:8080', ?Provenance $userContext = null): self + { + if (function_exists('Bottledcode\DurablePhp\Ext\emit_event')) { + return self::local($userContext); + } + return self::remote($apiHost); + }src/LocalEntityClient.php (2)
100-109: Minor: avoid recomputing StateId and reuse it for emit_eventCompute once and reuse for readability and micro-efficiency.
- $event = WithEntity::forInstance( - StateId::fromEntityId($entityId), - RaiseEvent::forSignal($operationName, SerializedArray::fromArray($input), $scheduledTime), - ); - - $eventDescription = new EventDescription($event); - $userArray = $this->userContext ? Serializer::serialize($this->userContext) : null; - - emit_event($userArray, $eventDescription->toArray(), StateId::fromEntityId($entityId)->id); + $stateId = StateId::fromEntityId($entityId); + $event = WithEntity::forInstance( + $stateId, + RaiseEvent::forSignal($operationName, SerializedArray::fromArray($input), $scheduledTime), + ); + + $eventDescription = new EventDescription($event); + $userArray = $this->userContext ? Serializer::serialize($this->userContext) : null; + + emit_event($userArray, $eventDescription->toArray(), $stateId->id);
60-63: Silence PHPMD unused-parameter warnings in unsupported methodsThese methods intentionally throw. Unset parameters to appease static analysis.
public function listEntities(EntityFilter $filter, int $page): Generator { + unset($filter, $page); throw new Exception('listEntities not supported in LocalEntityClient - use RemoteEntityClient for this operation'); } @@ public function getEntitySnapshot(EntityId $entityId): ?EntityState { + unset($entityId); throw new Exception('getEntitySnapshot not supported in LocalEntityClient - use RemoteEntityClient for this operation'); } @@ public function deleteEntity(EntityId $entityId): void { + unset($entityId); throw new Exception('deleteEntity not supported in LocalEntityClient - use RemoteEntityClient for this operation'); } @@ public function shareEntityOwnership(EntityId $id, string $with): void { + unset($id, $with); throw new Exception('shareEntityOwnership not supported in LocalEntityClient - use RemoteEntityClient for this operation'); } @@ public function grantEntityAccessToUser(EntityId $id, string $user, Operation $operation): void { + unset($id, $user, $operation); throw new Exception('grantEntityAccessToUser not supported in LocalEntityClient - use RemoteEntityClient for this operation'); } @@ public function grantEntityAccessToRole(EntityId $id, string $role, Operation $operation): void { + unset($id, $role, $operation); throw new Exception('grantEntityAccessToRole not supported in LocalEntityClient - use RemoteEntityClient for this operation'); } @@ public function revokeEntityAccessToUser(EntityId $id, string $user): void { + unset($id, $user); throw new Exception('revokeEntityAccessToUser not supported in LocalEntityClient - use RemoteEntityClient for this operation'); } @@ public function revokeEntityAccessToRole(EntityId $id, string $role): void { + unset($id, $role); throw new Exception('revokeEntityAccessToRole not supported in LocalEntityClient - use RemoteEntityClient for this operation'); }Also applies to: 112-115, 129-132, 134-137, 139-142, 144-147, 149-152, 154-157
src/LocalOrchestrationClient.php (3)
72-83: Minor: compute StateId once in raiseEvent for clarityAvoid recomputing from the same input.
- $event = WithOrchestration::forInstance( - StateId::fromOrchestrationInstance($instance), - RaiseEvent::forSignal($eventName, SerializedArray::fromArray($eventData)), - ); + $stateId = StateId::fromOrchestrationInstance($instance); + $event = WithOrchestration::forInstance( + $stateId, + RaiseEvent::forSignal($eventName, SerializedArray::fromArray($eventData)), + ); @@ - emit_event($userArray, $eventDescription->toArray(), StateId::fromOrchestrationInstance($instance)->id); + emit_event($userArray, $eventDescription->toArray(), $stateId->id);
33-33: Remove unused SpyProxy dependency from LocalOrchestrationClientSpyProxy isn't used in this class. Drop the import and constructor param to reduce noise.
-use Bottledcode\DurablePhp\Proxy\SpyProxy; @@ - public function __construct( - private SpyProxy $spyProxy = new SpyProxy(), - ) {} + // No constructor requiredAlso applies to: 49-51
60-63: Silence PHPMD unused-parameter warnings in unsupported methodsUnset parameters at the start of each throwing method.
public function purge(OrchestrationInstance $instance): void { + unset($instance); throw new Exception('purge not supported in LocalOrchestrationClient - use RemoteOrchestrationClient for this operation'); } @@ public function getStatus(OrchestrationInstance $instance): Status { + unset($instance); throw new Exception('getStatus not supported in LocalOrchestrationClient - use RemoteOrchestrationClient for this operation'); } @@ public function restart(OrchestrationInstance $instance): void { + unset($instance); throw new Exception('restart not implemented in LocalOrchestrationClient'); } @@ public function resume(OrchestrationInstance $instance, string $reason): void { + unset($instance, $reason); throw new Exception('resume not implemented in LocalOrchestrationClient'); } @@ public function suspend(OrchestrationInstance $instance, string $reason): void { + unset($instance, $reason); throw new Exception('suspend not implemented in LocalOrchestrationClient'); } @@ public function terminate(OrchestrationInstance $instance, string $reason): void { + unset($instance, $reason); throw new Exception('terminate not implemented in LocalOrchestrationClient'); } @@ public function waitForCompletion(OrchestrationInstance $instance): void { + unset($instance); throw new Exception('waitForCompletion not supported in LocalOrchestrationClient - use RemoteOrchestrationClient for this operation'); } @@ public function shareOrchestrationOwnership(OrchestrationInstance $id, string $with): void { + unset($id, $with); throw new Exception('shareOrchestrationOwnership not supported in LocalOrchestrationClient - use RemoteOrchestrationClient for this operation'); } @@ public function grantOrchestrationAccessToUser(OrchestrationInstance $id, string $user, Operation $operation): void { + unset($id, $user, $operation); throw new Exception('grantOrchestrationAccessToUser not supported in LocalOrchestrationClient - use RemoteOrchestrationClient for this operation'); } @@ public function grantOrchestrationAccessToRole(OrchestrationInstance $id, string $role, Operation $operation): void { + unset($id, $role, $operation); throw new Exception('grantOrchestrationAccessToRole not supported in LocalOrchestrationClient - use RemoteOrchestrationClient for this operation'); } @@ public function revokeOrchestrationAccessToUser(OrchestrationInstance $id, string $user): void { + unset($id, $user); throw new Exception('revokeOrchestrationAccessToUser not supported in LocalOrchestrationClient - use RemoteOrchestrationClient for this operation'); } @@ public function revokeOrchestrationAccessToRole(OrchestrationInstance $id, string $role): void { + unset($id, $role); throw new Exception('revokeOrchestrationAccessToRole not supported in LocalOrchestrationClient - use RemoteOrchestrationClient for this operation'); }Also applies to: 66-69, 86-89, 92-95, 116-120, 123-126, 129-132, 145-148, 150-153, 155-158, 160-163, 165-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/DurableClient.php(3 hunks)src/EntityClientInterface.php(2 hunks)src/LocalEntityClient.php(1 hunks)src/LocalOrchestrationClient.php(1 hunks)src/OrchestrationClientInterface.php(1 hunks)src/RemoteEntityClient.php(2 hunks)src/RemoteOrchestrationClient.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/RemoteEntityClient.php (3)
src/Glue/Provenance.php (1)
Provenance(30-33)src/EntityClientInterface.php (1)
withAuth(38-38)src/OrchestrationClientInterface.php (1)
withAuth(35-35)
src/RemoteOrchestrationClient.php (2)
src/Glue/Provenance.php (1)
Provenance(30-33)src/OrchestrationClientInterface.php (1)
withAuth(35-35)
src/EntityClientInterface.php (2)
src/Glue/Provenance.php (1)
Provenance(30-33)src/OrchestrationClientInterface.php (1)
withAuth(35-35)
src/OrchestrationClientInterface.php (2)
src/Glue/Provenance.php (1)
Provenance(30-33)src/EntityClientInterface.php (1)
withAuth(38-38)
src/LocalOrchestrationClient.php (10)
src/Events/EventDescription.php (1)
EventDescription(37-223)src/Events/WithOrchestration.php (1)
WithOrchestration(30-59)src/Glue/Provenance.php (1)
Provenance(30-33)src/State/Serializer.php (2)
Serializer(31-82)serialize(35-48)src/DurableClient.php (17)
Override(173-178)__construct(42-45)listInstances(118-121)purge(123-126)getStatus(113-116)raiseEvent(128-131)restart(133-136)resume(138-141)startNew(143-146)suspend(148-151)terminate(153-156)waitForCompletion(158-161)shareOrchestrationOwnership(210-213)grantOrchestrationAccessToUser(215-218)grantOrchestrationAccessToRole(220-223)revokeOrchestrationAccessToUser(225-228)revokeOrchestrationAccessToRole(230-233)cli/ext/build/ext.stub.php (1)
emit_event(7-7)src/OrchestrationClientInterface.php (16)
listInstances(39-39)purge(41-41)getStatus(37-37)raiseEvent(43-43)restart(45-45)resume(47-47)startNew(49-49)suspend(51-51)terminate(53-53)waitForCompletion(55-55)withAuth(35-35)shareOrchestrationOwnership(57-57)grantOrchestrationAccessToUser(59-59)grantOrchestrationAccessToRole(61-61)revokeOrchestrationAccessToUser(63-63)revokeOrchestrationAccessToRole(65-65)src/SerializedArray.php (1)
SerializedArray(30-85)src/Events/With.php (1)
id(32-39)src/State/Ids/StateId.php (1)
toOrchestrationInstance(90-101)
src/DurableClient.php (7)
src/Glue/Provenance.php (1)
Provenance(30-33)src/LocalEntityClient.php (2)
LocalEntityClient(48-158)setUserContext(123-126)src/LocalOrchestrationClient.php (2)
LocalOrchestrationClient(45-169)setUserContext(140-143)src/RemoteEntityClient.php (1)
RemoteEntityClient(44-228)src/RemoteOrchestrationClient.php (1)
RemoteOrchestrationClient(47-263)src/EntityClientInterface.php (1)
withAuth(38-38)src/OrchestrationClientInterface.php (1)
withAuth(35-35)
src/LocalEntityClient.php (14)
src/Events/EventDescription.php (1)
EventDescription(37-223)src/Events/WithEntity.php (1)
WithEntity(29-59)src/Glue/Provenance.php (1)
Provenance(30-33)src/Proxy/SpyException.php (1)
SpyException(5-5)src/Proxy/SpyProxy.php (1)
SpyProxy(31-126)src/Search/EntityFilter.php (1)
EntityFilter(27-30)src/State/Serializer.php (2)
Serializer(31-82)serialize(35-48)src/Proxy/Generator.php (2)
Generator(35-166)define(44-69)src/RemoteEntityClient.php (10)
Override(56-57)Override(59-73)Override(75-102)Override(104-130)__construct(48-54)shareEntityOwnership(169-179)grantEntityAccessToUser(181-191)grantEntityAccessToRole(193-203)revokeEntityAccessToUser(205-215)revokeEntityAccessToRole(217-227)cli/ext/build/ext.stub.php (1)
emit_event(7-7)src/EntityClientInterface.php (12)
cleanEntityStorage(43-43)listEntities(50-50)signal(70-70)signalEntity(55-60)getEntitySnapshot(79-79)withAuth(38-38)deleteEntity(84-84)shareEntityOwnership(86-86)grantEntityAccessToUser(88-88)grantEntityAccessToRole(90-90)revokeEntityAccessToUser(92-92)revokeEntityAccessToRole(94-94)src/State/Ids/StateId.php (1)
fromEntityId(69-72)src/SerializedArray.php (1)
SerializedArray(30-85)src/Events/With.php (1)
id(32-39)
🪛 PHPMD (2.15.0)
src/LocalOrchestrationClient.php
60-60: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
66-66: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
86-86: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
92-92: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
92-92: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
111-111: Avoid unused local variables such as '$sequence'. (Unused Code Rules)
(UnusedLocalVariable)
117-117: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
117-117: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
123-123: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
123-123: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
129-129: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
145-145: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
145-145: Avoid unused parameters such as '$with'. (Unused Code Rules)
(UnusedFormalParameter)
150-150: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
150-150: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
150-150: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
155-155: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
155-155: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
155-155: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
165-165: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
165-165: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
src/LocalEntityClient.php
60-60: Avoid unused parameters such as '$filter'. (Unused Code Rules)
(UnusedFormalParameter)
60-60: Avoid unused parameters such as '$page'. (Unused Code Rules)
(UnusedFormalParameter)
112-112: Avoid unused parameters such as '$entityId'. (Unused Code Rules)
(UnusedFormalParameter)
129-129: Avoid unused parameters such as '$entityId'. (Unused Code Rules)
(UnusedFormalParameter)
134-134: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
134-134: Avoid unused parameters such as '$with'. (Unused Code Rules)
(UnusedFormalParameter)
139-139: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
139-139: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
139-139: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
149-149: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
149-149: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
154-154: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
154-154: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (4)
src/RemoteEntityClient.php (1)
114-118: Potential missing import/namespace for SerializedArraySerializedArray is referenced without import; within this namespace it resolves to Bottledcode\DurablePhp\SerializedArray. If the class actually lives under State (e.g., Bottledcode\DurablePhp\State\SerializedArray), add the appropriate use statement or FQN.
Would you like me to scan the repo and propose the correct import?
src/RemoteOrchestrationClient.php (1)
115-121: Potential missing import/namespace for SerializedArraySame as entity client: ensure SerializedArray resolves to the intended class.
src/DurableClient.php (2)
174-178: Forwarding union auth is fine; ensure implementers handle Provenance correctlyThis assumes both clients support string|Provenance|null. Given the mismatches in Remote* clients, this currently risks TypeErrors or malformed headers.
Once Remote* fixes land, this forwarding is good to go.
47-69: Overall: Nice split between local and remote paths, and provenance-aware local flowThe extension auto-detection and dedicated factories are a solid direction for enabling in-process execution with provenance context.
Also applies to: 71-92
| public function withAuth(Provenance|string|null $token): void | ||
| { | ||
| $this->userToken = $token; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type mismatch: storing Provenance|null into string-typed $userToken
withAuth accepts Provenance|string|null but assigns directly to a string-typed property. This will fatally error for non-string inputs and also incorrectly attempts to send a Provenance as a Bearer token.
[ suggest_essential_refactor ]
Recommended fix: store bearer tokens and provenance separately, and centralize header application.
class RemoteEntityClient implements EntityClientInterface
{
- private string $userToken = '';
+ private ?string $userToken = null;
+ private ?Provenance $provenance = null;
@@
- public function withAuth(Provenance|string|null $token): void
- {
- $this->userToken = $token;
- }
+ public function withAuth(Provenance|string|null $token): void
+ {
+ $this->userToken = \is_string($token) ? $token : null;
+ $this->provenance = $token instanceof Provenance ? $token : null;
+ }Additionally, add a small helper to apply headers consistently (apply outside this range):
// Add to class:
private function applyAuth(Request $req): void
{
if ($this->userToken !== null) {
$req->setHeader('Authorization', 'Bearer ' . $this->userToken);
} elseif ($this->provenance !== null) {
// Align with your API: either a JSON header or separate fields.
$req->setHeader('X-User-Id', $this->provenance->userId);
if ($this->provenance->roles !== []) {
$req->setHeader('X-Roles', implode(',', $this->provenance->roles));
}
}
}Then replace each repeated Authorization-header block with $this->applyAuth($req).
Confirm what the remote API expects for provenance (custom headers vs JWT). If JWT is required, we should convert Provenance to a token instead of sending custom headers.
🏁 Script executed:
#!/bin/bash
set -e
echo "1. Checking property declaration in src/RemoteEntityClient.php"
rg "private string \$userToken" -n src/RemoteEntityClient.php
echo "2. Inspecting withAuth implementation"
rg "function withAuth" -n -A3 src/RemoteEntityClient.php
echo "3. Finding where userToken is applied to request headers"
rg "userToken" -n -C3 src/RemoteEntityClient.phpLength of output: 206
🏁 Script executed:
#!/bin/bash
set -e
# Locate the file
echo "1. Locating RemoteEntityClient.php"
fd --full-path "RemoteEntityClient.php" || true
# Show class definition context
echo "2. Class definition and property declarations"
rg -n -C3 "class RemoteEntityClient" src/RemoteEntityClient.php
# Search for userToken usage
echo "3. All occurrences of userToken"
rg -n "userToken" -C2 src/RemoteEntityClient.php
# Inspect withAuth implementation
echo "4. withAuth method"
rg -n -A5 "function withAuth" src/RemoteEntityClient.php
# Check where Authorization header is set
echo "5. Authorization header instances"
rg -n "Authorization" -C2 src/RemoteEntityClient.phpLength of output: 6409
Separate string tokens from Provenance and centralize header logic
The withAuth method currently allows a Provenance instance or null to be assigned to a property declared as string, causing a runtime TypeError. Moreover, each API call repeats the same authorization‐header logic.
Changes needed in src/RemoteEntityClient.php:
- Update property declarations
- Adjust
withAuthto split string tokens andProvenanceobjects - Add a private
applyAuth(Request $req)helper - Replace every inline
Authorizationheader block withapplyAuth($req)
Affected locations:
• Line 46: private string $userToken = '';
• Lines 150–153: withAuth(Provenance|string|null $token)
• Header blocks around lines 67–70, 123–126, 136–139, 159–162, 172–175, 184–187, 196–199, 208–211, 220–223 (all if ($this->userToken) { $req->setHeader('Authorization', …) })
Recommended diff:
--- a/src/RemoteEntityClient.php
+++ b/src/RemoteEntityClient.php
@@ class RemoteEntityClient implements EntityClientInterface
- private string $userToken = '';
+ private ?string $userToken = null;
+ private ?Provenance $provenance = null;
@@
- public function withAuth(Provenance|string|null $token): void
- {
- $this->userToken = $token;
- }
+ public function withAuth(Provenance|string|null $token): void
+ {
+ $this->userToken = is_string($token) ? $token : null;
+ $this->provenance = $token instanceof Provenance ? $token : null;
+ }
+
+ private function applyAuth(Request $req): void
+ {
+ if ($this->userToken !== null) {
+ $req->setHeader('Authorization', 'Bearer ' . $this->userToken);
+ } elseif ($this->provenance !== null) {
+ // adjust these headers to match the remote API’s expectations
+ $req->setHeader('X-User-Id', $this->provenance->userId);
+ if (!empty($this->provenance->roles)) {
+ $req->setHeader('X-Roles', implode(',', $this->provenance->roles));
+ }
+ }
+ }Then in each method, replace:
if ($this->userToken) {
$req->setHeader('Authorization', 'Bearer ' . $this->userToken);
}with:
$this->applyAuth($req);Please confirm what the remote API expects for non‐string provenance (custom headers vs. JWT) so we can finalize header names or encode Provenance as a token.
🤖 Prompt for AI Agents
In src/RemoteEntityClient.php around lines 46 and 150–153, the userToken
property and withAuth allow non-string Provenance values which leads to runtime
type errors and duplicated header logic; change the property to allow
Provenance|null|string (or add a separate private Provenance|null $provenance
and string $userToken), update withAuth to detect if the argument is a string
(store into $userToken) or a Provenance object (store into $provenance) or null,
add a private applyAuth(Request $req) method that sets the appropriate
Authorization or custom provenance headers based on whichever credential is set,
and replace every inline Authorization-if block at the listed locations (around
lines 67–70, 123–126, 136–139, 159–162, 172–175, 184–187, 196–199, 208–211,
220–223) with a call to $this->applyAuth($req); confirm what the remote API
expects for non-string provenance so applyAuth can set the correct header name
or encoding.
| public function withAuth(Provenance|string|null $token): void | ||
| { | ||
| $this->userToken = $token; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Same type mismatch as RemoteEntityClient::withAuth
$token can be Provenance|null, but $userToken is declared string and is used as a Bearer token. This will either fatally error or mis-send headers.
Apply the same pattern as suggested for RemoteEntityClient:
final class RemoteOrchestrationClient implements OrchestrationClientInterface
{
- private string $userToken = '';
+ private ?string $userToken = null;
+ private ?Provenance $provenance = null;
@@
- public function withAuth(Provenance|string|null $token): void
- {
- $this->userToken = $token;
- }
+ public function withAuth(Provenance|string|null $token): void
+ {
+ $this->userToken = \is_string($token) ? $token : null;
+ $this->provenance = $token instanceof Provenance ? $token : null;
+ }Add an applyAuth(Request $req): void helper (similar to the entity client) and replace all Authorization-setting blocks with a call to it. If the remote API supports provenance via custom headers, set them here; otherwise convert Provenance to a token before sending.
Please confirm the server’s expected format for provenance (custom headers vs token).
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "=== RemoteOrchestrationClient: Class & Properties ==="
rg -n "class RemoteOrchestrationClient" src/RemoteOrchestrationClient.php
rg -n "private .*userToken" src/RemoteOrchestrationClient.php
echo
echo "=== RemoteOrchestrationClient: withAuth Implementation ==="
rg -n "public function withAuth" -n src/RemoteOrchestrationClient.php
sed -n '180,240p' src/RemoteOrchestrationClient.php
echo
echo "=== RemoteEntityClient: withAuth for Comparison ==="
rg -n "class RemoteEntityClient" src/RemoteEntityClient.php
rg -n "public function withAuth" -n src/RemoteEntityClient.php
sed -n '100,160p' src/RemoteEntityClient.phpLength of output: 5615
Ensure type safety in withAuth: separate string tokens from Provenance
The userToken property is declared as string but withAuth accepts a Provenance object (or null), so assigning $token directly will throw a TypeError or silently fail. We need distinct properties for the bearer token and the provenance object, then centralize header application.
Affected locations:
src/RemoteOrchestrationClient.php- Line 49:
private string $userToken = ''; - Lines 199–202:
public function withAuth(Provenance|string|null $token): void { … }
- Line 49:
src/RemoteEntityClient.php- Similar property and
withAuthdefinition around lines 150–154
- Similar property and
Proposed changes:
final class RemoteOrchestrationClient implements OrchestrationClientInterface
{
- private string $userToken = '';
+ private ?string $userToken = null;
+ private ?Provenance $provenance = null;
@@
- public function withAuth(Provenance|string|null $token): void
- {
- $this->userToken = $token;
- }
+ public function withAuth(Provenance|string|null $token): void
+ {
+ $this->userToken = is_string($token) ? $token : null;
+ $this->provenance = $token instanceof Provenance ? $token : null;
+ }
+ private function applyAuth(Request $req): void
+ {
+ if ($this->userToken) {
+ $req->setHeader('Authorization', 'Bearer ' . $this->userToken);
+ }
+ if ($this->provenance) {
+ // e.g. $req->setHeader('X-Provenance', $this->provenance->toHeaderValue());
+ }
+ }
}Repeat the same pattern in RemoteEntityClient. Replace every raw header‐setting block (if ($this->userToken) { … }) with a call to applyAuth($req).
Please confirm the server’s expected format for provenance—should it be sent via custom headers (e.g. X-Provenance) or converted into a bearer‐style token before transmission?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function withAuth(Provenance|string|null $token): void | |
| { | |
| $this->userToken = $token; | |
| } | |
| final class RemoteOrchestrationClient implements OrchestrationClientInterface | |
| { | |
| private ?string $userToken = null; | |
| private ?Provenance $provenance = null; | |
| public function withAuth(Provenance|string|null $token): void | |
| { | |
| $this->userToken = is_string($token) ? $token : null; | |
| $this->provenance = $token instanceof Provenance ? $token : null; | |
| } | |
| private function applyAuth(Request $req): void | |
| { | |
| if ($this->userToken) { | |
| $req->setHeader('Authorization', 'Bearer ' . $this->userToken); | |
| } | |
| if ($this->provenance) { | |
| // e.g. $req->setHeader('X-Provenance', $this->provenance->toHeaderValue()); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/RemoteOrchestrationClient.php around lines 199-202 (and adjust
src/RemoteOrchestrationClient.php line 49 where userToken is declared), the
withAuth method currently accepts Provenance|string|null and assigns it to a
property declared as string causing a type mismatch; create two separate
properties (e.g. private string $userToken = '' and private ?Provenance
$provenance = null), change withAuth to detect and store either the bearer token
or the Provenance into the appropriate property (or provide two explicit
setters), add a private applyAuth(Request $req): Request method that applies
headers based on which property is set (set Authorization: Bearer <token> when
$userToken is present, or the agreed provenance header(s) when $provenance is
present), then replace every raw header-setting block (if ($this->userToken)
{...}) with calls to applyAuth($req); repeat the same refactor in
src/RemoteEntityClient.php around lines ~150-154, and verify with the server
whether provenance should be sent as a custom header (e.g. X-Provenance) or
converted to a bearer-style token before implementing the provenance header
format.
Signed-off-by: Robert Landers <[email protected]>
coderabbitai
bot
added
the
dependencies
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
src/LocalOrchestrationClient.php (1)
101-101: Use UUIDv7 to maintain time-ordered IDs (consistency with codebase)Project conventions prefer UUIDv7 for identifiers. Replace
Uuid::uuid4()withUuid::uuid7()here, as flagged previously.- $orchestrationId = \Bottledcode\DurablePhp\OrchestrationInstance($name, $id ?? Uuid::uuid4()->toString()); + $orchestrationId = \Bottledcode\DurablePhp\OrchestrationInstance($name, $id ?? Uuid::uuid7()->toString());src/LocalEntityClient.php (1)
69-73: Defensive checks for signal() reflection to avoid fatal errorsAccessing parameter 0 and calling getType()->getName() without checks will throw if the closure has no params or lacks a named type. Add robust guards and error messages.
- $interfaceReflector = new ReflectionFunction($signal); - $interfaceName = $interfaceReflector->getParameters()[0]->getType()?->getName(); - if (interface_exists($interfaceName) === false) { - throw new Exception("Interface {$interfaceName} does not exist"); - } + $rf = new ReflectionFunction($signal); + $params = $rf->getParameters(); + if (count($params) === 0) { + throw new Exception('Signal closure must accept a typed interface as the first parameter'); + } + $type = $params[0]->getType(); + $interfaceName = $type?->getName(); + if (!is_string($interfaceName) || !interface_exists($interfaceName)) { + throw new Exception("Interface {$interfaceName} does not exist"); + }cli/ext/build/ext.go (1)
301-309: Non-blocking send can drop local messages under loadFallback to error if the channel is full drops requests. Prefer blocking send or a context-aware send with timeout/backpressure.
- // Send the local message - select { - case globalWorkerInstance.localMessageChan <- localMsg: - // Wait for response - response := <-responseCh - return response, nil - default: - return nil, errors.New("local message channel is full") - } + // Send the local message (blocking) + globalWorkerInstance.localMessageChan <- localMsg + // Wait for response + response := <-responseCh + return response, nil
🧹 Nitpick comments (6)
cli/ext/build/ext.stub.php (2)
13-14: Optional: Narrow static return type to?selfUsing
?selfkeeps the signature resilient to class renames/refactors and mirrors common PHP patterns. No functional change.- public static function GetCurrent(): ?Worker {} + public static function GetCurrent(): ?self {}
7-7: PHPMD “unused parameter” hints can be ignored for stubsThis file is for arginfo generation; empty bodies with unused params are expected. Consider excluding stubs from PHPMD or adding a ruleset exception for this path.
src/LocalOrchestrationClient.php (2)
75-84: Inconsistent StateId helper usage: unify to a single methodThis file uses both
StateId::fromOrchestrationInstance(...)andStateId::fromInstance(...). For clarity and to reduce surface area, prefer one consistently (likelyfromInstancefor orchestrations).- $event = WithOrchestration::forInstance( - StateId::fromOrchestrationInstance($instance), + $event = WithOrchestration::forInstance( + StateId::fromInstance($instance), RaiseEvent::forSignal($eventName, SerializedArray::fromArray($eventData)), ); ... - emit_event($userArray, $eventDescription->toArray(), StateId::fromOrchestrationInstance($instance)->id); + emit_event($userArray, $eventDescription->toArray(), StateId::fromInstance($instance)->id);Please confirm both helpers exist or adjust accordingly.
50-52: Remove unused dependency to reduce noise
SpyProxy $spyProxyis injected but never used in this class. Drop the property (and import) until you actually need it.- public function __construct( - private SpyProxy $spyProxy = new SpyProxy(), - ) {} + public function __construct() {}cli/ext/build/ext.c (1)
30-52: Remove or guard verbose fprintf debug statementsThe extension is extremely noisy. Wrap debug prints in a compile-time guard (e.g., DEBUG_EXTENSION) or use zend logging. This reduces stderr spam in production.
- fprintf(stderr, "[C DEBUG] ext_create_object called!\n"); - fflush(stderr); + #ifdef DEBUG_EXTENSION + fprintf(stderr, "[C DEBUG] ext_create_object called!\n"); + fflush(stderr); + #endifApply similar guards to the other fprintf blocks in this file.
Also applies to: 77-111, 237-268, 281-297, 299-312
cli/ext/build/ext.go (1)
370-375: Reduce sensitive details in authorization failure logsLogging full
err.Error()or operation values on auth failures can leak sensitive info. Log minimal context.- helpers.Logger.Error("AUTHORIZATION FAILURE: Request blocked during resource discovery", - zap.String("error", err.Error()), + helpers.Logger.Error("AUTHORIZATION FAILURE: Request blocked during resource discovery", zap.String("destination", ev.Destination), zap.String("phase", "resource-discovery"), zap.String("action", "request-rejected")) ... - helpers.Logger.Error("AUTHORIZATION FAILURE: Operation not permitted", - zap.String("operation", string(operation)), + helpers.Logger.Error("AUTHORIZATION FAILURE: Operation not permitted", zap.String("destination", ev.Destination), zap.String("phase", "operation-check"), zap.String("action", "request-rejected"))Also applies to: 381-387
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (6)
cli/ext/build/ext.c(1 hunks)cli/ext/build/ext.go(1 hunks)cli/ext/build/ext.stub.php(1 hunks)cli/ext/build/ext_arginfo.h(1 hunks)src/LocalEntityClient.php(1 hunks)src/LocalOrchestrationClient.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/ext/build/ext_arginfo.h
🧰 Additional context used
🧬 Code Graph Analysis (5)
cli/ext/build/ext.stub.php (1)
cli/ext/build/ext.go (1)
Worker(760-770)
cli/ext/build/ext.c (3)
cli/ext/build/ext.stub.php (13)
__construct(11-11)__destruct(15-15)queryState(17-17)getUser(19-19)getSource(21-21)getCurrentId(23-23)getCorrelationId(25-25)getState(27-27)updateState(29-29)emitEvent(31-31)delete(33-33)GetCurrent(13-13)emit_event(7-7)src/Glue/Glue.php (2)
__construct(81-119)queryState(126-134)cli/ext/build/ext_arginfo.h (1)
register_class_Bottledcode_DurablePhp_Ext_Worker(80-88)
src/LocalEntityClient.php (14)
src/Events/EventDescription.php (1)
EventDescription(37-223)src/Events/WithDelay.php (1)
WithDelay(30-55)src/Events/WithEntity.php (1)
WithEntity(29-59)src/Glue/Provenance.php (1)
Provenance(30-33)src/Proxy/SpyException.php (1)
SpyException(5-5)src/Proxy/SpyProxy.php (1)
SpyProxy(31-126)src/Search/EntityFilter.php (1)
EntityFilter(27-30)src/State/Serializer.php (2)
Serializer(31-82)serialize(35-48)src/DurableClient.php (13)
Override(173-178)__construct(42-45)cleanEntityStorage(94-97)listEntities(99-102)signal(168-171)signalEntity(104-111)getEntitySnapshot(163-166)deleteEntity(180-183)shareEntityOwnership(185-188)grantEntityAccessToUser(190-193)grantEntityAccessToRole(195-198)revokeEntityAccessToUser(200-203)revokeEntityAccessToRole(205-208)cli/ext/build/ext.stub.php (2)
emit_event(7-7)__construct(11-11)src/EntityClientInterface.php (12)
cleanEntityStorage(43-43)listEntities(50-50)signal(70-70)signalEntity(55-60)getEntitySnapshot(79-79)withAuth(38-38)deleteEntity(84-84)shareEntityOwnership(86-86)grantEntityAccessToUser(88-88)grantEntityAccessToRole(90-90)revokeEntityAccessToUser(92-92)revokeEntityAccessToRole(94-94)src/State/Ids/StateId.php (1)
fromEntityId(69-72)src/Events/RaiseEvent.php (1)
forOperation(39-42)src/Events/With.php (1)
id(32-39)
cli/ext/build/ext.go (12)
cli/glue/glue.go (3)
DeleteState(225-255)StateArray(287-291)Array(257-259)cli/ids/id.go (4)
StateId(188-191)Subject(31-33)IdKind(9-9)Activity(12-12)cli/ext/helpers/helpers.go (8)
Ctx(59-59)Logger(54-54)Js(56-56)LogError(35-37)ParseStateId(61-70)Config(57-57)Consumer(48-52)GetUserContext(111-130)cli/glue/response_writer.go (10)
HeaderProvenance(26-26)HeaderDelay(21-21)HeaderStateId(20-20)HeaderEventType(22-22)HeaderEmittedBy(24-24)HeaderTargetType(23-23)HeaderEmittedAt(25-25)HeaderTargetOps(27-27)HeaderSourceOps(28-28)HeaderMeta(29-29)cli/auth/keys.go (1)
DecorateContextWithUser(30-36)cli/ext/build/ext.stub.php (1)
Worker(9-35)cli/lib/api.go (1)
GetCorrelationId(39-53)cli/lib/consumer.go (2)
ProcessMessage(144-360)StartConsumer(21-35)cli/auth/resource.go (1)
SendLocalMessage(25-25)cli/auth/user.go (2)
Operation(11-11)UserId(13-13)cli/config/config.go (1)
GetProjectConfig(74-92)cli/lib/billing.go (1)
StartBillingProcessor(22-173)
src/LocalOrchestrationClient.php (11)
src/Events/EventDescription.php (1)
EventDescription(37-223)src/Events/WithOrchestration.php (1)
WithOrchestration(30-59)src/Glue/Provenance.php (1)
Provenance(30-33)cli/ids/id.go (1)
StateId(188-191)src/State/Serializer.php (2)
Serializer(31-82)serialize(35-48)src/State/Status.php (1)
Status(32-45)src/DurableClient.php (2)
Override(173-178)__construct(42-45)cli/ext/build/ext.stub.php (2)
emit_event(7-7)__construct(11-11)src/SerializedArray.php (1)
SerializedArray(30-85)src/State/Ids/StateId.php (1)
fromInstance(59-62)src/Events/StartExecution.php (1)
asParent(48-54)
🪛 PHPMD (2.15.0)
cli/ext/build/ext.stub.php
7-7: Avoid unused parameters such as '$userContext'. (Unused Code Rules)
(UnusedFormalParameter)
7-7: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
7-7: Avoid unused parameters such as '$from'. (Unused Code Rules)
(UnusedFormalParameter)
17-17: Avoid unused parameters such as '$stateId'. (Unused Code Rules)
(UnusedFormalParameter)
29-29: Avoid unused parameters such as '$state'. (Unused Code Rules)
(UnusedFormalParameter)
31-31: Avoid unused parameters such as '$eventDescription'. (Unused Code Rules)
(UnusedFormalParameter)
src/LocalEntityClient.php
61-61: Avoid unused parameters such as '$filter'. (Unused Code Rules)
(UnusedFormalParameter)
61-61: Avoid unused parameters such as '$page'. (Unused Code Rules)
(UnusedFormalParameter)
117-117: Avoid unused parameters such as '$entityId'. (Unused Code Rules)
(UnusedFormalParameter)
134-134: Avoid unused parameters such as '$entityId'. (Unused Code Rules)
(UnusedFormalParameter)
139-139: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
139-139: Avoid unused parameters such as '$with'. (Unused Code Rules)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
149-149: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
149-149: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
149-149: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
154-154: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
154-154: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
159-159: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
159-159: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
src/LocalOrchestrationClient.php
61-61: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
67-67: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
87-87: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
93-93: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
93-93: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
122-122: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
122-122: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
128-128: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
128-128: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
134-134: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
150-150: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
150-150: Avoid unused parameters such as '$with'. (Unused Code Rules)
(UnusedFormalParameter)
155-155: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
155-155: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
155-155: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
160-160: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
165-165: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
165-165: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
170-170: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
170-170: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: Unit Tests - PHP
🔇 Additional comments (2)
src/LocalEntityClient.php (2)
122-126: withAuth handling is correct nowGood fix: you only accept Provenance for the local client and ignore strings, preventing TypeError when DurableClient forwards a string token.
110-114: Confirm “from” semantics for emit_eventYou’re setting Emitted-By to the destination entity’s StateId. If authorization expects the real emitter (e.g., an orchestration or caller state), this may bypass intent or fail auth. Verify design and adjust if a distinct source is required.
| static void ext_free_object(zend_object *object) { | ||
| ext_object *intern = ext_object_from_obj(object); | ||
|
|
||
| if (intern->go_handle != 0) { | ||
| removeGoObject(intern->go_handle); | ||
| } | ||
|
|
||
| zend_object_std_dtor(&intern->std); | ||
| } |
There was a problem hiding this comment.
Avoid stale handle: clear thread-local worker on free
If the freed object was using the thread-local handle, subsequent GetCurrent() calls would hand out an object with a deleted handle. Clear the TLS handle when freeing the object to prevent use-after-free.
static void ext_free_object(zend_object *object) {
ext_object *intern = ext_object_from_obj(object);
if (intern->go_handle != 0) {
removeGoObject(intern->go_handle);
+ /* Clear TLS handle if we owned it */
+ if (intern->go_handle == get_current_worker_handle()) {
+ clear_current_worker();
+ }
}
zend_object_std_dtor(&intern->std);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cli/ext/build/ext.c around lines 56 to 64, the free path removes the Go
handle but does not clear the thread-local worker pointer, risking GetCurrent()
returning a stale handle; after calling removeGoObject(intern->go_handle) check
whether the thread-local current handle equals intern->go_handle (via
GetCurrent() or the module's TLS accessor) and if so clear it (set TLS current
to 0/NULL) before finishing destruction so subsequent GetCurrent() calls cannot
return the freed handle.
| /* Create a new Worker object - PHP constructor will pick up the thread-local handle */ | ||
| zval obj; | ||
| if (object_init_ex(&obj, Worker_ce) != SUCCESS) { | ||
| fprintf(stderr, "[C DEBUG] object_init_ex failed\n"); | ||
| fflush(stderr); | ||
| RETURN_NULL(); | ||
| } | ||
|
|
||
| fprintf(stderr, "[C DEBUG] object_init_ex succeeded, returning object\n"); | ||
| fflush(stderr); | ||
|
|
||
| RETURN_ZVAL(&obj, 0, 0); | ||
| } |
There was a problem hiding this comment.
Constructed object isn’t bound to Go handle: __construct is never called
object_init_ex does not invoke the PHP constructor. As a result, go_handle remains 0 and any method call will throw “Go object not found.” Explicitly call __construct after object_init_ex.
/* Create a new Worker object - PHP constructor will pick up the thread-local handle */
zval obj;
if (object_init_ex(&obj, Worker_ce) != SUCCESS) {
fprintf(stderr, "[C DEBUG] object_init_ex failed\n");
fflush(stderr);
RETURN_NULL();
}
+ /* Explicitly invoke the PHP constructor to bind the go_handle */
+ zval ctor_ret;
+ ZVAL_UNDEF(&ctor_ret);
+ zend_call_method_with_0_params(Z_OBJ(obj), Worker_ce, NULL, "__construct", &ctor_ret);
+ zval_ptr_dtor(&ctor_ret);
+
fprintf(stderr, "[C DEBUG] object_init_ex succeeded, returning object\n");
fflush(stderr);
RETURN_ZVAL(&obj, 0, 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* Create a new Worker object - PHP constructor will pick up the thread-local handle */ | |
| zval obj; | |
| if (object_init_ex(&obj, Worker_ce) != SUCCESS) { | |
| fprintf(stderr, "[C DEBUG] object_init_ex failed\n"); | |
| fflush(stderr); | |
| RETURN_NULL(); | |
| } | |
| fprintf(stderr, "[C DEBUG] object_init_ex succeeded, returning object\n"); | |
| fflush(stderr); | |
| RETURN_ZVAL(&obj, 0, 0); | |
| } | |
| /* Create a new Worker object - PHP constructor will pick up the thread-local handle */ | |
| zval obj; | |
| if (object_init_ex(&obj, Worker_ce) != SUCCESS) { | |
| fprintf(stderr, "[C DEBUG] object_init_ex failed\n"); | |
| fflush(stderr); | |
| RETURN_NULL(); | |
| } | |
| /* Explicitly invoke the PHP constructor to bind the go_handle */ | |
| zval ctor_ret; | |
| ZVAL_UNDEF(&ctor_ret); | |
| zend_call_method_with_0_params( | |
| Z_OBJ(obj), Worker_ce, NULL, "__construct", &ctor_ret | |
| ); | |
| zval_ptr_dtor(&ctor_ret); | |
| fprintf(stderr, "[C DEBUG] object_init_ex succeeded, returning object\n"); | |
| fflush(stderr); | |
| RETURN_ZVAL(&obj, 0, 0); | |
| } |
🤖 Prompt for AI Agents
In cli/ext/build/ext.c around lines 256 to 268, the newly created PHP Worker
object is not having its __construct invoked so the go_handle remains zero;
after object_init_ex succeeds, explicitly invoke the object's __construct (use
the Zend API to call the method on the zval, check/clear any exceptions and
handle the return zval if needed) before returning the object so the Go handle
is initialized and subsequent method calls succeed.
| // Recursively handle delayed message by putting it back in channel | ||
| globalWorkerInstance.requestChan <- msg | ||
| return nil |
There was a problem hiding this comment.
Incorrect requeue of delayed message causes immediate reprocessing
After NakWithDelay, the server will redeliver after the delay. Pushing the same msg back onto your internal channel immediately defeats the delay and can loop. Remove the manual requeue.
- // Recursively handle delayed message by putting it back in channel
- globalWorkerInstance.requestChan <- msg
- return nil
+ // Server will redeliver after the delay; just return
+ return nil🤖 Prompt for AI Agents
In cli/ext/build/ext.go around lines 132 to 134, the code manually requeues the
same message onto globalWorkerInstance.requestChan after calling NakWithDelay,
which defeats the broker-side delay and can cause immediate reprocessing/loops;
remove the manual requeue so the function simply returns after NakWithDelay
(i.e., do not write msg back into requestChan), ensuring the server's delayed
redelivery is respected.
| worker.authContext, worker.activeId, worker.state, err = lib.ProcessMessage(ctx, logger, msg, rm, helpers.Config, js) | ||
| if err != nil { | ||
| helpers.LogError(err.Error()) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure worker TLS is cleared on early error paths
If ProcessMessage fails, clear the current worker to avoid leaving a stale handle bound to the thread.
worker.authContext, worker.activeId, worker.state, err = lib.ProcessMessage(ctx, logger, msg, rm, helpers.Config, js)
if err != nil {
helpers.LogError(err.Error())
+ ClearCurrentWorker()
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| worker.authContext, worker.activeId, worker.state, err = lib.ProcessMessage(ctx, logger, msg, rm, helpers.Config, js) | |
| if err != nil { | |
| helpers.LogError(err.Error()) | |
| return nil | |
| } | |
| worker.authContext, worker.activeId, worker.state, err = lib.ProcessMessage(ctx, logger, msg, rm, helpers.Config, js) | |
| if err != nil { | |
| helpers.LogError(err.Error()) | |
| ClearCurrentWorker() | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In cli/ext/build/ext.go around lines 174 to 178, when lib.ProcessMessage returns
an error the code currently logs and returns without clearing the thread-local
worker; call the helper that clears the current worker TLS (e.g.
helpers.ClearCurrentWorker() or the project's equivalent) immediately before
returning so the stale worker handle is removed from the thread; if no such
helper exists, set the TLS to nil via the appropriate API (for example
helpers.SetCurrentWorker(nil)) before the return.
| func (w *localResponseWriter) Write(data []byte) (int, error) { | ||
| // Accumulate response data in buffer | ||
| w.buffer = append(w.buffer, data...) | ||
|
|
||
| // Start a goroutine to wait for completion if not already done | ||
| if !w.sent { | ||
| w.sent = true | ||
| go w.waitForCompletion() | ||
| } | ||
|
|
||
| return len(data), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Race conditions in localResponseWriter (buffer/sent not synchronized)
Concurrent writes and the “first write starts goroutine” pattern are racy. Protect buffer/sent with a mutex and send response exactly once when done closes.
type localResponseWriter struct {
responseCh chan interface{}
doneCh chan struct{}
buffer []byte
- sent bool
+ sent bool
+ mu sync.Mutex
}
func (w *localResponseWriter) Write(data []byte) (int, error) {
- // Accumulate response data in buffer
- w.buffer = append(w.buffer, data...)
-
- // Start a goroutine to wait for completion if not already done
- if !w.sent {
- w.sent = true
- go w.waitForCompletion()
- }
+ w.mu.Lock()
+ w.buffer = append(w.buffer, data...)
+ if !w.sent {
+ w.sent = true
+ go w.waitForCompletion()
+ }
+ w.mu.Unlock()
return len(data), nil
}
func (w *localResponseWriter) waitForCompletion() {
// Wait for the request to complete
<-w.doneCh
// Send the complete response
- w.responseCh <- string(w.buffer)
+ w.mu.Lock()
+ resp := string(w.buffer)
+ w.mu.Unlock()
+ w.responseCh <- resp
close(w.responseCh)
}Also applies to: 267-274, 239-244
| public function queryState(string $stateId): array {} | ||
|
|
There was a problem hiding this comment.
Return type mismatch: queryState can return null
C implementation returns NULL when no state is found, but the stub declares a non-nullable array. Update to ?array to avoid runtime type violations and mismatched arginfo.
- public function queryState(string $stateId): array {}
+ public function queryState(string $stateId): ?array {}🧰 Tools
🪛 PHPMD (2.15.0)
17-17: Avoid unused parameters such as '$stateId'. (Unused Code Rules)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In cli/ext/build/ext.stub.php around lines 17-18, the queryState signature
declares a non-nullable array but the C implementation returns NULL when no
state is found; change the return type to ?array (nullable) in the stub and
update any related PHPDoc and arginfo/metadata to reflect the nullable return to
prevent runtime type violations and mismatched arginfo.
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
Signed-off-by: Robert Landers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
src/LocalEntityClient.php (1)
74-79: Harden signal() reflection: guard missing/untyped first param and union typesDirectly indexing param 0 and calling getName() can emit notices or fatal when the closure has no params or uses union/nullable types. Perform defensive checks and only accept named interface types.
- $interfaceReflector = new ReflectionFunction($signal); - $interfaceName = $interfaceReflector->getParameters()[0]?->getType()?->getName(); - if ($interfaceName === null || interface_exists($interfaceName) === false) { - throw new Exception("Interface {$interfaceName} does not exist"); - } + $rf = new ReflectionFunction($signal); + $params = $rf->getParameters(); + if (count($params) === 0) { + throw new Exception('Signal closure must accept a typed interface as the first parameter'); + } + $type = $params[0]->getType(); + if ($type === null || !($type instanceof \ReflectionNamedType)) { + throw new Exception('Signal closure must accept a named interface type as the first parameter'); + } + $interfaceName = $type->getName(); + if (!is_string($interfaceName) || interface_exists($interfaceName) === false) { + throw new Exception("Interface {$interfaceName} does not exist"); + }cli/ext/build/ext.go (5)
132-135: Don’t manually requeue after NakWithDelay; it defeats broker-side delayReinserting the message immediately bypasses the intended delay and can cause immediate reprocessing/loops. Let the server redeliver.
- // Recursively handle delayed message by putting it back in channel - globalWorkerInstance.requestChan <- msg - return nil + // Server will redeliver after the delay + return nil
174-178: Clear current worker on early error to avoid stale TLS handleIf ProcessMessage fails, the current worker remains bound to the thread. Clear it before returning.
worker.authContext, worker.activeId, worker.state, err = lib.ProcessMessage(ctx, logger, msg, rm, helpers.Config, js) if err != nil { helpers.LogError(err.Error()) + ClearCurrentWorker() return nil }Add the missing ClearCurrentWorker and handle tracking (outside this hunk):
// At package scope (near other globals) var currentWorkerHandle C.uintptr_t func ClearCurrentWorker() { C.clear_current_worker() if currentWorkerHandle != 0 { removeGoObject(currentWorkerHandle) currentWorkerHandle = 0 } }
238-244: Fix race conditions in localResponseWriter (buffer/sent not synchronized)Concurrent writes and the “start goroutine on first write” pattern are racy. Protect buffer/sent with a mutex and read the buffer under lock when done fires.
type localResponseWriter struct { responseCh chan interface{} doneCh chan struct{} buffer []byte - sent bool + sent bool + mu sync.Mutex } func (w *localResponseWriter) Header() http.Header { return make(http.Header) } func (w *localResponseWriter) Write(data []byte) (int, error) { - // Accumulate response data in buffer - w.buffer = append(w.buffer, data...) - - // Start a goroutine to wait for completion if not already done - if !w.sent { - w.sent = true - go w.waitForCompletion() - } + w.mu.Lock() + w.buffer = append(w.buffer, data...) + if !w.sent { + w.sent = true + go w.waitForCompletion() + } + w.mu.Unlock() return len(data), nil } func (w *localResponseWriter) waitForCompletion() { // Wait for the request to complete <-w.doneCh // Send the complete response - w.responseCh <- string(w.buffer) + w.mu.Lock() + resp := string(w.buffer) + w.mu.Unlock() + w.responseCh <- resp close(w.responseCh) }Also applies to: 250-274
301-309: Avoid dropping local messages with non-blocking sendThe default branch drops requests when the channel is full. Use a blocking send or backpressure/timeout strategy.
- // Send the local message - select { - case globalWorkerInstance.localMessageChan <- localMsg: - // Wait for response - response := <-responseCh - return response, nil - default: - return nil, errors.New("local message channel is full") - } + // Send the local message (blocking) + globalWorkerInstance.localMessageChan <- localMsg + // Wait for response + response := <-responseCh + return response, nil
950-959: Prevent cgo handle leaks in SetCurrentWorkerEach SetCurrentWorker registers a new handle without releasing the previous one. Track and free the previous handle; also clear it in ClearCurrentWorker.
-func SetCurrentWorker(worker *Worker) { - if worker == nil { - C.clear_current_worker() - return - } - - handle := registerGoObject(worker) - C.set_current_worker_handle(C.uintptr_t(handle)) -} +func SetCurrentWorker(worker *Worker) { + if worker == nil { + ClearCurrentWorker() + return + } + // Release any previous handle + if currentWorkerHandle != 0 { + removeGoObject(currentWorkerHandle) + currentWorkerHandle = 0 + } + handle := registerGoObject(worker) + currentWorkerHandle = handle + C.set_current_worker_handle(C.uintptr_t(handle)) +}Add (outside this hunk) the global and ClearCurrentWorker() if not already present:
var currentWorkerHandle C.uintptr_t func ClearCurrentWorker() { C.clear_current_worker() if currentWorkerHandle != 0 { removeGoObject(currentWorkerHandle) currentWorkerHandle = 0 } }cli/ext/build/ext.stub.php (1)
17-18: Return type mismatch: queryState can return nullThe C/Go layer returns NULL when no state is found. Change return type to ?array and regenerate arginfo to prevent runtime type violations.
- public function queryState(string $stateId): array {} + public function queryState(string $stateId): ?array {}src/LocalOrchestrationClient.php (3)
100-105: Use UUIDv7 for time-ordered IDs (consistency with rest of codebase)uuid4 breaks ordering and is inconsistent. Switch to uuid7.
- $orchestrationId = \Bottledcode\DurablePhp\OrchestrationInstance($name, $id ?? Uuid::uuid4()->toString()); + $orchestrationId = \Bottledcode\DurablePhp\OrchestrationInstance($name, $id ?? Uuid::uuid7()->toString());
114-119: Bug: passing ack sequence as “from” breaks emit_event semanticsemit_event’s third arg is a StateId string. Using the JetStream ack sequence causes ids.ParseStateId to fail in Go. Pass the same state id to both calls and drop the unused $sequence.
- $sequence = emit_event($userArray, $eventDescription->toArray(), $stateId->id); + emit_event($userArray, $eventDescription->toArray(), $stateId->id); $event = WithOrchestration::forInstance($stateId, StartExecution::asParent($args, [])); $eventDescription = new EventDescription($event); - emit_event($userArray, $eventDescription->toArray(), $sequence); + emit_event($userArray, $eventDescription->toArray(), $stateId->id);
141-145: withAuth must serialize Provenance before passing to Worker::setUserWorker::setUser expects a serialized array; passing the object will fail in the Go bridge. Mirror LocalEntityClient’s approach.
- $this->worker->setUser($token instanceof Provenance ? $token : null); + $this->worker->setUser($token instanceof Provenance ? Serializer::serialize($token) : null);
🧹 Nitpick comments (4)
src/LocalEntityClient.php (1)
92-97: Disambiguate EntityId factory call to avoid confusion with the EntityId classCalling EntityId(...) while also importing the EntityId class is ambiguous to readers. Prefer a fully-qualified function call (or import the function explicitly) to signal intent.
- is_string($entityId) ? EntityId($interfaceName, $entityId) : $entityId, + is_string($entityId) ? \Bottledcode\DurablePhp\EntityId($interfaceName, $entityId) : $entityId,cli/ext/build/ext_arginfo.h (1)
25-27: Nit: parameter name “providence” is likely a typo for “provenance”Consider renaming in the stub, then regenerate arginfo for clarity and consistency across the codebase.
cli/ext/build/ext.stub.php (1)
21-21: Nit: fix parameter name typo “providence” → “provenance”For clarity and alignment with domain terminology, rename the parameter; then regenerate arginfo.
- public function setUser(?array $providence): void {} + public function setUser(?array $provenance): void {}src/LocalOrchestrationClient.php (1)
51-55: Unused dependency: $spyProxy is not referencedIf no methods require SpyProxy here, remove it to keep the class minimal. Otherwise, add usage or a TODO indicating intended use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (7)
cli/ext/build/ext.c(1 hunks)cli/ext/build/ext.go(1 hunks)cli/ext/build/ext.stub.php(1 hunks)cli/ext/build/ext_arginfo.h(1 hunks)src/DurableClient.php(2 hunks)src/LocalEntityClient.php(1 hunks)src/LocalOrchestrationClient.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/DurableClient.php
- cli/ext/build/ext.c
🧰 Additional context used
🧬 Code Graph Analysis (3)
cli/ext/build/ext_arginfo.h (2)
cli/ext/build/ext.stub.php (14)
Worker(9-36)__construct(11-11)GetCurrent(13-13)__destruct(15-15)queryState(17-17)getUser(19-19)setUser(21-21)getSource(23-23)getCurrentId(25-25)getCorrelationId(27-27)getState(29-29)updateState(31-31)emitEvent(33-33)delete(35-35)cli/ext/build/ext.c (1)
Bottledcode_DurablePhp_Ext_emit_event(347-359)
cli/ext/build/ext.go (14)
cli/glue/glue.go (4)
DeleteState(225-255)StateArray(287-291)Array(257-259)GetStateArray(293-342)cli/ids/id.go (4)
StateId(188-191)Subject(31-33)IdKind(9-9)Activity(12-12)cli/ext/helpers/helpers.go (6)
Ctx(59-59)Logger(54-54)Js(56-56)LogError(35-37)ParseStateId(61-70)Config(57-57)cli/glue/response_writer.go (4)
HeaderProvenance(26-26)HeaderDelay(21-21)HeaderStateId(20-20)HeaderEventType(22-22)cli/auth/keys.go (1)
DecorateContextWithUser(30-36)cli/ext/build/ext.stub.php (1)
Worker(9-36)cli/lib/api.go (1)
GetCorrelationId(39-53)cli/auth/resourceManager.go (1)
GetResourceManager(29-46)cli/lib/consumer.go (1)
ProcessMessage(144-360)cli/auth/resource.go (1)
SendLocalMessage(25-25)cli/config/config.go (1)
GetProjectConfig(74-92)cli/lib/indexer.go (1)
IndexerListen(19-262)cli/lib/billing.go (1)
StartBillingProcessor(22-173)cli/auth/user.go (1)
UserId(13-13)
cli/ext/build/ext.stub.php (4)
cli/ext/build/ext.go (1)
Worker(729-739)src/LocalEntityClient.php (1)
__construct(57-60)src/DurableClient.php (1)
__construct(44-47)src/LocalOrchestrationClient.php (1)
__construct(51-54)
🪛 PHPMD (2.15.0)
src/LocalOrchestrationClient.php
63-63: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
89-89: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
95-95: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
95-95: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
124-124: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
124-124: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
130-130: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
130-130: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
136-136: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
147-147: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
147-147: Avoid unused parameters such as '$with'. (Unused Code Rules)
(UnusedFormalParameter)
152-152: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
152-152: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
152-152: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
157-157: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
157-157: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
157-157: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
162-162: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
162-162: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
167-167: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
167-167: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
cli/ext/build/ext.stub.php
7-7: Avoid unused parameters such as '$userContext'. (Unused Code Rules)
(UnusedFormalParameter)
7-7: Avoid unused parameters such as '$event'. (Unused Code Rules)
(UnusedFormalParameter)
7-7: Avoid unused parameters such as '$from'. (Unused Code Rules)
(UnusedFormalParameter)
17-17: Avoid unused parameters such as '$stateId'. (Unused Code Rules)
(UnusedFormalParameter)
21-21: Avoid unused parameters such as '$providence'. (Unused Code Rules)
(UnusedFormalParameter)
31-31: Avoid unused parameters such as '$state'. (Unused Code Rules)
(UnusedFormalParameter)
33-33: Avoid unused parameters such as '$eventDescription'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (1)
src/LocalEntityClient.php (1)
129-133: withAuth handling looks correctYou correctly ignore string tokens and only serialize a Provenance instance before forwarding to Worker::setUser. This avoids the previous TypeError path.
| ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Bottledcode_DurablePhp_Ext_Worker_queryState, 0, 1, IS_ARRAY, 0) | ||
| ZEND_ARG_TYPE_INFO(0, stateId, IS_STRING, 0) | ||
| ZEND_END_ARG_INFO() | ||
|
|
||
| ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Bottledcode_DurablePhp_Ext_Worker_getUser, 0, 0, IS_ARRAY, 1) | ||
| ZEND_END_ARG_INFO() |
There was a problem hiding this comment.
Make queryState return type nullable to match implementation (returns NULL on miss)
The Go/C layer returns NULL when no state is found, but arginfo declares a non-nullable array. This leads to runtime type violations. Update the stub to ?array and regenerate this header.
Regenerated diff (generated header will reflect this change):
-ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Bottledcode_DurablePhp_Ext_Worker_queryState, 0, 1, IS_ARRAY, 0)
+ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Bottledcode_DurablePhp_Ext_Worker_queryState, 0, 1, IS_ARRAY, 1)
ZEND_ARG_TYPE_INFO(0, stateId, IS_STRING, 0)
ZEND_END_ARG_INFO()Note: edit cli/ext/build/ext.stub.php first (see separate comment) and then regenerate this file.
Also applies to: 36-37
🤖 Prompt for AI Agents
In cli/ext/build/ext_arginfo.h around lines 18 to 23 (and similarly at lines
36-37), the arginfo for Bottledcode\\DurablePhp\\Ext\\Worker::queryState
declares a non-nullable array return but the implementation can return NULL on a
miss; update the stub to make the return type nullable (?array) in
cli/ext/build/ext.stub.php, then regenerate ext_arginfo.h so the generated
header reflects the nullable return type and avoids runtime type violations.
| func (w *Worker) __destruct() { | ||
| w.consumer.Msg.Stop() | ||
| w.consumer.Done() | ||
| } |
There was a problem hiding this comment.
Guard __destruct against nil consumer to prevent panics
consumer is never set in this flow; calling methods on nil will panic.
func (w *Worker) __destruct() {
- w.consumer.Msg.Stop()
- w.consumer.Done()
+ if w.consumer != nil {
+ w.consumer.Msg.Stop()
+ w.consumer.Done()
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (w *Worker) __destruct() { | |
| w.consumer.Msg.Stop() | |
| w.consumer.Done() | |
| } | |
| func (w *Worker) __destruct() { | |
| if w.consumer != nil { | |
| w.consumer.Msg.Stop() | |
| w.consumer.Done() | |
| } | |
| } |
🤖 Prompt for AI Agents
In cli/ext/build/ext.go around lines 765 to 768, the __destruct method calls
methods on w.consumer without checking for nil, which can panic when consumer is
unset; update the method to first test whether w.consumer is nil and only call
w.consumer.Msg.Stop() and w.consumer.Done() when w.consumer is non-nil (also
consider guarding w.consumer.Msg if Msg can be nil) so the destructor safely
no-ops when consumer is absent.
| $state = $this->worker->queryState(StateId::fromEntityId($entityId)); | ||
| if (empty($state)) { | ||
| return null; | ||
| } | ||
|
|
||
| return Serializer::deserialize($state, EntityState::class); | ||
| } |
There was a problem hiding this comment.
Fix Worker::queryState argument type: pass string ID, not a StateId object
Worker::queryState is declared as queryState(string $stateId). Passing a StateId object will throw a TypeError. Also handle the potential null/empty case defensively.
- $state = $this->worker->queryState(StateId::fromEntityId($entityId));
+ $stateId = StateId::fromEntityId($entityId)->id;
+ $state = $this->worker->queryState($stateId);
if (empty($state)) {
return null;
}
return Serializer::deserialize($state, EntityState::class);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $state = $this->worker->queryState(StateId::fromEntityId($entityId)); | |
| if (empty($state)) { | |
| return null; | |
| } | |
| return Serializer::deserialize($state, EntityState::class); | |
| } | |
| $stateId = StateId::fromEntityId($entityId)->id; | |
| $state = $this->worker->queryState($stateId); | |
| if (empty($state)) { | |
| return null; | |
| } | |
| return Serializer::deserialize($state, EntityState::class); | |
| } |
🤖 Prompt for AI Agents
In src/LocalEntityClient.php around lines 121 to 127, the call to
Worker::queryState uses a StateId object but queryState(string $stateId) expects
a string; change the call to pass the string form of the state id (e.g. convert
StateId::fromEntityId($entityId) to its string representation) and then
defensively check the result for null/empty before attempting to deserialize; if
the result is empty return null, otherwise call Serializer::deserialize and
return the EntityState.
| $result = $this->worker->queryState(StateId::fromInstance($instance)); | ||
|
|
||
| return Serializer::deserialize($result, Status::class); | ||
| } |
There was a problem hiding this comment.
Fix Worker::queryState argument type and handle empty result
queryState expects a string state ID. Passing a StateId object will TypeError. Also, if no state is found, consider handling that case explicitly before deserializing.
- $result = $this->worker->queryState(StateId::fromInstance($instance));
-
- return Serializer::deserialize($result, Status::class);
+ $stateId = StateId::fromInstance($instance)->id;
+ $result = $this->worker->queryState($stateId);
+ if (empty($result)) {
+ throw new Exception("No orchestration state found for {$stateId}");
+ }
+ return Serializer::deserialize($result, Status::class);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $result = $this->worker->queryState(StateId::fromInstance($instance)); | |
| return Serializer::deserialize($result, Status::class); | |
| } | |
| // extract the raw state ID string | |
| $stateId = StateId::fromInstance($instance)->id; | |
| // query by string ID, not by object | |
| $result = $this->worker->queryState($stateId); | |
| // handle case where no state was returned | |
| if (empty($result)) { | |
| throw new Exception("No orchestration state found for {$stateId}"); | |
| } | |
| return Serializer::deserialize($result, Status::class); | |
| } |
🤖 Prompt for AI Agents
In src/LocalOrchestrationClient.php around lines 71-74, queryState is being
passed a StateId object which causes a TypeError and the code does not handle an
empty result; convert the StateId to the expected string (e.g.
StateId::fromInstance($instance)->toString() or
(string)StateId::fromInstance($instance)) when calling
$this->worker->queryState, then check if $result is null/empty before calling
Serializer::deserialize and return an appropriate value (e.g. null or a default)
when no state is found.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
src/LocalOrchestrationClient.php (2)
69-74: Fix: Pass string StateId to Worker::queryState and handle empty results
Worker::queryStateexpects a string, not aStateIdobject, and the result may be empty. This currently risks a TypeError and an unhandled empty state.Apply this diff:
- $result = $this->worker->queryState(StateId::fromInstance($instance)); - - return Serializer::deserialize($result, Status::class); + $stateId = StateId::fromInstance($instance)->id; + $result = $this->worker->queryState($stateId); + if (empty($result)) { + throw new Exception("No orchestration state found for {$stateId}"); + } + return Serializer::deserialize($result, Status::class);
100-121: Three fixes: use UUIDv7, stop using ack sequence as “from”, and drop unused $sequence
- Use UUIDv7 (time-ordered) to align with the rest of the codebase.
- The third arg to
emit_eventis a StateId string, not the JetStream ack sequence. Passing the ack “sequence” breaksids.ParseStateIddownstream.- Remove the unused
$sequence.public function startNew(string $name, array $args = [], ?string $id = null): OrchestrationInstance { - $orchestrationId = \Bottledcode\DurablePhp\OrchestrationInstance($name, $id ?? Uuid::uuid4()->toString()); + $orchestrationId = \Bottledcode\DurablePhp\OrchestrationInstance($name, $id ?? Uuid::uuid7()->toString()); $stateId = StateId::fromInstance($orchestrationId); $event = WithOrchestration::forInstance( $stateId, StartOrchestration::forInstance($orchestrationId), ); $eventDescription = new EventDescription($event); $userArray = $this->userContext ? Serializer::serialize($this->userContext) : null; - $sequence = emit_event($userArray, $eventDescription->toArray(), $stateId->id); + emit_event($userArray, $eventDescription->toArray(), $stateId->id); $event = WithOrchestration::forInstance($stateId, StartExecution::asParent($args, [])); $eventDescription = new EventDescription($event); - emit_event($userArray, $eventDescription->toArray(), $sequence); + emit_event($userArray, $eventDescription->toArray(), $stateId->id); return $orchestrationId; }cli/ext/build/ext.go (7)
117-135: Do not requeue after NakWithDelay; broker will redeliverManually pushing the message onto
requestChanafterNakWithDelaydefeats the delay and risks tight loops.if err := msg.NakWithDelay(delay); err != nil { helpers.LogError(err.Error()) return nil } - // Recursively handle delayed message by putting it back in channel - globalWorkerInstance.requestChan <- msg return nil
174-178: Clear current worker on error to avoid stale TLS and handle leaksOn
lib.ProcessMessagefailure, clear the current worker binding before returning.worker.authContext, worker.activeId, worker.state, err = lib.ProcessMessage(ctx, logger, msg, rm, helpers.Config, js) if err != nil { helpers.LogError(err.Error()) + ClearCurrentWorker() return nil }Additionally, see the SetCurrentWorker/handle-leak fix below.
229-236: Make localResponseWriter goroutine-safe and always send on completion
- Protect buffer with a mutex.
- Start completion watcher at creation time so empty responses still complete.
- Remove the racy “start goroutine on first write” pattern.
responseWriter := &localResponseWriter{ responseCh: localMsg.ResponseCh, doneCh: doneCh, buffer: make([]byte, 0), } +go responseWriter.waitForCompletion()type localResponseWriter struct { responseCh chan interface{} doneCh chan struct{} buffer []byte - sent bool + mu sync.Mutex }func (w *localResponseWriter) Write(data []byte) (int, error) { - // Accumulate response data in buffer - w.buffer = append(w.buffer, data...) - - // Start a goroutine to wait for completion if not already done - if !w.sent { - w.sent = true - go w.waitForCompletion() - } + w.mu.Lock() + w.buffer = append(w.buffer, data...) + w.mu.Unlock() return len(data), nil }func (w *localResponseWriter) waitForCompletion() { // Wait for the request to complete <-w.doneCh // Send the complete response - w.responseCh <- string(w.buffer) + w.mu.Lock() + resp := string(w.buffer) + w.mu.Unlock() + w.responseCh <- resp close(w.responseCh) }Also applies to: 238-244, 250-261, 267-274
301-309: Avoid dropping local messages; use a blocking sendThe non-blocking send drops requests when the channel is full. Prefer a blocking send or a context-aware timeout.
- // Send the local message - select { - case globalWorkerInstance.localMessageChan <- localMsg: - // Wait for response - response := <-responseCh - return response, nil - default: - return nil, errors.New("local message channel is full") - } + // Send the local message (blocking) + globalWorkerInstance.localMessageChan <- localMsg + // Wait for response + response := <-responseCh + return response, nil
775-778: Guard __destruct against nil consumer to prevent panics
w.consumermay be nil in this flow.func (w *Worker) __destruct() { - w.consumer.Msg.Stop() - w.consumer.Done() + if w.consumer != nil { + w.consumer.Msg.Stop() + w.consumer.Done() + } }
729-731: Track currentWorkerHandle to prevent cgo handle leaksRegistering a new handle each time without releasing the old one leaks memory. Introduce a package-level tracker.
type Worker struct { + // tracked via currentWorkerHandle kind ids.IdKind started boolAdd this to the package scope (near other globals):
var currentWorkerHandle C.uintptr_t
960-969: Fix SetCurrentWorker to release previous handle; add ClearCurrentWorkerAvoid leaking cgo handles and clear TLS properly.
-func SetCurrentWorker(worker *Worker) { - if worker == nil { - C.clear_current_worker() - return - } - - handle := registerGoObject(worker) - C.set_current_worker_handle(C.uintptr_t(handle)) -} +func SetCurrentWorker(worker *Worker) { + if worker == nil { + ClearCurrentWorker() + return + } + // Release any existing handle + if currentWorkerHandle != 0 { + removeGoObject(currentWorkerHandle) + currentWorkerHandle = 0 + } + handle := registerGoObject(worker) + currentWorkerHandle = handle + C.set_current_worker_handle(C.uintptr_t(handle)) +} + +func ClearCurrentWorker() { + C.clear_current_worker() + if currentWorkerHandle != 0 { + removeGoObject(currentWorkerHandle) + currentWorkerHandle = 0 + } +}
🧹 Nitpick comments (2)
src/LocalOrchestrationClient.php (2)
47-48: Silence PHPMD unused parameter noise for unimplemented methodsA number of methods intentionally ignore parameters and throw. Add a class-level suppression to avoid repeated PHPMD warnings.
- final class LocalOrchestrationClient implements OrchestrationClientInterface + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + final class LocalOrchestrationClient implements OrchestrationClientInterface
51-54: Unused dependency: SpyProxy is injected but never usedEither remove it from the constructor or wire it in where intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
cli/ext/build/ext.go(1 hunks)src/DurableClient.php(2 hunks)src/LocalOrchestrationClient.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/DurableClient.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/LocalOrchestrationClient.php (12)
src/Events/EventDescription.php (1)
EventDescription(37-223)src/Events/WithOrchestration.php (1)
WithOrchestration(30-59)cli/ext/build/ext.stub.php (5)
Worker(9-36)__construct(11-11)queryState(17-17)emitEvent(33-33)setUser(21-21)cli/ext/build/ext.go (1)
Worker(729-739)src/Glue/Provenance.php (1)
Provenance(30-33)src/State/Serializer.php (3)
Serializer(31-82)deserialize(61-76)serialize(35-48)src/State/Status.php (1)
Status(32-45)src/OrchestrationClientInterface.php (3)
getStatus(37-37)raiseEvent(43-43)withAuth(35-35)src/State/Ids/StateId.php (1)
fromInstance(59-62)src/Events/RaiseEvent.php (1)
forOperation(39-42)src/Events/With.php (1)
id(32-39)src/Events/StartExecution.php (1)
asParent(48-54)
cli/ext/build/ext.go (13)
cli/glue/glue.go (2)
DeleteState(225-255)Array(257-259)cli/ids/id.go (4)
StateId(188-191)Subject(31-33)IdKind(9-9)Activity(12-12)cli/ext/helpers/helpers.go (7)
Ctx(59-59)Logger(54-54)LogError(35-37)ParseStateId(61-70)Config(57-57)GetUserContext(111-130)ParseEvent(72-109)cli/glue/response_writer.go (10)
HeaderProvenance(26-26)HeaderDelay(21-21)HeaderStateId(20-20)HeaderEventType(22-22)HeaderEmittedBy(24-24)HeaderTargetType(23-23)HeaderEmittedAt(25-25)HeaderTargetOps(27-27)HeaderSourceOps(28-28)HeaderMeta(29-29)cli/auth/keys.go (1)
DecorateContextWithUser(30-36)cli/ext/build/ext.stub.php (1)
Worker(9-36)cli/lib/api.go (1)
GetCorrelationId(39-53)cli/auth/resourceManager.go (1)
GetResourceManager(29-46)cli/auth/resource.go (1)
SendLocalMessage(25-25)cli/config/config.go (1)
GetProjectConfig(74-92)cli/lib/indexer.go (1)
IndexerListen(19-262)cli/lib/billing.go (1)
StartBillingProcessor(22-173)cli/auth/user.go (1)
UserId(13-13)
🪛 PHPMD (2.15.0)
src/LocalOrchestrationClient.php
63-63: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
89-89: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
95-95: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
95-95: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
124-124: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
124-124: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
130-130: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
130-130: Avoid unused parameters such as '$reason'. (Unused Code Rules)
(UnusedFormalParameter)
136-136: Avoid unused parameters such as '$instance'. (Unused Code Rules)
(UnusedFormalParameter)
147-147: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
147-147: Avoid unused parameters such as '$with'. (Unused Code Rules)
(UnusedFormalParameter)
152-152: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
152-152: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
152-152: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
157-157: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
157-157: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
157-157: Avoid unused parameters such as '$operation'. (Unused Code Rules)
(UnusedFormalParameter)
162-162: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
162-162: Avoid unused parameters such as '$user'. (Unused Code Rules)
(UnusedFormalParameter)
167-167: Avoid unused parameters such as '$id'. (Unused Code Rules)
(UnusedFormalParameter)
167-167: Avoid unused parameters such as '$role'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (1)
cli/ext/build/ext.go (1)
792-806: Verify PHP array schema for getUser: “user” vs “userId”
getUser()returns array key “user”, but the PHP Provenance expects “userId”. Confirm alignment with the PHP consumer; otherwise, rename the key to “userId”.Would you like me to patch
getUser()to return “userId” for consistency?
| currentUser := &auth.User{} | ||
| b := msg.Headers().Get(string(glue.HeaderProvenance)) | ||
| err := json.Unmarshal([]byte(b), currentUser) | ||
| if err != nil { | ||
| logger.Warn("Failed to unmarshal event provenance", | ||
| zap.Any("Provenance", msg.Headers().Get(string(glue.HeaderProvenance))), | ||
| zap.Error(err), | ||
| ) | ||
| currentUser = nil | ||
| } else { | ||
| ctx = auth.DecorateContextWithUser(ctx, currentUser) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard provenance unmarshal when header is absent to avoid noisy warnings
When the Provenance header is missing/empty, json.Unmarshal on an empty string fails and logs a warning. Short-circuit when header is empty.
- currentUser := &auth.User{}
- b := msg.Headers().Get(string(glue.HeaderProvenance))
- err := json.Unmarshal([]byte(b), currentUser)
- if err != nil {
- logger.Warn("Failed to unmarshal event provenance",
- zap.Any("Provenance", msg.Headers().Get(string(glue.HeaderProvenance))),
- zap.Error(err),
- )
- currentUser = nil
- } else {
- ctx = auth.DecorateContextWithUser(ctx, currentUser)
- }
+ b := msg.Headers().Get(string(glue.HeaderProvenance))
+ if b != "" {
+ currentUser := &auth.User{}
+ if err := json.Unmarshal([]byte(b), currentUser); err != nil {
+ logger.Warn("Failed to unmarshal event provenance",
+ zap.Any("Provenance", b),
+ zap.Error(err),
+ )
+ } else {
+ ctx = auth.DecorateContextWithUser(ctx, currentUser)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| currentUser := &auth.User{} | |
| b := msg.Headers().Get(string(glue.HeaderProvenance)) | |
| err := json.Unmarshal([]byte(b), currentUser) | |
| if err != nil { | |
| logger.Warn("Failed to unmarshal event provenance", | |
| zap.Any("Provenance", msg.Headers().Get(string(glue.HeaderProvenance))), | |
| zap.Error(err), | |
| ) | |
| currentUser = nil | |
| } else { | |
| ctx = auth.DecorateContextWithUser(ctx, currentUser) | |
| } | |
| // Guard against missing provenance header to avoid noisy warnings. | |
| b := msg.Headers().Get(string(glue.HeaderProvenance)) | |
| if b != "" { | |
| currentUser := &auth.User{} | |
| if err := json.Unmarshal([]byte(b), currentUser); err != nil { | |
| logger.Warn("Failed to unmarshal event provenance", | |
| zap.Any("Provenance", b), | |
| zap.Error(err), | |
| ) | |
| } else { | |
| ctx = auth.DecorateContextWithUser(ctx, currentUser) | |
| } | |
| } |
🤖 Prompt for AI Agents
In cli/ext/build/ext.go around lines 104 to 116, the code calls json.Unmarshal
on the Provenance header even when it's empty, causing noisy warnings; update
the logic to read the header value into b and short-circuit when b is empty (or
only whitespace) so you only call json.Unmarshal when there is actual content,
and retain the existing warning/logging and ctx decoration behavior for the
non-empty case and error case.
| // Handle delete messages | ||
| if strings.HasSuffix(msg.Subject(), ".delete") { | ||
| id := ids.ParseStateId(msg.Headers().Get(string(glue.HeaderStateId))) | ||
| err := glue.DeleteState(ctx, js, logger, id) | ||
| if err != nil { | ||
| helpers.LogError(err.Error()) | ||
| return nil | ||
| } | ||
| // Return nil to get next message | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Ack delete messages to prevent redelivery loops
After deleting state, ack the message, otherwise it will be redelivered.
if strings.HasSuffix(msg.Subject(), ".delete") {
id := ids.ParseStateId(msg.Headers().Get(string(glue.HeaderStateId)))
err := glue.DeleteState(ctx, js, logger, id)
if err != nil {
helpers.LogError(err.Error())
return nil
}
- // Return nil to get next message
+ // Ack and return to get next message
+ if err := msg.Ack(); err != nil {
+ helpers.LogError(err.Error())
+ }
return nil
}🤖 Prompt for AI Agents
In cli/ext/build/ext.go around lines 137 to 147, after successfully calling
glue.DeleteState you must acknowledge the message to avoid redelivery: call
msg.Ack() (or the appropriate NATS/JetStream Ack method) immediately after
glue.DeleteState returns nil, handle/log any Ack error if present, and then
return nil; ensure the Ack occurs before returning so the message is not
requeued.
| userJson, err := json.Marshal(user) | ||
| if err != nil { | ||
| helpers.LogError(err.Error()) | ||
| return 0 | ||
| } | ||
|
|
||
| header := make(nats.Header) | ||
| header.Add(string(glue.HeaderStateId), destinationId.String()) | ||
| header.Add(string(glue.HeaderEventType), eventType) | ||
| header.Add(string(glue.HeaderTargetType), ev.TargetType) | ||
| header.Add(string(glue.HeaderEmittedAt), string(now)) | ||
| header.Add(string(glue.HeaderProvenance), string(userJson)) | ||
| header.Add(string(glue.HeaderTargetOps), ev.TargetOps) |
There was a problem hiding this comment.
Only include Provenance header when user context is present
Sending “null” as provenance leads to JSON unmarshal errors downstream.
- userJson, err := json.Marshal(user)
- if err != nil {
- helpers.LogError(err.Error())
- return 0
- }
-
header := make(nats.Header)
header.Add(string(glue.HeaderStateId), destinationId.String())
header.Add(string(glue.HeaderEventType), eventType)
header.Add(string(glue.HeaderTargetType), ev.TargetType)
header.Add(string(glue.HeaderEmittedAt), string(now))
- header.Add(string(glue.HeaderProvenance), string(userJson))
+ if user != nil {
+ userJson, err := json.Marshal(user)
+ if err != nil {
+ helpers.LogError(err.Error())
+ return 0
+ }
+ header.Add(string(glue.HeaderProvenance), string(userJson))
+ }
header.Add(string(glue.HeaderTargetOps), ev.TargetOps)
header.Add(string(glue.HeaderSourceOps), ev.SourceOps)
header.Add(string(glue.HeaderMeta), ev.Meta)
header.Add(string(glue.HeaderEmittedBy), from.String())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| userJson, err := json.Marshal(user) | |
| if err != nil { | |
| helpers.LogError(err.Error()) | |
| return 0 | |
| } | |
| header := make(nats.Header) | |
| header.Add(string(glue.HeaderStateId), destinationId.String()) | |
| header.Add(string(glue.HeaderEventType), eventType) | |
| header.Add(string(glue.HeaderTargetType), ev.TargetType) | |
| header.Add(string(glue.HeaderEmittedAt), string(now)) | |
| header.Add(string(glue.HeaderProvenance), string(userJson)) | |
| header.Add(string(glue.HeaderTargetOps), ev.TargetOps) | |
| header := make(nats.Header) | |
| header.Add(string(glue.HeaderStateId), destinationId.String()) | |
| header.Add(string(glue.HeaderEventType), eventType) | |
| header.Add(string(glue.HeaderTargetType), ev.TargetType) | |
| header.Add(string(glue.HeaderEmittedAt), string(now)) | |
| if user != nil { | |
| userJson, err := json.Marshal(user) | |
| if err != nil { | |
| helpers.LogError(err.Error()) | |
| return 0 | |
| } | |
| header.Add(string(glue.HeaderProvenance), string(userJson)) | |
| } | |
| header.Add(string(glue.HeaderTargetOps), ev.TargetOps) | |
| header.Add(string(glue.HeaderSourceOps), ev.SourceOps) | |
| header.Add(string(glue.HeaderMeta), ev.Meta) | |
| header.Add(string(glue.HeaderEmittedBy), from.String()) |
🤖 Prompt for AI Agents
In cli/ext/build/ext.go around lines 692 to 704, the code always adds a
Provenance header even when user context is absent, causing the string "null" to
be sent and breaking downstream JSON unmarshalling; modify the logic to only
marshal and add the Provenance header when a non-nil user/context exists (e.g.,
check user != nil or equivalent), and only call header.Add for provenance after
successful json.Marshal that produces a non-"null" payload so no "null" header
is sent.
| #[Override] | ||
| public function raiseEvent(OrchestrationInstance $instance, string $eventName, array $eventData): void | ||
| { | ||
| $event = WithOrchestration::forInstance( | ||
| StateId::fromInstance($instance), | ||
| RaiseEvent::forOperation($eventName, $eventData), | ||
| ); | ||
|
|
||
| $eventDescription = new EventDescription($event); | ||
| $this->worker->emitEvent($eventDescription->toArray()); | ||
| } |
There was a problem hiding this comment.
Use emit_event to actually publish the event (Worker::emitEvent only enqueues)
For local clients, calling $this->worker->emitEvent(...) only enqueues events in the worker and may never publish them. Use the extension's emit_event with provenance and the orchestration state id.
public function raiseEvent(OrchestrationInstance $instance, string $eventName, array $eventData): void
{
- $event = WithOrchestration::forInstance(
- StateId::fromInstance($instance),
- RaiseEvent::forOperation($eventName, $eventData),
- );
-
- $eventDescription = new EventDescription($event);
- $this->worker->emitEvent($eventDescription->toArray());
+ $stateId = StateId::fromInstance($instance);
+ $event = WithOrchestration::forInstance(
+ $stateId,
+ RaiseEvent::forOperation($eventName, $eventData),
+ );
+ $eventDescription = new EventDescription($event);
+ $userArray = $this->userContext ? Serializer::serialize($this->userContext) : null;
+ emit_event($userArray, $eventDescription->toArray(), $stateId->id);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[Override] | |
| public function raiseEvent(OrchestrationInstance $instance, string $eventName, array $eventData): void | |
| { | |
| $event = WithOrchestration::forInstance( | |
| StateId::fromInstance($instance), | |
| RaiseEvent::forOperation($eventName, $eventData), | |
| ); | |
| $eventDescription = new EventDescription($event); | |
| $this->worker->emitEvent($eventDescription->toArray()); | |
| } | |
| #[Override] | |
| public function raiseEvent(OrchestrationInstance $instance, string $eventName, array $eventData): void | |
| { | |
| $stateId = StateId::fromInstance($instance); | |
| $event = WithOrchestration::forInstance( | |
| $stateId, | |
| RaiseEvent::forOperation($eventName, $eventData), | |
| ); | |
| $eventDescription = new EventDescription($event); | |
| $userArray = $this->userContext ? Serializer::serialize($this->userContext) : null; | |
| emit_event($userArray, $eventDescription->toArray(), $stateId->id); | |
| } |
🤖 Prompt for AI Agents
In src/LocalOrchestrationClient.php around lines 76 to 86, the code calls
$this->worker->emitEvent(...) which only enqueues the event in the worker;
replace this with the extension-level emit_event call that actually publishes
the event. Build the payload with provenance and the orchestration StateId (use
StateId::fromInstance($instance)), include the event name and data, and call the
extension's emit_event API (passing the provenance and state id) instead of
Worker::emitEvent so the event is published immediately.
| #[Override] | ||
| public function withAuth(Provenance|string|null $token): void | ||
| { | ||
| $this->worker->setUser($token instanceof Provenance ? Serializer::serialize($token) : null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Persist user context locally so emit_event carries provenance
withAuth sets the worker user but leaves $this->userContext null. As a result, startNew emits events without provenance, producing a “null” provenance header on the Go side.
public function withAuth(Provenance|string|null $token): void
{
- $this->worker->setUser($token instanceof Provenance ? Serializer::serialize($token) : null);
+ $this->userContext = $token instanceof Provenance ? $token : null;
+ $this->worker->setUser($this->userContext ? Serializer::serialize($this->userContext) : null);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[Override] | |
| public function withAuth(Provenance|string|null $token): void | |
| { | |
| $this->worker->setUser($token instanceof Provenance ? Serializer::serialize($token) : null); | |
| } | |
| #[Override] | |
| public function withAuth(Provenance|string|null $token): void | |
| { | |
| // Persist provenance locally so emit_event can pick it up | |
| $this->userContext = $token instanceof Provenance ? $token : null; | |
| $this->worker->setUser( | |
| $this->userContext | |
| ? Serializer::serialize($this->userContext) | |
| : null | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/LocalOrchestrationClient.php around lines 141 to 145, withAuth currently
sets the worker user but leaves $this->userContext null, causing emitted events
to lack provenance; update the method to set $this->userContext to the same
value given to $this->worker->setUser (serialize the Provenance when $token is
an instance, pass through a string token, or null) so startNew/emit_event carry
the correct provenance header.
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores/Build