diff --git a/docs/reviews/CODE-REVIEW-517-r1.md b/docs/reviews/CODE-REVIEW-517-r1.md new file mode 100644 index 00000000..67110cff --- /dev/null +++ b/docs/reviews/CODE-REVIEW-517-r1.md @@ -0,0 +1,162 @@ +# CODE-REVIEW-517-r1 + +Issue: #517 · этап: code · заход r1 · блокирующих циклов израсходовано 0 из 4 +Материал: `156835c048a70497d7f4116c60f4c60457a05107` (ветка `issue/517-spec-in-issue-body`, один коммит на `origin/dev`) + +## Скоуп + +Класс B/C: `.github/workflows/process.yml`, `PROCESS.md`, `AGENTS.md`, +`docs/specs/README.md`, `scripts/mutation-gate.mjs`, `scripts/process-gate.mjs`, +`scripts/review-doc-guard.mjs`, `scripts/task-packet.mjs`, `test/process-gate.test.mjs`, +`test/review-doc-guard.test.mjs`, `test/task-packet.test.mjs`. Ни одного файла класса A — +`User-Visible: no` корректен, changelog не тронут и не должен быть. + +Задача переносит доказуемость «ТЗ полного трека вынесено на этом тексте» из файла +`docs/specs/NN-slug.md` в хеш тела issue, снимаемый конвейером, и замораживает каталог +`docs/specs/` как архив. Решение владельца зафиксировано в самом issue (2026-09-10). + +Это первый заход код-ревью (`S6→S7`); дельты к прошлому раунду нет — раздел +«Унаследовано из r» не применяется, разбор ниже полный, как и требуется на r1. + +## Как проверялось + +**Прочитано.** `docs/SCOPE.md` (задача — процессная, персона не описана в SCOPE, что +верно отмечено в самом ТЗ), `PROCESS.md` целиком (§1–14), `AGENTS.md`, тело issue #517 +и все пять комментариев (S2-аналитика, вердикт ревью ТЗ r1 жёлтый, правки автора, +вердикт ревью ТЗ r2 зелёный, хендофф S6→S7). Канонические доки подсистем (SUN/LIGHT/…) +не относятся — задача не трогает продукт. + +**Гейты.** + +| Гейт | Результат | +|---|---| +| `npx tsc --noEmit` | чисто | +| `npm test` | 2469 tests, 2468 pass, 1 skipped, 0 fail | +| `npm run build` | собран; `cmp dist/houseplan-card.js custom_components/houseplan/frontend/houseplan-card.js` — идентичны | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | «Исполняемого frontend-диффа нет — браузер-смоки не выбираются» (src/** не тронут) | +| `node scripts/check-docs.mjs` | «Documentation checks passed (7 files, 12 external links)» — не обязателен (src/** не тронут), прогнан для очистки совести | +| `node scripts/process-gate.mjs --issues` | «гейт пройден, предупреждений 1» (WARN п.8: инфраструктурный диапазон, класса A нет — статус issue не требуется) | +| 4 названных мутанта, `node scripts/mutation-gate.mjs --id=` | все 4 «поймано 1 из 1» (ниже) | + +Не прогонялись и почему: `golden:verify` (нет визуальных изменений), `pytest tests_backend` +(Python не тронут), `no-new-any.mjs` (правки в `.mjs`, не в `src/**` TS), инварианты модели +(геометрия не тронута), performance-профили (не названы в AC и не задеты). + +**Мутанты — таблица «чем краснеет» (§2.7):** + +| AC | Чем доказан | Чем краснеет | +|---|---|---| +| AC1 (хеш тела в якорях) | `test/review-doc-guard.test.mjs` + мутант `review-anchor-drops-issue-body` | строка `- Тело issue:` не печатается → тест красный (проверено: `node scripts/mutation-gate.mjs --id=review-anchor-drops-issue-body` → «тест покраснел, как обязан», поймано 1/1) | +| AC2 (находка «ТЗ менялось») | тот же файл + мутант `review-ignores-changed-spec-body` | `issueBodyChanged` всегда возвращает `null` → красный (проверено, поймано 1/1) | +| AC6 (reuse видит правку тела) | тот же файл + мутант `reuse-ignores-changed-issue-body` | сравнение хеша в `reusableGreenVerdict` снято → красный (проверено, поймано 1/1) | +| AC3 (гейт без файла ТЗ) | `test/process-gate.test.mjs` + мутант `process-gate-requires-spec-file` | проверка текста заменена на «только файл» → красный (проверено, поймано 1/1) | +| AC4 (документы без требования файла) | `test/review-doc-guard.test.mjs`, текстовые ассерты по `PROCESS.md`/`AGENTS.md`/README | сравнение прочтением: строки-требования файла ТЗ нет нигде (grep ниже) | +| AC5 (AC из тела первым) | `test/task-packet.test.mjs` — прогнан в составе `npm test`, три сценария (тело/файл/ничего) | явно не воспроизводил мутацию отдельно — чистая функция, покрытие достаточное по трём веткам условия; риск низкий | + +Живая проверка `issueBodyDigest` на реальном теле issue #517 (снятом `gh issue view 517 +--json body`) пересчитана мной независимо: `d7258d4432fab7757039bc05e1ea932f9e023f61449bdf667da69002a06906c2` +— совпадает с указанным в хендоффе побайтово. Не заявление автора, а мой собственный прогон. + +## Проверка AC по существу (чтение + тесты) + +- **AC1.** `materialAnchorBlock` пишет `- Тело issue: \`\`` только когда передан + `issueBody` (иначе строки нет — старые документы не ломаются, тест это утверждает). + Единый шаг «Опубликовать документ ревью» (`process.yml:912-988`) вызывает + `--anchor` с `--issue-body="$MATERIAL_ISSUE_BODY"` **на обоих этапах** (spec и code) — + не только для code, как можно было бы прочитать из плана поверхностно; проверено + чтением шага целиком, условие `if:` не сужает его до одного этапа. Хеш снимается в + шаге `material` через `gh issue view --json body` — не из `github.event.issue.body` + (У1 выполнен буквально, а не только продекларирован). +- **AC2.** Шаг `spec_body` (`process.yml:652-684`) сравнивает текущий хеш с записью + **последнего зелёного** `SPEC-REVIEW-NN-r*.md` через `issueBodyChanged`, включён в + промпт ревьюера форматированной строкой и запускается только на этапе `code` и только + когда `reuse` не сработал — что корректно, потому что `reuse=true` уже само по себе + гарантирует неизменность тела (см. AC6). Условие «нет зелёного документа или в нём нет + записи → не находка» проверено и тестом, и чтением (`issueBodyChanged` возвращает `null`). +- **AC6.** `reusableGreenVerdict(docs, differs, issueBodyDigest)` — третий параметр + сравнивается с `anchorIssueBodyFrom(latest.text)`; документ без записи (весь бэклог) + судится по дереву, как раньше — проверено тестом с «legacy»-документом без строки. + Единственное отступление от буквы AC6 не найдено. +- **AC3.** `checkSpecs` теперь судит текст тела (`## ТЗ` заголовок или токен `AC1`), + архивный файл всё ещё принимается; офлайн — молчание, `--issues` — предупреждение, + никогда не отказ. Соответствует и ТЗ, и записанному в PROCESS.md §8/§10.5 п.3. + `checkFrozenSpecs` — новая, отдельная, тёплое предупреждение на новый файл в + `docs/specs/**` (README.md исключён явно и по regex, и потому что это правка, не + добавление — `git diff-filter=A` его не найдёт). +- **AC4.** Прочитаны `PROCESS.md` (§2.3, §5, §7.1, §7.3 п.1, §8 правило 3, §13 п.2), + `AGENTS.md` (раздел Specs) и `docs/specs/README.md` целиком: ни один не требует файла + ТЗ для полного трека; `grep -n "docs/specs/NN\|docs/specs/"` не находит ни одного + места, где файл называется обязательным артефактом — только «архивный, доживает». + Ссылка на раздел «Обязательные release-артефакты» в README сохранена и корректна. +- **AC5.** `task-packet.mjs:buildPacket` — `fromBody = extractAcceptanceCriteria(issue.body)`; + если непусто **или** файлов ТЗ нет — источник тело, иначе файл. Тест на трёх фикстурах + (тело есть/тела нет-файл есть/ни одного) проходит. Небольшая избыточность — тело + парсится дважды (`fromBody` и затем `acSource === issue.body` снова через + `extractAcceptanceCriteria(acSource)`), но это чистая функция без побочных эффектов и + без наблюдаемой разницы в поведении — не нахожу это дефектом. + +## Находки + +Ни одной находки уровня High или Medium. + +**Low-1 (снимаю записью, без правки).** В `reusableGreenVerdict(docs, differs, +issueBodyDigest = null)` имя третьего параметра совпадает с именем экспортируемой функции +`issueBodyDigest`, объявленной выше в том же модуле (`scripts/review-doc-guard.mjs`). +Внутри `reusableGreenVerdict` эта функция не вызывается, поэтому побочного эффекта нет — +подтверждено чтением и тем, что все тесты и мутанты зелёные/красные штатно. Чисто +стилистическая путаница для читателя («это хеш-значение, а не вызов функции»). Ниже +порога, который стоило бы возвращать автору отдельным циклом; исправление тривиально, +если автор решит его сделать при следующей правке этого файла. + +## Что проверено и корректно + +- Обратная совместимость: документы без строки `Тело issue:` (весь бэклог) продолжают + читаться как раньше во всех трёх местах (`anchorIssueBodyFrom` → `null`, + `issueBodyChanged` → `null`, `reusableGreenVerdict` пропускает новую проверку) — + проверено тестами и чтением. +- `--specs` формат не тронут, страховка #414 и reuse #499 не задеты для старых документов. +- Трейлеры коммита: `Issue: #517`, `User-Visible: no` — оба корректны для этого диффа. +- `process-gate --issues` на самом этом диапазоне — предупреждение п.8, ожидаемое для + инфраструктурного диапазона без класса A (сам гейт это и объясняет). +- docs/specs/README.md переписан по AC4, ссылка на раздел release-артефактов сохранена; + `check-docs.mjs` зелёный. +- Порядок шагов workflow (`material` → `reuse` → `spec_body` → `Review`) — проверено и + тестом на текст `process.yml`, и чтением: хеш снимается до вызова модели, находка + «ТЗ менялось» готова до промпта. +- Live-проверка `issueBodyDigest` независимо воспроизведена мной на актуальном теле + issue #517 и совпала с заявленной автором. + +## Чего не проверял + +- Живой прогон конвейера end-to-end (событие `labeled` → шаг `material` → анкор в + реальном PR-документе) — невозможен до слияния этой же задачи (курица и яйцо, + честно названо автором как «живая проверка на первой задаче после слияния»). + Проверено вместо этого модульно: все явные и неявные ветки покрыты тестами, я + перепрогнал их и мутанты сам, а не поверил отчёту. +- `golden`, `pytest tests_backend`, `no-new-any`, инварианты модели, performance — + не прогонялись, диф их не касается (см. таблицу гейтов выше). +- Стилистическая придирка Low-1 не проверялась на предмет «а что если где-то ещё в + файле есть похожий шэдоуинг» — искал только по этому диффу. + +## Материал раунда + +- SHA: `156835c048a70497d7f4116c60f4c60457a05107` +- Ветка: `issue/517-spec-in-issue-body` +- Дерево материала: `git rev-parse HEAD^{tree}` на указанном SHA +- Документы этапа spec: `docs/reviews/SPEC-REVIEW-517-r1.md` (жёлтый), `docs/reviews/SPEC-REVIEW-517-r2.md` (зелёный) +- Это первый документ этапа code — «Унаследовано из r» не применяется. + +Вердикт: зелёный · заход r1 · блокирующих циклов 0/4 · High: 0 · Medium: 0 → в задаче + +--- + + + +## Материал раунда + +- Ветка: `issue/517-spec-in-issue-body`, коммит `156835c048a7` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `7a703201a159d0e3a288e467a652af3a618a499e` + ``` + git log --all --format='%H %T' | grep 7a703201a159 + ``` +- Вердикт конвейера: `green` · High 0