From eeb3fa5c6641ff32aef577452af030918b3f9252 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 3 May 2026 18:59:41 +0000 Subject: [PATCH] refactor: use __len__ for MailDebugRecorder, str() size estimate, clearer tests, fix unused import Agent-Logs-Url: https://github.com/christianlouis/InboxConverge/sessions/5d2918de-630e-4a66-8355-1b036c620b1c Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- backend/app/services/mail_processor.py | 6 ++- backend/app/workers/tasks.py | 2 +- .../unit/test_format_connection_error.py | 42 ++++++++++--------- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/backend/app/services/mail_processor.py b/backend/app/services/mail_processor.py index aee1668..f54f01b 100644 --- a/backend/app/services/mail_processor.py +++ b/backend/app/services/mail_processor.py @@ -4,7 +4,6 @@ Supports both POP3 and IMAP protocols with secure connections. """ import asyncio -import json as _json import poplib import re import smtplib @@ -198,7 +197,7 @@ class MailDebugRecorder: if data: entry["data"] = data try: - entry_bytes = len(_json.dumps(entry, default=str)) + entry_bytes = len(str(entry)) except Exception: entry_bytes = 256 # conservative fallback if ( @@ -223,6 +222,9 @@ class MailDebugRecorder: def has_entries(self) -> bool: return bool(self._entries) + def __len__(self) -> int: + return len(self._entries) + def as_details(self) -> Dict[str, Any]: """Return the trace as a dict suitable for storage in error_details.""" return {"trace": self._entries, "truncated": self._truncated} diff --git a/backend/app/workers/tasks.py b/backend/app/workers/tasks.py index d751294..c9a5434 100644 --- a/backend/app/workers/tasks.py +++ b/backend/app/workers/tasks.py @@ -479,7 +479,7 @@ async def process_mail_account(account_id: int): mail_account_id=account.id, processing_run_id=run.id, level="DEBUG", - message=f"Connection trace ({len(_debug_recorder._entries)} entries)", + message=f"Connection trace ({len(_debug_recorder)} entries)", success=True, error_details=_debug_recorder.as_details(), ) diff --git a/backend/tests/unit/test_format_connection_error.py b/backend/tests/unit/test_format_connection_error.py index f0a94c2..7cb3630 100644 --- a/backend/tests/unit/test_format_connection_error.py +++ b/backend/tests/unit/test_format_connection_error.py @@ -64,7 +64,9 @@ class TestFormatConnectionErrorTimeout: def test_socket_timeout_includes_host_port(self): exc = socket.timeout("timed out") msg = _fmt(exc, host="imap.gmx.net", port=993) - assert "imap.gmx.net:993" in msg + # Verify both the host and timeout indicator appear in the message + assert "imap.gmx.net" in msg + assert "993" in msg assert "timed out" in msg.lower() or "timeout" in msg.lower() def test_asyncio_timeout_error(self): @@ -89,7 +91,9 @@ class TestFormatConnectionErrorRefused: def test_connection_refused_includes_host_port(self): exc = ConnectionRefusedError(111, "Connection refused") msg = _fmt(exc, host="smtp.example.com", port=587) - assert "smtp.example.com:587" in msg + # Verify both host and port appear in the message + assert "smtp.example.com" in msg + assert "587" in msg assert "refused" in msg.lower() def test_connection_reset(self): @@ -133,7 +137,9 @@ class TestFormatConnectionErrorPop3: def test_auth_error_identifies_auth_failure(self): exc = poplib.error_proto("-ERR authentication failed") msg = _format_connection_error(exc, "pop.example.com", 995, "POP3") - assert "pop.example.com:995" in msg + # Verify both host and port appear in the message + assert "pop.example.com" in msg + assert "995" in msg assert "Authentication" in msg or "authentication" in msg def test_generic_pop3_error(self): @@ -210,9 +216,9 @@ class TestMailDebugRecorder: details = rec.as_details() assert details["truncated"] # After truncation, recording new entries is a no-op - prev_count = len(details["trace"]) + prev_count = len(rec) rec.record("after_truncate", "should be ignored") - assert len(rec.as_details()["trace"]) == prev_count + assert len(rec) == prev_count def test_entry_cap_triggers_truncation(self): """Recording more than _MAX_TRACE_ENTRIES entries truncates.""" @@ -223,28 +229,24 @@ class TestMailDebugRecorder: # Total entries should be MAX + 1 (the truncation sentinel) assert len(rec.as_details()["trace"]) <= _MAX_TRACE_ENTRIES + 1 - def test_no_password_in_trace(self): - """Passwords must never appear in any trace entry.""" + def test_no_password_in_trace_from_production_phases(self): + """Production instrumentation records only usernames and counts, never passwords. + + The recorder itself has no auto-redaction; the contract is that callers + (the instrumented IMAP/POP3 code) must never pass credential data. + This test verifies the expected production-phase entries contain no + password strings. + """ password = "s3cr3tP@ssw0rd!" rec = MailDebugRecorder() - # Simulate what instrumented code records — only timing and counts + # Simulate the entries that production code actually records rec.record("auth", f"Authenticated as user@example.com", {"elapsed_ms": 5}) rec.record("stat", "Mailbox has 3 messages", {"count": 3}) - # A bug might accidentally include the password — assert it doesn't - rec.record("bad_attempt", f"user={password}") # test robustness import json - serialised = json.dumps(rec.as_details()) - # The entries only include the phase/msg/data we explicitly pass in - # production code — we just verify the test doesn't accidentally - # redact by accident. More importantly, prod code never passes the - # actual password object, only usernames and counts. - # Here we did pass it deliberately, so it IS present — the real - # protection is in the instrumentation not passing it. - # This test documents that the recorder itself has no auto-redaction - # (the contract is that callers must not pass credentials). - assert serialised # non-empty serialisation + # The password must not appear in any of these entries + assert password not in serialised def test_timestamps_are_iso_format(self): rec = MailDebugRecorder()