mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-01 04:09:17 +00:00
claude[bot]
@@ -0,0 +1,275 @@
|
||||
# SPEC-REVIEW — issue #454 · заход r1
|
||||
|
||||
- Issue: https://github.com/Matysh/houseplan-card/issues/454
|
||||
- Этап: ТЗ на ревью (PROCESS.md §2.4), заход r1, блокирующих циклов израсходовано 0 из 4 (лимит полный, `small`/`trivial` не стоят)
|
||||
- Материал: `docs/specs/454-review-round-counter.md` на коммите `ebcf1223` (комментарий «ТЗ готово», ветка `issue/454-review-round-counter`)
|
||||
- Ревьюер: свежая сессия, без контекста написания ТЗ
|
||||
|
||||
## Скоуп
|
||||
|
||||
Issue описывает дефект в `guard`-джобе `.github/workflows/process.yml`: счёт
|
||||
`attempt`/`spent` идёт по regex над телом комментария-вердикта, и вердикт без
|
||||
явной подстроки `SPEC-REVIEW`/`CODE-REVIEW` в тексте выпадает из подсчёта
|
||||
навсегда — что уже привело к перезаписи `docs/reviews/SPEC-REVIEW-449-r1.md` и
|
||||
к занижению бюджета циклов. ТЗ предлагает считать оба числа по опубликованным
|
||||
файлам `docs/reviews/${marker}-${NUM}-r*.md` на ветке задачи (`attempt` — от
|
||||
максимального номера файла плюс один, не от их количества), со страховкой
|
||||
«максимум из счёта по файлам и счёта по комментариям» на случай отказа шага
|
||||
публикации.
|
||||
|
||||
Класс изменения — B (инфраструктура): трогаются только
|
||||
`.github/workflows/process.yml` и `scripts/review-doc-guard.mjs` /
|
||||
`test/review-doc-guard.test.mjs`. Ни одного файла класса A нет, то есть по
|
||||
механическому признаку AGENTS.md/PROCESS.md §1 задача могла бы идти вне флоу
|
||||
вовсе (без ТЗ и его ревью). Автор в комментарии `S2-analysis` (2026-09-04)
|
||||
сознательно выбрал полный трек, назвав причину — правка меняет механизм,
|
||||
который управляет всеми остальными задачами, и ошибка в нём стоит дороже
|
||||
обычной инфраструктурной правки. Это решение владельца/автора по существу
|
||||
задачи, а не нарушение процесса, и не является предметом этого ревью.
|
||||
|
||||
`docs/SCOPE.md` к этой задаче неприменим по смыслу: правка не служит ни одному
|
||||
Core user job — это внутренний инструмент конвейера ревью, не продуктовая
|
||||
функция. Отмечаю это, а не пропускаю проверку молча.
|
||||
|
||||
## Как проверялось
|
||||
|
||||
- Прочитаны `docs/SCOPE.md`, `AGENTS.md`, `PROCESS.md` (целиком, включая §2.10,
|
||||
§4, §7.1, §8, §10–12).
|
||||
- Прочитано тело issue #454 и оба комментария (`S2-analysis` владельца,
|
||||
«ТЗ готово» автора).
|
||||
- Прочитан `docs/specs/454-review-round-counter.md` целиком.
|
||||
- Сверены факты ТЗ с текущим кодом на `dev` (`git show origin/dev:...`):
|
||||
- `.github/workflows/process.yml` — блок `guard`/`decide` (строки ~60–164),
|
||||
построение имени документа `doc="docs/reviews/${marker}-${NUM}-r${CYCLE}.md"`
|
||||
(строки 661, 803), шаг резолва ветки задачи (строки ~211–225);
|
||||
- `scripts/review-doc-guard.mjs` — весь файл: существующие три режима CLI
|
||||
(`--allow=`/stdin, `--anchor=`, `--doc=`) и их назначение (allowlist пути
|
||||
публикации, дозапись якорей материала, проверка осиротевших SHA);
|
||||
- реальный формат строки вердикта в опубликованном документе
|
||||
(`docs/reviews/CODE-REVIEW-450-r1.md` на `dev`) — подтверждено, что строка
|
||||
`Вердикт:` встречается ровно один раз и в конце документа, заголовок
|
||||
`## Вердикт` подстроку `Вердикт:` не даёт.
|
||||
- Прослежена арифметика контракта (§1–§3 ТЗ) вручную на реальной хронологии
|
||||
#449 из комментария владельца (таблица времени/вердиктов/наличия маркера) —
|
||||
см. находку M1 ниже.
|
||||
|
||||
Код не читался как продукт (ветки реализации ещё нет — этап `S4-spec-review`,
|
||||
`scripts/review-doc-guard.mjs` и `process.yml` на ветке задачи ещё не изменены
|
||||
относительно `dev`). Это ревью ТЗ, не код-ревью.
|
||||
|
||||
## Находки
|
||||
|
||||
### M1 (Medium, в скоупе) — AC2 не доказуем тем способом, который сам называет
|
||||
|
||||
**Формулировка AC2:** «Вердикт без маркера в теле больше не занижает ни
|
||||
`attempt`, ни `spent`: сценарий #449 (r1 без маркера, r2 с маркером) даёт
|
||||
`attempt=3`, `spent=2»`, доказательство — «unit на фикстуре реальных данных
|
||||
#449». План тестирования уточняет: «фикстура «как на #449» строится из
|
||||
реальных заголовков документов и строк вердиктов».
|
||||
|
||||
**Проблема.** Реальная история #449 (таблица в комментарии `S2-analysis`
|
||||
владельца) даёт для spec-этапа ровно три вердикта:
|
||||
|
||||
| Время | Вердикт | Маркер в теле |
|
||||
|---|---|---|
|
||||
| 14:46 | жёлтый, «заход r1» | нет |
|
||||
| 14:56 | жёлтый, «заход r2» | да |
|
||||
| 15:17 | зелёный, «заход r3» | да |
|
||||
|
||||
Из-за бага документ `SPEC-REVIEW-449-r1.md` физически перезаписан: под этим
|
||||
именем сейчас лежит содержимое **второго** раунда (жёлтый), а третий раунд
|
||||
лежит под именем `-r2.md` (зелёный). Оригинальное содержимое первого раунда
|
||||
(тоже жёлтое) сохранилось только в старом git-блобе `1ce62613`, вне рабочего
|
||||
дерева — и его восстановление ТЗ прямо выносит в «не-скоуп» («восстановление
|
||||
затёртого документа… отдельная ручная операция владельца, к механизму не
|
||||
относится»).
|
||||
|
||||
Если строить фикстуру **буквально** из реальных, то есть сейчас существующих
|
||||
на ветке файлов (`-r1.md` = жёлтый, `-r2.md` = зелёный) и применить контракт
|
||||
§1–§3 ТЗ:
|
||||
|
||||
- `attemptFromFiles = max(1, 2) + 1 = 3` — совпадает с AC2;
|
||||
- `spentFromFiles` = число файлов с блокирующим вердиктом = **1**
|
||||
(`-r1.md` жёлтый; `-r2.md` зелёный не считается) — **не совпадает** с
|
||||
заявленным `spent=2`;
|
||||
- страховка `spentFromComments` (правило контракта §3, не изменяется) даёт то
|
||||
же самое: `of_stage` по маркеру = [комментарий 14:56, комментарий 15:17],
|
||||
`blocking` = только 14:56 (жёлтый) = **1**. `max(1, 1) = 1`.
|
||||
|
||||
Иными словами: подлинность «жёлтый» первого раунда физически утрачена вместе
|
||||
с его файлом, и ни счёт по файлам, ни счёт по комментариям (комментарий 14:46
|
||||
не несёт маркера — это и есть исходный баг) не могут её восстановить. Это не
|
||||
ошибка алгоритма — это прямое следствие того, что данные уже потеряны, и сам
|
||||
же документ признаёт это в «не-скоупе». Но тогда посчитанное по **реальным**
|
||||
данным #449 значение — `spent=1`, а не `2`, как утверждает AC2.
|
||||
|
||||
Число `spent=2` достижимо только на **реконструированной** фикстуре: два
|
||||
отдельных, никогда не перезаписывавшихся файла (`-r1.md` и `-r2.md`), оба с
|
||||
вердиктом «жёлтый» — то есть на данных, которые не являются «реальными данными
|
||||
#449» в буквальном смысле, а представляют, как выглядела бы история #449,
|
||||
если бы коллизии имён не произошло. План тестирования не говорит, какое из
|
||||
двух прочтений имеется в виду, а расхождение материально: разработчик,
|
||||
построивший фикстуру по первому (буквальному) прочтению, получит
|
||||
подтверждённый унит-тестом `spent=1` и не будет знать, ошибка это в реализации
|
||||
или в самом AC — до следующего раунда ревью.
|
||||
|
||||
**Как воспроизвести рассуждение:** взять реальные три вердикта и два реальных
|
||||
файла #449 (перечислены выше), применить формулы контракта §1–§3 ТЗ вручную —
|
||||
результат `attempt=3, spent=1`, а не `attempt=3, spent=2`.
|
||||
|
||||
**Что нужно поправить.** Явно указать в AC2/плане тестирования одно из двух:
|
||||
(a) фикстура — два независимых, не перезаписанных документа с вердиктом
|
||||
«жёлтый» у обоих (тогда `spent=2` доказуемо, но это не «реальные данные
|
||||
#449», а реконструкция «как должно было быть»); либо (b) фикстура — буквально
|
||||
текущее состояние ветки #449, и тогда ожидаемое значение — `attempt=3,
|
||||
spent=1`, а способность корректно посчитать «сколько удаётся восстановить»
|
||||
(не «сколько было на самом деле») — это и есть то, что стоит проверять и
|
||||
описывать. Второй вариант честнее: он проверяет ровно то, что новый механизм
|
||||
физически способен дать при уже случившейся потере данных, не обещая
|
||||
невозможного.
|
||||
|
||||
Из скоупа задачи (спецификация того же issue #454), без High-находок —
|
||||
жёлтый вердикт с возвратом автору.
|
||||
|
||||
### L1 (Low, снимается с записью) — нет явных разделов «UX» и «i18n»
|
||||
|
||||
`PROCESS.md` §7.1 перечисляет обязательные разделы ТЗ, включая «UX» и «i18n»
|
||||
отдельными пунктами. В документе нет ни одного из них как самостоятельного
|
||||
раздела (headers): «UX» частично закрыт строкой в шапке `Touch editor: not
|
||||
exposed — задача не касается интерфейса вовсе», «i18n» не упомянут вовсе ни
|
||||
разу.
|
||||
|
||||
Не блокирует и не создаёт риска: задача не трогает ни одного файла с
|
||||
интерфейсом или строками (`src/**`, `i18n/*.json`), это прямо следует из
|
||||
раздела «Класс изменения» и списка задеваемых файлов, и вывод «i18n/UX не
|
||||
затронуты» очевиден по содержанию, а не додуман. Снимаю находку с записью, а
|
||||
не требую правки: добавление двух формальных строк-заглушек «UX: нет» / «i18n:
|
||||
нет» было бы ритуалом без содержания. Если следующий раунд правит документ по
|
||||
M1, было бы уместно добавить эти строки заодно (не отдельным циклом).
|
||||
|
||||
### L2 (Low, к сведению, не в счёт вердикта) — cohesion `review-doc-guard.mjs`
|
||||
|
||||
`scripts/review-doc-guard.mjs` сейчас узко специализирован — заголовок модуля
|
||||
прямо говорит «публикация ревью-документа не имеет права трогать ничего, кроме
|
||||
него» (проверка путей пуша, дозапись и проверка якорей материала). ТЗ
|
||||
добавляет туда четвёртую, содержательно не связанную обязанность — счёт
|
||||
`attempt`/`spent` для `guard`-джоба, который отрабатывает **до** публикации и
|
||||
даже до самого ревью. Технический вопрос именования/расположения модуля —
|
||||
это то, что PROCESS.md §7.1 явно отдаёт автору («где стоит гвард… агенты
|
||||
решают сами»), поэтому не поднимаю его до статуса находки, требующей правки, а
|
||||
оставляю как наблюдение: отдельный файл (например
|
||||
`scripts/review-round-counter.mjs`) сделал бы границу ответственности яснее и
|
||||
не расширял бы область, которую уже покрывает существующее название и
|
||||
докстринг. Автор/ревьюер кода вправе оспорить это на следующем этапе без
|
||||
возврата сюда.
|
||||
|
||||
## Что проверено и корректно
|
||||
|
||||
- **Контракт §1 (источник истины — файлы, `max+1`, а не количество).**
|
||||
Проверено рассуждением на реальных данных #449 и на таблице крайних случаев
|
||||
«Ошибки и крайние случаи»: дыра в нумерации (`r1`, `r3`) корректно даёт
|
||||
`attempt=4`, поведение при пустом множестве файлов (`attempt=1, spent=0`)
|
||||
совпадает с сегодняшним умолчанием `attempt=1` в `process.yml:93`.
|
||||
- **Контракт §2 (строка вердикта, `жёлт`/`красн` без учёта регистра).**
|
||||
Совпадает с существующим правилом `process.yml:100` («blocking» через тот
|
||||
же regex `(жёлт|красн)`, `i`), не меняет то, что уже работает верно для
|
||||
идентификации блокирующих вердиктов внутри одного документа — подтверждено
|
||||
на реальном файле `CODE-REVIEW-450-r1.md` (единственное вхождение строки
|
||||
`Вердикт:` — финальная, помеченная как искомая).
|
||||
- **Контракт §3 (страховка максимумом).** Само правило корректно устраняет
|
||||
риск «перерасчёта», которого боялся автор исходного механизма
|
||||
(`process.yml:88-93`): максимум из двух счётов не может дать значение выше
|
||||
фактического числа опубликованных документов/вердиктов ни при каком их
|
||||
сочетании — арифметически `max(a,b) ≤ true`, только если оба источника не
|
||||
завышают, что для обоих верно по построению (ни файлы, ни комментарии не
|
||||
порождают лишних записей).
|
||||
- **AC1, AC3–AC7.** Формулировки однозначны, способ доказательства (unit,
|
||||
частично + мутант) указан и достижим — не нашёл ни одного нереализуемого
|
||||
условия среди них при том же рассуждении, что для M1 (AC4/AC5/AC7
|
||||
дополнительно совпадают с уже работающими сегодня инвариантами #227/#89,
|
||||
которые контракт §5 явно объявляет неизменными).
|
||||
- **Не-скоуп.** Явно и корректно исключает правку задним числом чужого
|
||||
комментария и восстановление утраченного `SPEC-REVIEW-449-r1.md` —
|
||||
соответствует «владелец решает вручную», не додумано автором.
|
||||
- **Ограничение «`process.yml` идентичен в `main` и `dev`»** учтено в разделе
|
||||
«Карта реализации» (п. 5) и в разделе рисков; соответствует существующему
|
||||
шагу Validate, который эту идентичность проверяет — не проверял сам шаг
|
||||
Validate заново, доверяю его существованию по цитате в ТЗ (совпадает с тем,
|
||||
что описывает `PROCESS.md`/комментарий автора при `S2-analysis`).
|
||||
- **Откат.** План (один коммит-ревёрт `process.yml` плюс зеркало в `main`,
|
||||
скрипт и тесты безвредны сами по себе) реалистичен и не требует миграции
|
||||
данных — подтверждаю чтением, без исполнения (откатывать нечего, кода ещё
|
||||
нет).
|
||||
- **Release-артефакты.** `User-Visible: no`, трейлеры корректны для класса B;
|
||||
условие обновления `PROCESS.md` («если он описывает нынешний механизм
|
||||
подсчёта по комментариям») сверено с текстом `PROCESS.md` — документ
|
||||
обсуждает разделение `attempt`/`spent` концептуально (§4, §2.10), но не
|
||||
описывает реализацию «regex по телу комментария», поэтому условие, скорее
|
||||
всего, не сработает; это корректно оставлено условным, а не заявлено фактом.
|
||||
|
||||
## Чего не проверял
|
||||
|
||||
- Само существование и содержимое `scripts/review-doc-guard.mjs` /
|
||||
`test/review-doc-guard.test.mjs` **на ветке задачи** — на момент этого
|
||||
ревью ветка `issue/454-review-round-counter` содержит только
|
||||
`docs/specs/454-review-round-counter.md` (по коммиту `ebcf1223`, «ТЗ
|
||||
готово»), реализации ещё нет — это ожидаемо для этапа `S4-spec-review`, не
|
||||
пропуск.
|
||||
- Гейты (`typecheck`/`test`/`build`) не гонял: класс изменения — документация
|
||||
ТЗ, кода нет вовсе, гонять нечего. `check-docs`/`golden`/смоки/`invariants`
|
||||
неприменимы — diff не касается `src/**`, геометрии, визуала или бэкенда.
|
||||
- Не проверял исполнением, действительно ли `gh api`/`git ls-remote` способны
|
||||
без `actions/checkout` перечислить файлы `docs/reviews/` на произвольной
|
||||
ветке из `guard`-джоба (контракт §4 ТЗ) — формально это описано в «Карте
|
||||
реализации» как `gh api`, что технически осуществимо (REST `contents`
|
||||
эндпоинт не требует локального дерева), но сам вызов не выполнял; это
|
||||
техническая деталь реализации, а не продуктовое ограничение, и её вправе
|
||||
уточнить автор на этапе кода.
|
||||
- Не проверял, как именно единственный существующий на сегодня файл
|
||||
`docs/reviews/CODE-REVIEW-449-r1.md` (единственный опубликованный документ
|
||||
на код-этапе #449) поведёт себя под новым алгоритмом — не требовалось для
|
||||
находки M1, которая полностью доказывается на spec-этапе; арифметика
|
||||
code-этапа (`attempt=2, spent=1`, что совпадает с ожиданием автора в
|
||||
комментарии `S2-analysis`) проверена мысленно и расхождений не показала.
|
||||
|
||||
## Итог
|
||||
|
||||
Один Medium в скоупе (M1: AC2 недоказуем буквальным прочтением собственной
|
||||
формулировки — конкретное числовое расхождение показано выше), High —
|
||||
ноль. Остальные девять AC однозначны и доказуемы описанным способом. Low
|
||||
L1 снимается с записью, L2 — наблюдение вне вердикта. Вердикт — жёлтый,
|
||||
возврат автору на уточнение AC2 (одна строка/абзац, не переписывание ТЗ).
|
||||
|
||||
---
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/454-review-round-counter`, ТЗ прочитано на коммите `ebcf1223`
|
||||
(тот же SHA, что указан автором в комментарии «ТЗ готово»).
|
||||
- Сверка с базовым состоянием — `origin/dev` на момент ревью (см. «Как
|
||||
проверялось»): `.github/workflows/process.yml`, `scripts/review-doc-guard.mjs`,
|
||||
`docs/reviews/CODE-REVIEW-450-r1.md`.
|
||||
- Воспроизвести факты о #449, использованные в M1:
|
||||
```
|
||||
gh issue view 449 --repo Matysh/houseplan-card --json comments
|
||||
gh issue view 454 --repo Matysh/houseplan-card --json body,comments
|
||||
git show origin/dev:.github/workflows/process.yml
|
||||
git show origin/dev:scripts/review-doc-guard.mjs
|
||||
git show origin/issue/454-review-round-counter:docs/specs/454-review-round-counter.md
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
<!-- material-anchors: сгенерировано конвейером (#414) -->
|
||||
|
||||
## Материал раунда
|
||||
|
||||
- Ветка: `issue/454-review-round-counter`, коммит `ebcf12233af7` — ребейз его осиротит, и это нормально: ниже якоря, которые ребейз не меняет.
|
||||
- Дерево материала: `e7a2554603963e6da4f3aa44b30502dc852a43f1`
|
||||
```
|
||||
git log --all --format='%H %T' | grep e7a255460396
|
||||
```
|
||||
- ТЗ `docs/specs/454-review-round-counter.md`, блоб `730f7f3964e2269e7f867bedfde50290afe9a1a9`
|
||||
```
|
||||
git log --all --find-object=730f7f3964e2269e7f867bedfde50290afe9a1a9 -- docs/specs/454-review-round-counter.md
|
||||
```
|
||||
Reference in New Issue
Block a user