From d5e114524cde32144802c3614a2633665217b4a5 Mon Sep 17 00:00:00 2001 From: Codex Date: Fri, 4 Sep 2026 20:18:59 +0300 Subject: [PATCH] =?UTF-8?q?fix:=20=D0=BA=D0=BE=D0=BC=D0=BC=D0=B5=D0=BD?= =?UTF-8?q?=D1=82=D0=B0=D1=80=D0=B8=D0=B9=20=D0=B7=D0=B0=D1=81=D1=87=D0=B8?= =?UTF-8?q?=D1=82=D1=8B=D0=B2=D0=B0=D0=B5=D1=82=D1=81=D1=8F=20=D0=B2=D0=B5?= =?UTF-8?q?=D1=80=D0=B4=D0=B8=D0=BA=D1=82=D0=BE=D0=BC=20=D1=82=D0=BE=D0=BB?= =?UTF-8?q?=D1=8C=D0=BA=D0=BE=20=D0=BF=D0=BE=20=D0=B4=D0=BE=D0=BA=D1=83?= =?UTF-8?q?=D0=BC=D0=B5=D0=BD=D1=82=D1=83=20=D1=81=D0=B2=D0=BE=D0=B5=D0=B9?= =?UTF-8?q?=20=D0=B7=D0=B0=D0=B4=D0=B0=D1=87=D0=B8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User-Visible: no Issue: #454 --- .github/workflows/process.yml | 30 +++++----- docs/specs/454-review-round-counter.md | 28 +++++++-- scripts/mutation-gate.mjs | 14 +++++ scripts/review-doc-guard.mjs | 80 ++++++++++++++++++++++++-- test/review-doc-guard.test.mjs | 49 ++++++++++++++++ 5 files changed, 174 insertions(+), 27 deletions(-) diff --git a/.github/workflows/process.yml b/.github/workflows/process.yml index 880ecab4..af6dc9b6 100644 --- a/.github/workflows/process.yml +++ b/.github/workflows/process.yml @@ -97,9 +97,11 @@ jobs: # # Вердикты считаются ТОЛЬКО своего этапа: иначе вердикт по ТЗ съедал # цикл из бюджета код-ревью (#89 получило r2/4). Этап опознаётся по - # имени документа в теле комментария; документа нет — вердикт не - # посчитается. Недосчёт считался обратимой ошибкой — «даёт лишний - # заход» — и в этой оценке была ошибка, см. ниже. + # имени документа — раньше по подстроке маркера в теле комментария, + # теперь по имени документа ЭТОЙ задачи, `-`: голая + # подстрока протекала на прозе. #454 поймала это на себе — разбор + # чужих задач в комментарии содержал `CODE-REVIEW`, и первый же + # код-ревью получил заход r3. # # Счёт по комментариям остаётся ровно тем же, но он БОЛЬШЕ НЕ # ЕДИНСТВЕННЫЙ (#454). Маркер этапа попадает в тело комментария, @@ -116,14 +118,8 @@ jobs: # обоих, перерасчёт невозможен по построению. attempt=1; spent=0; spent_list="" if [ -n "$stage" ]; then - comments=$(gh issue view "$NUM" --repo "$REPO" --json comments) - of_stage="[.comments[] | select(.body | test(\"Вердикт:\")) | select(.body | test(\"$marker\"))]" - # Блокирующим считается вердикт, у которого в строке вердикта стоит - # «жёлтый» или «красный». Регистр и окружение слова не важны. - blocking="$of_stage | map(select(.body | test(\"Вердикт:[^\\n]*(жёлт|красн)\"; \"i\")))" - attempt=$(( $(printf '%s' "$comments" | jq -r "$of_stage | length") + 1 )) - spent=$(printf '%s' "$comments" | jq -r "$blocking | length") - spent_list=$(printf '%s' "$comments" | jq -r "$blocking | map(\"- \" + .url) | join(\"\\n\")") + comments=$(mktemp) + gh issue view "$NUM" --repo "$REPO" --json comments > "$comments" # Ветка задачи — та же, что выберет шаг ревью: свежая по коммиту. # Её нет у задач, размеченных до появления конвейера; тогда счёт по @@ -153,16 +149,18 @@ jobs: gh api "repos/$REPO/contents/docs/reviews/$name?ref=$target" \ -H 'Accept: application/vnd.github.raw' > "$docs/$name" 2>/dev/null || rm -f "$docs/$name" done + list=$(mktemp) counters=$(node scripts/review-doc-guard.mjs --counters \ --marker="$marker" --num="$NUM" --names="$names" --docs="$docs" \ - --comment-attempt="$attempt" --comment-spent="$spent") - # Пустой ответ означает, что скрипт не отработал. Тогда действуют - # прежние значения: guard обязан продолжить работу, а не встать — - # худшее, что даёт откат к прозе, это сегодняшнее поведение. + --comments="$comments" --spent-list="$list") + spent_list=$(cat "$list") + # Пустой ответ означает, что скрипт не отработал. Тогда остаются + # значения по умолчанию (заход 1, циклов 0): guard обязан + # продолжить работу, а не встать. new_attempt=$(printf '%s\n' "$counters" | sed -n 's/^attempt=//p') new_spent=$(printf '%s\n' "$counters" | sed -n 's/^spent=//p') new_blocking=$(printf '%s\n' "$counters" | sed -n 's/^blocking=//p') - case "$new_attempt" in ''|*[!0-9]*) echo "::warning::счёт по файлам не дал числа — остаётся счёт по комментариям" ;; *) attempt="$new_attempt" ;; esac + case "$new_attempt" in ''|*[!0-9]*) echo "::warning::счётчик раундов не дал числа — работают значения по умолчанию" ;; *) attempt="$new_attempt" ;; esac case "$new_spent" in ''|*[!0-9]*) : ;; *) spent="$new_spent" ;; esac # Перечень учтённого обязан сходиться с числом: если цикл виден # только документом, ссылка на комментарий его не объяснит. diff --git a/docs/specs/454-review-round-counter.md b/docs/specs/454-review-round-counter.md index b5af2921..2d236d4f 100644 --- a/docs/specs/454-review-round-counter.md +++ b/docs/specs/454-review-round-counter.md @@ -138,9 +138,19 @@ attempt = max(attemptFromFiles, attemptFromComments) spent = max(spentFromFiles, spentFromComments) ``` -Счёт по комментариям остаётся ровно тем же, что сегодня. Недосчёт возможен -только при отказе обоих источников; перерасчёт невозможен по построению, потому -что берётся максимум, а не сумма. +Недосчёт возможен только при отказе обоих источников. Удвоение невозможно по +построению — берётся максимум, а не сумма, — но это НЕ значит, что завышение +невозможно вовсе: максимум наследует ошибку той компоненты, которая завысила. +Поэтому у каждого источника своё правило точности. + +Счёт по комментариям в первой редакции ТЗ предполагался неизменным. Ревью +реализации показало, что оставлять его нельзя: правило «в теле есть подстрока +`Вердикт:` и подстрока маркера» протекает на прозе. Поймано на самой #454 — +разбор чужих задач в комментарии содержал `CODE-REVIEW`, и первый же код-ревью +получил заход r3 вместо r1. Поэтому комментарий засчитывается вердиктом этапа, +только если он (а) объявляет вердикт той же строгой строкой, что и документ, и +(б) называет документ ЭТОЙ задачи и ЭТОГО этапа — `-`. Голая +подстрока маркера больше не годится: именно она и протекала. ### 4. Ветка задачи @@ -178,7 +188,7 @@ spent = max(spentFromFiles, spentFromComments) | AC2b | На той же истории, прожитой уже с исправлением, ничего не теряется: фикстура трёх файлов `r1` (жёлтый), `r2` (жёлтый), `r3` (зелёный) даёт `attempt=4`, `spent=2` | unit на реконструированной фикстуре | | AC3 | Имя документа никогда не повторяет уже существующее на ветке | unit + мутант | | AC4 | Зелёный вердикт цикла не тратит (#227 не сломан) | unit | -| AC5 | Вердикт чужого этапа не влияет на счёт (#89 не сломан) | unit | +| AC5 | Вердикт чужого этапа не влияет на счёт (#89 не сломан) — ни по имени файла, ни по прозе комментария | unit + мутант | | AC6 | Отказ публикации (документа нет при существующем вердикте) не занижает счёт — работает максимум | unit | | AC7 | Отсутствие ветки/недоступность API не роняет `guard` и не меняет сегодняшнего поведения | unit | | AC8 | `process.yml` идентичен в `main` и `dev` | существующий шаг Validate | @@ -197,8 +207,14 @@ spent = max(spentFromFiles, spentFromComments) вместо максимума; краснеет AC1/AC3; - `review-round-drops-file-source` — счёт по файлам выключен, остаётся только проза; краснеет AC2; - - `review-round-takes-comments-over-max` — вместо максимума берётся счёт по - комментариям; краснеет AC6. + - `review-round-drops-comment-insurance` — вместо максимума берётся счёт + только по файлам; краснеет AC6. (В первой редакции ТЗ мутант назывался + `review-round-takes-comments-over-max`; в таком виде он был неотличим от + предыдущего — обе мутации дают результат, равный счёту по комментариям, — и + AC6 не краснил, потому что там комментарии как раз больше файлов. Ломать + страховку нужно противоположной мутацией.) + - `review-comment-source-ignores-issue-number` — резервный матч по голому + маркеру вместо `-`; краснеет AC5. - Ручная проверка на самом себе: эта задача проходит spec-review и code-review штатным конвейером; после её мержа номера документов #454 обязаны идти подряд. diff --git a/scripts/mutation-gate.mjs b/scripts/mutation-gate.mjs index 1fd23d76..47e097f3 100644 --- a/scripts/mutation-gate.mjs +++ b/scripts/mutation-gate.mjs @@ -2439,6 +2439,20 @@ const MUTANT_DEFINITIONS = [ replace: ' if (true) return null;', }], }, + { + id: 'review-comment-source-ignores-issue-number', + guard: 'node --test --test-name-pattern="по документу ЭТОЙ задачи|чужой номер задачи" ' + + 'test/review-doc-guard.test.mjs', + because: 'matching a bare stage marker counts any comment that merely mentions another ' + + "issue's review document as a verdict of this one — #454 gave its own first code " + + 'review round number r3 that way, and a contaminated yellow would burn the §4 budget ' + + 'without a single real cycle (#89)', + patches: [{ + file: 'scripts/review-doc-guard.mjs', + find: ' const own = new RegExp(`${marker}-${num}(?![0-9])`);', + replace: ' const own = new RegExp(marker);', + }], + }, { id: 'review-round-counts-files-not-max', guard: 'node --test --test-name-pattern="от максимума номеров" test/review-doc-guard.test.mjs', diff --git a/scripts/review-doc-guard.mjs b/scripts/review-doc-guard.mjs index 202a9b11..cd0c8657 100644 --- a/scripts/review-doc-guard.mjs +++ b/scripts/review-doc-guard.mjs @@ -449,6 +449,47 @@ export function blockingFromDocs(docs) { return { blocking, unread }; } +/** + * Вердикты этапа среди комментариев issue — вторая, страховочная половина + * счёта (#454). + * + * Прежнее правило было двумя тестами подстроки по всему телу: `Вердикт:` и имя + * маркера. Оба ловят прозу. Поймано на самой этой задаче: guard кода #454 + * насчитал заход r3 при первом же код-ревью, потому что маркер `CODE-REVIEW` + * случайно встретился в зелёном вердикте СПЕК-ревью и в комментарии-передаче + * работы — оба разбирали историю чужих задач и цитировали имена их документов. + * Завышение здесь опаснее занижения: попади заражающий вердикт в свой этап + * жёлтым, бюджет §4 сгорел бы без единого настоящего цикла — ровно вред + * класса #89, от которого этап и отделяли. + * + * Поэтому два условия вместо двух подстрок: + * + * - комментарий ОБЪЯВЛЯЕТ вердикт (та же строгая строка, что и в документе), а + * не упоминает слово «вердикт» в разборе; + * - он называет документ ЭТОЙ задачи и ЭТОГО этапа: `-`. Голое + * имя маркера больше не годится — именно оно и протекало. + */ +export function stageVerdictComments(comments, marker, num) { + if (!/^[A-Z-]+$/.test(String(marker || '')) || !/^\d+$/.test(String(num || ''))) return []; + const own = new RegExp(`${marker}-${num}(?![0-9])`); + return (comments || []) + .map((item) => (typeof item === 'string' ? { body: item } : (item || {}))) + .filter((item) => own.test(String(item.body ?? ''))) + .map((item) => ({ ...item, verdict: verdictDeclaration(item.body) })) + .filter((item) => Boolean(item.verdict)); +} + +/** Счётчики по комментариям в том же виде, в каком их даёт файловая половина. */ +export function commentCounters(comments, marker, num) { + const verdicts = stageVerdictComments(comments, marker, num); + const blocking = verdicts.filter((item) => isBlockingVerdict(item.verdict)); + return { + attempt: verdicts.length + 1, + spent: blocking.length, + list: blocking.map((item) => item.url).filter(Boolean), + }; +} + /** * Итоговые счётчики: максимум двух независимых источников. * @@ -516,16 +557,37 @@ if (invokedDirectly) { + ` (${error.code || error.message}) — цикл по нему не засчитан`); } } - const counters = reviewCounters({ - rounds, - docs, - comments: { attempt: Number(value('comment-attempt', '1')), spent: Number(value('comment-spent', '0')) }, - }); + // Комментарии — вторая половина счёта. Разбор их тоже здесь, а не в jq: + // прежнее правило «подстрока в теле» протекало на прозе (см. + // stageVerdictComments), и чинить его в inline-shell означало бы снова + // оставить счёт без единого теста. + let fromComments = { attempt: Number(value('comment-attempt', '1')), + spent: Number(value('comment-spent', '0')), list: [] }; + const commentsFile = value('comments'); + if (commentsFile) { + try { + const payload = JSON.parse(readFileSync(commentsFile, 'utf8')); + fromComments = commentCounters(payload.comments || payload, marker, num); + } catch (error) { + console.error(`::warning::комментарии не разобраны (${error.code || error.message})` + + ' — счёт по комментариям отключён, работает счёт по документам'); + fromComments = { attempt: 1, spent: 0, list: [] }; + } + } + const counters = reviewCounters({ rounds, docs, comments: fromComments }); if (counters.unread.length) { console.error(`::warning::вердикт не объявлен машиночитаемой строкой в:` + ` ${counters.unread.join(', ')} — цикл по этим документам считается` + ' только по комментариям'); } + // Расхождение источников — не отказ, но и не рутина: оно означает, что один + // из них чего-то не видит. Пусть это будет видно в прогоне, а не только в + // арифметике максимума. + if (counters.attemptComments !== counters.attemptFiles) { + console.error('::warning::источники счёта расходятся: по документам заход' + + ` ${counters.attemptFiles}, по комментариям ${counters.attemptComments}.` + + ' Взят максимум. Документы надёжнее: их имена собирает конвейер.'); + } console.error(`::notice::раунды по файлам ${rounds.length ? rounds.join(',') : '—'};` + ` заход: файлы ${counters.attemptFiles}, комментарии ${counters.attemptComments};` + ` циклы: файлы ${counters.spentFiles}, комментарии ${counters.spentComments}`); @@ -534,6 +596,14 @@ if (invokedDirectly) { // Перечень учтённого по файлам: без него владелец видит число, но не может // сверить, из чего оно сложилось, когда комментарии цикла не показывают. console.log(`blocking=${counters.blocking.join(', ')}`); + const listFile = value('spent-list'); + if (listFile) { + try { + writeFileSync(listFile, fromComments.list.map((url) => `- ${url}`).join('\n'), 'utf8'); + } catch (error) { + console.error(`::warning::перечень учтённых вердиктов не записан (${error.code || error.message})`); + } + } process.exit(0); } diff --git a/test/review-doc-guard.test.mjs b/test/review-doc-guard.test.mjs index 4cc70171..0c2e3d93 100644 --- a/test/review-doc-guard.test.mjs +++ b/test/review-doc-guard.test.mjs @@ -5,6 +5,7 @@ import { readFileSync } from 'node:fs'; import { ANCHOR_MARKER, REVIEW_DOC_ALLOWLIST, anchorLiveness, REVIEW_HEADER_LINES, citedMaterialShas, danglingMaterialRefusal, materialAnchorBlock, materialAnchorsFrom, parseSpecList, pathsOutsideAllowlist, reviewDocPushRefusal, withMaterialAnchors, attemptFromRounds, blockingFromDocs, isBlockingVerdict, reviewCounters, reviewRoundsFromFiles, verdictDeclaration, + commentCounters, stageVerdictComments, } from '../scripts/review-doc-guard.mjs'; // #365. 28.08 шаг публикации ревью-дока запушил в dev коммит bb2919f с тридцатью @@ -477,3 +478,51 @@ test('описание чужого раунда не объявляет вер assert.ok(verdictDeclaration('- **Вердикт:** жёлтый')); assert.ok(verdictDeclaration('- Вердикт: зелёный')); }); + +// Вторая половина счёта — комментарии. Прежнее правило («в теле есть +// «Вердикт:» и есть имя маркера») протекало на прозе, и поймано это было на +// самой #454: разбор чужих задач в комментарии-передаче работы содержал слово +// CODE-REVIEW, и первый же код-ревью получил заход r3 вместо r1. + +const C = (body, url = 'https://x/1') => ({ body, url }); + +test('вердикт этапа опознаётся по документу ЭТОЙ задачи (#454 AC5, #89)', () => { + const comments = [ + C('Вердикт: жёлтый · заход r1\n\nДокумент: docs/reviews/CODE-REVIEW-454-r1.md'), + // Зелёный вердикт СПЕК-этапа, случайно упомянувший чужой документ. + C('Вердикт: зелёный · заход r2 — разобрано по аналогии с CODE-REVIEW-441-r1.md' + + '\n\nДокумент: docs/reviews/SPEC-REVIEW-454-r2.md'), + // Передача работы: слово «Вердикт:» в прозе и имя чужого документа. + C('Реализация готова. Строки `Вердикт:` в документе нет; ср. CODE-REVIEW-439-r1.md'), + ]; + const code = commentCounters(comments, 'CODE-REVIEW', '454'); + assert.equal(code.attempt, 2, 'настоящий код-вердикт ровно один'); + assert.equal(code.spent, 1); + const spec = commentCounters(comments, 'SPEC-REVIEW', '454'); + assert.equal(spec.attempt, 2); + assert.equal(spec.spent, 0, 'зелёный цикла не тратит'); +}); + +test('чужой номер задачи не засчитывается своим (#454 AC5)', () => { + const comments = [C('Вердикт: красный · заход r1\n\nДокумент: docs/reviews/CODE-REVIEW-4540-r1.md')]; + assert.equal(commentCounters(comments, 'CODE-REVIEW', '454').attempt, 1); + assert.equal(commentCounters(comments, 'CODE-REVIEW', '4540').attempt, 2); +}); + +test('перечень учтённого сходится с числом циклов (#454)', () => { + const comments = [ + C('Вердикт: жёлтый · заход r1 — CODE-REVIEW-454-r1.md', 'https://x/1'), + C('Вердикт: зелёный · заход r2 — CODE-REVIEW-454-r2.md', 'https://x/2'), + C('Вердикт: красный · заход r3 — CODE-REVIEW-454-r3.md', 'https://x/3'), + ]; + const counters = commentCounters(comments, 'CODE-REVIEW', '454'); + assert.equal(counters.spent, 2); + assert.deepEqual(counters.list, ['https://x/1', 'https://x/3']); +}); + +test('мусор в комментариях не роняет счёт (#454 AC7)', () => { + assert.equal(commentCounters(null, 'CODE-REVIEW', '454').attempt, 1); + assert.equal(commentCounters([{}, { body: null }, 'строка'], 'CODE-REVIEW', '454').attempt, 1); + assert.equal(commentCounters([C('Вердикт: жёлтый CODE-REVIEW-454-r1.md')], '', '454').attempt, 1); + assert.deepEqual(stageVerdictComments([C('Вердикт: жёлтый CODE-REVIEW-454-r1.md')], 'CODE-REVIEW', 'x'), []); +});