fix: avoid panic in BuildRequest when request response has no request - #7601
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesRequest construction safety
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
One scope clarification, so this is judged on what it actually fixes. I tried to reach the panic through the CLI and could not: the Where it is reachable is the library/SDK surface, which is what this PR targets:
So this is a robustness fix for embedders rather than a live CLI crash. I'd rather say that plainly than overstate it. The supporting argument for it being an oversight stands either way: Happy to close this if you consider the nil state out of contract for the exported API. |
5bdde4b to
6f5a8ec
Compare
Fixes #7600.
What was wrong
RequestResponse.BuildRequest()dereferencedrr.Requestunconditionally and panicked when it was absent:Requestis optional —UnmarshalJSONonly sets it when a"request"key is present:So an entry with only a
urlunmarshals cleanly and then panics on use. The repo's existingTestUnmarshalJSONalready covers that exact shape ({"url": "example.com"}), so the state was reachable from code already under test.Reachability, verified rather than assumed — through the public
MetaInputAPI, which is JSON round-tripped:pkg/protocols/http/request_fuzz.go:61reaches it after checking onlyReqResp != nil, not.Request.The fix
An early nil check that sets
reqErr, matching how the function already reports its other failure:This follows the convention already used on the same field elsewhere in the file —
Clone()andID()both guard withif rr.Request != nil.BuildRequestwas the odd one out. No new error type, no behaviour change for well-formed input.Placing it inside the existing
sync.Oncekeeps the error cached like the other path, so repeat calls stay consistent.Tests
TestBuildRequestWithoutRequest{"url": ...}, confirmsRequestis nil, thenrequire.NotPanics+ returns an error and a nil requestTestBuildRequestWithRequestStillWorksThe second is the guard rail: a "fix" that simply refused to build anything would pass the first test and fail this one.
Bite-proofed — reverting only
http.gomakesTestBuildRequestWithoutRequestfail with the original nil-pointer panic.Verification
go test ./pkg/input/... ./pkg/protocols/common/contextargs/...— all packages ok.gofmtclean. Two files, +39 lines, no behaviour change for valid input.Summary by CodeRabbit