diff --git a/docs/reviews/CODE-REVIEW-646-r1.md b/docs/reviews/CODE-REVIEW-646-r1.md new file mode 100644 index 00000000..fe40e39f --- /dev/null +++ b/docs/reviews/CODE-REVIEW-646-r1.md @@ -0,0 +1,81 @@ +# CODE-REVIEW-646-r1 + +Issue: [#646](https://github.com/Matysh/houseplan-card/issues/646) — temp dirs are removed even when a test fails; lint guards it. +Материал: `fa91f3a7311145e9c3f6e2d22d55aca07f844ef1` (ветка `issue/646-temp-dir-hygiene`, ребейз на `origin/dev` @ `524908b2`), один коммит. Дерево и блобы из блока «Материал раунда» сверены с `git ls-tree HEAD` — совпадают побайтово. +Трек: инфраструктурный, класс B. `Issue: #646`, `User-Visible: no` — оба трейлера на коммите присутствуют; изменений в `src/**` нет, changelog не требуется. + +## Скоуп + +Симптом: `test/golden-capture-provenance.test.mjs`'s `accept()` создавал `mkdtemp(hp-golden-571-baselines-*)` (~20 МБ) и не удалял его ни при успехе, ни при ожидаемом падении — 433 копии за сутки параллельной работы агентов, 8,5 ГБ, ENOSPC. Ещё три места (`bundle-assets`, `docs-accept`, `rebase-generated`) протекали так же. + +Правка: +1. Четыре реальных утечки закрыты — очистка в `finally` или `t.after`. +2. Новый статический lint `test/temp-dir-hygiene.test.mjs`: AST-разбор (TypeScript) `test/**/*.mjs`, требует, чтобы каждый `mkdtemp`/`mkdtempSync` был связан с именем и убран `rm*`/`rmSync`/`rmdir*` в `finally` либо в хуке `after`/`afterEach`/`t.after`, с поддержкой цепочки именованных помощников (до 3 звеньев) и исключением `// tmp-ok: <причина>`. +3. Два мутанта в `scripts/mutation-registry.mjs`, привязанные к guard'у `node --test test/temp-dir-hygiene.test.mjs`. + +Какую работу из `docs/SCOPE.md` это обслуживает: не продуктовую строку J1–J7 напрямую — это внутренняя инфраструктура тестового прогона (гигиена CI/локальной песочницы), предмет `AGENTS.md`/`PROCESS.md`, а не пользовательский сценарий. Продуктового поведения дифф не меняет ни строкой — весь diff лежит в `test/**` и `scripts/mutation-registry.mjs`. + +## Как проверялось + +Зелёный Validate уже подтверждён на этом SHA (https://github.com/Matysh/houseplan-card/actions/runs/36079627502) — `typecheck`, `npm test`, `npm run build` со сверкой бандла и `check-docs` (не по `src/**`, но прогоняется всегда) на этом прогоне не перегонялись. + +| Гейт | Статус | Результат | +|---|---|---| +| Validate на `fa91f3a7` (typecheck/test/build/docs) | Не перегонял, зачёл по ссылке | success | +| `npx tsc -p tsconfig.test.json` | Прогнал | чисто, без вывода | +| `node --test test/temp-dir-hygiene.test.mjs` | Прогнал | 2/2 pass (реальное дерево — 0 нарушений; самопроверка правила — 7 плохих/7 хороших примеров) | +| `node --test` на четырёх изменённых файлах (`golden-capture-provenance`, `bundle-assets`, `docs-accept`, `rebase-generated`) | Прогнал | 73/73 pass | +| Замер `TMPDIR` вокруг того же прогона (свой пустой каталог `/tmp/t-646-verify`) | Прогнал | 0 записей до и после — независимое подтверждение AC1 на изменённых файлах | +| `node scripts/mutation-gate.mjs --id=golden-accept-sandbox-leaks` на `fa91f3a7` | Прогнал сам (автор гонял его на до-ребейзном `98d45838`) | поймано 1 из 1 | +| `node scripts/mutation-gate.mjs --id=temp-hygiene-accepts-cleanup-outside-finally` на `fa91f3a7` | Прогнал сам | поймано 1 из 1 | +| `node scripts/mutation-gate.mjs --check` (полный реестр, 920 мутантов) на `fa91f3a7` | Прогнал сам | rc 0, 920 ok — новые два мутанта не сломали ничего в остальном реестре | +| Ручной просмотр остальных ~55 сайтов `mkdtemp` в `test/**` (`merge-candidate`, `process-gate` и др.) | Прогнал (выборочно) | все используют распознаваемый паттерн `try/finally` — нет признаков, что lint слеп на реальном коде | +| `grep .sandbox` в `golden-capture-provenance.test.mjs` после удаления поля из возврата `accept()` | Прогнал | нет использований — поле убрано целиком и безопасно | + +### AC · чем доказан · чем краснеет + +| AC | Чем доказан | Чем краснеет | Проверено ревьюером | +|---|---|---|---| +| AC1. После полного юнит-набора в `TMPDIR` не остаётся тестовых каталогов | Автор: ручной замер 6 частей полного набора, 0→0, контроль на `origin/dev` — 11 каталогов/160 МБ | Возврат любой из 4 утечек → красный `temp-dir-hygiene` (AC2); мутант `golden-accept-sandbox-leaks` | Частично исполнением: независимый прогон 4 изменённых файлов с чистым `TMPDIR` — 0 записей. Полный набор (все 3048 тестов) не перегонял — соразмерно объёму задачи, ссылаюсь на Validate + собственный частичный прогон | +| AC2. Lint ловит новый `mkdtemp` без очистки | `test/temp-dir-hygiene.test.mjs`: тест 1 — реальное дерево (63 вызова, 0 нарушений, канарейка `sites >= 20`); тест 2 — самопроверка на 7 плохих/7 хороших примерах | Мутант `golden-accept-sandbox-leaks` (снята очистка в `accept()`), мутант `temp-hygiene-accepts-cleanup-outside-finally` (правило принимает очистку вне `finally`) | Исполнением: оба мутанта перегнаны на `fa91f3a7` лично, оба пойманы (1 из 1 каждый) | + +Третий столбец не пуст ни в одной строке, и обе названные мутации перегнаны на материале ревью, а не только на коммите автора до ребейза — это закрывает главный риск раунда (`--id=…` гонялся автором на `98d45838`, а не на `fa91f3a7`). + +## Что проверено и корректно + +- **`accept()` в `golden-capture-provenance.test.mjs`**: `sandbox` больше не покидает функцию — `index`/`stdout`/`error` читаются до `finally`, где `rmSync(sandbox, { recursive: true, force: true })` исполняется всегда, включая путь `expectFailure`. Ни один вызывающий тест больше не читает поле `.sandbox` (проверено grep) и не делает собственный `rmSync(from, …)` в конце теста — заменено на `artifact(t, …)`, снимающий каталог в `t.after` независимо от исхода assert'ов. +- **`bundle-assets.test.mjs`, `docs-accept.test.mjs`, `rebase-generated.test.mjs`**: тот же класс дефекта (очистка отсутствовала или стояла после assert'ов вне `finally`) закрыт стандартным паттерном `t.after`/`try…finally`. Диффы точечные, смысл тестов не менялся — assert'ы и структура тестов совпадают до и после. +- **Lint сам себя не обманывает**: правило отличает «очистка в `finally`» от «очистка после ассертов» (мутант 2 это буквально и проверяет), различает помощника, отдающего каталог целиком, от помощника, отдающего каталог полем объекта, которое вызывающий не забирает (ровно кейс старого `accept()` — предусмотрен намеренно), и требует непустую причину у `tmp-ok:`. +- **Мутанты интегрированы корректно**: guard `node --test test/temp-dir-hygiene.test.mjs` — это тот же тест, который является доказательством AC2, оба мутанта поймал именно он на текущем материале; полный `mutation-gate.mjs --check` (920 мутантов) зелёный — новые два не конфликтуют с остальным реестром. +- **Трейлеры и changelog**: `Issue: #646`, `User-Visible: no` на коммите — верно для чисто тестовой правки без видимого поведения; changelog не тронут и не должен быть. +- **Одно число, один источник**: правка не вводит новых видимых пользователю чисел — нечего сверять по §8. + +## Находки + +Нет ни High, ни Medium, ни Low. + +## Чего не проверял + +- **Полный набор из 3048 тестов под наблюдаемым `TMPDIR`** — не перегонял целиком (это дорогой гейт не по этому диффу); ссылаюсь на Validate (success) плюс собственный независимый прогон четырёх изменённых файлов с чистым `TMPDIR`, давший 0 записей, как частичное, но прямое подтверждение AC1 поверх слов автора. +- **`hp-mutant-*` worktree от оборванных прогонов мутантов** — не читал `scripts/mutation-execution.mjs`; согласен с автором, что это отдельный дефект (утечка из *скриптов*, не из `test/**/*.mjs`, куда сфокусирован lint) и вне AC этой задачи, раз он явно назван в разделе «Чего не проверял» хендоффа, а не спрятан. +- **Windows-путь** — не проверял; `t.after`/`try…finally` платформенно нейтральны, и это разумное необоснованное риском допущение для CI-only паттерна. +- **Полный ручной перебор всех ~63 сайтов `mkdtemp`** — прочитал выборочно (`merge-candidate.test.mjs`, `process-gate.test.mjs`) и опёрся на канарейку `sites >= 20` плюс собственный запуск lint-теста на реальном дереве вместо построчного аудита каждого из 63 сайтов. +- **`scripts/check-docs.mjs`** — не гонял: диффа по `src/**` нет, репозиторий несёт независимо существующее предупреждение о протухшем скриншот-отпечатке (#479), не относящееся к этой правке. + +## Вердикт + +Зелёный. Ни один AC не в скоупе не остался без доказательства, обе защитные мутации перепроверены лично на материале раунда `fa91f3a7`, независимая проверка AC1 на изменённых файлах подтвердила 0 утечек. + +--- + + + +## Материал раунда + +- Ветка: `issue/646-temp-dir-hygiene`, коммит `fa91f3a73111` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `b96dc160827eec6840263e83a0ef2b761c21724a` + ``` + git log --all --format='%H %T' | grep b96dc160827e + ``` +- Тело issue: `6f23efe32c765c152615c83303b8817f3690c5b1bfa19d5cb62475810575f839` +- Вердикт конвейера: `green` · High 0 diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index ce639796..98c3a772 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,9 +1,10 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1038, issue: 365. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 1039, issue: 366. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| +| #646 | [CODE-REVIEW-646-r1.md](CODE-REVIEW-646-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #643 | [CODE-REVIEW-643-r1.md](CODE-REVIEW-643-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #642 | [SPEC-REVIEW-642-r1.md](SPEC-REVIEW-642-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — | | #642 | [CODE-REVIEW-642-r1.md](CODE-REVIEW-642-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — |