houseplan-card/docs/reviews/CODE-REVIEW-596-r1.md

Код-ревью #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