diff --git a/docs/reviews/SPEC-REVIEW-535-r1.md b/docs/reviews/SPEC-REVIEW-535-r1.md new file mode 100644 index 00000000..4e8e904a --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-535-r1.md @@ -0,0 +1,280 @@ +# SPEC-REVIEW-535-r1 + +**Issue:** #535 — «Страница панели грузит карточку по URL без версии: плашка о несовпадении версий переживает перезагрузку на /houseplan» +**Этап:** S4-spec-review (ревью ТЗ, PROCESS.md §2.4) +**Заход:** r1 · блокирующих циклов израсходовано 0 из 4 +**Ревьюер:** Claude (Sonnet 5), роль «ревьюер ТЗ» +**Автор ТЗ:** Matysh (в теле issue, раздел `## ТЗ`) + +## Материал раунда + +- Тело issue #535, раздел `## ТЗ` (включая «Продуктовая рамка», К1–К6, «Принято + предположительно», таблица AC1–AC6, «Риски», «Затронутые файлы», «Откат»). +- SHA256 нормализованного тела issue (как получено через `gh issue view --json body`, + без дополнительной нормализации переносов строк): + `77204930eaea05482f2a7da2bac3af79247e7d7356e0449551cb997da2c76fc8` +- Оба комментария issue (S2-аналитика, «ТЗ готово — на ревью»). +- Код на момент ревью: `dev` @ `1ca3b5fae3e2df9fbd41ead35d98c8d58c87f5b8` (рабочая + копия чиста, HEAD == origin/dev). Читался только для проверки выполнимости и + однозначности ТЗ, продуктовый код не менялся. + +## Скоуп ревью + +Проверялись: обязательные разделы ТЗ (PROCESS.md §7.1), однозначность и +проверяемость каждого AC, наличие способа доказательства и «чем краснеет», +отсутствие невыделенных догадок о поведении, соответствие `docs/SCOPE.md`, +согласованность контракта К1–К6 с текущей реализацией `entryFallbackPlugin` +(`scripts/bundle-manifest.mjs`) и существующими тестами `#486` в +`test/bundle-assets.test.mjs`, а также спорный вопрос классификации трека, +который автор прямо адресовал ревьюеру. + +## Как проверялось + +1. Прочитан `docs/SCOPE.md` — задача не вводит новую функциональность, это + правка доставки уже существующего продукта (сходится по духу с J6 «Keep the + plan true», хотя формально это баг доставки бандла, а не пользовательской + фичи — отдельного пункта J-карты не требует). +2. Прочитан `PROCESS.md` §1 (границы классов файлов), §2.4, §2.5 (DoR-чеклист, + чтобы понять, что должно быть закрыто до перехода в `S5-ready`), §7.1 + (обязательные разделы ТЗ), §7.2 (формат вердикта). +3. Прочитано тело issue #535 целиком и оба комментария. +4. Прочитан текущий код `entryFallbackPlugin` в `scripts/bundle-manifest.mjs` + (строки 300–386) — как сегодня панель переписывается на импорт фасада, как + считается `imports`/`initialPanelFiles`/`initialPanelOnlyGzipBytes` + (`reachable()`, `initialPanelOnly`). +5. Прочитаны оба существующих теста `#486` в `test/bundle-assets.test.mjs` + (`both stable entries install a visible stale-load fallback`, + `panel entry routes through the card facade and fails loudly too`), чтобы + проверить, действительно ли их утверждения пиннуют именно фасадную + маршрутизацию (К5) и действительно ли они «переворачиваются» предложенной + правкой, а не остаются случайно истинными. +6. Прочитан фрагмент `docs/ARCHITECTURE.md` про два стабильных входа + (строки 69–86) — проверить, не противоречит ли К3/АС6 уже + зафиксированному описанию маршрутизации. +7. Прослежена логика графа вручную (`reachable()` от `entry`=facade и от + `panelEntry`), чтобы проверить арифметику риска №2 (бюджет) и корректность + К4/AC4 до и после предложенной правки. + +Гейты (typecheck/test/build/golden/backend) не гонялись — это этап ревью ТЗ, +продуктовый код ещё не написан; §2.4 их не требует. + +## Разбор по существу + +### Обязательные разделы ТЗ (§7.1) + +Присутствуют по содержанию, хотя не всегда под буквальными заголовками: + +| Раздел §7.1 | Есть? | Где | +|---|---|---| +| Сценарий | ✅ | «Продуктовая рамка»: персона — домочадцы и администратор, поверхность — пункт меню House Plan | +| Что человек увидит до/после | ✅ | «До.» / «После.» | +| Проблема | ✅ | Симптом + «Причина найдена и измерена» + «Как это даёт ровно наблюдаемое поведение» в теле issue до ТЗ | +| Скоуп и не-скоуп | ⚠️ частично | Не-скоуп явно перечислен в комментарии S2 («Чего в этой задаче нет»: формулировка плашки, #536, заголовки прокси) и повторён в комментарии «ТЗ готово», но **не продублирован внутри самого раздела `## ТЗ`** | +| Контракт поведения | ✅ | К1–К6 | +| UX | ✅ | К2 (текст фолбэка дословно, 4 языка, класс элемента) | +| Модель данных и миграция | ⚠️ подразумевается | Нет явного «миграция: нет»; из контекста (только `scripts/**`, `test/**`, документация, три копии бандла) очевидно, что миграции нет | +| i18n | ✅ (по факту «нет новых ключей») | К2: текст фолбэка не меняется ни в одном из 4 языков | +| AC1…ACn с доказательством | ✅ | Таблица AC1–AC6, у каждого «чем доказан» и «чем краснеет» | +| План автотестов | ✅ | Встроен в таблицу AC (имена тестов/файлов) | +| Риски | ✅ | 4 пункта | +| Откат | ✅ | Один абзац, простой и точный | +| Release-артефакты | ✅ | Названы в преамбуле ТЗ: `docs/ARCHITECTURE.md`, оба changelog | +| Влияние на touch | ❌ не названо явно | Задача не трогает ни один UI/editor/touch-контракт (только сборочный конвейер и панельный/карточный загрузчик); фактическое влияние — «нет», но фраза отсутствует | + +Два ⚠️/❌ пункта (модель данных/миграция и touch — явные «нет» не написаны) +разбираю ниже как находки Low. + +### Техническая состоятельность контракта (К1–К6) и AC + +Проверил код `entryFallbackPlugin` вручную, чтобы понять, действительно ли +предложенная правка (импортировать `cardAsset` напрямую вместо фасада, +сохранив `try/catch`) реализуема ровно так, как описано, и действительно ли +существующие тесты `#486` меняют смысл, а не остаются проходить случайно. + +- **К1/AC1/AC2 реализуемы буквально.** Сегодня `panelEntry.code` переписывается + regex'ом `panelPattern` (совпадающим с уже существующим статическим импортом + `cardAsset`, который туда кладёт сам Rollup) на + `try{await import("./houseplan-card.js")}catch{...}`, и `panelEntry.imports` + жёстко ставится в `[CARD_ENTRY_FILE]` (`scripts/bundle-manifest.mjs:377–383`). + Предложенная правка меняет ровно эти две строки: подстановка в `try{await + import("${cardAsset}")}...` и `panelEntry.imports = [cardAsset]`. Никакого + скрытого усложнения нет — это буквально «убрать один слой переадресации», + как и заявлено в «Откате». +- **К5/тесты `#486` действительно переворачиваются, а не остаются случайно + зелёными.** Прочитал оба теста построчно (`test/bundle-assets.test.mjs:305–360` + и `:522–546`). Первый матчит `panel` на `try{await + import("./houseplan-card.js")}catch` и `bundle['houseplan-panel.js'].imports + === ['houseplan-card.js']` — оба утверждения станут ложными на новом коде и + должны замениться на `.../houseplan-assets/card-HASH.js` и + `['houseplan-assets/card-HASH.js']` соответственно, что ровно и обещает AC1. + Второй тест читает **собранный** `dist/houseplan-panel.js` и матчит ту же + строку `./houseplan-card.js` — тоже переворачивается, ровно как обещает AC2 + («проверка по `dist`, а не по плагину»). +- **К4/AC4 — риск №1 в ТЗ корректно предсказывает реальную поломку + существующего утверждения о манифесте**, а не выдуманную. Прошёл вручную + логику `reachable()`: сейчас `initial` = reachable(entry=`houseplan-card.js`) + включает сам фасад как корень; `initialPanel` = reachable(panel) = {panel} ∪ + `initial`, потому что `panelEntry.imports=[CARD_ENTRY_FILE]`. Отсюda + `initialViewFiles.every(p => initialPanelFiles.includes(p))` истинно + тривиально. После правки `panelEntry.imports=[cardAsset]`, и `initialPanel` + перестаёт включать сам файл фасада (`houseplan-card.js`) — утверждение в + строке 542 текущего теста station станет **ложным**, именно это и называет + риск №1 ТЗ («написано под фасад... надо переформулировать через граф + реализации»). Это открытое место корректно помечено как риск, а не выдано за + решённое — соответствует правилу «размытое место выносится, а не + додумывается». Как именно переформулировать сравнение (через граф от + `cardAsset`, а не от `entry`) — техническая деталь реализации, которую §7.1 + прямо отдаёт исполнителю («агенты решают сами... стратегия тестов»), поэтому + не считаю это находкой ревью ТЗ. +- **Риск №2 (бюджет) арифметически неточен, но не вреден.** По той же + логике `initialPanelOnlyGzipBytes` = `initialPanel − initial` — множество, + которое **и до, и после** правки состоит только из самого файла панели + (фасад всегда был частью `initial`, а не эксклюзивным для панели), то есть + сам метрический бюджет `initialPanelOnlyGzipBytes`, скорее всего, не + сдвинется на «размер фасада», сдвинется общий размер `initialPanelFiles` + (граф целиком). Это неточность в обосновании риска, а не в контракте: ТЗ уже + предусматривает митигацию («рекалибруется с датированной записью, а не тихо + поднимается») независимо от того, какая метрика реально изменится и на + сколько. AC4 всё равно остаётся проверяемым (`bundle-budget.mjs` плюс + утверждения теста), поэтому не блокирую — но называю ревьюеру кода: при + проверке AC4 стоит явно свериться, какая метрика в итоге изменилась, и что + это совпадает с тем, что записано в риске/changelog. +- **К3 (фасад карточки не трогается) не имеет отдельного AC, и это + корректно** — правка физически не касается кода, рисующего `cardEntry.code` + (строки 344–358 `bundle-manifest.mjs`), и существующая (переписываемая, но не + удаляемая) часть тех же двух тестов `#486`, которая проверяет + карточную сторону, продолжает быть тем самым свидетелем регрессии. +- **AC3 (фолбэк дословно)** — проверил, что механизм построения текста + (`fallbackDefinition`) вообще не тронут К1; он используется одинаково для + обоих контрактов и не зависит от того, что импортируется. Корректно. +- **AC5 (общедистрибутивный инвариант)** — новый тест, скоуп ясен + («ни один входной файл не ссылается на карточку без версии» по `dist/*.js`), + однозначен и проверяем. +- **AC6** помечен как доказываемый «ревьюером кода построчно» — это + недефенсивный AC (расположение текста в документации), правило §2.7 про + таблицу «чем краснеет» на него не распространяется по собственной оговорке + процесса. + +### Классификация трека — вопрос, адресованный ревьюеру + +Автор прямо просит решить: инфраструктурная это задача (по механическому +признаку §1 класса файлов — да, только `scripts/**`/`test/**`/документация) или +полный трек (по факту видимого поведения). Довод автора: на устаревшем +загрузчике сегодня пользователь **молча** получает старую карточку; после +правки он **всегда** увидит громкий фолбэк в тех случаях, где раньше молча +работал (или не работал вовсе) старый код. Проверил механику вручную +(см. выше) — это не гипотеза: до правки возможен путь, где старый `houseplan-panel.js` +из кэша браузера подтягивает **свежий** фасад `houseplan-card.js` (тоже без +версии, тоже подверженный эвристике, но не обязательно ровно такой же +устаревший), который в свою очередь тянет уже новый чанк — рабочая, но +рассинхронизированная версия без единой ошибки. После правки этот путь +исчезает: старый панельный вход всегда просит старый (и, по контракту сервера +из `docs/ARCHITECTURE.md:82–84`, отсутствующий после деплоя) чанк напрямую и +надёжно показывает фолбэк. Это действительно новый наблюдаемый контракт (не +новый текст, но новое условие срабатывания), и довод автора о нарушении +критерия §5 «нет нового UX-контракта» подтверждается чтением кода, а не +принимается на веру. **Соглашаюсь с автором: полный трек оправдан.** Это +решает технический спор в пользу продолжения ревью, как и предписывает §7.1 +(«технический спор автора и ревьюера решается вердиктом»). + +### Проверка «не выдана ли догадка за решение» + +Прошёлся по всем формулировкам поведения на предмет непомеченных допущений: + +- Утверждение «загрузчик выбирает версию всей карточки» и таблица заголовков + кэша — это измерение на боевом стенде (проверяемый факт, не догадка). +- «Через какое-то время окно истекает... и всё само чинится» — помечено как + объяснение уже наблюдавшегося поведения («не всегда», «точных шагов не + нашёл»), не выдаётся за новый AC. +- Три пункта «Принято предположительно» корректно помечены как техническое + предположение, которое можно менять свободно, включая явную причину отказа + от вариантов B и C. +- Риск №1 и №2 корректно поданы как риски, а не как решённые факты (см. выше). + +Непомеченных догадок о поведении, которого нет ни в одном документе, не нашёл. + +## Находки + +Блокирующих (High) находок нет. Находок Medium в скоупе или вне скоупа нет. + +### Low (сняты решением ревьюера, без правки) + +1. **Не-скоуп не продублирован внутри `## ТЗ`.** Перечень «чего в этой задаче + нет» (формулировка плашки, побочная находка #536, заголовки прокси) есть в + комментарии S2-аналитики и повторён в комментарии «ТЗ готово», но не + переписан внутри самого раздела ТЗ в теле issue. Содержательно вопрос + закрыт и однозначен — при чтении всего issue целиком путаницы не возникает, + поэтому не считаю это основанием для возврата. Снимаю с записью: для + исполнителя это не создаёт риска, потому что ветка/PR будут ссылаться на тот + же issue целиком. +2. **Явное «миграция: нет» и «touch: нет» не написаны отдельной строкой**, как + того просит чек-лист DoR §2.5. По содержанию задачи (только + `scripts/**`/`test/**`/документация/три копии бандла, никакого нового + конфига, никакого UI/editor-контракта) оба пункта очевидно «нет». Снимаю + с записью: не считаю нужным гонять задачу на ещё один цикл ради двух строк + текста; ревьюер кода или сам автор могут дописать их при переводе в + `S5-ready` без нового цикла ревью ТЗ. +3. **Риск №2 (бюджет) арифметически не точен** (см. разбор AC4 выше): скорее + всего сдвинется не `initialPanelOnlyGzipBytes`, а общий размер + `initialPanelFiles`. Митигация в риске («рекалибруется явно, а не тихо») + покрывает оба случая одинаково, поэтому не блокирую, но передаю ревьюеру + кода как пункт для проверки при разборе AC4. + +## Что проверено и корректно + +- Все обязательные по существу элементы §7.1 присутствуют (сценарий, что + человек увидит, проблема, контракт, AC с доказательством, риски, откат, + release-артефакты). +- Каждый AC1–AC6 однозначен, имеет названный способ доказательства и (где + применимо как защитный) — «чем краснеет». +- К1/К5 реализуемы буквально существующим кодом `entryFallbackPlugin`; + проверил вручную, что оба переписываемых теста `#486` действительно меняют + истинностное значение своих утверждений на новом коде, а не остаются + случайно зелёными. +- К4/риск №1 корректно определяет реальную поломку существующего утверждения + о манифесте (арифметика `reachable()` проверена вручную) и корректно + оставляет способ переформулировки на усмотрение реализации. +- Классификация полного трека, оспоренная самим автором, подтверждена + независимым разбором механики: изменение действительно вводит новое условие + срабатывания видимого фолбэка, а не только смену текста. +- Непомеченных догадок о недокументированном поведении не найдено. +- Названные затронутые файлы (`scripts/bundle-manifest.mjs`, + `test/bundle-assets.test.mjs`, `scripts/mutation-gate.mjs`, + `docs/ARCHITECTURE.md`, оба changelog, три копии бандла) соответствуют + реальному расположению кода, который придётся менять. + +## Чего не проверял + +- Гейты (`typecheck`/`test`/`build`/golden/backend) — не требуются на этапе + ревью ТЗ (§2.4), кода ещё нет. +- Реальный прогон сборки/тестов с внесённой правкой (её не существует; это + этап кода-ревью). +- Формулировку самой плашки о несовпадении версий и побочную находку про + `disconnect()`/`_hideBanner()` — явно вне скоупа этой задачи (вынесены + автором в отдельный разговор и в #536 соответственно), не проверял по + существу. +- Заголовки прокси/reverse-proxy перед Home Assistant — вне скоупа по тексту + ТЗ, не проверял. +- `scripts/mutation-gate.mjs` — не проверял, существует ли уже похожий по духу + мутант или конфликт имени `panel-imports-unversioned-card-entry`; это + деталь реализации, а не предмет ревью ТЗ. + +## Вердикт + +Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 → в задаче + +Issue #535 переходит в `S5-ready`. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `1ca3b5fae3e2` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `3721f5a271916bfb2cc467947132525042363285` + ``` + git log --all --format='%H %T' | grep 3721f5a27191 + ``` +- Тело issue: `77204930eaea05482f2a7da2bac3af79247e7d7356e0449551cb997da2c76fc8` +- Вердикт конвейера: `green` · High 0