diff --git a/docs/reviews/CODE-REVIEW-637-r1.md b/docs/reviews/CODE-REVIEW-637-r1.md new file mode 100644 index 00000000..3b58bb1e --- /dev/null +++ b/docs/reviews/CODE-REVIEW-637-r1.md @@ -0,0 +1,170 @@ +# CODE-REVIEW · issue #637 · заход r1 + +Материал: `966928135b6a40fd6a73c7631bd514a79015d48a` (единственный коммит поверх +`dev`=`ee0f97c8`). Класс B: `scripts/process-metrics.mjs`, +`test/process-metrics.test.mjs`, `.github/workflows/process-metrics.yml`, +`scripts/mutation-registry.mjs`. Файлов класса A и документов — нет. +Трейлеры: `Issue: #637`, `User-Visible: no` — верно, видимого поведения +продукта изменение не касается, правки changelog не требуются. + +## Скоуп + +Разовый инструмент вместо ручного сбора метрик процесса (аудит 22.09): +чистые функции над снимком GitHub (issue + таймлайн меток, прогоны Actions, +имена документов ревью) → Markdown-отчёт; тонкий CLI на `gh`; workflow — +понедельник 05:00 UTC + ручной запуск, только чтение, отчёт в step summary +и артефакт. Это инфраструктура процесса, не продуктовая фича — к +`docs/SCOPE.md` (Core user jobs) не апеллирует, что для класса B корректно. + +## Как проверялось + +- Полное чтение `scripts/process-metrics.mjs`, `test/process-metrics.test.mjs`, + `.github/workflows/process-metrics.yml`, диффа `scripts/mutation-registry.mjs`. +- `node --test test/process-metrics.test.mjs` — 7 pass (см. ниже, «Гейты»). +- Оба новых мутанта применены вручную к рабочей копии и прогнаны под своим + guard-тестом: оба ловятся (тест падает), рабочая копия возвращена в + исходное состояние (`git status --porcelain` пуст после отката). +- Точечный ручной прогон `buildReport` на синтетическом таймлайне с + быстрыми (< 3 мин) переходами — искал регресс в вычислении медиан. +- Сверка трейлеров коммита и путей файлов с хендофф-комментарием + (§2.10 material/blobs совпадают с `git show`). + +## Находки + +### Medium (в скоупе задачи) — ложный «—» вместо настоящего «0 ч» в медианах + +`scripts/process-metrics.mjs:162-165` (`buildReport`): + +```js +medianLeadToS7Hours: hours(median(completed.map((i) => i.leadToS7Ms)) ?? NaN) || null, +medianReviewToMergeHours: hours(median(completed.map((i) => i.reviewToMergeMs)) ?? NaN) || null, +medianLeadToS8Hours: hours(median(completed.map((i) => i.leadToS8Ms)) ?? NaN) || null, +medianSpecLeadHours: hours(median(completed.map((i) => i.specLeadMs)) ?? NaN) || null, +``` + +`??` корректно отличает «нет данных» (`median` вернул `null`) от валидного +результата, но финальное `|| null` — нет: если медиана и правда равна 0 ч +(переход занял меньше ≈3 минут, что при автоматической простановке меток +конвейером — обычное дело для быстрых/тривиальных задач), `hours(0)` даёт +`0`, а `0 || null` схлопывает его в `null`. В отчёте (и в Markdown, и в +JSON-артефакте) настоящий «0 ч» неотличим от «данных нет» — оба рендерятся +как «—». + +**Воспроизведение** (не тестовый файл, ручной прогон в песочнице ревью): + +```js +const T = (h) => new Date(Date.UTC(2026,8,15,0,Math.round(h*60))).toISOString(); +const issues = [{number:1, title:'t', closed_at:T(1), state_reason:'completed'}]; +const timelines = new Map([[1,[ + {event:'labeled', label:{name:'S1-new'}, created_at:T(0)}, + {event:'labeled', label:{name:'S7-code-review'}, created_at:T(0.01)}, + {event:'labeled', label:{name:'S8-merged'}, created_at:T(0.02)}, +]]]); +buildReport({since:T(0), until:T(1), issues, timelines, reviewFiles:[], runs:[]}).issues.medianLeadToS7Hours +// → null (ожидалось 0) +``` + +Существующие тесты этот путь не задевают: во всех фикстурах (`buildReport`, +аудит-фикстура) переходы занимают часы, медиана никогда не равна 0. + +Почему это релевантно именно здесь: единственная цель инструмента — «без +регулярного замера решения об ускорении (#620, #636) нельзя проверить» +(текст issue). Если ускорение конвейера доходит до долей часа, самый +успешный случай — самый быстрый — окажется замаскирован под «нет данных», +то есть баг бьёт ровно по метрике, ради которой скрипт написан. Это не +крайний случай на бумаге: автоматическая простановка меток конвейером +(#636) делает переходы короче секунд для части событий уже сегодня. + +В скоупе задачи (правка внутри тех же чистых функций, что уже покрыты +тестами), High-находок нет → жёлтый вердикт, возврат автору. + +## Что проверено и корректно + +- `issueMetrics`: вход по первой статусной метке (не любой), различение + `entered`/`s4`/`s5`/`s7`/`s8`, счётчик повторных постановок S7/S4 отдельно + от времени первой — тест воспроизводит ровно кейс с `unlabeled`+повтором, + прочитан и логика соответствует. +- `reviewRounds`: регэксп `^(CODE|SPEC)-REVIEW-(\d+)-r(\d+)\.md$` строгий, + не матчит `README.md`/`CODE-REVIEW-issue-5.md` — верно по тесту. +- `runMetrics`: прогоны конвейера (`display_title` вида `process #NN · …`) + сведены в одну строку `process`, остальные — по имени workflow; отменённый + прогон без длительности не учитывается в wall-time — прочитано и + соответствует правке, которая решает проблему из хендоффа («имена + прогонов конвейера — на каждое событие метки, не сотни строк»). + Здесь та же группировка использована для оценки инструмента: + ложноположительного объединения разных workflow с совпадающим префиксом + не нашёл, `process #\d+ · ` — достаточно специфичный якорь для этого + репозитория. +- `pipelineMetrics`: skipped-прогоны не считаются — подтверждено мутантом + `metrics-count-skipped-pipeline-runs` (тест падает при снятии условия, + проверено запуском). +- `jobMinutes`: доля «Мутанты» по имени job, сломанные (`started_at: 'x'`) + записи не портят сумму — по тесту и чтению кода. +- `buildReport`/`renderMarkdown`: раунды считаются по документам, а не по + событиям S7-метки — подтверждено мутантом + `metrics-rounds-by-s7-events` (тест падает при подмене источника, + проверено запуском). Причина в хендоффе (конвейер #636 переставляет S7 + сам) соответствует коду. +- `fetchSnapshot`: только чтение (`gh api` на issues/timeline/actions/runs + + `git ls-tree`), пагинация с разумными пределами (5×100 issue, 3×100 + событий таймлайна на issue, 15×100 прогонов); фильтр по `closed_at` внутри + окна — корректен, поскольку `since` в GitHub Issues API фильтрует по + `updated_at`, а закрытие всегда обновляет `updated_at`, так что ни один + закрытый в окне issue не потеряется на этом шаге. +- `.github/workflows/process-metrics.yml`: `permissions` — только + `contents/actions/issues: read`, нет `issues: write`; код и `ref: dev` — + согласовано с хендоффом (расписание сработает после зеркалирования в + `main`, что явно оставлено как задача владельцу после мержа); экшены + запиненны по SHA. Тест `test/process-metrics.test.mjs:108` проверяет это + структурно (регэкспы по тексту workflow) — само по себе слабое + доказательство, но здесь избыточно: я прочитал файл и подтвердил то же + глазами. +- Трейлеры коммита (`Issue: #637`, `User-Visible: no`) и заявленные в + §2.10-блоке SHA/blob-хэши соответствуют `git show`. + +## Гейты + +- `node --test test/process-metrics.test.mjs` — прогнал сам: 7 pass, 0 fail. +- `npx tsc --noEmit`, `npm test` (полный), `npm run build` — не гонял: + Validate на этом самом SHA (`96692813`) уже зелёный + (https://github.com/Matysh/houseplan-card/actions/runs/35828853731), + дифф с тех пор не менялся. +- `node scripts/check-docs.mjs` — не требуется: диф не трогает `src/**` + (только `scripts/`, `test/`, `.github/workflows/`). +- `npm run invariants` — не требуется: диф не касается геометрии, `layout`, + толщины стен, `marker.space`/`open_spans`. +- `demo/smoke_*.mjs`, `npm run golden:verify` — не требуется: диф не + меняет рендер, геометрию, стили или слои; смок-селектор не запускал — + никакой продуктовый рендер-путь не тронут (правка целиком в + CI-инструментарии, работающем на `gh api`-снимках, а не на карточке). +- `python -m pytest tests_backend -q` — не требуется: `custom_components/**` + не тронут. +- Мутанты `metrics-count-skipped-pipeline-runs` и `metrics-rounds-by-s7-events` + — проверил лично (патч → тест красный → откат), оба ловятся. +- `test/single-source-numbers.test.mjs` — не относится: диф не добавляет + пользовательских чисел (превью/подпись/подсветка), это внутренние + метрики процесса, а не карточка. + +## Чего не проверял + +- Живой прогон `fetchSnapshot`/CLI против реального `gh api` за 15–22.09 — + не выполнял: это ручное тестирование сети, вне цикла ревью, и сам автор + прямо пишет, что сверка с таблицей аудита §7.1 состоится первым живым + запуском после мержа и зеркалирования в `main`. Логика самой функции + (пагинация, фильтрация, только-чтение) прочитана и корректна. +- Точность `wallMs`/`job-минут` на реальных исторических прогонах — не + считал вручную, доверяю арифметике, подтверждённой тестами на фикстурах. + +--- + + + +## Материал раунда + +- Ветка: `issue/637-process-metrics`, коммит `966928135b6a` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `0687eddba719e669212123bf80bf8c6d683bbf50` + ``` + git log --all --format='%H %T' | grep 0687eddba719 + ``` +- Тело issue: `187c1bf4bbdea65f4114112ea0ee62c313cb015d343fbb44d68507420f204e41` +- Вердикт конвейера: `yellow` · High 0