From c41b71b17f30d8f2d25fa4ba155e8257b388faa0 Mon Sep 17 00:00:00 2001 From: YuvaKunaal Date: Thu, 23 Jul 2026 10:20:08 +0530 Subject: [PATCH 1/2] test(api): add mtime regression + acceptance coverage for mounts file/archive endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a unit regression test pinning the epoch-ms/seconds mtime bug in stream_mounts_archive, plus acceptance coverage for the shallow (depth=1) listing and the download-all archive endpoint (/mounts/files/export) — the two gaps called out in #5416. _rollup_recent_entries already had coverage from #5411, so this PR doesn't touch it. Closes #5416 --- .../acceptance/mounts/test_mounts_basics.py | 193 ++++++++++++++++++ .../tests/pytest/unit/test_mounts_file_ops.py | 56 ++++- 2 files changed, 247 insertions(+), 2 deletions(-) diff --git a/api/oss/tests/pytest/acceptance/mounts/test_mounts_basics.py b/api/oss/tests/pytest/acceptance/mounts/test_mounts_basics.py index e446dbb032..977cdd4297 100644 --- a/api/oss/tests/pytest/acceptance/mounts/test_mounts_basics.py +++ b/api/oss/tests/pytest/acceptance/mounts/test_mounts_basics.py @@ -4,6 +4,8 @@ path-injection rejection. Requires a running API (OSS or EE). """ +import io +import zipfile from uuid import uuid4 @@ -250,6 +252,15 @@ def _skip_if_no_store(resp): pytest.skip("Mount storage backend not configured in this environment") +def _write_file(authed_api, mount_id, path, content=b"x"): + resp = authed_api( + "PUT", f"/mounts/{mount_id}/files", params={"path": path}, data=content + ) + _skip_if_no_store(resp) + assert resp.status_code == 200, resp.text + return resp + + class TestMountFileOps: def test_write_read_list_delete_roundtrip(self, authed_api): mount_id = _create_mount(authed_api) @@ -384,3 +395,185 @@ def test_files_on_missing_mount_returns_404(self, authed_api): fake_id = str(uuid4()) resp = authed_api("GET", f"/mounts/{fake_id}/files") assert resp.status_code == 404, resp.text + + +# --------------------------------------------------------------------------- +# Shallow (depth=1) listing +# +# GET /mounts/{id}/files?depth=1[&with_counts&git_aware&include_gitignored] is the +# lazy per-directory summary the drive drawer loads on open (#5400) — one delimiter +# level, not the whole tree. Previously covered only by unit tests against the fake +# store; no acceptance test exercised it against the real API + object store. +# --------------------------------------------------------------------------- + + +class TestMountShallowListing: + def test_shallow_listing_returns_top_level_only(self, authed_api): + mount_id = _create_mount(authed_api) + _write_file(authed_api, mount_id, "a.txt") + _write_file(authed_api, mount_id, "sub/b.txt") + + resp = authed_api("GET", f"/mounts/{mount_id}/files", params={"depth": 1}) + assert resp.status_code == 200, resp.text + by_path = {f["path"]: f for f in resp.json()["files"]} + + assert by_path["a.txt"]["is_folder"] is False + assert by_path["sub"]["is_folder"] is True + assert "sub/b.txt" not in by_path + + def test_shallow_listing_with_counts_reports_item_count(self, authed_api): + mount_id = _create_mount(authed_api) + _write_file(authed_api, mount_id, "dir/one.txt") + _write_file(authed_api, mount_id, "dir/two.txt") + + resp = authed_api( + "GET", + f"/mounts/{mount_id}/files", + params={"depth": 1, "with_counts": "true"}, + ) + assert resp.status_code == 200, resp.text + by_path = {f["path"]: f for f in resp.json()["files"]} + assert by_path["dir"]["item_count"] == 2 + + def test_shallow_listing_git_aware_prunes_ignored_and_git(self, authed_api): + mount_id = _create_mount(authed_api) + _write_file(authed_api, mount_id, ".gitignore", b"node_modules/\n") + _write_file(authed_api, mount_id, "src/app.py") + _write_file(authed_api, mount_id, "node_modules/react/index.js") + _write_file(authed_api, mount_id, ".git/HEAD") + + resp = authed_api( + "GET", + f"/mounts/{mount_id}/files", + params={"depth": 1, "git_aware": "true"}, + ) + assert resp.status_code == 200, resp.text + paths = {f["path"] for f in resp.json()["files"]} + assert paths == {".gitignore", "src"} + + def test_shallow_listing_include_gitignored_reveals_ignored_but_hides_git( + self, authed_api + ): + mount_id = _create_mount(authed_api) + _write_file(authed_api, mount_id, ".gitignore", b"node_modules/\n") + _write_file(authed_api, mount_id, "src/app.py") + _write_file(authed_api, mount_id, "node_modules/react/index.js") + _write_file(authed_api, mount_id, ".git/HEAD") + + resp = authed_api( + "GET", + f"/mounts/{mount_id}/files", + params={"depth": 1, "git_aware": "true", "include_gitignored": "true"}, + ) + assert resp.status_code == 200, resp.text + paths = {f["path"] for f in resp.json()["files"]} + assert paths == {".gitignore", "src", "node_modules"} + + def test_shallow_listing_depth_two_rejected(self, authed_api): + # depth=1 is the only implemented shallow view; anything else must 422 loudly + # rather than silently falling through to the most expensive full-tree branch. + # No mount needs to exist: Query validation runs before the handler body. + fake_id = str(uuid4()) + resp = authed_api("GET", f"/mounts/{fake_id}/files", params={"depth": 2}) + assert resp.status_code == 422, resp.text + + +# --------------------------------------------------------------------------- +# Download-all archive (POST /mounts/files/export) +# +# Streams a zip across one or more mounts (#5400), renamed from /files/archive in +# #5412 to stop colliding with this router's own archive/unarchive lifecycle verb. +# Previously untested at any layer beyond the work-list-building unit tests. +# --------------------------------------------------------------------------- + + +class TestMountArchiveExport: + def test_export_single_mount_returns_valid_zip(self, authed_api): + mount_id = _create_mount(authed_api) + _write_file(authed_api, mount_id, "a.txt", b"alpha") + _write_file(authed_api, mount_id, "b.txt", b"beta") + + resp = authed_api( + "POST", + "/mounts/files/export", + json={"mounts": [{"mount_id": mount_id, "prefix": "", "path": ""}]}, + ) + assert resp.status_code == 200, resp.text + assert "zip" in resp.headers.get("content-type", "") + + with zipfile.ZipFile(io.BytesIO(resp.content)) as zf: + assert set(zf.namelist()) == {"a.txt", "b.txt"} + assert zf.read("a.txt") == b"alpha" + assert zf.read("b.txt") == b"beta" + + def test_export_applies_prefix_folding(self, authed_api): + # The folded drive layout: cwd + agent-files land under their own prefix in + # the zip so "download all" reproduces the drive's tree, not a flat dump. + mount_id = _create_mount(authed_api) + _write_file(authed_api, mount_id, "a.txt", b"alpha") + + resp = authed_api( + "POST", + "/mounts/files/export", + json={"mounts": [{"mount_id": mount_id, "prefix": "cwd", "path": ""}]}, + ) + assert resp.status_code == 200, resp.text + + with zipfile.ZipFile(io.BytesIO(resp.content)) as zf: + assert zf.namelist() == ["cwd/a.txt"] + + def test_export_scopes_to_source_path(self, authed_api): + mount_id = _create_mount(authed_api) + _write_file(authed_api, mount_id, "workspace/a.txt", b"in") + _write_file(authed_api, mount_id, "outside.txt", b"out") + + resp = authed_api( + "POST", + "/mounts/files/export", + json={ + "mounts": [{"mount_id": mount_id, "prefix": "", "path": "workspace"}] + }, + ) + assert resp.status_code == 200, resp.text + + with zipfile.ZipFile(io.BytesIO(resp.content)) as zf: + assert zf.namelist() == ["workspace/a.txt"] + + def test_export_missing_mount_returns_404(self, authed_api): + # Pins #5411: mounts are resolved EAGERLY, before the response starts + # streaming, so a missing mount is a real 404 — never a 200 with a broken zip. + fake_id = str(uuid4()) + + resp = authed_api( + "POST", + "/mounts/files/export", + json={"mounts": [{"mount_id": fake_id, "prefix": "", "path": ""}]}, + ) + assert resp.status_code == 404, resp.text + + def test_export_path_traversal_in_request_returns_422(self, authed_api): + # source_path is validated before the mount is even resolved, so a fake + # mount_id is enough to isolate the check. + fake_id = str(uuid4()) + + resp = authed_api( + "POST", + "/mounts/files/export", + json={"mounts": [{"mount_id": fake_id, "prefix": "", "path": "../evil"}]}, + ) + assert resp.status_code == 422, resp.text + + def test_export_filename_reflected_in_content_disposition(self, authed_api): + mount_id = _create_mount(authed_api) + _write_file(authed_api, mount_id, "a.txt", b"alpha") + + resp = authed_api( + "POST", + "/mounts/files/export", + json={ + "mounts": [{"mount_id": mount_id, "prefix": "", "path": ""}], + "filename": "custom.zip", + }, + ) + assert resp.status_code == 200, resp.text + assert "custom.zip" in resp.headers.get("content-disposition", "") diff --git a/api/oss/tests/pytest/unit/test_mounts_file_ops.py b/api/oss/tests/pytest/unit/test_mounts_file_ops.py index ad3476a3fb..98a15dc969 100644 --- a/api/oss/tests/pytest/unit/test_mounts_file_ops.py +++ b/api/oss/tests/pytest/unit/test_mounts_file_ops.py @@ -12,12 +12,17 @@ stack. """ +import io +import zipfile from typing import List, Tuple from uuid import UUID, uuid4 import pytest -from oss.src.apis.fastapi.mounts.utils import _content_disposition_attachment +from oss.src.apis.fastapi.mounts.utils import ( + _content_disposition_attachment, + stream_mounts_archive, +) from oss.src.core.mounts import service as mounts_service_module from oss.src.core.mounts.dtos import Mount, MountArchiveSource, MountFile from oss.src.core.mounts.service import ( @@ -195,11 +200,17 @@ class FakeMountStorage: def __init__(self): # {bucket: {key: bytes}} self._store: dict[str, dict[str, bytes]] = {} + # {bucket: {key: epoch-ms}}; opt-in, only set where a test needs a realistic mtime. + self._mtimes: dict[str, dict[str, int]] = {} + + def set_mtime(self, bucket: str, key: str, mtime: int) -> None: + self._mtimes.setdefault(bucket, {})[key] = mtime async def list_objects_v2(self, *, bucket: str, prefix: str) -> List[StoreObject]: b = self._store.get(bucket, {}) + m = self._mtimes.get(bucket, {}) return [ - StoreObject(key=k, size=len(v)) + StoreObject(key=k, size=len(v), mtime=m.get(k)) for k, v in b.items() if k.startswith(prefix) ] @@ -377,6 +388,47 @@ async def test_traversal_key_does_not_alias_a_real_entry(self): assert zip_paths == ["a/report.txt"] +@pytest.mark.asyncio +class TestArchiveMtimeRegression: + async def test_export_survives_epoch_millisecond_mtime(self): + # Pins the bug fixed in #5400/#5411: StoreObject.mtime is epoch MILLISECONDS, but + # stream_mounts_archive builds a zip member's `datetime` from it. Without dividing by + # 1000 first, a realistic ms-scale value overflows datetime.fromtimestamp mid-stream — + # AFTER the 200 response headers are already on the wire — truncating the download to a + # broken/empty zip while the client still sees a success status. + mount = _make_mount() + storage = FakeMountStorage() + service = MountsService( + mounts_dao=_StubDAO(mount), + mounts_store=storage, + bucket=_BUCKET, + ) + pid, mid = mount.project_id, mount.id + + await service.write_file( + project_id=pid, mount_id=mid, path="report.txt", content=b"hello world" + ) + key = f"mounts/{pid}/{mid}/report.txt" + storage.set_mtime( + _BUCKET, key, 1_700_000_000_000 + ) # realistic epoch-ms LastModified + + response = await stream_mounts_archive( + mounts_service=service, + project_id=pid, + mounts=[MountArchiveSource(mount_id=mid)], + ) + + zip_bytes = b"".join( + [chunk async for chunk in response.body_iterator] # type: ignore[union-attr] + ) + + assert len(zip_bytes) > 0 + with zipfile.ZipFile(io.BytesIO(zip_bytes)) as zf: + assert zf.namelist() == ["report.txt"] + assert zf.read("report.txt") == b"hello world" + + # --------------------------------------------------------------------------- # Roundtrip # --------------------------------------------------------------------------- From 4bde0c656edb49572f325735720b7f28ca96929a Mon Sep 17 00:00:00 2001 From: YuvaKunaal Date: Thu, 23 Jul 2026 10:36:15 +0530 Subject: [PATCH 2/2] test(api): address CodeRabbit feedback on mtime regression test Clean up FakeMountStorage._mtimes on delete_keys so a recreated key can't inherit a stale timestamp. Assert the zip member's converted date_time explicitly, verified against the real stream_zip output, so a regression that ignores mtime and falls back to datetime.now() would still fail this test. --- api/oss/tests/pytest/unit/test_mounts_file_ops.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/api/oss/tests/pytest/unit/test_mounts_file_ops.py b/api/oss/tests/pytest/unit/test_mounts_file_ops.py index 98a15dc969..94522765d6 100644 --- a/api/oss/tests/pytest/unit/test_mounts_file_ops.py +++ b/api/oss/tests/pytest/unit/test_mounts_file_ops.py @@ -268,6 +268,7 @@ async def delete_keys(self, *, bucket: str, keys: List[str]) -> int: for k in keys: if k in b: del b[k] + self._mtimes.get(bucket, {}).pop(k, None) n += 1 return n @@ -426,6 +427,10 @@ async def test_export_survives_epoch_millisecond_mtime(self): assert len(zip_bytes) > 0 with zipfile.ZipFile(io.BytesIO(zip_bytes)) as zf: assert zf.namelist() == ["report.txt"] + # Not just "didn't crash" — the member's timestamp must be the CONVERTED value + # (1_700_000_000_000 ms -> 2023-11-14 22:13:20 UTC), so a regression that silently + # falls back to datetime.now() instead of the real mtime still fails this test. + assert zf.getinfo("report.txt").date_time == (2023, 11, 14, 22, 13, 20) assert zf.read("report.txt") == b"hello world"