21 KiB
SPEC-REVIEW-617-r1
Issue: #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
Материал раунда
- Ветка:
dev, коммит0f97e3e639c4— ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. - Дерево материала:
f2603b2998c17fd63d3bc1248b5e2be36e4b598fgit log --all --format='%H %T' | grep f2603b2998c1 - Тело issue:
f8d7f51e97db367fb59302863a5ab105d72a8aa34c7fa955fdc6ee29139973c6 - Вердикт конвейера:
green· High 0