Fix Windows attachment corruption: add O_BINARY flag to os.open - #495
Merged
taylorwilsdon merged 2 commits intoFeb 24, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds a Windows-safe open flag to save_attachment() by ORing in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
mickey-mikey
marked this pull request as ready for review
February 20, 2026 03:55
mickey-mikey
force-pushed
the
fix/windows-attachment-corruption
branch
from
February 20, 2026 04:00
ee6d083 to
57f5049
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/gmail/test_attachment_fix.py (2)
59-75: Test doesn't actually exercise newline stripping for "large" input.The PDF header
b"%PDF-1.7"encodes to only 12 base64 characters ("JVBERi0xLjc="). Since the chunking uses 76-character segments, no newlines are actually inserted—the list comprehension produces a single element, and"\n".join()returns the string unchanged.Consider using a larger payload to actually test multi-line base64 stripping.
♻️ Proposed fix: Use a payload large enough to span multiple lines
def test_large_base64_with_line_breaks(): """Test with a large PDF-like base64 string.""" - # Create a PDF-like binary (starts with %PDF-1.7) - pdf_header = b"%PDF-1.7" + # Create a larger PDF-like binary to ensure multiple base64 lines + pdf_header = b"%PDF-1.7" + b"\x00" * 100 # 108 bytes -> ~144 base64 chars pdf_base64 = base64.b64encode(pdf_header).decode() # Simulate embedded newlines (how large base64 is formatted) pdf_base64_with_newlines = "\n".join( [pdf_base64[i:i+76] for i in range(0, len(pdf_base64), 76)] ) + # Verify newlines were actually inserted + assert "\n" in pdf_base64_with_newlines + # Apply the fix cleaned = pdf_base64_with_newlines.replace("\r", "").replace("\n", "") # Should decode to original PDF header result = base64.b64decode(cleaned) assert result == pdf_header🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gmail/test_attachment_fix.py` around lines 59 - 75, The test_large_base64_with_line_breaks currently uses pdf_header = b"%PDF-1.7" which base64-encodes to only ~12 chars so pdf_base64_with_newlines never gets split; change the payload to be large enough (e.g. repeat pdf_header or use a larger binary) so pdf_base64 becomes longer than 76 chars, then regenerate pdf_base64_with_newlines, strip newlines into cleaned, decode and assert the decoded bytes equal the original repeated payload; update references in the test to pdf_header, pdf_base64, pdf_base64_with_newlines and cleaned accordingly.
1-84: Tests duplicate the stripping logic instead of calling the production function.These tests re-implement the fix logic inline rather than importing and testing
get_gmail_attachment_content(or a helper extracted from it). If the production code diverges, these tests won't catch the regression.Consider adding at least one integration-style test that exercises the actual production code path, or extract the stripping logic into a testable helper function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gmail/test_attachment_fix.py` around lines 1 - 84, Tests in test_base64_* currently re-implement the stripping logic instead of exercising the production code; update the tests to call the real function (get_gmail_attachment_content) or extract the normalization into a small helper (e.g., normalize_base64 or strip_base64_line_endings) in production and import that into tests, then replace the inline .replace("\r","").replace("\n","") calls with calls to that function in the test cases (including the parametrized test and the large PDF-like test) so any future changes to get_gmail_attachment_content are covered by tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/gmail/test_attachment_fix.py`:
- Around line 7-17: The tests use the wrong base64 payloads: the string
"VGVz\ndA==" decodes to b"Test", not b"Testdata", causing failures in
test_base64_stripping_with_unix_newlines and the other listed tests; update each
failing test (test_base64_stripping_with_unix_newlines,
test_base64_stripping_with_windows_crlf,
test_base64_stripping_with_mixed_line_endings, test_various_line_endings) so the
embedded-newline base64 encodes the intended "Testdata" (replace the base64
literals with the correct base64 for "Testdata") or alternatively adjust the
expected bytes to b"Test" to match the current literal—ensure the cleaned = ...
replace("\r", "").replace("\n", "") step remains and the assertion compares to
the matching decoded bytes.
---
Nitpick comments:
In `@tests/gmail/test_attachment_fix.py`:
- Around line 59-75: The test_large_base64_with_line_breaks currently uses
pdf_header = b"%PDF-1.7" which base64-encodes to only ~12 chars so
pdf_base64_with_newlines never gets split; change the payload to be large enough
(e.g. repeat pdf_header or use a larger binary) so pdf_base64 becomes longer
than 76 chars, then regenerate pdf_base64_with_newlines, strip newlines into
cleaned, decode and assert the decoded bytes equal the original repeated
payload; update references in the test to pdf_header, pdf_base64,
pdf_base64_with_newlines and cleaned accordingly.
- Around line 1-84: Tests in test_base64_* currently re-implement the stripping
logic instead of exercising the production code; update the tests to call the
real function (get_gmail_attachment_content) or extract the normalization into a
small helper (e.g., normalize_base64 or strip_base64_line_endings) in production
and import that into tests, then replace the inline
.replace("\r","").replace("\n","") calls with calls to that function in the test
cases (including the parametrized test and the large PDF-like test) so any
future changes to get_gmail_attachment_content are covered by tests.
mickey-mikey
force-pushed
the
fix/windows-attachment-corruption
branch
2 times, most recently
from
February 20, 2026 04:06
f23af94 to
1ca75d2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gmail/gmail_tools.py (1)
902-910:⚠️ Potential issue | 🟡 MinorDeduplicate stateless-mode warning text.
Line 903 and Line 907 both emit the same warning, and Line 903 starts with a leading newline. This produces duplicate output and a stray blank line in the response.
🧹 Suggested fix
- result_lines = [ - "\n⚠️ Stateless mode: File storage disabled.", + result_lines = [ + "⚠️ Stateless mode: File storage disabled.", f"Message ID: {message_id}", f"Size: {size_kb:.1f} KB ({size_bytes} bytes)", - "\n⚠️ Stateless mode: File storage disabled.", "\nBase64-encoded content (first 100 characters shown):", f"{base64_data[:100]}...", "\nNote: Attachment IDs are ephemeral. Always use IDs from the most recent message fetch.", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gmail/gmail_tools.py` around lines 902 - 910, The result_lines list in gmail_tools.py currently includes the stateless-mode warning twice (elements around the f"Message ID: {message_id}" and the later "\n⚠️ Stateless mode: File storage disabled.") and one occurrence has a leading newline causing a blank line; update the result_lines construction to include the stateless-mode warning only once (remove the duplicate entry) and ensure the remaining warning string has no leading newline so it doesn't emit an extra blank line—modify the list where result_lines is defined to keep a single "⚠️ Stateless mode: File storage disabled." entry and remove the "\n" prefix from any preserved warning text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gmail/gmail_tools.py`:
- Around line 890-896: The assignment to base64_data contains unescaped literal
newlines in the string literals causing a SyntaxError; update the sanitization
to use escaped sequences instead of actual line breaks, e.g. call
attachment.get("data", "") and then .replace("\r", "").replace("\n", "") (or a
single .replace("\r\n", "") plus .replace("\n", "") to be safe) so the
base64_data variable is built from properly escaped "\r" and "\n" characters and
the code parses correctly.
---
Outside diff comments:
In `@gmail/gmail_tools.py`:
- Around line 902-910: The result_lines list in gmail_tools.py currently
includes the stateless-mode warning twice (elements around the f"Message ID:
{message_id}" and the later "\n⚠️ Stateless mode: File storage disabled.") and
one occurrence has a leading newline causing a blank line; update the
result_lines construction to include the stateless-mode warning only once
(remove the duplicate entry) and ensure the remaining warning string has no
leading newline so it doesn't emit an extra blank line—modify the list where
result_lines is defined to keep a single "⚠️ Stateless mode: File storage
disabled." entry and remove the "\n" prefix from any preserved warning text.
mickey-mikey
force-pushed
the
fix/windows-attachment-corruption
branch
3 times, most recently
from
February 20, 2026 04:24
d1ea154 to
9740b96
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gmail/gmail_tools.py`:
- Around line 894-895: Call to is_stateless_mode() in gmail_tools.py is missing
an import and will raise NameError; add an import for is_stateless_mode from its
defining module at the top of the file (alongside other imports) so the symbol
is available where it's used, e.g., import is_stateless_mode into gmail_tools.py
and run tests/lint to confirm no unresolved references remain.
mickey-mikey
force-pushed
the
fix/windows-attachment-corruption
branch
4 times, most recently
from
February 20, 2026 04:28
fdb7248 to
464197a
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
mickey-mikey
force-pushed
the
fix/windows-attachment-corruption
branch
from
February 20, 2026 04:51
8d36f62 to
464197a
Compare
mickey-mikey
changed the title
mickey-mikey
force-pushed
the
fix/windows-attachment-corruption
branch
from
February 22, 2026 00:05
95b6dd3 to
72e95f0
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/gmail/test_attachment_fix.py (2)
70-113: Consider usingtmp_pathfixture for better test isolation.The integration tests write to the default
STORAGE_DIR(~/.workspace-mcp/attachments), which modifies the user's home directory during test runs. Using pytest'stmp_pathfixture and monkeypatchingSTORAGE_DIRwould improve test isolation and avoid side effects.♻️ Example approach using tmp_path
`@pytest.fixture` def isolated_storage(tmp_path, monkeypatch): """Create an isolated AttachmentStorage with temp directory.""" import core.attachment_storage as storage_module monkeypatch.setattr(storage_module, "STORAGE_DIR", tmp_path) return storage_module.AttachmentStorage() def test_save_attachment_uses_binary_mode(isolated_storage): # Use isolated_storage instead of creating new AttachmentStorage() payload = b"\x89PNG\r\n\x1a\n" + b"\x00" * 100 b64_data = base64.urlsafe_b64encode(payload).decode() result = isolated_storage.save_attachment(b64_data, filename="test.png") # No manual cleanup needed - tmp_path handles it with open(result.path, "rb") as f: assert f.read() == payload🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gmail/test_attachment_fix.py` around lines 70 - 113, Update the tests to avoid writing to the user's home STORAGE_DIR by using pytest's tmp_path fixture and monkeypatch to override STORAGE_DIR in the module under test; specifically, create a fixture (e.g., isolated_storage) that monkeypatches core.attachment_storage.STORAGE_DIR to tmp_path and returns AttachmentStorage(), then replace direct instantiation in test_save_attachment_uses_binary_mode and test_save_attachment_preserves_various_binary_formats with that fixture (use isolated_storage) so files are written into the temporary directory and no manual cleanup is required.
52-54: Consider aligning the platform check style with production code.The test uses
hasattr(os, "O_BINARY")while production code usessys.platform == "win32". Both approaches work correctly, but using consistent patterns aids maintainability. Thehasattrapproach is slightly more defensive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gmail/test_attachment_fix.py` around lines 52 - 54, Change the platform check in the test so it matches production style: replace the hasattr(os, "O_BINARY") condition around setting flags with a sys.platform == "win32" check; ensure sys is imported in tests/gmail/test_attachment_fix.py and keep the rest of the flags logic (flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC and flags |= os.O_BINARY) unchanged, referencing the flags variable and os.O_BINARY symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/gmail/test_attachment_fix.py`:
- Around line 70-113: Update the tests to avoid writing to the user's home
STORAGE_DIR by using pytest's tmp_path fixture and monkeypatch to override
STORAGE_DIR in the module under test; specifically, create a fixture (e.g.,
isolated_storage) that monkeypatches core.attachment_storage.STORAGE_DIR to
tmp_path and returns AttachmentStorage(), then replace direct instantiation in
test_save_attachment_uses_binary_mode and
test_save_attachment_preserves_various_binary_formats with that fixture (use
isolated_storage) so files are written into the temporary directory and no
manual cleanup is required.
- Around line 52-54: Change the platform check in the test so it matches
production style: replace the hasattr(os, "O_BINARY") condition around setting
flags with a sys.platform == "win32" check; ensure sys is imported in
tests/gmail/test_attachment_fix.py and keep the rest of the flags logic (flags =
os.O_WRONLY | os.O_CREAT | os.O_TRUNC and flags |= os.O_BINARY) unchanged,
referencing the flags variable and os.O_BINARY symbol.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/gmail/test_attachment_fix.py (1)
24-43: Consider using pytest'stmp_pathfixture for cleaner cleanup.The manual temp file management works, but if
os.openfails before file creation,os.unlink(tmp)in the finally block will raiseFileNotFoundError. Using pytest'stmp_pathfixture handles cleanup automatically.♻️ Optional: Use tmp_path fixture
-def test_os_open_without_o_binary_corrupts_on_windows(): +def test_os_open_without_o_binary_corrupts_on_windows(tmp_path): """On Windows, os.open without O_BINARY translates LF to CRLF in written bytes.""" # PNG header contains 0x0a (LF) bytes that trigger text-mode translation payload = b"\x89PNG\r\n\x1a\n" + b"\x00" * 50 - tmp = os.path.join(tempfile.gettempdir(), "test_no_binary.bin") - try: - fd = os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) - try: - os.write(fd, payload) - finally: - os.close(fd) + tmp = tmp_path / "test_no_binary.bin" + fd = os.open(str(tmp), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + try: + os.write(fd, payload) + finally: + os.close(fd) - with open(tmp, "rb") as f: - written = f.read() + with open(tmp, "rb") as f: + written = f.read() - if sys.platform == "win32": - # Without O_BINARY, Windows inserts extra \r before each \n - assert written != payload, "Expected corruption without O_BINARY on Windows" - assert len(written) > len(payload) - else: - # Unix doesn't have this problem - assert written == payload - finally: - os.unlink(tmp) + if sys.platform == "win32": + # Without O_BINARY, Windows inserts extra \r before each \n + assert written != payload, "Expected corruption without O_BINARY on Windows" + assert len(written) > len(payload) + else: + # Unix doesn't have this problem + assert written == payload🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gmail/test_attachment_fix.py` around lines 24 - 43, The test manually creates tmp and calls os.unlink(tmp) in a finally block which can raise FileNotFoundError if os.open fails; switch to using pytest's tmp_path fixture to create the temporary file (e.g., tmp_path / "test_no_binary.bin") and use Path or open(..., "wb") to write/read payload so pytest handles cleanup automatically; replace references to tmp, os.open/os.unlink with the tmp_path Path object and regular file open calls to avoid the manual unlink and race conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/gmail/test_attachment_fix.py`:
- Around line 24-43: The test manually creates tmp and calls os.unlink(tmp) in a
finally block which can raise FileNotFoundError if os.open fails; switch to
using pytest's tmp_path fixture to create the temporary file (e.g., tmp_path /
"test_no_binary.bin") and use Path or open(..., "wb") to write/read payload so
pytest handles cleanup automatically; replace references to tmp,
os.open/os.unlink with the tmp_path Path object and regular file open calls to
avoid the manual unlink and race conditions.
mickey-mikey
force-pushed
the
fix/windows-attachment-corruption
branch
from
February 22, 2026 00:24
72e95f0 to
093b8f8
Compare
Fixes #494 - Windows attachment corruption for PNG/PDF files. On Windows, os.open() defaults to text mode, which translates LF (0x0a) bytes to CRLF (0x0d 0x0a) during os.write(). This corrupts any binary attachment containing 0x0a bytes (PNG headers, PDFs, etc.). The fix adds os.O_BINARY to the os.open() flags using the standard getattr(os, 'O_BINARY', 0) idiom, which returns the flag on Windows and 0 (no-op) on platforms where it doesn't exist. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mickey-mikey
force-pushed
the
fix/windows-attachment-corruption
branch
from
February 22, 2026 00:45
093b8f8 to
ffeba1f
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where binary file attachments were being corrupted on Windows due to text-mode file writes. The fix adds the O_BINARY flag to the os.open() call in the attachment storage module to ensure binary data is written correctly on Windows while maintaining compatibility with Unix-like systems.
Changes:
- Added
O_BINARYflag toos.open()call incore/attachment_storage.pyusing the standardgetattr(os, 'O_BINARY', 0)pattern - Added comprehensive test suite in
tests/gmail/test_attachment_fix.pycovering base64 decoding, binary file writes, and attachment storage
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/attachment_storage.py | Fixed Windows attachment corruption by adding O_BINARY flag to os.open() call |
| tests/gmail/test_attachment_fix.py | Added comprehensive tests verifying binary file handling across platforms |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import base64 | ||
| import os | ||
| import sys | ||
|
|
There was a problem hiding this comment.
This test file is missing the sys.path.insert pattern used by other test files in the codebase. Other test files (e.g., tests/core/test_comments.py, tests/gdrive/test_drive_tools.py) include sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "../.."))) before importing modules. This ensures the test can find the source modules regardless of how pytest is invoked. Consider adding this import pattern at the beginning of the file for consistency with the rest of the test suite.
Suggested change
| sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "../.."))) |
taylorwilsdon
approved these changes
Feb 24, 2026
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
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.
Fix Windows attachment corruption from text-mode file writes
Fixes #494
Type of Change
Root Cause
On Windows,
os.open()defaults to text mode unlessos.O_BINARYis explicitly set. In text mode,os.write()translates every\n(0x0a) byte into\r\n(0x0d 0x0a). This corrupts any binary attachment containing 0x0a bytes — which includes PNG headers (89 50 4e 47 0d 0a 1a 0a), PDFs, and most binary formats.The previous commit incorrectly attributed the corruption to base64 decoding and attempted to strip
\r/\nfrom the base64 string before decoding. This was a no-op — Python'sbase64.urlsafe_b64decode()already silently ignores whitespace characters.Proof (tested on Windows against 100 real files — PDF, JPG, DOCX, EML, ICS, XLS — all 100 corrupted without fix, 0 with fix):
os.openwithoutO_BINARY89504e470d**0d0a**1a**0d0a**...os.openwithO_BINARY89504e470d0a1a0a...Path.write_bytes()89504e470d0a1a0a...The
0d0d0apattern reported in #494 is exactly what text-mode write translation produces: the existing\r\n(0d 0a) in the PNG header has its\ndoubled to\r\n, yielding\r\r\n(0d 0d 0a).The Fix
Location:
core/attachment_storage.pyline 105 insave_attachment()Before:
After:
Uses the standard
getattr(os, 'O_BINARY', 0)idiom (same pattern used in CPython'szipfileandtarfile). Returns theO_BINARYflag on Windows,0(no-op) on platforms where it doesn't exist.Also reverted the no-op base64 stripping in
gmail/gmail_tools.pysinceurlsafe_b64decodealready handles whitespace.Testing
urlsafe_b64decodealready ignores\r/\n(no stripping needed)os.openwithoutO_BINARYcorrupts binary writes on Windowsos.openwithO_BINARYpreserves binary data correctlyAttachmentStorage.save_attachmentwith PNG payloadsImpact
getattr(os, 'O_BINARY', 0)evaluates to0, making it a no-op on these platformsSummary by CodeRabbit
Bug Fixes
Tests