From ec62c63e683d44820b959a5d64662490e7f259db Mon Sep 17 00:00:00 2001 From: nelsonduarte Date: Tue, 11 Aug 2026 13:17:35 +0100 Subject: [PATCH 1/3] refactor(editor): reuse shared atomic PDF write in TabEditar._run Extract BasePage._atomic_pdf_write / _check_not_same_path into a pure low-level app/pdf_io module and route both BasePage (all tools) and TabEditar._run through it, removing the duplicated mkstemp + doc.save + os.replace block in the editor's save path. - app/pdf_io.py: new pure module (stdlib + lazy app.i18n import); adds an opt-in close_writer flag that preserves the editor's original save -> close -> replace ordering, needed on Windows when the output overwrites the same file the document was opened from. - app/base.py: _atomic_pdf_write / _check_not_same_path become thin delegating staticmethods (signatures unchanged, so the ~20 call sites and the regression tests are untouched); drop the now-unused contextlib import. - app/editor/tab.py: _run builds save_opts via dict(...) and calls the shared atomic_pdf_write with close_writer=True. Encryption behaviour (AES-256, owner==user, permissions) is byte-for-byte identical. Behaviour is unchanged: plain and AES-256 save, temp cleanup on error, the outer show_error path, and the close ordering all match. _apply_forms keeps its own pypdf write for now (a follow-up increment). Tests: retarget the os.replace monkeypatch to app.pdf_io (the code moved there); add tests/test_editor_atomic_write.py covering plain + AES-256 round-trip, wrong-password rejection, close_writer ordering, verbatim save_opts forwarding and the same-source guard. Co-Authored-By: Claude Opus 4.8 --- app/base.py | 75 ++++------- app/editor/tab.py | 65 +++++----- app/pdf_io.py | 124 ++++++++++++++++++ tests/test_atomic_pdf_write.py | 7 +- tests/test_editor_atomic_write.py | 200 ++++++++++++++++++++++++++++++ 5 files changed, 388 insertions(+), 83 deletions(-) create mode 100644 app/pdf_io.py create mode 100644 tests/test_editor_atomic_write.py diff --git a/app/base.py b/app/base.py index bf5a95d..49d9c0d 100644 --- a/app/base.py +++ b/app/base.py @@ -1,6 +1,5 @@ """PDFApps – BasePage: standard page layout (header + scroll + action bar).""" -import contextlib import os import subprocess import sys @@ -13,6 +12,7 @@ from PySide6.QtWidgets import (QWidget, QVBoxLayout, QHBoxLayout, QFileDialog, QPushButton, QLabel) +from app import pdf_io from app.constants import DESKTOP, ACCENT from app.i18n import t from app.utils import ToolHeader, ActionBar, scrolled, _paint_bg @@ -413,19 +413,15 @@ def _check_not_same_path(dst: str, input and output, opening the output for writing truncates the input before the writer's lazy stream reads complete and we get silent dataloss + corrupted output. + + Thin wrapper around :func:`app.pdf_io.check_not_same_path` + (R3): the logic now lives in the low-level ``pdf_io`` module so + the visual editor can reuse the exact same guard without + importing this Qt-heavy page base. Kept as a ``@staticmethod`` + so ``BasePage._check_not_same_path`` / ``self._check_not_same_path`` + call sites and the unit tests stay unchanged. """ - try: - dst_real = os.path.realpath(dst) - except OSError: - return - for src in (sources or ()): - if not src: - continue - try: - if os.path.realpath(src) == dst_real: - raise RuntimeError(t("tool.err.same_source_output")) - except OSError: - continue + pdf_io.check_not_same_path(dst, sources) @staticmethod def _atomic_pdf_write(writer, dst: str, *, @@ -433,11 +429,15 @@ def _atomic_pdf_write(writer, dst: str, *, save_opts: "dict | None" = None) -> None: """Write a PdfWriter (pypdf) or fitz.Document to ``dst`` atomically. - Two defensive layers fix the silent dataloss bug where opening - ``open(dst, "wb")`` truncates the input file BEFORE the writer's - lazy stream reads complete (PdfWriter holds references into - the PdfReader; same applies to fitz.Document.save() with - incremental flags). + Thin wrapper around :func:`app.pdf_io.atomic_pdf_write` (R3): + the tempfile + ``os.replace`` + same-source-guard logic now + lives in the low-level ``pdf_io`` module so ``TabEditar._run`` + reuses the identical write path instead of duplicating it. + Kept as a ``@staticmethod`` with the same signature so the + ~20 ``self._atomic_pdf_write`` / ``BasePage._atomic_pdf_write`` + call sites and the regression tests are untouched. + + The two defensive layers are unchanged: 1. Reject up-front if ``dst`` resolves to any path in ``sources`` (via ``os.path.realpath``) — this catches the @@ -449,44 +449,17 @@ def _atomic_pdf_write(writer, dst: str, *, ``writer`` may be a pypdf ``PdfWriter`` (uses ``writer.write(fh)``) or a PyMuPDF ``fitz.Document`` (uses ``writer.save(tmp)``). - Anything else with a ``.write(fh)`` method is accepted. + Anything else with a ``.write(fh)`` method is accepted. The + writer is left OPEN (BasePage tools never save back onto the + input handle); the editor opts into ``close_writer`` directly + via ``pdf_io.atomic_pdf_write``. Raises :class:`RuntimeError` with a translated message when the same-source check fails; the caller's existing ``show_error`` path surfaces it as a friendly dialog. """ - BasePage._check_not_same_path(dst, sources) - - dst_dir = os.path.dirname(dst) or os.getcwd() - # mkstemp returns an OS-level fd; close via os.fdopen so the - # writer can stream into it. Same-volume placement guarantees - # os.replace() stays atomic. - fd, tmp = tempfile.mkstemp(suffix=".pdf", dir=dst_dir) - # Detect fitz.Document via its module to avoid importing fitz - # at base.py load time (every page imports BasePage). Modern - # PyMuPDF reports module="pymupdf"; legacy versions used "fitz". - # Both expose Document.save(path, ...). - writer_mod = type(writer).__module__ - is_fitz_doc = (writer_mod.startswith("pymupdf") - or writer_mod.startswith("fitz")) and hasattr(writer, "save") - try: - if is_fitz_doc: - # fitz.Document.save(path, ...) accepts a filesystem - # path and writes through cleanly. We close the fd - # we opened first so save() can take exclusive access. - os.close(fd) - writer.save(tmp, **(save_opts or {})) - else: - # pypdf.PdfWriter (and anything else with .write(fh)) - # streams into the open file handle. - with os.fdopen(fd, "wb") as fh: - writer.write(fh) - os.replace(tmp, dst) - except Exception: - with contextlib.suppress(Exception): - if os.path.exists(tmp): - os.unlink(tmp) - raise + pdf_io.atomic_pdf_write(writer, dst, sources=sources, + save_opts=save_opts) # ── background-task helper ──────────────────────────────────────────── diff --git a/app/editor/tab.py b/app/editor/tab.py index 5aeee28..706e06a 100644 --- a/app/editor/tab.py +++ b/app/editor/tab.py @@ -25,6 +25,7 @@ from app.editor.canvas import PdfEditCanvas, _get_icon_cursor from app.editor.dialogs import _NoteDialog from app.editor.apply_edits import apply_pending_edits +from app.pdf_io import atomic_pdf_write _log = logging.getLogger(__name__) @@ -1266,37 +1267,39 @@ def _run(self): # unexplained illegibly-shrunk line. _apply_result = apply_pending_edits(doc, self._pending) text_fit_warnings = _apply_result.text_fit_warnings - fd, tmp = tempfile.mkstemp(prefix=".pdfapps_save_", suffix=".pdf", - dir=os.path.dirname(out) or ".") - os.close(fd) - try: - if encrypt_choice == "protect" and self._pdf_password: - # Documented limitation: owner_pw == user_pw because we - # only captured a single password from the load prompt - # — the original owner password is not recoverable from - # the input file. Future enhancement: ask the user for - # a separate owner password. - # ``_fitz_permissions_of`` already returns -1 on any - # internal failure (PyMuPDF sentinel for "all perms"), - # so a wrapping try/except here would be dead code. - perms = self._fitz_permissions_of(doc) - doc.save( - tmp, garbage=4, deflate=True, - encryption=fitz.PDF_ENCRYPT_AES_256, - user_pw=self._pdf_password, - owner_pw=self._pdf_password, - permissions=perms, - ) - _log.info( - "Re-encrypted output with user password as owner") - else: - doc.save(tmp, garbage=4, deflate=True) - doc.close() - os.replace(tmp, out) - except Exception: - try: os.unlink(tmp) - except OSError: pass - raise + # Atomic save via the shared low-level writer (R3): the exact + # tempfile + os.replace path every BasePage tool already uses, + # reused here instead of a duplicated mkstemp/save/replace + # block. ``close_writer=True`` preserves the editor's original + # save→close→replace ordering — ``doc`` was opened from + # ``self._doc_path`` so on Windows its handle must be released + # before the rename (the user may be overwriting the input). + # ``save_opts`` is built with ``dict(...)`` so the encryption + # kwargs (encryption=…, user_pw=…, owner_pw=…) read exactly as + # the direct ``doc.save`` call did before. + reencrypt = bool(encrypt_choice == "protect" and self._pdf_password) + if reencrypt: + # Documented limitation: owner_pw == user_pw because we + # only captured a single password from the load prompt + # — the original owner password is not recoverable from + # the input file. Future enhancement: ask the user for + # a separate owner password. + # ``_fitz_permissions_of`` already returns -1 on any + # internal failure (PyMuPDF sentinel for "all perms"), + # so a wrapping try/except here would be dead code. + perms = self._fitz_permissions_of(doc) + save_opts = dict( + garbage=4, deflate=True, + encryption=fitz.PDF_ENCRYPT_AES_256, + user_pw=self._pdf_password, + owner_pw=self._pdf_password, + permissions=perms, + ) + else: + save_opts = dict(garbage=4, deflate=True) + atomic_pdf_write(doc, out, save_opts=save_opts, close_writer=True) + if reencrypt: + _log.info("Re-encrypted output with user password as owner") self._pending.clear(); self._pending_list.clear() self._status(t("edit.status.saved", path=out)) if text_fit_warnings: diff --git a/app/pdf_io.py b/app/pdf_io.py new file mode 100644 index 0000000..0cf307f --- /dev/null +++ b/app/pdf_io.py @@ -0,0 +1,124 @@ +"""PDFApps – pdf_io: low-level atomic PDF write helpers. + +Extracted from :class:`app.base.BasePage` (R3) so the same safe-write +logic is shared by BOTH the tool pages (via +``BasePage._atomic_pdf_write``) and the visual editor +(``TabEditar._run``) WITHOUT either side re-implementing the tempfile + +``os.replace`` dance, the same-source guard, or the fitz/pypdf writer +dispatch. + +This module is deliberately pure and low-level: it imports only stdlib +at load time (``app.i18n`` is imported lazily inside the one function +that needs a translated message). It pulls in no Qt and no other app +module, so it sits at the bottom of the dependency graph and is safe to +import from ``app.base`` and ``app.editor.tab`` alike with no import +cycle. +""" + +import contextlib +import os +import tempfile +from typing import Iterable + +__all__ = ["atomic_pdf_write", "check_not_same_path"] + + +def check_not_same_path(dst: str, + sources: "Iterable[str] | None" = None) -> None: + """Raise RuntimeError if ``dst`` resolves to any of ``sources``. + + Shared invariant for every tool that takes a PDF in and writes + a result back to disk: if the user picks the same path for + input and output, opening the output for writing truncates the + input before the writer's lazy stream reads complete and we + get silent dataloss + corrupted output. + """ + from app.i18n import t + try: + dst_real = os.path.realpath(dst) + except OSError: + return + for src in (sources or ()): + if not src: + continue + try: + if os.path.realpath(src) == dst_real: + raise RuntimeError(t("tool.err.same_source_output")) + except OSError: + continue + + +def atomic_pdf_write(writer, dst: str, *, + sources: "Iterable[str] | None" = None, + save_opts: "dict | None" = None, + close_writer: bool = False) -> None: + """Write a PdfWriter (pypdf) or fitz.Document to ``dst`` atomically. + + Two defensive layers fix the silent dataloss bug where opening + ``open(dst, "wb")`` truncates the input file BEFORE the writer's + lazy stream reads complete (PdfWriter holds references into + the PdfReader; same applies to fitz.Document.save() with + incremental flags). + + 1. Reject up-front if ``dst`` resolves to any path in + ``sources`` (via ``os.path.realpath``) — this catches the + "user picked the same path for input and output" case which + was producing corrupt output + losing the original. + + 2. Write to a same-directory tempfile and atomically rename to + ``dst`` via :func:`os.replace` (works on POSIX and Windows). + + ``writer`` may be a pypdf ``PdfWriter`` (uses ``writer.write(fh)``) + or a PyMuPDF ``fitz.Document`` (uses ``writer.save(tmp)``). + Anything else with a ``.write(fh)`` method is accepted. + + ``close_writer`` closes the writer AFTER a successful save but + BEFORE ``os.replace``. The visual editor needs this: it opens the + document from the SAME file it may be overwriting, so on Windows + the handle must be released before the rename or ``os.replace`` + fails with a sharing violation. The tool pages leave the writer + open (default ``False``) — they never save back onto the input + handle, and some reuse the writer/doc after the call. + + Raises :class:`RuntimeError` with a translated message when the + same-source check fails; the caller's existing ``show_error`` + path surfaces it as a friendly dialog. + """ + check_not_same_path(dst, sources) + + dst_dir = os.path.dirname(dst) or os.getcwd() + # mkstemp returns an OS-level fd; close via os.fdopen so the + # writer can stream into it. Same-volume placement guarantees + # os.replace() stays atomic. + fd, tmp = tempfile.mkstemp(suffix=".pdf", dir=dst_dir) + # Detect fitz.Document via its module to avoid importing fitz + # here (this module is imported by every page through BasePage). + # Modern PyMuPDF reports module="pymupdf"; legacy versions used + # "fitz". Both expose Document.save(path, ...). + writer_mod = type(writer).__module__ + is_fitz_doc = (writer_mod.startswith("pymupdf") + or writer_mod.startswith("fitz")) and hasattr(writer, "save") + try: + if is_fitz_doc: + # fitz.Document.save(path, ...) accepts a filesystem + # path and writes through cleanly. We close the fd + # we opened first so save() can take exclusive access. + os.close(fd) + writer.save(tmp, **(save_opts or {})) + else: + # pypdf.PdfWriter (and anything else with .write(fh)) + # streams into the open file handle. + with os.fdopen(fd, "wb") as fh: + writer.write(fh) + # Release the writer's file lock (if any) before the rename — + # see the ``close_writer`` note in the docstring. Placed + # between save and replace to preserve the editor's original + # save→close→replace ordering exactly. + if close_writer: + writer.close() + os.replace(tmp, dst) + except Exception: + with contextlib.suppress(Exception): + if os.path.exists(tmp): + os.unlink(tmp) + raise diff --git a/tests/test_atomic_pdf_write.py b/tests/test_atomic_pdf_write.py index b055fbf..5d34d3d 100644 --- a/tests/test_atomic_pdf_write.py +++ b/tests/test_atomic_pdf_write.py @@ -128,7 +128,12 @@ def spy(a, b): calls.append((a, b)) return real_replace(a, b) - monkeypatch.setattr("app.base.os.replace", spy) + # R3: the atomic-rename logic moved from BasePage into the low-level + # app.pdf_io module (BasePage._atomic_pdf_write now delegates to it), + # so os.replace runs in the pdf_io namespace. Patch it there — the + # assertion is otherwise unchanged: calling BasePage._atomic_pdf_write + # must still ultimately invoke os.replace for the atomic rename. + monkeypatch.setattr("app.pdf_io.os.replace", spy) BasePage._atomic_pdf_write(w, str(dst), sources=[str(src)]) assert calls, "os.replace was not called — the rename isn't atomic" assert calls[-1][1] == str(dst) diff --git a/tests/test_editor_atomic_write.py b/tests/test_editor_atomic_write.py new file mode 100644 index 0000000..e7e43b9 --- /dev/null +++ b/tests/test_editor_atomic_write.py @@ -0,0 +1,200 @@ +"""Behavioural tests for the shared atomic PDF writer as the editor uses it. + +R3 unified ``TabEditar._run``'s save (a duplicated mkstemp/``doc.save``/ +``os.replace`` block) onto :func:`app.pdf_io.atomic_pdf_write` — the same +low-level helper every ``BasePage`` tool already used. These tests drive +that helper with the EXACT ``save_opts`` shapes ``_run`` builds (plain +and AES-256 re-encryption with a permissions flag) plus the +``close_writer=True`` ordering the editor relies on, proving the save +behaviour is preserved without instantiating a Qt widget. + +Run with ``QT_QPA_PLATFORM=offscreen`` (``app.pdf_io`` imports ``app.i18n`` +lazily for its one error message, which needs a QCoreApplication). +""" + +import os +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") +from PySide6.QtWidgets import QApplication # noqa: E402 +_app = QApplication.instance() or QApplication([]) + +pymupdf = pytest.importorskip("pymupdf") +fitz = pymupdf + +from app.pdf_io import atomic_pdf_write # noqa: E402 + + +# ── helpers ────────────────────────────────────────────────────────────── + + +def _make_doc(marker: str = "Hello editor", pages: int = 2): + """A small text-bearing document, unsaved (in memory).""" + doc = fitz.open() + for i in range(pages): + page = doc.new_page(width=595, height=842) + page.insert_text((72, 72), f"{marker} {i}", fontsize=14) + return doc + + +def _plain_opts() -> dict: + """Exactly what TabEditar._run builds for a non-encrypted save.""" + return dict(garbage=4, deflate=True) + + +def _encrypted_opts(password: str, perms: int = -1) -> dict: + """Exactly what TabEditar._run builds for the AES-256 re-encrypt save + (owner_pw == user_pw, permissions read from the source doc).""" + return dict( + garbage=4, deflate=True, + encryption=fitz.PDF_ENCRYPT_AES_256, + user_pw=password, + owner_pw=password, + permissions=perms, + ) + + +# ── plain save (the common editor path) ────────────────────────────────── + + +def test_editor_plain_save_produces_valid_pdf(tmp_path: Path): + out = tmp_path / "out.pdf" + doc = _make_doc("Plain save", pages=3) + atomic_pdf_write(doc, str(out), save_opts=_plain_opts(), close_writer=True) + + assert out.exists() + reopened = fitz.open(str(out)) + try: + assert reopened.page_count == 3 + # PyMuPDF reports needs_pass as an int (0/1), so compare + # truthiness rather than the ``bool`` singleton. + assert not reopened.needs_pass + assert "Plain save 0" in reopened[0].get_text() + assert "Plain save 2" in reopened[2].get_text() + finally: + reopened.close() + + +def test_editor_close_writer_closes_doc_before_return(tmp_path: Path): + """close_writer=True must leave the fitz doc closed once the helper + returns — this is what let the editor overwrite the same file it had + open (the handle is released before os.replace).""" + out = tmp_path / "out.pdf" + doc = _make_doc() + atomic_pdf_write(doc, str(out), save_opts=_plain_opts(), close_writer=True) + assert doc.is_closed is True + + +def test_default_leaves_writer_open_for_base_tools(tmp_path: Path): + """The BasePage default (close_writer omitted) must NOT close the doc — + tool pages keep it alive and close it themselves.""" + out = tmp_path / "out.pdf" + doc = _make_doc() + try: + atomic_pdf_write(doc, str(out), save_opts=_plain_opts()) + assert doc.is_closed is False + finally: + doc.close() + + +# ── AES-256 re-encryption (the encrypted editor path) ──────────────────── + + +def test_editor_aes256_save_round_trips_with_password(tmp_path: Path): + out = tmp_path / "secret.pdf" + pw = "Corr3ct-Horse" + doc = _make_doc("Secret body", pages=2) + atomic_pdf_write(doc, str(out), + save_opts=_encrypted_opts(pw), close_writer=True) + + assert out.exists() + # Output is genuinely encrypted: opening without the password locks it. + # needs_pass / authenticate return ints (1/0) in PyMuPDF — compare + # truthiness, not the bool singletons. + locked = fitz.open(str(out)) + try: + assert locked.needs_pass + assert locked.authenticate(pw) # correct password unlocks (non-zero) + assert locked.page_count == 2 + assert "Secret body 0" in locked[0].get_text() + assert "Secret body 1" in locked[1].get_text() + finally: + locked.close() + + +def test_editor_aes256_rejects_wrong_password(tmp_path: Path): + out = tmp_path / "secret.pdf" + doc = _make_doc("Body", pages=1) + atomic_pdf_write(doc, str(out), + save_opts=_encrypted_opts("right-pw"), close_writer=True) + + locked = fitz.open(str(out)) + try: + assert locked.needs_pass + assert locked.authenticate("wrong-pw") == 0 # wrong password fails + finally: + locked.close() + + +def test_save_opts_forwarded_verbatim_to_fitz_writer(tmp_path: Path): + """The helper must forward ``save_opts`` VERBATIM as kwargs to a + fitz-style writer's ``save()`` — that is precisely what carries the + editor's encryption / user_pw / owner_pw / permissions options + through untouched. Asserted deterministically with a recording + stand-in (real PyMuPDF's signed permissions int makes an on-disk + round-trip brittle, and ``owner_pw == user_pw`` grants owner rights + that mask the restricted flag). ``close_writer=True`` must also fire + between save and rename.""" + out = tmp_path / "o.pdf" + + class _RecordingDoc: + def __init__(self): + self.saved_kwargs = None + self.closed = False + + def save(self, path, **kw): + self.saved_kwargs = dict(kw) + with open(path, "wb") as fh: + fh.write(b"%PDF-1.7\n%%EOF\n") + + def close(self): + self.closed = True + + # The helper detects a fitz.Document by its class module — spoof it so + # this pure stand-in takes the same ``writer.save(tmp, **save_opts)`` + # branch the editor's real fitz doc does. + _RecordingDoc.__module__ = "pymupdf" + + doc = _RecordingDoc() + opts = dict(garbage=4, deflate=True, encryption=99, + user_pw="u", owner_pw="o", permissions=1234) + atomic_pdf_write(doc, str(out), save_opts=opts, close_writer=True) + + assert doc.saved_kwargs == opts # nothing dropped, added or mutated + assert doc.closed is True # close_writer honoured before rename + assert out.exists() + + +# ── same-source guard still active on the fitz + close_writer path ─────── + + +def test_editor_save_rejects_same_source_and_preserves_input(tmp_path: Path): + """Even with close_writer=True, the up-front same-source check must + fire BEFORE any bytes are written, leaving the input intact.""" + src = tmp_path / "in.pdf" + _make_doc("Original", pages=2).save(str(src)) + before = src.read_bytes() + + doc = fitz.open(str(src)) + try: + with pytest.raises(RuntimeError): + atomic_pdf_write(doc, str(src), sources=[str(src)], + save_opts=_plain_opts(), close_writer=True) + finally: + doc.close() + assert src.read_bytes() == before From a59b0935868c016a7cb448292a0b4b22994e1e7b Mon Sep 17 00:00:00 2001 From: nelsonduarte Date: Tue, 11 Aug 2026 13:42:50 +0100 Subject: [PATCH 2/3] test(editor): assert writer closed before os.replace in atomic write The existing close_writer tests only inspect doc.is_closed after atomic_pdf_write returns, so they cannot catch a regression that moved writer.close() to AFTER os.replace: the doc ends up closed either way. That reorder is load-bearing on Windows (the editor overwrites the file it has open, so the handle must be released before the rename or os.replace raises a sharing violation) yet silent on POSIX CI. Pin the ordering portably by monkeypatching the os.replace app.pdf_io calls with a spy that records doc.is_closed at the instant the rename fires, then delegates to the real replace. One test asserts the fitz handle is already closed at that instant (close_writer=True), the inverse asserts it is still open under the BasePage default. Co-Authored-By: Claude Opus 4.8 --- tests/test_editor_atomic_write.py | 64 +++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/test_editor_atomic_write.py b/tests/test_editor_atomic_write.py index e7e43b9..027e1af 100644 --- a/tests/test_editor_atomic_write.py +++ b/tests/test_editor_atomic_write.py @@ -102,6 +102,70 @@ def test_default_leaves_writer_open_for_base_tools(tmp_path: Path): doc.close() +# ── close-before-replace ORDERING (the Windows overwrite invariant) ────── +# +# The two tests above only inspect ``doc.is_closed`` AFTER the helper +# returns. That cannot distinguish the correct save→close→replace order +# from a regression that moved ``writer.close()`` to AFTER ``os.replace``: +# in both cases the doc ends up closed on return. On Windows the editor +# overwrites the very file it has open, so the handle MUST be released +# before the rename or ``os.replace`` raises a sharing violation — but on +# POSIX (the CI) that reorder is silent and would slip through. +# +# These tests pin the ordering PORTABLY by probing the writer's state at +# the instant ``os.replace`` fires: we monkeypatch the ``os.replace`` that +# ``app.pdf_io`` actually calls with a spy that records ``doc.is_closed`` +# as it runs, then delegates to the real rename so the write still lands. + + +def test_close_writer_true_closes_doc_before_os_replace(tmp_path: Path, + monkeypatch): + """close_writer=True: the fitz handle is already released at the exact + moment os.replace is invoked. Falsifies a save→replace→close reorder, + which would observe an OPEN doc here and fail the assertion (while the + weaker 'closed on return' test would still pass).""" + out = tmp_path / "out.pdf" + doc = _make_doc() + + real_replace = os.replace + seen: dict = {} + + def _spy(src, dst): + seen["closed_at_replace"] = doc.is_closed + return real_replace(src, dst) + + monkeypatch.setattr(os, "replace", _spy) + atomic_pdf_write(doc, str(out), save_opts=_plain_opts(), close_writer=True) + + assert seen["closed_at_replace"] is True # closed BEFORE the rename + assert out.exists() # rename still completed + + +def test_default_leaves_writer_open_at_os_replace(tmp_path: Path, + monkeypatch): + """The inverse contract: with close_writer omitted (BasePage default) + the writer is STILL OPEN when os.replace fires — tool pages own the + doc's lifetime. Probed at the same instant so the pair fixes the exact + relationship between close_writer and the handle state at the rename.""" + out = tmp_path / "out.pdf" + doc = _make_doc() + + real_replace = os.replace + seen: dict = {} + + def _spy(src, dst): + seen["closed_at_replace"] = doc.is_closed + return real_replace(src, dst) + + monkeypatch.setattr(os, "replace", _spy) + try: + atomic_pdf_write(doc, str(out), save_opts=_plain_opts()) + assert seen["closed_at_replace"] is False # still open at the rename + assert out.exists() + finally: + doc.close() + + # ── AES-256 re-encryption (the encrypted editor path) ──────────────────── From 59d3bbc1e81347cd3d7b6ce4ca064ba8920f820d Mon Sep 17 00:00:00 2001 From: nelsonduarte Date: Tue, 11 Aug 2026 14:36:36 +0100 Subject: [PATCH 3/3] test: rename unused QApplication holder to satisfy CodeQL Rename the module-level QApplication holder from _app to _unused_app in tests/test_editor_atomic_write.py, matching the existing repo convention (tests/test_encrypted_pdf_tools.py). The variable only exists to keep a live QApplication for the Qt-backed lazy i18n import; the underscore- unused name preserves that side effect while resolving CodeQL alert #304 (Unused global variable). Co-Authored-By: Claude Opus 4.8 --- tests/test_editor_atomic_write.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_editor_atomic_write.py b/tests/test_editor_atomic_write.py index 027e1af..c7ed0d9 100644 --- a/tests/test_editor_atomic_write.py +++ b/tests/test_editor_atomic_write.py @@ -22,7 +22,7 @@ os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") from PySide6.QtWidgets import QApplication # noqa: E402 -_app = QApplication.instance() or QApplication([]) +_unused_app = QApplication.instance() or QApplication([]) pymupdf = pytest.importorskip("pymupdf") fitz = pymupdf