diff --git a/docs/reviews/INDEX.md b/docs/reviews/INDEX.md index 259ddbf3..cfe29ccf 100644 --- a/docs/reviews/INDEX.md +++ b/docs/reviews/INDEX.md @@ -1,10 +1,11 @@ # Индекс ревью -Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 230, issue: 110. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. +Генерируется `node scripts/reviews-index.mjs` (#635) — не редактировать руками. Документов: 231, issue: 111. Вердикт: 🟢 зелёный · 🟡 жёлтый · 🔴 красный · ⚪ не распознан (свободная форма старых документов). H/M — число High/Medium по строке вердикта или заголовкам находок. Файлы — пути, названные в находках; ищите по имени файла: `grep form-kit INDEX.md`. | Issue | Документ | Этап · раунд | Вердикт | H | M | Находки | Файлы | |---|---|---|---|---:|---:|---|---| | бета v1.79.0-beta.1 | [SHIP-REVIEW-v1.79.0-beta.1.md](SHIP-REVIEW-v1.79.0-beta.1.md) | пакетное ревью ship · — | ⚪ — | 0 | 0 | — | — | +| #737 | [SPEC-REVIEW-737-r1.md](SPEC-REVIEW-737-r1.md) | spec · r1 | 🟢 зелёный | 0 | 0 | — | — | | #732 | [CODE-REVIEW-732-r1.md](CODE-REVIEW-732-r1.md) | code · r1 | 🟢 зелёный | 0 | 0 | — | — | | #730 | [CODE-REVIEW-730-r1.md](CODE-REVIEW-730-r1.md) | code · r1 | 🟡 жёлтый | 0 | 1 | текст сводки «повтор и ребейз не помогут» вводит в заблуждение именно в сценарии, котор… | `scripts/merge-candidate.mjs` | | #730 | [CODE-REVIEW-730-r2.md](CODE-REVIEW-730-r2.md) | code · r2 | 🟢 зелёный | 0 | 0 | — | — | diff --git a/docs/reviews/SPEC-REVIEW-737-r1.md b/docs/reviews/SPEC-REVIEW-737-r1.md new file mode 100644 index 00000000..093f68b0 --- /dev/null +++ b/docs/reviews/SPEC-REVIEW-737-r1.md @@ -0,0 +1,208 @@ +# SPEC-REVIEW-737-r1 + +## Материал раунда + +- Issue #737, тело на момент ревью (раздел `## ТЗ`, полный текст прочитан и + процитирован ниже построчно). +- Рабочая копия репозитория — `3b9f25ea659c645baded44a82c15ed635798b977` + (`origin/dev`), использована только для проверки фактических утверждений ТЗ + о текущем состоянии кода (К1–К7, Проблема, Затронутые файлы). Продуктовый код + этой задачей ещё не тронут — в `dev` его нет, и это ожидаемо для этапа spec. +- Внешняя зависимость, факты по которой сверены: `anthropics/claude-code-action` + на пине `9cdae7f0d995e3ba7c33f226087fdf82a59cd520` (указан в + `.github/workflows/_process.yml:1142` и `_ship-review.yml:191` — совпадает с + тем, что назвал автор ТЗ). + +## Скоуп + +Задача (трек `ask`, инфраструктура, `User-Visible: no`) учит три стадии модели +(`spec`/`code`/`ship`-ревью) снимать расход токенов из `execution_file` +`claude-code-action` и публиковать его одной машинной строкой в документе +ревью; читатель `process-metrics.mjs` (#728, К6) получает настоящий источник +данных вместо фиктивного регэкспа. Семь контрактных пунктов К1–К7, семь AC, +план автотестов и граничные случаи — все описаны в теле issue. + +## Как проверялось + +Ревьюер ≠ исполнитель, устных пояснений нет — только текст issue. Для ТЗ, +которое утверждает конкретные факты о чужом API (а не о продукте House Plan), +проверка «не бывает дешёвым гейтом» означает: свериться с реальным источником, +а не поверить формулировке. Сделано построчно: + +1. Прочитаны требуемые §7.1 разделы, сверена нумерация AC↔К и их полнота. +2. Каждая ссылка на текущий код (`scripts/review-doc-guard.mjs`, + `scripts/ship-review.mjs`, `scripts/process-metrics.mjs`, оба workflow, + `scripts/review-result-gate.mjs`) открыта и сверена построчно с номерами + строк, которые называет ТЗ. +3. Внешние факты о `claude-code-action` на названном пине и о пакетах + `@anthropic-ai/claude-agent-sdk` / `@anthropic-ai/sdk` получены `curl` с + `raw.githubusercontent.com` / `registry.npmjs.org` (сеть в среде ревью + доступна) — это единственный способ отличить проверенный факт от догадки, + выглядящей как факт (§7.1). +4. Проверено, что новые файлы (`scripts/model-usage.mjs`, + `test/model-usage.test.mjs`) в дереве действительно отсутствуют, а файлы, + которые ТЗ обещает только расширить, существуют. +5. Трек и зависимости (#726, #727, #728) сверены по `git log`: все три + `S8-merged` до этого issue, предположения ТЗ о их API актуальны на HEAD. + +Гейты (`typecheck`/`npm test`/`npm run build`) на этом этапе неприменимы: кода +ещё нет, стадия — ревью ТЗ, а не ревью кода (§2.4). Это не сокращение объёма, а +корректный объём для spec review. + +## Находки + +Находок уровня High или Medium нет. + +Low — ни одной с записью на снятие: формулировки избыточно подробны местами +(например, повтор одного и того же тезиса в «Проблема» и «Принято +предположительно» п.5), но это не дефект проверяемости, а стиль; снимать нечего +чинить не в праве ревьюера менять текст. + +## Что проверено и корректно + +- **Обязательные разделы §7.1 — все на месте**: сценарий, что человек увидит до + и после, проблема, скоуп/не-скоуп, контракт поведения, UX·данные·i18n· + миграция (один комбинированный раздел, содержание покрывает все подпункты + явно словом «нет» там, где неприменимо), критерии приёмки AC1–AC7 с + доказательством и oracle, план автотестов, риски, откат, release-артефакты. + DoR (§2.5) по каждому пункту закрывается однозначно: i18n — нет, миграция — + нет, touch/perf — нет и обоснованно («секунда на шаг CI»), release-артефакты + — названы явным «нет» + `PROCESS.md §10.4`, откат — описан (revert коммитов, + публикации не трогаются), открытых продуктовых вопросов нет. +- **Факты о собственном коде репозитория верны**, построчно: + - `materialAnchorBlock`, `withMaterialAnchors`, `anchorVerdictFrom`, + `anchorTreeFrom`, `anchorIssueBodyFrom`, `materialAnchorsFrom`, + `reusableGreenVerdict` — все существуют в `scripts/review-doc-guard.mjs` + под теми именами, что называет ТЗ (строки 117, 281, 315, 385, 517, 526, + 556). + - `materialAnchorsFrom` действительно ищет 40-символьные hex-подстроки после + `ANCHOR_MARKER` (`scripts/review-doc-guard.mjs:110–121`) — формат К2, + запрещающий 40 hex-символов и `-->`/перевод строки внутри строки `hp:usage`, + предотвращает ложное распознавание якоря именно этим механизмом. + - `anchorBlock`/`parseAnchorBlock`/`SHIP_REVIEW_ANCHOR` в + `scripts/ship-review.mjs` — на месте (строки 47, 345, 372), содержимое + совпадает с описанием К5. + - `_ship-review.yml`: `model_review` (строка 122) **действительно не имеет** + `outputs:` — ровно то расхождение, которое К3 называет проблемой и которое + К3/AC5 обещают исправить. + - `_process.yml`: шаг `Review` имеет `id: review` (строка 1141), `outputs:` + job несёт только `duration_seconds` (строки 1013–1014) — совпадает с + текстом ТЗ «рядом с `duration_seconds`». + - `scripts/process-metrics.mjs`: `TOKENS_NO_DATA`, `USAGE_LINE_RE`, + `tokenUsage` — присутствуют (строки 198, 217, 448), и воспроизведён именно + тот дефект, который ТЗ описывает как текущую проблему: `found` ставится + в `true` по самому факту совпадения регэкспа независимо от того, нашлись + ли внутри пары `ключ=число` — то есть «строка без чисел» сегодня + действительно считается документом с пустой суммой, как и написано в ТЗ. + - `fetchSnapshot`/`git grep -l -e hp:usage` (строки ~1073–1080) при + совпадении включает в выборку документов и `hp:usage-none` — подстрока + `hp:usage` в нём есть; К6 в курсе этого и опирается на это явно. + - `REQUIRED_FILES` в `scripts/review-result-gate.mjs:25` действительно не + содержит ничего про расход — граница доверия (#556), которую К3 выбирает + не трогать, описана верно. + - Тестовые файлы, которые ТЗ обещает расширить + (`test/review-doc-guard.test.mjs`, `test/publish-push-refusal.test.mjs`, + `test/ship-review.test.mjs`, `test/process-metrics.test.mjs`, + `test/default-branch-workflows.test.mjs`) существуют; новые + (`scripts/model-usage.mjs`, `test/model-usage.test.mjs`) отсутствуют — + ровно то, что ожидается до реализации. + - `STEP_SCRIPTS`/`importClosure` в `test/publish-push-refusal.test.mjs:57–58` + действительно включает `review-doc-guard.mjs` в замыкание импортов — если + реализация подключит новый модуль оттуда, тест подхватит его сам, как и + заявляет АС3. + - `npm run gate:small`, `node scripts/mutation-gate.mjs --check`, + `node scripts/reviews-index.mjs --check` (AC7) — все существуют. + - В `PROCESS.md` §10.4 абзаца «Расход модели» пока нет — задача действительно + добавляет новый текст, а не дублирует существующий. +- **Факты о внешней зависимости `claude-code-action` на названном пине — + верны**, сверено по исходнику на GitHub и по пакетам npm: + - `base-action/action.yml` (используется top-level `action.yml`, который его + оборачивает) отдаёт `outputs.execution_file` — «Path to the Claude Code + execution output file»; совпадает с ТЗ. + - `execution-file.ts`: файл — `$RUNNER_TEMP/claude-execution-output.json`, + `writeExecutionFile` вызывается и при нормальном завершении, и в `catch` + при ошибке SDK (`run-claude-sdk.ts`, строки ~208–222) — совпадает с + формулировкой «пишется после конца сессии, и при ошибке SDK тоже». + - `sanitizeModelUsage`/`sanitizeSdkOutput` и комментарий «без утечки token + usage or cost details» — на месте, строка комментария ровно та, что + процитирована в ТЗ. + - Тип `SDKResultSuccess` (пакет `@anthropic-ai/claude-agent-sdk`, актуальная + версия 0.3.286) несёт оба поля: `modelUsage: Record` + (по моделям, camelCase: `inputTokens`, `outputTokens`, + `cacheReadInputTokens`, `cacheCreationInputTokens`) и `usage: + NonNullableUsage` — тип на основе `BetaUsage` из `@anthropic-ai/sdk`, где + поля снэйк-кейсом: `cache_creation_input_tokens`, + `cache_read_input_tokens` (и стандартные `input_tokens`/`output_tokens` + рядом). Ровно то разделение camelCase/snake_case, на которое опирается К1. + - Тестовая фикстура `base-action/test/run-claude-sdk.test.ts` (строки + 83–108) использует именно эти ключи и `num_turns` — К1 не придумывает + формат, а списывает его с реального контракта action на этом пине. + - Пин `9cdae7f0d995e3ba7c33f226087fdf82a59cd520` в workflow совпадает с тем, + что называет ТЗ, в обоих файлах. +- **AC однозначны и проверяемы**: каждый из AC1–AC7 называет конкретный тестовый + файл, точку входа и ожидаемый результат; у защитных случаев (AC1 — коды + причин «нет данных», AC2 — отклонение искажённого формата, AC3/AC4 — + неизменность старых якорей/блока, AC6 — счёт только по машинному блоку) + названо «чем краснеет» прямо в таблице и/или в разделе «План автотестов» — + требование §2.7 о непустом третьем столбце для защитного AC выполнено + текстуально, хотя формально это код-ревью просит таблицу «AC · доказано · + краснеет»; здесь эквивалент есть в объединённом виде. +- **Явный блок «Принято предположительно»** разделяет продуктовые решения (нет: + весь интерфейс — внутренний, процессный) от технических допущений (имена + ключей, канал передачи, формат «нет данных», позиция строки в ship-документе, + источник чтения в process-metrics, `if: always()`, свобода в именовании + модуля/функций) — ревьюер эти допущения не оспаривает: они согласуются с уже + принятыми прецедентами (`duration_seconds` как job output, `#556` граница + доверия, принцип «один модуль — одна сборка/разбор формата» уже применяется + в `review-doc-guard.mjs`/`ship-review.mjs`). +- **Открытых продуктовых вопросов владельцу нет** — и это оправдано: весь + интерфейс задачи процессный (комментарий HTML в машинном блоке, который + GitHub не рендерит, плюс строка отчёта `process-metrics`), персон + `docs/SCOPE.md` она не касается никак; ни один вопрос ТЗ не маскирует + техническое решение под продуктовое. +- **Откат и риски реалистичны**: откат — ревертом коммитов в `dev`, уже + опубликованные строки остаются (миграции нет, это явно принято), риск + «тело конвейера читается из dev в момент события» снят `continue-on-error` и + тем, что публикация на любой строке не падает, а пишет `reason=invalid`. + +## Чего не проверял + +- **Не проверялось исполнением** — кода ещё нет, это ревью ТЗ, а не ревью кода; + не было ни одного вызова `npx tsc`, `npm test`, `npm run build`, + `smoke-select.mjs`, `golden:verify`, `pytest tests_backend`, + `invariants` — ни один из них не применим на этапе spec (нет диффа продукта), + это не пропуск гейта, а корректный для этапа объём (§8, «Объём гейтов + соразмерен задаче»). +- Не проверялась эксплуатационная точность комментария `run-claude-sdk.ts:94` в + построчной нумерации — проверена по содержимому (grep -n), не по точному + индексу символа; расхождение на ±1 строку не меняет сути цитаты. +- Не проверялось поведение `claude-code-action` в проде (реальный прогон на CI + с реальной сессией) — только статический разбор исходника на пине. Первый + живой прогон — наблюдение, а не AC, как сама задача и указывает в разделе + «Риски». +- Не оценивалась цена «секунда на шаг CI» эмпирически — принято как заявленная + экспертная оценка автора, проверить её может только первый живой прогон. +- Не проверялись архивные спецификсации в `docs/specs/` — задача создана после + 2026-09-10, ТЗ по правилу живёт только в теле issue. + +## Вердикт + +Зелёный. ТЗ полно по §7.1, каждый AC проверяем и привязан к названному +автотесту с указанным способом провала, технические допущения отделены от +продуктовых и не требуют эскалации владельцу, а фактические утверждения о +текущем состоянии репозитория и о пином внешней зависимости подтверждены +построчным чтением обоих источников, а не приняты на веру. + +--- + + + +## Материал раунда + +- Ветка: `dev`, коммит `3b9f25ea659c` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет. +- Дерево материала: `e94ca566360e026e50ecff7594279f9db87cfbbc` + ``` + git log --all --format='%H %T' | grep e94ca566360e + ``` +- Тело issue: `cd1cf82917e9f72b048ec5382cfacfe8d1d1609e4b972a18c1dad2912a3987c3` +- Вердикт конвейера: `green` · High 0 · маршрут `fix`