[Pulse demo] psf/requests PR #5922: Always use value of session.verify when set, fixes #5921 - #28
[Pulse demo] psf/requests PR #5922: Always use value of session.verify when set, fixes #5921#28dominic097 wants to merge 1 commit into
Conversation
dominic097
left a comment
There was a problem hiding this comment.
Aziron Pulse PR Review
This PR's title/body claim a fix for psf#5921 (session.verify handling in requests/sessions.py), but the actual diff only adds a demo metadata JSON file and a comment line to tests/test_requests.py. No source fix or real test was added.
Confidence Score: 5/5
The diff is tiny and fully visible: a metadata JSON plus one comment line, with no change to requests/sessions.py despite the PR claiming one, so the mismatch between intent and content is certain.
| Field | Value |
|---|---|
| Verdict | comment |
| Confidence | high |
| Changed files | 2 |
Engineering Review
What changed: The diff adds a new metadata file .pulse-demo/sampled-prs/2026-06-24/upstream-pr-5922.json describing upstream psf/requests PR psf#5922, and appends two lines to tests/test_requests.py (a blank line plus the comment # Pulse demo seed 2026-06-24: metadata from psf/requests PR #5922) after test_json_decode_errors_are_serializable_deserializable. The referenced production change to requests/sessions.py and the 12-line test cited in the PR metadata are NOT present in this diff.
Correctness: The added content is inert and syntactically harmless: the comment at tests/test_requests.py:3096 is a Python comment and does not affect collection. However, the PR is functionally incorrect relative to its stated intent — the metadata file (upstream-pr-5922.json) claims requests/sessions.py is MODIFIED (+1/-1) and tests/test_requests.py gains +12 lines of real test coverage, but the actual changed_files show only a JSON seed and a single comment line. The verify-handling fix described in the body is absent.
Completeness: Incomplete with respect to the described issue. To fix psf#5921 the change must read session.verify and avoid applying REQUESTS_CA_BUNDLE/CURL_CA_BUNDLE when verification is disabled — that logic lives in requests/sessions.py (Session.merge_environment_settings / send) and is not touched here. No behavioral test asserting that env CA bundles are ignored when verify=False was added.
Risks: Low direct runtime risk: only a comment and an out-of-tree demo JSON were added, neither affects the requests library or test outcomes. The real risk is a false sense of resolution — merging this would close/track psf#5921 without delivering the actual fix or test, leaving the original certificate-verification bug in place.
Missing tests: A real regression test is missing: one that sets REQUESTS_CA_BUNDLE (or CURL_CA_BUNDLE) in the environment, issues a request through a Session with verify=False, and asserts the env CA bundle is not applied (verification stays disabled). The lone comment line at tests/test_requests.py:3096 provides no assertion or coverage.
Follow-up work: Open a task to actually implement the psf#5921 fix in requests/sessions.py (honor session.verify over REQUESTS_CA_BUNDLE/CURL_CA_BUNDLE when verify is falsy) and add the corresponding behavioral test described in the PR metadata. Separately, decide whether .pulse-demo/ seed artifacts belong in the repository or should be gitignored.
Decision — COMMENT: Not request_changes because the diff introduces no correctness, security, or regression risk to shipped code — the additions are inert. Not approve because the root issue (psf#5921 verify handling) is not addressed and the claimed test coverage is absent, so this is purely advisory: the PR does not do what it says.
Important Files Changed
| Filename | Overview |
|---|---|
.pulse-demo/sampled-prs/2026-06-24/upstream-pr-5922.json |
New demo metadata describing upstream psf/requests PR psf#5922. It asserts requests/sessions.py is modified and tests/test_requests.py gains +12 lines, but neither appears in this PR's actual diff — the metadata does not match the change. |
tests/test_requests.py |
Adds a blank line and a single comment (line 3096) after test_json_decode_errors_are_serializable_deserializable. No assertions or test logic; provides no coverage for the verify/CA-bundle behavior the PR claims to fix. |
Findings
No blocking findings were included in the approved draft.
Aziron Pulse generated this from sandbox/MCP-assisted LLM review with native code intelligence context.
tests/test_requests.py(MODIFIED)