|
| 1 | +# PR-3E — LLM-call + vision dedup (DESIGN) |
| 2 | + |
| 3 | +**Status:** DRAFT — awaiting approval. No code changed yet (per AGENTS.md §11). |
| 4 | +**Findings:** A-11 (vision dup, MEDIUM) + A-12 (51→45 `chat/completions` sites, MEDIUM, audit-flagged **large**). |
| 5 | +**Wave:** 3. This is the **hottest code path in the repo** (every feature calls an LLM), so it gets design-first + a phased plan. |
| 6 | + |
| 7 | +--- |
| 8 | + |
| 9 | +## 1. Reality check (what the trace found) |
| 10 | + |
| 11 | +- **`codec_llm_proxy.py` is NOT a proxy.** It's a priority *queue* (semaphore) — its own docstring: *"Does NOT proxy HTTP — callers still make their own requests."* So A-12's "the module already exists, just add `call()`/`stream()`" is **inaccurate**: there is no call/stream helper to reuse. A-12 means **building a new canonical call API** (which uses the queue internally). |
| 12 | +- **45 `chat/completions` sites** (was 51; some removed in earlier PRs) across **three shapes**: sync `requests`, async `httpx`, and streaming SSE — with copy-pasted headers, `Authorization: Bearer`, `enable_thinking=False`, `<think>` stripping, and `choices[0].message.content`/`.reasoning` parsing. |
| 13 | +- **A-11 vision = 3 divergent impls:** |
| 14 | + - `codec.py` `vision_describe`/`_gemini_vision`/`_local_vision` — **sync** (`requests`), Gemini-flash → local-Qwen-VL fallback, PNG. |
| 15 | + - `codec_voice._analyze_screenshot` — **async** (`httpx`), Gemini → local fallback, JPEG. |
| 16 | + - `codec_session.screenshot_ctx` — **sync**, local-Qwen-VL **only** (no Gemini), PNG, with inline screencapture. |
| 17 | + |
| 18 | +## 2. Why this is high-risk |
| 19 | + |
| 20 | +These are the call paths behind voice, chat, vision, agents, bridges. A subtle |
| 21 | +regression in payload shape, `<think>` stripping, streaming chunk parsing, |
| 22 | +timeout, or error handling silently degrades a core feature. Blast radius = |
| 23 | +everything. So: **small, behavior-parity tranches with mocked-HTTP tests that |
| 24 | +assert payload/response equivalence — never a 45-site big-bang.** |
| 25 | + |
| 26 | +## 3. Recommended plan — split A-11 from A-12, phase A-12 |
| 27 | + |
| 28 | +The audit lumps A-11 + A-12 as "PR-3E," but they're independent and A-12 is |
| 29 | +"large." Recommended: |
| 30 | + |
| 31 | +### This PR (PR-3E) — **A-11 vision dedup only** (contained, ~3 consumers) |
| 32 | +- New **`codec_vision.py`**: the single canonical vision helper. |
| 33 | + - `describe_sync(image_b64, prompt, *, mime="image/png", max_tokens=800) -> str` |
| 34 | + - `async describe_async(image_b64, prompt, *, mime="image/jpeg", max_tokens=500, http=None) -> str` |
| 35 | + - Both: Gemini-flash (if `VISION_PROVIDER=="gemini"` and key present) → local-Qwen-VL fallback, reading config (`vision_base_url`, `vision_model`, `get_gemini_api_key`). One place to change the model / provider / API shape. |
| 36 | +- Migrate the 3 consumers to delegate: |
| 37 | + - `codec.py`: `vision_describe` → `codec_vision.describe_sync`; drop `_gemini_vision`/`_local_vision`. |
| 38 | + - `codec_voice._analyze_screenshot` → `await codec_vision.describe_async(..., http=self._http)`. |
| 39 | + - `codec_session.screenshot_ctx` → `codec_vision.describe_sync` (gains Gemini fallback it lacked — a minor *improvement*, behaviorally a superset; flagged in the PR). |
| 40 | +- **Tests:** mock HTTP; assert Gemini-first + local-fallback, payload shapes, mime handling, empty-on-failure. ~8 tests. |
| 41 | +- **Risk:** medium-low (vision is less hot than chat; 3 well-understood sites). Behavior parity except session gaining the Gemini fallback (documented). |
| 42 | + |
| 43 | +### Follow-on (PR-3E-2+, separate design) — **A-12 chat/completions** |
| 44 | +- Build **`codec_llm.py`**: `call(messages, *, model, temperature, max_tokens, priority, **kw) -> str` (sync) + `stream(...)` (SSE generator) + an async variant. Centralizes headers, `enable_thinking`, `<think>` strip, `choices/reasoning` parse, queue-slot acquisition, timeouts, error shape. |
| 45 | +- Migrate the 45 sites **in small tranches by subsystem**, each its own PR with parity tests: e.g. (1) codec.py + codec_session, (2) dashboard, (3) voice, (4) agents/agent_plan/agent_runner, (5) bridges (telegram/imessage), (6) misc (compaction/self_improve/watcher/textassist/dictate). Each tranche is independently revertable. |
| 46 | +- This is deliberately **not** in this PR — 45 hot-path sites in one diff is unreviewable + high-risk. |
| 47 | + |
| 48 | +## 4. API / schema changes |
| 49 | +- New module `codec_vision.py` (this PR). No on-disk schema, no config changes |
| 50 | + (reuses existing `vision_*` config keys + `get_gemini_api_key`). |
| 51 | +- `codec.py` loses `_gemini_vision`/`_local_vision` (internal); `vision_describe` |
| 52 | + kept as a thin delegate for any external caller. |
| 53 | +- (A-12's `codec_llm.py` is a later PR.) |
| 54 | + |
| 55 | +## 5. Test plan (this PR — A-11) |
| 56 | +- New `tests/test_vision_dedup.py`: |
| 57 | + - `describe_sync`: Gemini path returns text; Gemini failure → local fallback; |
| 58 | + both fail → `""`; correct payload shape per provider; mime respected. |
| 59 | + - `describe_async`: same matrix with a mocked httpx client. |
| 60 | + - Source invariants: codec.py no longer defines `_gemini_vision`/`_local_vision`; |
| 61 | + voice + session call `codec_vision`. |
| 62 | +- Regression: full suite (expect the 23 known failures, zero new). No `skills/` |
| 63 | + touched → no manifest regen. |
| 64 | +- Manual (Mac Studio): voice "look at my screen" + a chat screenshot still |
| 65 | + describe correctly via both providers. |
| 66 | + |
| 67 | +## 6. Risk + rollback |
| 68 | +- **Blast radius (this PR):** 3 files edited + 1 new module. Vision only — chat |
| 69 | + paths untouched. |
| 70 | +- **Rollback:** single-commit revert restores the inline impls. No persistent |
| 71 | + state touched. |
| 72 | +- A-12 risk is deferred to its own phased PRs (each small + revertable). |
| 73 | + |
| 74 | +## 7. Open question for you (Mickael) |
| 75 | +**Q: scope of PR-3E?** |
| 76 | +- **Option 1 (recommended):** PR-3E = **A-11 vision dedup only**, now. A-12 |
| 77 | + (chat/completions) becomes its own phased effort with a separate design doc |
| 78 | + (build `codec_llm.call/stream` + migrate sites tranche-by-tranche). Keeps every |
| 79 | + PR reviewable + low-risk on the hottest path. |
| 80 | +- **Option 2:** PR-3E = A-11 **+** A-12's canonical `codec_llm` API **+** the |
| 81 | + first chat tranche (codec.py + codec_session). Bigger, riskier single PR. |
| 82 | +- **Option 3:** Do A-12 API first (no A-11 yet). |
| 83 | + |
| 84 | +I recommend **Option 1**. Pick one and I'll implement + open the PR |
| 85 | +(chat-review-then-merge — hot path). |
0 commit comments