From 39f811a870ee06317b7c9ebd2e30cf1fe2f07298 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Tue, 1 Sep 2026 13:33:17 +0000 Subject: [PATCH] docs: review document for #404 Issue: #404 User-Visible: no --- docs/reviews/SPEC-REVIEW-404-r1.md | 243 +++++++++++++++++++++++++++++ 1 file changed, 243 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-404-r1.md diff --git a/docs/reviews/SPEC-REVIEW-404-r1.md b/docs/reviews/SPEC-REVIEW-404-r1.md new file mode 100644 index 00000000..2e98bdf5 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-404-r1.md @@ -0,0 +1,243 @@ +# SPEC-REVIEW-404-r1 + +- Issue: https://github.com/Matysh/houseplan-card/issues/404 +- ТЗ: `docs/specs/404-smoke-exception-guard.md`, ветка `issue/404-smoke-exception-guard` +- SHA материала ревью: `884387d770665d7ca8f6dd11e7885e3ea6f4d6b7` +- Трек: полный (метка `small` отсутствует), лимит циклов — 4 +- Заход: r1 · вердикт: **жёлтый** + +## Скоуп + +Класс файлов задачи — только B (`demo/serve.mjs`, `demo/smoke_danger_confirmation.mjs`, +новые фикстуры `demo/fixtures/guard_*.mjs`, новый тест `test/smoke-exception-guard.test.mjs`, +запись в `scripts/mutation-gate.mjs`). Ни одного файла класса A — по §1 PROCESS.md задача +формально могла бы идти вне флоу, автор выбрал полный трек с файлом ТЗ по прецедентам +#398/#399; это решение задокументировано в комментарии, придирок к самому выбору трека нет. + +Продуктового поведения задача не меняет (`User-Visible: no`), поэтому продуктовая рамка +`docs/SCOPE.md` к ней не применяется буквально — это инфраструктурная починка гейта, а не +фича. Читал `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` полностью, тело issue #404 и оба +комментария (S2-анализ владельца, объявление ТЗ). + +## Как проверялось + +Ревью ТЗ на полном треке — это чтение и сверка утверждений документа с фактическим +состоянием дерева на SHA материала (без прогона тяжёлых гейтов — на этапе ТЗ они не нужны, +кода ещё нет). Конкретно: + +1. Прочитан весь `docs/specs/404-smoke-exception-guard.md` (201 строка). +2. Прочитан `demo/serve.mjs` целиком — сверены номера строк, сигнатура `finish()`, механизм + `_pageErrors`/`page.on('pageerror')`, отсутствие текущей регистрации страниц. +3. Прочитан `scripts/mutation-gate.mjs` — подтверждена применимость AC2 (реестр уже патчит + `demo/serve.mjs` в существующем мутанте `smoke-launcher-skips-freshness`, прецедент есть). +4. Проверена фикстура `demo/smoke_danger_confirmation.mjs:167` и объявленный тип + `_markerDialog` в `src/houseplan-card.ts:2222-2235` — подтверждён дефект фикстуры (нет + `binding`/`bindingMode`), как описано в ТЗ. +5. Пересчитаны все числовые утверждения ТЗ командами на дереве: + - `ls demo/smoke_*.mjs | wc -l` → 211 (совпадает); + - `grep -l "finish(" demo/smoke_*.mjs | wc -l` → 205 (совпадает с «205 смоков зовут её + как `finish(browser, out)`»); + - `grep -rl "spawnSync\|execFileSync" test/*.mjs | wc -l` → 8 (совпадает с «8 тестов в + `test/` уже запускают процессы»). +6. Живым прогоном Playwright/Chromium (тот же движок, что использует проект) проверено + недокументированное в ТЗ утверждение AC3 «Проверено, что `page.on('pageerror')` в + Chromium [ловит `Promise.reject`]» — команда и результат см. в разделе Low ниже. +7. Проверена полнота охвата механизма «регистрация страниц»: прогреп всех + `demo/smoke_*.mjs` на `newContext|newPage` вне `launch()/launchInternal`, разобраны все + три найденных файла построчно (`smoke_entry_stale.mjs`, `smoke_svg_sandbox.mjs`, + `smoke_zoom_flash.mjs`) — см. находку Medium-1. +8. Проверено, какие смоки вообще не читают гард: `grep -L "finish(" demo/smoke_*.mjs` → 6 + файлов, разобраны все шесть — см. находку Medium-2. + +Гейты `typecheck`/`test`/`build` не гонялись: продуктового и тестового кода задача ещё не +содержит (только `docs/specs/**`), гонять их не на чем и не за чем — это подтверждается +самим диффом (`git show --stat HEAD` → один файл, `docs/specs/404-smoke-exception-guard.md`). + +## Находки + +### Medium-1 (в скоупе задачи) — «регистрация страниц» покрывает не все страницы, которые ТЗ обязано покрыть + +**Файл**: `docs/specs/404-smoke-exception-guard.md`, разделы «Контракт» (строки 75-77), +«Честная граница» (79-83), АС6 (141-143). + +**Суть**. ТЗ формулирует контракт регистрации так: «Страницы регистрируются там, где +создаются, — в `launchInternal`». Это неполно: как минимум два смока, которые уже пользуются +общим гардом (`launch()`/`finish()` из `serve.mjs`), создают дополнительные страницы **вне** +`launchInternal`, и эти страницы гарантированно останутся слепой зоной гарда и после починки +— ровно тот же класс дефекта, который описывает issue («любое исключение внутри карточки во +время смока остаётся незамеченным»), просто с другим механизмом, чем асинхронная гонка. + +- `demo/smoke_zoom_flash.mjs:90-91,108`: фаза 1 получает `page` из `launch()` и закрывает её + (`await page.close()`); фаза 2 открывает `p2 = await ctx.newPage()` на **новом** контексте + (`ctx = await browser.newContext(...)`, строка 26) и вешает свой отдельный слушатель: + `p2.on('pageerror', (e) => console.log('EXC2', e.message));` — он только печатает `EXC2` в + лог и не трогает `_failures`/`_pageErrors` из `serve.mjs`. В конце вызывается + `await finish(browser)` (строка 108) — по контракту ТЗ он опросит `_livePages`, но `p2` там + никогда не окажется, потому что она создана не в `launchInternal`. +- `demo/smoke_svg_sandbox.mjs:38,48,58`: три страницы `before`/`after`/`card` создаются + напрямую через `ctx.newPage()` на `ctx = await browser.newContext()` (строка 22), и ни для + одной из них вообще не вешается `page.on('pageerror')` — не «слепа к хвосту», а слепа + полностью, с рождения. Файл при этом зовёт `finish(browser)` последней строкой. + +**Сценарий отказа**. Задача реализована по ТЗ буквально, AC6 (211 смоков, зелены все, кроме +`smoke_infinite_canvas`) проходит зелёным прогоном — и при этом внутри `smoke_zoom_flash.mjs` +на странице `p2` (фаза сэмплирования кадров зума) или внутри любой из трёх страниц +`smoke_svg_sandbox.mjs` продолжает падать необработанное исключение в карточке — смок как +печатал `OK`, так и будет печатать, потому что `p2`/`before`/`after`/`card` никогда не были +частью того, что читает `finish()`. Ровно симптом issue, просто АС не может его поймать. + +**Почему это находка ТЗ, а не наблюдение к реализации**: раздел «Честная граница, которую +задача не закрывает» называет ровно один исключённый случай — окно после round-trip'а +(`beforeunload` при закрытии браузера). Это честно, но неполно: второй, куда более широкий +разрыв (страницы/контексты, порождённые смоком напрямую, минуя `launchInternal`) в этом +разделе не назван вовсе, а формулировка контракта («страницы регистрируются там, где +создаются») читается как утверждение полноты. Это и есть «догадка, выданная за решение»: +автор, судя по тексту, не проверял `demo/smoke_*.mjs` на использование `newContext`/`newPage` +в обход `launchInternal`, иначе разрыв был бы назван явно, как это сделано для `beforeunload`. + +Отдельно — ТЗ само создаёт внутреннее противоречие: заявляет «регистрация страниц» в скоупе +(«В скоупе: `demo/serve.mjs` (регистрация страниц и round-trip в `finish`)»), но тут же +исключает из скоупа «остальные 204 смока» (не-скоуп) — а именно там и рождаются +незарегистрированные страницы `p2`/`before`/`after`/`card`. Одновременно «регистрация всех +страниц» и «204 смока не трогаем» невыполнимы вместе для этих двух файлов. + +**Что нужно от автора**: явно разрешить противоречие в тексте ТЗ — либо (а) распространить +регистрацию на страницы/контексты, порождаемые смоком после возврата из `launchInternal` (это +неизбежно тронет `smoke_zoom_flash.mjs` и `smoke_svg_sandbox.mjs`, тогда AC7 нужно +скорректировать: «204 смока, кроме уже перечисленных N, не тронуты»), либо (б) вписать этот +разрыв вторым пунктом в «Честную границу» рядом с `beforeunload`, назвав оба файла по имени, +и убрать из АС6 всё, что читается как утверждение полного покрытия. Технический выбор между +(а) и (б) — за автором, вопрос владельцу не нужен. + +### Medium-2 (вне скоупа задачи → отдельный issue) — шесть смоков вообще не читают гард + +**Файлы**: `demo/smoke_deeplink.mjs`, `demo/smoke_entry_stale.mjs`, +`demo/smoke_glow_blending.mjs`, `demo/smoke_icon_center.mjs`, +`demo/smoke_long_press_gesture.mjs`, `demo/smoke_space_card.mjs`. + +**Суть**. `grep -L "finish(" demo/smoke_*.mjs` даёт ровно эти 6 файлов из 211 — они никогда +не вызывают `finish()` из `serve.mjs`, а значит, независимо от починки round-trip'а, счётчик +`_pageErrors` для них никогда не читается и не может уронить смок. + +- Пять из шести (`smoke_deeplink`, `smoke_glow_blending`, `smoke_icon_center`, + `smoke_long_press_gesture`, `smoke_space_card`) импортируют `launch` из `serve.mjs` — значит + `page.on('pageerror')` вешается и `_pageErrors` инкрементируется, — но у каждого своя + ручная логика выхода (`if (!ok) { …; process.exit(1); }`), которая `_pageErrors` не + проверяет вовсе. Исключение внутри карточки в этих смоках инкрементирует счётчик, который + никто никогда не прочитает. +- `smoke_entry_stale.mjs` — отдельный случай, хуже: он вообще не пользуется + `launch()`/`launchInternal`, заводит `browser`/`page` сам, ведёт свой собственный + `pageErrors` (строка 23, локальная переменная, не связанная с `serve.mjs`), и в конце + (последние строки файла) делает `if (Object.values(out).every(Boolean)) console.log('OK');` + без единого `process.exitCode =` или `process.exit(`. То есть даже провал его собственных + `check()`/`checkAll()` не красит выход процесса — это тот самый паттерн «до 27.07.2026», + который комментарий `demo/serve.mjs:15-18` называет прямо: «смоки печатали булевы значения + и всегда выходили нулём». + +**Почему вне скоупа #404**: починка требует править сами эти 6 файлов +(`demo/smoke_*.mjs`), а ТЗ #404 прямо и обоснованно исключает такую правку в AC7 («дифф +задачи не содержит `demo/smoke_*.mjs`, кроме `smoke_danger_confirmation.mjs`») — расширять +это в текущей задаче значило бы нарушить её же собственный контракт. Причина дефекта тоже +другая: не асинхронная гонка доставки `pageerror`, а отсутствие вызова проверяющей функции. + +**Действие**: заведён отдельный issue [#407](https://github.com/Matysh/houseplan-card/issues/407) +со ссылкой на #404, метки `bug`, `tests`, `S1-new` — ревью не патчит и не решает scope за +автора, только даёт отдельный трек находке, которая по формальным критериям (§202) не может +чиниться в текущей задаче. + +### Low (снято решением ревьюера, без действия автора) + +**АС3, `docs/specs/404-smoke-exception-guard.md:132-134`**: утверждение «Проверено, что +`page.on('pageerror')` в Chromium его получает» подано как установленный факт, но ни в теле +ТЗ, ни в комментариях issue не приведена команда/результат этой проверки — в отличие от АС1/ +АС2, где транскрипт пробы (`EXC .../exit=`) приведён дословно. Это ровно тот шаблон «догадка, +выданная за решение», который должен становиться замечанием, если утверждение не подтверждено. + +Проверил сам, живым прогоном на том же движке (Playwright + Chromium, `--no-sandbox`, как в +`demo/serve.mjs`): + +``` +node -e " +const { chromium } = require('playwright'); +(async () => { + const browser = await chromium.launch({ args: ['--no-sandbox'] }); + const page = await browser.newPage(); + let errors = 0; + page.on('pageerror', (e) => { errors++; console.log('PAGEERROR:', e.message); }); + await page.goto('data:text/html,'); + await page.waitForTimeout(500); + console.log('errors after rejection:', errors); + await browser.close(); +})(); +" +``` +``` +PAGEERROR: boom-rejection +errors after rejection: 1 +``` + +Утверждение подтвердилось независимо. Снимаю находку без возврата автору — но фиксирую +здесь, чтобы АС3 в реализации имела ссылку на реальную проверку, а не только на веру в текст +ТЗ, ведь при код-ревью «verified без команды доказательством не является» уже будет применяться +в полную силу. + +## Что проверено и корректно + +- Обязательные разделы §7.1 присутствуют все: сценарий, что человек увидит до/после, + проблема и контракт, скоуп/не-скоуп, UX/данные/i18n («не применимо», обоснованно — код не + продуктовый), АС1…АС8 с доказательством, план автотестов, риски, откат, release-артефакты. +- Технические цифры и ссылки на код в ТЗ пересчитаны и подтверждены: номера строк + `demo/serve.mjs` (34-45 в момент разбора владельца → 35-46 сейчас, сдвиг на одну строку не + меняет сути и не вводит в заблуждение), `205 из 211` смоков зовут `finish(browser, out)`, + `8` тестов-прецедентов в `test/` уже гоняют процессы, объявленный тип `_markerDialog` + (`src/houseplan-card.ts:2222-2235`) действительно требует `binding`/`bindingMode`, + реальная строка фикстуры `demo/smoke_danger_confirmation.mjs:167` действительно их не + задаёт. +- АС1/АС2 (тайминг-гонка `pageerror` vs `finish()`) подтверждены исполнением в S2-анализе + владельца (пробный `launch()` → исключение → `finish()` даёт `exit=0`; с одним + `page.evaluate(() => 0)` между ними — `exit=1`) — это ровно то доказательство «тест умеет + падать», которого ждёт процесс, просто снятое до входа в S3, а не заново мной: я прочитал + транскрипт и он внутренне непротиворечив с описанным механизмом (`Runtime.exceptionThrown` + доставляется по CDP асинхронно, round-trip вытесняет очередь). + AC2 отдельно требует зарегистрированного в `scripts/mutation-gate.mjs` мутанта, а не ручной + правки — прецедент такого мутанта на `demo/serve.mjs` уже есть в реестре + (`smoke-launcher-skips-freshness`, патчит ту же функцию `launchInternal`), то есть + требование выполнимо буквально как написано. +- АС4 (порог 5% на замере трёх смоков), АС5 (правка только фикстуры, без ослабления + утверждений), АС7 (дифф не трогает 204 смока), АС8 (закрытая страница не ломает `finish`) — + однозначны и проверяемы, способ доказательства назван для каждого. +- Продуктовая рамка `docs/SCOPE.md` к задаче неприменима буквально (задача не меняет продукт), + и в тексте это явно проговорено («Видимого поведения продукта задача не меняет»), а не + просто пропущено — ТЗ не выдаёт молчание за решение. +- Открытых продуктовых вопросов к владельцу нет и не должно быть: всё, что решает ТЗ, — + техническое (где регистрировать страницы, как считать round-trip, формат фикстур), ни один + вопрос не про то, что видит или делает человек — задача инфраструктурная. +- Откат простой и правдоподобный: удаление двух фрагментов кода делает фикстуры/тест + красными — это явный сигнал отката, а не молчаливая деградация. + +## Чего не проверял + +- Не гонял `typecheck`/`test`/`build` — в диффе на этом SHA нет ничего, кроме + `docs/specs/404-smoke-exception-guard.md`, гонять гейты не на чем. +- Не проверял оставшиеся ~200 файлов `demo/smoke_*.mjs` на предмет собственных `newContext`/ + `newPage` построчно за пределами найденных через `grep -rln "newContext\|newPage"` трёх — + сам grep по всему набору отработал и дал исчерпывающий список (3 файла), дальше не искал. +- Не проверял, вызывает ли `_bindingHasHaPage`/типизация диалога маркера какие-то другие + дефекты — ТЗ прямо и справедливо относит это к «дефекта поведения нет», и это совпадает с + прочитанным кодом (15 точек создания диалога действительно передают `binding`, беглым + `grep`), глубже не копал, так как продуктовый код вне скоупа этой задачи и этого ревью. +- Не запускал полный набор `demo/smoke_*.mjs` (211 файлов) — на этапе ТЗ кода для прогона нет, + а S2-анализ владельца этот прогон уже провёл и результат (211 файлов, красный ровно один — + `smoke_danger_confirmation`) внутренне согласуется с находками Medium-1/Medium-2: обе они + про смоки, которые физически не могут покраснеть от `_pageErrors`, так что прогон владельца + их и не показал бы — это не противоречие, а подтверждение находок. + +## Итог + +High: 0. Medium: 2 (1 в скоупе — исправляется правкой ТЗ; 1 вне скоупа — новый issue #407). +Low: 1 (снят). Полностью выполненных АС недостаточно для зелёного вердикта: находка Medium-1 +бьёт именно по доказательной силе АС6 и по честности раздела «Честная граница», это находка +про сам текст ТЗ, а не про гипотетическую реализацию. + +**Вердикт: жёлтый.** Возврат автору на правку ТЗ (Medium-1), фикс проходит r2 в рамках той же +задачи. Medium-2 закрыт отдельным issue [#407](https://github.com/Matysh/houseplan-card/issues/407).