diff --git a/docs/reviews/SPEC-REVIEW-428-r1.md b/docs/reviews/SPEC-REVIEW-428-r1.md new file mode 100644 index 00000000..a330dbc2 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-428-r1.md @@ -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`. + +--- + + + +## Материал раунда + +- Ветка: `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 + ```