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

# 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
действует со второго цикла).