From e249e3dbc27857a5f1e349d40a9034e72ed95016 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Sat, 19 Sep 2026 07:43:58 +0000 Subject: [PATCH] docs: review document for #596 Issue: #596 User-Visible: no --- docs/reviews/CODE-REVIEW-596-r1.md | 146 +++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-596-r1.md diff --git a/docs/reviews/CODE-REVIEW-596-r1.md b/docs/reviews/CODE-REVIEW-596-r1.md new file mode 100644 index 00000000..14376aa0 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-596-r1.md @@ -0,0 +1,146 @@ +# Код-ревью #596 — заход r1 + +Материал: `337d85ea4c40db8308e35caac4303254c0874306` (ветка `issue/596-merge-candidate-maxbuffer`, один коммит). +Класс B: `scripts/merge-candidate.mjs`, `test/merge-candidate.test.mjs`, `scripts/mutation-registry.mjs`. Файлов класса A нет. +Трейлеры коммита: `Issue: #596`, `User-Visible: no` — верно, изменение внутреннего инструмента конвейера, продуктового поведения не касается. + +## Скоуп задачи + +`sh()` в `scripts/merge-candidate.mjs` вызывал `spawnSync` без `maxBuffer` (умолчание 1 МиБ). +Шаг слияния считает patch-id материала и кандидата через `git diff --full-index`, когда `dev` +сдвинулся за время ревью; дифф задачи, пересобирающей бандл, несёт три копии +`houseplan-card.js` — воспроизведено на #594 (7 103 616 байт). Процесс убивался по ENOBUFS, +`status` становился `null`, а `status: r.status ?? 1` выдавало это за «git вернул 1» — с +усечённым `stdout` в сообщении об ошибке (`must()` берёт `stderr || stdout`, stderr был пуст). +Задача — issue #596, инфраструктурная, класс B, входит в флоу на `S7-code-review` сразу (#562). + +## Как проверялось + +Дешёвые гейты (`npx tsc --noEmit`, `npm test`, `npm run build`) на этом SHA уже зелёные — +Validate https://github.com/Matysh/houseplan-card/actions/runs/35429741728, повторно не гонял. +`node scripts/check-docs.mjs` не нужен: diff не трогает `src/**` (только `scripts/**` и +`test/**`). Инварианты модели не нужны: diff не трогает геометрию, `layout`, `marker.space`, +`open_spans`, толщину стен. Браузерные смоки не нужны: смоки исполняют собранный бандл и DOM +(`src/**`/`demo/**`), а диффа там нет — смок-раздел `smoke-select.mjs` рассчитан на связь с +изменённым исполняемым символом фронтенда, здесь такого символа нет в принципе (правка Node-скрипта +CI, не фронтенда). `golden:verify`, `pytest tests_backend`, перф-профили — не применимы, +не тронуты соответствующие пути. + +Ручная проверка (то, что Validate не покрывает и что специфично для этой задачи): + +1. Прочитал итоговый `scripts/merge-candidate.mjs`: `sh()` теперь передаёт + `maxBuffer: MAX_COMMAND_OUTPUT_BYTES` (256 МиБ) первым в объекте опций, `opts` может его + переопределить (порядок спреда `{ encoding, maxBuffer, ...opts }` корректен — опции + вызова, если есть, побеждают дефолт, как и раньше для остальных полей). `r.error` теперь + читается: причина (`ENOENT`, `ENOBUFS`, таймаут) уходит в `stderr` результата, и при её + наличии `status` принудительно `1` (не полагается на `r.status ?? 1`, которое для + провала запуска всегда `null`). +2. Прогнал целевые тесты изолированно: + `node --test --test-name-pattern="#596" test/merge-candidate.test.mjs` → 2/2 pass. +3. Дисциплина «тест умеет падать» — не поверил заявлению автора, воспроизвёл мутацию вручную + импортом модуля и вызовом `sh()` с `maxBuffer: 1024*1024` (то же, что делает мутант + `merge-candidate-truncates-the-candidate-diff` в `scripts/mutation-registry.mjs`) на + 3 МиБ вывода: + ``` + status 1 stderr "…/node не выполнился: ENOBUFS" stdoutLen 1114112 + ``` + Тест AC1 (`assert.equal(r.status, 0, …)`) в этом состоянии красный, как и заявлено. Причём + само сообщение об ошибке называет `ENOBUFS`, а не отдаёт усечённый `stdout` — это и есть + AC2 в действии на конкретном сценарии дефекта. +4. Проверил, что `scripts/mutation-registry.mjs` синтаксически корректен и грузится + (`node -e "import('./scripts/mutation-registry.mjs')"` → ok), новый элемент массива не + ломает соседние (`declared-baseline-review-run-never-checked` идёт следом без искажений), + `id` уникален по файлу. +5. Проверил, что экспорт `sh` не используется больше нигде в `scripts/*.mjs` за пределами + `merge-candidate.mjs` — расширение публичного API файла не создаёт скрытых потребителей и + не меняет поведение `realOps` (сигнатура `sh` не поменялась, только опции по умолчанию и + структура `stderr`). +6. Просмотрел `must()`: `stderr || stdout` — при заполненном `stderr` (наш случай после + исправления) в исключение уходит причина, а не дифф. Логика согласована с текстом AC2. + +## AC — построчно + +- **AC1** (вывод >1 МиБ доезжает целиком, `status: 0`) — доказано автотестом + `test/merge-candidate.test.mjs:397-404`, тест реальный (гоняет процесс, не заглушку), + падение при откате правки подтверждено вручную (см. п.3 выше). **Выполнено.** +- **AC2** (сбой запуска называет причину в `stderr`, `must()` печатает её) — доказано + автотестом `test/merge-candidate.test.mjs:406-413` и чтением `must()` + (`scripts/merge-candidate.mjs:134`, `${what}: ${r.stderr || r.stdout}`). **Выполнено.** +- **AC3** (существующие тесты остаются зелёными, подмена `exec` не ломается) — сигнатура + `sh(cmd, args, opts)` и форма возврата `{status, stdout, stderr}` не изменились, только + дефолт `maxBuffer` и заполнение `stderr` при `r.error`; `realOps` конструирует `git`/`exec` + через тот же `sh` по умолчанию, тестовые фикстуры подменяют `exec` целиком и его не + затрагивают. Подтверждено прогоном Validate на этом SHA (2775/2776, 1 skip — без + регрессии) и точечным прогоном новых тестов. **Выполнено.** +- **AC4** (мутант снимает `maxBuffer`, AC1 краснеет) — мутант + `merge-candidate-truncates-the-candidate-diff` (`scripts/mutation-registry.mjs:9940-9951`) + возвращает `maxBuffer` к `1024*1024`; воспроизвёл эффект вручную (п.3) — AC1 действительно + краснеет, и вдобавок причина в этом состоянии верно называется `ENOBUFS`, а не молчит. + **Выполнено.** +- **AC5** (`npm test`, `npm run typecheck`, `npm run build` зелёные) — покрыто Validate на + этом же SHA (ссылка выше), повторный прогон не требовался. **Выполнено.** + +## Продуктовое рассуждение (docs/SCOPE.md) + +Задача не создаёт и не меняет пользовательского поведения; это инструмент самого ревью- +конвейера (§2.4/§2.9 PROCESS.md, класс B). К Core user jobs (J1–J7) прямого отношения нет — +она инфраструктурная и входит в флоу по отдельному правилу (#562), что и отражено в issue. +Ничего в SCOPE.md эту работу не ограничивает и не блокирует. + +## Находки + +Нет ни одной находки уровня High или Medium. Мелочей уровня Low, которые стоило бы отметить, +тоже не нашёл: код лаконичен, комментарии по существу объясняют «почему», тесты +целенаправленные и действительно проверяют регрессию, а не пересказывают код. + +## Что проверено и корректно + +- Причина исходного дефекта (ENOBUFS на 1 027 868 байт, тот же обрыв, что в комментарии о + сбое #594) устранена явным `maxBuffer` с большим запасом (256 МиБ) — обоснование запаса + («переживает рост бандла и задачу, которая трогает бандл и эталоны одновременно») + разумно для CI-скрипта, не тянет памяти сверх нормы для однократного вызова `git diff`. +- Вторая половина дефекта (сокрытие `r.error`) устранена: `must()` теперь получает причину, + а не огрызок диффа, который «уводит разбор в сторону» — именно то, что случилось на #594. +- `sh` экспортирован и используется в тестах напрямую (не только через `realOps`), так что + оба AC имеют исполнимого свидетеля, а не рассуждение автора. +- Мутационный тест зарегистрирован по тем же соглашениям, что и соседние записи в файле + (`guard`, `because`, `patches`), синтаксис файла не нарушен. +- Скоуп изменения не выходит за пределы заявленных трёх файлов, продуктовый код не тронут. + +## Чего не проверял и почему + +- `npx tsc --noEmit`, `npm test` (полный), `npm run build` со сверкой бандла — не гонял + повторно: зелёный Validate уже есть на этом точном SHA (ссылка выше), диффа не по бандлу и + не по типам, что подтверждается локальным точечным прогоном тестов файла. +- `node scripts/check-docs.mjs` — diff не трогает `src/**`, гейт не устареет от этой правки. +- `npm run invariants` — diff не трогает геометрию/толщину стен/`layout`/`marker.space`. +- Браузерные смоки (`demo/smoke_*.mjs`) — diff не трогает фронтенд/бандл/DOM, смоки его в + принципе не исполняют; `smoke-select.mjs` не запускал, так как предмет диффа (Node-скрипт + CI-инструмента) вне домена, который инструмент оценивает. +- `npm run golden:verify` — diff не может изменить видимый рендер. +- `python -m pytest tests_backend -q` — `custom_components/**/*.py` не тронут. +- Перф-профили — не названы в AC, чувствительные к перфу пути не тронуты. +- `node scripts/process-gate.mjs --issues` — автор отметил недоступность `gh` в песочнице; + это гейт публикации/CI, а не код-ревью, отдельно не гонял. + +## Вердикт + +Все пять AC выполнены и доказаны исполнимыми тестами, один из которых (мутационный) я +воспроизвёл вручную и убедился, что он действительно ловит регрессию, а не просто +существует. Скоуп не нарушен, продуктовый код не затронут, трейлеры корректны. Находок нет. + +**Зелёный.** + +--- + + + +## Материал раунда + +- Ветка: `issue/596-merge-candidate-maxbuffer`, коммит `337d85ea4c40` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `24ca27d21d24b5d66aba292f22d8e5aafd51c074` + ``` + git log --all --format='%H %T' | grep 24ca27d21d24 + ``` +- Тело issue: `9b589ac902b977dac360fa27b962cac1d90e89020e3987a38de00c4aae14c2df` +- Вердикт конвейера: `green` · High 0