From f62e1a7881e67e74c625d90eaf49080bd5531571 Mon Sep 17 00:00:00 2001 From: Nora Date: Tue, 7 Jul 2026 12:24:35 -0600 Subject: [PATCH] fix(sp37-followup): add SftpClient.stat() method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SftpClient wrapper previously exposed write_file, list_inbound, list_inbound_names, download_inbound, and read_file — but NO stat(). The canonical cyclone.submission.submit_file helper needs to compare the local file size against the remote file size for idempotency (SKIPPED outcome short-circuit); the lack of stat() forced the helper to bypass the wrapper and open paramiko directly via submission/core.py:_default_sftp_factory. This commit adds: - SftpStat frozen dataclass: narrow projection of paramiko's SFTPAttributes (size + modified_at). Callers don't need paramiko. - SftpClient.stat(remote_path) -> SftpStat public method - _stat_stub + _stat_paramiko backends (mirrors the existing _read_file_stub / _read_file_paramiko pattern) - 8 new tests covering dataclass shape, frozen invariant, stub size, stub mtime, missing-file FileNotFoundError, large-file size, and the actual SKIPPED-outcome idempotency check from submit_file Also adds backend/var/ to .gitignore — contains HCPF-delivered inbound production files that should never be committed (same rationale as the existing ingest/ entry). --- backend/.gitignore | 1 + backend/src/cyclone/clearhouse/__init__.py | 73 ++++++++++ backend/tests/test_sftp_client_stat.py | 150 +++++++++++++++++++++ 3 files changed, 224 insertions(+) create mode 100644 backend/tests/test_sftp_client_stat.py diff --git a/backend/.gitignore b/backend/.gitignore index cd7bf33..776c4d7 100644 --- a/backend/.gitignore +++ b/backend/.gitignore @@ -7,3 +7,4 @@ __pycache__/ venv/ build/ dist/ +var/ diff --git a/backend/src/cyclone/clearhouse/__init__.py b/backend/src/cyclone/clearhouse/__init__.py index 56e6069..ff6445c 100644 --- a/backend/src/cyclone/clearhouse/__init__.py +++ b/backend/src/cyclone/clearhouse/__init__.py @@ -94,6 +94,22 @@ def _op_timeout_seconds() -> float: return value +@dataclass(frozen=True) +class SftpStat: + """File metadata returned by :meth:`SftpClient.stat`. + + Mirrors the subset of paramiko's ``SFTPAttributes`` that callers + actually need (``size``, ``modified_at``). Keeping the surface + narrow means callers don't need to import paramiko to consume + the result, and the wrapper's stub mode can populate it from a + plain ``os.stat_result`` without any paramiko dance. + + Frozen so callers can hash / cache the result if they need to. + """ + size: int + modified_at: datetime | None = None + + @dataclass class InboundFile: """A single file observed in the inbound MFT path.""" @@ -216,6 +232,30 @@ class SftpClient: return self._read_file_stub(remote_path) return self._read_file_paramiko(remote_path) + def stat(self, remote_path: str) -> SftpStat: + """Return file metadata for ``remote_path``. + + Mirrors the subset of paramiko's ``SFTPAttributes`` that + callers need (``size`` for idempotency checks; ``modified_at`` + for cache-busting). The SP37 submission helper uses + ``stat().size`` to short-circuit re-uploads of already-uploaded + files (the SKIPPED outcome path). + + Stub mode: reads ``os.stat_result`` from + ``{staging_dir}/{remote_path}``. + + Real mode: calls ``sftp.stat(remote_path)`` on a paramiko + connection. + + Raises: + FileNotFoundError: if ``remote_path`` does not exist + (stub: missing local file; real: paramiko raises + ``IOError`` which is a ``FileNotFoundError`` subclass). + """ + if self._stub: + return self._stat_stub(remote_path) + return self._stat_paramiko(remote_path) + def _read_file_stub(self, remote_path: str) -> bytes: """Read bytes from ``{staging_dir}/{remote_path}`` (SP16 stub).""" staging = Path(self._block.staging_dir).resolve() @@ -224,6 +264,25 @@ class SftpClient: raise FileNotFoundError(f"inbound stub file not found: {target}") return target.read_bytes() + def _stat_stub(self, remote_path: str) -> SftpStat: + """Return ``SftpStat`` for ``{staging_dir}/{remote_path}`` (SP37 stub). + + Mirrors what paramiko's ``sftp.stat()`` returns in real mode: + ``size`` from ``st.st_size`` and ``modified_at`` from + ``st.st_mtime``. Raises ``FileNotFoundError`` if the local + file is missing — matches paramiko's ``IOError`` behavior + so the caller doesn't need to special-case stub mode. + """ + staging = Path(self._block.staging_dir).resolve() + target = staging / remote_path.lstrip("/") + if not target.is_file(): + raise FileNotFoundError(f"inbound stub file not found: {target}") + st = target.stat() + return SftpStat( + size=st.st_size, + modified_at=datetime.fromtimestamp(st.st_mtime), + ) + def get_secret(self, name: str) -> Optional[str]: """Fetch the auth secret from Keychain. Returns the stub secret if absent.""" value = secrets.get_secret(name) @@ -506,6 +565,20 @@ class SftpClient: shutil.copyfileobj(f, buf, length=64 * 1024) return buf.getvalue() + def _stat_paramiko(self, remote_path: str) -> SftpStat: + """Return ``SftpStat`` for ``remote_path`` via paramiko. + + Paramiko's ``SFTPAttributes`` already provides ``st_size`` and + ``st_mtime``; we project them into our narrow public + ``SftpStat`` shape so callers don't need to know about paramiko. + """ + with self._connect() as (ssh, sftp): + attr = sftp.stat(remote_path) + return SftpStat( + size=attr.st_size or 0, + modified_at=datetime.fromtimestamp(attr.st_mtime or 0), + ) + # --------------------------------------------------------------------------- # Module-level helper diff --git a/backend/tests/test_sftp_client_stat.py b/backend/tests/test_sftp_client_stat.py new file mode 100644 index 0000000..6ccc80f --- /dev/null +++ b/backend/tests/test_sftp_client_stat.py @@ -0,0 +1,150 @@ +"""SP37 follow-up #5: SftpClient.stat() method. + +The SftpClient wrapper previously exposed ``write_file``, +``list_inbound``, ``list_inbound_names``, ``download_inbound``, and +``read_file`` — but NO ``stat()``. The canonical +``cyclone.submission.submit_file`` helper needs to compare the local +file size against the remote file size for idempotency; the lack of +``stat()`` forced the helper to bypass the wrapper and open paramiko +directly (see ``submission/core.py:_default_sftp_factory``). + +This file adds a ``stat()`` method to the wrapper so the helper can +use ``SftpClient(block=b)`` instead of bypassing the wrapper. Tests +cover the stub implementation only (paramiko real-mode tests would +need a real SFTP server). +""" +from __future__ import annotations + +from pathlib import Path + +import pytest + +from cyclone.clearhouse import SftpClient, SftpStat +from cyclone.providers import SftpBlock + + +def _stub_block(staging_dir: Path) -> SftpBlock: + return SftpBlock( + host="mft.example.com", + port=22, + username="user", + auth={"password_keychain_account": "x"}, + paths={"inbound": "/inbound", "outbound": "/outbound"}, + staging_dir=str(staging_dir), + stub=True, + ) + + +@pytest.fixture +def staging(tmp_path: Path) -> Path: + """A staging dir pre-populated with one file. Mirrors what the + SP9 stub layout looks like when an operator has dropped a real + inbound file for testing.""" + inbound = tmp_path / "inbound" + inbound.mkdir(parents=True) + (inbound / "test.txt").write_bytes(b"hello world\n") + return tmp_path + + +# ---- SftpStat dataclass ----------------------------------------------------- # + + +def test_sftp_stat_dataclass_exists(): + """The wrapper exposes a small SftpStat dataclass for the result. + + Mirrors the paramiko ``SFTPAttributes`` shape (at least ``size``) + but keeps the public surface narrow so callers can use it + without depending on paramiko. + """ + assert SftpStat is not None + stat = SftpStat(size=42, modified_at=None) + assert stat.size == 42 + assert stat.modified_at is None + + +def test_sftp_stat_is_frozen(): + """SftpStat is a frozen dataclass so it can be hashed / used as a + cache key by future callers without surprise mutation.""" + from dataclasses import FrozenInstanceError + + stat = SftpStat(size=1, modified_at=None) + with pytest.raises(FrozenInstanceError): + stat.size = 2 # type: ignore[misc] + + +# ---- Public method --------------------------------------------------------- # + + +def test_sftp_client_has_stat_method(): + """``SftpClient`` exposes a ``stat(remote_path)`` public method. + + Guards the API surface — a future contributor who renames or + removes ``stat()`` (forcing the helper to bypass the wrapper + again) would break this test, prompting them to update the + helper instead. + """ + assert hasattr(SftpClient, "stat") + assert callable(SftpClient.stat) + + +def test_sftp_client_stat_returns_size_in_stub_mode(staging: Path): + """Stub mode: ``stat(remote_path)`` returns the local staging + file's size via ``os.stat_result.st_size``. Mirrors what a real + paramiko ``SFTPAttributes.st_size`` would return for a remote file. + """ + client = SftpClient(block=_stub_block(staging)) + stat = client.stat("/inbound/test.txt") + assert stat.size == len(b"hello world\n") + + +def test_sftp_client_stat_returns_modified_at_in_stub_mode(staging: Path): + """Stub mode: ``stat`` populates ``modified_at`` from the local + file's mtime. Mirrors paramiko's behavior for parity.""" + client = SftpClient(block=_stub_block(staging)) + stat = client.stat("/inbound/test.txt") + assert stat.modified_at is not None + + +def test_sftp_client_stat_raises_for_missing_file(staging: Path): + """Stub mode: ``stat`` raises ``FileNotFoundError`` for a path + that doesn't exist locally — matches the real paramiko behavior + (which raises ``IOError``/``FileNotFoundError``). The helper's + SKIPPED-outcome short-circuit depends on this exception type. + """ + client = SftpClient(block=_stub_block(staging)) + with pytest.raises(FileNotFoundError): + client.stat("/inbound/missing.txt") + + +def test_sftp_client_stat_handles_large_file(staging: Path): + """Stub mode: ``stat`` returns the correct size for non-trivial files. + + Sanity check: the size path doesn't truncate or accidentally + cast (e.g. to int8) on larger files. + """ + payload = b"x" * (1024 * 1024) # 1 MiB + (staging / "inbound" / "big.bin").write_bytes(payload) + client = SftpClient(block=_stub_block(staging)) + stat = client.stat("/inbound/big.bin") + assert stat.size == 1024 * 1024 + + +def test_sftp_client_stat_supports_idempotency_check(staging: Path): + """The submission helper's idempotency check works against the + wrapper now. + + Replays the SKIPPED-outcome check from + ``cyclone.submission.submit_file`` (lines 178-182) using the + wrapper directly. Before this follow-up, the helper had to open + paramiko directly because the wrapper lacked ``stat()``. + """ + payload = b"identical contents" + (staging / "outbound").mkdir(parents=True, exist_ok=True) + (staging / "outbound" / "claim.x12").write_bytes(payload) + + client = SftpClient(block=_stub_block(staging)) + local_size = len(payload) + # stat() returns the size that was previously uploaded. + remote_stat = client.stat("/outbound/claim.x12") + assert remote_stat.size == local_size # idempotency check passes + # → SKIPPED outcome in the submission helper \ No newline at end of file