Skip to content
This repository was archived by the owner on May 6, 2026. It is now read-only.

fix: use os.replace to handle existing .apkg files on Windows #112

Merged
aalemayhu merged 2 commits into from
Apr 4, 2026
Merged

fix: use os.replace to handle existing .apkg files on Windows #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
596b93f
fix: use os.replace instead of os.rename to handle existing files on …
aalemayhu Apr 1, 2026
cc97cc4
fix: update mocks from os.rename to os.replace in write_apkg tests
aalemayhu Apr 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions helpers/write_apkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,16 +124,14 @@ def rename_temp_file(temp_path, sanitized_name, first_deck_id):
_verify_path_within_cwd(final_path, cwd)

try:
os.rename(temp_path, final_path)
os.replace(temp_path, final_path)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
except OSError as e:
if e.errno == 36:
fallback_filename = f"deck_{uuid.uuid4().hex[:8]}.apkg"
final_path = os.path.join(cwd, fallback_filename)
_verify_path_within_cwd(final_path, cwd)
os.rename(temp_path, final_path)
print(f"Warning: Filename too long. Saved as {final_path}")
os.replace(temp_path, final_path)
else:
print(f"Error renaming file: {e}")
raise ValueError("Failed to rename file") from e
Comment on lines 128 to 135

@coderabbitai coderabbitai Bot Apr 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd -type f -name "write_apkg.py"

Repository: 2anki/create_deck

Length of output: 231

🏁 Script executed:

find . -name "write_apkg.py" -type f

Repository: 2anki/create_deck

Length of output: 84

🏁 Script executed:

cat -n ./helpers/write_apkg.py | head -150

Repository: 2anki/create_deck

Length of output: 5844

🏁 Script executed:

python3 << 'EOF'
import errno
# Verify errno 36
print(f"errno 36 = {errno.errorcode.get(36, 'UNKNOWN')}")
EOF

Repository: 2anki/create_deck

Length of output: 84

Include paths and original error details in the ValueError exception message.

At line 135, non-errno == 36 errors are converted to a generic ValueError("Failed to rename file"). This masks actionable context (the temp_path and final_path being renamed, plus the original error details), which complicates triage. Both variables are in scope and should be included in the error message for better debuggability.

Proposed error-handling refinement 
     try:
         os.replace(temp_path, final_path)
     except OSError as e:
         if e.errno == 36:
             fallback_filename = f"deck_{uuid.uuid4().hex[:8]}.apkg"
             final_path = os.path.join(cwd, fallback_filename)
             _verify_path_within_cwd(final_path, cwd)
             os.replace(temp_path, final_path)
         else:
-            raise ValueError("Failed to rename file") from e
+            raise ValueError(
+                f"Failed to rename temp file '{temp_path}' to '{final_path}': {e}"
+            ) from e
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
except OSError as e:
if e.errno == 36:
fallback_filename = f"deck_{uuid.uuid4().hex[:8]}.apkg"
final_path = os.path.join(cwd, fallback_filename)
_verify_path_within_cwd(final_path, cwd)
os.rename(temp_path, final_path)
print(f"Warning: Filename too long. Saved as {final_path}")
os.replace(temp_path, final_path)
else:
print(f"Error renaming file: {e}")
raise ValueError("Failed to rename file") from e
except OSError as e:
if e.errno == 36:
fallback_filename = f"deck_{uuid.uuid4().hex[:8]}.apkg"
final_path = os.path.join(cwd, fallback_filename)
_verify_path_within_cwd(final_path, cwd)
os.replace(temp_path, final_path)
else:
raise ValueError(
f"Failed to rename temp file '{temp_path}' to '{final_path}': {e}"
) from e
🧰 Tools
🪛 GitHub Actions: Test

[error] 127-135: File rename in rename_temp_file failed: FileNotFoundError when executing os.replace(temp_path, final_path), temp file does not exist. Exception is caught and re-raised as ValueError("Failed to rename file").

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@helpers/write_apkg.py` around lines 128 - 135, The ValueError raised in the
OSError handler inside helpers/write_apkg.py loses useful context; update the
exception raised in the else branch to include temp_path, final_path (both in
scope) and the original error details (e) in the error message so callers can
see which files were involved and the underlying OS error; preserve the
raise-from semantics (raise ValueError(...) from e) and keep the existing
_verify_path_within_cwd / os.replace flow intact.


return final_path
Expand Down
26 changes: 13 additions & 13 deletions tests/test_write_apkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ def test_sanitize_filename(self):
self.assertEqual(sanitize_filename(input_name), expected)

@mock.patch('helpers.write_apkg.Package')
@mock.patch('os.rename')
def test_write_new_apkg_single_deck(self, mock_rename, mock_package):
@mock.patch('helpers.write_apkg.os.replace')
def test_write_new_apkg_single_deck(self, mock_replace, mock_package):
note = Note(model=self.test_model, fields=['Q1', 'A1'])
deck_payload = {
"id": 1234567890,
Expand All @@ -49,11 +49,11 @@ def test_write_new_apkg_single_deck(self, mock_rename, mock_package):

# Verify file operations
mock_package.return_value.write_to_file.assert_called_once()
mock_rename.assert_called_once()
mock_replace.assert_called_once()

@mock.patch('helpers.write_apkg.Package')
@mock.patch('os.rename')
def test_write_new_apkg_multiple_decks(self, mock_rename, mock_package):
@mock.patch('helpers.write_apkg.os.replace')
def test_write_new_apkg_multiple_decks(self, mock_replace, mock_package):
note1 = Note(model=self.test_model, fields=['Q1', 'A1'])
note2 = Note(model=self.test_model, fields=['Q2', 'A2'])

Expand Down Expand Up @@ -84,11 +84,11 @@ def test_write_new_apkg_multiple_decks(self, mock_rename, mock_package):

# Verify file operations
mock_package.return_value.write_to_file.assert_called_once()
mock_rename.assert_called_once()
mock_replace.assert_called_once()

@mock.patch('helpers.write_apkg.Package')
@mock.patch('os.rename')
def test_write_new_apkg_with_media(self, mock_rename, mock_package):
@mock.patch('helpers.write_apkg.os.replace')
def test_write_new_apkg_with_media(self, mock_replace, mock_package):
note = Note(model=self.test_model, fields=['Q1', 'A1'])
deck_payload = {
"id": 1234567890,
Expand All @@ -108,21 +108,21 @@ def test_write_new_apkg_with_media(self, mock_rename, mock_package):

# Verify file operations
mock_package.return_value.write_to_file.assert_called_once()
mock_rename.assert_called_once()
mock_replace.assert_called_once()

@mock.patch('helpers.write_apkg.Package')
@mock.patch('os.rename')
def test_write_new_apkg_empty_deck_list(self, mock_rename, mock_package):
@mock.patch('helpers.write_apkg.os.replace')
def test_write_new_apkg_empty_deck_list(self, mock_replace, mock_package):
with tempfile.TemporaryDirectory() as tmpdir:
with mock.patch('os.getcwd', return_value=tmpdir):
_write_new_apkg([], [])

mock_package.assert_called_once()
mock_package.return_value.write_to_file.assert_called_once()
mock_rename.assert_called_once()
mock_replace.assert_called_once()

# Verify the default name is used
_, dst = mock_rename.call_args[0]
_, dst = mock_replace.call_args[0]
self.assertIn("default-", dst)

def test_sanitize_filename_security(self):
Expand Down
Loading