diff --git a/backend/app/gateway/routers/uploads.py b/backend/app/gateway/routers/uploads.py index 7c5628273..006b69314 100644 --- a/backend/app/gateway/routers/uploads.py +++ b/backend/app/gateway/routers/uploads.py @@ -3,10 +3,7 @@ import logging import os import stat -import tempfile -from dataclasses import dataclass from pathlib import Path -from typing import BinaryIO from fastapi import APIRouter, Depends, File, HTTPException, Request, UploadFile from pydantic import BaseModel, Field @@ -17,23 +14,24 @@ from deerflow.config.app_config import AppConfig from deerflow.config.paths import get_paths from deerflow.runtime.user_context import get_effective_user_id from deerflow.sandbox.sandbox_provider import SandboxProvider, get_sandbox_provider +from deerflow.uploads.conversion import convert_uploaded_file_to_markdown +from deerflow.uploads.layout import artifact_url_for_virtual_path, conversion_virtual_path from deerflow.uploads.manager import ( - UPLOAD_STAGING_PREFIX, - UPLOAD_STAGING_SUFFIX, PathTraversalError, - UnsafeUploadPathError, - claim_unique_filename, + StagedUpload, + abort_staged_upload, + create_upload_staging_file, delete_file_safe, enrich_file_listing, ensure_uploads_dir, get_uploads_dir, list_files_in_dir, normalize_filename, + publish_staged_upload, upload_artifact_url, upload_virtual_path, - validate_upload_destination, ) -from deerflow.utils.file_conversion import CONVERTIBLE_EXTENSIONS, convert_file_to_markdown +from deerflow.utils.file_conversion import CONVERTIBLE_EXTENSIONS from deerflow.utils.file_io import run_file_io from deerflow.utils.thread_id import ThreadId @@ -47,13 +45,6 @@ DEFAULT_MAX_FILE_SIZE = 50 * 1024 * 1024 DEFAULT_MAX_TOTAL_SIZE = 100 * 1024 * 1024 -@dataclass(slots=True) -class _UploadTempFile: - file_path: Path - temp_path: Path - handle: BinaryIO - - class UploadedFileInfo(BaseModel): """Uploaded file metadata exposed by upload and list APIs.""" @@ -178,52 +169,6 @@ def _cleanup_uploaded_paths(paths: list[os.PathLike[str] | str]) -> None: logger.warning("Failed to clean up upload path after rejected request: %s", path, exc_info=True) -def _prepare_upload_destination(uploads_dir: os.PathLike[str] | str, display_filename: str) -> _UploadTempFile: - uploads_dir_path = Path(uploads_dir) - file_path = validate_upload_destination(uploads_dir_path, display_filename) - temp_fd, temp_path_str = tempfile.mkstemp(prefix=UPLOAD_STAGING_PREFIX, suffix=UPLOAD_STAGING_SUFFIX, dir=uploads_dir_path) - temp_path = Path(temp_path_str) - try: - handle = os.fdopen(temp_fd, "wb") - except Exception: - try: - os.close(temp_fd) - except OSError: - pass - try: - os.unlink(temp_path) - except FileNotFoundError: - pass - raise - return _UploadTempFile(file_path=file_path, temp_path=temp_path, handle=handle) - - -def _write_upload_chunk(upload_temp: _UploadTempFile, chunk: bytes) -> None: - upload_temp.handle.write(chunk) - - -def _abort_upload_temp(upload_temp: _UploadTempFile) -> None: - try: - upload_temp.handle.close() - finally: - try: - os.unlink(upload_temp.temp_path) - except FileNotFoundError: - pass - - -def _commit_upload_temp(upload_temp: _UploadTempFile) -> None: - upload_temp.handle.close() - try: - os.replace(upload_temp.temp_path, upload_temp.file_path) - except Exception: - try: - os.unlink(upload_temp.temp_path) - except FileNotFoundError: - pass - raise - - def _make_uploaded_paths_sandbox_readable(paths: list[os.PathLike[str] | str]) -> None: for file_path in paths: _make_file_sandbox_readable(file_path) @@ -247,7 +192,7 @@ def _list_uploaded_files_for_thread(thread_id: str, user_id: str) -> dict: def _delete_uploaded_file_for_thread(thread_id: str, filename: str, user_id: str) -> dict: uploads_dir = get_uploads_dir(thread_id, user_id=user_id) - return delete_file_safe(uploads_dir, filename, convertible_extensions=CONVERTIBLE_EXTENSIONS) + return delete_file_safe(uploads_dir, filename) async def _write_upload_file_with_limits( @@ -260,9 +205,9 @@ async def _write_upload_file_with_limits( total_size: int, ) -> tuple[os.PathLike[str] | str, int, int]: file_size = 0 - upload_temp: _UploadTempFile | None = None + upload_temp: StagedUpload | None = None try: - upload_temp = await run_file_io(_prepare_upload_destination, uploads_dir, display_filename) + upload_temp = await run_file_io(create_upload_staging_file, Path(uploads_dir)) while chunk := await file.read(UPLOAD_CHUNK_SIZE): file_size += len(chunk) total_size += len(chunk) @@ -270,14 +215,13 @@ async def _write_upload_file_with_limits( raise HTTPException(status_code=413, detail=f"File too large: {display_filename}") if total_size > max_total_size: raise HTTPException(status_code=413, detail="Total upload size too large") - await run_file_io(_write_upload_chunk, upload_temp, chunk) + await run_file_io(upload_temp.handle.write, chunk) - await run_file_io(_commit_upload_temp, upload_temp) - file_path = upload_temp.file_path + file_path = await run_file_io(publish_staged_upload, upload_temp, display_filename) upload_temp = None except Exception: if upload_temp is not None: - await run_file_io(_abort_upload_temp, upload_temp) + await run_file_io(abort_staged_upload, upload_temp) raise return file_path, file_size, total_size @@ -324,11 +268,6 @@ async def upload_files( sandbox_sync_targets = [] skipped_files = [] total_size = 0 - # Track filenames within this request so duplicate form parts do not - # silently truncate each other. Existing uploads keep the historical - # overwrite behavior for a single replacement upload. - seen_filenames: set[str] = set() - sandbox_provider = get_sandbox_provider() sync_to_sandbox = not _uses_thread_data_mounts(sandbox_provider) sandbox = None @@ -345,7 +284,6 @@ async def upload_files( try: original_filename = normalize_filename(file.filename) - safe_filename = claim_unique_filename(original_filename, seen_filenames) except ValueError: logger.warning(f"Skipping file with unsafe filename: {file.filename!r}") continue @@ -354,12 +292,13 @@ async def upload_files( file_path, file_size, total_size = await _write_upload_file_with_limits( file, uploads_dir=uploads_dir, - display_filename=safe_filename, + display_filename=original_filename, max_single_file_size=limits.max_file_size, max_total_size=limits.max_total_size, total_size=total_size, ) written_paths.append(file_path) + safe_filename = Path(file_path).name virtual_path = upload_virtual_path(safe_filename) @@ -380,39 +319,28 @@ async def upload_files( file_ext = file_path.suffix.lower() if auto_convert_documents and file_ext in CONVERTIBLE_EXTENSIONS: - # Reserve the companion .md name in this request's seen set - # before writing so conversion cannot silently truncate another - # uploaded or derived file (same invariant as form-part dedupe). - provisional_md_name = Path(safe_filename).with_suffix(".md").name - unique_md_name = claim_unique_filename(provisional_md_name, seen_filenames) - md_output = file_path.with_name(unique_md_name) - md_path = await convert_file_to_markdown(file_path, output_path=md_output) + try: + md_path = await convert_uploaded_file_to_markdown(file_path) + except Exception: + logger.warning("Failed to convert uploaded file: %s", file_path, exc_info=True) + md_path = None if md_path: written_paths.append(md_path) - md_virtual_path = upload_virtual_path(md_path.name) + md_virtual_path = conversion_virtual_path(safe_filename) if sync_to_sandbox: sandbox_sync_targets.append((md_path, md_virtual_path)) file_info["markdown_file"] = md_path.name - file_info["markdown_path"] = str(sandbox_uploads / md_path.name) + file_info["markdown_path"] = str(md_path) file_info["markdown_virtual_path"] = md_virtual_path - file_info["markdown_artifact_url"] = upload_artifact_url(thread_id, md_path.name) - else: - # Conversion failed and wrote nothing, so release the claim; - # holding it would rename a later same-stem upload against - # a name nothing occupies. - seen_filenames.discard(unique_md_name) + file_info["markdown_artifact_url"] = artifact_url_for_virtual_path(thread_id, md_virtual_path) uploaded_files.append(file_info) except HTTPException as e: await run_file_io(_cleanup_uploaded_paths, written_paths) raise e - except UnsafeUploadPathError as e: - logger.warning("Skipping upload with unsafe destination %s: %s", file.filename, e) - skipped_files.append(safe_filename) - continue except Exception as e: logger.error(f"Failed to upload {file.filename}: {e}") await run_file_io(_cleanup_uploaded_paths, written_paths) diff --git a/backend/tests/test_uploads_router.py b/backend/tests/test_uploads_router.py index 6f72bd0bd..f1f305b05 100644 --- a/backend/tests/test_uploads_router.py +++ b/backend/tests/test_uploads_router.py @@ -13,6 +13,7 @@ from fastapi.testclient import TestClient from app.gateway.deps import get_config from app.gateway.routers import uploads +from deerflow.uploads.layout import conversion_path_for_upload class ChunkedUpload: @@ -45,6 +46,20 @@ def _symlink_to_or_skip(link_path: Path, target_path: Path) -> None: raise +def _fake_owned_conversion(content_by_source: dict[str, str] | None = None): + async def fake_convert(file_path: Path) -> Path: + md_path = conversion_path_for_upload(file_path) + md_path.parent.mkdir(parents=True, exist_ok=True) + if content_by_source is not None and file_path.name in content_by_source: + text = content_by_source[file_path.name] + else: + text = f"converted-from:{file_path.name}" + md_path.write_text(text, encoding="utf-8") + return md_path + + return fake_convert + + def test_upload_files_writes_thread_storage_and_skips_local_sandbox_sync(tmp_path): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) @@ -132,6 +147,59 @@ def test_upload_files_auto_renames_duplicate_form_filenames(tmp_path): assert (thread_uploads_dir / "data_1.txt").read_bytes() == b"second" +def test_separate_upload_requests_never_replace_same_name(tmp_path): + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir(parents=True) + + async def upload(payload: bytes): + return await call_unwrapped( + uploads.upload_files, + "thread-local", + request=MagicMock(), + files=[UploadFile(filename="report.txt", file=BytesIO(payload))], + config=SimpleNamespace(), + ) + + with ( + patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), + ): + first = asyncio.run(upload(b"first")) + second = asyncio.run(upload(b"second")) + + assert [first.files[0].filename, second.files[0].filename] == ["report.txt", "report_1.txt"] + assert (thread_uploads_dir / "report.txt").read_bytes() == b"first" + assert (thread_uploads_dir / "report_1.txt").read_bytes() == b"second" + + +def test_concurrent_upload_requests_preserve_all_payloads(tmp_path): + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir(parents=True) + payloads = [f"payload-{index}".encode() for index in range(8)] + + async def upload(payload: bytes): + return await call_unwrapped( + uploads.upload_files, + "thread-local", + request=MagicMock(), + files=[UploadFile(filename="same.bin", file=BytesIO(payload))], + config=SimpleNamespace(), + ) + + async def run_all(): + return await asyncio.gather(*(upload(payload) for payload in payloads)) + + with ( + patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), + ): + results = asyncio.run(run_all()) + + paths = [thread_uploads_dir / result.files[0].filename for result in results] + assert len({path.name for path in paths}) == len(payloads) + assert {path.read_bytes() for path in paths} == set(payloads) + + def test_upload_files_skips_acquire_when_thread_data_is_mounted(tmp_path): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) @@ -170,7 +238,7 @@ def test_upload_files_does_not_auto_convert_documents_by_default(tmp_path): patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "get_sandbox_provider", return_value=provider), patch.object(uploads, "_auto_convert_documents_enabled", return_value=False), - patch.object(uploads, "convert_file_to_markdown", AsyncMock()) as convert_mock, + patch.object(uploads, "convert_uploaded_file_to_markdown", AsyncMock()) as convert_mock, ): file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes")) result = asyncio.run(call_unwrapped(uploads.upload_files, "thread-local", request=MagicMock(), files=[file], config=SimpleNamespace())) @@ -180,7 +248,7 @@ def test_upload_files_does_not_auto_convert_documents_by_default(tmp_path): assert result.files[0].filename == "report.pdf" assert result.files[0].markdown_file is None convert_mock.assert_not_called() - assert not (thread_uploads_dir / "report.md").exists() + assert not conversion_path_for_upload(thread_uploads_dir / "report.pdf").exists() def test_upload_files_syncs_non_local_sandbox_and_marks_markdown_file(tmp_path): @@ -194,17 +262,16 @@ def test_upload_files_syncs_non_local_sandbox_and_marks_markdown_file(tmp_path): sandbox = MagicMock() provider.get.return_value = sandbox - async def fake_convert(file_path: Path, output_path: Path | None = None) -> Path: - md_path = output_path if output_path is not None else file_path.with_suffix(".md") - md_path.write_text("converted", encoding="utf-8") - return md_path - with ( patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "get_sandbox_provider", return_value=provider), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), - patch.object(uploads, "convert_file_to_markdown", AsyncMock(side_effect=fake_convert)), + patch.object( + uploads, + "convert_uploaded_file_to_markdown", + AsyncMock(side_effect=_fake_owned_conversion({"report.pdf": "converted"})), + ), ): file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes")) result = asyncio.run(call_unwrapped(uploads.upload_files, "thread-aio", request=MagicMock(), files=[file], config=SimpleNamespace())) @@ -213,13 +280,16 @@ def test_upload_files_syncs_non_local_sandbox_and_marks_markdown_file(tmp_path): assert len(result.files) == 1 file_info = result.files[0] assert file_info.filename == "report.pdf" - assert file_info.markdown_file == "report.md" + assert file_info.markdown_file == "report.pdf.md" + assert file_info.markdown_virtual_path == "/mnt/user-data/.upload-conversions/report.pdf.md" + assert file_info.markdown_artifact_url == ("/api/threads/thread-aio/artifacts/mnt/user-data/.upload-conversions/report.pdf.md") assert (thread_uploads_dir / "report.pdf").read_bytes() == b"pdf-bytes" - assert (thread_uploads_dir / "report.md").read_text(encoding="utf-8") == "converted" + conversion = conversion_path_for_upload(thread_uploads_dir / "report.pdf") + assert conversion.read_text(encoding="utf-8") == "converted" sandbox.update_file.assert_any_call("/mnt/user-data/uploads/report.pdf", b"pdf-bytes") - sandbox.update_file.assert_any_call("/mnt/user-data/uploads/report.md", b"converted") + sandbox.update_file.assert_any_call("/mnt/user-data/.upload-conversions/report.pdf.md", b"converted") def test_upload_files_makes_non_local_files_sandbox_writable(tmp_path): @@ -233,17 +303,16 @@ def test_upload_files_makes_non_local_files_sandbox_writable(tmp_path): sandbox = MagicMock() provider.get.return_value = sandbox - async def fake_convert(file_path: Path, output_path: Path | None = None) -> Path: - md_path = output_path if output_path is not None else file_path.with_suffix(".md") - md_path.write_text("converted", encoding="utf-8") - return md_path - with ( patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "get_sandbox_provider", return_value=provider), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), - patch.object(uploads, "convert_file_to_markdown", AsyncMock(side_effect=fake_convert)), + patch.object( + uploads, + "convert_uploaded_file_to_markdown", + AsyncMock(side_effect=_fake_owned_conversion({"report.pdf": "converted"})), + ), patch.object(uploads, "_make_file_sandbox_writable") as make_writable, ): file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes")) @@ -251,7 +320,7 @@ def test_upload_files_makes_non_local_files_sandbox_writable(tmp_path): assert result.success is True make_writable.assert_any_call(thread_uploads_dir / "report.pdf") - make_writable.assert_any_call(thread_uploads_dir / "report.md") + make_writable.assert_any_call(conversion_path_for_upload(thread_uploads_dir / "report.pdf")) def test_upload_files_does_not_adjust_permissions_for_local_sandbox(tmp_path): @@ -432,7 +501,7 @@ def test_upload_files_does_not_sync_non_local_sandbox_when_total_size_exceeds_li sandbox.update_file.assert_not_called() -def test_upload_files_does_not_sync_non_local_sandbox_when_conversion_fails(tmp_path): +def test_upload_files_keeps_and_syncs_primary_when_conversion_fails(tmp_path): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) @@ -448,18 +517,31 @@ def test_upload_files_does_not_sync_non_local_sandbox_when_conversion_fails(tmp_ patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "get_sandbox_provider", return_value=provider), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), - patch.object(uploads, "convert_file_to_markdown", AsyncMock(side_effect=RuntimeError("conversion failed"))), + patch.object( + uploads, + "convert_uploaded_file_to_markdown", + AsyncMock(side_effect=RuntimeError("conversion failed")), + ), ): file = UploadFile(filename="report.pdf", file=BytesIO(b"pdf-bytes")) - with pytest.raises(HTTPException) as exc_info: - asyncio.run(call_unwrapped(uploads.upload_files, "thread-aio", request=MagicMock(), files=[file], config=SimpleNamespace())) + result = asyncio.run( + call_unwrapped( + uploads.upload_files, + "thread-aio", + request=MagicMock(), + files=[file], + config=SimpleNamespace(), + ) + ) - assert exc_info.value.status_code == 500 + assert result.success is True + assert result.files[0].filename == "report.pdf" + assert result.files[0].markdown_file is None provider.acquire.assert_not_called() provider.acquire_async.assert_awaited_once_with("thread-aio", user_id="owner-upload") provider.get.assert_called_once_with("aio-1") - sandbox.update_file.assert_not_called() - assert not (thread_uploads_dir / "report.pdf").exists() + sandbox.update_file.assert_called_once_with("/mnt/user-data/uploads/report.pdf", b"pdf-bytes") + assert (thread_uploads_dir / "report.pdf").read_bytes() == b"pdf-bytes" def test_make_file_sandbox_writable_adds_write_bits_for_regular_files(tmp_path): @@ -575,7 +657,7 @@ def test_upload_files_rejects_dotdot_and_dot_filenames(tmp_path): assert [f.name for f in thread_uploads_dir.iterdir()] == ["passwd"] -def test_upload_files_rejects_preexisting_symlink_destination(tmp_path): +def test_upload_files_renames_around_preexisting_symlink_destination(tmp_path): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) outside_file = tmp_path / "outside.txt" @@ -593,15 +675,15 @@ def test_upload_files_rejects_preexisting_symlink_destination(tmp_path): file = UploadFile(filename="victim.txt", file=BytesIO(b"attacker upload")) result = asyncio.run(uploads.upload_files("thread-local", files=[file])) - assert result.success is False - assert result.files == [] - assert result.skipped_files == ["victim.txt"] - assert "skipped 1 unsafe file" in result.message + assert result.success is True + assert result.files[0].filename == "victim_1.txt" + assert result.files[0].original_filename == "victim.txt" assert outside_file.read_text(encoding="utf-8") == "protected" assert (thread_uploads_dir / "victim.txt").is_symlink() + assert (thread_uploads_dir / "victim_1.txt").read_bytes() == b"attacker upload" -def test_upload_files_rejects_dangling_symlink_destination(tmp_path): +def test_upload_files_renames_around_dangling_symlink_destination(tmp_path): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) missing_target = tmp_path / "missing-target.txt" @@ -618,14 +700,14 @@ def test_upload_files_rejects_dangling_symlink_destination(tmp_path): file = UploadFile(filename="victim.txt", file=BytesIO(b"attacker upload")) result = asyncio.run(uploads.upload_files("thread-local", files=[file])) - assert result.success is False - assert result.files == [] - assert result.skipped_files == ["victim.txt"] + assert result.success is True + assert result.files[0].filename == "victim_1.txt" assert not missing_target.exists() assert (thread_uploads_dir / "victim.txt").is_symlink() + assert (thread_uploads_dir / "victim_1.txt").read_bytes() == b"attacker upload" -def test_upload_files_rejects_hardlinked_destination_without_truncating(tmp_path): +def test_upload_files_renames_around_hardlinked_destination_without_truncating(tmp_path): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) outside_file = tmp_path / "outside.txt" @@ -643,14 +725,14 @@ def test_upload_files_rejects_hardlinked_destination_without_truncating(tmp_path file = UploadFile(filename="victim.txt", file=BytesIO(b"attacker upload")) result = asyncio.run(uploads.upload_files("thread-local", files=[file])) - assert result.success is False - assert result.files == [] - assert result.skipped_files == ["victim.txt"] + assert result.success is True + assert result.files[0].filename == "victim_1.txt" assert outside_file.read_text(encoding="utf-8") == "protected" assert (thread_uploads_dir / "victim.txt").read_text(encoding="utf-8") == "protected" + assert (thread_uploads_dir / "victim_1.txt").read_bytes() == b"attacker upload" -def test_upload_files_overwrites_existing_regular_file(tmp_path): +def test_upload_files_renames_existing_regular_file(tmp_path): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) existing_file = thread_uploads_dir / "notes.txt" @@ -669,8 +751,9 @@ def test_upload_files_overwrites_existing_regular_file(tmp_path): result = asyncio.run(uploads.upload_files("thread-local", files=[file])) assert result.success is True - assert [file_info.filename for file_info in result.files] == ["notes.txt"] - assert existing_file.read_bytes() == b"new upload" + assert [file_info.filename for file_info in result.files] == ["notes_1.txt"] + assert existing_file.read_bytes() == b"old upload" + assert (thread_uploads_dir / "notes_1.txt").read_bytes() == b"new upload" assert existing_file.stat().st_nlink == 1 @@ -706,18 +789,24 @@ def test_upload_files_oversized_replacement_preserves_existing_regular_file(tmp_ assert [path.name for path in thread_uploads_dir.iterdir()] == ["a.txt"] -def test_delete_uploaded_file_removes_generated_markdown_companion(tmp_path): +def test_delete_uploaded_file_removes_owned_conversion_and_preserves_user_markdown(tmp_path): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) - (thread_uploads_dir / "report.pdf").write_bytes(b"pdf-bytes") - (thread_uploads_dir / "report.md").write_text("converted", encoding="utf-8") + primary = thread_uploads_dir / "report.pdf" + primary.write_bytes(b"pdf-bytes") + user_markdown = thread_uploads_dir / "report.md" + user_markdown.write_text("user", encoding="utf-8") + conversion = conversion_path_for_upload(primary) + conversion.parent.mkdir() + conversion.write_text("converted", encoding="utf-8") with patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir): result = asyncio.run(call_unwrapped(uploads.delete_uploaded_file, "thread-aio", "report.pdf", request=MagicMock())) assert result == {"success": True, "message": "Deleted report.pdf"} - assert not (thread_uploads_dir / "report.pdf").exists() - assert not (thread_uploads_dir / "report.md").exists() + assert not primary.exists() + assert not conversion.exists() + assert user_markdown.read_text(encoding="utf-8") == "user" def test_auto_convert_documents_enabled_defaults_to_false_on_config_errors(): @@ -819,28 +908,8 @@ def test_upload_files_uses_configured_file_count_limit(tmp_path): assert exc_info.value.status_code == 413 -def _fake_convert_honoring_output_path(content_by_source: dict[str, str] | None = None): - """Mimic convert_file_to_markdown, including optional output_path.""" - - async def fake_convert(file_path: Path, output_path: Path | None = None) -> Path: - md_path = output_path if output_path is not None else file_path.with_suffix(".md") - if content_by_source is not None and file_path.name in content_by_source: - text = content_by_source[file_path.name] - else: - text = f"converted-from:{file_path.name}" - md_path.write_text(text, encoding="utf-8") - return md_path - - return fake_convert - - def test_upload_files_converted_markdown_does_not_overwrite_user_markdown(tmp_path): - """Companion .md from auto-convert must not clobber a same-request .md upload. - - Declared invariant (upload_files): filenames within one request must not - silently truncate each other. convert_file_to_markdown used to write - stem.md unconditionally, bypassing claim_unique_filename. - """ + """Owned conversion output must not clobber a same-request Markdown upload.""" thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) @@ -851,8 +920,8 @@ def test_upload_files_converted_markdown_does_not_overwrite_user_markdown(tmp_pa patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object( uploads, - "convert_file_to_markdown", - AsyncMock(side_effect=_fake_convert_honoring_output_path({"notes.docx": "FROM_DOCX"})), + "convert_uploaded_file_to_markdown", + AsyncMock(side_effect=_fake_owned_conversion({"notes.docx": "FROM_DOCX"})), ), ): result = asyncio.run( @@ -872,10 +941,8 @@ def test_upload_files_converted_markdown_does_not_overwrite_user_markdown(tmp_pa assert [f.filename for f in result.files] == ["notes.md", "notes.docx"] # User upload preserved assert (thread_uploads_dir / "notes.md").read_bytes() == b"USER_MARKDOWN" - # Converted companion got a unique name instead of overwriting - assert result.files[1].markdown_file == "notes_1.md" - assert (thread_uploads_dir / "notes_1.md").read_text(encoding="utf-8") == "FROM_DOCX" - assert not (thread_uploads_dir / "notes.md").read_text(encoding="utf-8") == "FROM_DOCX" + assert result.files[1].markdown_file == "notes.docx.md" + assert conversion_path_for_upload(thread_uploads_dir / "notes.docx").read_text(encoding="utf-8") == "FROM_DOCX" def test_upload_files_two_convertibles_get_distinct_markdown_companions(tmp_path): @@ -890,8 +957,8 @@ def test_upload_files_two_convertibles_get_distinct_markdown_companions(tmp_path patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object( uploads, - "convert_file_to_markdown", - AsyncMock(side_effect=_fake_convert_honoring_output_path({"a.docx": "FROM_DOCX", "a.pdf": "FROM_PDF"})), + "convert_uploaded_file_to_markdown", + AsyncMock(side_effect=_fake_owned_conversion({"a.docx": "FROM_DOCX", "a.pdf": "FROM_PDF"})), ), ): result = asyncio.run( @@ -908,17 +975,14 @@ def test_upload_files_two_convertibles_get_distinct_markdown_companions(tmp_path ) assert result.success is True - assert result.files[0].markdown_file == "a.md" - assert result.files[1].markdown_file == "a_1.md" - assert (thread_uploads_dir / "a.md").read_text(encoding="utf-8") == "FROM_DOCX" - assert (thread_uploads_dir / "a_1.md").read_text(encoding="utf-8") == "FROM_PDF" - # Each response entry points at content that belongs to that source - assert (thread_uploads_dir / result.files[0].markdown_file).read_text(encoding="utf-8") == "FROM_DOCX" - assert (thread_uploads_dir / result.files[1].markdown_file).read_text(encoding="utf-8") == "FROM_PDF" + assert result.files[0].markdown_file == "a.docx.md" + assert result.files[1].markdown_file == "a.pdf.md" + assert conversion_path_for_upload(thread_uploads_dir / "a.docx").read_text(encoding="utf-8") == "FROM_DOCX" + assert conversion_path_for_upload(thread_uploads_dir / "a.pdf").read_text(encoding="utf-8") == "FROM_PDF" -def test_upload_files_user_markdown_after_convertible_is_renamed_not_overwritten(tmp_path): - """If convert claims stem.md first, a later same-request .md is renamed.""" +def test_upload_files_user_markdown_after_convertible_keeps_its_name(tmp_path): + """Generated output uses a separate namespace from a later user Markdown.""" thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) @@ -929,8 +993,8 @@ def test_upload_files_user_markdown_after_convertible_is_renamed_not_overwritten patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), patch.object( uploads, - "convert_file_to_markdown", - AsyncMock(side_effect=_fake_convert_honoring_output_path({"notes.docx": "FROM_DOCX"})), + "convert_uploaded_file_to_markdown", + AsyncMock(side_effect=_fake_owned_conversion({"notes.docx": "FROM_DOCX"})), ), ): result = asyncio.run( @@ -948,11 +1012,11 @@ def test_upload_files_user_markdown_after_convertible_is_renamed_not_overwritten assert result.success is True assert result.files[0].filename == "notes.docx" - assert result.files[0].markdown_file == "notes.md" - assert result.files[1].filename == "notes_1.md" - assert result.files[1].original_filename == "notes.md" - assert (thread_uploads_dir / "notes.md").read_text(encoding="utf-8") == "FROM_DOCX" - assert (thread_uploads_dir / "notes_1.md").read_bytes() == b"USER_MARKDOWN" + assert result.files[0].markdown_file == "notes.docx.md" + assert result.files[1].filename == "notes.md" + assert result.files[1].original_filename is None + assert (thread_uploads_dir / "notes.md").read_bytes() == b"USER_MARKDOWN" + assert conversion_path_for_upload(thread_uploads_dir / "notes.docx").read_text(encoding="utf-8") == "FROM_DOCX" def test_upload_files_failed_conversion_releases_the_claimed_markdown_name(tmp_path): @@ -965,7 +1029,7 @@ def test_upload_files_failed_conversion_releases_the_claimed_markdown_name(tmp_p patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), - patch.object(uploads, "convert_file_to_markdown", AsyncMock(return_value=None)), + patch.object(uploads, "convert_uploaded_file_to_markdown", AsyncMock(return_value=None)), ): result = asyncio.run( call_unwrapped( @@ -985,7 +1049,7 @@ def test_upload_files_failed_conversion_releases_the_claimed_markdown_name(tmp_p assert result.files[1].filename == "notes.md" assert result.files[1].original_filename is None assert (thread_uploads_dir / "notes.md").read_bytes() == b"USER_MARKDOWN" - assert not (thread_uploads_dir / "notes_1.md").exists() + assert not conversion_path_for_upload(thread_uploads_dir / "notes.docx").exists() def test_upload_files_failed_conversion_does_not_push_the_next_companion_to_suffix(tmp_path): @@ -993,10 +1057,11 @@ def test_upload_files_failed_conversion_does_not_push_the_next_companion_to_suff thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir(parents=True) - async def convert_failing_on_docx(file_path: Path, output_path: Path | None = None) -> Path | None: + async def convert_failing_on_docx(file_path: Path) -> Path | None: if file_path.suffix.lower() == ".docx": return None - md_path = output_path if output_path is not None else file_path.with_suffix(".md") + md_path = conversion_path_for_upload(file_path) + md_path.parent.mkdir(parents=True, exist_ok=True) md_path.write_text(f"FROM:{file_path.name}", encoding="utf-8") return md_path @@ -1005,7 +1070,11 @@ def test_upload_files_failed_conversion_does_not_push_the_next_companion_to_suff patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), patch.object(uploads, "_auto_convert_documents_enabled", return_value=True), - patch.object(uploads, "convert_file_to_markdown", AsyncMock(side_effect=convert_failing_on_docx)), + patch.object( + uploads, + "convert_uploaded_file_to_markdown", + AsyncMock(side_effect=convert_failing_on_docx), + ), ): result = asyncio.run( call_unwrapped( @@ -1022,6 +1091,5 @@ def test_upload_files_failed_conversion_does_not_push_the_next_companion_to_suff assert result.success is True assert result.files[0].markdown_file is None - assert result.files[1].markdown_file == "notes.md" - assert (thread_uploads_dir / "notes.md").read_text(encoding="utf-8") == "FROM:notes.pdf" - assert not (thread_uploads_dir / "notes_1.md").exists() + assert result.files[1].markdown_file == "notes.pdf.md" + assert conversion_path_for_upload(thread_uploads_dir / "notes.pdf").read_text(encoding="utf-8") == "FROM:notes.pdf"