mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-03 21:28:59 +00:00
claude[bot]
@@ -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 выполнены и доказаны исполнимыми тестами, один из которых (мутационный) я
|
||||
воспроизвёл вручную и убедился, что он действительно ловит регрессию, а не просто
|
||||
существует. Скоуп не нарушен, продуктовый код не затронут, трейлеры корректны. Находок нет.
|
||||
|
||||
**Зелёный.**
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/596-merge-candidate-maxbuffer`, коммит `337d85ea4c40` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `24ca27d21d24b5d66aba292f22d8e5aafd51c074`
|
||||
```
|
||||
git log --all --format='%H %T' | grep 24ca27d21d24
|
||||
```
|
||||
- Тело issue: `9b589ac902b977dac360fa27b962cac1d90e89020e3987a38de00c4aae14c2df`
|
||||
- Вердикт конвейера: `green` · High 0
|
||||
Reference in New Issue
Block a user