diff --git a/src/basic_memory/api/v2/routers/importer_router.py b/src/basic_memory/api/v2/routers/importer_router.py index aa5452b73..0fa9e764f 100644 --- a/src/basic_memory/api/v2/routers/importer_router.py +++ b/src/basic_memory/api/v2/routers/importer_router.py @@ -191,6 +191,15 @@ async def import_memory_json( ) except HTTPException: raise + except (json.JSONDecodeError, UnicodeDecodeError) as e: + # Trigger: a line in the upload is not valid JSON, or the bytes are not + # UTF-8 (truncated archive, wrong file, binary upload). + # Why: this is a client-input problem, not a server fault (#1276). + # Outcome: a 400 with the parse position instead of an opaque 500. + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=f"Import file is not valid JSON: {e}", + ) except Exception as e: logger.exception("V2 Import failed") raise HTTPException( @@ -244,6 +253,15 @@ async def import_file[ImportResultT: ImportResult]( except HTTPException: raise + except (json.JSONDecodeError, UnicodeDecodeError) as e: + # Trigger: the upload is not valid JSON, or the bytes are not UTF-8 + # (truncated archive, wrong file, binary upload). + # Why: this is a client-input problem, not a server fault (#1276). + # Outcome: a 400 with the parse position instead of an opaque 500. + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=f"Import file is not valid JSON: {e}", + ) except Exception as e: logger.exception("V2 Import failed") raise HTTPException( diff --git a/src/basic_memory/importers/chatgpt_importer.py b/src/basic_memory/importers/chatgpt_importer.py index 97348535c..b5bce7699 100644 --- a/src/basic_memory/importers/chatgpt_importer.py +++ b/src/basic_memory/importers/chatgpt_importer.py @@ -11,6 +11,11 @@ logger = logging.getLogger(__name__) +# One day past the Unix epoch: deterministic, still obviously a sentinel, and +# never a pre-epoch value in any local timezone — Windows' CRT raises OSError +# converting epoch-adjacent times through fromtimestamp/astimezone. +UNKNOWN_DATE_SENTINEL = 86400.0 + class ChatGPTImporter(Importer[ChatImportResult]): """Service for importing ChatGPT conversations.""" @@ -53,7 +58,7 @@ async def import_data( chats_imported = 0 for chat in conversations: - created_at = chat["create_time"] + created_at, modified_at = self._resolve_timestamps(chat) date_prefix = datetime.fromtimestamp(created_at).astimezone().strftime("%Y%m%d") clean_title = clean_filename(chat["title"]) relative_path = ( @@ -64,7 +69,7 @@ async def import_data( permalink, file_path = self.build_import_paths(relative_path) # Convert to entity - entity = self._format_chat_content(chat, permalink) + entity = self._format_chat_content(chat, permalink, created_at, modified_at) # Write file using relative path - FileService handles base_path await self.write_entity(entity, file_path) @@ -93,22 +98,58 @@ async def import_data( logger.exception("Failed to import ChatGPT conversations") return self.handle_error("Failed to import ChatGPT conversations", e) + def _resolve_timestamps(self, conversation: Dict[str, Any]) -> tuple[float, float]: + """Resolve conversation timestamps, tolerating absent fields. + + OpenAI's export format does not guarantee `create_time` or `update_time` + on every conversation object (#1276). Fall back in order: the earliest + message timestamp, the conversation's `update_time`, an epoch sentinel. + Every rung must stay stable across re-exports — the resolved date names + the output file (`YYYYMMDD-title.md`), so a value that changes between + exports would make the next import write a duplicate note under a new + name instead of updating the original. That is why the earliest message + time outranks `update_time` (existing messages keep their timestamps + while `update_time` advances whenever the conversation continues) and + why the last resort is a fixed sentinel whose obviously-wrong 1970 + prefix reads as "date unknown" rather than faking a plausible one. + + Args: + conversation: ChatGPT conversation data. + + Returns: + Tuple of (created_at, modified_at) unix timestamps. + """ + created_at = conversation.get("create_time") + modified_at = conversation.get("update_time") + if created_at is None: + message_times = [ + node["message"]["create_time"] + for node in conversation.get("mapping", {}).values() + if node.get("message") and node["message"].get("create_time") is not None + ] + created_at = min(message_times) if message_times else None + if created_at is None: + created_at = modified_at + if created_at is None: + created_at = UNKNOWN_DATE_SENTINEL + if modified_at is None: + modified_at = created_at + return created_at, modified_at + def _format_chat_content( - self, conversation: Dict[str, Any], permalink: str + self, conversation: Dict[str, Any], permalink: str, created_at: float, modified_at: float ) -> EntityMarkdown: # pragma: no cover """Convert chat conversation to Basic Memory entity. Args: - folder: Destination folder name. conversation: ChatGPT conversation data. + permalink: Permalink for the entity. + created_at: Resolved creation timestamp. + modified_at: Resolved modification timestamp. Returns: EntityMarkdown instance representing the conversation. """ - # Extract timestamps - created_at = conversation["create_time"] - modified_at = conversation["update_time"] - root_id = None # Find root message for node_id, node in conversation["mapping"].items(): diff --git a/tests/api/v2/test_importer_router.py b/tests/api/v2/test_importer_router.py index 7364d0e85..6ffc94c2c 100644 --- a/tests/api/v2/test_importer_router.py +++ b/tests/api/v2/test_importer_router.py @@ -197,9 +197,33 @@ async def test_import_chatgpt_invalid_file(client: AsyncClient, tmp_path, v2_pro # Send request - this should return an error response = await client.post(f"{v2_project_url}/import/chatgpt", files=files, data=data) - # Check response - assert response.status_code == 500 - assert "Import failed" in response.json()["detail"] + # Check response: undecodable JSON is a client-input problem, not a server fault (#1276) + assert response.status_code == 400 + assert "not valid JSON" in response.json()["detail"] + + +@pytest.mark.asyncio +async def test_import_chatgpt_invalid_utf8_file(client: AsyncClient, v2_project_url: str): + """Invalid UTF-8 bytes are a client-input problem too, not a 500 (#1276).""" + files = {"file": ("invalid.json", b"\xff\xfe not utf-8", "application/json")} + data = {"directory": "test_chatgpt"} + + response = await client.post(f"{v2_project_url}/import/chatgpt", files=files, data=data) + + assert response.status_code == 400 + assert "not valid JSON" in response.json()["detail"] + + +@pytest.mark.asyncio +async def test_import_memory_json_invalid_utf8_file(client: AsyncClient, v2_project_url: str): + """The line-oriented memory-json decode rejects invalid UTF-8 with a 400 as well.""" + files = {"file": ("memory.json", b"\xff\xfe not utf-8", "application/json")} + data = {"directory": "test_memory"} + + response = await client.post(f"{v2_project_url}/import/memory-json", files=files, data=data) + + assert response.status_code == 400 + assert "not valid JSON" in response.json()["detail"] @pytest.mark.asyncio @@ -261,9 +285,9 @@ async def test_import_claude_conversations_invalid_file( f"{v2_project_url}/import/claude/conversations", files=files, data=data ) - # Check response - assert response.status_code == 500 - assert "Import failed" in response.json()["detail"] + # Check response: undecodable JSON is a client-input problem, not a server fault (#1276) + assert response.status_code == 400 + assert "not valid JSON" in response.json()["detail"] @pytest.mark.asyncio @@ -326,9 +350,9 @@ async def test_import_claude_projects_invalid_file( f"{v2_project_url}/import/claude/projects", files=files, data=data ) - # Check response - assert response.status_code == 500 - assert "Import failed" in response.json()["detail"] + # Check response: undecodable JSON is a client-input problem, not a server fault (#1276) + assert response.status_code == 400 + assert "not valid JSON" in response.json()["detail"] @pytest.mark.asyncio @@ -414,9 +438,9 @@ async def test_import_memory_json_invalid_file(client: AsyncClient, tmp_path, v2 # Send request - this should return an error response = await client.post(f"{v2_project_url}/import/memory-json", files=files, data=data) - # Check response - assert response.status_code == 500 - assert "Import failed" in response.json()["detail"] + # Check response: undecodable JSON is a client-input problem, not a server fault (#1276) + assert response.status_code == 400 + assert "not valid JSON" in response.json()["detail"] @pytest.mark.asyncio @@ -515,9 +539,9 @@ async def test_import_empty_file(client: AsyncClient, tmp_path, v2_project_url: # Send request response = await client.post(f"{v2_project_url}/import/chatgpt", files=files, data=data) - # Check response - assert response.status_code == 500 - assert "Import failed" in response.json()["detail"] + # Check response: undecodable JSON is a client-input problem, not a server fault (#1276) + assert response.status_code == 400 + assert "not valid JSON" in response.json()["detail"] @pytest.mark.asyncio @@ -544,6 +568,6 @@ async def test_import_malformed_json(client: AsyncClient, tmp_path, v2_project_u # Send request response = await client.post(endpoint, files=files, data=data) - # Check response - assert response.status_code == 500 - assert "Import failed" in response.json()["detail"] + # Check response: undecodable JSON is a client-input problem, not a server fault (#1276) + assert response.status_code == 400 + assert "not valid JSON" in response.json()["detail"] diff --git a/tests/cli/test_import_chatgpt.py b/tests/cli/test_import_chatgpt.py index b6eb62bab..5b87d1813 100644 --- a/tests/cli/test_import_chatgpt.py +++ b/tests/cli/test_import_chatgpt.py @@ -1,6 +1,7 @@ """Tests for import_chatgpt command.""" import json +from datetime import datetime import pytest from typer.testing import CliRunner @@ -8,6 +9,7 @@ from basic_memory.cli.app import app, import_app from basic_memory.cli.commands import import_chatgpt # noqa from basic_memory.config import get_project_config +from basic_memory.importers.chatgpt_importer import UNKNOWN_DATE_SENTINEL # Set up CLI runner runner = CliRunner() @@ -197,3 +199,91 @@ def test_import_chatgpt_with_custom_folder(tmp_path, sample_chatgpt_json, monkey # Check files in custom folder conv_path = tmp_path / conversations_folder / "20250111-Test_Conversation.md" assert conv_path.exists() + + +def test_import_chatgpt_missing_create_time(tmp_path, sample_conversation): + """Without create_time, the earliest message time wins over update_time (#1276). + + update_time advances whenever a conversation continues, so using it while + messages carry timestamps would rename the output file on the next export + and fork a duplicate note. The messages' 2025-01-11 prefix proves the + immutable rung was chosen over the later update_time day. + """ + config = get_project_config() + config.home = tmp_path + del sample_conversation["create_time"] + sample_conversation["update_time"] = 1736703003.0 # 2025-01-12, later than messages + json_file = tmp_path / "conversations.json" + json_file.write_text(json.dumps([sample_conversation]), encoding="utf-8") + + result = runner.invoke(app, ["import", "chatgpt", str(json_file), "--folder", "chats"]) + assert result.exit_code == 0 + assert "Imported 1 conversations" in result.output + + conv_path = tmp_path / "chats" / "20250111-Test_Conversation.md" + assert conv_path.exists() + + +def test_import_chatgpt_update_time_rung_when_no_message_times(tmp_path, sample_conversation): + """update_time is the fallback only when no message carries a timestamp (#1276).""" + config = get_project_config() + config.home = tmp_path + del sample_conversation["create_time"] + sample_conversation["update_time"] = 1736703003.0 # 2025-01-12 + for node in sample_conversation["mapping"].values(): + if node.get("message"): + node["message"]["create_time"] = None + json_file = tmp_path / "conversations.json" + json_file.write_text(json.dumps([sample_conversation]), encoding="utf-8") + + result = runner.invoke(app, ["import", "chatgpt", str(json_file), "--folder", "chats"]) + assert result.exit_code == 0 + assert "Imported 1 conversations" in result.output + + conv_path = tmp_path / "chats" / "20250112-Test_Conversation.md" + assert conv_path.exists() + + +def test_import_chatgpt_null_create_time(tmp_path, sample_conversation): + """Null conversation timestamps fall back to the earliest message time (#1276).""" + config = get_project_config() + config.home = tmp_path + sample_conversation["create_time"] = None + sample_conversation["update_time"] = None + # msg1 has no usable time either; msg2 keeps its timestamp and becomes the fallback + sample_conversation["mapping"]["msg1"]["message"]["create_time"] = None + json_file = tmp_path / "conversations.json" + json_file.write_text(json.dumps([sample_conversation]), encoding="utf-8") + + result = runner.invoke(app, ["import", "chatgpt", str(json_file), "--folder", "chats"]) + assert result.exit_code == 0 + assert "Imported 1 conversations" in result.output + assert "Containing 2 messages" in result.output + + conv_path = tmp_path / "chats" / "20250111-Test_Conversation.md" + assert conv_path.exists() + + +def test_import_chatgpt_no_timestamps_anywhere_is_deterministic(tmp_path, sample_conversation): + """With no usable timestamp at all, the epoch sentinel keeps reimports stable (#1276). + + The resolved date names the output file, so an import-time fallback would + write a duplicate note under a new name on a later reimport. + """ + config = get_project_config() + config.home = tmp_path + sample_conversation["create_time"] = None + sample_conversation["update_time"] = None + for node in sample_conversation["mapping"].values(): + if node.get("message"): + node["message"]["create_time"] = None + json_file = tmp_path / "conversations.json" + json_file.write_text(json.dumps([sample_conversation]), encoding="utf-8") + + result = runner.invoke(app, ["import", "chatgpt", str(json_file), "--folder", "chats"]) + assert result.exit_code == 0 + assert "Imported 1 conversations" in result.output + + epoch_prefix = datetime.fromtimestamp(UNKNOWN_DATE_SENTINEL).astimezone().strftime("%Y%m%d") + conv_path = tmp_path / "chats" / f"{epoch_prefix}-Test_Conversation.md" + assert conv_path.exists()