docs: review document for #225

Issue: #225
User-Visible: no

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

@@ -0,0 +1,180 @@
+# SPEC-REVIEW — issue #225, цикл r2/2
+
+- **Issue:** https://github.com/Matysh/houseplan-card/issues/225
+- **ТЗ:** тело issue #225 (лёгкий трек `small`, файл в `docs/specs/` не создаётся,
+  §5 PROCESS.md)
+- **Трек:** `small` — лимит циклов ревью ТЗ = 2 (§4 PROCESS.md); это последний
+  допустимый цикл
+- **Ревьюер:** Claude (роль «Ревьюер ТЗ», отдельная сессия от автора)
+- **Вердикт:** зелёный · цикл r2/2 · High: 0 · Medium: 0
+
+## Скоуп проверки (по дельте, §2.10 PROCESS.md)
+
+Раунд r1 закончился жёлтым вердиктом с одной Medium-находкой в скоупе (M1:
+третий пример в AC4 требовал `None`, хотя по собственному контракту ТЗ («query
+не участвует в резолвинге») корректная реализация обязана вернуть валидный
+путь — критерий был недоказуем без нарушения контракта). Автор внёс правку
+только в тело issue; ветка `issue/225-*` не создавалась, код не менялся —
+подтверждено: `git log --oneline` после коммита `be0277f` (докладной документ
+r1) новых коммитов нет.
+
+**SHA/версия r1 не названы в вердикте явно** — это найдено при подготовке
+этого раунда, как и предупреждает §2.10: комментарий-вердикт r1
+(2026-08-20T18:14:47Z) не указывает, к какой версии тела issue он относится.
+Восстановлено косвенно: r1 дословно цитирует AC4 в том виде, в каком он был
+опубликован комментарием автора 2026-08-20T18:08:06Z («ТЗ написано в теле
+issue... Отправляю на ревью ТЗ»), и это совпадает с текущей историей правок
+issue (правки по r1 внесены следующим комментарием, 2026-08-20T18:18:05Z, уже
+после вердикта). Коммит `be0277f5f5ca7595b15f2c64d3b79bd031ed4130` — тот, что
+добавил документ `SPEC-REVIEW-225-r1.md`, и остаётся HEAD `dev` на момент
+этого раунда: код и остальные файлы репозитория с r1 не менялись, дельта
+целиком в тексте issue.
+
+Дельта r1 → r2 (сравнение цитат из документа r1 с текущим телом issue):
+
+1. Таблица AC: пример `…/files/m1/doc.pdf?x=/../../etc` убран из AC4 и
+   перенесён в новый **AC4a** с обратным ожиданием (резолвится в валидный
+   путь), плюс добавлен второй пример с `#fragment`.
+2. AC1 расширен веткой `plans/_/...` с `query`
+   (`/houseplan_files/plans/f1.svg?v=1` → `<root>/plans/f1.svg`).
+3. Блок «Принято предположительно» получил пункты 3 и 4, документирующие
+   ревизию r2.
+
+Контракт поведения, AC2/AC3/AC5/AC6/AC7, план автотестов, мутационный гейт,
+риски, откат, release-артефакты — текст не менялся. По правилу «дельта, а не
+задача целиком» они не разбираются заново; закрытие round r1 показано ниже,
+а неизменное — в разделе «Унаследовано».
+
+Дельта локальна (правка двух строк таблицы + один пункт в предположениях,
+без смены контракта, без ребейза, без новой подсистемы) — полный разбор не
+требуется по критериям §2.10.
+
+## Как проверялось
+
+- Получено тело issue #225 (текущая версия) и все 5 комментариев через
+  `gh issue view 225 --json body,comments,labels`.
+- Восстановлен текст AC4 на момент r1 из цитат в
+  `docs/reviews/SPEC-REVIEW-225-r1.md` (коммит `be0277f`) и построчно сверен с
+  текущим AC4/AC4a.
+- Перепроверена сама правка на непротиворечивость контракту:
+  - **AC4a, пример 1** (`…/files/m1/doc.pdf?x=/../../etc`): по контракту путь
+    берётся из `urlsplit(url).path`; `query` — это `x=/../../etc`, в `.path`
+    не попадает. `.path` = `.../files/m1/doc.pdf`, без единой точки `..`.
+    Хвост после префикса — `m1/doc.pdf`, ровно 2 сегмента, оба проходят
+    `sanitize_*` без изменений → ожидание «резолвится в валидный
+    `<root>/files/m1/doc.pdf`» совпадает с тем, что контракт обязан выдать.
+    Совпадает и с уже проверенным в r1 расчётом (там это был пример 3 AC4,
+    только с противоположным, ошибочным ожиданием `None`).
+  - **AC4a, пример 2** (`…/files/m1/doc.pdf#/../..`): `urlsplit` кладёт
+    `/../..` во `fragment`, `.path` не меняется. По контракту `fragment` тоже
+    не участвует в резолвинге — то же ожидание, тот же вывод: валидный путь.
+    Новый пример, ранее не разбирался; логика идентична предыдущему.
+  - **AC4 (оставшиеся 3 примера)**: не изменились, построчно уже проверены
+    в r1 без ошибок (все три сегментированы так, что дают `None` уже сегодняшней
+    проверкой числа сегментов/наличия `/`, независимо от query) — сверено, что
+    текст этих трёх строк в issue не редактировался.
+  - **AC1, добавленная ветка `plans/_/...` с query**: ветка `plans` в
+    `_internal_path` (по описанию в issue и r1: `import_export.py:355`)
+    отклоняет только хвосты, содержащие `/`; `f1.svg?v=1` после отсечения
+    query по `urlsplit` даёт хвост `f1.svg` — один сегмент без `/`, сравнение
+    `sanitize_filename` проходит. Ожидание AC1 (резолвится в
+    `<root>/plans/f1.svg`) согласуется с контрактом, новой неоднозначности не
+    вносит.
+  - **Пункты 3–4 блока «Принято предположительно»**: описывают ровно
+    произошедшую правку (перенос примера, расширение AC1) и не подменяют
+    продуктовое решение — технические примечания к диффу, а не новое решение
+    за пользователя.
+- Проверено, что контракт поведения, AC2/AC3/AC5/AC6/AC7, план автотестов,
+  мутационный гейт, риски и «Не в скоупе» текстуально идентичны версии,
+  которую разобрал r1 (посимвольное сравнение с цитатами в
+  `SPEC-REVIEW-225-r1.md`) — правок нет, переразбор не требуется.
+- Мутационный гейт (3 id: `internal-path-ignores-query`,
+  `internal-path-allows-traversal`, `roundtrip-import-with-attachment`)
+  остался без изменений; для AC4a отдельного id не заведено — не требуется:
+  AC4a не защищает от регрессии существующего поведения, а описывает новое
+  ожидание, которое доказывается собственным будущим тестом на AC4a
+  напрямую (если реализация ошибочно отклонит валидный запрос, красным
+  станет сам тест AC4a, отдельный мутант не нужен).
+- Гейты кода не запускались: этап — ревью ТЗ, реализации нет, ни один файл
+  репозитория кроме `docs/reviews/*.md` не менялся с r1.
+
+## Закрытие раунда r1
+
+| Находка r1 | Чем закрыта | Где это видно |
+|---|---|---|
+| **M1** (Medium, в скоупе) — третий пример AC4 (`…/files/m1/doc.pdf?x=/../../etc`) требовал `None`, что противоречит контракту ТЗ («query не участвует в резолвинге») | Пример вынесен из AC4 в новый **AC4a** с противоположным (корректным по контракту) ожиданием — «резолвятся в валидный `<root>/files/m1/doc.pdf`»; AC4 сохранил только три traversal-примера, ранее уже проверенных как корректные | Тело issue #225, раздел «Критерии приёмки»: строки AC4 (3 примера) и AC4a (2 примера); авторский комментарий 2026-08-20T18:18:05Z: «M1 закрыт... пример перенесён в новый AC4a с обратным ожиданием» |
+
+Дополнительно (не находка, инициатива автора, не требовала действия
+ревьюера): AC1 расширен веткой `plans/_/...` с `query` — комментарий автора
+сам называет это «дополнительно, не находка»; проверено выше и признано
+корректным.
+
+## Унаследовано из r1
+
+Документ: `docs/reviews/SPEC-REVIEW-225-r1.md`, SHA `be0277f5f5ca7595b15f2c64d3b79bd031ed4130`
+(коммит, добавивший документ; версия текста issue на тот момент —
+комментарий-публикация ТЗ от 2026-08-20T18:08:06Z, до правок по r1).
+
+Принято без повторной проверки в этом раунде, так как дельта их не касается:
+
+- Диагноз бага (`_internal_path` режет URL по `/` без отделения query,
+  `_looks_internal` смотрит только на префикс, `_SAFE_NAME_RE` ломает
+  сравнение на `?`/`=`) — проверен чтением кода в r1, текст issue не менялся.
+- Применимость лёгкого трека `small` (сложность ≤3, одна поверхность, без
+  миграции/compatibility/UX-контракта/перф/touch) — критерии не пересматривались.
+- AC2, AC3, AC5, AC6, AC7 — текст не менялся, однозначны и доказуемы по
+  выводам r1.
+- AC1 в части, не связанной с добавленной веткой `plans` (резолвинг
+  `files/...` с `query`, `CONTENT_URL`-префиксом, `#fragment`,
+  `?v=1#page=2`) — не менялась, проверена в r1.
+- Риск смены классификации `content_manifest` (`external` → `internal`) и
+  сохранение совместимости через `identity()` (`:1034-1039`, `storage` не в
+  ключе сравнения) — код не менялся, проверка r1 остаётся в силе.
+- Идемпотентность `plan_only`-проверки (`:704`, AC7) — не затронута.
+- Решение не брать текст ошибки `invalid_content` в скоуп — продуктовое
+  решение владельца на этапе аналитики, не пересматривается ревью ТЗ.
+- Откат и release-артефакты (`User-Visible: yes`, оба changelog) — текст не
+  менялся.
+- Мутационный гейт (3 id) — текст не менялся, соответствие AC не
+  пересматривалось за исключением явной проверки, что деление AC4/AC4a не
+  требует нового id (см. «Как проверялось»).
+
+## Находки
+
+Нет находок в этом раунде (High: 0, Medium: 0, Low: 0). Единственная
+Medium-находка r1 (M1) закрыта корректно, новых противоречий правка не
+вносит.
+
+## Что проверено и корректно
+
+- M1 закрыт по существу, а не переименован: новое AC4a требует ровно то
+  поведение, которое обязан выдать заявленный контракт («query и fragment
+  не участвуют в резолвинге»), и оба его примера (`?x=/../../etc` и
+  `#/../..`) построчно пересчитаны и дают валидный путь, а не `None`.
+- AC4 после удаления спорного примера остался внутренне согласован: три
+  оставшихся примера — реальные traversal-случаи в самом пути, а не в
+  query/fragment, и ожидание `None` для них верно уже сегодняшней логикой
+  подсчёта сегментов.
+- Расширение AC1 веткой `plans/_/...?v=...` не создаёт скрытого нового
+  контракта — это то же отделение query той же функцией, применённое ко
+  второй ветке, которую r1 отдельно отметил как непроверенную (и намеренно
+  не поднял до находки).
+- Дельта не расширяет скоуп и не меняет контракт поведения, поэтому
+  ограниченный по объёму раунд корректен по критериям §2.10 (локальность
+  правки, отсутствие ребейза, отсутствие новой подсистемы).
+- Лимит циклов лёгкого трека (2) не превышен: это второй и последний цикл,
+  вердикт зелёный — задача может двигаться в «Готово к разработке».
+
+## Чего не проверял
+
+- Гейты кода (`npx tsc --noEmit`, `npm test`, `npm run build`,
+  `python -m pytest tests_backend -q`) — не запускались: реализации ещё нет,
+  ни один продуктовый файл не менялся между r1 и r2, только текст issue.
+- Не пересматривался диагноз бага и код `_internal_path` /
+  `_looks_internal` / `content_manifest` / `_content_state` — не изменился
+  с r1, где уже проверен чтением построчно.
+- Не проверялась ветка `plans` в реальном коде на предмет иных, не
+  описанных в ТЗ побочных эффектов — вне зоны этого ревью (ревью ТЗ судит
+  текст, а не реализацию, которой ещё нет).
+- Percent-encoding в именах файлов и нормализация существующих конфигов —
+  по-прежнему намеренно вне скоупа issue, не проверялись.