diff --git a/docs/reviews/SPEC-REVIEW-225-r2.md b/docs/reviews/SPEC-REVIEW-225-r2.md new file mode 100644 index 00000000..5cc681f3 --- /dev/null +++ b/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` → `/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_*` без изменений → ожидание «резолвится в валидный + `/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 (резолвится в + `/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** с противоположным (корректным по контракту) ожиданием — «резолвятся в валидный `/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, не проверялись.