Repository navigation
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
webhooks->delete() sends no request body, but Client still attaches its default Content-Type: application/json header. The API treats that header as a promise of JSON content and rejects the empty body with a 400. Every other delete-verb call in the SDK already sends an explicit body object, even when empty, so none of them hit this: profiles, users, templates, contacts, profiles campaigns, voice agents, and calls participants (both remove() and removeAll()). webhooks->delete() is the one bodyless delete in the SDK today. Drop the header when no body is sent, instead of sending a body just to get past the check.
2868586 to
89036c8
Compare
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The per-method fix only covered webhooks->delete(). The real cause is in BaseClient::buildRequest(): it attaches a default Content-Type header to every request regardless of whether that request has a body, which is what made webhooks->delete() 400 in the first place. Move the fix there instead: drop Content-Type whenever a request has no body and the caller didn't set one explicitly. This covers every current and future bodyless operation, not just this one method, and matches how the TypeScript and Ruby SDKs already handle the identical case (confirmed by reading their request-building code directly). This also changes every typed GET call, which never sends a body either: GET requests no longer carry a Content-Type header. No GET request has a body to describe, so this brings GET in line with the same sibling SDKs' behavior rather than carrying a header that never meant anything on those calls.
sudiptpa
changed the title
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
What's wrong
webhooks->delete()sends no request body, butClientattaches its defaultContent-Type: application/jsonheader on every request, body or not. The API treatsthat header as a promise of JSON content and rejects the empty body with a 400.
Confirmed live against api.sent.dm:
Every other delete-verb call in the SDK already sends an explicit body object, even when
empty, so none of them hit this: profiles, users, templates, contacts, profiles campaigns,
voice agents, and calls participants (both
remove()andremoveAll()).webhooks->delete()is the one bodyless call in the whole SDK that isn't a GET.
Checked the other Sent SDKs before deciding where to fix this
Three sibling SDKs already solve this generically, at the request builder, not per method:
client.ts): "A resource method always passes abodykey when itsoperation defines a request body, even if the caller omitted an optional body param.
Keep the content-type for those, and only elide it for operations with no body at all
(e.g. GET/DELETE)."
base_client.rb): "Generated methods always passreq[:body]for operationsthat define a request body, so only elide the content-type header when the operation has
no body at all, not when an optional body param was omitted."
requestconfig.go): never setsContent-Typeunless a request body was actuallyserialized (
if reader != nil { req.Header.Set("Content-Type", contentType) }).attached to the request when the params actually have one (
params._body().ifPresent { ... }),and Content-Type is derived from that body object, so no body means no header.
BodyContent()defaults tonullandWebhookDeleteParamsneveroverrides it, so .NET's
HttpContent-based Content-Type is naturally absent when there'sno content object.
is_body_allowed = options.method.lower() != "get"keepsContent-Typefor any non-GET call regardless of whether a body was actuallygiven, and none of Python's
_base_client.pyhas the TS/Ruby/Go style "operation has nobody at all" check. Not confirmed live, read from source only.
This PR fixes it at the same layer as TypeScript, Ruby, and Go:
BaseClient::buildRequest().What changed
The fix lives in
BaseClient::buildRequest(): dropContent-Typefrom the merged headerswhenever the request has no body and the caller didn't set
Content-Typeexplicitly.Before:
After:
An explicit override still wins: passing
Content-Typein a call's own headers isrespected either way, same as before.
This also changes every GET call, on purpose
A GET request never has a body either, so the same condition that fixes the one DELETE
call also drops
Content-Typefrom every typed GET method already in the SDK. Checkedevery
client->request()call site across every service: the only calls that omit thebodyargument are the 29 GET methods and this one DELETE method. No POST, PUT, or PATCHcall omits it, so nothing else is affected.
Flagging this explicitly since it is a real behavior change, not just a bug fix for one
method: every GET request's wire format changes, even though no GET request had a body to
describe in the first place. This matches what TypeScript, Ruby, and Go already do for GET
too, not something new introduced here.
How I verified it
Added
BaseClientTest-level coverage inClientTest.php:testBodylessDeleteOmitsContentType, confirmed it fails against the code before thischange and passes after
testBodylessGetOmitsContentType, same before/after check for a GET calltestDeleteWithExplicitBodyKeepsContentType, confirms a delete that does send a body(e.g.
profiles->delete()) is untouchedtestBodylessRequestKeepsAnExplicitContentTypeOverride, confirms a caller-suppliedContent-Typeheader still wins./scripts/lintand./scripts/testboth pass.Also re-verified the static claim above directly: counted every
client->request()callsite across every service file by hand (not sampled), 67 total, 30 omit
body:(29 GET,this one DELETE), the other 37 (8 delete, 7 patch, 19 post, 3 put) all pass an explicit
body and are structurally untouched.