From 0672c2b2ff9018f5308680a3c7056e6dcaae9d3f Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 23 Sep 2026 09:50:57 +0000 Subject: [PATCH] docs: review document for #621 Issue: #621 User-Visible: no --- docs/reviews/CODE-REVIEW-621-r1.md | 163 +++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 docs/reviews/CODE-REVIEW-621-r1.md diff --git a/docs/reviews/CODE-REVIEW-621-r1.md b/docs/reviews/CODE-REVIEW-621-r1.md new file mode 100644 index 00000000..81f41177 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-621-r1.md @@ -0,0 +1,163 @@ +# CODE-REVIEW-621-r1 + +**Issue:** [#621](https://github.com/Matysh/houseplan-card/issues/621) — `process.yml`: счёт раундов ревью читает `docs/reviews` через `contents` API с потолком 1 000 записей. +**Трек:** инфраструктурный (#562) — вход сразу на `S7-code-review`, статусов `S1…S6` не было. +**Заход:** r1 · блокирующих циклов израсходовано **0 из 4**. +**Материал:** `50e67c0988d91866b06c9967fae8c2f54f88efe9` (`issue/621-reviews-listing`, база `origin/dev`). Дерево коммита `1f842cfe8bab1f5716bdb3f32d11533f22abaa84` совпадает с деревом, указанным автором в блоке «Материал раунда» — сверено `git ls-tree HEAD` и `git rev-parse HEAD^{tree}`. + +## Скоуп + +Изменены только файлы класса B: `.github/workflows/process.yml` (job `guard`, шаг +`decide`) и `test/review-doc-guard.test.mjs`. Класса A нет — задача корректно +классифицирована как инфраструктурная (#562), продуктовый код и SCOPE.md не +затронуты: это правка внутреннего процесса ревью, а не пользовательского +поведения. + +Суть правки: листинг каталога `docs/reviews`, по которому считаются заходы и +циклы ревью, переведён с REST `contents` API (молчаливый потолок 1 000 записей) +на спуск по Git Trees API (`commits/<ветка>` → `.commit.tree.sha` → `docs` → +`reviews` → список блобов), где `truncated:true` — явный отказ листинга, а не +частичный список. Предупреждение о потолке 1000 снято вместе с зависимостью. +На 23.09 в `docs/reviews` 990 файлов — риск был реальным и близким (задача сама +описывает срок «примерно через неделю»). + +## Как проверялось + +| Гейт | Команда | Результат | +|---|---|---| +| Дешёвые гейты (typecheck/test/build/bundle) | Validate на `50e67c09` | success, см. https://github.com/Matysh/houseplan-card/actions/runs/35844333827 — не перегонялись повторно (см. системную инструкцию раунда) | +| Целевой unit-тест | `node --test test/review-doc-guard.test.mjs` | 63 pass / 0 fail (совпадает с хендоффом) | +| YAML-валидность | `python3 -c "yaml.safe_load(...)"` | OK | +| Синтаксис изменённого shell-шага | извлёк `jobs.guard.steps[id=decide].run` через PyYAML, прогнал `bash -n` | OK, без ошибок | +| Семантика `truncated` в jq | `echo '{"truncated":true,...}' | jq 'if .truncated then error(...) ...'` | подтверждено: `jq` возвращает ненулевой код (5) — `gh api --jq` в этом случае тоже завершится с ошибкой, `tree_names` вернёт 1, листинг корректно деградирует к «счёт отключён», как и раньше при отказе `contents` | +| Мутация (снятие защиты) | вручную заменил `--jq 'if .truncated then error("truncated") else ... end'` на `--jq '.tree[] | select(...) | .path'` (без обработки `truncated`) и перезапустил тест | AC2-тест **краснеет** (62 pass / 1 fail) — тест умеет падать; изменение отменено, дерево восстановлено `diff` (пусто) | +| Провенанс коммита | `node scripts/validate-commit-provenance.mjs` | чисто, трейлеры `Issue: #621` / `User-Visible: no` на месте | +| Ветка/классы файлов | `git diff origin/dev...HEAD --stat` | только `.github/workflows/**` и `test/**` — класс B, `User-Visible: no` корректен, changelog не требуется | + +**Не прогонялось и почему:** +- `npx tsc --noEmit`, `npm test` (полный), `npm run build` + сверка бандлов — не + повторял: Validate зелёный на этом же SHA (см. таблицу), diff не касается + `src/**`, `dist/**` или сборки. +- `node scripts/check-docs.mjs` — не требуется: diff не трогает `src/**`. +- `node scripts/model-invariants.mjs` — не требуется: геометрия, `layout`, + `marker.space`, толщина стен не затронуты. +- browser-смоки (`demo/smoke_*.mjs`) — не требуется: diff не трогает + `src/**`/`demo/**`-рендер, `smoke-select.mjs` неприменим (нет фронтенд-диффа). +- `golden:verify` — не требуется: нет визуальных изменений. +- `python -m pytest tests_backend` — не требуется: Python не тронут. +- Живой прогон нового `tree_names` против настоящего `gh api git/trees` с + токеном `HP_PROCESS_TOKEN` — не воспроизводим локально (нет сетевого доступа + к API из песочницы ревьюера); автор честно указал это в хендоффе. Первый + реальный прогон — этот же guard-job на данном issue при простановке + `S7-code-review`: если листинг откажет, в логе появится + `::warning::дерево docs/reviews … не получено`, и счёт по файлам отключится + (та же деградация, что и раньше при отказе `contents`), не блокируя ревью + полностью — есть страховка по комментариям. + +## Разбор по AC + +- **AC1** («Счёт раундов не зависит от числа файлов в каталоге»). Доказано + двумя слоями: (а) JS-агрегатор (`reviewRoundsFromFiles`/`attemptFromRounds`) + уже не имел ограничения по размеру массива — новый тест на фикстуре 2 400 + имён это подтверждает, но сам по себе не был местом бага; (б) реальный + источник обрезки — REST `contents` API — заменён на Git Trees API с потолком + 100 000 записей на уровень и явным флагом `truncated`, проверено чтением + кода (`tree_names`) плюс локальным воспроизведением семантики `jq + error()` (ненулевой код завершения) и мутацией, снимающей эту защиту + (тест AC2 краснеет). При текущем росте ~60 файлов/неделю до 100 000 — + десятилетия, разумный запас. **Доказано.** +- **AC2** («Предупреждение о потолке удалено из `process.yml` вместе с + зависимостью от `contents`»). Доказано тестом `guard перечисляет + docs/reviews деревом, а не contents, и без предупреждения о потолке` — + regex-свидетель на самом тексте job `guard`, подтверждено мутацией (см. + таблицу выше: снятие `if .truncated then error(...)` из текста делает тест + красным). **Доказано.** + +## Находки + +Блокирующих (High) и находок в скоупе (Medium) нет. + +- **Low, не блокирует, снимаю с записью.** Логика `tree_names` — инлайновый + bash внутри `run:` шага workflow, а не отдельный скрипт; проверяется только + регулярными выражениями по тексту YAML и моей ручной мутацией, а не прямым + исполнением с замоканным `gh api`. Комментарий в самом файле рядом (про + вынос счёта раундов в `scripts/review-doc-guard.mjs` «потому что inline-shell + не покрывается тестами», #454) — тот же класс риска, что здесь остался + непокрытым. Не блокирую: (1) автор прозрачно указал это в разделе «Чего не + проверял» хендоффа; (2) я вручную проверил семантику `truncated`/`error()` и + подтвердил мутацией, что регресс кода будет пойман тестом; (3) первый живой + прогон происходит на этом же issue при следующем событии `S7-code-review`, и + деградация при отказе безопасна (откат к счёту по комментариям, как и + раньше). Предложение на будущее, не для этой задачи: если `docs`/`reviews` + когда-нибудь переименуют или Git Trees API изменит форму ответа, стоит + вынести `tree_names` в тестируемый скрипт по образцу + `scripts/review-doc-guard.mjs`. + +## Что проверено и корректно + +- Классификация задачи как инфраструктурной (#562) верна: ни одного файла + класса A, вход сразу на `S7-code-review` правомерен. +- Трейлеры коммита (`Issue: #621`, `User-Visible: no`) верны для этого диффа: + правка не меняет ничего, наблюдаемого пользователем House Plan; changelog не + требуется. +- Единственный коммит, ветка `issue/621-reviews-listing`, дерево совпадает с + заявленным в «Материале раунда» — блок не расходится с фактическим HEAD. +- Замена API технически корректна: спуск `commit.tree.sha → docs → reviews` + адресует именно плоский список файлов в `docs/reviews/` (без рекурсии, + подкаталогов там нет), `truncated:true` трактуется как отказ листинга (я + воспроизвёл, что `jq error()` даёт ненулевой код и обрывает `tree_names`), + а деградация при отказе (пустой список → `spent`/`attempt` по умолчанию, + предупреждение в лог) сохраняет прежнее поведение отказа `contents`. +- YAML синтаксически валиден, изменённый `run`-блок проходит `bash -n` — + собственное требование файла (шапка `process.yml`, п.3) выполнено. +- Тесты добавлены по обоим AC, оба **умеют падать** — подтверждено моей + ручной мутацией (снятие `truncated`-обработки на реальном тексте файла), + тест 63 краснеет, остальные 62 остаются зелёными. +- Других мест в `process.yml`, читающих `docs/reviews` через `contents` с тем + же риском, не найдено (`grep -n "docs/reviews"` — единственное другое + использование, строка ~737, идёт через локальный `git ls-tree` на checkout + с историей, лимита API не имеет и вне скоупа этой задачи). + +## Чего не проверял + +- Живой прогон нового листинга против настоящего GitHub API (см. таблицу + гейтов) — нет сетевого доступа из песочницы ревьюера; первый реальный прогон + состоится на этом же issue. +- `actionlint` недоступен в окружении ревьюера; заменено связкой + `yaml.safe_load` + `bash -n` на изменённый шаг, что покрывает оба класса + ошибок, о которых предупреждает шапка файла (некорректный YAML и heredoc, + обрезающий скрипт). +- Полные тяжёлые гейты (`golden`, browser-смоки, backend pytest, + perf-профили) — не запускал: diff их не касается, AC их не требует. + +## Вердикт + +Зелёный. AC1 и AC2 доказаны исполняемыми тестами, оба умеют падать (проверено +мутацией), код-риск (некорректная трактовка `truncated`) проверен чтением и +воспроизведением семантики `jq`/`gh api`. Единственное отступление — Low, +снятое с запиской выше, не требует правки в этой задаче. + +--- + +## Материал раунда + +``` +tree 1f842cfe8bab1f5716bdb3f32d11533f22abaa84 +blob 3eb81d2a24753cd885ac6f7a0104e24fe36ebc1f .github/workflows/process.yml +blob ec7517895d5e90f53d4bf39db133f735145ddcf6 test/review-doc-guard.test.mjs +SHA: 50e67c0988d91866b06c9967fae8c2f54f88efe9 +``` + +--- + + + +## Материал раунда + +- Ветка: `issue/621-reviews-listing`, коммит `50e67c0988d9` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `1f842cfe8bab1f5716bdb3f32d11533f22abaa84` + ``` + git log --all --format='%H %T' | grep 1f842cfe8bab + ``` +- Тело issue: `4d9b8e6b2263ab99eaeeedb33ad1d9010a5bdfbf079ab627c01949ac9f20ac7f` +- Вердикт конвейера: `green` · High 0