From afab7ed46ea8e3fee556757b7a96f327b8a756db Mon Sep 17 00:00:00 2001 From: Florian Valade Date: Wed, 22 Jul 2026 10:20:16 +0200 Subject: [PATCH] =?UTF-8?q?fix(photon):=20address=20review=20=E2=80=94=20U?= =?UTF-8?q?+FFFC=20before=20=5Frecord=5Flast=5Finbound,=20MIME-based=20CAF?= =?UTF-8?q?=20promotion,=20add=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move U+FFFC placeholder detection before _record_last_inbound() so the placeholder message id is not recorded as the reaction target - Cancel pending U+FFFC tasks in disconnect() to prevent task leaks - Check mimeType audio/x-caf in addition to filename for CAF→VOICE promotion, fixing unnamed attachments that default to '(unnamed)' - Add 7 Photon adapter tests: CAF named/unnamed promotion, U+FFFC no-dispatch, U+FFFC not recorded as last inbound, U+FFFC+attachment cancel, timeout fire, disconnect cleanup - Add 6 transcription tests: ffmpeg conversion, afconvert fallback, all-fail, CAF→WAV before Groq, conversion failure error, local provider skip --- plugins/platforms/photon/adapter.py | 50 ++-- .../plugins/platforms/photon/test_inbound.py | 218 ++++++++++++++++++ tests/tools/test_transcription_tools.py | 142 ++++++++++++ 3 files changed, 391 insertions(+), 19 deletions(-) diff --git a/plugins/platforms/photon/adapter.py b/plugins/platforms/photon/adapter.py index e9a8fec8646d..8daf743e2bf0 100644 --- a/plugins/platforms/photon/adapter.py +++ b/plugins/platforms/photon/adapter.py @@ -496,6 +496,11 @@ class PhotonAdapter(BasePlatformAdapter): except Exception: pass self._inbound_task = None + # Cancel any pending U+FFFC placeholder tasks. + for chat_key, (_, fffc_task) in list(self._pending_fffc.items()): + if fffc_task and not fffc_task.done(): + fffc_task.cancel() + self._pending_fffc.clear() await self._stop_sidecar() if self._http_client is not None: try: @@ -689,8 +694,10 @@ class PhotonAdapter(BasePlatformAdapter): is_voice = payload.get("type") == "voice" name = payload.get("name") or ("voice" if is_voice else "(unnamed)") mime = payload.get("mimeType") or "" - # Only .caf files are promoted to VOICE (iMessage voice notes use CAF). - if not is_voice and name.lower().endswith(".caf"): + # Promote CAF attachments to VOICE (iMessage voice notes use CAF). + # Check both filename and MIME: the sidecar may send "(unnamed)" + # when no name is supplied, so the MIME type is the reliable signal. + if not is_voice and (name.lower().endswith(".caf") or mime == "audio/x-caf"): is_voice = True mtype = MessageType.VOICE if is_voice else _attachment_message_type(mime) cached = _cache_inbound_attachment( @@ -762,6 +769,28 @@ class PhotonAdapter(BasePlatformAdapter): ) ) return + # U+FFFC placeholder — wait for the real attachment instead of + # dispatching. Detected before _record_last_inbound so the placeholder + # is not recorded as the reaction target — the real attachment will be. + if ctype == "text" and (content.get("text") or "").strip() == "\ufffc": + chat_key = space_id + prev = self._pending_fffc.pop(chat_key, None) + if prev and prev[1] and not prev[1].done(): + prev[1].cancel() + task = asyncio.create_task( + self._fffc_timeout_handler(chat_key, event.get("messageId") or "") + ) + self._pending_fffc[chat_key] = (time.monotonic(), task) + logger.debug("[photon] U+FFFC placeholder received — waiting for attachment") + return + + # Cancel any pending U+FFFC timeout — the real message arrived. + if ctype in {"attachment", "voice"}: + prev = self._pending_fffc.pop(space_id, None) + if prev and prev[1] and not prev[1].done(): + prev[1].cancel() + logger.debug("[photon] attachment arrived — cancelling U+FFFC timeout") + # Anything past here is a real (reactable) message — remember it as # the chat's latest inbound so `add_reaction` can target it when the # caller doesn't pass an explicit message id. Recorded before the @@ -769,25 +798,8 @@ class PhotonAdapter(BasePlatformAdapter): self._record_last_inbound(space_id, event.get("messageId")) if ctype == "text": text = content.get("text") or "" - # U+FFFC placeholder — wait for the real attachment instead of dispatching. - if text.strip() == "\ufffc": - chat_key = space_id - prev = self._pending_fffc.pop(chat_key, None) - if prev and prev[1] and not prev[1].done(): - prev[1].cancel() - task = asyncio.create_task( - self._fffc_timeout_handler(chat_key, event.get("messageId") or "") - ) - self._pending_fffc[chat_key] = (time.monotonic(), task) - logger.debug("[photon] U+FFFC placeholder received — waiting for attachment") - return mtype = MessageType.TEXT elif ctype in {"attachment", "voice"}: - # Cancel any pending U+FFFC timeout — the real attachment arrived. - prev = self._pending_fffc.pop(space_id, None) - if prev and prev[1] and not prev[1].done(): - prev[1].cancel() - logger.debug("[photon] attachment arrived — cancelling U+FFFC timeout") text, mtype, media_urls, media_types = _normalize_binary_payload(content) elif ctype == "group": text_parts: List[str] = [] diff --git a/tests/plugins/platforms/photon/test_inbound.py b/tests/plugins/platforms/photon/test_inbound.py index 521bc26aa915..3531e6f56882 100644 --- a/tests/plugins/platforms/photon/test_inbound.py +++ b/tests/plugins/platforms/photon/test_inbound.py @@ -6,6 +6,7 @@ sidecar-event parsing without spawning the Node sidecar or binding ports. """ from __future__ import annotations +import asyncio import base64 import json from pathlib import Path @@ -362,3 +363,220 @@ def test_check_requirements_without_node(monkeypatch: pytest.MonkeyPatch) -> Non monkeypatch.setattr(adapter_mod.shutil, "which", lambda _name: None) assert adapter_mod.check_requirements() is False + + +# --------------------------------------------------------------------------- +# CAF attachment promotion + U+FFFC placeholder tests +# --------------------------------------------------------------------------- + +_CAF_BYTES = b"caff" + b"\x00" * 60 # Minimal CAF header magic + + +def _caf_attachment_event( + content: Dict[str, Any], msg_id: str = "spc-msg-caf" +) -> Dict[str, Any]: + return { + "messageId": msg_id, + "space": {"id": "+155****4567", "type": "dm", "phone": "+155****4567"}, + "sender": {"id": "+155****4567"}, + "content": {"type": "attachment", **content}, + "timestamp": "2026-05-14T19:06:32.000Z", + } + + +@pytest.mark.asyncio +async def test_caf_attachment_named_promoted_to_voice( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A named .caf attachment is promoted to VOICE for STT routing.""" + adapter = _make_adapter(monkeypatch) + captured = _capture(adapter, monkeypatch) + + raw = _CAF_BYTES + event = _caf_attachment_event( + { + "name": "voice_note.caf", + "mimeType": "audio/x-caf", + "size": len(raw), + "data": base64.b64encode(raw).decode("ascii"), + "encoding": "base64", + } + ) + await adapter._dispatch_inbound(event) + + assert len(captured) == 1 + ev = captured[0] + assert ev.message_type == MessageType.VOICE + assert ev.media_types == ["audio/x-caf"] + assert len(ev.media_urls) == 1 + cached = Path(ev.media_urls[0]) + try: + assert cached.is_file() + assert cached.read_bytes() == raw + assert ev.text == "(voice)" + finally: + cached.unlink(missing_ok=True) + + +@pytest.mark.asyncio +async def test_caf_attachment_unnamed_promoted_via_mime( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An unnamed attachment with mimeType audio/x-caf is promoted to VOICE. + + The sidecar sends "(unnamed)" when no filename is supplied, so the MIME + type must be the fallback signal for CAF promotion. + """ + adapter = _make_adapter(monkeypatch) + captured = _capture(adapter, monkeypatch) + + raw = _CAF_BYTES + event = _caf_attachment_event( + { + "name": "(unnamed)", + "mimeType": "audio/x-caf", + "size": len(raw), + "data": base64.b64encode(raw).decode("ascii"), + "encoding": "base64", + } + ) + await adapter._dispatch_inbound(event) + + assert len(captured) == 1 + ev = captured[0] + assert ev.message_type == MessageType.VOICE + assert ev.media_types == ["audio/x-caf"] + cached = Path(ev.media_urls[0]) + try: + assert cached.is_file() + assert cached.suffix == ".caf" + finally: + cached.unlink(missing_ok=True) + + +@pytest.mark.asyncio +async def test_fffc_placeholder_no_dispatch( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A U+FFFC placeholder text does not trigger a message dispatch.""" + adapter = _make_adapter(monkeypatch) + captured = _capture(adapter, monkeypatch) + + event = _dm_event("\ufffc", msg_id="spc-msg-fffc") + chat_key = event["space"]["id"] + await adapter._dispatch_inbound(event) + + assert len(captured) == 0 + assert chat_key in adapter._pending_fffc + + +@pytest.mark.asyncio +async def test_fffc_placeholder_not_recorded_as_last_inbound( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The U+FFFC placeholder must not be recorded as the reaction target. + + _record_last_inbound runs after the U+FFFC early-return, so the placeholder + message id is never stored. A subsequent real message will be recorded. + """ + adapter = _make_adapter(monkeypatch) + _capture(adapter, monkeypatch) + + fffc_event = _dm_event("\ufffc", msg_id="spc-msg-fffc") + chat_key = fffc_event["space"]["id"] + await adapter._dispatch_inbound(fffc_event) + assert chat_key not in adapter._last_inbound_by_chat + + real_event = _dm_event("hello", msg_id="spc-msg-real") + await adapter._dispatch_inbound(real_event) + assert adapter._last_inbound_by_chat.get(chat_key) == "spc-msg-real" + + +@pytest.mark.asyncio +async def test_fffc_then_attachment_cancels_timeout( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """When an attachment arrives after a U+FFFC placeholder, the pending + timeout task is cancelled and the attachment is dispatched normally.""" + adapter = _make_adapter(monkeypatch) + captured = _capture(adapter, monkeypatch) + + fffc_event = _dm_event("\ufffc", msg_id="spc-msg-fffc") + chat_key = fffc_event["space"]["id"] + await adapter._dispatch_inbound(fffc_event) + assert len(captured) == 0 + assert chat_key in adapter._pending_fffc + + raw = _CAF_BYTES + att_event = _caf_attachment_event( + { + "name": "voice.caf", + "mimeType": "audio/x-caf", + "size": len(raw), + "data": base64.b64encode(raw).decode("ascii"), + "encoding": "base64", + }, + msg_id="spc-msg-att", + ) + att_event["space"]["id"] = chat_key + await adapter._dispatch_inbound(att_event) + + assert len(captured) == 1 + assert captured[0].message_type == MessageType.VOICE + assert chat_key not in adapter._pending_fffc + assert adapter._last_inbound_by_chat.get(chat_key) == "spc-msg-att" + + cached = Path(captured[0].media_urls[0]) + try: + assert cached.is_file() + finally: + cached.unlink(missing_ok=True) + + +@pytest.mark.asyncio +async def test_fffc_timeout_fires_when_no_attachment( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """When no attachment arrives within the timeout, the pending entry is + cleaned up and a warning is logged.""" + import plugins.platforms.photon.adapter as adapter_mod + + monkeypatch.setattr(adapter_mod, "_FFFC_WAIT_SECONDS", 0.1) + + adapter = _make_adapter(monkeypatch) + captured = _capture(adapter, monkeypatch) + + fffc_event = _dm_event("\ufffc", msg_id="spc-msg-fffc") + chat_key = fffc_event["space"]["id"] + await adapter._dispatch_inbound(fffc_event) + assert chat_key in adapter._pending_fffc + + await asyncio.sleep(0.3) + + assert chat_key not in adapter._pending_fffc + assert len(captured) == 0 + + +@pytest.mark.asyncio +async def test_disconnect_cancels_pending_fffc_tasks( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """disconnect() cancels any pending U+FFFC placeholder tasks.""" + adapter = _make_adapter(monkeypatch) + _capture(adapter, monkeypatch) + + await adapter._dispatch_inbound(_dm_event("\ufffc", msg_id="spc-msg-fffc")) + assert len(adapter._pending_fffc) == 1 + + async def _noop_stop_sidecar(): + pass + + monkeypatch.setattr(adapter, "_stop_sidecar", _noop_stop_sidecar) + monkeypatch.setattr(adapter, "_inbound_running", False) + monkeypatch.setattr(adapter, "_inbound_task", None) + monkeypatch.setattr(adapter, "_sidecar_health_task", None) + monkeypatch.setattr(adapter, "_http_client", None) + + await adapter.disconnect() + + assert len(adapter._pending_fffc) == 0 diff --git a/tests/tools/test_transcription_tools.py b/tests/tools/test_transcription_tools.py index ee0287ca01f6..1f8032b5e6b4 100644 --- a/tests/tools/test_transcription_tools.py +++ b/tests/tools/test_transcription_tools.py @@ -11,6 +11,7 @@ import struct import subprocess import types import wave +from pathlib import Path from unittest.mock import MagicMock, call, patch import pytest @@ -2173,3 +2174,144 @@ class TestLocalBaseUrlNoApiKey: assert _is_local_or_private_url("http://stt.internal/v1") assert not _is_local_or_private_url("https://api.openai.com/v1") assert not _is_local_or_private_url("") + + +# ============================================================================ +# CAF (iMessage voice note) conversion tests +# ============================================================================ + +class TestCafConversion: + """Tests for _convert_caf_to_wav and CAF dispatch in transcribe_audio.""" + + def test_convert_caf_with_ffmpeg(self, tmp_path, monkeypatch): + """_convert_caf_to_wav uses ffmpeg when available.""" + caf_path = tmp_path / "voice.caf" + caf_path.write_bytes(b"caff\x00" * 20) + wav_path = str(tmp_path / "voice.wav") + + def fake_run(cmd, **kwargs): + Path(wav_path).write_bytes(b"RIFF\x00\x00\x00\x00") + return MagicMock(returncode=0) + + monkeypatch.setattr( + "tools.transcription_tools._find_ffmpeg_binary", + lambda: "/usr/bin/ffmpeg", + ) + monkeypatch.setattr(subprocess, "run", fake_run) + + from tools.transcription_tools import _convert_caf_to_wav + result = _convert_caf_to_wav(str(caf_path)) + assert result == wav_path + assert Path(result).exists() + + def test_convert_caf_fallback_to_afconvert(self, tmp_path, monkeypatch): + """When ffmpeg is not found, falls back to afconvert (macOS).""" + caf_path = tmp_path / "voice.caf" + caf_path.write_bytes(b"caff\x00" * 20) + wav_path = str(tmp_path / "voice.wav") + + call_count = {"n": 0} + + def fake_run(cmd, **kwargs): + call_count["n"] += 1 + if cmd[0] == "/usr/bin/ffmpeg": + raise subprocess.CalledProcessError(1, cmd) + Path(wav_path).write_bytes(b"RIFF\x00\x00\x00\x00") + return MagicMock(returncode=0) + + monkeypatch.setattr( + "tools.transcription_tools._find_ffmpeg_binary", + lambda: "/usr/bin/ffmpeg", + ) + monkeypatch.setattr(subprocess, "run", fake_run) + monkeypatch.setattr( + "tools.transcription_tools.shutil.which", + lambda name: "/usr/bin/afconvert" if name == "afconvert" else None, + ) + + from tools.transcription_tools import _convert_caf_to_wav + result = _convert_caf_to_wav(str(caf_path)) + assert result == wav_path + assert call_count["n"] == 2 + + def test_convert_caf_all_converters_fail(self, tmp_path, monkeypatch): + """When both ffmpeg and afconvert are unavailable, returns None.""" + caf_path = tmp_path / "voice.caf" + caf_path.write_bytes(b"caff\x00" * 20) + + monkeypatch.setattr( + "tools.transcription_tools._find_ffmpeg_binary", lambda: None + ) + monkeypatch.setattr( + "tools.transcription_tools.shutil.which", lambda name: None + ) + + from tools.transcription_tools import _convert_caf_to_wav + result = _convert_caf_to_wav(str(caf_path)) + assert result is None + + def test_transcribe_caf_converted_before_groq(self, tmp_path, monkeypatch): + """transcribe_audio converts .caf to .wav before dispatching to Groq.""" + caf_path = tmp_path / "voice.caf" + caf_path.write_bytes(b"caff\x00" * 20) + wav_path = str(tmp_path / "voice.wav") + + def fake_convert(file_path): + Path(wav_path).write_bytes(b"RIFF\x00\x00\x00\x00") + return wav_path + + with patch("tools.transcription_tools._load_stt_config", + return_value={"provider": "groq"}), \ + patch("tools.transcription_tools._get_provider", + return_value="groq"), \ + patch("tools.transcription_tools._convert_caf_to_wav", + side_effect=fake_convert) as mock_convert, \ + patch("tools.transcription_tools._transcribe_groq", + return_value={"success": True, "transcript": "hello", + "provider": "groq"}) as mock_groq: + from tools.transcription_tools import transcribe_audio + result = transcribe_audio(str(caf_path)) + + assert result["success"] is True + mock_convert.assert_called_once_with(str(caf_path)) + mock_groq.assert_called_once() + call_args = mock_groq.call_args + sent_path = call_args[0][0] if call_args[0] else call_args[1].get("file_path") + assert sent_path == wav_path + + def test_transcribe_caf_conversion_failure_returns_error( + self, tmp_path, monkeypatch + ): + """When CAF conversion fails, transcribe_audio returns an error.""" + caf_path = tmp_path / "voice.caf" + caf_path.write_bytes(b"caff\x00" * 20) + + with patch("tools.transcription_tools._load_stt_config", + return_value={"provider": "groq"}), \ + patch("tools.transcription_tools._get_provider", + return_value="groq"), \ + patch("tools.transcription_tools._convert_caf_to_wav", + return_value=None): + from tools.transcription_tools import transcribe_audio + result = transcribe_audio(str(caf_path)) + + assert result["success"] is False + assert "could not be converted" in result["error"] + + def test_transcribe_caf_not_converted_for_local(self, tmp_path, monkeypatch): + """CAF conversion is skipped for local provider (native handling).""" + caf_path = tmp_path / "voice.caf" + caf_path.write_bytes(b"caff\x00" * 20) + + with patch("tools.transcription_tools._load_stt_config", + return_value={"provider": "local"}), \ + patch("tools.transcription_tools._get_provider", + return_value="local"), \ + patch("tools.transcription_tools._convert_caf_to_wav") as mock_convert, \ + patch("tools.transcription_tools._transcribe_local", + return_value={"success": True, "transcript": "hi"}): + from tools.transcription_tools import transcribe_audio + result = transcribe_audio(str(caf_path)) + + assert result["success"] is True + mock_convert.assert_not_called()