mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-03 21:28:59 +00:00
claude[bot]
@@ -0,0 +1,127 @@
|
||||
# SPEC-REVIEW-498-r1
|
||||
|
||||
- **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 `95493de2fcdbf4e27b91038de92ccebab42719ec` (единственный коммит поверх аналитики; тело issue не редактировалось после S2)
|
||||
- **Этап:** ТЗ на ревью (PROCESS.md §2.4), заход r1, блокирующих циклов израсходовано 0 из 4
|
||||
- **Трек:** полный (аналитик назвал нарушенный критерий §5 «одна поверхность» — три независимых модуля; корректно)
|
||||
|
||||
## Скоуп
|
||||
|
||||
Три независимых защитных дефекта из аудита 2026-09-08:
|
||||
|
||||
- **B5** — `check_quota` дважды считает собственный staged-файл (`.upload-*` уже лежит в `files_root`, `dir_usage` его находит, `incoming` прибавляется поверх).
|
||||
- **B6** — проекция support-пакета копирует любые строковые ключи `fill_colors`, а не только 11 ключей, которые читает карточка.
|
||||
- **B7** — обход графа ссылок SVG (`href`/`url(#…)`) рекурсивен, плоская цепь длиной ~2500 роняет `RecursionError`, наружу уходит 500 вместо кода отказа.
|
||||
|
||||
Ревью ТЗ проверяет: обязательные разделы §7.1 PROCESS.md, однозначность и доказуемость AC, отсутствие выданных за факт догадок, соответствие `docs/SCOPE.md` (не проверяю за отсутствием конфликта — задача чинит существующие защитные гейты upload/support/decor, все три поверхности внутри уже принятого функционала J4/J6, не расширяет продукт).
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Материал — только текст ТЗ и тело/комментарии issue; это стадия spec-review, гейты (`typecheck`/`test`/`build`) к ней не относятся (они гоняются на код-ревью) и не запускались. Вместо этого каждое утверждение ТЗ о текущем коде сверено чтением фактического дерева на `95493de2`:
|
||||
|
||||
- `custom_components/houseplan/plans.py:201-239` (`dir_usage`, `check_quota`) — арифметика двойного счёта подтверждена построчно, совпадает с описанием §1/§4.
|
||||
- `custom_components/houseplan/http_api.py:352-491` (`HouseplanUploadView.post`) — подтверждено: `tmp_path` создаётся в `files_root` до вызова `check_quota`, `check_quota` вызывается с телом временного файла как `incoming` без исключения. `tmp_path` доступен в области видимости для `exclude=tmp_path`, как предлагает ТЗ.
|
||||
- `custom_components/houseplan/http_api.py:230-263` — подтверждено: `except DecorAssetError` — единственный перехват, `RecursionError` (подкласс `RuntimeError`, не `DecorAssetError`) уйдёт как необработанное исключение → 500. Совпадает с §1 B7.
|
||||
- `custom_components/houseplan/decor_assets.py:200-289` (`_validate_svg`, `_visit`) — подтверждена рекурсивная реализация цикл-детектора, семантика `visiting`/`visited`, регэкспы `href`/`url()`, существующие лимиты `MAX_SVG_ELEMENTS=5000`, `MAX_SVG_DEPTH=64` (глубина XML-дерева, не графа ссылок — предлагаемый `MAX_SVG_REF_DEPTH` действительно независимая величина, как и написано в §11).
|
||||
- `custom_components/houseplan/support_package.py:96-152` (`_copy_keys`, `_global_settings`, `_project_value_badge`) — подтверждено: `fill_colors` копируется по всем строковым ключам без allowlist; паттерн «пустое опускается» (`out or None`) действительно уже используется в `_project_value_badge`, так что ссылка на прецедент в §5 корректна.
|
||||
- `src/logic.ts:1412-1424` (`DEFAULT_FILL_COLORS`) — сверил построчно с константой `SUPPORT_FILL_COLOR_KEYS` из §5 ТЗ: `light_on, light_off, light_none, temp_cold, temp_ok, temp_hot, lqi_low, lqi_high, glow_base, glow_light, wall_fill` — 11 из 11, полное совпадение, порядок не важен. Предложенный тест «сверка с `src/logic.ts`» по образцу `_ts_list` (`tests_backend/test_validation.py:909`) — паттерн действительно существует и применим (хотя `DEFAULT_FILL_COLORS` объектный литерал, а не массив, так что регэксп будет другим — это техническая деталь, не входит в компетенцию владельца).
|
||||
- `tests_backend/test_support_package.py:213-227` — существующий тест действительно закрепляет пропуск произвольного ключа `"warm"`, как написано в §1/§7.2.
|
||||
- `tests_backend/test_decor_assets.py:104,156` — оба названных существующих теста (`test_svg_rejects_the_whole_unsafe_document`, `test_svg_preserves_safe_local_gradient_clip_mask_and_transparency`) существуют под указанными именами.
|
||||
- Другие вызовы `dir_usage`/`check_quota` (`tests_backend/test_validation.py:1530-1531`, `test_ha_websocket.py:3665`, `websocket_api.py` для `ws_plan_upload`) — единственные, все вызывают без `exclude`; добавление `exclude` как keyword-only с默认 `None` не ломает ни один. `Не-скоуп` про `ws_plan_upload` корректен (считает `len(raw)` до записи, `dir_usage` не вызывает).
|
||||
|
||||
Продуктовый код не менялся, мутантов и тестов ещё нет — это ожидаемо для стадии spec.
|
||||
|
||||
## Находки
|
||||
|
||||
### Medium (в скоупе задачи) — обязательный раздел «AC1…ACn» отсутствует, часть пунктов DoR не названа явно
|
||||
|
||||
PROCESS.md §7.1 перечисляет обязательные разделы ТЗ, включая «критерии приёмки AC1…ACn с указанием доказательства», и §2.5 (DoR) требует по каждому AC явно назвать способ доказательства (`unit`/`backend`/`smoke`/`golden`/«ревью кода»), а также «влияние на производительность и бюджеты названо (или явно "нет")» и «влияние на touch… (View и киоск — блокирующие)».
|
||||
|
||||
В документе `docs/specs/498-*.md` таких разделов нет: `grep -n "AC[0-9]"` по файлу не находит ни одного вхождения. Есть скоуп-пункты (§2.1-4), граничные условия (§4.2) и список тестов (§7.1-7.4), из которых AC можно собрать вручную, но они не сведены в пронумерованный список «AC → доказательство», как того требует DoR-чеклист — а именно по немуissue переводится в `S5-ready`. Аналогично нет ни одной явной строки о влиянии на производительность или touch (в этой задаче они действительно нулевые — чистый backend без UI, — но чеклист требует **явного** «нет», а не отсутствия упоминания).
|
||||
|
||||
**Почему это не Low:** без этого раздела формально нельзя перевести issue в `S5-ready` по букве DoR-чеклиста — не хватает одного из обязательных пунктов, а не стилистической мелочи.
|
||||
|
||||
**Как чинится:** свести существующее содержание §2/§4.2/§5/§6/§7 в явный список, например:
|
||||
- AC1: `check_quota` не считает свой staged-файл дважды — `backend`, `test_validation.py::test_issue_498_check_quota_excludes_the_staged_upload_itself`.
|
||||
- AC2: endpoint принимает файл/файл-по-счёту ровно на границе квоты — `backend`, `test_ha_upload.py::test_issue_498_upload_accepts_the_last_bytes_and_the_last_file_of_the_quota`.
|
||||
- AC3: параллельные загрузки по-прежнему учитывают друг друга — `backend`, `test_ha_upload.py::test_issue_498_concurrent_uploads_still_count_each_other`.
|
||||
- AC4: support-пакет переносит только 11 ключей палитры продукта — `backend`, `test_support_package.py::test_rich_plan_projection_preserves_safe_structure_and_drops_unknown_values` (обновлённый) + `test_issue_498_palette_allowlist_matches_the_card_defaults`.
|
||||
- AC5: пустая палитра не пишет ключ `fill_colors` — `backend`, `test_issue_498_projection_omits_an_empty_palette`.
|
||||
- AC6: цепочка ссылок SVG обрабатывается итеративно и ограничена 64 без `RecursionError` — `backend`, `test_decor_assets.py::test_issue_498_flat_reference_chain_is_bounded_not_recursive` + endpoint-тест на 413.
|
||||
- AC7: цикл ссылок по-прежнему `invalid_image` (регрессия) — `backend`, тот же тест.
|
||||
- Плюс строка «Производительность: нет влияния (чистая замена алгоритма на локальных данных, без изменения форматов)» и «Touch: не применимо (backend, без UI)».
|
||||
|
||||
Это техническая перестановка уже написанного текста, продуктовых вопросов не порождает — фиксится автором в этом же цикле.
|
||||
|
||||
### Medium (в скоупе задачи) — предложенный алгоритм обхода графа ссылок (§6) не гарантирует заявленный предел глубины при недоброжелательном именовании id
|
||||
|
||||
§6 предписывает: «для каждого `node_id in sorted(ids)` — явный стек, множества `visiting`/`visited`… узел из `visited` пропускается». Узел, уже находящийся в `visited`, обрывает traversal без учёта его собственной глубины — это делает измеренную «глубину стека» зависимой от того, **в каком порядке `sorted(ids)` начинает обходить компоненты**, а не от истинной длины самого длинного пути в графе ссылок.
|
||||
|
||||
**Конкретное воспроизведение** (уменьшенный пример, лимит = 2 вместо 64, но конструкция линейно масштабируется на реальные значения 64/2500 из ТЗ):
|
||||
|
||||
Цепочка `v0 → v1 → v2 → v3` (длина пути 4, что вдвое больше лимита 2), но id даны так, что `sorted()` посещает их в порядке `v2, v0, v1, v3`:
|
||||
|
||||
```
|
||||
id("v2") = "a" id("v0") = "b" id("v1") = "c" id("v3") = "d"
|
||||
ref_graph: a→d, b→c, c→a (то есть исходно v0→v1→v2→v3)
|
||||
```
|
||||
|
||||
Обход `sorted(ids) = ["a","b","c","d"]`:
|
||||
1. `_visit("a")`: стек `a`(depth1)→`d`(depth2, лимит не превышен, у `d` нет исходящих) → оба помечены `visited`. Максимальная зафиксированная глубина = 2.
|
||||
2. `_visit("b")`: стек `b`(depth1)→`c`(depth2, лимит не превышен) → ref `c→a`, но `a` уже в `visited` → **пропускается без учёта её собственной глубины 2** → `b`,`c` помечены `visited`. Максимальная зафиксированная глубина = 2.
|
||||
|
||||
Ни разу условие «глубина стека > лимит» не сработало, хотя истинный путь `b→c→a→d` имеет длину 4 — вдвое больше лимита. При реальных значениях (лимит 64) достаточно нарезать любую сколь угодно длинную плоскую цепочку на сегменты по ≤64 узлов и присвоить id так, чтобы `sorted()` посещал сегменты от конца цепи к началу — весь граф пройдёт проверку `too_large`, невзирая на фактическую длину. Атрибут `id` подчиняется только формальному регэкспу (`decor_assets.py:265`), содержательных ограничений на него нет — конструирование такого имени полностью в руках того, кто формирует SVG.
|
||||
|
||||
**Почему это не High.** Вход writer-only (сам автор ТЗ это фиксирует в §1 и §8.2), обхода `RecursionError`/500 это не создаёт (переход на явный стек сам по себе полностью убирает исходный дефект B7 независимо от порядка обхода — падать нечему). Обойти можно только **дополнительный** лимит глубины цепочки, а обойти его может лишь тот же человек, кто сам загружает decor-ассет в свою инсталляцию — эксплуатировать через границу пользователей нечего. Тем не менее AC §1.2 буквально обещает «цепочка глубже 64 отклоняется», и предписанный алгоритм этого не гарантирует — а тест, построенный «естественно» (id вида `g0, g1, …, g2499`, как в примере ТЗ), никогда не вскроет проблему, потому что `sorted(["g0","g1",…])` в лексикографическом порядке случайно совпадает с топологическим и стартует с настоящей головы цепи.
|
||||
|
||||
**Как чинится (без продуктовых вопросов, чисто техническое решение):** заменить проверку «глубина стека при обходе» на мемоизированную длину самого длинного пути от узла (`depth[node] = 1 + max(depth[ref] for ref in children)`, вычисляется один раз при первом посещении и переиспользуется, а не просто булев `visited`) — стандартный «longest path in DAG» поверх уже имеющегося цикл-детектора `visiting`. Плюс добавить в §7.3 один тест с недоброжелательно упорядоченными id (как в примере выше, в масштабе 64/128), доказывающий, что предложенный тест `test_issue_498_flat_reference_chain_is_bounded_not_recursive` с «естественными» именами не покрывает этот случай сам по себе.
|
||||
|
||||
### Low — «После» в §1.2 смешивает пользовательский язык с деталями реализации
|
||||
|
||||
AGENTS.md требует раздел «что человек увидит» одной фразой, без терминов реализации. §1.2 содержит «отклоняется кодом `too_large` (413)» и «c`invalid_image` (400)» — это HTTP-статусы и внутренние коды ответа, а не то, что видит редактор на экране (карточка показывает переведённый тост, см. `src/decor-image-editor.ts:154`, `too_large → backdrop.too_large_title`). Не блокирует — по существу поведение не искажено, реального нового пользовательского текста ТЗ не придумывает (переиспользуется существующий код `too_large` и его существующий перевод), только стиль изложения. Оставляю на усмотрение автора без отдельного цикла.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Все три описанных дефекта (B5/B6/B7) подтверждены построчным чтением кода на материале ревью — не переоценка аудита на устаревшем SHA.
|
||||
- Константа `SUPPORT_FILL_COLOR_KEYS` (§5) совпадает 1:1 с `DEFAULT_FILL_COLORS` в `src/logic.ts` — не догадка, а точное соответствие.
|
||||
- Не-скоуп корректен: `ws_plan_upload`, численные лимиты, схема `fill_colors`, автоочистка чужих `.upload-*`, перенос staging — все обоснованно исключены и не открывают продуктовых вопросов.
|
||||
- Совместимость сигнатур (`exclude` как keyword-only с `None` по умолчанию) не ломает три существующих вызова `dir_usage`/`check_quota`.
|
||||
- Персона и сценарий (§1.1) определены и совпадают с `docs/SCOPE.md` (Home admin, редактор/владелец, поверхности upload/support/decor).
|
||||
- Продуктовых догадок, выданных за факт, не найдено — единственное найденное несоответствие (обход графа ссылок) относится к техническому решению автора, а не к продуктовой неопределённости, и разрешается вердиктом, а не владельцем.
|
||||
- Откат (revert одного коммита), release-артефакты, риски — присутствуют и по существу верны.
|
||||
- Задача внутри `docs/SCOPE.md`: чинит существующие защитные гейты в рамках уже принятых поверхностей (upload/support/decor), не расширяет продукт — конфликта нет.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Гейты `typecheck`/`test`/`build`/`pytest` не запускал — стадия spec-review их не требует (продуктовый код ещё не написан), они относятся к код-ревью.
|
||||
- Не проверял `docs/CONFIG-COMPATIBILITY.md` и `docs/TOUCH-SUPPORT.md` построчно — задача явно не трогает конфиг-схему и не имеет UI/touch-поверхности, что видно из диапазона затронутых файлов (только Python backend + его тесты); при появлении в дельте следующего раунда чего-либо, касающегося конфига или UI, потребуется отдельная сверка.
|
||||
- Не проверял производительность обхода (объём это дешёвая операция на ≤5000 узлах) — задача сама не заявляет влияния на перф-бюджеты, и после исправления Medium-находки выше это должно быть явно зафиксировано автором, а не мной.
|
||||
- Не проверял корректность работы `asyncio.gather`-теста для конкурентных загрузок (§7.1) на предмет детерминизма/флейкости — это вопрос дизайна теста, а не ТЗ, и вернётся на код-ревью, если тест окажется нестабильным.
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/498-backend-hardening-quota-palette-svg-refs`
|
||||
- SHA: `95493de2fcdbf4e27b91038de92ccebab42719ec`
|
||||
- ТЗ: `docs/specs/498-backend-hardening-quota-palette-svg-refs.md` (единственная редакция на момент ревью)
|
||||
|
||||
## Вердикт
|
||||
|
||||
**Жёлтый.** High-находок нет, найдено 2 Medium в скоупе задачи (недостающий формальный раздел AC/DoR-пунктов; несостоятельный при недоброжелательном именовании id алгоритм ограничения глубины цепочки ссылок SVG) — обе чинятся автором в текущей задаче без нового issue. Заход r1, цикл израсходован (первое возвращение с блокирующими для перехода в `S5-ready` находками).
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/498-backend-hardening-quota-palette-svg-refs`, коммит `95493de2fcdb` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `7840be23823e5e1d1da310d66948225ddf7d4812`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 7840be23823e
|
||||
```
|
||||
- ТЗ `docs/specs/498-backend-hardening-quota-palette-svg-refs.md`, блоб `a86281fe06197278ffeda08bfc271be8ff70b496`
|
||||
```
|
||||
git log --all --find-object=a86281fe06197278ffeda08bfc271be8ff70b496 -- docs/specs/498-backend-hardening-quota-palette-svg-refs.md
|
||||
```
|
||||
- Вердикт конвейера: `yellow` · High 0
|
||||
Reference in New Issue
Block a user