Open
Conversation
- Fix timeout passed as local_hostname in SMTP/SMTP_SSL constructors - Fix stale config_item variable in _get_params for per-message headers - Wrap SMTP connection in try/finally to prevent connection leak - Move error log behind conditional so successful sends don't log errors
Header charset was incorrectly set to QP when only body_encoding was 'quoted-printable'. Each encoding block now only checks its own setting.
- Rename partText/partHtml to part_text/part_html - Replace str.strip(x) with x.strip() - Move TypeError for invalid body type to else clause, removing redundant type() check before isinstance chain
Extract _build_charsets, _build_body_parts, _build_mime_structure, _set_headers, _attach_body, and _attach_files from the monolithic _make_message method. All are private with no public API change.
Consolidate X-header name normalization in _get_params (underscores to hyphens) and remove the .title() call in _set_headers, which could mangle acronyms like X-API-Key into X-Api-Key.
Replace module-level mutable defaults dict with a _get_defaults() helper that each test calls independently, eliminating implicit ordering dependencies between tests.
Register deprecation 3.0.16-1 and emit DeprecationWarning on every send() call. The bool return is preserved for backward compatibility but will be replaced with the smtplib senderrs dict in a future version.
📝 WalkthroughWalkthroughThis update adds deprecation warnings for SMTP handler return type changes and includes bug fixes for SMTP connection handling, timeout usage, header encoding, and X-header normalization. Message construction logic is refactored into modular helper methods with test coverage updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🧹 Nitpick comments (1)
cement/ext/ext_smtp.py (1)
385-395: Filenames with special characters may cause issues inContent-Disposition.The filename is directly interpolated into the header value without RFC 2231 encoding or quoting. Filenames containing spaces, quotes, or non-ASCII characters may result in malformed headers or be misinterpreted by mail clients.
♻️ Consider using email library's parameter encoding
if cid: part.add_header( 'Content-Disposition', - f'inline; filename={altname}', + 'inline', + filename=altname, ) part.add_header('Content-ID', f'<{cid}>') else: part.add_header( 'Content-Disposition', - f'attachment; filename={altname}', + 'attachment', + filename=altname, )Using keyword arguments lets
add_headerhandle proper RFC 2231 encoding for special characters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cement/ext/ext_smtp.py` around lines 385 - 395, The Content-Disposition header is being built by interpolating altname directly which can break for spaces, quotes or non-ASCII; update the two add_header calls that set 'Content-Disposition' (in ext_smtp.py where part.add_header is used with f'inline; filename={altname}' and f'attachment; filename={altname}') to pass the filename as a keyword parameter (e.g., use part.add_header('Content-Disposition', 'inline' or 'attachment', filename=altname) so the email library will perform proper RFC 2231 encoding/quoting for altname), keeping the existing Content-ID handling for cid unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cement/ext/ext_smtp.py`:
- Around line 231-237: The code that builds MIME parts when body is a dict (the
branch using variables part_text, part_html and calling MIMEText with cs_body)
currently calls body['text'].strip() and body['html'].strip() without null
checks; update those conditions to ensure the dict values are not None (e.g.,
check that body.get('text') is not None and is truthy before calling .strip(),
and similarly for body.get('html')) so you only call .strip() on real strings
and avoid AttributeError when values are None.
---
Nitpick comments:
In `@cement/ext/ext_smtp.py`:
- Around line 385-395: The Content-Disposition header is being built by
interpolating altname directly which can break for spaces, quotes or non-ASCII;
update the two add_header calls that set 'Content-Disposition' (in ext_smtp.py
where part.add_header is used with f'inline; filename={altname}' and
f'attachment; filename={altname}') to pass the filename as a keyword parameter
(e.g., use part.add_header('Content-Disposition', 'inline' or 'attachment',
filename=altname) so the email library will perform proper RFC 2231
encoding/quoting for altname), keeping the existing Content-ID handling for cid
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 012c85a9-dffc-4722-bc87-98db29dade7e
📒 Files selected for processing (5)
CHANGELOG.mdCLAUDE.mdcement/core/deprecations.pycement/ext/ext_smtp.pytests/ext/test_ext_smtp.py
Comment on lines
231
to
+237
| elif isinstance(body, dict): | ||
| # handle plain text | ||
| if 'text' in body and str.strip(body['text']) != '': | ||
| partText = MIMEText(str.strip(body['text']), | ||
| 'plain', | ||
| _charset=cs_body) | ||
| # handle html | ||
| if 'html' in body and str.strip(body['html']) != '': | ||
| partHtml = MIMEText(str.strip(body['html']), 'html', _charset=cs_body) | ||
|
|
||
| # To define the correct message content-type | ||
| # we need to indentify the content of this mail. | ||
| if 'text' in body and body['text'].strip() != '': | ||
| part_text = MIMEText(body['text'].strip(), | ||
| 'plain', | ||
| _charset=cs_body) | ||
| if 'html' in body and body['html'].strip() != '': | ||
| part_html = MIMEText(body['html'].strip(), 'html', _charset=cs_body) |
There was a problem hiding this comment.
Potential AttributeError if dict values are None.
Lines 232 and 236 call .strip() on dict values without null-checking first. If body['text'] or body['html'] is None, this will raise AttributeError: 'NoneType' object has no attribute 'strip'.
🛡️ Proposed fix to add null checks
elif isinstance(body, dict):
- if 'text' in body and body['text'].strip() != '':
+ if 'text' in body and body['text'] and body['text'].strip() != '':
part_text = MIMEText(body['text'].strip(),
'plain',
_charset=cs_body)
- if 'html' in body and body['html'].strip() != '':
+ if 'html' in body and body['html'] and body['html'].strip() != '':
part_html = MIMEText(body['html'].strip(), 'html', _charset=cs_body)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cement/ext/ext_smtp.py` around lines 231 - 237, The code that builds MIME
parts when body is a dict (the branch using variables part_text, part_html and
calling MIMEText with cs_body) currently calls body['text'].strip() and
body['html'].strip() without null checks; update those conditions to ensure the
dict values are not None (e.g., check that body.get('text') is not None and is
truthy before calling .strip(), and similarly for body.get('html')) so you only
call .strip() on real strings and avoid AttributeError when values are None.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive bugfix, refactor, and deprecation pass on the SMTP mail extension (cement/ext/ext_smtp.py).
Bugs Fixed
Refactoring
Test Improvements
Deprecations
Summary by CodeRabbit
Release Notes
Bug Fixes
Deprecations
Documentation
Tests