chore: удалён мёртвый elmer/, версия 0.35.0-dev, документы Опуса
This commit is contained in:
Repinoid
@@ -0,0 +1,236 @@
|
||||
# Ревью проекта elmAI (ответы на opus-questions.md)
|
||||
|
||||
> Ревьювер: Opus 4.8 · 31 мая 2026 · v0.35.0-dev
|
||||
> Разбор по коду: `obd/protocol.py`, `brain/client.py`, `brain/prompts.py`, `api/routes.py`, `api/db.py`, `api/parser.py`, `web/app.py`.
|
||||
> Android-модуль (`elmer-android/`) в workspace отсутствует — по нему выводы на основе описаний в вопросах.
|
||||
|
||||
---
|
||||
|
||||
## Вопрос 1. Стейт-машина ELM327: баги и крайние случаи
|
||||
|
||||
### 🔴 Стартовый буфер не сбрасывается перед командой → десинхронизация
|
||||
В `_exec()` сразу идёт `_write(cmd)` без очистки входного буфера. Если предыдущая команда отвалилась по таймауту, в ОС-буфере остаются «хвосты» (часть ответа, поздний `>`). Следующий `_read()` прочитает этот мусор как ответ на новую команду и классифицирует его неверно — классическая рассинхронизация ELM327.
|
||||
|
||||
```python
|
||||
def _write(self, cmd: str):
|
||||
self._ser.reset_input_buffer() # сбросить хвосты предыдущего ответа
|
||||
self._ser.write((cmd + "\r").encode())
|
||||
self._ser.flush()
|
||||
logger.debug(f"AndrOBD → {cmd}")
|
||||
```
|
||||
|
||||
### 🔴 Состояние `DISCONNECTED`/`ERROR` затирается в `send()`
|
||||
`send()` проверяет только `State.ERROR` перед `_recover()`. Но BUS ERROR в `_handle()` ставит `DISCONNECTED`, а в конце `send()` безусловно пишет `self._state = State.READY`. То есть после фатальной ошибки шины машина всё равно объявляется READY, и накопленный сбой маскируется.
|
||||
|
||||
```python
|
||||
def send(self, cmd: str) -> str:
|
||||
if self._state in (State.ERROR, State.DISCONNECTED):
|
||||
self._recover()
|
||||
self._state = State.BUSY
|
||||
result = self._exec(cmd, self._timing.ms)
|
||||
# НЕ ставить READY безусловно — _exec мог уйти в ERROR
|
||||
if self._state == State.BUSY:
|
||||
self._state = State.READY
|
||||
return result
|
||||
```
|
||||
|
||||
### 🟡 `>` посреди мусора (вопрос 1.2)
|
||||
`_read()` возвращает всё накопленное до первого `>`. Если ELM прислал `SEARCHING...` затем данные затем `>`, всё склеится в одну строку через `\n`, а `Rsp.identify()` смотрит только на начало (`startswith`) — реальные данные после `SEARCHING` будут потеряны/неверно классифицированы. AndrOBD обрабатывает каждую строку отдельно. Рекомендация: классифицировать построчно, а не всю склейку.
|
||||
|
||||
### 🟢 Бесконечный цикл в `_exec()` (вопрос 1.5)
|
||||
Цикл жёстко ограничен `range(10)`, по выходу — `State.ERROR` и `return ""`. Бесконечного цикла нет. Но обратите внимание: при инициализации шаг `t += 1000` за 10 итераций даёт суммарно до ~55с ожидания на одну команду — для `INIT_TMO=10000` это может неприятно затянуть `init()`.
|
||||
|
||||
### 🟡 Восстановление после BUS ERROR (вопрос 1.3)
|
||||
Логика `ATPC → ATSP0` корректна по сути, но ответы на них читаются `_try_read()` и **молча выбрасываются**. Если `ATSP0` не подтвердился (ELM завис), машина об этом не узнает и пойдёт слать команды в неинициализированный протокол. Желательно проверять, что на `ATSP0` пришёл `OK`/`>`, иначе — полный reset (`ATZ`).
|
||||
|
||||
### 🟡 Поллинг 1мс (вопрос 1.4)
|
||||
1мс `time.sleep` в Python реально даёт ~1–15мс из-за гранулярности планировщика — на практике это не вредит (ELM медленнее), но и «честных» 1мс там нет. На быстрых ELM327 v1.5/v2.1 это не узкое место; узкое место — таймаут адаптива, а не поллинг. Менять не нужно.
|
||||
|
||||
### Race conditions
|
||||
В Python-версии всё однопоточное — гонок нет, **пока** один экземпляр `AndrOBD` не шарится между потоками. Если планируется параллельный доступ — добавьте `threading.Lock` вокруг `send()`.
|
||||
|
||||
---
|
||||
|
||||
## Вопрос 2. HTTP 499 при upload с мобильной сети
|
||||
|
||||
### 🔴 Нет идемпотентности → дубликаты при ретрае (вопрос 2.5)
|
||||
Это главная проблема. Сценарий 499: сервер **уже принял и обработал** запрос (LLM-анализ 30–120с), но клиент отвалился по read timeout и шлёт ретрай. Результат — вторая полная LLM-сессия и **вторая запись в `sessions`**. `upload_session()` не имеет ключа идемпотентности.
|
||||
|
||||
Решение — клиент генерирует `request_id` (UUID), сервер кэширует результат:
|
||||
```python
|
||||
data = request.get_json(silent=True)
|
||||
req_id = data.get("request_id")
|
||||
if req_id:
|
||||
cached = db.get_session_by_request_id(req_id) # + колонка request_id UNIQUE
|
||||
if cached:
|
||||
return jsonify(cached["response_json"]), 200
|
||||
```
|
||||
|
||||
### 🟡 Стратегия ретраев — нужен backoff и идемпотентность
|
||||
3 ретрая с фиксированной задержкой 2с на мобильной сети мало помогают: если причина — долгий LLM-ответ (>read timeout 180с), то все 3 попытки упрутся в тот же таймаут и каждая запустит новый LLM-прогон. Рекомендация: exponential backoff (2/4/8с + jitter) **и** обязательно идемпотентность (см. выше), иначе ретраи только множат нагрузку.
|
||||
|
||||
### 🟡 Корень 499 — рассинхрон таймаутов клиент/сервер (вопрос 2.1)
|
||||
Клиентский read 180с ≈ gunicorn timeout 180с. При длинном ответе LLM (`Diagnoser.timeout=120`, но сам upload может суммарно дольше) клиент рвёт соединение ровно в момент, когда сервер ещё пишет ответ. Прочие частые причины 499 на мобильной: смена сети Wi-Fi↔LTE (новый IP, старый сокет мёртв), NAT-таймаут оператора (часто 30–60с тишины), Doze/засыпание приложения. Рекомендация: клиентский read timeout должен быть **строго больше** серверного (например, 240с против gunicorn 180с), а сервер — отвечать быстрее (streaming, см. ниже).
|
||||
|
||||
### 🟡 Write timeout на медленной сети (вопрос 2.3)
|
||||
Да, при толстом батче (`raw_responses` целиком) и слабом upload на LTE write timeout 60с реально достижим. Тело JSON со всеми сырыми ответами может быть десятки–сотни КБ.
|
||||
|
||||
### 🟡 Чанки/сжатие (вопрос 2.4)
|
||||
Чанкинг избыточен для типичного объёма, а вот **gzip тела** даст быстрый выигрыш (JSON сжимается в 5–10 раз) и снимет риск write timeout:
|
||||
```kotlin
|
||||
// OkHttp: добавить gzip-обёртку RequestBody + заголовок
|
||||
.header("Content-Encoding", "gzip")
|
||||
```
|
||||
Сервер: nginx сам разожмёт при наличии `gunzip`/decompression, либо Flask с `request.get_data()` + `gzip.decompress`. Это дешевле, чем переписывать на чанки.
|
||||
|
||||
### Главная архитектурная рекомендация
|
||||
Разделите «приём данных» и «LLM-анализ». Эндпоинт должен **быстро** (1–2с) принять батч, сохранить, вернуть `session_id`, а диагноз отдавать отдельным polling-эндпоинтом (`GET /api/v1/session/<id>/result`) или через streaming. Тогда 499 из-за долгого LLM исчезнет как класс.
|
||||
|
||||
---
|
||||
|
||||
## Вопрос 3. Архитектура: три модуля + Android пакеты
|
||||
|
||||
### 🟢 Границы модулей в целом чистые
|
||||
`obd/` ничего не знает про `brain/` и `api/`; `brain/` — изолированный LLM-клиент; `api/` оркестрирует. Направление зависимостей `web → api → brain/obd` корректное (вопрос 3.4 — да, правильное).
|
||||
|
||||
### 🟡 Импорты внутри функций (вопрос 3.2)
|
||||
В `routes.py` все `from brain.client import Diagnoser`, `from api.db import Database`, `from api.config import load` сделаны внутри обработчиков. Это не «нормально», а компромисс — обычно так лечат циклические импорты или ускоряют старт. Минусы: `load()` читает конфиг с диска **на каждый запрос**, импорт-резолвинг повторяется. Рекомендация: поднять импорты на уровень модуля, а конфиг закэшировать:
|
||||
```python
|
||||
# api/config.py
|
||||
from functools import lru_cache
|
||||
@lru_cache(maxsize=1)
|
||||
def load(): ...
|
||||
```
|
||||
Если поднятие импортов ломает цикл — это сигнал, что цикл надо разорвать явно, а не прятать.
|
||||
|
||||
### 🟡 `config.yaml` (вопрос 3.3)
|
||||
Конфиг сейчас грузится через `api/config.py`. Держать `config.yaml` в корне проекта (рядом с `web/app.py`) логичнее — он общий для `api/`, `brain/`, `obd/`, а не принадлежит только `api/`. Вынесите на верхний уровень, путь резолвьте от корня.
|
||||
|
||||
### 🟢 Заменяемость модулей (вопрос 3.5)
|
||||
`brain/` заменяется на локальный LLM тривиально — он зависит только от OpenAI-совместимого HTTP (`/chat/completions`). Достаточно сменить `base_url`/`model` в конфиге; код менять не нужно. `obd/` тоже изолирован. Это хороший знак для дизайна.
|
||||
|
||||
---
|
||||
|
||||
## Вопрос 4. SQL-схема: таблица sessions
|
||||
|
||||
### 🔴 Утечка соединений + конкурентный доступ (вопрос 4.5)
|
||||
`Database()` создаётся в каждом запросе, открывает `sqlite3.connect(...)` и **никогда не закрывается** — connection leak. При 4 gunicorn-воркерах одновременные записи в один файл дают `database is locked` (SQLite по умолчанию: 1 писатель, нет ожидания). Минимум:
|
||||
```python
|
||||
self.conn = sqlite3.connect(str(self.path), timeout=30, check_same_thread=False)
|
||||
self.conn.execute("PRAGMA journal_mode=WAL") # параллельные читатели + 1 писатель
|
||||
self.conn.execute("PRAGMA busy_timeout=30000")
|
||||
```
|
||||
И закрывать соединение (контекстный менеджер / `try/finally` / `db.close()`), либо держать один пул на воркер. WAL критичен для multi-worker.
|
||||
|
||||
### 🟡 30+ колонок в одной таблице (вопрос 4.1)
|
||||
Для SQLite это **нормально** (лимит 2000 колонок), денормализация под аналитику оправдана. Но смешаны три логических домена: телефон, ELM, LLM. Это не баг, а запах. Пока таблица аналитическая (одна запись = одна сессия) — оставьте; если начнёте часто менять набор полей телефона/ELM — выносите в отдельные таблицы или JSON-колонку.
|
||||
|
||||
### 🟢 raw_responses как JSON TEXT (вопрос 4.2)
|
||||
Ок для SQLite. При необходимости запросов внутрь — используйте `json_extract()` (есть в SQLite ≥3.38). Менять не нужно.
|
||||
|
||||
### 🟡 Индексы (вопрос 4.3)
|
||||
`created_at`, `vin`, `elm_mac`, `android_id` — разумный набор. Но `vin` nullable и часто NULL — индекс будет «разреженным», это норм. Добавьте составной `(android_id, created_at)` если будете строить историю по устройству — иначе текущих достаточно.
|
||||
|
||||
### 🟡 Мёртвые таблицы (вопрос 4.4)
|
||||
`cars`, `diagnostic_tokens`, `llm_messages`, `ecu_parameters`, `dtc_codes` создаются в `_init_schema()`, имеют методы-обёртки в `db.py`, но в текущем пути `upload`/`chat` **не используются**. Это «второй контур», который вводит в заблуждение (например, история диалога в `/chat` идёт из клиента, а не из `llm_messages`). Решение: либо подключите их (тогда `/chat` сможет хранить историю на сервере по VIN), либо удалите вместе с методами. Сейчас они — технический долг и риск рассинхрона схемы.
|
||||
|
||||
### 🟢 Инъекции
|
||||
Все запросы параметризованы (`?`), SQL-инъекций нет.
|
||||
|
||||
---
|
||||
|
||||
## Вопрос 5. LLM-интеграция: промпты и таймауты
|
||||
|
||||
### 🔴 Рассинхрон модели и таймаута в коде
|
||||
- `brain/client.py`: `DEFAULT_MODEL = "gpt-oss-20b"`, а конфиг/доки — `gpt-oss-120b`. Дефолт-fallback тихо подменит модель, если конфиг недокинул `model`.
|
||||
- `Diagnoser.ask(... timeout=120)`, но в вопросе и nginx/gunicorn заявлено 180с. Таймаут захардкожен и не берётся из конфига.
|
||||
- Докстринги и комментарии говорят «DeepSeek», хотя API — `api.aillm.ru` / gpt-oss. Чисто косметика, но путает.
|
||||
|
||||
```python
|
||||
def __init__(self, api_key, model="gpt-oss-120b", base_url=DEFAULT_BASE, timeout=180):
|
||||
...
|
||||
self.timeout = timeout
|
||||
def ask(self, messages):
|
||||
resp = requests.post(..., timeout=self.timeout)
|
||||
```
|
||||
|
||||
### 🟡 Нет streaming + heartbeat (вопрос 5.2)
|
||||
`requests.post` без `stream=True` на 120–180с — это «чёрный ящик»: клиент не видит прогресса и рвёт по таймауту (см. Вопрос 2). Для длинной генерации лучше streaming (SSE) с проксированием токенов клиенту — тогда соединение «живое», NAT не закрывает, 499 пропадает. Минимум — heartbeat-байты каждые N секунд.
|
||||
|
||||
### 🟡 История диалога в /chat (вопрос 5.3)
|
||||
История склеивается в **один user-prompt** строкой («Водитель: …/Автоэксперт: …»), а не передаётся как полноценный массив `messages` с ролями. Модель хуже держит контекст, и при длинной истории (даже срезанной до 10) промпт может раздуться. Лучше передавать историю настоящими `role: user/assistant` сообщениями (метод `diagnose` это уже умеет через `history`!) и считать токены, а не сообщения:
|
||||
```python
|
||||
hist_msgs = [{"role": m["role"], "content": m["content"]} for m in history[-10:]]
|
||||
answer = diagnoser.diagnose(SYSTEM_CHAT, question, history=hist_msgs)
|
||||
```
|
||||
Переполнения контекста сейчас никто не контролирует — добавьте бюджет по токенам.
|
||||
|
||||
### 🟡 Обработка ошибок LLM (вопрос 5.5)
|
||||
Сейчас один общий `except Exception` → строка «LLM недоступен: {e}». Нет различия rate limit (429, нужен retry-after), timeout (нужен ретрай), 5xx (ретрай) vs 4xx (не ретраить). И текст исключения уходит **прямо в ответ пользователю** — может протечь URL/детали. Разделите коды:
|
||||
```python
|
||||
try:
|
||||
...
|
||||
except requests.Timeout: # ретрай
|
||||
except requests.HTTPError as e:
|
||||
if e.response.status_code == 429: ... # backoff по Retry-After
|
||||
```
|
||||
|
||||
### 🟢 Промпт для диагностики (вопрос 5.1)
|
||||
`SYSTEM_PROMPT` сильный: 10 правил, явный формат с таблицами, проценты уверенности, «проверь перед заменой», секция «если не поможет». Это хорошо. Чего не хватает: (1) данных об авто (make/model/year/engine почти всегда отсутствуют — VIN есть, но не расшифровывается), (2) пробег/условия, (3) явного запрета галлюцинировать значения PID, которых нет в данных. Добавьте расшифровку VIN→марка/год (хотя бы WMI) перед отправкой — резко поднимет качество.
|
||||
|
||||
### 🟡 Выбор gpt-oss-120b (вопрос 5.4)
|
||||
Для авто-диагностики ключевое — знание DTC и инженерная логика. 120b разумен как баланс цена/качество. Альтернативы под задачу: Qwen2.5-72B/Qwen3 (хорош в технике, но у вас отмечен CoT-leak баг на fp8-варианте), DeepSeek-V3 (сильная техничка), либо рассуждающая модель (o-серия/R1) для сложных взаимосвязей — но они дороже и медленнее, что усугубит проблему таймаутов из Вопроса 2. Вывод: 120b ок, менять стоит только если качество разбора DTC не устраивает.
|
||||
|
||||
---
|
||||
|
||||
## Вопрос 6. Безопасность API
|
||||
|
||||
### 🔴 Любой эндпоинт без аутентификации → бесплатный прокси к платному LLM (вопросы 6.1, 6.4)
|
||||
`/api/v1/chat`, `/api/v1/session/upload`, `/api/v1/ping-llm` дёргают платный LLM **без какой-либо аутентификации и без rate limit**. Любой, кто узнал домен, может в цикле слать `/chat` и жечь ваш токен `api.aillm.ru`, а `/ping-llm` вообще тратит LLM-вызов на каждый GET. Для MVP HTTPS защищает только канал, но не от абуза. Минимум:
|
||||
- статический API-ключ приложения в заголовке (да, его можно вытащить из APK, но он отсекает массовый скан-абуз);
|
||||
- rate limiting на nginx (`limit_req_zone`) и/или Flask-Limiter по IP/`android_id`;
|
||||
- `/ping-llm` не должен реально вызывать LLM на каждый пинг — кэшируйте результат на 1–5 мин.
|
||||
|
||||
```nginx
|
||||
limit_req_zone $binary_remote_addr zone=api:10m rate=10r/m;
|
||||
location /api/v1/chat { limit_req zone=api burst=5 nodelay; ... }
|
||||
```
|
||||
|
||||
### 🟡 XSS через diagnosis/raw (вопрос 6.3 — не инъекция, а отображение)
|
||||
SQL-инъекции через `raw_responses` нет (запросы параметризованы). **Но**: ответ ELM327 и текст диагноза от LLM (markdown с таблицами) где-то рендерятся в вебе (`web/templates/index.html`, дашборд сессий). Если markdown/HTML вставляется без экранирования — это stored XSS: вредонос в `raw` ELM или в ответе LLM выполнится в браузере админа. Проверьте, что вывод экранируется (Jinja autoescape по умолчанию вкл — не отключайте `|safe` на этих полях; markdown рендерьте через санитайзер).
|
||||
|
||||
### 🟡 Утечка деталей в ответах
|
||||
`except ... return f"LLM недоступен: {e}"` и `error: str(e)[:100]` отдают внутренние сообщения наружу. Логируйте полностью, клиенту — обобщённый текст.
|
||||
|
||||
### 🟡 API-ключ LLM (вопрос 6.5)
|
||||
Текущая схема (ключ только на сервере, не в APK) — **правильная**, это лучшее в безопасности проекта. Дополнительный прокси не нужен; достаточно закрыть абуз (rate limit + ключ приложения), чтобы вашим серверным ключом не пользовались чужие.
|
||||
|
||||
### Сводка по безопасности
|
||||
| Мера | Приоритет | Статус |
|
||||
|------|-----------|--------|
|
||||
| Rate limiting (nginx/Flask-Limiter) | 🔴 высокий | нет |
|
||||
| Ключ приложения в заголовке | 🟡 средний | нет |
|
||||
| `/ping-llm` без реального LLM-вызова | 🟡 средний | вызывает LLM |
|
||||
| Экранирование diagnosis/raw в вебе | 🟡 средний | проверить |
|
||||
| Не отдавать текст исключений клиенту | 🟡 средний | отдаёт |
|
||||
| Ключ LLM только на сервере | 🟢 | сделано |
|
||||
|
||||
---
|
||||
|
||||
## Итоговый топ проблем (по убыванию важности)
|
||||
|
||||
1. 🔴 **Нет идемпотентности upload** → дубликаты сессий и двойной расход LLM при 499/ретраях (Q2).
|
||||
2. 🔴 **Открытые LLM-эндпоинты без auth/rate-limit** → абуз платного токена (Q6).
|
||||
3. 🔴 **SQLite: утечка соединений + нет WAL/busy_timeout** при 4 воркерах → `database is locked` (Q4).
|
||||
4. 🔴 **Долгий синхронный LLM в запросе** — корень 499; разделить приём данных и анализ, добавить streaming (Q2, Q5).
|
||||
5. 🔴 **`reset_input_buffer` перед командой** в стейт-машине — иначе десинхрон ELM327 (Q1).
|
||||
6. 🟡 Рассинхрон модели/таймаута в `client.py` (20b vs 120b, 120с vs 180с) (Q5).
|
||||
7. 🟡 История диалога `/chat` строкой вместо ролей `messages` (Q5).
|
||||
8. 🟡 Мёртвые таблицы в схеме — подключить или удалить (Q4).
|
||||
|
||||
## Что уже хорошо 🟢
|
||||
- Чистые границы модулей, заменяемый `brain/`.
|
||||
- Сильный диагностический системный промпт.
|
||||
- Параметризованный SQL (нет инъекций).
|
||||
- Ключ LLM не в APK.
|
||||
- Стейт-машина ограничена по ретраям (нет бесконечных циклов).
|
||||
Reference in New Issue
Block a user