From b0f06f2421371751ac75792cc80e796490f308ce Mon Sep 17 00:00:00 2001 From: Mickael Farina Date: Sat, 23 May 2026 15:30:16 +0200 Subject: [PATCH] refactor(bridges): extract shared codec_bridges core (A-19, Audit-A complete) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-3F, Option 1 (scoped). The audit's full BridgeRouter (unifying process_message) was high-risk/low-value: the headline call_llm dup was already fixed in #71, and the two process_message flows have intentionally drifted (telegram audio/Gemini/ briefing; imessage goals/intent) — forcing them into one pipeline would risk regressing a working channel. So this extracts ONLY the genuinely-identical helpers and leaves process_message per-bridge. New codec_bridges.py: - load_dispatch() / try_skill(text) — the lazy codec_dispatch loader + skill match (+ the _SKIP_SKILLS terminal/GUI set), one copy. - call_llm(channel, text, llm_cfg, ...) — the canonical bridge LLM call: persona selected by channel, routed through codec_llm.call, None-contract preserved, chat_template_kwargs filtered. - save_to_memory(channel, conv_id, user, assistant) — the memory.db write under session_id "-". codec_telegram + codec_imessage now `from codec_bridges import try_skill` and keep thin channel-injecting wrappers for call_llm / save_to_memory, so every process_message call site is unchanged (telegram audio + imessage goal-tracking untouched). Removed telegram's now-dead sqlite3 import. codec_bridges.py is the documented "add a channel" seed (CLAUDE.md §1: future WhatsApp/Discord). Updated 2 stale #71 source-invariants (the bridges now delegate via codec_bridges rather than calling codec_llm.call directly). Tests: tests/test_bridges.py (10 — call_llm persona/None/kwargs-filter/history+ override, try_skill skip-set/run/no-match, save_to_memory channel-prefixed session_id, source invariants). Full suite 1502 passing, 23 known-baseline failures, zero new. Net -148 LOC in the bridges. Zero net-new ruff. **Audit-A (Wave 3, code quality) is complete** — A-1..A-22 all closed or verified. Co-Authored-By: Claude Opus 4.7 --- AGENTS.md | 5 +- codec_bridges.py | 139 +++++++++++++++++++++ codec_imessage.py | 113 +++-------------- codec_telegram.py | 99 +++------------ docs/PR3F-BRIDGE-UNIFICATION-DESIGN.md | 43 +++++++ docs/audits/PHASE-1-CODE-QUALITY.md | 2 + docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md | 2 +- tests/test_bridges.py | 118 +++++++++++++++++ tests/test_llm_bridges.py | 7 +- 9 files changed, 340 insertions(+), 188 deletions(-) create mode 100644 codec_bridges.py create mode 100644 docs/PR3F-BRIDGE-UNIFICATION-DESIGN.md create mode 100644 tests/test_bridges.py diff --git a/AGENTS.md b/AGENTS.md index ba4da40..1a3545d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,8 +43,9 @@ codec_session.py Session lifecycle codec_sandbox.py Sandboxed file/shell wrappers codec_config.py Config + dangerous-pattern detection + _HTTP_BLOCKED codec_ava_client.py AVA cloud proxy client (Gemini/Claude/GPT routing for paid users) -codec_imessage.py iMessage outbound bridge -codec_telegram.py Telegram outbound bridge +codec_imessage.py iMessage outbound bridge (thin adapter over codec_bridges) +codec_telegram.py Telegram outbound bridge (thin adapter over codec_bridges) +codec_bridges.py Shared outbound-bridge core (A-19): load_dispatch/try_skill, call_llm(channel,…), save_to_memory(channel,…). The "add a channel" surface; process_message stays per-bridge whisper_server.py Local STT server routes/agents.py /api/agents/* endpoints (custom agents, crew launcher) routes/_shared.py notifications.json read/write helpers diff --git a/codec_bridges.py b/codec_bridges.py new file mode 100644 index 0000000..a63842a --- /dev/null +++ b/codec_bridges.py @@ -0,0 +1,139 @@ +"""CODEC outbound-bridge shared core (A-19, PR-3F). + +The genuinely-identical helpers that `codec_telegram` and `codec_imessage` each +had their own copy of: skill dispatch (`load_dispatch`/`try_skill`), the +canonical bridge LLM call (`call_llm`, persona chosen by channel, via the +canonical `codec_llm.call`), and memory persistence (`save_to_memory`). Each +bridge keeps its OWN `process_message` — those have intentionally drifted +(telegram: audio transcription + Gemini fallback + daily briefing; imessage: +goal tracking + intent classification) and unifying them would risk regressing a +working channel. This module is the seed for an "add a channel" surface +(CLAUDE.md §1: future WhatsApp / Discord). + +Inbound stays PWA-only — these helpers are OUTBOUND only. +""" +from __future__ import annotations + +import logging +import os +import sqlite3 +from datetime import datetime + +log = logging.getLogger("codec.bridges") + +MEMORY_DB = os.path.expanduser("~/.codec/memory.db") + +# Skills that need a terminal / GUI and must not run from a headless bridge. +_SKIP_SKILLS = {"open_terminal", "run_command", "vibe_code", "deep_chat", + "memory_search", "ask_mike_to_build"} + +# Per-channel assistant persona (the only thing that differed between the two +# bridges' call_llm). `{now}` is the formatted current date/time. +_PERSONAS = { + "telegram": ( + "You are CODEC, a personal AI assistant replying via Telegram. " + "Today is {now}. Be concise and direct. " + "Keep replies under 3 sentences unless more detail is needed. " + "You can use Markdown formatting. Be natural and helpful." + ), + "imessage": ( + "You are CODEC, a personal AI assistant replying via iMessage. " + "Today is {now}. Be concise — this is a text message conversation. " + "Keep replies under 3 sentences unless more detail is needed. " + "Be natural and conversational, like texting a smart friend." + ), +} + +# ── Skill dispatch (lazy — codec_dispatch pulls pynput/GUI deps) ────────────── +_dispatch_loaded = False +_check_skill = None +_run_skill = None + + +def load_dispatch() -> bool: + """Lazy-load codec_dispatch (handling pynput/GUI import issues). Returns + True if skill dispatch is available.""" + global _dispatch_loaded, _check_skill, _run_skill + if _dispatch_loaded: + return _check_skill is not None + _dispatch_loaded = True + try: + from codec_dispatch import check_skill, run_skill + _check_skill = check_skill + _run_skill = run_skill + log.info("Skill dispatch loaded") + return True + except Exception as e: + log.warning(f"Skill dispatch unavailable ({e}) — LLM-only mode") + return False + + +def try_skill(text): + """Match a CODEC skill for `text`. Returns (skill_name, result) or + (None, None) — skipping terminal/GUI skills that a bridge can't run.""" + if not load_dispatch(): + return (None, None) + try: + skill = _check_skill(text) + if skill: + if skill["name"] in _SKIP_SKILLS: + return (None, None) + result = _run_skill(skill, text) + if result: + return (skill["name"], str(result)) + except Exception as e: + log.warning(f"Skill error: {e}") + return (None, None) + + +# ── Canonical bridge LLM call ──────────────────────────────────────────────── +def call_llm(channel, text, llm_cfg, conversation_history=None, + system_prompt_override=None): + """The shared outbound-bridge LLM call. `channel` selects the persona; + routes through codec_llm.call (default never-raise → "" → None for graceful + bridge degradation). `chat_template_kwargs` is filtered out of + `llm_cfg["kwargs"]` so codec_llm's enable_thinking=False is preserved.""" + import codec_llm + if system_prompt_override: + sys_prompt = system_prompt_override + else: + now_str = datetime.now().strftime("%A %B %d, %Y at %H:%M") + sys_prompt = _PERSONAS[channel].format(now=now_str) + + messages = [{"role": "system", "content": sys_prompt}] + if conversation_history: + messages.extend(conversation_history[-8:]) + messages.append({"role": "user", "content": text}) + + extra = {k: v for k, v in llm_cfg["kwargs"].items() if k != "chat_template_kwargs"} + content = codec_llm.call( + messages, base_url=llm_cfg["base_url"], model=llm_cfg["model"], + api_key=llm_cfg["api_key"], max_tokens=1500, temperature=0.7, + timeout=120, extra_kwargs=extra, + ) + return content if content else None + + +# ── Memory persistence (cross-channel recall via memory.db) ────────────────── +def save_to_memory(channel, conv_id, user_text, assistant_text): + """Store the exchange in memory.db under session_id `-`.""" + try: + os.makedirs(os.path.dirname(MEMORY_DB), exist_ok=True) + conn = sqlite3.connect(MEMORY_DB) + c = conn.cursor() + c.execute(""" + CREATE TABLE IF NOT EXISTS conversations ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, timestamp TEXT, role TEXT, content TEXT + ) + """) + session_id = f"{channel}-{conv_id}" + ts = datetime.now().isoformat() + c.execute("INSERT INTO conversations (session_id, timestamp, role, content) VALUES (?,?,?,?)", + (session_id, ts, "user", user_text[:2000])) + c.execute("INSERT INTO conversations (session_id, timestamp, role, content) VALUES (?,?,?,?)", + (session_id, ts, "assistant", assistant_text[:2000])) + conn.commit() + conn.close() + except Exception as e: + log.debug(f"Memory save error: {e}") diff --git a/codec_imessage.py b/codec_imessage.py index ce00b73..0cd215a 100644 --- a/codec_imessage.py +++ b/codec_imessage.py @@ -260,80 +260,22 @@ def is_sender_allowed(sender, im_cfg): # ── CODEC Skill dispatch ──────────────────────────────────────────────────── -_dispatch_available = True -_check_skill = None -_run_skill = None - -def _load_dispatch(): - """Lazy-load codec_dispatch, handling pynput/GUI dependency issues.""" - global _dispatch_available, _check_skill, _run_skill - if _check_skill is not None: - return _dispatch_available - try: - from codec_dispatch import check_skill, run_skill - _check_skill = check_skill - _run_skill = run_skill - return True - except Exception as e: - log.warning(f"Skill dispatch unavailable ({e}) — LLM-only mode") - _dispatch_available = False - return False - - -def try_skill(text): - """Try matching a CODEC skill. Returns (skill_name, result) or (None, None).""" - if not _load_dispatch(): - return (None, None) - try: - skill = _check_skill(text) - if skill: - # Skip skills that need a terminal/GUI - _SKIP_SKILLS = {"open_terminal", "run_command", "vibe_code", "deep_chat", - "memory_search", "ask_mike_to_build"} - if skill["name"] in _SKIP_SKILLS: - return (None, None) - result = _run_skill(skill, text) - if result: - return (skill["name"], str(result)) - except Exception as e: - log.warning(f"Skill error: {e}") - return (None, None) +# A-19 (PR-3F): skill dispatch, the LLM call, and memory persistence are shared +# with codec_telegram via codec_bridges. process_message stays local (the two +# bridges' flows have intentionally drifted — goals/intent here, audio there). +from codec_bridges import try_skill # ── LLM call ──────────────────────────────────────────────────────────────── def call_llm(text, sender, llm_cfg, conversation_history=None, system_prompt_override=None): - """Send text to CODEC's LLM and return response.""" - if system_prompt_override: - sys_prompt = system_prompt_override - else: - now_str = datetime.now().strftime("%A %B %d, %Y at %H:%M") - sys_prompt = ( - f"You are CODEC, a personal AI assistant replying via iMessage. " - f"Today is {now_str}. Be concise — this is a text message conversation. " - f"Keep replies under 3 sentences unless more detail is needed. " - f"Be natural and conversational, like texting a smart friend." - ) - - messages = [{"role": "system", "content": sys_prompt}] - - # Add conversation history for context - if conversation_history: - messages.extend(conversation_history[-8:]) # last 8 exchanges - - messages.append({"role": "user", "content": text}) - - # A-12 (PR-3E-bridges): canonical codec_llm.call. Never-raise -> "" on any - # failure (error/no-choices/timeout/empty); mapped back to the bridge's None - # contract for graceful degradation. codec_llm strips ; kwargs are - # passed minus chat_template_kwargs so enable_thinking=False is preserved. - import codec_llm - extra = {k: v for k, v in llm_cfg["kwargs"].items() if k != "chat_template_kwargs"} - content = codec_llm.call( - messages, base_url=llm_cfg["base_url"], model=llm_cfg["model"], - api_key=llm_cfg["api_key"], max_tokens=1500, temperature=0.7, - timeout=120, extra_kwargs=extra, + """Send text to CODEC's LLM and return response (A-19 PR-3F: shared core in + codec_bridges; `sender` is unused by the LLM call, kept for call-site compat).""" + import codec_bridges + return codec_bridges.call_llm( + "imessage", text, llm_cfg, + conversation_history=conversation_history, + system_prompt_override=system_prompt_override, ) - return content if content else None # ── Vision (image attachments) ────────────────────────────────────────────── @@ -872,35 +814,10 @@ def _extract_goals_from_reply(text, sender): # ── Save to CODEC memory DB ───────────────────────────────────────────────── def save_to_memory(sender, user_text, assistant_text): - """Store the exchange in CODEC's memory.db for cross-channel recall.""" - try: - os.makedirs(os.path.dirname(MEMORY_DB), exist_ok=True) - conn = sqlite3.connect(MEMORY_DB) - c = conn.cursor() - # Ensure conversations table exists - c.execute(""" - CREATE TABLE IF NOT EXISTS conversations ( - id INTEGER PRIMARY KEY AUTOINCREMENT, - session_id TEXT, - timestamp TEXT, - role TEXT, - content TEXT - ) - """) - session_id = f"imessage-{sender}" - ts = datetime.now().isoformat() - c.execute( - "INSERT INTO conversations (session_id, timestamp, role, content) VALUES (?,?,?,?)", - (session_id, ts, "user", user_text[:2000]), - ) - c.execute( - "INSERT INTO conversations (session_id, timestamp, role, content) VALUES (?,?,?,?)", - (session_id, ts, "assistant", assistant_text[:2000]), - ) - conn.commit() - conn.close() - except Exception as e: - log.debug(f"Memory save error: {e}") + """Store the exchange in memory.db (A-19 PR-3F: shared in codec_bridges; + session_id = "imessage-").""" + import codec_bridges + return codec_bridges.save_to_memory("imessage", sender, user_text, assistant_text) # ── Main processing loop ──────────────────────────────────────────────────── diff --git a/codec_telegram.py b/codec_telegram.py index 1c733f5..79300a8 100644 --- a/codec_telegram.py +++ b/codec_telegram.py @@ -23,7 +23,6 @@ import time import logging import requests -import sqlite3 from datetime import datetime # ── Logging ────────────────────────────────────────────────────────────────── @@ -207,43 +206,10 @@ def download_file(self, file_url, dest_path): # ── CODEC Skill dispatch ──────────────────────────────────────────────────── -_dispatch_loaded = False -_check_skill = None -_run_skill = None - - -def _load_dispatch(): - global _dispatch_loaded, _check_skill, _run_skill - if _dispatch_loaded: - return _check_skill is not None - _dispatch_loaded = True - try: - from codec_dispatch import check_skill, run_skill - _check_skill = check_skill - _run_skill = run_skill - log.info("Skill dispatch loaded") - return True - except Exception as e: - log.warning(f"Skill dispatch unavailable ({e}) — LLM-only mode") - return False - - -def try_skill(text): - if not _load_dispatch(): - return (None, None) - try: - skill = _check_skill(text) - if skill: - _SKIP = {"open_terminal", "run_command", "vibe_code", "deep_chat", - "memory_search", "ask_mike_to_build"} - if skill["name"] in _SKIP: - return (None, None) - result = _run_skill(skill, text) - if result: - return (skill["name"], str(result)) - except Exception as e: - log.warning(f"Skill error: {e}") - return (None, None) +# A-19 (PR-3F): skill dispatch, the LLM call, and memory persistence are shared +# with codec_imessage via codec_bridges. process_message stays local (the two +# bridges' flows have intentionally drifted — audio/Gemini here, goals there). +from codec_bridges import try_skill # ── Daily Briefing: premium data gathering ─────────────────────────────── @@ -447,34 +413,14 @@ def _run_deep_report(chat_id): # ── LLM call ──────────────────────────────────────────────────────────────── def call_llm(text, llm_cfg, conversation_history=None, system_prompt_override=None): - if system_prompt_override: - sys_prompt = system_prompt_override - else: - now_str = datetime.now().strftime("%A %B %d, %Y at %H:%M") - sys_prompt = ( - f"You are CODEC, a personal AI assistant replying via Telegram. " - f"Today is {now_str}. Be concise and direct. " - f"Keep replies under 3 sentences unless more detail is needed. " - f"You can use Markdown formatting. Be natural and helpful." - ) - - messages = [{"role": "system", "content": sys_prompt}] - if conversation_history: - messages.extend(conversation_history[-8:]) - messages.append({"role": "user", "content": text}) - - # A-12 (PR-3E-bridges): canonical codec_llm.call. Never-raise -> "" on any - # failure (error/no-choices/timeout/empty); mapped back to the bridge's None - # contract for graceful degradation. codec_llm strips ; kwargs are - # passed minus chat_template_kwargs so enable_thinking=False is preserved. - import codec_llm - extra = {k: v for k, v in llm_cfg["kwargs"].items() if k != "chat_template_kwargs"} - content = codec_llm.call( - messages, base_url=llm_cfg["base_url"], model=llm_cfg["model"], - api_key=llm_cfg["api_key"], max_tokens=1500, temperature=0.7, - timeout=120, extra_kwargs=extra, + # A-19 (PR-3F): shared bridge LLM call lives in codec_bridges (channel persona + # + codec_llm.call + None-contract). Thin wrapper keeps this call signature. + import codec_bridges + return codec_bridges.call_llm( + "telegram", text, llm_cfg, + conversation_history=conversation_history, + system_prompt_override=system_prompt_override, ) - return content if content else None # ── Vision (photo messages) ───────────────────────────────────────────────── @@ -541,26 +487,9 @@ def add_history(chat_id, role, content): # ── Save to CODEC memory DB ───────────────────────────────────────────────── def save_to_memory(chat_id, user_text, assistant_text): - try: - os.makedirs(os.path.dirname(MEMORY_DB), exist_ok=True) - conn = sqlite3.connect(MEMORY_DB) - c = conn.cursor() - c.execute(""" - CREATE TABLE IF NOT EXISTS conversations ( - id INTEGER PRIMARY KEY AUTOINCREMENT, - session_id TEXT, timestamp TEXT, role TEXT, content TEXT - ) - """) - session_id = f"telegram-{chat_id}" - ts = datetime.now().isoformat() - c.execute("INSERT INTO conversations (session_id, timestamp, role, content) VALUES (?,?,?,?)", - (session_id, ts, "user", user_text[:2000])) - c.execute("INSERT INTO conversations (session_id, timestamp, role, content) VALUES (?,?,?,?)", - (session_id, ts, "assistant", assistant_text[:2000])) - conn.commit() - conn.close() - except Exception as e: - log.debug(f"Memory save error: {e}") + # A-19 (PR-3F): shared in codec_bridges (session_id = "telegram-"). + import codec_bridges + return codec_bridges.save_to_memory("telegram", chat_id, user_text, assistant_text) # ── Process message ───────────────────────────────────────────────────────── diff --git a/docs/PR3F-BRIDGE-UNIFICATION-DESIGN.md b/docs/PR3F-BRIDGE-UNIFICATION-DESIGN.md new file mode 100644 index 0000000..81519b8 --- /dev/null +++ b/docs/PR3F-BRIDGE-UNIFICATION-DESIGN.md @@ -0,0 +1,43 @@ +# PR-3F — bridge unification (A-19) (DESIGN) + +**Status:** IMPLEMENTED — **Option 1** (scoped). New `codec_bridges.py` holds the 4 shared helpers (`load_dispatch`, `try_skill`, `call_llm(channel, …)`, `save_to_memory(channel, conv_id, …)`); `codec_telegram` + `codec_imessage` now `from codec_bridges import try_skill` + keep thin channel-injecting wrappers for `call_llm`/`save_to_memory` (call sites unchanged). `process_message` left per-bridge (drifted). Removed telegram's now-dead `sqlite3` import; updated the 2 stale #71 source-invariants. 10 new tests (`tests/test_bridges.py`); full suite 1502 passing, zero new; zero net-new ruff. **Audit-A complete.** +**Finding:** A-19 (MEDIUM, "large") — `codec_telegram` + `codec_imessage` duplicate `try_skill`, `_load_dispatch`, `call_llm`, `save_to_memory`, and parts of `process_message`. +**Wave:** 3 (last Audit-A item). Outbound bridges = security-sensitive, working code → small + behavior-preserving, **don't force the drifted paths together**. + +--- + +## 1. Reality check (what changed since the audit was written) + +- **The churny dup is already gone.** The audit's headline pain — "every fix to the outbound-bridge LLM call has to be applied twice" — was resolved in **PR-3E-bridges (#71)**: both `call_llm`s now call `codec_llm.call` (the only remaining diff is the one-line persona: "via Telegram" vs "via iMessage"). +- **The two `process_message` flows have intentionally DRIFTED** (per the audit's own impact note): telegram has audio transcription + Gemini fallback + daily-briefing; imessage has goal-tracking + intent classification. Forcing them into one `BridgeRouter` pipeline (the audit's "large" recommendation) means reconciling those divergent features — **high risk of breaking a working bridge**. +- The genuinely-identical, **low-churn** leftovers: `try_skill` (character-identical incl. the `_SKIP` set), `_load_dispatch` (near-identical lazy shim), `save_to_memory` (both `CodecMemory.save(channel, …)`), and the `call_llm` wrapper (now ~persona + `codec_llm.call`). + +## 2. The risk in the audit's full vision + +`BridgeRouter` unifying `process_message` is the **large + risky** part: it would have to absorb telegram-only (audio/Gemini/briefing) and imessage-only (goals/intent) behavior into one flow without regressing either. That contradicts "never break working code" for two live outbound channels. The dedup payoff there is **low** (the flows are genuinely different), the risk **high**. + +## 3. Recommended scope — extract the 4 shared helpers, leave `process_message` + +New **`codec_bridges.py`**: +- `load_dispatch()` — the lazy `codec_dispatch` import shim (one copy). +- `try_skill(text) -> (name|None, result|None)` — identical skill match + the `_SKIP` set. +- `call_llm(channel, text, llm_cfg, conversation_history=None, system_prompt_override=None) -> str|None` — the canonical bridge LLM call (`codec_llm.call` + the persona chosen by `channel`, `None`-contract preserved, `chat_template_kwargs` filtered). iMessage's extra `sender` arg is dropped (it was unused in the LLM call). +- `save_to_memory(channel, user_text, reply)` — the `CodecMemory.save` pair. + +`codec_telegram` + `codec_imessage` import these; **their `process_message` (and telegram's audio / imessage's goals) stay put** — `process_message` calls the shared `try_skill`/`call_llm`/`save_to_memory` instead of local copies. Net: real dedup of the shared surface + `codec_bridges.py` becomes the documented "add a channel" seed (CLAUDE.md §1 WhatsApp/Discord), WITHOUT touching the drifted flows. + +## 4. Test plan (recommended scope) +- `tests/test_bridges.py`: `try_skill` honors the `_SKIP` set + returns `(name, result)`; `call_llm("telegram"/"imessage", …)` builds the right persona + maps `codec_llm.call` → `None` on empty (monkeypatched); `save_to_memory` calls `CodecMemory.save` with the channel. Source invariants: both bridges import from `codec_bridges`; the duplicated `try_skill`/`_load_dispatch` defs are gone. +- Regression: bridge import smoke + full suite — 23 known-baseline failures, **zero new**. No `skills/` touched → no manifest regen. + +## 5. Risk + rollback +- **Blast radius (recommended scope):** new `codec_bridges.py` + the 2 bridges' helper defs replaced by imports. `process_message` flows untouched → telegram audio + imessage goals can't regress. Inbound stays PWA-only (unchanged). +- **Rollback:** single-commit revert. + +## 6. Open question for you (Mickael) — scope + +- **Option 1 (recommended):** extract the **4 shared helpers** into `codec_bridges.py`; both bridges import them; `process_message` stays per-bridge. Real dedup + the extension seed, low risk. (~the safe 70% of A-19.) +- **Option 2 (full BridgeRouter):** also unify `process_message` into one pipeline. Matches the audit's vision + the "add a channel" unlock, but must reconcile the **drifted** telegram/imessage flows → highest risk on two working outbound channels. +- **Option 3 (skip / document-closed):** A-19's churny pain (`call_llm`) is already fixed by #71; the rest is low-churn. Mark A-19 "largely addressed; full unification deferred — flows intentionally drifted," and move on (no code change). + +I lean **Option 1** — it banks the safe dedup + seeds `codec_bridges.py` without risking the drifted flows. Pick one and I'll implement (or, for Option 3, just update the audit docs). diff --git a/docs/audits/PHASE-1-CODE-QUALITY.md b/docs/audits/PHASE-1-CODE-QUALITY.md index e23a844..c8279b7 100644 --- a/docs/audits/PHASE-1-CODE-QUALITY.md +++ b/docs/audits/PHASE-1-CODE-QUALITY.md @@ -203,6 +203,8 @@ Both scan `SKILLS_DIR` independently, so a skill file is loaded twice in differe **Effort:** medium (wiring all 9 to their routes is a couple hours; documenting body shapes for POST endpoints is the bigger win) ### A-19 — codec_telegram + codec_imessage duplicate try_skill, _load_dispatch, call_llm, save_to_memory [MEDIUM] + +> **Closed by PR-3F (Option 1, scoped).** New **`codec_bridges.py`** holds the 4 genuinely-shared helpers — `load_dispatch`, `try_skill` (+ the `_SKIP_SKILLS` set), `call_llm(channel, …)` (channel-selected persona → `codec_llm.call`, `None`-contract preserved), `save_to_memory(channel, conv_id, …)` (session_id `-`). Both bridges `from codec_bridges import try_skill` and keep thin channel-injecting wrappers for `call_llm`/`save_to_memory` (call sites unchanged). **`process_message` deliberately NOT unified** — the two flows have intentionally drifted (telegram audio/Gemini/briefing; imessage goals/intent) and the audit's headline pain (the `call_llm` dup) was already fixed in PR-3E-bridges (#71); forcing the drifted flows into one `BridgeRouter` was high-risk/low-value. `codec_bridges.py` is now the documented "add a channel" seed (CLAUDE.md §1). Pinned by `tests/test_bridges.py` (10). See `docs/PR3F-BRIDGE-UNIFICATION-DESIGN.md`. **Location:** - `try_skill`: `codec_telegram.py:220` vs `codec_imessage.py:282` — character-identical logic and skip list (`open_terminal`, `run_command`, `vibe_code`, `deep_chat`, `memory_search`, `ask_mike_to_build`). - `_load_dispatch`: `codec_telegram.py:204-217` vs `codec_imessage.py:266-279` — near-identical lazy-load with try/except shim. diff --git a/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md b/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md index 797d6e6..ce71c9f 100644 --- a/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md +++ b/docs/audits/PHASE-1-CONSOLIDATED-TRIAGE.md @@ -253,7 +253,7 @@ Mirror the Intake Phase 3 wave pattern. 7 waves planned; sizes are PR-counts, NO - PR-3E-chat-stream: A-12 dashboard chat pair ✅ (branch `fix/pr3-a12-chat-stream`, design-first → `docs/PR3E-CHAT-STREAM-DESIGN.md`). Added `codec_llm.KEEPALIVE` + `codec_llm.stream(keepalive=)` (sentinel on empty thinking-chunks → dashboard emits `: keepalive`; default off → `qwen_stream` unaffected). Migrated `chat_completion`'s stream path (`codec_llm.stream(keepalive=True)` → existing `SkillTagBuffer`) + non-stream fallback (`codec_llm.call(raise_on_error=True)` preserves 500-on-failure), off shared `_common` args (no drift). Removed dead `import requests as rq` + `headers`. Closed-without-`[DONE]` now also emits a terminating `[DONE]` + blank-bubble fallback (documented improvement). 3 keepalive tests + chat-handler invariants; zero net-new ruff; full suite 1473 passing, zero new. **Dashboard A-12 complete** (only the 5 vision sites remain inline = A-11). **Remaining A-12:** voice `_stream_qwen` + agents `Agent.run` (async `astream()` + queue), self_improve/watcher, skills tranche. - PR-3E-async: A-12 voice + agents async ✅ (branch `fix/pr3-a12-async`, design-first → `docs/PR3E-ASYNC-DESIGN.md`; **Option 2**). Added async `codec_llm.acall()` (mirrors `call()` + `raise_on_error`, injected httpx client) + `codec_llm.astream()` (mirrors `stream()` + keepalive, but **propagates** exceptions so voice can speak failures). Migrated all 3 queue-coupled async sites: `codec_voice._stream_qwen` (`astream`, queue CRITICAL, per-token `` strip + spoken error kept), `codec_agents.Agent.run` (`acall(raise_on_error=True)`, queue MEDIUM), agents research-refiner (`acall`, never-raise → defaults). Queue stays at every call site (`codec_llm` never owns the semaphore). Removed dead `_qwen_url()`. 12 tests (`tests/test_llm_async.py`); crew + voice regression green; zero net-new ruff; full suite 1485 passing, zero new. **A-12 streaming complete.** **Remaining A-12:** self_improve/watcher + skills tranche. - PR-3E-skills-misc: A-12 final tranche ✅ (branch `fix/pr3-a12-skills-misc`, design-first → `docs/PR3E-SKILLS-MISC-DESIGN.md`). Migrated the last 6 non-hot graceful sites: `codec_self_improve._draft_skill` (`call(retries=2)` → None), `codec_watcher.handle_draft` (kept 3× notify-retry loop, swapped inner POST + `extract_content` → `call`), `skills/translate` + `create_skill` + `skill_forge` (graceful → `call` + fallback), `skills/fact_extract._call_llm` (`call(retries=3, raise_on_error=True)` → keeps `__ERR__` sentinel). Removed 3 dead imports; regenerated `skills/.manifest.json` (76 skills, `--check` clean). 7 tests (`tests/test_llm_skills_misc.py`); net-negative ruff; full suite 1492 passing, zero new. **A-12 FULLY CLOSED** — every chat/completions text site is on `codec_llm`; only vision POSTs (A-11) remain inline. **Wave-3 Audit-A remaining:** only PR-3F (optional, large — bridge unification A-19). -- PR-3F (optional, large): A-19 — bridge unification (iMessage + Telegram → `BridgeRouter`) +- PR-3F: A-19 — bridge unification ✅ (branch `fix/pr3f-bridge-unification`, design-first → `docs/PR3F-BRIDGE-UNIFICATION-DESIGN.md`; **Option 1 scoped**). New `codec_bridges.py` with the 4 shared helpers (`load_dispatch`, `try_skill`, `call_llm(channel,…)`, `save_to_memory(channel, conv_id,…)`); both bridges import `try_skill` + keep thin channel-injecting wrappers for `call_llm`/`save_to_memory` (call sites unchanged). **`process_message` left per-bridge** — flows intentionally drifted (telegram audio/Gemini, imessage goals/intent); the churny `call_llm` dup was already fixed in #71, so the full `BridgeRouter` (unifying process_message) was high-risk/low-value. `codec_bridges.py` seeds the "add a channel" surface. Removed telegram's dead `sqlite3` import; fixed 2 stale #71 invariants. 10 tests (`tests/test_bridges.py`); zero net-new ruff; full suite 1502 passing, zero new. **Audit-A (Wave 3) complete.** - PR-3G: small misc ✅ (branch `fix/pr3g-small-misc-cleanup`) — closed A-9 (DISABLED overlay, ~90 LOC), A-10 (run_session_module, 33 LOC + orphan `import sys`), A-14 (close_session shadow import), A-18 (9 unused Pydantic models + dead typing import). A-13 (dashboard pattern blocker) verified **already closed by PR-2C**. 6 regression tests; zero net-new ruff (net −); full suite 1344 passing. **Deferred from this batch (each needs its own focused PR):** A-8 (codec_keyboard.py 398 LOC — verify-first delete-or-migrate decision), A-15 (config_version — additive migration feature touching `load_config`), A-20 (inline sqlite in the live dispatch path — reliability fix needing a CodecMemory method). - A-15: config schema versioning ✅ (branch `fix/pr3-a15-config-versioning`; `CONFIG_SCHEMA_VERSION=1` + migration ladder + idempotent atomic write-back in `load_config`; never creates-on-missing or overwrites-corrupt; 12 tests; zero net-new ruff; full suite 1356 passing). - A-20: inline-sqlite reliability fix ✅ (branch `fix/pr3-a20-inline-sqlite`; added `codec_core._db_connect()` with WAL+busy_timeout + `update_session_response()`; replaced codec.py's inline lock-prone UPDATE; retrofitted all 4 codec_core session connects; removed now-unused sqlite3/DB_PATH imports; 9 tests; net-negative ruff; full suite 1365 passing). diff --git a/tests/test_bridges.py b/tests/test_bridges.py new file mode 100644 index 0000000..76a6358 --- /dev/null +++ b/tests/test_bridges.py @@ -0,0 +1,118 @@ +"""Tests for PR-3F — A-19: codec_bridges shared outbound-bridge core. + +Extracts the genuinely-identical helpers shared by codec_telegram + codec_imessage: +load_dispatch / try_skill (skill dispatch), call_llm (canonical bridge LLM call +via codec_llm, channel-selected persona), save_to_memory (memory.db write). Each +bridge keeps its own (drifted) process_message. Reference: docs/PR3F-BRIDGE-UNIFICATION-DESIGN.md. +""" +from __future__ import annotations + +import sqlite3 +import sys +from pathlib import Path + +REPO = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO)) + +import codec_bridges # noqa: E402 +import codec_llm # noqa: E402 + + +def _cfg(api_key="", kwargs=None): + return {"base_url": "http://x/v1", "model": "m", + "api_key": api_key, "kwargs": kwargs or {}} + + +# ── call_llm (channel persona + None contract + kwargs filter) ───────────────── + + +def test_call_llm_telegram_persona(monkeypatch): + cap = {} + monkeypatch.setattr(codec_llm, "call", + lambda messages, **k: cap.update(messages=messages, **k) or "reply") + out = codec_bridges.call_llm("telegram", "hi", _cfg()) + assert out == "reply" + assert "via Telegram" in cap["messages"][0]["content"] + assert cap["messages"][-1] == {"role": "user", "content": "hi"} + + +def test_call_llm_imessage_persona(monkeypatch): + cap = {} + monkeypatch.setattr(codec_llm, "call", + lambda messages, **k: cap.update(messages=messages) or "r") + codec_bridges.call_llm("imessage", "hi", _cfg()) + assert "via iMessage" in cap["messages"][0]["content"] + + +def test_call_llm_none_on_empty(monkeypatch): + monkeypatch.setattr(codec_llm, "call", lambda *a, **k: "") + assert codec_bridges.call_llm("telegram", "hi", _cfg()) is None + + +def test_call_llm_filters_chat_template_kwargs(monkeypatch): + cap = {} + monkeypatch.setattr(codec_llm, "call", lambda messages, **k: cap.update(k) or "r") + codec_bridges.call_llm("telegram", "hi", _cfg(api_key="K", kwargs={ + "top_p": 0.9, "chat_template_kwargs": {"enable_thinking": True}})) + assert cap["extra_kwargs"] == {"top_p": 0.9} + assert cap["api_key"] == "K" + + +def test_call_llm_history_and_override(monkeypatch): + cap = {} + monkeypatch.setattr(codec_llm, "call", lambda messages, **k: cap.update(messages=messages) or "r") + hist = [{"role": "user", "content": "earlier"}, {"role": "assistant", "content": "ok"}] + codec_bridges.call_llm("telegram", "now", _cfg(), conversation_history=hist, + system_prompt_override="SYS") + assert cap["messages"][0] == {"role": "system", "content": "SYS"} + assert cap["messages"][1:] == hist + [{"role": "user", "content": "now"}] + + +# ── try_skill (skip set + result) ───────────────────────────────────────────── + + +def test_try_skill_skips_blocked(monkeypatch): + monkeypatch.setattr(codec_bridges, "_dispatch_loaded", True) + monkeypatch.setattr(codec_bridges, "_check_skill", lambda t: {"name": "open_terminal"}) + monkeypatch.setattr(codec_bridges, "_run_skill", lambda s, t: "should not run") + assert codec_bridges.try_skill("open a terminal") == (None, None) + + +def test_try_skill_runs_allowed(monkeypatch): + monkeypatch.setattr(codec_bridges, "_dispatch_loaded", True) + monkeypatch.setattr(codec_bridges, "_check_skill", lambda t: {"name": "weather"}) + monkeypatch.setattr(codec_bridges, "_run_skill", lambda s, t: "sunny") + assert codec_bridges.try_skill("weather?") == ("weather", "sunny") + + +def test_try_skill_no_match(monkeypatch): + monkeypatch.setattr(codec_bridges, "_dispatch_loaded", True) + monkeypatch.setattr(codec_bridges, "_check_skill", lambda t: None) + monkeypatch.setattr(codec_bridges, "_run_skill", lambda s, t: "x") + assert codec_bridges.try_skill("chitchat") == (None, None) + + +# ── save_to_memory (channel-prefixed session_id) ────────────────────────────── + + +def test_save_to_memory(monkeypatch, tmp_path): + db = tmp_path / "memory.db" + monkeypatch.setattr(codec_bridges, "MEMORY_DB", str(db)) + codec_bridges.save_to_memory("telegram", "12345", "hello", "hi there") + rows = sqlite3.connect(str(db)).execute( + "SELECT session_id, role, content FROM conversations ORDER BY id").fetchall() + assert rows == [ + ("telegram-12345", "user", "hello"), + ("telegram-12345", "assistant", "hi there"), + ] + + +# ── source-level migration invariants ───────────────────────────────────────── + + +def test_bridges_use_codec_bridges(): + for mod in ("codec_telegram.py", "codec_imessage.py"): + src = (REPO / mod).read_text() + assert "codec_bridges" in src, f"{mod} doesn't use codec_bridges" + # the duplicated skill-dispatch internals are gone + assert "from codec_dispatch import check_skill, run_skill" not in src, f"{mod} still has its own dispatch loader" diff --git a/tests/test_llm_bridges.py b/tests/test_llm_bridges.py index 603fc65..8eb7048 100644 --- a/tests/test_llm_bridges.py +++ b/tests/test_llm_bridges.py @@ -89,12 +89,15 @@ def fake(messages, **k): def test_telegram_uses_codec_llm(): + # PR-3F (A-19): the LLM call moved into codec_bridges (which calls codec_llm). + # The bridge now delegates via codec_bridges.call_llm; only the vision POST + # (llm_cfg["vision_url"]) remains inline. src = (REPO / "codec_telegram.py").read_text() - assert "codec_llm.call(" in src + assert "codec_bridges" in src assert src.count("/chat/completions") == 1 # only the vision site remains def test_imessage_uses_codec_llm(): src = (REPO / "codec_imessage.py").read_text() - assert "codec_llm.call(" in src + assert "codec_bridges" in src assert src.count("/chat/completions") == 1