diff --git a/docs/reviews/CODE-REVIEW-562-r1.md b/docs/reviews/CODE-REVIEW-562-r1.md new file mode 100644 index 00000000..4f049cff --- /dev/null +++ b/docs/reviews/CODE-REVIEW-562-r1.md @@ -0,0 +1,197 @@ +# CODE-REVIEW-562-r1 + +## Скоуп + +Issue #562 «Process: отвязать роли и инфраструктурный трек от конкретных агентов» — +инфраструктурная задача (класс A не задет ни одним файлом): `AGENTS.md`, +`PROCESS.md`, `scripts/process-gate.mjs`, `scripts/task-packet.mjs`, +`test/process-gate.test.mjs`, `test/task-packet.test.mjs`. Материал ревью — +ровно `53f0635eddcd66346bca3bc2a92056d333ed1ca3` (единственный коммит впереди +`origin/dev`, `merge-base` = `9c08d583`). Трейлеры: `Issue: #562`, +`User-Visible: no` — корректны, changelog не требуется и не тронут. + +Заход r1, цикл 0/4. + +## Как проверялось + +| Гейт | Результат | Источник | +|---|---|---| +| `npx tsc --noEmit` | зелёный | Validate на `53f0635e`, https://github.com/Matysh/houseplan-card/actions/runs/34742978297 (переиспользован по указанию, не перегонялся) | +| `npm test` (полный) | зелёный, 2560/2559+1 skip | тот же прогон Validate | +| `npm run build` + сверка бандлов | зелёный | тот же прогон Validate | +| `node test/task-packet.test.mjs` (таргетно) | зелёный, 8/8 | прогнан в этом ревью | +| `node test/process-gate.test.mjs` (таргетно) | зелёный, 35/35 | прогнан в этом ревью | +| `node scripts/check-docs.mjs` | не запускался | diff не трогает `src/**` — правило §8 не требует | +| смоки (`demo/smoke_*.mjs`) | не запускались | diff не трогает `demo/**`/`src/**`; `smoke-select` по этому диффу тоже неприменим | +| `golden:verify` | не запускался | нет визуальных изменений | +| `pytest tests_backend` | не запускался | Python не тронут | +| `npm run invariants` | не запускался | геометрия не тронута | + +Основная работа этого раунда — чтение диффа против `origin/dev` (`git diff +origin/dev...HEAD`), сверка `AGENTS.md`/`PROCESS.md` с телом issue и +воспроизведение поведения `scripts/task-packet.mjs` (`rightsFor`) напрямую +через `node -e`, а не только чтением. + +## AC → как доказан + +- **AC1** (AGENTS.md/PROCESS.md/локальный CODEX-RUNBOOK.md не закрепляют роли + за Codex/Claude) — проверено чтением. `AGENTS.md:204` и `PROCESS.md:516,913` + упоминают Codex/Claude только как равноправные примеры «любой агент»; + `PROCESS.md:911-912` называет `anthropics/claude-code-action` текущей + технической реализацией ревьюера и прямо оговаривает, что это деталь + автоматизации, а не закрепление роли. Локальный `CODEX-RUNBOOK.md` + (`C:\Users\Admin\...`) физически вне репозитория и вне материала ревью — + не проверялся, проверить нечем. +- **AC2** (оба документа одинаково описывают маршруты S1–S8 и инфра-без-S → + S7 ↔ S6 → S8) — проверено чтением. Формулировка идентична по существу в + `AGENTS.md` («Agent-neutral workflow») и `PROCESS.md` (§1, диаграмма §2, + §9, блок §14). CODEX-RUNBOOK.md — не проверялся (вне репозитория). +- **AC3** (независимость автора/ревьюера, автоматическое ревью, автоматическое + слияние, прямое разрешение владельца на выпуск) — проверено чтением. §6 и + §10.4 `PROCESS.md`, соответствующий раздел `AGENTS.md` не тронуты по + механике, только переформулированы; поведение конвейера (событие от метки, + ребейз/слияние, `S8-merged` после push) не изменилось. +- **AC4** (исторические документы ревью не переписаны) — доказано `git diff + origin/dev...HEAD --stat`: `docs/reviews/**` в диапазоне отсутствует. + +## Находки + +### Medium — `task-packet.mjs` определяет инфра-трек и право трогать класс A по тематической метке `infra`, а не по механическому критерию, и это противоречит соседнему модулю в том же диффе + +- **Файл:** `scripts/task-packet.mjs:29-30,36-38,49-51,115-117` +- **Что не так:** `rightsFor` и `buildPacket` относят issue к «инфраструктурному» + треку и запрещают класс A по одному признаку — наличию метки `infra` в + `labels`. Но: + 1. `PROCESS.md` §9 (эта строка диффом не тронута) прямо называет `infra` + **тематической меткой, ортогональной процессу**, наравне с `polish`, + `tests`, `docs`, `security`, `vacuum` — то есть меткой, которая ничего + не решает и может стоять на любой продуктовой задаче любого трека. + 2. Механический критерий инфраструктурной задачи, который эта же задача + подтверждает и в `AGENTS.md`, и в `PROCESS.md` §1, — «ни одного файла + класса A в диапазоне», а не метка. + 3. `scripts/process-gate.mjs:431-434` (комментарий сохранён этим же диффом, + не переписан) объясняет ровно это: «Исключение опирается на diff, а не + на метку-разрешение: метку `infra` можно поставить продуктовой задаче и + увести продуктовый коммит от проверки статуса». Реальный гейт + (`isInfrastructureRange`) действительно смотрит только на классы файлов + коммитов, не на метки issue. + + `task-packet.mjs` — единственное место в этом диффе, которое читает метку + `infra` как источник правды об «инфраструктурности», и делает это ровно там, + где `process-gate.mjs` в том же диффе объясняет, почему так делать нельзя. + +- **Воспроизведение (не только чтением — вызовом функции):** + + ``` + $ node -e "import('./scripts/task-packet.mjs').then(({ rightsFor }) => + console.log(rightsFor('S6-in-progress', ['infra'])))" + [ + 'файлы класса A трогать НЕЛЬЗЯ; инфраструктурную реализацию МОЖНО вести сразу по issue (#562)', + 'следующий шаг: gate:small + смоки по AC → push ветки → метка S7-code-review (метку после push)' + ] + ``` + + По правилу №1 issue в `S6-in-progress` разрешено трогать класс A — это + единственный смысл статуса. Метка `infra`, по документированному §9 + значению, орthogональна и не должна это отменять (пример: полноценная + продуктовая задача с темой «инфраструктура/бэкенд», прошедшая полный + S1→S8, помечена и `S6-in-progress`, и тематически `infra` — ничто в + процессе такое сочетание не запрещает). Для такой задачи `task-packet.mjs` + выдаёт заведомо неверный ответ на прямой вопрос «можно ли трогать класс A» + — тот самый вопрос, ради которого инструмент существует + (`права выводятся из статусной метки по правилу №1`, комментарий в + `test/task-packet.test.mjs:13`). + + Это не гипотетический контрпример: в репозитории уже есть открытые issue с + меткой `infra` одновременно со статусной `S*`-меткой (#557 `S8-merged`, + #541 `S6-in-progress`) — оба сейчас действительно инфраструктурные по + содержанию, так что конкретно на них ответ инструмента случайно верен, но + ничто в процессе не гарантирует, что продуктовая задача не получит ту же + комбинацию меток. + +- **Почему это не поймали тесты:** новый тест + `test/task-packet.test.mjs:19-23` проверяет только `rightsFor(null, + ['infra'])` (статуса нет вообще) — счастливый путь, совпадающий с + ментальной моделью автора. Нет ни одного теста на `infra` вместе с реальной + `S*`-меткой, то есть ровно на тот случай, для которого модуль отвечает + неверно. +- **Серьёзность:** Medium, в скоупе задачи (файл изменён этим же диффом). + Не High: `task-packet.mjs` — консультативный инструмент («источник правды + остаётся GitHub и git: пакет ничего не пишет и ничего не решает», собственный + комментарий модуля), реальный блокирующий гейт (`process-gate.mjs`, + pre-push/CI) метку `infra` не читает и остаётся диффо-зависимым, так что + ни один реальный push не будет пропущен или отклонён из-за этого дефекта. + Риск — недостоверная подсказка агенту/человеку, читающему пакет задачи, + вплоть до попытки продуктовой правки под видом инфраструктурной или + наоборот отказа от легитимной правки класса A. +- **Что чинить:** либо не выводить «инфраструктурность» из метки `infra` + вовсе (использовать наличие/отсутствие `S*`-метки как единственный + предсказуемый по issue-данным сигнал — без диапазона коммитов `task-packet` + и так не может проверить класс файлов, значит стоит явно писать «трек + неизвестен без diff» вместо утвердительного «нельзя/можно»), либо + однозначно developed заново привязать «инфраструктурный трек» к отсутствию + `S*`-метки (что уже и так вычисляется как `status === null`), а не к метке + `infra`, и добавить тест на комбинацию `infra` + реальный `S*`. + +## Что проверено и корректно + +- Единственный коммит в диапазоне, трейлеры `Issue: #562` / `User-Visible: no` + верны и соответствуют характеру изменения (документация процесса + два + файла класса B, ни одного класса A). +- `AGENTS.md` и `PROCESS.md` согласованно описывают продуктовый маршрут + `S1`→`S8` и инфраструктурный `без S → S7-code-review ↔ S6-in-progress → + S8-merged»; терминология («ускоренный вход», «accelerated entry») совпадает + по смыслу в обоих документах. +- Ни Codex, ни Claude не закреплены нормативно ни за одной ролью ни в + `AGENTS.md`, ни в `PROCESS.md`; единственное оставшееся упоминание + `anthropics/claude-code-action` явно помечено как деталь текущей + автоматизации, а не правило. +- `scripts/process-gate.mjs`: правки — только текст сообщений/комментариев + (замена ссылки `#118` → `#562`, уточнение формулировки); логика + `isInfrastructureRange`, `checkIssueStatuses`, `statusOptional` не менялась + и остаётся диффо-зависимой, как и было. Соответствующие тесты + (`test/process-gate.test.mjs`) — тоже только переименования, проходят + 35/35. +- `docs/reviews/**` не тронут — AC4 выполнено буквально. +- Изменение не задевает `src/**`, `custom_components/**/*.py`, i18n, + манифесты — инфраструктурная классификация задачи верна механически. + +## Чего не проверял и почему + +- Локальный `CODEX-RUNBOOK.md` владельца — физически вне репозитория и вне + материала ревью (SHA `53f0635e` его не содержит); проверить существование + и содержание невозможно с этой стороны. Это ограничение AC1/AC2 для + третьего документа, а не находка. +- `node scripts/check-docs.mjs`, браузерные смоки, `golden:verify`, `pytest + tests_backend`, `npm run invariants` — не гоняны: diff не касается + `src/**`, `demo/**`, Python и геометрии, поэтому по §8 они не обязательны + для этой задачи. +- Полный `npx tsc --noEmit` / `npm test` / `npm run build` не перегонялись + заново — переиспользован зелёный прогон Validate на этом самом SHA + (ссылка выше), как разрешено инструкцией по этой задаче; вместо этого + таргетно перегнаны оба изменённых test-файла напрямую. +- Реальное использование `task-packet.mjs` агентами на живых issue (кроме + приведённого воспроизведения `rightsFor`) — инструмент не входит в + блокирующий гейт, поэтому эффект находки ограничен качеством подсказки, а + не корректностью пайплайна; это отражено в оценке серьёзности как Medium, + а не High. + +## Материал раунда + +- SHA материала: `53f0635eddcd66346bca3bc2a92056d333ed1ca3` +- Диапазон: `origin/dev(9c08d583)..HEAD`, один коммit +- Дерево: рабочая копия на этом SHA во время всего разбора + +--- + + + +## Материал раунда + +- Ветка: `issue/562-agent-neutral-workflow`, коммит `53f0635eddcd` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `5eef2a99a76777df0a8642b5156e6755083e8f8e` + ``` + git log --all --format='%H %T' | grep 5eef2a99a767 + ``` +- Тело issue: `520e2e2d0230ee82ee9e86795aa5e61e4172442aa24bea85a9429dd5aa7677ae` +- Вердикт конвейера: `yellow` · High 0