From 341c9fcfce59f4758e32d53c2adbf12988eded6d Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 1 Oct 2026 14:33:53 +0000 Subject: [PATCH] docs: review document for #752 Issue: #752 User-Visible: no --- docs/reviews/CODE-REVIEW-752-r1.md | 226 +++++++++++++++++++++++++++++ 1 file changed, 226 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-752-r1.md diff --git a/docs/reviews/CODE-REVIEW-752-r1.md b/docs/reviews/CODE-REVIEW-752-r1.md new file mode 100644 index 00000000..a84baef5 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-752-r1.md @@ -0,0 +1,226 @@ +# CODE-REVIEW-752-r1 + +Issue: #752 · Трек: show · Заход: r1 · Блокирующих циклов использовано 0 из 2 +Материал: `1bd7435c9d206aa980df0d53711f2063bac9d1d4` (`origin/dev..HEAD`, один коммит) + +## Скоуп + +Issue #752 — три уточнения к еженедельному отчёту процесса (`process-metrics.mjs`), +найденные при реализации #728 и ТЗ #737/#729: + +1. **Объём задачи (П.1).** Объём К7 теперь — `+/−` строк только классов A и B + (продукт, тесты, инструменты); документация, перенос архива (`docs/reviews` → + `legacy/reviews`) и бандл в объём не входят. Коммиты задачи с трейлером + `Release:` (приёмка эталонов, перепривязка тестов к бете) входят в объём; + не входят только коммит кандидата беты/релиза и коммит бота `beta-derived` + (признак — тот же предикат, что в `bundle-policy.mjs`, #657, а не патч-набор + #727, который отбрасывал все `Release:`-коммиты). `isInfra` теперь судит по + файлам классов A/B/C без `docs/reviews/**` — задача только с документом + ревью признака инфраструктуры не получает. +2. **Причины возвратов (П.2).** `merge-candidate.mjs` экспортирует `OUTCOME_SIGNS`/ + `outcomeOf` — регэксп-признак заголовка на каждый исход `commentFor`/ + `describePushRefusal`, кроме успешного слияния; текст сбоя шага (`action=error`) + перенесён в `commentFor` байт-в-байт. `process-metrics.mjs` импортирует эти + признаки: исходы стадии слияния после зелёного вердикта → `merge`, отказ push + на стадии ребейза (до ревью) → новая причина `push-refused`. `rereview` + причины не даёт (задача уходит в `S7`, а не автору). +3. **Черновик ТЗ (П.4, #729).** Новый раздел «Черновик ТЗ (#729)» после «По + трекам»: эпохи `S4-spec-review` на треке `ask` (эпоха — от постановки `S4` + до следующей статусной метки, повторная `S4` эпоху не начинает), доля с + комментарием «Черновик:» и их исход (`S5-ready` — в дело, `S3-spec` — + выброшен), коммиты `Spec-Draft:` в `dev` за окно, и медиана `S5 → S7` по + трекам с черновиком и без (`n < 3` — «мало данных»). `fetchSnapshot` теперь + тянет таймлайны и для задач в `S5-ready`…`S7-code-review`, не только `S8`/ + закрытых — иначе раздел не видел бы задачи, ещё не домёрженные. + +П.3 (токены за окно) по тексту ТЗ прямо вынесен в отдельный issue (#761, +зависит от #737) и в материале отсутствует — это ожидаемо, не находка. + +`User-Visible: no` — отчёт не меняет поведения карточки/интеграции, только +инструмент мейнтейнера; трейлеры `Issue: #752` и `User-Visible: no` на +коммите на месте, changelog не тронут — верно. + +## Как проверялось + +Прочитан полный дифф (`scripts/merge-candidate.mjs`, `scripts/process-metrics.mjs`, +оба тестовых файла), тело issue #752 и все 4 комментария (включая уточнение ТЗ +от 2026-10-01T13:36 и финальный отчёт разработчика). Логика каждой новой функции +(`isBetaCommit`, `countsToVolume`, `isTaskFile`, `changeVolume`, `specEpochs`, +`draftSection`, `outcomeOf`/`outcomeReason`) прослежена вручную по тестовым +фикстурам построчно — не только прочитана, но пересчитана на конкретных входах +теста, чтобы убедиться, что ожидаемые числа в `assert` получаются именно этим +кодом, а не совпадение. Отдельно сверены регэкспы `OUTCOME_SIGNS` против +реальных строк, которые возвращает каждый `case` в `commentFor` — совпадение +подтверждено построчно, не на глаз. + +### Гейты + +| Гейт | Статус | Комментарий | +|---|---|---| +| `npx tsc --noEmit`, `npm test`, `npm run build` + bundle-policy verify | **не перегонял** | Validate на этом SHA зелёный (ссылка в постановке задачи) — дешёвые гейты подтверждены, бюджет раунда не тратится повторно | +| `node --test test/process-metrics.test.mjs test/merge-candidate.test.mjs` | **прогнал сам** | 64 + 34 = 98 тестов, все `ok`, 0 fail — дополнительная проверка поверх Validate, т.к. это основной тестовый материал задачи | +| `node scripts/smoke-select.mjs --base origin/dev --head HEAD` | **прогнал** | «Исполняемого frontend-диффа нет (`src/**/*.ts` не тронут)» — смоки не выбираются, выбирать нечего | +| `golden:verify` | не прогонял | нет метки `ci:golden`, дифф не рендер | +| `pytest tests_backend` | не прогонял | Python не тронут | +| `npm run invariants` | не прогонял | геометрия/модель не тронуты | +| performance-профили | не прогонял | не названы в AC | +| мутанты по реестру | не прогонял и не требуется | трек `show`: мутанты в разработке не гоняются ни на каком треке (#709), ревьюер их тоже не применяет | + +## Проверка AC + +**AC1 (объём).** Доказан `test/process-metrics.test.mjs`: `#752 AC1 объём: классы +A и B…` и `#752 AC1 объём: Release:-коммиты задачи входят…`. Пересчитал вручную +обе фикстуры: +- первая — `src/a.ts +10` (A, считается) + `docs/x.md +5000` (C, не считается) + + перенос `docs/reviews/R.md → legacy/reviews/v1/R.md` (не считается) → объём + `751` = 10, корзина `≤30`; задача `#7` только с документом ревью → `isInfra` + ложь, `changeVolume` = `null`. Добавление `test/a.test.mjs +40` (B) даёт 50 — + тесты входят. +- вторая — «приёмка эталонов» (`demo/golden/attestation.json`, класс B, 6 строк) + и «перепривязка тестов» (`test/b.test.mjs`, 4 строки) входят в объём `751` + (10+6+4=20); кадры `demo/golden/baselines/*.png` — класс D, не входят. + Кандидат беты (подпись `Release v1.0.0-beta.2 candidate`), его рефреш без + пересборки бандла (подпись кандидата без изменённого бандла — тоже + `committedBundleMustMatch` true по подписи), промоушен (`release: promote…`, + бандл в файлах) и бот `beta-derived` (по подписи из `_beta-derived.yml`, + сверено отдельным контрактным тестом) — все четыре правильно отфильтрованы + `isBetaCommit`, подтверждено `assert.deepEqual(commits.filter(isBetaCommit)…)` + с точным списком subject'ов. Отдельно проверен краевой случай: текст + «Release v1.0.0 candidate notes» в теле, где `Release: notes wrap here` не + является трейлером (строка перед терминальным блоком, не в нём) — + `isBetaCommit` корректно возвращает `false`; прочитал `releaseTrailers()` и + убедился, что парсинг идёт строго с конца сообщения до первой несовпадающей + строки, то есть этот тест действительно проверяет границу терминального + блока, а не дублирует предыдущий. **Чем краснеет:** со старым `countsToVolume` + (класс ≠ D и не `docs/reviews/**`) первая фикстура дала бы `751` = 5010 + (документация в объёме) — именно то число, которое ТЗ называет как + регрессионный маркер; подтверждено чтением, тест этого не исполняет заново со + старым кодом, но разница однозначно видна по диффу `countsToVolume`. +- Старый тест AC7 (`#728 сравнение: объём из git без Release:, dist/** и + docs/reviews/**…`) не менялся и остаётся зелёным — пересчитал его фикстуру + вручную под новой логикой (класс кандидата по подписи вместо голого + `Release:`-фильтра) и убедился, что оба коммита (`701`, `702`) дают те же + числа объёма и инфраструктуры, что и раньше. + +**AC2 (причины возвратов).** Доказан `test/merge-candidate.test.mjs` (`#752 AC2: +каждый шаблон исхода…`) и `test/process-metrics.test.mjs` (`#752 AC2 причины…`). +Сверил каждый из 11 `OUTCOME_SIGNS` построчно с текстом соответствующего `case` +в `commentFor`/`describePushRefusal` — совпадение есть и оно единственное +(тест `merge-candidate.test.mjs` проверяет это явно: `OUTCOME_SIGNS.filter(...)` +даёт ровно один признак на каждый текст). Тест `process-metrics.test.mjs` +прогоняет сценарий «зелёный вердикт → 6 разных исходов слияния → `merge`, +отказ push на ребейзе → `push-refused`, возврат без комментария → `unknown`» +через полный `issueTrackMetrics`/`trackSection`/`renderMarkdown` — то есть +проверена не только чистая функция, но и интеграция с существующей таблицей +«По трекам» (строка `| show | 8 | 0 | 0 | 0 | 0 | 1 | 6 | 0 | 0 | 1 |` сверена +построчно с входными данными). Также изменены (не ослаблены, а приведены к +новому определению) три утверждения теста #728: фикстура `Release:`-коммита в +тесте `trackAt` получила подпись кандидата (иначе по новому правилу это был бы +коммит задачи, а не беты — старая фикстура со старым намерением перестала бы +значить то же самое); `returnSignal` для отказа push на ребейзе теперь ждёт +`'push-refused'` вместо `null`; агрегат по треку `show` в `returnReason` +аналогично. Это ожидаемое следствие самого фикса, а не ослабление проверки — +логика до изменения доказуемо отличалась (`sig(refused)` была `null`, стала +`'push-refused'`, и это ровно то поведение, которое чинит AC2). **Чем +краснеет:** тест называет это явно — «на нынешнем коде все пять первых — +unknown» (с кодом до правки `sig()` возвращает `null`/`'unknown'` вместо +`'merge'`/`'push-refused'`). + +**AC3 (черновики).** Доказан двумя тестами `#752 AC3 черновик ТЗ…`. Прошёл +`specEpochs` вручную по фикстуре (повторная постановка `S4-spec-review` не +открывает новую эпоху — геометрия событий и результат `[['S3-spec',2], +['S5-ready',1]]` сошлись). Прошёл `draftSection` вручную по обеим веткам +(черновик выброшен в `S3`, черновик пошёл в `S5`) — числа `{total, withDraft, +used, thrown, other}` совпадают с ручным пересчётом. Второй тест проверяет +`s5ToS7` по трекам: пересчитал вручную для `ask` (3 задачи с черновиком, +медиана 2ч, `n=3` — «данных достаточно»; 2 задачи без черновика, медиана 20ч, +`n=2 < 3` — «мало данных») и для `ship` (без эпох `S4` вовсе — `draftedS5` +остаётся `false` по умолчанию, что корректно для трека, не проходящего `S4`). +Задача, у которой `S7` вне окна (`noS7`), и коммит с `Spec-Draft:` вне окна +или без трейлера — оба случая исключены корректно. + +Отдельно нашёл место, не покрытое прямым тестом: расширение `fetchSnapshot` +(`allIssues` теперь включает задачи с метками `S5-ready`…`S7-code-review`, не +только `S8-merged`/закрытые) — необходимое условие, чтобы `draftSection` +вообще увидел ещё не домёрженные `ask`-задачи в проде. Единственный тест на +`fetchSnapshot` (`#728 fetchSnapshot…`) не тронут и проверяет попадание задачи +`#703` (метка `S6-in-progress`) в `allIssues` — но она туда попадает и без +нового условия, через существующий путь `runIssues` (задача упомянута в +`display_title` прогона конвейера). То есть новая ветка фильтра в этом тесте +не отличима от старой. Прочитал код `fetchSnapshot` (строка с +`['S5-ready', 'S6-in-progress', 'S7-code-review', 'S8-merged'].includes(...)`) +и убедился, что она синтаксически верна и соответствует описанию в ТЗ и в +комментарии над функцией — **проверено чтением, не исполнением**. Это разрыв в +покрытии интеграционного пути (AC3 доказан на уровне чистой функции +`draftSection`, но не на уровне «фактически попадёт ли нужная задача в tolist +при реальном вызове `fetchSnapshot`»), не найденный мной дефект поведения — +отмечаю как Low, не как Medium, потому что сама проверка кода не вскрыла +несоответствия. + +## Находки + +**Low.** `fetchSnapshot`: новый фильтр `allIssues` (задачи `S5-ready`… +`S7-code-review`) не покрыт отдельным тестом — существующий тест на эту +функцию проходит через параллельный путь (`runIssues`) независимо от нового +условия. Код прочитан и признан корректным; это пробел в доказательстве +интеграции, не поведенческий дефект. Трек `show`: бухгалтерия такого рода — +Low, цикла не открывает (REVIEWER.md, «Трек show»). + +Больше находок нет: High — 0, Medium — 0. + +## Что проверено и корректно + +- Классификация коммитов беты/кандидата/бота (`isBetaCommit`) — все пять + категорий из уточнения ТЗ (кандидат по подписи, кандидат по бандлу, + промоушен, бот, ложный текст «Release:» не в трейлере) разобраны и + пересчитаны вручную, совпадают с тестом. +- Классы файлов для объёма (A+B) и признака инфраструктуры (A+B+C без + `docs/reviews/**`) соответствуют единой таблице `change-classes.mjs`, не + переопределяются локально. +- `OUTCOME_SIGNS`/`outcomeOf`/`outcomeReason` — однозначное соответствие + признак → исход → причина отчёта, включая не-причину `rereview`; защитный + `throw` при отсутствии `rereview` в реестре — на месте. +- `specEpochs`/`draftSection` — границы эпох, повторная метка, попадание в + окно по первому `S5-ready`, `MIN_COHORT=3` — все пересчитаны вручную на + тестовых таймлайнах. +- Единственное число — единственный источник: объём/корзина берутся из одного + `changeVolume`/`volumeBucket`, используемых и в «По трекам», и в «До и + после»; причина возврата — из одного `returnSignal`. +- Трейлеры коммита (`Issue: #752`, `User-Visible: no`) корректны, changelog не + тронут — соответствует `User-Visible: no`. +- Отклонения, заявленные автором (предикат #657 вместо патч-набора #727; три + скорректированных утверждения теста #728), проверены по существу и + признаны обоснованными, а не прикрытием дефекта. + +## Чего не проверял + +- Полный прогон `npx tsc --noEmit` / `npm test` (весь набор) / `npm run build` + — зелёный Validate на этом SHA уже их подтвердил (ссылка в постановке). + Сам прогнал только два изменённых тестовых файла (98 тестов) как + дополнительную проверку. +- `golden:verify`, `pytest tests_backend`, `npm run invariants`, + performance-профили — не применимы к этому диффу (нет меток/файлов, + требующих эти гейты); `smoke-select.mjs` подтвердил, что frontend-диффа нет. +- Мутанты по реестру — не прогонял и не обязан: трек `show`, их ловит ночной + прогон (#709). +- Реальный GitHub API / реальные issue конвейера — интеграция `fetchSnapshot` + с реальными данными репозитория не воспроизводилась; см. находку Low выше. +- «16 ручных поломок», заявленные автором в комментарии — не переигрывал + заново; трек `show` не требует от ревьюера повторения авторской мутационной + проверки, а точечная проверка по тестовым файлам и чтению кода совпадений с + авторским описанием не выявила расхождений. + +--- + + + +## Материал раунда + +- Ветка: `issue/752-metrics-volume-reasons`, коммит `1bd7435c9d20` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `fba4442b8b85bf3ba1ac7d06c04e1d9eb104fe1c` + ``` + git log --all --format='%H %T' | grep fba4442b8b85 + ``` +- Тело issue: `0f72ec0637dcb254d7af7ca5f978581fd8cb3d9206198b0dd0f6666b1d1008c3` +- Вердикт конвейера: `green` · High 0 · маршрут `fix` +