mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 20:29:00 +00:00
claude[bot]
@@ -0,0 +1,188 @@
|
||||
# SPEC-REVIEW-428-r1
|
||||
|
||||
Issue: [#428](https://github.com/Matysh/houseplan-card/issues/428) — «Экспорт с
|
||||
недостающей картинкой декора не импортируется — ImportFailure на весь документ».
|
||||
|
||||
Материал: тело issue #428, комментарии (аналитика + автор ТЗ), файл
|
||||
`docs/specs/428-missing-decor-asset-roundtrip.md` на коммите `85ba7a5f`
|
||||
(HEAD ветки `issue/428-missing-decor-asset-roundtrip`), исходный контракт
|
||||
`docs/specs/051-custom-decor-images.md` (AC10/AC11 и раздел «Import/export и
|
||||
совместимость»), текущий код `custom_components/houseplan/import_export.py`,
|
||||
существующий тест `tests_backend/test_ha_import_export.py`.
|
||||
|
||||
Заход: r1 (первый), правила §2.10 о разборе по дельте не применяются — разбор
|
||||
полный.
|
||||
|
||||
## Скоуп
|
||||
|
||||
Полный трек (метка `small` не выставлена; аналитика явно называет нарушенный
|
||||
критерий §5 — «сложность и риск ≤ 3» не выполнен, изменение затрагивает
|
||||
fail-closed границу export/import). ТЗ лежит в `docs/specs/`, как и требуется
|
||||
для не-`small` задачи. Задача — точечное ослабление одной проверки в
|
||||
`_content_state()`: строка `decor_asset` с `exists_at_export: false` и
|
||||
`mime: null` должна проходить импорт вместо `ImportFailure("invalid_content")`
|
||||
на весь документ.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
Проверка велась состязательно: не поверил на слово авторскому «подтверждено
|
||||
исполнением» из тела issue, а самостоятельно прочитал код и воспроизвёл вывод.
|
||||
|
||||
1. **Первопричина независимо подтверждена чтением кода**, не только текстом
|
||||
issue/ТЗ:
|
||||
- `content_manifest()` (`import_export.py:418-479`) — при отсутствии и blob,
|
||||
и `.json`-sidecar `metadata.get("mime")` пусто, `blob` равен `None`,
|
||||
`.get(blob.suffix if blob else "")` → `.get("")` → `None`. Значит
|
||||
`mime: null` в манифесте при `exists_at_export: false` — реальный, не
|
||||
гипотетический случай.
|
||||
- `_content_state()` (`import_export.py:1643-1649`) безусловно требует
|
||||
`declared.get("mime") in {"image/png", "image/jpeg", "image/webp",
|
||||
"image/svg+xml"}` для *любой* строки `decor_asset`, независимо от
|
||||
`exists_at_export`. При `mime: null` это всегда `ImportFailure`. Баг
|
||||
воспроизводится чтением, эквивалентен тому, что показал субагентский
|
||||
прогон автора.
|
||||
- Существующий тест `test_issue_51_missing_decor_asset_stays_as_repairable_geometry`
|
||||
(`tests_backend/test_ha_import_export.py:55-80`) действительно строит
|
||||
только случай «blob был у источника (`exists_at_export: True`, MIME
|
||||
известен из sidecar), отсутствует у target» — заявление ТЗ о непокрытом
|
||||
случае C подтверждено, тест не проверяет `mime: null`.
|
||||
2. **Источник контракта — не выдумка автора.** Сверил ссылку на ТЗ #51: AC11
|
||||
(«Import принимает v1/v2, fail-closed проверяет manifest… а после
|
||||
подтверждения сохраняет missing image placeholder») и раздел «Import/export
|
||||
и совместимость» (`051-custom-decor-images.md:339-361`) действительно
|
||||
объявляют missing-at-export легальным восстановимым состоянием с
|
||||
confirmation + repair-placeholder. Новое ТЗ не придумывает продуктовое
|
||||
поведение, а восстанавливает уже принятый контракт, который код нарушает.
|
||||
3. **Проверка регрессионной матрицы (AC5/AC6) на реализуемость.** Строка `row
|
||||
["exists_at_export"] = declared.get("exists_at_export")` в текущем коде
|
||||
вообще не проверяет тип поля — значит требование AC5/AC6 «строгий
|
||||
`type(x) is bool`» — это новая, а не восстанавливаемая проверка;
|
||||
она согласована с разделом «Риски» (`0`/`1` как под-класс `int`) и не
|
||||
конфликтует с уже существующими данными: JSON `true/false` парсится
|
||||
Python'ом только как `bool`, так что регресс для валидных прежних
|
||||
экспортов исключён.
|
||||
4. **Использование `mime` вне этой проверки.** Проверил, что декларированный в
|
||||
манифесте `mime` — не источник истины ни для чего, кроме этой валидации:
|
||||
`decor_assets.py` определяет и проверяет MIME отдельно, по фактическим
|
||||
байтам загруженного файла (`_validate_asset`, строки ~312-350), а
|
||||
`config`-запись decor-объекта вообще не хранит `mime` — только `asset_id`.
|
||||
Ослабление проверки поля `mime` в манифесте не открывает MIME-confusion:
|
||||
реальная доступность строки по-прежнему определяется пересчётом SHA-256 по
|
||||
байтам кандидата на target (`import_export.py:1650-1659`), а не
|
||||
декларацией источника. Раздел «Безопасность и privacy» ТЗ обоснован, не
|
||||
декларативен.
|
||||
5. Проверил соответствие процессу: аналитика правильно называет нарушенный
|
||||
критерий лёгкого трека; артефакт лежит по правильному пути
|
||||
`docs/specs/428-missing-decor-asset-roundtrip.md`; `docs/specs/README.md`
|
||||
получил строку с рабочей ссылкой; коммит `85ba7a5f` несёт `Issue: #428` и
|
||||
`User-Visible: no` — корректно для docs-only коммита ТЗ.
|
||||
6. Сверил обязательные разделы §7.1: сценарий, «что человек увидит до/после»
|
||||
(таблица), проблема («Подтверждённая причина»), скоуп/не-скоуп, контракт
|
||||
поведения («Контракт manifest и валидации»), совместимость/миграция,
|
||||
touch/i18n/perf, затронутые файлы, AC1–AC9 с доказательствами, план
|
||||
автотестов, риски, откат, release-артефакты, принятые предположения — все
|
||||
присутствуют по содержанию (раздел «UX» не выделен отдельным заголовком, но
|
||||
его содержание — «нового диалога, текста ошибки или элемента управления
|
||||
нет» — прямо сказано в тексте; см. находку Low ниже).
|
||||
|
||||
## Находки
|
||||
|
||||
### Low — формулировка граничного значения `mime: ""` в таблице раздела 2 неполна
|
||||
|
||||
Таблица «Допустимые строки при импорте» (раздел «Контракт manifest и
|
||||
валидации», п.2) описывает ветку `exists_at_export: false` тремя строками:
|
||||
«поддерживаемая строка» → допустимо; «отсутствует или `null`» → допустимо;
|
||||
«неподдерживаемая **непустая** строка либо значение другого типа» →
|
||||
`ImportFailure`. Пустая строка `mime: ""` не входит буквально ни в одну из
|
||||
трёх формулировок: она не «отсутствует или `null`», но и не «непустая».
|
||||
|
||||
Проверил, ломает ли это реализуемость: естественная реализация из кода
|
||||
(`declared.get("mime") not in SUPPORTED and declared.get("mime") is not
|
||||
None` при `exists_at_export is False`) отклоняет `""` тем же путём, что и
|
||||
любую другую неподдерживаемую строку — то есть содержательного разночтения в
|
||||
поведении нет, реализация детерминирована. Дефект чисто в формулировке
|
||||
таблицы («непустая» лишнее слово), не в контракте. Снимаю находку как
|
||||
**Low, не блокирует**: замечание оставлено с записью для точности документа,
|
||||
править не обязательно, так как план автотестов (п.5, «параметризовать
|
||||
`exists_at_export` и `mime` по таблице») в любом случае может включить `""`
|
||||
как один из «unsupported non-null MIME» без противоречия итоговому коду.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- Первопричина бага реальна и подтверждена независимо (не только доверием к
|
||||
тексту автора) — см. «Как проверялось» п.1.
|
||||
- Контракт-источник (#51 AC11) реален, процитирован точно, новое ТЗ его не
|
||||
меняет, а восстанавливает.
|
||||
- Скоуп узкий и не расширяется: не задета `EXPORT_FORMAT_VERSION`,
|
||||
config/model schema, UI подтверждения, upload/delete/replace, frontend —
|
||||
всё явно перечислено в «Не-скоуп» и это согласуется с «Затронутые файлы»
|
||||
(только backend + backend-тесты + доки + changelog).
|
||||
- Таблица допустимых/недопустимых значений (раздел 2) в остальном
|
||||
исчерпывающая и корректно закрывает найденный класс уязвимости («risk 1» —
|
||||
«слишком широкое ослабление manifest») точной формулировкой инвариантов,
|
||||
которые остаются обязательными (exact `asset_id`/`hash`, identity полей,
|
||||
повторная проверка target blob по байтам).
|
||||
- AC1–AC9 пронумерованы, каждый с указанным способом доказательства (backend
|
||||
/ docs gate / ревью кода / commands + Linux CI), формулировки однозначны,
|
||||
не пересекаются по ответственности.
|
||||
- Риски названы предметно (широкое ослабление, `bool`/`int` в Python,
|
||||
supplied-metadata как authority, helper vs настоящий export/import) и у
|
||||
каждого явно назван снимающий его механизм в контракте/AC.
|
||||
- «Принятые предположения» оформлены как предположения, а не факты, и
|
||||
ревьюер с ними согласен по итогам независимой проверки кода — не
|
||||
гадание, выданное за решение.
|
||||
- Откат описан и достаточен (revert коммита, без миграции данных).
|
||||
- Track/процесс: причина полного трека названа явно (критерий §5 не
|
||||
выполнен), путь артефактов и трейлер коммита ТЗ соответствуют PROCESS.md.
|
||||
- Продуктовых вопросов владельцу в ТЗ нет — обоснованно: видимое поведение
|
||||
уже зафиксировано принятым контрактом #51, разбираемый вопрос был
|
||||
технический (валидация fail-closed границы) и решён автором, а не вынесен.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Реализация ещё не написана (стадия ТЗ) — код-ревью, автотесты и прогон
|
||||
гейтов (`typecheck`/`test`/`build`/backend pytest) не в скоупе этого этапа
|
||||
и будут выполнены на код-ревью по факту диффа.
|
||||
- Не проверял golden/smoke/performance — задача не трогает frontend/визуал
|
||||
(сама ТЗ явно это утверждает и обоснование подтверждено чтением: правки
|
||||
ограничены `custom_components/houseplan/import_export.py` и бэкенд-тестами).
|
||||
- Не проверял точный будущий текст правок `docs/USER-GUIDE.md`,
|
||||
`docs/USER-GUIDE.ru.md`, `docs/CONFIG-COMPATIBILITY.md` и changelog — они
|
||||
ещё не написаны; AC8 корректно называет их обязательными и привязывает к
|
||||
тому же `User-Visible: yes` коммиту, этого на этапе ТЗ достаточно.
|
||||
- Не проверял поведение с `space_id`/single-space export код-путём построчно
|
||||
за пределами того, что нужно для оценки AC3 (проверил только то, что
|
||||
`content_manifest()` вызывается на уже спроецированный `config` во всех
|
||||
трёх режимах, включая `plan_only`, и что `asset_id` не выпадает из
|
||||
plan-only проекции decor-объекта).
|
||||
|
||||
## Вердикт
|
||||
|
||||
Зелёный. High: 0. Medium: 0. Единственная находка — Low, снята с запиской
|
||||
(см. выше), автор ничего чинить не обязан.
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- SHA материала: `85ba7a5f304f9121815b947c9234c545bfaad65e`
|
||||
(`origin/issue/428-missing-decor-asset-roundtrip`, идентичен HEAD на момент
|
||||
ревью).
|
||||
- Дерево: `docs/specs/428-missing-decor-asset-roundtrip.md`,
|
||||
`docs/specs/README.md`.
|
||||
- Поиск при необходимости: `git log --all --format='%H %T' | grep <дерево>`;
|
||||
`git log --all --find-object=<блоб> -- docs/specs/428-missing-decor-asset-roundtrip.md`.
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/428-missing-decor-asset-roundtrip`, коммит `85ba7a5f304f` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `413e5407d2d52959155b011df38118f15fc31c0c`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 413e5407d2d5
|
||||
```
|
||||
- ТЗ `docs/specs/428-missing-decor-asset-roundtrip.md`, блоб `9271c0a82f4072229c39c408330cb1dfb9ca4fe2`
|
||||
```
|
||||
git log --all --find-object=9271c0a82f4072229c39c408330cb1dfb9ca4fe2 -- docs/specs/428-missing-decor-asset-roundtrip.md
|
||||
```
|
||||
Reference in New Issue
Block a user