fix(reliability): remove ambient HTTPX Connection default - #195
fix(reliability): remove ambient HTTPX Connection default#195seonghobae wants to merge 6 commits into
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 (3)
📝 WalkthroughWalkthrough동기 및 비동기 HTTPX 클라이언트 생성 로직을 공통 헬퍼로 통합했습니다. 헬퍼는 리다이렉트를 비활성화하고 환경 설정을 무시하며 기본 ChangesHTTPX 헤더 정책
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 review |
|
seonghobae
changed the title
|
@coderabbitai review exact head |
|
@opencode-agent review exact head |
|
@cwl-noema-review independently review exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Checked-out commit =='
git rev-parse HEAD
echo
echo '== Exact commit metadata =='
git show -s --format='%H%n%P%n%s' 5abc442bb4269c2bdce74cd870bfc9889c70be89
echo
echo '== Changed files at requested commit =='
git diff-tree --no-commit-id --name-status -r 5abc442bb4269c2bdce74cd870bfc9889c70be89
echo
echo '== Commit diff =='
git show --format= --find-renames --find-copies 5abc442bb4269c2bdce74cd870bfc9889c70be89 -- \
src/egressweave/sync_transport.py \
src/egressweave/transport.py \
tests/test_public_client_default_headers.py
echo
echo '== Structure of changed Python files =='
ast-grep outline src/egressweave/sync_transport.py --items all --type class,function
ast-grep outline src/egressweave/transport.py --items all --type class,function
ast-grep outline tests/test_public_client_default_headers.py --items all --type class,function
echo
echo '== Header validation and HTTPX client construction references =='
rg -n -C 5 -i \
'hop[-_ ]by[-_ ]hop|forbidden.*header|header.*forbidden|connection|httpx\.(Client|AsyncClient)\s*\(' \
src/egressweave testsLength of output: 130398 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='5abc442bb4269c2bdce74cd870bfc9889c70be89'
parent_one="$(git rev-parse "${commit}^1")"
parent_two="$(git rev-parse "${commit}^2")"
echo '== Per-parent changes in merge commit =='
echo "-- Diff from first parent: ${parent_one} --"
git diff --stat "${parent_one}" "${commit}"
git diff --find-renames --find-copies "${parent_one}" "${commit}" -- \
src/egressweave/sync_transport.py \
src/egressweave/transport.py \
tests/test_public_client_default_headers.py
echo
echo "-- Diff from second parent: ${parent_two} --"
git diff --stat "${parent_two}" "${commit}"
git diff --find-renames --find-copies "${parent_two}" "${commit}" -- \
src/egressweave/sync_transport.py \
src/egressweave/transport.py \
tests/test_public_client_default_headers.py
echo
echo '== Sync builder and transport dispatch =='
sed -n '220,380p' src/egressweave/sync_transport.py
echo
echo '== Async builder and transport dispatch =='
sed -n '380,530p' src/egressweave/transport.py
echo
echo '== Header safety implementation =='
sed -n '1,240p' src/egressweave/request_safety.py
echo
echo '== New default-header regression tests =='
cat -n tests/test_public_client_default_headers.py
echo
echo '== Existing protocol-switching regression tests =='
cat -n tests/test_protocol_switching_header_safety.py
echo
echo '== Public-builder forbidden-header test references =='
rg -n -C 4 \
'(build_egress_sync_client|build_egress_http_client|build_pinned_https_client|build_pinned_https_async_client).*(headers|Connection|Keep-Alive|Upgrade|Proxy-)' \
tests || trueLength of output: 44638 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Pull request metadata =='
gh pr view 195 --repo ContextualWisdomLab/EgressWeave \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeCommit,url \
--jq '.'
echo
base="$(gh pr view 195 --repo ContextualWisdomLab/EgressWeave --json baseRefOid --jq '.baseRefOid')"
head='5abc442bb4269c2bdce74cd870bfc9889c70be89'
echo "== Full PR diff: ${base}...${head} =="
git diff --stat "${base}" "${head}"
git diff --name-status "${base}" "${head}"
git diff --check "${base}" "${head}"
echo
echo '== HTTPX client constructors in the PR range =='
git diff -U12 "${base}" "${head}" -- \
src/egressweave/sync_transport.py \
src/egressweave/transport.py \
tests/test_public_client_default_headers.py
echo
echo '== All current caller paths into request-header validation =='
rg -n -C 8 \
'_build_safe_request_headers\s*\(|safe_headers\s*=\s*_build_safe_request_headers' \
src/egressweave/sync_transport.py \
src/egressweave/transport.py \
src/egressweave/request_safety.py
echo
echo '== Repository test configuration and dependency pins =='
fd -a '^(pyproject\.toml|tox\.ini|pytest\.ini|requirements.*\.txt|uv\.lock|poetry\.lock)$' . \
-x sh -c 'echo "--- $1"; sed -n "1,220p" "$1"' sh {}Length of output: 26688
No findings in the requested scope.
I did not execute repository code or tests because this review environment permits static inspection only. You are interacting with an AI system. |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head source review for 5abc442bb4269c2bdce74cd870bfc9889c70be89 found no blocking defect in the three-file public-client-default scope. The helper removes only HTTPX's ambient case-insensitive Connection default after constructing each sync/async client, keeps caller-supplied hop-by-hop fields subject to the existing strict transport rejection, and uses injected pinned-pool regressions to prove ordinary requests reach dispatch without widening authority, proxy, redirect, DNS, TLS, framing or resource policy. This is author-side review evidence only, not independent approval. Keep Draft until the corrected central Dependency Review and Strix evidence paths produce substantive exact-head results and independent review is present.
|
@cwl-noema-review review Review exact current head |
1 participant
Purpose
Fix #194 at the public EgressWeave client-construction boundary without weakening the strict hop-by-hop request-header validator. This PR is also the required predecessor for Draft #193's response-cookie-state repair.
Exact current identity
7faf7a3b8a47980113982914000e724ab6a6cda5;5abc442bb4269c2bdce74cd870bfc9889c70be89;src/egressweave/sync_transport.py,src/egressweave/transport.py, andtests/test_public_client_default_headers.py.Historical checks, reviews, comments, base snapshots and generated merge revisions from predecessor heads do not transfer.
RCA and narrow fix
HTTPX adds an ambient
Connectionclient default. EgressWeave correctly rejects hop-by-hop request headers at its transport boundary, so an ordinary policy-allowed request built through the public client could deny itself before pinned-pool dispatch unless the caller manually removed that framework default.The narrow repair removes only HTTPX's ambient
Connectiondefault when EgressWeave constructs its synchronous/asynchronous public clients. Caller-supplied hop-by-hop fields remain fail-closed at the existing strict transport validator. Destination authorization, normalized(hostname, port), method/target/framing policy, redirects, proxies, Unix sockets, TLS identity, DNS pinning/revalidation, credentials, finite resource budgets and cleanup remain unchanged.Focused regressions require an ordinary allowed request to reach the injected pinned pool without caller surgery, require the final low-level request to omit ambient hop-by-hop controls, and retain denial of caller-supplied forbidden fields.
Exact-current-head evidence
On unchanged
5abc442bb4269c2bdce74cd870bfc9889c70be89:31596370532: completed success;31596370551: completed success;31596370786: aggregate success, but dependency-review job94112774879shows the actual immutable-pinned Dependency review action as skipped; wrapper green is therefore non-passing supply-chain evidence;94121293201: completed success, but check/model evidence is not promoted into a formal approval;94112774229: completed success as separate machine/model evidence;94112774246: wrapper success with annotation Strix backend unavailable and no vulnerability report; this is non-passing semantic security evidence under security: fail closed when required Strix review is unavailable #197;The PR was returned to Draft because its exact current head is not merge-acceptable despite green wrappers. No source head was churned to retrigger external providers.
Read-only central prerequisites
Dependency Review and Strix fail-closed result semantics are organization-owned. The current Dependency Review repair remains separately governed as
ContextualWisdomLab/.github#897; Strix no-report/unavailable semantics are tracked centrally by.github#891and locally by #197..githubis read-only from this EgressWeave writer. Do not create an EgressWeave-local substitute, weaken either gate, invent credentials, duplicate the workflows, or mutate this clean source head solely to retrigger provider behavior.Remaining acceptance
Progresses #194; predecessor of #193/#134.
Summary by CodeRabbit
개선 사항
Connection헤더가 자동으로 제거됩니다.버그 수정