From 117ef9e8277a3f80ce3b8580e824580825bac959 Mon Sep 17 00:00:00 2001
From: Sean Whalen <44679+seanthegeek@users.noreply.github.com>
Date: Sat, 25 Jul 2026 22:42:31 -0400
Subject: [PATCH] Address review feedback on docstrings and the
 placeholder-cleanup except

- _move_file_to_archive's docstring no longer claims the copy2 fallback
  replaces the placeholder atomically; only the same-filesystem os.rename
  path is atomic. The fallback is a plain copy-and-overwrite, which is
  still collision-safe because the placeholder already claimed the name.
- The empty except OSError in the placeholder cleanup now carries a
  comment explaining that it is deliberate best-effort cleanup and the
  re-raised move failure is the actionable error.
- test_general_archive_directory_unset_leaves_attribute_absent's
  docstring now describes what the assertion actually tests (the
  attribute staying absent from a bare Namespace) and moves the
  real-CLI None-default behavior to a parenthetical.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 parsedmarc/cli.py | 10 ++++++++--
 tests/test_cli.py |  7 +++++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/parsedmarc/cli.py b/parsedmarc/cli.py
index 3bd5b6ce..9844a986 100644
--- a/parsedmarc/cli.py
+++ b/parsedmarc/cli.py
@@ -314,8 +314,12 @@ def _move_file_to_archive(file_path: str, dest_dir: str) -> str:
     existing destination on POSIX, which would violate the
     never-overwrite guarantee. Instead, each candidate name is staked
     out with a zero-byte placeholder file before the real move happens;
-    ``os.rename`` (POSIX) or the ``copy2`` fallback (Windows) that
-    backs ``shutil.move()`` then replaces that placeholder atomically.
+    ``shutil.move()`` then replaces that placeholder with the real file
+    — atomically via ``os.rename`` when source and destination are on
+    the same POSIX filesystem, otherwise (Windows, or a cross-device
+    move) via a ``copy2``-and-overwrite that is not atomic but still
+    cannot collide with a concurrent invocation, since the placeholder
+    already claimed the name.
     """
     os.makedirs(dest_dir, exist_ok=True)
     base, ext = os.path.splitext(os.path.basename(file_path))
@@ -338,6 +342,8 @@ def _move_file_to_archive(file_path: str, dest_dir: str) -> str:
         try:
             os.remove(dest_path)
         except OSError:
+            # Best-effort cleanup of the just-created placeholder; the
+            # move failure re-raised below is the error that matters.
             pass
         raise
     return dest_path
diff --git a/tests/test_cli.py b/tests/test_cli.py
index cf49261d..b50404ca 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -4525,8 +4525,11 @@ class TestParseConfigGeneral(unittest.TestCase):
 
     def test_general_archive_directory_unset_leaves_attribute_absent(self):
         """When archive_directory is absent from the INI, _parse_config
-        never sets opts.archive_directory (the CLI's Namespace default of
-        None applies instead)."""
+        never sets opts.archive_directory at all — asserted here as the
+        attribute staying absent from this test's bare Namespace. (In the
+        real CLI the attribute pre-exists with the Namespace default of
+        None, so _parse_config leaving it untouched is what keeps
+        archiving disabled.)"""
         from parsedmarc.cli import _parse_config
 
         cp = _config_with("general", {"silent": "false"})