From ad2f9afd2bfe83b6aea758af91e0e55ebd7bdd55 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:20:07 +0000 Subject: [PATCH] docs: review document for #520 Issue: #520 User-Visible: no --- docs/reviews/SPEC-REVIEW-520-r1.md | 280 +++++++++++++++++++++++++++++ 1 file changed, 280 insertions(+) create mode 100644 docs/reviews/SPEC-REVIEW-520-r1.md diff --git a/docs/reviews/SPEC-REVIEW-520-r1.md b/docs/reviews/SPEC-REVIEW-520-r1.md new file mode 100644 index 00000000..431da59f --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-520-r1.md @@ -0,0 +1,280 @@ +# SPEC-REVIEW-520-r1 + +## Якоря + +- Issue: [#520](https://github.com/Matysh/houseplan-card/issues/520) + «Первый устойчивый кадр большого дома вырос до 3.36 с при потолке 3.0 с — + регрессия после #509/#500», метка `S4-spec-review` (подтверждено `gh issue + view 520 --json labels`, 2026-09-10). +- ТЗ: тело issue #520, раздел `## ТЗ (полный трек — критерий §5 ...)`, + строки 30–67 полученного тела (см. ниже). +- sha256 нормализованного тела issue (как получено `gh issue view 520 + --repo Matysh/houseplan-card --json body -q .body`, без дополнительной + нормализации переводов строк): + `5477d052e0e87182f51a9b3b05d37fce0a7ada3d9dc66c10f927f64564ccc6e4` +- Материал ветки: `issue/520-first-slug` → фактически `issue/520-first-frame` + на `27ea23c1ec25e6421542c9901eb6e8e2484d0665` — по собственному признанию + автора в комментарии `2026-09-10T15:09:06Z`: «пока только диагностика + бенчмарка, продуктовой правки в ней ещё нет; она появится после + `S5-ready`». Подтверждено чтением `git log`/`git show --stat` на трёх + последних коммитах — все три с префиксом `test(perf):`, правок в + `src/**` нет. +- Заход r1. Бюджет циклов §4 до этого ревью: 0/4. Вердикт ниже — + жёлтый, поэтому расходует цикл: после этого ревью бюджет **1/4**. + +**Примечание по маршрутизации.** Шапка задания содержит блок инструкций +для этапа `code` (диапазон SHA, требование не делать `git fetch`, +`Зелёного Validate на этом SHA нет` и т.д.) и фразу «Для этапа spec: ТЗ +живёт в теле issue (#517)», где `#517` — не опечатка про номер текущей +задачи, а ссылка на решение владельца от 2026-09-10 (issue #517), +которым сам архив `docs/specs/` закрыт для новых ТЗ (см. PROCESS.md +§2.3, строка 125). Реальный этап подтверждён меткой `S4-spec-review` на +#520 — далее в разборе применяются правила §2.4/§7.1, а раздел про +`git diff origin/dev...HEAD`/SHA к этому раунду не относится: код +продукта ещё не менялся. + +## Скоуп ревью + +Ревью ТЗ (PROCESS.md §2.4) для полного трека — задача сама называет +нарушенный критерий лёгкого трека («нет влияния на производительность»), +выбор трека верен. Предмет: раздел `## ТЗ` тела issue #520 — контракт, +AC1…AC5, откат, release-артефакты — против §7.1 (обязательные разделы), +§2.5 (DoR) и `docs/SCOPE.md` (продуктовая рамка). + +## Как проверялось + +1. Прочитано тело issue #520 целиком и оба комментария (`S2-анализ`, + «Причина найдена…», переход в `S3-spec`) — понадобилось для контекста + диагностики, на которой стоит контракт. +2. Сверено с PROCESS.md §1 (класс A), §2.3–2.5, §4, §5, §7.1. +3. Сверено с `docs/SCOPE.md`: персона/поверхность ТЗ (администратор, + первый показ большого дома) сопоставлены с J1 и таблицей Target + audience («large flat, 20–200 devices»; фикстура 60 комнат/200 + устройств — то же самое). +4. **Технические утверждения о поведении Lit перепроверены чтением + исходника**, а не приняты на веру — это и есть весь фундамент + контракта: + - `node_modules/@lit/reactive-element/development/reactive-element.js:249-252` + — `wrapped = true` действительно ставится в `createProperty`, когда + `this.prototype.hasOwnProperty(name)`, **до** проверки `noAccessor` + (строка чуть ниже `if (!options.noAccessor)`) — подтверждает пункт 2 + контракта дословно; + - `:880-886` — принудительная запись `wrapped`-свойства в + `changedProperties` со значением `undefined` действительно происходит + внутри блока первого обновления (`hasUpdated` ещё не выставлен; + комментарий в самом Lit: «only for the case of properties created via + `createProperty` on accessors») — подтверждает механизм лишней эпохи; + - `:339-341` (`getPropertyOptions`) и `:697-712` (`requestUpdate`) — + необъявленное имя свойства действительно получает + `defaultPropertyDeclaration`, сравнивается через `notEqual` и уходит в + `_$changeProperty` наравне с объявленным — подтверждает пункт 1 + контракта («`requestUpdate` работает с необъявленным именем»). +5. Цитаты по `src/houseplan-card.ts` сверены построчно с текущим файлом: + аксессоры `_layout`/`_serverCfg` (923–940), `static properties` со + `_layout`/`_serverCfg: { state: true }` (2556, 2560), `willUpdate` и + `_cfgEpoch++`/`preserveGeometry` (4104–4114), ключ памятки модели + `` `${_cfgEpoch}|${_cfgFingerprint()}` `` (3919), ключ + `cache.entries.cleanFloor` `` `${space.id}|${_cfgEpoch}|${roomKey}` `` + (9916) — все точны, расхождений с текстом ТЗ нет. +6. Оба файла бюджетов из AC5 (`demo/performance/budgets-interaction-smoke.json`, + `demo/performance/budgets-large-house-interaction.json`) прочитаны: + `cleanFloor: 180` в обоих подтверждён, других budgets-файлов с + `cleanFloor` в дереве нет — список AC5 полон и точен. Дополнительно + сверен коммит `914e8402`, поднявший потолок: его `git show --stat` + трогает ровно эти два файла. +7. Пересчитаны относительные лимиты `modelReadyMs` (формула + `demo/performance/evaluate.mjs:73-118`, `min(hardMaxMs, max(base*(1+ratio), + base+noiseAllowanceMs))`) для профилей `large-house-interaction` **и** + `large-house-isometric`, чтобы понять, действительно ли изометрия тоже + нарушает свой бюджет (см. находку Medium-3) — числа изометрии из + «Симптома» не были проверены автором на предмет реального превышения, + я это сделал сам. +8. Проверено содержимое `test/config-adoption-ownership.test.mjs` (техника + — регэксп-скан исходников на предмет прямой записи identity/полей вне + владельца) и `test/config-adoption.test.mjs` (host-стаб, юнит без + реального Lit-компонента) — чтобы оценить, насколько реалистичны + заявленные способы доказательства AC1/AC2. +9. Проверено, куда пишет диагностика `bootDiag` + (`demo/benchmark_large_house.mjs:401,1142,1213-1218`) — только + `console.log` строки прогона, не бюджетируемое поле — это и есть + основание находки Medium-2. +10. `git status` — рабочая копия чистая относительно продукта (кроме + отсутствующего `node_modules`, не связано с этим ревью). + +## Находки + +### Medium-1 — обязательный раздел «Риски» отсутствует (§7.1) + +**Файл:** тело issue #520, раздел `## ТЗ` (строки 30–67 полученного тела). + +`PROCESS.md:535-538` перечисляет обязательные разделы ТЗ, включая +**риски**; DoR (`PROCESS.md:164`) отдельно требует «риски перечислены» +как условие перехода в «Готово к разработке». В тексте ТЗ #520 такого +раздела нет вовсе — ни явного заголовка, ни даже строки вида «риски: +минимальны, потому что …». Для сравнения — прямой прецедент того же +класса задачи, `docs/specs/506-startup-performance.md:149-161` +(«Техническая гипотеза, риски и откат»), перечисляет риски явным +списком («двойное подключение при overlapping promises…», «исправление +только glow без устранения isometric regression» и т.д.) и привязывает +каждый к AC, который его закрывает. + +**Сценарий, который упущен без раздела риска.** Контракт снимает +`_serverCfg`/`_layout` с объявленных реактивных свойств Lit. Реальный +риск есть и не назван: любой код, читающий +`houseplan-card.elementProperties` — Lit devtools, будущий рефлектор +атрибутов, сторонний код, полагающийся на `hasOwnProperty` в +`options`/`reflect` — перестанет видеть эти два поля в перечне +реактивных свойств. Ничего в контракте не говорит, проверено ли +отсутствие такого потребителя (или почему он невозможен) — риск +следовало либо назвать и снять, либо принять с обоснованием. + +**Чем чинится:** одним абзацем в теле issue — не блокирует исполнимость +контракта, но обязателен для DoR. + +### Medium-2 — AC3 не имеет дешёвого воспроизводимого способа доказательства + +**Файл:** тело issue #520, строка 56 (`AC3` в таблице AC). + +DoR (`PROCESS.md:154-155`) требует, чтобы у каждого AC был назван способ +доказательства из набора `unit`/`backend`/`smoke`/`golden`/«ревью кода». +AC1 (source-scan тест по образцу уже существующего +`test/config-adoption-ownership.test.mjs`), AC2 (юнит), AC4/AC5 (уже +существующие budgeted-гейты Validate) — все аккуратно попадают в этот +набор. **AC3 — нет.** Его «чем доказан» — «диагностика `bootDiag` в +полном сравнении против `a44fbd37`, числа в комментарии issue»; «чем +краснеет» — «лишняя эпоха вернётся — числа разойдутся с базой». + +Проверено чтением `demo/benchmark_large_house.mjs:1213-1218`: `bootDiag` +только печатается в лог прогона (`console.log`) и явно исключён из +бюджетируемой записи — сам текст ТЗ подтверждает это разделом «Принято +предположительно» («печатается в лог прогона, в бюджетируемую запись не +попадает»). Значит AC3 нельзя перепроверить иначе, чем вручную читая лог +дорогого прогона `Full Performance` (семь образцов, сравнение с базой) — +ровно того класса гейтов, который по прямому указанию в задании этого +ревью «предрелизный, а не гейт ревью». Автор и на этом ревью, и на +будущем код-ревью вынужден либо доверять уже вставленным в issue числам +без способа их независимо переснять дёшево, либо вручную гонять полное +сравнение при каждом цикле. + +По существу AC3 не даёт ничего, чего не даёт уже AC4 (тот же корень +регрессии закрывает и хронометраж, и счётчик эпох) — это не лишний +критерий, а лишний **недоказуемый** критерий. Не блокирует контракт: +вариантов чинения минимум два и оба дёшевы — либо явно понизить AC3 до +«проверено чтением кода при код-ревью (AC1+AC2 логически влекут AC3) + +однократная сверка `bootDiag` на конкретном SHA перед публикацией беты, +не гейт CI», либо убрать AC3 из таблицы как поглощённый AC1/AC2/AC4 и +оставить числа диагностики просто иллюстрацией в тексте причины. + +### Medium-3 — регрессия профиля изометрии показана в «Симптоме», но не отражена в контракте/AC + +**Файл:** тело issue #520, строка 3 («Профиль изометрии — то же: +`modelReadyMs` 2165.6 → 2626.3») против раздела «Контракт»/«AC и +доказательства» (строки 38–58), где изометрия не упомянута вовсе. + +Автор сам приводит эти числа как «то же» (то есть тот же механизм +регрессии) для профиля `large-house-isometric`, но ни один AC, ни +скоуп/не-скоуп не называют этот профиль. Проверил сам, чтобы понять, +пустая это формальность или реальный пропуск: у профиля +`large-house-isometric` (`demo/performance/budgets-large-house-isometric.json:6`) +`maxRegressionRatio` для `modelReadyMs` — 0.2, `noiseAllowanceMs` — 200. +По формуле гейта (`demo/performance/evaluate.mjs:73-118`, +`relativeLimit = max(base*(1+ratio), base+allowance)`) эффективный потолок +— `max(2165.6*1.2, 2165.6+200) = 2598.72`. Кандидат **2626.3 > 2598.72** — +изометрия **тоже** нарушает свой относительный бюджет (на ~1 %, у +`large-house-interaction` — на ~7 %), просто это не абсолютный гейт +Validate и потому не горит красным в CI сейчас, точно как и обсуждаемый +в AC4 `modelReadyMs` профиля interaction. + +Раз ТЗ уже включает в AC4 покрытие «мягкого» (не абсолютного, а +относительного) регресса `modelReadyMs` для одного профиля, тот же +стандарт по симметрии должен применяться и ко второму профилю, который +сам автор привёл как доказательство единого механизма. Сейчас неясно: +это фиксится тем же изменением автоматически и просто не нуждается в +отдельном AC (правдоподобно, раз причина общая для любого первого +обновления карточки), или это осознанно оставлено вне скоупа задачи — +но текст ТЗ не говорит ни того, ни другого явно. Это техническая +неопределённость (не продуктовая — какая часть исправления какой профиль +чинит, решают агенты между собой, не владелец), поэтому вопрос не выносится +владельцу, а возвращается автору как находка: либо расширить AC4/AC5 +явным упоминанием `large-house-isometric` (и его budgets-файлов, если они +регрессировали похожим образом по `cleanFloor`), либо одной фразой +зафиксировать «то же изменение чинит и профиль изометрии, отдельного +гейта не заводим, потому что…». + +## Что проверено и корректно + +- **Причина — измерена, не догадка.** Все три опорных технических + утверждения о поведении Lit (`wrapped`, форсированная запись в + `changedProperties` на первом апдейте, работа `requestUpdate` с + необъявленным именем) подтверждены чтением фактического исходника + `@lit/reactive-element`, а не только словами автора. Все цитаты по + `houseplan-card.ts` (номера строк) сверены с текущим файлом и точны. +- **AC1** доказуем дёшево и детерминированно: тот же приём (regex-скан + исходников), что уже применяется в этом же тестовом файле для + соседнего инварианта — реалистичный, недорогой способ доказательства с + чётким «чем краснеет» (мутант возвращает `_serverCfg: { state: true }`). +- **AC2** корректно опирается на уже проверенное (мной, чтением исходника + Lit) поведение — сам механизм гарантирован фреймворком, тесту остаётся + подтвердить, что `onBodyReplaced` вызывается на замену и не вызывается + на идентичном значении; это не новая инфраструктура. +- **AC4/AC5** привязаны к точным существующим файлам бюджетов; значения + (потолок 180 → 100, `hardMaxMs`/`maxRegressionRatio` для + `modelReadyMs`) подтверждены построчно и соответствуют тому, что + реально сейчас нарушено (и по абсолютному, и по относительному гейту). +- **Откат** — простой и однозначный (revert двух строк `static + properties` + потолка 180), данные и контракты не затронуты. +- **Release-артефакты** корректно называют `User-Visible: no` и + отсутствие правки changelog, с указанием, какой бета-кандидат + разблокирован. +- **Продуктовая рамка** называет персону и поверхность (`docs/SCOPE.md` + J1, «Home admin», «large flat… 20–200 devices») и совпадает с фикстурой + (60 комнат/200 устройств); «что человек увидит» сформулировано одной + фразой без раздутия. +- **Выбор полного трека корректен**: perf-влияние безоговорочно нарушает + критерий лёгкого трека §5, и автор сам это называет. +- **Ни одного вопроса владельцу не задано** — и не нужно: весь материал + технический, продуктовых развилок в задаче нет. +- **«Принято предположительно»** оформлено ровно по формату §7.1 + (сохранение диагностики `bootDiag` в дереве помечено как решение, + которое ревьюер вправе оспорить) — не поглощено находками выше. + +## Чего не проверял + +- **Гейты кода** (`tsc --noEmit`, `npm test`, `npm run build`, + `check-docs.mjs`) — не гонял: на этом SHA нет ни строки продуктового + кода (`src/**`), только диагностика бенчмарка; предмет этапа spec — + текст ТЗ, а не код. Это будет предметом код-ревью после `S5-ready`. +- **`npm run invariants`** — не гонял по той же причине: геометрия и + ссылки на неё не тронуты. +- **Реальный прогон `Full Performance`/`Validate`** на этом SHA — не + запускал: продуктовой правки, которую можно было бы измерить, ещё нет. +- **Регистрацию мутанта `adoption-bodies-declared-reactive`** в + `scripts/mutation-gate.mjs` — не проверял; на этапе ТЗ его ещё не + существует по определению (мутант тестирует ещё не написанный код), + это предмет код-ревью. +- **Уровень шума** в уже приведённых числах `Full Performance` (7 + образцов) — не переоценивал статистически; принял как измеренные, со + ссылками на конкретные прогоны CI, приведённые автором. + +## Унаследовано / закрытие предыдущего раунда + +Не применимо — первый раунд (r1) ревью ТЗ #520. + +## Вердикт + +Жёлтый. High: 0, Medium: 3 (все в скоупе задачи — чинятся автором в том +же тексте ТЗ, без нового issue). Возврат в «ТЗ в работе» (`S3-spec`). + +--- + + + +## Материал раунда + +- Ветка: `issue/520-first-frame`, коммит `27ea23c1ec25` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `f80bcdd58b72ba2e7b38cfaac3e80e978cbe67c9` + ``` + git log --all --format='%H %T' | grep f80bcdd58b72 + ``` +- Тело issue: `d370780d07b6217cfaa48aa21213828fc3a6a70ec16dbea7fee15dbf00e22871` +- Вердикт конвейера: `yellow` · High 0