fix: add comma-ok guards for type assertion panics#30
Conversation
…cution plan.go has two unsafe type assertions on reflect.Value outputs that could panic if the second return value is not an error type. Although IsValidBuilder validates at registration time, this adds defense-in-depth comma-ok guards at both processWork (line 144) and doWorkAndGetResult (line 182).
📝 WalkthroughWalkthroughAdded a README constant and adjusted anchor links; introduced guarded runtime checks when interpreting builder function second return values to avoid panics; added a unit test verifying error propagation and no goroutine leaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Plan as Plan
participant Worker as Worker goroutine
participant Builder as Builder function
participant Span as Tracing Span
participant Result as Result aggregator
Plan->>Worker: schedule work item
Worker->>Builder: invoke builder(...) -> (value, second)
Builder-->>Worker: returns (value, second)
Worker->>Span: set attributes
alt second implements error
Worker->>Span: set error = second
Worker->>Result: record error
else second not error
Worker->>Span: set error = formatted type message
Worker->>Result: record formatted error
end
Worker->>Plan: send completion
Plan->>Result: aggregate outcomes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens runtime reflection error-handling in plan execution by avoiding potential panics from unsafe .(error) assertions, and adds a regression test around error propagation.
Changes:
- Add comma-ok guards around reflected
errorextraction inprocessWorkanddoWorkAndGetResult. - Add a new plan run test to ensure error propagation remains correct.
- Regenerate README docs (new “Constants” section + updated links).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| README.md | Regenerated documentation and index to include constants and updated source links. |
| plan.go | Adds guarded type assertions and fallback error creation when the second return isn’t an error. |
| plan_test.go | Adds a test validating error propagation through the guarded path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plan_test.go (1)
97-110: This test doesn’t actually exercise the new non-error fallback branch.
DBTestFuncErrreturns a propererror, so this validates existing error propagation, not the new"second return value is not an error"path. Consider either renaming the test for accuracy or adding a focused case that hits the mismatch branch directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plan_test.go` around lines 97 - 110, The test TestPlanRun_ErrorFlowWithCommaOk currently uses DBTestFuncErr which returns a real error and therefore doesn't exercise the new "second return value is not an error" fallback; either rename the test to reflect that it checks normal error propagation or add a focused test that triggers the mismatch branch by registering a builder that returns a non-error second value (e.g., create DBTestFuncSecondNotError that returns (someResult, "not-an-error") or another non-error type), compile the plan and call executionPlan.Run(...) expecting an error that contains the mismatch message (e.g., "second return value is not an error"), and keep references to processWork and doWorkAndGetResult in the test description so reviewers can locate the code paths being exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plan_test.go`:
- Around line 97-110: The test TestPlanRun_ErrorFlowWithCommaOk currently uses
DBTestFuncErr which returns a real error and therefore doesn't exercise the new
"second return value is not an error" fallback; either rename the test to
reflect that it checks normal error propagation or add a focused test that
triggers the mismatch branch by registering a builder that returns a non-error
second value (e.g., create DBTestFuncSecondNotError that returns (someResult,
"not-an-error") or another non-error type), compile the plan and call
executionPlan.Run(...) expecting an error that contains the mismatch message
(e.g., "second return value is not an error"), and keep references to
processWork and doWorkAndGetResult in the test description so reviewers can
locate the code paths being exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91470986-5ad6-4afd-b01a-f1e4a5af87ee
📒 Files selected for processing (3)
README.mdplan.goplan_test.go
Address review feedback: - Use type-only (%T) instead of value (%v) to avoid leaking sensitive data or bloating output in the defense-in-depth error path - Store Interface() result once to avoid double call - Fix indentation in doWorkAndGetResult comma-ok block
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plan.go`:
- Around line 145-150: The code calls reflect.Value.IsNil() on the second return
value (secondReturn := o.outputs[1].Interface()) without checking the value's
Kind, which can panic for non-nilable kinds; add a small helper (e.g.,
safeIsNil(v interface{}) bool) that returns true if v is nil or v is a nil-able
kind (Chan, Func, Interface, Map, Ptr, Slice) and its reflect.Value.IsNil() is
true, otherwise returns false; replace the inline IsNil checks around
o.outputs[1].Interface() and the other occurrence with calls to this helper and
use reflect.ValueOf(v).IsValid() / nil check as appropriate before calling IsNil
to avoid panics while preserving the existing error-logging behavior
(span.SetError(fmt.Errorf(...)) / span.SetError(errVal)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a86330e9-30d6-421f-bcfc-bad8095d7bd6
📒 Files selected for processing (2)
plan.goplan_test.go
✅ Files skipped from review due to trivial changes (1)
- plan_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
plan.gothat could panic ifreflect.Value.Interface().(error)failsIsValidBuildervalidates at registration time, but reflection at runtime can still misbehaveTestPlanRun_ErrorFlowWithCommaOkto verify error propagation through the guarded pathTest plan
make testpasses with-racemake lintcleanmake docregenerated READMESummary by CodeRabbit
Documentation
Bug Fixes
Tests