docs: review document for #225

Issue: #225
User-Visible: no

docs/reviews/SPEC-REVIEW-225-r1.md

@@ -0,0 +1,253 @@
+# SPEC-REVIEW — issue #225, цикл r1
+
+- **Issue:** https://github.com/Matysh/houseplan-card/issues/225
+- **ТЗ:** тело issue #225 (лёгкий трек `small`, файл в `docs/specs/` не
+  создаётся согласно §5 PROCESS.md)
+- **Трек:** `small` — сложность 2/10, одна поверхность
+  (`custom_components/houseplan/import_export.py`), лимит циклов ревью ТЗ — 2
+- **Ревьюер:** Claude (роль «Ревьюер ТЗ», отдельная сессия от автора)
+- **Вердикт:** жёлтый · цикл r1/2 · High: 0 · Medium: 1 → в задаче
+
+## Скоуп проверки
+
+Issue #225 — баг: собственный экспорт House Plan (PDF-вложения с
+кэш-бастером `?v=<timestamp>` в URL) не проходит собственный импорт,
+падая на `invalid_content`. ТЗ в теле issue (лёгкий трек) описывает фикс
+`_internal_path` (`import_export.py:350`): разбирать URL через
+`urlsplit`, игнорируя `query`/`fragment` при резолвинге пути к файлу.
+
+Проверялись:
+
+1. критерии лёгкого трека (§5 PROCESS.md) — действительно ли применимы;
+2. соответствие описания бага фактическому состоянию кода
+   (`_internal_path`, `_looks_internal`, `_content_state`,
+   `content_manifest`, `sanitize_filename`, `_SAFE_NAME_RE`);
+3. однозначность и доказуемость каждого AC1…AC7, включая перечисленные
+   в AC4 конкретные traversal-полезные нагрузки;
+4. отсутствие догадок, выданных за факт, без пометки «предположение»;
+5. корректность решения не эскалировать вопрос об error-message
+   владельцу (уже решено на этапе аналитики тем же лицом).
+
+Это первый цикл, раздел «объём по дельте» (§2.10 PROCESS.md) не
+применяется — разбор выполнен полностью.
+
+## Как проверялось
+
+- Прочитаны `docs/SCOPE.md` (J6 «Keep the plan true as the home
+  evolves» — перенос/восстановление конфигурации), `AGENTS.md`,
+  `PROCESS.md` целиком (§2.3–2.4, §5, §7.1, §7.2, §12).
+- Прочитано тело issue #225 полностью и оба комментария (аналитика с
+  оценками и меткой `small`, публикация ТЗ в теле issue). Метка на
+  момент ревью — `S4-spec-review`.
+- Прочитан `custom_components/houseplan/import_export.py`:
+  - `_internal_path` (350-370) — подтверждено: ветка `files/` режет
+    `url[len(prefix):]` по `/` **без** отделения query, `tail` включает
+    `?v=...` как часть последнего сегмента;
+  - `_looks_internal` (373-377) — подтверждено: смотрит только на
+    префикс, не видит query;
+  - `content_manifest` (380-405) — подтверждено: `storage: "internal"
+    if internal else "external"`, ровно как описано в ТЗ;
+  - `_content_state` (1021-1071) — подтверждено: строка 1060
+    `if _looks_internal(item["url"]) and internal is None: raise
+    ImportFailure("invalid_content", ...)` — воспроизводит механизм
+    отказа дословно; `identity()` (1034-1039) не включает `storage` в
+    ключ сравнения — подтверждает риск ТЗ про сохранение совместимости
+    по AC6;
+  - `_validate_plan_only_document` (685-712), в частности строка 704
+    (`placement_manifest`) — подтверждено, что классификация
+    `storage` не участвует в её проверках, только `owner == "space"` —
+    AC7 обоснован.
+- Прочитан `custom_components/houseplan/validation.py`:
+  `_SAFE_NAME_RE = [^A-Za-z0-9._-]+`, `sanitize_filename`,
+  `sanitize_marker_id` — подтверждено, что `?`/`=` заменяются на `_`,
+  из-за чего сравнение `name == tail[-1]` не проходит на суффиксе
+  `?v=...`.
+- Прочитан `custom_components/houseplan/http_api.py:290-316` — подтверждено,
+  что текущая ручка загрузки возвращает URL без query
+  (`f"{CONTENT_URL}/files/{marker_id}/{name}"`), то есть суффикс —
+  исторические данные, а не то, что генерирует текущий код.
+- Прочитан `src/logic.ts:1841-1861` (`migratePdfUrls`) — подтверждено,
+  что фронт уже явно отделяет `query` от `name` при переносе вложений;
+  довод ТЗ «контракт на фронте уже есть, бэкенд не в курсе» подтверждён
+  чтением, а не принят на слово.
+- Проверено существование прецедента для AC2/AC3 —
+  `tests_backend/test_ha_import_export.py:1439-1456`
+  (`test_apply_rechecks_attachment_under_the_write_lock`) использует
+  канонический URL без query; ТЗ верно называет его образцом для новой
+  фикстуры.
+- Проверено существование теста на внешний URL для AC5 —
+  `tests_backend/test_ha_import_export.py:366,459` использует
+  `https://example.invalid/floor.svg` — существующий тест, изменения не
+  требует.
+- Пройдена по шагам вычислением (без исполнения кода) каждая из четырёх
+  строк AC4 против описанного контракта (`urlsplit(url).path`, query и
+  fragment исключены из резолвинга) — см. находку M1 ниже.
+- Гейты кода не запускались: предмет ревью — ТЗ, реализации ещё нет
+  (`S4-spec-review`), код не менялся.
+
+## Находки
+
+### Medium — в скоупе задачи
+
+**M1. Третий пример в AC4 требует поведения, которое прямо противоречит
+описанному в этом же ТЗ контракту резолвинга — критерий недоказуем без
+нарушения собственного контракта.**
+
+AC4 перечисляет четыре traversal-нагрузки и требует, чтобы все четыре
+резолвились в `None`:
+
+> `…/files/../../secret.pdf?v=1`, `…/files/m1/../../secret.pdf`,
+> `…/files/m1/doc.pdf?x=/../../etc`, `…/plans/_/../x.svg?v=1`
+
+Контракт поведения этого же ТЗ формулирует фикс так: «путь для
+резолвинга берётся из `urlsplit(url).path`, `query` и `fragment` в
+определении файла не участвуют».
+
+Проверка построчно:
+
+- пример 1 (`…/files/../../secret.pdf?v=1`): `path` после отсечения
+  query = `.../files/../../secret.pdf`; хвост после префикса —
+  `../../secret.pdf`, при разбиении по `/` даёт 3 сегмента → уже сейчас,
+  до всякого фикса, возвращает `None` (проверка `len(tail) != 2`,
+  `import_export.py:364`). Легитимный regression-тест, поведение верно
+  предсказано.
+- пример 2 (`…/files/m1/../../secret.pdf`): хвост `m1/../../secret.pdf`
+  → 4 сегмента → `None`. Тоже легитимно и уже работает без фикса.
+- пример 4 (`…/plans/_/../x.svg?v=1`): `path` без query = `.../plans/_/../x.svg`;
+  `raw_name` после префикса — `../x.svg`, содержит `/` → ветка `plans`
+  (`import_export.py:355`) уже сегодня возвращает `None` по проверке
+  `"/" in raw_name`. Легитимно.
+- **пример 3 (`…/files/m1/doc.pdf?x=/../../etc`): по заявленному
+  контракту `query` не участвует в резолвинге пути. `urlsplit(url).path`
+  для этой строки — `.../files/m1/doc.pdf`, то есть чистый, без единой
+  точки `..`. Хвост после префикса — `m1/doc.pdf`, ровно 2 сегмента,
+  `marker="m1"`, `name="doc.pdf"` — оба проходят `sanitize_*` без
+  изменений. Корректная по контракту реализация обязана вернуть
+  `("attachment", root/FILES_DIR/"m1"/"doc.pdf")`, а не `None`.**
+
+Иначе говоря: содержимое `query` в этом примере физически не может
+повлиять на путь к файлу — «`../../etc`» лежит внутри значения
+параметра `x`, а не в пути. Требование «резолвится в `None`» либо
+недостижимо без дополнительной, нигде в ТЗ не описанной проверки
+(«отклонять `..` при обнаружении где угодно в исходной строке URL, а не
+только в пути») — тогда это новый пункт контракта, а не следствие уже
+заявленного; либо AC написан по ошибке и пример перепутан местами:
+скорее всего он должен был доказывать *обратное* — что «мусорный»
+traversal-паттерн в `query` безопасен именно потому, что резолвинг его
+игнорирует, и годился бы как положительный кейс в AC1, а не как
+негативный в AC4.
+
+**Почему это Medium, а не Low.** Это не стилистическая неточность:
+разработчик, который напишет тест по этому пункту буквально, либо
+получит красный тест на корректной реализации (совпадающей с
+контрактом ТЗ), либо втихую ослабит заявленный контракт («`query` не
+участвует»), добавив непрописанную проверку — оба исхода нежелательны,
+и оба возникают из-за противоречия внутри одного документа. Именно
+такую непроверяемость обязано ловить ревью ТЗ (§7.1 PROCESS.md:
+«Ambiguity is asked, not guessed»).
+
+**Фикс, ожидаемый в этом же цикле:** одно из двух — либо убрать третий
+пример из AC4 и перенести его (в виде «резолвится корректно, `query` с
+`../../` внутри не создаёт traversal и не бьёт по AC1») в AC1 как ещё
+один параметр, подтверждающий, что мусор в `query` безопасен именно
+потому что игнорируется; либо, если автор действительно имел в виду
+доп. defense-in-depth проверку по всей исходной строке — явно дописать
+её в «Контракт поведения» и объяснить, зачем она нужна при том, что
+`query` и так не участвует в резолвинге. Само по себе это правка на
+две строки текста ТЗ, не код.
+
+### Low
+
+Не найдено находок, которые стоило бы фиксировать отдельно и не
+чинить. (См. отдельно нижеследующее наблюдение по AC1 — оно снимается
+ревьюером с записью, не заводится как Low-находка.)
+
+## Что проверено и корректно
+
+- Обязательные для лёгкого трека элементы шаблона (§5 PROCESS.md)
+  присутствуют и по существу: проблема (с воспроизведением и точными
+  номерами строк), контракт поведения, AC1…AC7 каждый с указанием
+  доказательства (`backend`), план автотестов, риски, откат. ТЗ также
+  добавляет сверх минимума сценарий/персону, «что человек увидит»,
+  «не в скоупе», модель данных/миграция/i18n и блок явных технических
+  предположений — то есть избыточно полно для лёгкого трека, а не
+  урезано.
+- Критерии лёгкого трека (§5) выполнены одновременно: сложность 2/10,
+  одна поверхность (`import_export.py`), нет миграции конфига и
+  compatibility-полей, нет нового UX-контракта (импорт собственного
+  бэкапа — уже описанное поведение, меняется только исход), нет влияния
+  на perf/touch. Трек выбран верно.
+- Диагноз бага воспроизведён чтением кода, а не принят на слово автора
+  — см. «Как проверялось»: `_internal_path`/`_looks_internal` реально
+  расходятся именно так, как описано, `_SAFE_NAME_RE` реально ломает
+  сравнение на `?`/`=`.
+- AC1, AC2, AC5, AC6, AC7 однозначны и доказуемы: у каждого понятный
+  вход/выход и существующая или явно описанная новая точка проверки.
+  AC3 корректно указывает на существующий прецедент фикстуры
+  (`test_apply_rechecks_attachment_under_the_write_lock`) как образец,
+  без которого регрессия дожила до боевого конфига.
+- Риск «резолвер стоит на пути защиты от traversal» назван явно и
+  вынесен в отдельный AC (AC4), а не спрятан в примечание — правильная
+  структура даже с учётом находки M1 (ошибка в одном из четырёх
+  примеров, не в самом принципе выделения traversal в отдельный
+  критерий).
+- Риск смены классификации `content_manifest` (`external` → `internal`)
+  проверен по коду: `identity()` в `_content_state` (`:1034-1039`)
+  действительно не использует `storage` в ключе сравнения — совместимость
+  со старыми экспортами реально сохраняется, а не заявлена без
+  проверки.
+- Решение не выносить текст ошибки `invalid_content` пользователю в
+  скоуп — уже принято владельцем на этапе аналитики (тот же аккаунт,
+  комментарий S2-analysis) как сужение с обоснованием «вторая
+  поверхность, потеря лёгкого трека»; ревью с этим решением не спорит,
+  так как это ровно тот класс продуктового вопроса, который решает
+  владелец, а не автор ТЗ задним числом.
+- Блок «Принято предположительно» — реальные технические допущения
+  (выбор `urlsplit`, отказ от нормализации URL в сторону сохранения
+  как есть, расположение юнитов в `test_validation.py`), не подмена
+  продуктового решения; продуктовое решение «бэкап должен
+  импортироваться, вложения ведут себя как раньше» дано самим текстом
+  issue.
+- Открытых продуктовых вопросов владельцу нет и не должно быть:
+  ожидаемое поведение (импорт проходит, `detach_required` на чужом
+  инстансе штатно) уже зафиксировано существующим механизмом импорта,
+  решать в ТЗ было нечего кроме технического разбора URL.
+- Откат осмыслен и минимален: одна функция, откат — возврат прежнего
+  тела, данные не мигрируют.
+- Release-артефакты корректно требуют `User-Visible: yes` и оба
+  changelog — пользователь видит другой исход операции импорта, это не
+  внутренняя правка.
+
+### Наблюдение по AC1 (снято ревьюером, не Low-находка)
+
+AC1 явно тестирует резолвинг только для ветки вложений (`files/...` с
+`marker_id`+`name`); ветка планов (`plans/_/...`) с `query` отдельным
+кейсом не покрыта, хотя это та же функция `_internal_path` и правка
+(отделение query через `urlsplit` в начале функции) по построению
+одинаково затрагивает обе ветки. Не поднимаю до Low: реальных
+источников `query` у `plan_url` в кодовой базе не найдено (ни
+`contentUrl()` в `src/logic.ts`, ни ручка загрузки не добавляют его), и
+баг-репорт исходно и единственно про PDF-вложения. Разработчик волен
+добавить такой кейс без правки ТЗ — тестовое покрытие, а не контракт.
+
+## Чего не проверял
+
+- Гейты кода (`npx tsc --noEmit`, `npm test`, `npm run build`,
+  `python -m pytest tests_backend -q`) не запускались: на этапе ревью
+  ТЗ реализации ещё нет, продуктовый код не менялся (только тело
+  issue — не файл в репозитории).
+- Полный HA-харнесс и реальный сценарий экспорт→импорт с настоящим PDF
+  не прогонялся — на этом этапе прогонять нечего, это предмет AC3 на
+  код-ревью.
+- Не проверялась историческая полнота предыдущих ревью по #50 и #167
+  (упомянуты автором как соседние, не дубликаты) на предмет их
+  собственных незакрытых находок — вне предмета этого ревью.
+- Percent-encoding в именах файлов и нормализация существующих
+  конфигов — намеренно вне скоупа этого issue (см. «Не в скоупе» ТЗ),
+  не проверялись как отдельная тема.
+
+## Раздел «Унаследовано» — не применяется
+
+Это первый цикл ревью ТЗ (r1); раздел «Унаследовано из r<N-1>» и
+таблица закрытия предыдущего раунда не ведутся (§2.10 PROCESS.md
+действует со второго цикла).