Repository navigation
fix(transport): return null bodies for 204 and 205 on raw outbound transports - #5125
Conversation
…ansports Both raw outbound transports build a Response from a socket, so each applied the Fetch null-body rule on its own and the two disagreed. The SOCKS helper excluded 204 but not 205; the pinned direct helper attached a streaming body to every 2xx. A 204 or 205 therefore either raised a TypeError inside the response handler or waited for a peer that was entitled to keep the connection alive. The status set now lives in one place and both owners read it. Representation headers survive untouched: a 205 that advertises a coding it never sent is neither decoded nor refused, because there are no coded bytes to act on.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe raw outbound transports now share ChangesRaw transport response handling
Cross-transport regression coverage
Transport inventory update
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Upstream
participant PinnedHTTP
participant SOCKS5Fetch
participant Response
Upstream->>PinnedHTTP: Return 204, 205, or 304
PinnedHTTP->>Response: Resolve with null body and headers
Upstream->>SOCKS5Fetch: Return 204, 205, or 304
SOCKS5Fetch->>Response: Resolve as bodyless response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
The comment claimed an upgrade is refused before the shared predicate is consulted. It is not: the SOCKS helper stops its interim-response loop on 101 and carries that status into response construction. Describe 101 and 103 as statuses outside the final Response set these helpers support, which is what is actually true, without changing any runtime behavior.
The case was named for a release it did not check. It asserted status and body and then force-closed every socket in teardown, so the name claimed more than the assertions did. It now waits, under a bounded deadline rather than a fixed sleep, for the keep-alive peer's connection to be observably destroyed, which is the release the transport actually owes.
리뷰 · 우선순위 77 / 80raw outbound 두 경로(pinned direct·SOCKS5)는 이 PR은 호스티드 CI는 이 글을 쓰는 시점 기준 hygiene/gates/test 1–4/macos/npm-global/keyring/docker 등 핵심이 이미 그린입니다. #5126이 이 브랜치 위에 content-coding을 쌓고 있으므로, 이 PR이 스택의 바닥입니다. types/config 스플릿과 무관. 경로 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Maintainer integration decision for The seven-file change was independently reviewed. The informational-status prose was corrected, and the SOCKS 205 regression now observes connection destruction before fixture teardown. Runtime scope remains null-body handling; existing regressions were preserved. No unresolved review threads or maintainer change requests were present at the final check. Exact-head hosted run: https://github.com/lidge-jun/opencodex/actions/runs/35430059298. All four test shards, both macOS shards, storage/api/gates, keyring, docker, packaging/structure and aggregate The merge is bound to the reviewed SHA and current |
1 participant
Summary
Responsefrom a socket rather than going throughfetch, so each applied the Fetch null-body rule itself and the two disagreed:src/lib/socks5-fetch.tsexcluded 204 but not 205, andsrc/lib/pinned-http.tsattached a streaming body to every 2xx. On the pinned direct route a 204 raised aTypeErrorfromnew Response(stream, { status: 204 })inside the response handler instead of resolving; on either route a 205 waited for body bytes that were never coming, so the call settled only when a keep-alive peer eventually closed.src/lib/http-response-semantics.tsnow holds the null-body status set (204, 205, 304) and both transports read it. 101 and 103 are null-body statuses too, but neither is a final Response status these helpers support, and the file says so rather than claiming an upgrade is refused earlier.Content-Encoding: bris neither decoded nor refused, because there are no coded bytes to act on.Scope map:
src/lib/http-response-semantics.tssrc/lib/pinned-http.tssrc/lib/socks5-fetch.tsbodylessResponsereads the shared predicate, which adds 205 and keeps HEAD, 304 and informational behavior.tests/lib/transport-null-body.test.tsstructure/transports/inventory.mdNo behavior outside the null-body statuses changes. Content coding on the pinned direct route is a separate residual tracked in #5111, and the SOCKS upload lifecycle is #5110.
Verification
ocxcommand, so no local result is claimed. Exact-head hosted CI on this PR is the executable evidence.devhead, traced every path that reaches aResponseconstructor, and confirmedresponseBodyalready returnsnullfor anythingbodylessResponseaccepts, so adding 205 there needs no second change. Confirmed 304 cannot reach the new pinned branch because the existing non-2xx branch answers it first, and that the branch is still correct if that ever changes.tests/lib/transport-null-body.test.ts, all against loopback fake peers with no credentials: pinned 204 resolves with a null body while the peer holds the connection open; pinned 205 carryingContent-Encoding: brresolves with a null body and keeps the header; an ordinary pinned 200 still streams its JSON; a 205 through a fake SOCKS tunnel resolves with no body. The keep-alive peer is what distinguishes "resolved with no body" from "resolved because the connection went away".git diff --checkis clean. New test file registered inscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json; no file-size cap was raised, and none of the touched source files are tracked intests/fixtures/file-size-baseline.json.Checklist
Summary by CodeRabbit
Bug Fixes
Tests