Skip to content

[Pulse demo] psf/requests PR #5922: Always use value of session.verify when set, fixes #5921 - #28

Open
dominic097 wants to merge 1 commit into
mainfrom
pulse-demo/2026-06-24/pr-5922-6
Open

[Pulse demo] psf/requests PR #5922: Always use value of session.verify when set, fixes #5921#28
dominic097 wants to merge 1 commit into
mainfrom
pulse-demo/2026-06-24/pr-5922-6

Conversation

@dominic097

Copy link
Copy Markdown
Owner
    Pulse demo seed recreated this as a synthetic PR in the fork on `2026-06-24`.

    Original PR: https://github.com/psf/requests/pull/5922
    Original number: #5922
    Original author: @brmzkw
    Original head: `main`
    Original base: `main`
    Original labels: none

    Representative upstream files:
    - `requests/sessions.py` (MODIFIED)
  • tests/test_requests.py (MODIFIED)

      ---
    
      This fixes https://github.com/psf/requests/issues/5921 where the value of REQUESTS_CA_BUNDLE is read even when certificate verification is disabled through the session.
    

@dominic097 dominic097 added the pulse-demo-seed Generated Pulse demo seed data label Jun 24, 2026

@dominic097 dominic097 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pulse-demo-seed Generated Pulse demo seed data

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant