Files
houseplan-card/docs/reviews/SPEC-REVIEW-617-r1.md
T
2026-09-24 01:14:48 +00:00

230 lines
21 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# SPEC-REVIEW-617-r1
**Issue:** [#617](https://github.com/Matysh/houseplan-card/issues/617) — «Загрузка плана > ~3 МиБ обрывает WebSocket без сообщения: MAX_PLAN_BYTES 8 МиБ недостижим через 4 МиБ кадр»
**Этап:** ТЗ на ревью (PROCESS.md §2.4), трек `small`
**Заход:** r1 · блокирующих циклов израсходовано 0 из 2 (лимит лёгкого трека)
**Ревьюер:** независимая сессия, без контекста автора ТЗ
**Вердикт: зелёный**
---
## Скоуп ревью
ТЗ живёт в теле issue #617, раздел `## ТЗ` (решение владельца #517 — файл в
`docs/specs/` не создаётся). Материал ревью — ровно этот текст, см. блок
«Материал раунда» в конце документа.
Задача: 4 МиБ дефолтный кадр WS aiohttp делает объявленный `MAX_PLAN_BYTES = 8
МиБ` недостижимым — файл плана > ≈3 МиБ рвёт соединение вместо честной ошибки
`too_large`. Выбран вариант (б): новый HTTP-view `POST
/api/houseplan/plans/upload` со стриминговым пределом, карточка грузит план
только через него; WS `houseplan/plan/set` остаётся для старых закешированных
карточек без изменения контракта.
## Как проверялось
Ревью состязательное: identifiers и утверждения о существующем поведении из ТЗ
сверены с реальным кодом на `dev` (SHA см. ниже), а не приняты на слово автора.
Прочитано перед вынесением вердикта:
- `docs/SCOPE.md` (персона «Home admin», out-of-scope список — задача не
задевает ни один пункт: не CAD/PDF/3D/радар/оверлей, обычная правка
editor-only поверхности);
- `PROCESS.md` §1–§10 (классы файлов, ЖЦ, лёгкий трек §5, лимит циклов §4,
формат вердикта §7.2);
- `AGENTS.md` (роли, трейлеры, гейты);
- `docs/TOUCH-SUPPORT.md` (проверка утверждения ТЗ «touch не затронут» —
подтверждено: правка живёт в editor-only диалоге пространства, View/kiosk не
трогается, а редакторы по продукту desktop-first);
- само тело issue #617 целиком, включая аналитический комментарий-обоснование
трека `small`.
Код читан выборочно, но по каждому явному утверждению ТЗ о существующем
поведении — не по касательной:
| Утверждение ТЗ | Файл:строка | Результат сверки |
|---|---|---|
| `MAX_PLAN_BYTES = 8 * 1024 * 1024` в `validation.py` | `custom_components/houseplan/validation.py:24` | совпадает |
| `ws_plan_set` шлёт `too_large` при `len(raw) > MAX_PLAN_BYTES`, копия-при-записи `<space>.<token>.<ext>`, `check_quota`+`atomic_write` под `upload_lock` | `custom_components/houseplan/websocket_api.py:2330–2384` | совпадает |
| `may_write` / `_check_write` — общая точка авторизации WS и HTTP | `custom_components/houseplan/auth.py:16`, `websocket_api.py:306` | совпадает, один helper |
| `HouseplanDecorAssetUploadView` буферизует тело в памяти блоками, `_FLUSH_AT`-преflight по `Content-Length` | `custom_components/houseplan/http_api.py:218–352`, `:68` | совпадает — это и есть ближайший шаблон для «принято предположительно» п.5 |
| `HouseplanUploadView` — паттерн `fetchWithAuth` + `FormData`, разбор `err.too_large`/`err.unauthorized`/`err.bad_ext` | `houseplan-editor-runtime.ts:7371–7412` | совпадает, включая точный текст `err.too_large` с `{mb}` из ответа |
| `classifyPlanFile(file, guardAboveBytes)` — svg проходит без проверки размера, растр уходит в guard при `size > guardAboveBytes` независимо от `probe.kind` | `src/backdrop-pick.ts:87-99` | совпадает: раздельная обработка SVG/растра в ТЗ верна |
| `renderBackdropGuard(..., allowOriginal = true)` | `src/backdrop-pick.ts:157-163`, вызов для декора `houseplan-editor-runtime.ts:7949` (`allowOriginal = size <= 2 МиБ`) | **параметр уже существует и уже используется декором** — см. находку Low ниже |
| `PlanFilePayload.b64`, `space-dialog.ts` тип, `space-form-state.ts` сравнивает только `.name` | `src/backdrop-pick.ts:14-19`, `src/space-dialog.ts:17`, `src/editors/space-form-state.ts:30` | совпадает |
| `_saveSpaceDialog`: «загрузить ДО изменения конфига», перечитывание конфига в catch, `toast.error` | `houseplan-editor-runtime.ts:8084-8096`, `:8213-8227` | совпадает |
| `probeBackdrop` даёт ровно `safe/warn/hard/unknown` | `src/backdrop-probe.ts:29,50-52` | совпадает |
| USER-GUIDE уже обещает «8 МБ» (ru/en) | `docs/USER-GUIDE.ru.md:2225`, `docs/USER-GUIDE.md:1385` | совпадает |
| `PLAN_EXTENSIONS`, `valid_space_id`, `CONTENT_URL`, `atomic_write`, `check_quota` | `validation.py:22,1156`, `const.py:21`, `plans.py:29,240` | все существуют, сигнатуры как в ТЗ |
| `demo/smoke_backdrop_guard.mjs` сегодня сверяет `planFile.b64` побайтово («Оставить оригинал») | `demo/smoke_backdrop_guard.mjs:73-84,108,270` | совпадает — правка смока действительно необходима и её объём соответствует заявленному |
| `scripts/mutation-registry.mjs`, `scripts/mutation-gate.mjs`, `scripts/check-inputs.mjs`, `scripts/smoke-select.mjs` существуют | `ls scripts/*.mjs` | совпадает |
Отдельно проверена «граница включительная» (AC: `MAX_PLAN_BYTES` проходит,
`+1` отклоняется): и `ws_plan_set` (`> MAX_PLAN_BYTES`), и
`HouseplanDecorAssetUploadView` (`> MAX_DECOR_ASSET_BYTES`) используют строгое
`>`, т.е. равенство пределу проходит — контракт для нового view, списанный с
этого же шаблона, воспроизводим и непротиворечив.
Это не гейт-прогон (стадия ТЗ, кода ещё нет) — типизация/тесты/сборка здесь
неприменимы. Проверка кода была вместо «поверил автору» — цель прочитанного
выше.
## Разделы §7.1 — присутствуют все
Сценарий · что человек увидит до/после · проблема · скоуп/не-скоуп · контракт
поведения · UX · модель данных и миграция (явное «нет») · i18n · AC1…AC8 с
таблицей «чем доказан / чем краснеет» · план автотестов · перф/touch · риски ·
откат · release-артефакты · «принято предположительно» (8 пунктов). Пункты DoR
§2.5 покрыты: миграция/compatibility — явное «нет» со ссылкой на
`CONFIG-COMPATIBILITY.md`; touch — явное «не затронут» с корректным
обоснованием через персону/поверхность; открытых продуктовых вопросов нет.
## AC — проверка однозначности и доказуемости
AC1–AC8 пронумерованы, у каждого в таблице указан и тест, и мутация/негативная
проба («чем краснеет») — защитные AC (AC2–AC5, AC7 частично) не остаются без
названного свидетеля. Разобраны предметно:
- **AC1** — «PNG, проба `safe`» как явное условие теста снимает
недетерминированность (без этого уточнения 5 МиБ файл мог бы попасть в guard
по `warn`); формулировка «ровно один POST … и ни одного `plan/set`» считается
счётчиком, не текстом — однозначно.
- **AC2/AC3** — раздельные ветки SVG/растра в контракте совпадают с реальным
разделением в `classifyPlanFile` (SVG обходит проверку размера в самой
функции, поэтому проверка предела для SVG обязана жить отдельно в
`_pickPlanFile` — ТЗ описывает это как отдельный пункт списка, а не путает
два пути). Условие AC3 «с читаемыми размерами» корректно исключает
`hard`/`unknown`, для которых контракт сознательно не меняет поведение (см.
находку Low о непокрытом случае — не блокирует).
- **AC4** — таблица кодов ответа полная и без пересечений (403/503/413×2/400×3/507/200);
граница `MAX`/`MAX+1` и отсутствие временных файлов после 413 — измеримо
прогоном backend-теста.
- **AC5** — единственный AC, доказывающий защиту «клиент лжёт/обойдён» —
свидетель назван (мутация «удаление разбора `too_large`»), третий столбец не
пуст.
- **AC6** — «одно число» (см. предупреждение о числах в §8 гейтов) закрыто
явным тестом, читающим оба источника плюс оба USER-GUIDE — ровно то, что
требует правило «одно число — один источник».
- **AC7/AC8** — паритет и «общий хелпер» доказываются существующими тестами +
ревью кода с явной пометкой «проверено чтением, не исполнением» ожидается на
этапе код-ревью — здесь это корректно поставлено в план, не выдано за
готовый факт.
Ни одного места, где решение приложения выдано за факт без пометки: восемь
пунктов «принято предположительно» покрывают именно то, что пользователь не
наблюдает (выбор варианта (б) обоснован в аналитике и в самом ТЗ, а не
голословен) либо являются реализационными деталями (URL, буферизация,
именование). Продуктовых вопросов владельцу не задано и не нужно было —
формулировка «что человек увидит до/после» отвечает на оба вопроса, которые
вправе решать только он.
## Находки
### Low-1 — «новый необязательный параметр» уже существует
`docs/reviews` н/п. **Файл:** `src/backdrop-pick.ts:157-163` (сигнатура
`renderBackdropGuard`), уже используется декором в
`houseplan-editor-runtime.ts:7949`.
ТЗ (раздел «Не-скоуп») формулирует: «Общий `renderBackdropGuard` меняется
только добавлением необязательного параметра; поведение декора прежнее» —
подразумевая, что параметр `allowOriginal` появится этой задачей. Он уже
существует и уже используется тем же декором, на который ТЗ ссылается как на
прецедент. Значит для скрытия кнопки «Оставить оригинал» этой задаче не нужно
менять сигнатуру вовсе — только передать шестой аргумент из вызова для плана.
Действительно новым остаётся только параметр, переключающий ТЕЛО диалога
(`over_limit_body` vs `large_body`), который в разделе выше назван отдельно и
верно. Не блокирует ни один AC и не меняет объём работы по сути — это неточная
атрибуция «что уже есть» vs «что появится», сама работа от этого не меняется.
Правится словом при следующей правке ТЗ или снимается с записью — не
возвращает задачу на цикл.
### Наблюдение (не находка, не блокирует)
Случай «растр больше предела **и** проба `hard`» ТЗ явно не разбирает (AC3
условие «с читаемыми размерами» его обходит стороной). Сегодняшний код и без
этой задачи не даёт в `hard`-случае ни одной кнопки действия, кроме «Отмена»
(`renderBackdropGuard`: `${hard ? null : …}`) — то есть пользователь и сейчас
не может загрузить/уменьшить `hard`-файл, независимо от байтового предела.
Задача не ухудшает этот путь и не обязана его чинить (не про размер в байтах,
а про безопасность декодирования — чужой скоуп). Упоминаю, чтобы будущий
ревьюер кода не считал это пропущенным AC.
### Трек `small` — соразмерность (не находка)
Задача добавляет новый аутентифицированный write-эндпоинт — обычно повод для
внимательности при выборе лёгкого трека. Аналитический комментарий явно прошёл
все пять критериев §5 и обосновал каждый (сложность 3 — копия двух уже
существующих шаблонов view почти дословно; риск закрыт паритетом проверок с
`ws_plan_set` и общим writer'ом; одна поверхность — один диалог, один
эндпоинт; нет миграции/UX-контракта/перф/touch-влияния). Сверка кода в этом
документе подтверждает, что описанные шаблоны (`HouseplanDecorAssetUploadView`,
`HouseplanUploadView`, `may_write`, `check_quota`, `atomic_write`) существуют
и совпадают буквально — обоснование не голословно. Оставляю как отмеченное, а
не как находку: критерии §5 не про «есть ли новый эндпоинт», а про
сложность/риск/поверхность/миграцию/UX/перф-touch, и все пять по существу
закрыты.
## Что проверено и корректно
- Все обязательные разделы §7.1 присутствуют, DoR §2.5 выполним по тексту ТЗ.
- Каждый явный технический факт о существующем коде, на который опирается
контракт, сверен построчно (таблица выше) — ни одной догадки, выданной за
факт.
- AC1–AC8 пронумерованы, однозначны, у каждого назван способ доказательства;
защитные AC несут третий столбец «чем краснеет» с конкретной мутацией.
- «Одно число» (`MAX_PLAN_BYTES`) имеет план проверки на трёх источниках сразу
(TS, Python, оба USER-GUIDE) — правило §8 учтено на стадии ТЗ, а не оставлено
коду.
- i18n: ключи ru/en перечислены дословно, de/fr делегированы с сохранением
плейсхолдеров и стиля существующего `err.too_large`; мёртвых ключей не
вводится.
- Совместимость: WS `plan/set` не меняет контракт (не-скоуп явно), общий writer
для обоих путей записи специфицирован как единственная точка проверок.
- Откат тривиален и описан честно (файлы не мигрируют, revert коммита
достаточен).
- Touch/перф корректно обоснованы как «не затронуты» со ссылкой на
правильную персону/поверхность (сверено с `TOUCH-SUPPORT.md`).
## Чего не проверял
- Не проверял сами тесты/смоки — на стадии ТЗ их ещё нет; их придётся
перепроверить на код-ревью, включая «умеет ли тест падать» для каждого из
трёх заявленных мутантов.
- Не проверял, что `PLANS_DIR`, `MAX_PLANS_BYTES`, `MAX_PLANS_FILES`
(папочные квоты) действительно не нуждаются в правке для нового
write-пути — ТЗ утверждает переиспользование через общий writer, что снимает
вопрос конструктивно (не два места квот), но исполнение проверит код-ревью.
- Не гонял никаких CI/локальных гейтов — на этапе ТЗ они неприменимы (нет кода
для проверки; ссылка на них в самом ТЗ — план, а не отчёт об исполнении).
- Не оценивал производительность потоковой буферизации 8 МиБ в памяти под
параллельными аплоадами — тот же паттерн, что и у декора (2 МиБ), риск не
новый по порядку величины и явно назван в разделе «Риски» ТЗ.
## Материал раунда
```
Issue: #617
Repo: Matysh/houseplan-card
SHA рабочей копии (репозиторий, не материал ТЗ): 0f97e3e639c4eb06b7866b8c1c757d98e2c0a5f7
ТЗ: тело issue #617, раздел «## ТЗ»
sha256(нормализованное тело issue, UTF-8, как получено `gh issue view 617 --json body`): 696d986cca3720a468ba776055d544cececd4713062b4c5e5b6e2e44ced87092
Длина тела: 18220 байт
Аналитический комментарий (трек/оценка): Matysh, 2026-09-24T01:01:56Z
```
---
**Вердикт: зелёный · заход r1 · блокирующих циклов 0/2 · High: 0 · Medium: 0**
---
<!-- material-anchors: сгенерировано конвейером (#414) -->
## Материал раунда
- Ветка: `dev`, коммит `0f97e3e639c4` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
- Дерево материала: `f2603b2998c17fd63d3bc1248b5e2be36e4b598f`
```
git log --all --format='%H %T' | grep f2603b2998c1
```
- Тело issue: `f8d7f51e97db367fb59302863a5ab105d72a8aa34c7fa955fdc6ee29139973c6`
- Вердикт конвейера: `green` · High 0