Conversation
Contributor
📝 WalkthroughWalkthroughThe changes introduce Server-Sent Events (SSE) streaming support to the application. A streamCallback closure is added to the controller that captures streaming headers and appends data chunks to the HTTP response. When streaming is detected, the response is finalized and the execution document is populated with completion metadata, bypassing normal post-processing. The Executor class is updated with an optional streaming callback parameter that manages callback invocation—emitting headers first via a null signal, then subsequent data chunks—while maintaining backward compatibility for non-streaming code paths. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Executor/Executor.php (1)
356-405:⚠️ Potential issue | 🔴 CriticalCallback mode can suppress transport errors.
When callback mode is enabled, execution returns at Line 402 before curl error handling (Lines 436-441). A timeout/network failure can therefore be treated as success by the caller. Move curl error/status checks before the callback-mode return.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Executor/Executor.php` around lines 356 - 405, The early return in the callback branch suppresses curl transport errors because curl_exec() error/statu 8000 s checks run after the "if (isset($callback)) { curl_close($ch); return []; }" block; move the curl error/status inspection (use curl_errno(), curl_error(), and curl_getinfo() checks that currently run after curl_exec()) to occur immediately after curl_exec() and before the callback-mode return so callback mode still observes/receives transport failures; ensure you still close the handle ($ch) and return/propagate an appropriate error/result instead of silently returning [] in the callback branch (update the logic around curl_exec(), the isset($callback) check, and the return path in Executor::execute or the containing method).
🧹 Nitpick comments (1)
src/Executor/Executor.php (1)
134-138: Silence the unused-parameter warning in the wrapper callback.At Line 134,
$headersis intentionally ignored. Rename it to$_headers(or_) to avoid PHPMD noise without changing behavior.Suggested diff
-$wrappedCallback = function (?string $data, ?array $headers) use ($callback): void { +$wrappedCallback = function (?string $data, ?array $_headers) use ($callback): void { if ($data !== null) { $callback($data); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Executor/Executor.php` around lines 134 - 138, The wrapper callback assigned to $wrappedCallback currently accepts a second parameter named $headers which is intentionally unused and triggers PHPMD warnings; update the parameter name to a throwaway variable (e.g. $_headers or $_) in the anonymous function signature used for $wrappedCallback in Executor.php so the behavior remains unchanged but the unused-parameter warning is silenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/general.php`:
- Around line 612-621: The streaming branch currently overwrites the real
streamed status/headers and returns early, skipping accounting/retention; update
the block that runs when $streamingDetected is true to set $execution attributes
using the previously extracted streamed status/headers (do not hardcode
responseStatusCode to 200 or set responseHeaders to []), keep setting
duration/logs/errors as needed, and remove the early return so the function
continues to run the later usage metrics and retention/cleanup paths; locate and
adjust the code around $streamingDetected, $execution->setAttribute(...) and the
return true to preserve streamed metadata and allow downstream accounting to
execute.
- Around line 604-606: The code currently always passes streamCallback into
Executor::createExecution, forcing the execution into the streaming branch
(which returns []); change the call site so streamCallback is only passed when
the client explicitly requested streaming (e.g., detect Accept:
text/event-stream or a dedicated boolean like $request->get('stream', false)).
Update the call that sets responseFormat and streamCallback to conditionally
include streamCallback (and set RESPONSE_FORMAT_ARRAY_HEADERS only for
streaming), and ensure the non-stream path omits the callback so
Executor::createExecution runs the non-stream branch without returning an empty
array.
In `@src/Executor/Executor.php`:
- Around line 356-364: The handler $handleEvent currently invokes the headers
callback on the first data chunk unconditionally, causing non-SSE responses to
be treated as streaming; update $handleEvent to only fire the headers signal
when the response indicates a stream (check $responseHeaders['content-type'] for
'text/event-stream' or a configured stream marker flag) before calling
$callback(null, $responseHeaders) and setting $callbackHeadersFired; keep the
existing $callback($data, null) and strlen return behavior for all chunks, but
ensure $callbackHeadersFired is only set when the content-type/marker check
passes (and account for case-insensitive header keys).
---
Outside diff comments:
In `@src/Executor/Executor.php`:
- Around line 356-405: The early return in the callback branch suppresses curl
transport errors because curl_exec() error/status checks run after the "if
(isset($callback)) { curl_close($ch); return []; }" block; move the curl
error/status inspection (use curl_errno(), curl_error(), and curl_getinfo()
checks that currently run after curl_exec()) to occur immediately after
curl_exec() and before the callback-mode return so callback mode still
observes/receives transport failures; ensure you still close the handle ($ch)
and return/propagate an appropriate error/result instead of silently returning
[] in the callback branch (update the logic around curl_exec(), the
isset($callback) check, and the return path in Executor::execute or the
containing method).
---
Nitpick comments:
In `@src/Executor/Executor.php`:
- Around line 134-138: The wrapper callback assigned to $wrappedCallback
currently accepts a second parameter named $headers which is intentionally
unused and triggers PHPMD warnings; update the parameter name to a throwaway
variable (e.g. $_headers or $_) in the anonymous function signature used for
$wrappedCallback in Executor.php so the behavior remains unchanged but the
unused-parameter warning is silenced.
Comment on lines
+604
to
606
| responseFormat: Executor::RESPONSE_FORMAT_ARRAY_HEADERS, | ||
| streamCallback: $streamCallback, | ||
| ); |
Contributor
There was a problem hiding this comment.
streamCallback is always enabled, so all executions are forced into stream mode.
At Line 605 this unconditionally activates Executor::createExecution’s stream branch (which returns []). That makes non-stream fallback fragile and can produce undefined-index behavior when no stream events arrive. Gate callback usage by explicit client intent (e.g., Accept: text/event-stream) or a dedicated stream flag.
Suggested diff
+$isStreamRequested = \str_contains(
+ \strtolower($request->getHeader('accept', '')),
+ 'text/event-stream'
+);
+
$executionResponse = $executor->createExecution(
@@
- streamCallback: $streamCallback,
+ streamCallback: $isStreamRequested ? $streamCallback : null,
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/general.php` around lines 604 - 606, The code currently
always passes streamCallback into Executor::createExecution, forcing the
execution into the streaming branch (which returns []); change the call site so
streamCallback is only passed when the client explicitly requested streaming
(e.g., detect Accept: text/event-stream or a dedicated boolean like
$request->get('stream', false)). Update the call that sets responseFormat and
streamCallback to conditionally include streamCallback (and set
RESPONSE_FORMAT_ARRAY_HEADERS only for streaming), and ensure the non-stream
path omits the callback so Executor::createExecution runs the non-stream branch
without returning an empty array.
Comment on lines
+612
to
+621
| if ($streamingDetected) { | ||
| $response->chunk('', true); | ||
|
|
||
| $execution->setAttribute('status', 'completed'); | ||
| $execution->setAttribute('logs', ''); | ||
| $execution->setAttribute('errors', ''); | ||
| $execution->setAttribute('responseStatusCode', 200); | ||
| $execution->setAttribute('responseHeaders', []); | ||
| $execution->setAttribute('duration', \microtime(true) - $durationStart); | ||
| return true; |
Contributor
There was a problem hiding this comment.
Streaming branch records incorrect execution metadata and exits before accounting.
Line 618 hardcodes responseStatusCode to 200, and Line 619 drops response headers, even though Line 568 already extracts streamed status. Also, returning at Line 621 bypasses later usage metrics and retention cleanup logic in this function. Persist streamed status/headers and avoid returning before accounting paths execute.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/general.php` around lines 612 - 621, The streaming branch
currently overwrites the real streamed status/headers and returns early,
skipping accounting/retention; update the block that runs when
$streamingDetected is true to set $execution attributes using the previously
extracted streamed status/headers (do not hardcode responseStatusCode to 200 or
set responseHeaders to []), keep setting duration/logs/errors as needed, and
remove the early return so the function continues to run the later usage metrics
and retention/cleanup paths; locate and adjust the code around
$streamingDetected, $execution->setAttribute(...) and the return true to
preserve streamed metadata and allow downstream accounting to execute.
Comment on lines
+356
to
364
| $callbackHeadersFired = false; | ||
| $handleEvent = function ($ch, $data) use ($callback, &$callbackHeadersFired, &$responseHeaders) { | ||
| if (!$callbackHeadersFired) { | ||
| // Fire headers signal once before the first data chunk | ||
| $callback(null, $responseHeaders); | ||
| $callbackHeadersFired = true; | ||
| } | ||
| $callback($data, null); | ||
| return \strlen($data); |
Contributor
There was a problem hiding this comment.
Header signal is emitted for any callback response, not just SSE.
Line 360 currently fires the “headers” signal on the first chunk unconditionally. Downstream logic treats that as “streaming detected”, so this can incorrectly switch non-SSE responses into the streaming path. Gate the header signal by response content type (e.g., text/event-stream) or an explicit stream marker.
Suggested diff
$handleEvent = function ($ch, $data) use ($callback, &$callbackHeadersFired, &$responseHeaders) {
- if (!$callbackHeadersFired) {
+ $contentType = \strtolower($responseHeaders['content-type'] ?? '');
+ $isEventStream = \str_starts_with($contentType, 'text/event-stream');
+ if ($isEventStream && !$callbackHeadersFired) {
// Fire headers signal once before the first data chunk
$callback(null, $responseHeaders);
$callbackHeadersFired = true;
}
- $callback($data, null);
+ if ($isEventStream) {
+ $callback($data, null);
+ }
return \strlen($data);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Executor/Executor.php` around lines 356 - 364, The handler $handleEvent
currently invokes the headers callback on the first data chunk unconditionally,
causing non-SSE responses to be treated as streaming; update $handleEvent to
only fire the headers signal when the response indicates a stream (check
$responseHeaders['content-type'] for 'text/event-stream' or a configured stream
marker flag) before calling $callback(null, $responseHeaders) and setting
$callbackHeadersFired; keep the existing $callback($data, null) and strlen
return behavior for all chunks, but ensure $callbackHeadersFired is only set
when the content-type/marker check passes (and account for case-insensitive
header keys).
🔄 PHP-Retry SummaryFlaky tests detected across commits: |
✨ Benchmark results
⚡ Benchmark Comparison
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Towards SER-334