diff --git a/docs/reviews/CODE-REVIEW-562-r2.md b/docs/reviews/CODE-REVIEW-562-r2.md new file mode 100644 index 00000000..c9f4e2b8 --- /dev/null +++ b/docs/reviews/CODE-REVIEW-562-r2.md @@ -0,0 +1,200 @@ +# CODE-REVIEW-562-r2 + +## Скоуп + +Issue #562 «Process: отвязать роли и инфраструктурный трек от конкретных +агентов», заход r2, блокирующих циклов израсходовано 1/4 до этого раунда. + +Материал ревью — ровно `8f000dd459393cd0d17e9cfa58513a68f7f5ffa1` (рабочая +копия уже на нём). Полный диапазон против `dev`: `origin/dev(9c08d583)..HEAD` +(два содержательных коммита — `53f0635e` разобран и получил жёлтый вердикт в +r1, `8f000dd4` — правка этого раунда; `00ac0875` — только документ r1). + +Раунд не первый и предыдущий вердикт был жёлтым (Medium в скоупе), поэтому по +PROCESS.md §2.9 разбор ведётся по дельте: находка r1 плюс всё, до чего дельта +дотягивается, а не полный повторный прогон. Дельта раунда — ровно +`53f0635e..8f000dd4`: + +``` +scripts/task-packet.mjs | 16 +++++++++++----- +test/task-packet.test.mjs | 17 +++++++++++++++-- +2 files changed, 26 insertions(+), 7 deletions(-) +``` + +Это не ребейз (родитель тот же `dev`-merge-base `9c08d583`, что и в r1), +контракт поведения затронут ровно там же, где была находка r1, новая +подсистема не задета — сокращение объёма законно. + +Трейлеры коммита `8f000dd4`: `Issue: #562`, `User-Visible: no` — верны +(правка внутри `scripts/**` и `test/**`, класс A не тронут, changelog не +нужен и не тронут). + +## Как проверялось + +| Гейт | Результат | Источник | +|---|---|---| +| `npx tsc --noEmit` / `npm test` (полный) / `npm run build` + сверка бандлов | зелёный | Validate на `8f000dd4`, https://github.com/Matysh/houseplan-card/actions/runs/34743589488 (переиспользован по инструкции, не перегонялся) | +| `node --test test/task-packet.test.mjs` (таргетно, по дельте) | зелёный, 9/9 | прогнан в этом ревью | +| Тест на старом коде (`53f0635e`) с тем же файлом теста | красный, 2/9 | прогнан в этом ревью через `git worktree` — доказывает, что новый негативный тест умеет падать (см. находки закрытия r1 ниже) | +| `node scripts/smoke-select.mjs --base 53f0635e --head 8f000dd4` | «Исполняемого frontend-диффа нет... Тронуто файлов: 3» | прогнан; смоки не выбираются — диффу нечего проверять браузером | +| `node scripts/check-docs.mjs` | не запускался | дельта не трогает `src/**` | +| `golden:verify`, `pytest tests_backend`, `npm run invariants` | не запускались | нет визуальных/Python/геометрических изменений | + +## Закрытие раунда r1 + +| Находка r1 | Чем закрыта | Где это видно | +|---|---|---| +| Medium — `task-packet.mjs` определял «инфраструктурный» трек и запрет класса A по тематической метке `infra`, а не по механическому критерию из `process-gate.mjs`, из-за чего продуктовая задача с меткой `infra` + `S6-in-progress` получала неверный ответ «класс A трогать нельзя» | `rightsFor` больше не читает `infra` из `labels` вообще — трек передаётся явным параметром `{ infrastructure }`; `buildPacket` вычисляет его из `branch.infrastructure`, а тот в `collectInputs` — из реального diff `base..ref` через переиспользованный `classify()` из `process-gate.mjs` (тот же критерий, что у блокирующего гейта) | `scripts/task-packet.mjs:22,27,30,110-121,209-210`; воспроизведение — `node -e "rightsFor('S6-in-progress', ['infra'])"` теперь возвращает `'продуктовый код трогать МОЖНО'`, а не запрет | +| (следствие) отсутствие теста на комбинацию `infra` + реальная `S*`-метка | новый тест `test/task-packet.test.mjs:112-120` явно бьёт `labels: ['infra', 'S6-in-progress']` и требует `track === 'полный'` + разрешение на класс A | проверено запуском теста против кода r1 (`53f0635e`) — падает с `AssertionError: expected 'полный', actual 'инфраструктурный'`, то есть тест действительно ловит старый баг, а не проходит тавтологически | + +Находка r1 закрыта полностью для того сценария, для которого была написана +(ложное «нельзя» для помеченной `infra` продуктовой задачи). Дельта, однако, +меняет ещё одну ветку того же кода (как трек вычисляется, когда ветки ещё +нет) — это и есть новая находка этого раунда, см. ниже. + +## Унаследовано из r1 + +Приняты без повторной проверки (дельта этого раунда их не касается): + +- **AC1** (роли не закреплены за Codex/Claude в `AGENTS.md`/`PROCESS.md`; + локальный `CODEX-RUNBOOK.md` вне материала) и **AC2** (оба документа + одинаково описывают маршруты S1–S8 и инфра-без-S → S7 ↔ S6 → S8) — + `AGENTS.md` и `PROCESS.md` дельтой `8f000dd4` не тронуты вовсе (диф + ограничен `scripts/task-packet.mjs` и `test/task-packet.test.mjs`). + Документ: `docs/reviews/CODE-REVIEW-562-r1.md`, SHA `53f0635e`. +- **AC3** (независимость автора/ревьюера, автоматическое ревью и слияние, + разрешение владельца на выпуск) — механика конвейера этой дельтой не + затронута. Документ: тот же, SHA `53f0635e`. +- **AC4** (исторические документы ревью не переписаны) — дельта `8f000dd4` + не касается `docs/reviews/**` (см. `git show --stat 8f000dd4` в скоупе + выше — только `scripts/` и `test/`). Документ: тот же, SHA `53f0635e`. +- `scripts/process-gate.mjs` и `test/process-gate.test.mjs` — не изменены + этой дельтой (последний раз правились в `53f0635e`, только переименования + сообщений/ссылки на issue); логика `isInfrastructureRange` не тронута. + Документ: тот же, SHA `53f0635e`. + +## Находки + +### Medium (в скоупе) — после фикса статусless infra-issue без опубликованной ветки получает заведомо неверную подсказку «вне процесса, ждать S-метку владельца» + +- **Файл:** `scripts/task-packet.mjs:118` (`const infrastructure = branch?.infrastructure === true;`) и `scripts/task-packet.mjs:39-51` (`rightsFor`, ветка `default`). +- **Что не так:** новый критерий полностью корректен, когда ветка уже + опубликована и `collectInputs` успел посчитать diff. Но пока ветки на + `origin/issue/NN-*` ещё нет (`refs[0]` пуст в `collectInputs`), `branch` + остаётся `null`, и `infrastructure` теперь **всегда** `false` — старый + путь через `labels.includes('infra')`, который умел ответить верно даже + без ветки, убран целиком, а альтернативы для случая «ветки ещё нет» не + появилось. + + Это ровно момент, для которого сама задача #562 вводит ускоренный трек: + «Инфраструктурная задача выполняется сразу... без стадий S1–S6 до первого + хендоффа. По готовности ветка публикуется» (тело issue). И это ровно + прописанный сценарий использования инструмента: `AGENTS.md:378` — «Start a + task from its packet... node scripts/task-packet.mjs --issue NN» — команда, + которую в первую очередь запускают при взятии задачи, то есть до того, как + какая-либо ветка опубликована. + +- **Воспроизведение:** + + ``` + $ node -e "import('./scripts/task-packet.mjs').then(({ buildPacket }) => + console.log(buildPacket({ + issue: { number: 999, title: 'infra no branch yet', state: 'OPEN', url: 'u', body: '' }, + labels: ['infra'], + })))" + track: полный + rights: [ + 'продуктовый код трогать НЕЛЬЗЯ: статус не S5/S6/S7 (правило №1)', + 'статусной метки нет — продуктовая задача вне процесса; вход — первая S*-метка владельца' + ] + ``` + + На коде r1 (`53f0635e`) тот же вызов давал верный ответ: + + ``` + track: инфраструктурный + rights: [ + 'файлы класса A трогать НЕЛЬЗЯ; инфраструктурную реализацию МОЖНО вести сразу по issue (#562)', + 'инфраструктурный вход: реализовать и проверить → push ветки → S7-code-review; ТЗ и S1–S6 не нужны (#562)' + ] + ``` + + Это регресс, внесённый именно правкой этого раунда, а не унаследованная + проблема: до `8f000dd4` тот же вход обрабатывался верно. + +- **Почему это не поймал новый тест:** оба новых/изменённых теста этого + раунда (`test/task-packet.test.mjs:20-24,112-120`) передают `branch` + явным объектом с уже посчитанным `infrastructure`. Ни один тест не + вызывает `buildPacket`/`rightsFor` для statusless `infra`-issue **без** + `branch` — то есть без ветки, что и есть штатное состояние в момент + «взять задачу и вызвать пакет». + +- **Серьёзность:** Medium, в скоупе (файл и тест изменены этим же диффом). + Не High: `task-packet.mjs` — консультативный инструмент, ничего не решает + и не блокирует ни один реальный push; блокирующий гейт (`process-gate.mjs`) + метку не читает и этой правкой не затронут. Риск — неверная подсказка + ровно в момент, когда агент решает, можно ли начинать инфраструктурную + реализацию без S-метки: вместо разрешения «делай сразу» пакет теперь + говорит «жди S-метку от владельца», что для инфра-трека прямо противоречит + тексту issue #562 и `PROCESS.md`. +- **Что чинить:** для случая без ветки не возвращать уверенное «вне + процесса» — либо явно писать неопределённость («трек не подтверждён без + diff опубликованной ветки; по метке `infra` похоже на инфраструктурный»), + либо (эквивалент предложения r1, вариант «привязать трек к metки без + diff», применённый только к before-branch состоянию) при отсутствии ветки + временно доверять `labels.includes('infra')` как единственному + доступному сигналу, но маркировать его в выводе как непроверенный + diff'ом, а не как решённый факт. + +## Что проверено и корректно + +- Находка r1 закрыта именно так, как и была сформулирована: `task-packet.mjs` + для мисклассифицированной продуктовой задачи (`infra` + реальная + `S6-in-progress`) больше не запрещает класс A — трек и права теперь + выводятся из того же механического критерия (`classify()` из + `process-gate.mjs`), что и у блокирующего гейта, а не из темы issue. +- Новый тест `#562: the infra label alone never grants the accelerated + track` — не тавтологичен: проверено падением на снапшоте `53f0635e` + (`AssertionError`, 2 упавших теста из 9). +- Тесты дельты: 9/9 зелёных на `8f000dd4`. +- Трейлеры `Issue: #562` / `User-Visible: no` верны; класс A не тронут; + changelog не требуется и не тронут — соответствует характеру правки. +- `docs/reviews/**` этой дельтой не тронут — AC4 остаётся выполненным. +- `smoke-select` подтверждает: diff не задевает `src/**`/`demo/**`, браузерные + смоки этому раунду не нужны. + +## Чего не проверял и почему + +- Полный `npx tsc --noEmit` / `npm test` / `npm run build` не перегонялись — + переиспользован зелёный Validate на этом самом SHA `8f000dd4` (ссылка + выше); вместо этого таргетно перегнан изменённый тестовый файл и его же + версия на предыдущем SHA (для проверки «тест умеет падать»). +- `AGENTS.md`, `PROCESS.md`, локальный `CODEX-RUNBOOK.md` — не перечитывались + заново: дельта раунда их не трогает (см. «Унаследовано из r1»). +- `check-docs`, browser-смоки, `golden:verify`, `pytest tests_backend`, + `npm run invariants` — не запускались: дельта ограничена `scripts/**` и + `test/**`, ни `src/**`, ни `demo/**`, ни Python, ни геометрия не тронуты. +- Реальное поведение `scripts/process-gate.mjs` (блокирующий гейт) — не + перепроверялось: этой дельтой файл не изменён, вывод r1 (диффо-зависимый, + метку `infra` не читает) остаётся в силе. + +## Материал раунда + +- SHA материала: `8f000dd459393cd0d17e9cfa58513a68f7f5ffa1` +- Дельта раунда: `53f0635eddcd66346bca3bc2a92056d333ed1ca3..8f000dd459393cd0d17e9cfa58513a68f7f5ffa1` (2 файла, 26 строк) +- Полный диапазон против `dev`: `origin/dev(9c08d583)..HEAD` +- Дерево: рабочая копия на этом SHA во время всего разбора + +--- + + + +## Материал раунда + +- Ветка: `issue/562-agent-neutral-workflow`, коммит `8f000dd45939` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `36ef122f5778df5144077d79a3584a54a421ee1e` + ``` + git log --all --format='%H %T' | grep 36ef122f5778 + ``` +- Тело issue: `520e2e2d0230ee82ee9e86795aa5e61e4172442aa24bea85a9429dd5aa7677ae` +- Вердикт конвейера: `yellow` · High 0