fix: use certifi fallback in github-merge#7391
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit c69e7bf) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
Sequence Diagram(s)sequenceDiagram
participant github_merge as github-merge.py
participant ssl as ssl
participant certifi as certifi
participant urllib as urlopen
github_merge->>ssl: inspect trust store and build default context
github_merge->>certifi: use certifi.where() when fallback applies
github_merge->>urllib: call urlopen(req, context=context) or urlopen(req)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contrib/devtools/github-merge.py`:
- Around line 70-71: The SSL cert fallback check in the merge helper is treating
empty values for SSL_CERT_FILE and SSL_CERT_DIR as if they were unset, which can
override explicit user configuration. Update the logic in the github-merge.py
environment check to test for the presence of those keys in os.environ rather
than their truthiness, so `SSL_CERT_FILE=''` or `SSL_CERT_DIR=''` still disables
the certifi fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7b3790d-1e90-4913-b743-ec048cc4d55c
📒 Files selected for processing (1)
contrib/devtools/github-merge.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #7391 makes a small, conservative change to the maintainer-only contrib/devtools/github-merge.py script: opportunistically use certifi's CA bundle only when no SSL_CERT_* override is set, the default trust store is empty, and certifi is importable. The fallback uses correct Python SSL APIs and preserves existing behavior in all other cases. No in-scope issues.
Note: GitHub does not allow me to submit APPROVE on my own PR, so this is posted as a COMMENT while preserving the verified non-blocking state.
thepastaclaw
force-pushed
the
fix/github-merge-certifi
branch
from
c6d4e7d to
bedeca7
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Single-commit PR refining the optional certifi-based SSL fallback in contrib/devtools/github-merge.py. The latest delta tightens the override check from truthiness to env-var presence, so explicit empty SSL_CERT_FILE/SSL_CERT_DIR values are still respected. No in-scope issues found; prior review found no issues either.
Note: GitHub does not allow me to approve my own PR, so this is posted as a COMMENT while preserving the verified nonblocking state.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bedeca7753
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contrib/devtools/github-merge.py`:
- Around line 73-82: The trust-store detection in the default SSL context logic
is too aggressive because cert_store_stats() can report x509_ca == 0 even when a
lazily loaded capath is valid. Update the fallback logic in the function that
builds the SSL context so it checks ssl.get_default_verify_paths() first, or
only falls back to certifi when neither cafile nor capath is available. Keep the
existing flow around default_ctx, certifi import, and the final
ssl.create_default_context(cafile=certifi.where()) return, but avoid replacing a
working system trust store.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa1cf8f3-5714-4cd4-af49-22969ee6d854
📒 Files selected for processing (1)
contrib/devtools/github-merge.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative full-PR assessment: the github-merge.py certifi fallback is narrowly scoped and I found no code-level correctness issues in the final source. Carried-forward prior findings: none, because the prior review at bedeca775385f660b787458d1dd1c4a14d068f7b had no findings. New findings in the latest delta: one commit-history suggestion; the three-line default trust-path guard is a follow-up to the helper introduced by the immediately preceding commit and should be squashed before merge.
Note: posted as a COMMENT review because GitHub does not allow approving my own PR.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `contrib/devtools/github-merge.py`:
- [SUGGESTION] contrib/devtools/github-merge.py:74-76: Squash the SSL trust-path follow-up into the original fix
The latest commit `2bd13f39348f0e9cb4fa474e88cbdd20228ba355` only adds this guard inside `_ssl_context_with_certifi_fallback()`, which was introduced by the immediately preceding commit `bedeca775385f660b787458d1dd1c4a14d068f7b`. Since Dash preserves PR commits in merge history and the first commit has not shipped independently, leaving this as a separate follow-up records a fixup-style patch instead of one coherent logical change. Squash `2bd13f3934` into `bedeca7753` and keep the trust-path rationale in the combined commit message.
2bd13f3 to
ed1f409
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed1f4094d2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: none; prior-1 is FIXED because the PR stack is now a single commit and the former follow-up is no longer a separate fixup-style commit. New findings in the latest delta: none; the tree is unchanged from the prior reviewed SHA and the cumulative one-file change is scoped to the GitHub API SSL fallback.
Note: posted as a COMMENT review because GitHub does not allow approving my own PR.
thepastaclaw
force-pushed
the
fix/github-merge-certifi
branch
from
ed1f409 to
91627b1
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contrib/devtools/github-merge.py`:
- Around line 98-102: The certifi-backed SSL context creation in the helper that
imports certifi and returns ssl.create_default_context(cafile=certifi.where())
should fail open instead of throwing when the bundle path is missing or invalid.
Wrap the context creation in a guard that catches any error from the certifi
path or ssl context setup, and return None so the caller can fall back to
urllib’s default behavior; keep the optional-certifi handling localized in this
helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e3256790-55ee-432d-94bd-c779dad12375
📒 Files selected for processing (1)
contrib/devtools/github-merge.py
|
Tracker #1643 follow-up:
Current CI on the rewritten head is a fresh run; maintainers can let it finish or rerun if they want broader signal. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contrib/devtools/github-merge.py (1)
98-102: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFail open when the certifi-backed context cannot be created.
ssl.create_default_context(cafile=certifi.where())raises if certifi's bundle path is missing or invalid. Since certifi is optional and this is a best-effort fallback, wrap creation and returnNoneso urllib's default behavior is used instead of aborting the merge.Proposed fix
try: import certifi except ImportError: return None - return ssl.create_default_context(cafile=certifi.where()) + try: + return ssl.create_default_context(cafile=certifi.where()) + except (OSError, ssl.SSLError): + return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/devtools/github-merge.py` around lines 98 - 102, The certifi-based SSL context fallback in the helper that imports certifi and calls ssl.create_default_context should fail open instead of raising when the bundle path is invalid. Update that certifi-backed creation path to catch errors from certifi.where() or create_default_context and return None, so the merge flow can continue with urllib’s default behavior. Keep the change localized to the existing fallback helper and its certifi import logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@contrib/devtools/github-merge.py`:
- Around line 98-102: The certifi-based SSL context fallback in the helper that
imports certifi and calls ssl.create_default_context should fail open instead of
raising when the bundle path is invalid. Update that certifi-backed creation
path to catch errors from certifi.where() or create_default_context and return
None, so the merge flow can continue with urllib’s default behavior. Keep the
change localized to the existing fallback helper and its certifi import logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 19bb1ef6-af36-48d7-9ece-6a83c4606719
📒 Files selected for processing (1)
contrib/devtools/github-merge.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91627b1d7c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
force-pushed
the
fix/github-merge-certifi
branch
from
91627b1 to
cfcdf10
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: none; new findings in the latest delta: none. The current exact SHA 91627b1 remains a single coherent commit over upstream/develop, so the older commit-history suggestion stays fixed. The cumulative one-file change and the ed1f409..91627b1 delta both preserve urllib defaults when trust material exists and only use certifi as a narrow fallback for empty default trust stores.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: none; new findings in the latest delta: none. I verified exact SHA cfcdf10, the PR remains a single-commit, one-file change, and the 91627b1..cfcdf10 delta only adds defensive fallback handling for certifi context creation. The older commit-history suggestion remains fixed because the current stack is still one coherent non-merge commit.
Note: posted as a COMMENT review because GitHub does not allow approving my own PR.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
fix/github-merge-certifi
branch
from
cfcdf10 to
df5c9f2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contrib/devtools/github-merge.py (1)
91-101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the default-trust inspection exception handler.
Line 100 currently swallows any
Exception, which can hide unrelated coding errors and silently disable the certifi fallback. Catch the expected OS/TLS failures instead.Proposed refinement
- except Exception: + except (OSError, ssl.SSLError): return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/devtools/github-merge.py` around lines 91 - 101, The trust-store probe in the default-trust check is catching every Exception, which can mask unrelated bugs and wrongly suppress the certifi fallback. In the function that inspects ssl.create_default_context, ssl.get_default_verify_paths, and _ssl_path_has_trust_material, narrow the handler to the specific OS/TLS errors you expect from missing or inaccessible CA data, and let unexpected programming errors surface.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@contrib/devtools/github-merge.py`:
- Around line 91-101: The trust-store probe in the default-trust check is
catching every Exception, which can mask unrelated bugs and wrongly suppress the
certifi fallback. In the function that inspects ssl.create_default_context,
ssl.get_default_verify_paths, and _ssl_path_has_trust_material, narrow the
handler to the specific OS/TLS errors you expect from missing or inaccessible CA
data, and let unexpected programming errors surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4995f518-5fca-4ec0-83ef-114c5ed0a2cc
📒 Files selected for processing (1)
contrib/devtools/github-merge.py
Use certifi CA bundle for GitHub API requests only when Python default SSL context has no loaded CA certs, no explicit SSL_CERT_FILE or SSL_CERT_DIR override is present, and Python cannot find usable default cafile or capath trust material. This preserves urllib default OpenSSL trust behavior for normal system trust stores, including capath-only setups where OpenSSL loads hashed certificate entries lazily during the TLS handshake, while still falling back when compiled-in paths are empty or unusable.
thepastaclaw
force-pushed
the
fix/github-merge-certifi
branch
from
df5c9f2 to
c69e7bf
Compare
|
Addressed CodeRabbit’s latest nitpick in Validation rerun:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: none; new findings in the latest delta: none. I verified exact SHA df5c9f2, read the historical cfcdf10 review artifact, checked the one-commit stack, and confirmed the cfcdf10..df5c9f2 delta only narrows capath trust-material detection to OpenSSL-style hashed certificate entries. The older commit-history suggestion remains fixed because the current reviewed stack is still one coherent non-merge commit.
Note: posted as a COMMENT review because GitHub does not allow approving my own PR.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: none; the prior review had no active findings, and the older commit-history concern remains fixed because the current develop..HEAD stack is still a single non-merge commit. New findings in the latest delta: none; the only change narrows fallback-probe exception handling to expected SSL/filesystem errors, and the cumulative PR remains scoped to an optional certifi fallback for GitHub API HTTPS requests. I also verified the touched script parses with Python after redirecting bytecode output to a writable cache path.
Note: posted as a COMMENT review because GitHub does not allow approving my own PR.
2 participants
Issue being fixed or feature implemented
contrib/devtools/github-merge.pyfetches pull request metadata fromapi.github.comusing Python's defaulturllibHTTPS behavior. On machineswhere Python's OpenSSL cannot find system CA certificates, maintainers currently
need to export
SSL_CERT_FILEto thecertifiCA bundle before every merge run.This improves the maintainer merge workflow without requiring a global shell workaround.
What was done?
Added a small optional SSL-context fallback for GitHub API requests:
urllibdefaults untouched whenSSL_CERT_FILEorSSL_CERT_DIRisexplicitly set.
urllibdefaults untouched when Python's default trust store alreadyhas CA certificates.
certifiis importable, usecertifi.where()for the API request SSL context.certifioptional; systems without it continue using the existing behavior.How Has This Been Tested?
Validated locally on macOS with Python 3.9.6:
python3 -m py_compile contrib/devtools/github-merge.pytest/lint/lint-python.pySSL_CERT_FILEsetSSL_CERT_DIRsetcertificertifishipBreaking Changes
None. The fallback is only used when Python reports an empty default CA store,
no explicit
SSL_CERT_*override is set, andcertifiis installed.Checklist
code-owners and collaborators only)