Merged
Conversation
Neo - PR Security ReviewNo security issues found Highlights
Comment |
Contributor
WalkthroughThe PR adds warning-level logging for code execution failures and enhanced debug output for stderr capture in the code protocol handler. Concurrently, test infrastructure is refactored with helper functions to reduce duplication, and comprehensive test cases are added covering successful execution, stderr handling, and mixed output scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/protocols/code/code_test.go (1)
67-74: Assert execution error metadata in the failing-script case.Given the PR objective, this test should also validate that failure context is surfaced in the event payload (e.g.,
gotEvent["error"]is non-empty), not just stderr content.✅ Suggested test assertion
require.Contains(t, gotEvent["stderr"], "fail-message") require.Empty(t, gotEvent["response"], "stdout should be empty when script only writes to stderr") + require.NotEmpty(t, gotEvent["error"], "execution error should be surfaced in event payload")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/protocols/code/code_test.go` around lines 67 - 74, Update TestCodeProtocolFailingScript to also assert that the failure metadata is present in the event payload: after calling executeRequest (used with newTestRequest) verify that gotEvent["error"] is non-empty (or contains the expected error substring) in addition to the existing stderr/response checks so the test confirms the protocol surfaces execution error context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/protocols/code/code.go`:
- Around line 280-282: The handler currently only logs execution errors but
doesn't include them in the event payload; update the error branch in the
execute code path (the block checking if err != nil in
pkg/protocols/code/code.go around request.options.TemplateID) to set the payload
map entry data["error"] (or the existing result/event payload map variable) to
the error information (e.g., err.Error() or the error object) so downstream
consumers can detect failures; keep the existing gologger.Warning() call but
ensure data["error"] is populated whenever err != nil.
---
Nitpick comments:
In `@pkg/protocols/code/code_test.go`:
- Around line 67-74: Update TestCodeProtocolFailingScript to also assert that
the failure metadata is present in the event payload: after calling
executeRequest (used with newTestRequest) verify that gotEvent["error"] is
non-empty (or contains the expected error substring) in addition to the existing
stderr/response checks so the test confirms the protocol surfaces execution
error context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc0caeee-4c37-46c9-b662-dd587ad447f7
📒 Files selected for processing (2)
pkg/protocols/code/code.gopkg/protocols/code/code_test.go
Comment on lines
+280
to
+282
| if err != nil { | ||
| gologger.Warning().Msgf("[%s] Could not execute code: %s", request.options.TemplateID, err) | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate execution failures into the event payload, not only logs.
This currently logs the failure but does not set data["error"], so downstream result/event consumers still can’t reliably distinguish execution failure from non-match.
💡 Proposed fix
- if err != nil {
- gologger.Warning().Msgf("[%s] Could not execute code: %s", request.options.TemplateID, err)
- }
+ execError := ""
+ if err != nil {
+ execError = err.Error()
+ gologger.Warning().Msgf("[%s] Could not execute code: %s", request.options.TemplateID, err)
+ }
@@
data["template-path"] = request.options.TemplatePath
data["template-id"] = request.options.TemplateID
data["template-info"] = request.options.TemplateInfo
+ if execError != "" {
+ data["error"] = execError
+ }
if gOutput.Stderr.Len() > 0 {
data["stderr"] = fmtStdout(gOutput.Stderr.String())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/protocols/code/code.go` around lines 280 - 282, The handler currently
only logs execution errors but doesn't include them in the event payload; update
the error branch in the execute code path (the block checking if err != nil in
pkg/protocols/code/code.go around request.options.TemplateID) to set the payload
map entry data["error"] (or the existing result/event payload map variable) to
the error information (e.g., err.Error() or the error object) so downstream
consumers can detect failures; keep the existing gologger.Warning() call but
ensure data["error"] is populated whenever err != nil.
dogancanbakir
approved these changes
Mar 24, 2026
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
2 participants
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.
Proposed changes
Close #5350
Checklist
Summary by CodeRabbit
Improvements
Tests