Files
houseplan-card/docs/reviews/SPEC-REVIEW-498-r2.md
2026-09-09 07:06:20 +00:00

18 KiB
Raw Permalink Blame History

SPEC-REVIEW-498-r2

  • Issue: https://github.com/Matysh/houseplan-card/issues/498
  • ТЗ: docs/specs/498-backend-hardening-quota-palette-svg-refs.md
  • Материал: ветка issue/498-backend-hardening-quota-palette-svg-refs, SHA 2ed7bbe679ec06532359da87a3fa7dac9b898a7a (единственный коммит поверх r1: docs: spec #498 r2 — numbered AC, longest-chain reference limit, concurrent quota semantics)
  • Этап: ТЗ на ревью (PROCESS.md §2.4), заход r2, блокирующих циклов израсходовано 1 из 4
  • Трек: полный (не изменился)
  • Предыдущий раунд: docs/reviews/SPEC-REVIEW-498-r1.md, вердикт жёлтый на SHA 95493de2fcdbf4e27b91038de92ccebab42719ec (High: 0, Medium: 2 в скоупе)

Скоуп раунда

Разбор идёт по дельте (PROCESS.md §2.9/§2.10): дельта локальна — единственный коммит правит только docs/specs/498-backend-hardening-quota-palette-svg-refs.md (git diff 95493de2..2ed7bbe6 — 31 добавление / 10 удалений в одном файле), тело issue не менялось (последний человеческий комментарий — постановка от 2026-09-09, S2-аналитика; после вердикта r1 новых комментариев от владельца нет). Продуктовая рамка, персона и не-скоуп не затронуты — переносятся из r1 без повторной проверки. Полный разбор не требуется: рамка не изменилась, новая подсистема не появилась, объём дельты — точечное закрытие двух Medium предыдущего раунда, а не пересмотр задачи.

Дельта правит ровно две вещи:

  1. Добавляет раздел «## 8. Критерии приёмки» (AC1–AC8, пронумерованные, с доказательством) и делает ## 8. Совместимость и откат → ## 8.0.
  2. Переписывает §4.2 (семантика параллельных загрузок) и §6 (алгоритм обхода графа ссылок SVG) + добавляет тест на недоброжелательный порядок id в §7.3, шестой мутант в §7.4.

Как проверялось

Стадия spec-review — гейты (typecheck/test/build) не запускались, продуктовый код не менялся. Каждое утверждение дельты проверено:

  • AC-раздел (§8): сверил каждый AC1–AC8 построчно с соответствующим текстом §2/§4/§5/§6/§7 — противоречий нет, имена тестов в AC совпадают 1:1 с именами в §7 (grep -n "test_issue_498" по файлу — все шесть имён встречаются ровно дважды: один раз в §7, один раз в привязанном AC, без расхождений). AC8 явно закрывает «нет» по перфу/touch, как требовал r1.
  • Алгоритм §6 (мемоизированный longest-path вместо булева visited): прогнал руками адверсариальный пример из r1 (лимит=2, граф a→d, b→c, c→a, sorted(ids)=[a,b,c,d], что при старом алгоритме проходило все проверки, хотя истинный путь b→c→a→d длиной 4 вдвое больше лимита). Трасса нового алгоритма: _visit(a) → longest[d]=1, longest[a]=max(1, longest[d]+1)=2 (не >2, принято, путь a→d действительно длины 2 — верно); _visit(b) → c не в longest, спуск, ссылка c→a: a уже в longest → chain(c)=max(1, longest[a]+1)=3; c исчерпан → longest[c]=3 > 2 → too_large. Мемоизированная длина корректно всплывает через уже обработанный узел независимо от порядка обхода — дыра r1 закрыта, не декларативно, а по факту пересчёта.
  • Эндпойнт в AC6/§7.3 (/api/houseplan/assets/upload): в r1-версии ТЗ (и в исходном S2-тексте) фигурировал несуществующий путь /api/houseplan/decor/assets; в r2 путь исправлен. Сверил с кодом: custom_components/houseplan/http_api.py:221 — class HouseplanDecorAssetUploadView, url = "/api/houseplan/assets/upload" — совпадает буква в букву с AC6/§7.3 r2. Незамеченная r1 неточность устранена попутно.
  • Семантика параллельных загрузок §4.2 (консервативно отклоняются обе, если обе проверки идут, пока оба файла staged): сверил с реальным путём исполнения HouseplanUploadView.post (http_api.py:352-460) — check_quota вызывается через await hass.async_add_executor_job(...) после того, как тело файла уже полностью дописано в .upload-* через отдельные executor-джобы; между стадией «staged» и вызовом check_quota есть await-точка, так что при двух конкурентных запросах, стартовавших почти одновременно, к моменту вызова check_quota оба временных файла уже реально лежат на диске — сценарий «оба видят чужой staged-файл» не гипотетический, а прямое следствие текущей структуры кода. Тест 7.1 (не [200, 200]) корректно допускает оба исхода (один успех/один отказ и оба отказа), не переобещая детерминизм, которого код не даёт.
  • Все шесть мутантов §7.4: два новых (svg-reference-depth-per-start-not-per-chain, дублирующий обход svg-reference-walk-recursive-again не менялся) имеют явных свидетелей в §7.3; убедился, что новый тест на недоброжелательный порядок id (n0065→n0064→…→n0001, 65 узлов) — единственный тест-свидетель первого нового мутанта, и что «естественный» тест (g0…g2499, по возрастанию) его не покрывает (совпадает с рекомендацией r1).
  • Release-артефакты и DoR-опоры, не тронутые дельтой (release-артефакты §9, откат §8.0, риски §8.2, i18n §8.1, не-скоуп §3): сверены на факт присутствия и внутренней непротиворечивости, детальная построчная проверка кода под ними наследуется из r1 (код не менялся).
  • Существование docs/CHANGELOG.md, docs/CHANGELOG.ru.md, docs/SUPPORT-PRIVACY.md (фраза «safe display settings», на которую ссылается §9) подтверждено чтением файлов.
  • Issue #498: перечитал тело и оба комментария (gh issue view 498) — после вердикта r1 (2026-09-09T06:42) новых комментариев от владельца нет, тело issue не редактировалось. Открытых продуктовых вопросов не появилось.

Закрытие раунда r1

Находка r1 Чем закрыта Где это видно
Medium 1 — нет раздела «AC1…ACn» с доказательством, нет явного «нет» по перф/touch (DoR, §2.5/§7.1) Добавлен раздел «## 8. Критерии приёмки», AC1–AC8, у каждого назван тест-доказательство; AC8 — явная строка «Производительность и touch не затронуты» docs/specs/498-...md:127-136 (AC1–AC8), AC8 = строка 136
Medium 2 — алгоритм обхода графа ссылок (§6) не гарантирует предел 64 при недоброжелательном порядке id (глубина стека вместо длины самой длинной цепочки через узел) §6 переписан: мемоизированная longest[node] (длина самой длинной цепочки от узла вниз) вместо булева visited; узел из longest даёт chain = max(chain, longest[ref]+1) вместо простого пропуска; в §7.3 добавлен тест с id n0065→…→n0001 (sorted стартует с хвоста); в §7.4 — шестой мутант svg-reference-depth-per-start-not-per-chain docs/specs/498-...md:84-93 (алгоритм), :113 (тест недоброжелательного порядка), :122 (мутант) — вручную проверено трассировкой на адверсариальном примере r1, см. «Как проверялось»
Low — §1.2 «После» смешивает пользовательский язык с HTTP-кодами/именами ошибок Переписано на язык результата без кодов/статусов docs/specs/498-...md:25 — «слишком большой», «отклоняется как некорректная», без too_large (413)/invalid_image (400)

Все находки r1 закрыты в этом раунде, включая необязательный Low.

Унаследовано из r1

Без повторной проверки принято (код не менялся, дельта этих участков не касалась):

  • Построчное подтверждение трёх дефектов B5/B6/B7 в текущем коде (plans.py:201-239, http_api.py:352-491, http_api.py:230-263, decor_assets.py:200-289) — SPEC-REVIEW-498-r1, раздел «Как проверялось», SHA 95493de2.
  • Совпадение SUPPORT_FILL_COLOR_KEYS (§5) с DEFAULT_FILL_COLORS из src/logic.ts:1412-1424 (11 из 11) — SPEC-REVIEW-498-r1, там же.
  • Совместимость сигнатур exclude как keyword-only с None по умолчанию — не ломает три существующих вызова dir_usage/check_quota — SPEC-REVIEW-498-r1, там же.
  • Персона и сценарий (§1.1) внутри docs/SCOPE.md (Home admin, поверхности upload/support/decor, job J4/J6) — SPEC-REVIEW-498-r1, раздел «Скоуп»/«Что проверено».
  • Не-скоуп (§3): ослабление лимитов, сужение схемы fill_colors, автоочистка чужих .upload-*, перенос staging, квота планов — всё обоснованно исключено — SPEC-REVIEW-498-r1.
  • Существование и корректность существующих тестов-регрессий (test_rich_plan_projection_preserves_safe_structure_and_drops_unknown_values, test_svg_rejects_the_whole_unsafe_document, test_svg_preserves_safe_local_gradient_clip_mask_and_transparency) — SPEC-REVIEW-498-r1.

Находки

Ни одной High или Medium в дельте r2 не найдено.

Low — нумерация разделов «## 8» / «## 8.0» после вставки AC-раздела

Дельта вставила «## 8. Критерии приёмки» перед прежним «## 8. Совместимость и откат», и вместо сдвига нумерации (9, 9.1, 9.2, 10, 11, 12) переименовала старый раздел в «## 8.0», оставив «## 8.1»/«## 8.2» как были. Раздел «8.0» после «8» — не соответствует обычной десятичной нумерации (стандартно 8.0 предшествовал бы 8.1, а не следовал за целым 8). Не блокирует: содержание всех разделов присутствует и однозначно, PROCESS.md §7.1 требует наличия перечисленных разделов, а не конкретного числового формата заголовков. Снимаю решением ревьюера без возврата на цикл — техническая правка нумерации тривиальна и не требует отдельного прохода; на усмотрение автора при следующей правке файла.

Что проверено и корректно

  • Оба Medium из r1 закрыты по существу, не декларативно: алгоритм проверен вручную на исходном контрпримере r1 и даёт правильный результат независимо от порядка обхода; AC-раздел — не косметическая обёртка, а точное сведение уже написанного контракта в проверяемый список с именами тестов, совпадающими с §7.
  • Попутно исправлена унаследованная из S2/r1 неточность — несуществующий путь эндпойнта /api/houseplan/decor/assets заменён на реальный /api/houseplan/assets/upload (сверено с кодом).
  • Новая формулировка семантики конкурентных загрузок (§4.2, AC2) — не догадка: подтверждена структурой реального кода (await-точка между стадией staging и вызовом check_quota делает сценарий «оба видят staged-файл друг друга» реальным, а не гипотетическим), и AC2/тест 7.1 сформулированы так, что не переобещают детерминизм, которого код не может дать.
  • Открытых продуктовых вопросов нет; владелец не оставлял новых комментариев после вердикта r1, тело issue не редактировалось.
  • Все шесть мутантов §7.4 имеют явного тест-свидетеля; новый мутант svg-reference-depth-per-start-not-per-chain ловится именно новым тестом на недоброжелательный порядок id — «естественный» тест (по возрастанию) его не поймал бы, как и предсказывал r1.

Чего не проверял

  • Гейты typecheck/test/build/pytest — не требуются на стадии spec-review, продуктовый код ещё не написан.
  • Построчную повторную сверку кода B5/B6/B7 — не менялась дельтой, код на дереве не менялся с r1, наследуется (см. «Унаследовано из r1»).
  • Флейкость/детерминизм самого asyncio.gather-теста для §7.1 (барьер на два вызова check_quota) — вопрос дизайна теста, а не ТЗ; вернётся на код-ревью, если тест окажется нестабильным.
  • Реальный прогон предложенных тестов — их ещё не существует (стадия spec), только проверка на непротиворечивость текста и совпадение имён между §7 и §8.

Материал раунда

  • Ветка: issue/498-backend-hardening-quota-palette-svg-refs
  • SHA: 2ed7bbe679ec06532359da87a3fa7dac9b898a7a
  • ТЗ: docs/specs/498-backend-hardening-quota-palette-svg-refs.md, blob 11929110b55c3a7ac3c1ba41469b82de3eeb88b8
  • Предыдущий раунд: docs/reviews/SPEC-REVIEW-498-r1.md, SHA 95493de2fcdbf4e27b91038de92ccebab42719ec

Вердикт

Зелёный. Оба Medium из r1 закрыты по существу (проверено пересчётом алгоритма на контрпримере и сверкой AC с тестами), High-находок нет. Найден один Low (нумерация «8»/«8.0»), снят решением ревьюера без возврата на цикл. ТЗ готово к переходу в «Готово к разработке» (S5).


Материал раунда

  • Ветка: issue/498-backend-hardening-quota-palette-svg-refs, коммит 2ed7bbe679ec — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
  • Дерево материала: 52ce02cf46032fac5a67d7fd8cb290af16a70503
    git log --all --format='%H %T' | grep 52ce02cf4603
    
  • ТЗ docs/specs/498-backend-hardening-quota-palette-svg-refs.md, блоб 11929110b55c3a7ac3c1ba41469b82de3eeb88b8
    git log --all --find-object=11929110b55c3a7ac3c1ba41469b82de3eeb88b8 -- docs/specs/498-backend-hardening-quota-palette-svg-refs.md
    
  • Вердикт конвейера: green · High 0