Files
2026-09-24 01:14:48 +00:00

21 KiB
Raw Permalink Blame History

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 — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
  • Дерево материала: f2603b2998c17fd63d3bc1248b5e2be36e4b598f
    git log --all --format='%H %T' | grep f2603b2998c1
    
  • Тело issue: f8d7f51e97db367fb59302863a5ab105d72a8aa34c7fa955fdc6ee29139973c6
  • Вердикт конвейера: green · High 0