From c80f478df9802ac65e67b7908ed4fb830d6bf662 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 24 Sep 2026 01:16:27 +0000 Subject: [PATCH] docs: review document for #632 Issue: #632 User-Visible: no --- docs/reviews/CODE-REVIEW-632-r1.md | 231 +++++++++++++++++++++++++++++ docs/reviews/INDEX.md | 1 + 2 files changed, 232 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-632-r1.md diff --git a/docs/reviews/CODE-REVIEW-632-r1.md b/docs/reviews/CODE-REVIEW-632-r1.md new file mode 100644 index 00000000..953b6e15 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-632-r1.md @@ -0,0 +1,231 @@ +# CODE-REVIEW-632-r1 + +Issue: #632 · этап: код-ревью · заход r1 · блокирующих циклов израсходовано 0 из 4 +Трек: инфраструктурный (ускоренный вход, AGENTS.md §1 / PROCESS.md §1) — весь +дифф класса B, ни одного файла класса A. +Материал: `git log --oneline origin/dev..HEAD` = один коммит `a775080a` +(«task-packet: track follows issue status and history, not a class-A-free +diff»), рабочая копия уже на нём (detached HEAD). `git diff origin/dev...HEAD`: +`scripts/task-packet.mjs`, `test/task-packet.test.mjs`, +`scripts/mutation-registry.mjs` — 3 файла, +122/-6. + +## Скоуп + +Баг: `scripts/task-packet.mjs` классифицировал ветку продуктовой S6-задачи как +«инфраструктурную» и печатал «файлы класса A трогать НЕЛЬЗЯ», если текущий +diff к `origin/dev` ещё не содержал ни одного файла класса A — реальный кейс +#607, чья ветка в момент воспроизведения несла только +`docs/reviews/SPEC-REVIEW-607-r1.md`. Ожидаемое поведение по телу issue: +«продуктовый S-flow и статус S5/S6/S7 должны сохранять право менять class A +до появления первого такого файла». + +Правка вводит `branchIsInfrastructure()` (документы `docs/reviews/**` больше +не считаются материалом ветки) и `productFlowEvidence()` (статус S1–S5, +раздел `## ТЗ` в теле issue, файл `docs/specs/NN-*`, документ или вердикт +ревью ТЗ) — при любом из этих признаков эвристика «diff без класса A → +инфраструктура» не применяется. + +## Как проверялось + +Дешёвые гейты подтверждены зелёным Validate на этом же SHA `a775080a` +(https://github.com/Matysh/houseplan-card/actions/runs/35941025607) — по +инструкции раунда не перегонялись повторно: `npx tsc --noEmit`, `npm test`, +`npm run build` + сверка бандла. Diff не касается `src/**`, геометрии, +`open_spans`, `marker.space` → `check-docs.mjs` и `model-invariants.mjs` не +применимы. Смоки/golden/pytest/perf — diff их не касается (чистый +`scripts/**`+`test/**`, `src/**` не менялся). + +Что прогнал сам, целенаправленно по AC и по защитным свойствам: + +| Команда | Результат | +|---|---| +| `node --test test/task-packet.test.mjs` | 13/13 ok | +| `node scripts/mutation-gate.mjs --check` | `task-packet-product-flow-overrides-diff` ok, `task-packet-review-docs-not-material` ok, `task-packet-s6-s7-alone-not-product-flow` ok (анкоры валидны) | +| `node scripts/mutation-gate.mjs --id=task-packet-product-flow-overrides-diff` | поймано 1 из 1 | +| `node scripts/mutation-gate.mjs --id=task-packet-review-docs-not-material` | поймано 1 из 1 | +| `node scripts/mutation-gate.mjs --id=task-packet-s6-s7-alone-not-product-flow` | поймано 1 из 1 | + +Три мутанта реально красят ровно заявленный тест и не красят посторонние — +защитные AC1–AC3 доказаны по правилу «чем краснеет» (§2.7), не только +заявлением автора. + +Дополнительно (ручной разбор по коду, не мутацией — проверка гипотезы о +непокрытой ветке трека): вызвал `buildPacket()` напрямую с входами, +имитирующими реальный **`trivial`**-трек (§5.1) продуктовой задачи — +- синтетический кейс: `labels: ['bug','P2','S6-in-progress','trivial']`, + `branch.infrastructure: true`, тело issue с обычными `AC1./AC2.` без + заголовка `## ТЗ` (trivial-трек ТЗ не пишет вовсе, §5.1) → `track` остаётся + `'инфраструктурный'`, `rights` содержит «файлы класса A трогать НЕЛЬЗЯ»; +- то же самое воспроизвёл на **реальном** закрытом trivial-issue #612 (метки + `bug, P2, trivial`, тело — `## Дефект 1` / `## Дефект 2` / `## AC`, без + `## ТЗ`, без `docs/specs` файла, без `SPEC-REVIEW` документа/вердикта) — + `buildPacket` с тем же телом и `S6-in-progress` даёт тот же ложный + инфраструктурный запрет. + +## Находки + +### Medium (в скоупе задачи) — `trivial`-трек не входит в признаки продуктового потока + +**Файл:** `scripts/task-packet.mjs:127-143` (`PRE_CODE_STATUSES`, +`productFlowEvidence`) + +**Симптом:** `productFlowEvidence()` ищет ровно четыре признака: статус +S1–S5, заголовок `## ТЗ` в теле issue, файл `docs/specs/NN-*`, документ или +вердикт ревью ТЗ. Все четыре предполагают, что задача писала спецификацию. +Короткий трек (`trivial`, PROCESS.md §5.1) сознательно эту стадию пропускает +целиком: «Маршрут: `S1-new` → `S2-analysis` → `S5-ready` → `S6-in-progress` +→ …, минуя `S3-spec` и `S4-spec-review`» и «AC пишет автор в теле issue при +переводе в `S5-ready`» — без раздела `## ТЗ`, без файла в `docs/specs`, без +документа ревью ТЗ. Для такой задачи, как только она проходит `S5-ready` (то +есть как раз в `S6-in-progress`/`S7-code-review`, где и предполагается вызов +`task-packet`), `productFlowEvidence` возвращает пустой список тем же +образом, что и для настоящей ускоренной инфраструктурной задачи — их +неотличить. Итог: `branchIsInfrastructure(...)===true` (диффа пока нет +файлов класса A — обычная фаза «тесты/фикстуры раньше src») снова печатает +«файлы класса A трогать НЕЛЬЗЯ» для реальной продуктовой trivial-задачи — +это в точности симптом #607, воспроизведённый для целого документированного +трека, который в тестах задачи (`AC1…AC4` из хендоффа) не участвует ни разу: +`AC3` намеренно проверяет случай «инфраструктурная задача без ТЗ» теми же +метками `['infra', 'S6-in-progress']` / `['infra', 'S7-code-review']` — +без метки `trivial`, поэтому конфликт между «настоящая инфраструктурная +задача без ТЗ» и «настоящая trivial-задача без ТЗ» тестами не различается и +не покрыт. + +**Воспроизведение (проверено чтением + прямым вызовом `buildPacket`, не +исполнением всего скрипта — `gh` в песочнице недоступен, как и у автора):** + +```js +const packet = buildPacket({ + issue: { number: 612, title: '…', state: 'CLOSED', url: 'u', + body: '## Дефект 1\n…\n## Дефект 2\n…\n## AC\n- AC1. …\n- AC2. …\n- AC3. …' }, + labels: ['bug', 'P2', 'trivial', 'S6-in-progress'], + branch: { name: 'issue/612-x', tip: 'e'.repeat(40), base: 'f'.repeat(40), + ahead: 1, behind: 0, treeWithoutReviews: null, infrastructure: true }, +}); +// packet.track === 'инфраструктурный' +// packet.rights содержит 'файлы класса A трогать НЕЛЬЗЯ; …' +``` + +#612 — реальный, закрытый trivial-баг (не синтетика): его тело подтверждает, +что trivial-задачи не несут `## ТЗ`. + +**Почему не High:** `task-packet.mjs` — советующий инструмент («пакет ничего +не пишет и ничего не решает», комментарий в шапке файла), а не гейт CI; +ошибочная строка вводит в заблуждение читающего агента, но не блокирует и не +портит продуктовый код напрямую. Окно срабатывания уже (нужен push +trivial-ветки, чей текущий diff ещё не содержит класса A) из-за WIP-лимита 1 +и «push один раз по готовности», но метки `trivial` подтверждено +существуют и используются на практике (нашёл 10 issue с меткой `trivial`, +#612 — предметный пример) и не являются нишевым — это второй из двух +регулярных продуктовых треков. + +**Почему в скоупе задачи, а не отдельный issue:** тот же файл, та же функция +(`productFlowEvidence`), тот же класс дефекта, который #632 и должен закрыть +— «продуктовый S-flow… должен сохранять право менять class A» из тела +issue не содержит оговорки «кроме trivial». Решение владельца 2026-08-19 +(#202): Medium в скоупе чинится в этой же задаче. + +**Предлагаемое (не обязывающее автора) направление:** добавить +`labels.includes('trivial')` (по аналогии — стоит проверить и не помешает ли +это тому, что `trivial`+`infra` тематическая метка одновременно +встречается на реальных issue вроде #538/#467-470 — там `trivial` явно +описывает продуктовый short-track, а не ускоренный infra-вход, что я +проверил чтением PROCESS.md §1 и §5.1: ускоренный infra-вход не имеет +понятия трека `trivial`/`small` вовсе, это метки исключительно продуктового +S-flow) в список признаков `productFlowEvidence`, плюс тест на кейс +`['trivial', 'S6-in-progress']` и мутант, ловящий откат. + +### Что проверено и корректно + +- **AC1** (продуктовая S6-задача с ТЗ/ревью ТЗ сохраняет право класса A) — + доказано `test/task-packet.test.mjs` › `#632: product S6 issue keeps class + A rights…`, мутант `task-packet-product-flow-overrides-diff` красит именно + этот тест (проверено прогоном). +- **AC2** (`docs/reviews/**` не в материале ветки) — доказано `#632: review + documents never classify a branch as infrastructure`, мутант + `task-packet-review-docs-not-material` (проверено прогоном). Отдельно + проверил чтением: `branchIsInfrastructure` фильтрует по префиксу + `docs/reviews/` до вызова `classify`, поэтому вложенные пути + (`docs/reviews/legacy/…`, если появятся) тоже безопасны. + По исходному репро #607 (ветка содержит **только** `SPEC-REVIEW-607-r1.md`) + `branchIsInfrastructure` возвращает `false` уже на уровне `material.length + === 0` — сам факт, что #632 закрывается даже без `productFlowEvidence` для + этого конкретного случая; `productFlowEvidence` расширяет защиту на менее + тривиальные ситуации (например, ветка уже содержит один файл класса B — + тест/фикстуру — раньше первого файла класса A). +- **AC3** (настоящая инфраструктурная задача без ТЗ, включая возврат в S6 и + стояние на S7, сохраняет запрет) — доказано `#632: statusless or returned + infra issue without spec keeps the class A ban`, мутант + `task-packet-s6-s7-alone-not-product-flow` (проверено прогоном). Но + покрывает только `['bug','infra','process']` / `['infra','S6-in-progress']` + / `['infra','S7-code-review']` — не `trivial`, см. находку выше. +- **AC4** (каждый признак самодостаточен; `## ТЗшка` не считается разделом + ТЗ) — проверено чтением: `(?![\p{L}\p{N}_])` корректно отсекает + `ТЗшка`, поскольку `\b` в JS не различает кириллицу как «словесный» + символ (голый `\b` посчитал бы границу сразу после `З` и ложно совпал бы) — + логика верна, подтверждено прогоном теста `#632: product S6 issue keeps + class A rights…`, который явно проверяет и позитивный, и негативный случаи + заголовка. +- Реальный кейс #607 (единственный файл на ветке — `SPEC-REVIEW-607-r1.md`) + и обратный кейс (единственный файл — `scripts/task-packet.mjs`, + собственная ветка #632) оба воспроизведены тестами и не про регрессируют + друг друга. +- Трейлеры коммита: `Issue: #632`, `User-Visible: no` — корректно, + продуктового поведения нет, changelog не требуется. +- Класс файлов: `scripts/task-packet.mjs`, `test/task-packet.test.mjs`, + `scripts/mutation-registry.mjs` — весь дифф класса B, ни одного файла + класса A; трек «инфраструктурный (ускоренный вход)» issue #632 применён + корректно (метки: `bug, infra, S7-code-review, process`, без предыдущих + `S1…S6`). +- Формат нового мутанта в `scripts/mutation-registry.mjs` соответствует + соседним записям (`id`, `guard`, `because`, `patches[].find/replace`); + `find`-строки совпадают с реальными строками файла посимвольно (иначе + `--check` не прошёл бы анкоры — прогнано и подтверждено). + +### Чего не проверял + +- Живой `node scripts/task-packet.mjs --issue 607` с реальным `gh` — не + запускал по той же причине, что и автор (`gh`/`api.github.com` + недоступны из песочницы ревьюера). Проверено на уровне `buildPacket` + напрямую, что эквивалентно проверке решающей логики — сборка входов + (`collectInputs`) в этой правке не менялась содержательно (только вызов + вынесенной `branchIsInfrastructure`). + `check-inputs.mjs --coverage`, `no-new-any.mjs` — не перегонял, diff не + добавляет новых скриптов/тестов вне уже показанных и не содержит TS-типов + (`any` неприменим к `.mjs`); полагаюсь на зелёный Validate на этом SHA. +- `process-gate.mjs` целиком — не перегонял; трейлеры проверил вручную + (`git log -1 --format=full`), формат совпадает с §1 таблицей классов. + +## Материал раунда +``` +tree 7cd2f2bf4d1c876b39a41d53e489ec1bbe1019cf +blob e033c1014d283942b33fe4819a7def6ec710172d scripts/task-packet.mjs +blob 5cbca549226a4905c093671f6c55b394c17c0168 test/task-packet.test.mjs +blob a0ff104344554102d33db3e18a06cfa9d2932ec7 scripts/mutation-registry.mjs +SHA материала ревью: a775080ae231bf0e1524ab13fda85ee303234280 +``` + +## Вердикт + +**Жёлтый.** AC1/AC2/AC3/AC4 из хендоффа доказаны и воспроизведены (мутанты +красят заявленные тесты, тесты падать умеют). Один Medium **в скоупе**: +`productFlowEvidence` не покрывает `trivial`-трек продуктовых задач (§5.1), +из-за чего реальный продуктовый bug fix на этом треке в `S6-in-progress`/ +`S7-code-review` может получить тот же ложный «класс A запрещён», что и +исходный баг #607 — просто по другому репро. High нет. Возврат автору для +правки в этом же issue (решение владельца 2026-08-19, #202) — отдельный +issue не заводится. + +--- + + + +## Материал раунда + +- Ветка: `issue/632-task-packet-track`, коммит `a775080ae231` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `ea023197549dc674c02fae226d9324246cf522fe` + ``` + git log --all --format='%H %T' | grep ea023197549d + ``` +- Тело issue: `85577dba6802882fd2cb7cc293044cd8318bc67981c0f743e27b5e77294bdece` +- Вердикт конвейера: `yellow` · High 0 diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index e3ab54bf..2e0ec738 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -19,6 +19,7 @@ | #635 | [CODE-REVIEW-635-r3.md](CODE-REVIEW-635-r3.md) | code · r3 | 🟢 зелёный | 0 | 0 | firstParagraph: ветка нет\b в фильтре мёртвая из-за ASCII-only \b в JS-регэкспах, расхо… | `scripts/reviews-index.mjs` | | #634 | [CODE-REVIEW-634-r1.md](CODE-REVIEW-634-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #633 | [CODE-REVIEW-633-r1.md](CODE-REVIEW-633-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | +| #632 | [CODE-REVIEW-632-r1.md](CODE-REVIEW-632-r1.md) | code · r1 | 🟡 жёлтый | 0 | 1 | trivial-трек не входит в признаки продуктового потока | `scripts/task-packet.mjs` `task-packet.mjs` | | #630 | [CODE-REVIEW-630-r1.md](CODE-REVIEW-630-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #629 | [SPEC-REVIEW-629-r1.md](SPEC-REVIEW-629-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — | | #627 | [SPEC-REVIEW-627-r1.md](SPEC-REVIEW-627-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | избыточное (не противоречивое) условие в AC2; влияние на touch не названо явным пунктом | `docs/TOUCH-SUPPORT.md` |