mirror of
https://github.com/Matysh/houseplan-card
synced 2026-09-29 03:09:36 +00:00
ci: публикация ревью-дока не имеет права трогать ничего, кроме документа
28.08 коммитbb2919fуехал в dev с тридцатью файлами вместо одного markdown: откатил отревьюженную реализацию #359, вернул старые чанки, оставил в dist/ двойной набор. dev держал откаченное дерево три часа. Сообщение коммита было невинным, и от рутины инцидент отличался только диффом. Механизм воспроизведён локально, а не предположен. `git checkout -- .` восстанавливает рабочее дерево ИЗ ИНДЕКСА, `git clean -fd` убирает неотслеживаемое — ни то, ни другое индекс не трогает. Ревьюер работает с Bash и, проверяя «умеет ли тест падать», вполне может сделать git add; всё оставшееся у него в индексе прежняя уборка сохраняла, и следующий git commit забирал это вместе с документом. Отсюда три рубежа, каждый закрывает свой отрезок пути. База: reset --hard на свежий origin/$target снимает и индекс, и дерево разом. Терять нечего — документ приезжает из RUNNER_TEMP, а не из рабочей копии. Индексируется ровно один путь, а не каталог. Индекс: перед коммитом дифф проверяется allowlist'ом docs/reviews/. Диапазон: перед КАЖДЫМ push проверяется origin/$target...HEAD — то есть то, что пуш добавит в ветку. Проверок две, потому что push делается из двух мест, и второй путь срабатывает ровно тогда, когда dev ушёл вперёд — в тех самых условиях, при которых случилсяbb2919f. Пустой дифф — тоже отказ: публиковать нечего означает, что документа нет, а прежняя редакция шага выходила тут с нулём и оставляла вердикт без артефакта (#171). Сравнение по префиксу каталога, а не подстрокой: docs/reviews-old и docs/reviewsx разрешёнными не считаются. Форс-пуш отсутствует и закреплён тестом. Четыре мутанта проверены руками, два добавлены в реестр. Пятый — «убрать одну из двух проверок диапазона» — сначала выжил: тест требовал наличия, а не количества. Тест усилен до подсчёта, мутант убит. Issue: #365 User-Visible: no
This commit is contained in:
@@ -642,14 +642,31 @@ jobs:
|
||||
marker=CODE-REVIEW
|
||||
if [ "$STAGE" = "spec" ]; then marker=SPEC-REVIEW; fi
|
||||
doc="docs/reviews/${marker}-${NUM}-r${CYCLE}.md"
|
||||
# Рабочая копия отбрасывается ДО того, как документ попадёт в дерево:
|
||||
# ревьюер правит код, проверяя «умеет ли тест падать», и его правки
|
||||
# публиковаться не должны.
|
||||
git checkout -- . 2>/dev/null || true
|
||||
# docs/reviews исключён из уборки: ревьюер мог написать документ по
|
||||
# старому пути, и клин не должен его съесть до `git add` — ровно так
|
||||
# оба пути остаются работоспособными.
|
||||
git clean -fd -e docs/reviews -e node_modules >/dev/null 2>&1 || true
|
||||
# Документ спасается ПЕРВЫМ делом. Ревьюер мог написать его по старому
|
||||
# пути прямо в рабочую копию, а дальше эта копия будет отброшена
|
||||
# целиком — и вместе с ней пропал бы артефакт (#220).
|
||||
if [ ! -f "$SOURCE" ] && [ -f "$doc" ]; then
|
||||
cp "$doc" "$SOURCE"
|
||||
echo "документ найден в рабочей копии и сохранён в $SOURCE"
|
||||
fi
|
||||
# Reset, а не checkout+clean, и вот почему (#365).
|
||||
#
|
||||
# 28.08 коммит bb2919f уехал в dev с тридцатью файлами вместо одного
|
||||
# markdown: откатил отревьюженную реализацию #359, вернул старые чанки
|
||||
# и держал dev откаченным три часа. Механизм воспроизведён:
|
||||
# `git checkout -- .` восстанавливает рабочее дерево ИЗ ИНДЕКСА, а
|
||||
# `git clean -fd` убирает неотслеживаемое — ни то, ни другое индекс не
|
||||
# трогает. Ревьюер работает с Bash и в ходе проверки «умеет ли тест
|
||||
# падать» вполне может сделать `git add`; всё, что осталось у него в
|
||||
# индексе, прежняя уборка сохраняла, и следующий же `git commit`
|
||||
# забирал это вместе с документом. Сообщение при этом невинное, и от
|
||||
# рутины инцидент отличается только диффом.
|
||||
#
|
||||
# `reset --hard` снимает и индекс, и дерево разом. Терять нечего:
|
||||
# документ приезжает извне репозитория, из RUNNER_TEMP.
|
||||
git fetch -q origin "$target"
|
||||
git reset -q --hard "origin/$target"
|
||||
git clean -fdq -e node_modules >/dev/null 2>&1 || true
|
||||
# Документ приезжает извне репозитория (#220). Три раунда подряд он
|
||||
# терялся, пока лежал некоммитнутым файлом в том же дереве, которое
|
||||
# ревьюер мутирует и затем восстанавливает: `git checkout -- .` плюс
|
||||
@@ -661,11 +678,12 @@ jobs:
|
||||
cp "$SOURCE" "$doc"
|
||||
echo "документ взят из $SOURCE ($(wc -c < "$doc") байт)"
|
||||
else
|
||||
# Совместимость: ревьюер мог написать по старому пути, если промпт
|
||||
# ещё не обновился в этой ветке.
|
||||
echo "::warning::$SOURCE не найден — ищу документ в рабочей копии"
|
||||
echo "::warning::$SOURCE не найден — документа для публикации нет"
|
||||
fi
|
||||
git add docs/reviews 2>/dev/null || true
|
||||
# Индексируется ровно один путь, а не каталог: `git add docs/reviews`
|
||||
# забрал бы всё, что там окажется, а после reset там не должно быть
|
||||
# ничего постороннего — но полагаться на «не должно» здесь нельзя.
|
||||
git add -- "$doc" 2>/dev/null || true
|
||||
if git diff --cached --quiet; then
|
||||
# Пустая рабочая копия — ещё не провал: ревьюер иногда коммитит
|
||||
# документ сам, своим app-токеном мимо этого шага (CODE-REVIEW-150-r1,
|
||||
@@ -683,6 +701,8 @@ jobs:
|
||||
echo "::error::вердикт есть, а документа нет: ни $SOURCE, ни $doc в рабочей копии, ни $doc в $target — ревью без артефакта (#171, #220)"
|
||||
exit 1
|
||||
fi
|
||||
# Первый рубеж: что вообще проиндексировано.
|
||||
git diff --cached --name-only | node scripts/review-doc-guard.mjs
|
||||
git -c user.name="claude[bot]" \
|
||||
-c user.email="209825114+claude[bot]@users.noreply.github.com" \
|
||||
commit -q -F - <<EOF
|
||||
@@ -691,6 +711,10 @@ jobs:
|
||||
Issue: #$NUM
|
||||
User-Visible: no
|
||||
EOF
|
||||
# Второй рубеж, и он главный: что пуш ДОБАВИТ в целевую ветку. Первый
|
||||
# судит намерение шага, этот — результат, а расходились они именно
|
||||
# тогда, когда база оказывалась не той.
|
||||
git diff --name-only "origin/$target...HEAD" | node scripts/review-doc-guard.mjs
|
||||
# Публикация в dev идёт из детачнутого состояния поверх ветки задачи
|
||||
# либо dev, поэтому push нужен с явным перебазированием при гонке:
|
||||
# dev мог уйти вперёд, пока шло ревью — оно длится до 45 минут.
|
||||
@@ -705,6 +729,9 @@ jobs:
|
||||
echo "::error::документ ревью не удалось опубликовать в $target: конфликт (#171)"
|
||||
exit 1
|
||||
fi
|
||||
# После ребейза набор путей другой — проверяется заново. Форс здесь
|
||||
# запрещён и не появляется: ветка двигается только вперёд.
|
||||
git diff --name-only "origin/$target...HEAD" | node scripts/review-doc-guard.mjs
|
||||
git push -q "https://x-access-token:$TOKEN@github.com/${{ github.repository }}" \
|
||||
"HEAD:$target"
|
||||
fi
|
||||
|
||||
@@ -1236,6 +1236,28 @@ const MUTANT_DEFINITIONS = [
|
||||
replace: ' if (false) {',
|
||||
}],
|
||||
},
|
||||
{
|
||||
id: 'review-doc-guard-matches-by-substring',
|
||||
guard: 'node --test --test-name-pattern="соседний каталог" test/review-doc-guard.test.mjs',
|
||||
because: 'сравнение подстрокой пускает docs/reviews-old и docs/reviewsx: allowlist, который '
|
||||
+ 'ошибается в свою пользу, не защищает ни от чего (#365)',
|
||||
patches: [{
|
||||
file: 'scripts/review-doc-guard.mjs',
|
||||
find: " const prefixes = allowlist.map((item) => (item.endsWith('/') ? item : `${item}/`));",
|
||||
replace: " const prefixes = allowlist.map((item) => item.replace(/\\/$/, ''));",
|
||||
}],
|
||||
},
|
||||
{
|
||||
id: 'review-doc-guard-allows-empty-diff',
|
||||
guard: 'node --test --test-name-pattern="пустой дифф" test/review-doc-guard.test.mjs',
|
||||
because: 'пустой дифф означает, что документа нет: прежняя редакция шага выходила тут с '
|
||||
+ 'нулём, и вердикт ревью оставался без артефакта (#171, #365)',
|
||||
patches: [{
|
||||
file: 'scripts/review-doc-guard.mjs',
|
||||
find: ' if (!cleaned.length) {',
|
||||
replace: ' if (false) {',
|
||||
}],
|
||||
},
|
||||
{
|
||||
id: 'no-new-any-judges-every-line',
|
||||
guard: 'node --test --test-name-pattern="нетронутой строке гейт не блокирует" '
|
||||
|
||||
@@ -0,0 +1,79 @@
|
||||
#!/usr/bin/env node
|
||||
/**
|
||||
* Публикация ревью-документа не имеет права трогать ничего, кроме него (#365).
|
||||
*
|
||||
* git diff --name-only "origin/dev...HEAD" | node scripts/review-doc-guard.mjs
|
||||
* node scripts/review-doc-guard.mjs --allow 'docs/specs/' < paths.txt
|
||||
*
|
||||
* Что случилось. 28.08 шаг публикации запушил в `dev` коммит `bb2919f` с
|
||||
* тридцатью файлами вместо одного markdown: откатил отревьюженную реализацию
|
||||
* #359, вернул старые чанки и оставил в `dist/` двойной набор. `dev` держал
|
||||
* откаченное дерево три часа, пока владелец не восстановил его руками
|
||||
* (`fd762fa`). Сообщение коммита при этом было невинным — «docs: review document
|
||||
* for #359», — и от рутины инцидент отличался только диффом.
|
||||
*
|
||||
* Почему это класс, а не случай. Пушащий шаг ничем не ограничен по путям, а его
|
||||
* рабочая копия может разойтись с origin по десятку причин: гонка параллельных
|
||||
* агентов за `dev` (в тот вечер их было три), ревью длиной в сорок минут,
|
||||
* ветка задачи, которой нет. Любой такой рассинхрон превращает «положить один
|
||||
* markdown» в «затереть dev целиком», и заметить это может только аудит дельты.
|
||||
* Релиз собирается из `dev` — рецидив уехал бы пользователям.
|
||||
*
|
||||
* Поэтому проверка судит не намерение шага, а его результат: набор путей,
|
||||
* который пуш добавит в целевую ветку. Пустой список — тоже отказ: публиковать
|
||||
* нечего, значит что-то пошло не так раньше.
|
||||
*/
|
||||
import { readFileSync } from 'node:fs';
|
||||
|
||||
export const REVIEW_DOC_ALLOWLIST = ['docs/reviews/'];
|
||||
|
||||
/**
|
||||
* Пути вне разрешённых каталогов.
|
||||
*
|
||||
* Сравнение по префиксу каталога, а не по расширению: `docs/reviews/x.md`
|
||||
* разрешён, `docs/reviews-old/x.md` — нет, потому что префикс каталога
|
||||
* заканчивается слэшем и подстрокой не притворяется.
|
||||
*/
|
||||
export function pathsOutsideAllowlist(paths, allowlist = REVIEW_DOC_ALLOWLIST) {
|
||||
const prefixes = allowlist.map((item) => (item.endsWith('/') ? item : `${item}/`));
|
||||
return [...new Set((paths || [])
|
||||
.map((line) => String(line).trim())
|
||||
.filter(Boolean))]
|
||||
.filter((path) => !prefixes.some((prefix) => path.startsWith(prefix)))
|
||||
.sort();
|
||||
}
|
||||
|
||||
/** Вердикт по набору путей: `null` — можно публиковать. */
|
||||
export function reviewDocPushRefusal(paths, allowlist = REVIEW_DOC_ALLOWLIST) {
|
||||
const cleaned = [...new Set((paths || []).map((line) => String(line).trim()).filter(Boolean))];
|
||||
if (!cleaned.length) {
|
||||
return 'публиковать нечего: дифф пуст, а шаг вызван — значит документ не создан'
|
||||
+ ' либо база уже содержит его';
|
||||
}
|
||||
const outside = pathsOutsideAllowlist(cleaned, allowlist);
|
||||
if (!outside.length) return null;
|
||||
return `публикация ревью-документа задевает ${outside.length} путь(ей) вне`
|
||||
+ ` ${allowlist.join(', ')}:\n ${outside.join('\n ')}\n`
|
||||
+ 'Пуш отменён. Так 28.08 коммит bb2919f откатил dev на три часа:'
|
||||
+ ' рабочая копия шага разошлась с origin, и «положить один markdown»'
|
||||
+ ' превратилось в «затереть dev целиком» (#365).';
|
||||
}
|
||||
|
||||
const invokedDirectly = process.argv[1]
|
||||
&& import.meta.url === new URL(`file://${process.argv[1]}`).href;
|
||||
if (invokedDirectly) {
|
||||
const argv = process.argv.slice(2);
|
||||
const allowArg = argv.find((item) => item.startsWith('--allow='));
|
||||
const allowlist = allowArg
|
||||
? allowArg.slice('--allow='.length).split(',').map((item) => item.trim()).filter(Boolean)
|
||||
: REVIEW_DOC_ALLOWLIST;
|
||||
const paths = readFileSync(0, 'utf8').split('\n');
|
||||
const refusal = reviewDocPushRefusal(paths, allowlist);
|
||||
if (refusal) {
|
||||
console.error(`::error::${refusal.split('\n')[0]}`);
|
||||
console.error(refusal);
|
||||
process.exit(1);
|
||||
}
|
||||
const count = paths.map((line) => line.trim()).filter(Boolean).length;
|
||||
console.log(`дифф публикации чист: ${count} файл(ов), все в ${allowlist.join(', ')}`);
|
||||
}
|
||||
@@ -0,0 +1,89 @@
|
||||
import test from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
import { readFileSync } from 'node:fs';
|
||||
|
||||
import {
|
||||
REVIEW_DOC_ALLOWLIST, pathsOutsideAllowlist, reviewDocPushRefusal,
|
||||
} from '../scripts/review-doc-guard.mjs';
|
||||
|
||||
// #365. 28.08 шаг публикации ревью-дока запушил в dev коммит bb2919f с тридцатью
|
||||
// файлами вместо одного markdown: откатил отревьюженную реализацию #359, вернул
|
||||
// старые чанки, оставил в dist/ двойной набор. dev держал откаченное дерево три
|
||||
// часа. Сообщение коммита было невинным — «docs: review document for #359», — и
|
||||
// от рутины инцидент отличался только диффом. Релиз собирается из dev.
|
||||
|
||||
test('чистая публикация проходит (#365 AC1)', () => {
|
||||
assert.equal(reviewDocPushRefusal(['docs/reviews/CODE-REVIEW-359-r1.md']), null);
|
||||
assert.equal(reviewDocPushRefusal([
|
||||
'docs/reviews/SPEC-REVIEW-1-r1.md', 'docs/reviews/SPEC-REVIEW-1-r2.md',
|
||||
]), null);
|
||||
});
|
||||
|
||||
test('посторонний путь отменяет пуш и называет файлы (#365 AC2)', () => {
|
||||
const refusal = reviewDocPushRefusal([
|
||||
'docs/reviews/CODE-REVIEW-359-r1.md',
|
||||
'src/houseplan-card.ts',
|
||||
'dist/houseplan-card.js',
|
||||
]);
|
||||
assert.match(refusal, /задевает 2 путь\(ей\)/);
|
||||
assert.match(refusal, /dist\/houseplan-card\.js/);
|
||||
assert.match(refusal, /src\/houseplan-card\.ts/);
|
||||
// Причина названа, а не только факт: без неё следующий читатель решит, что
|
||||
// проверка придирается, и снимет её.
|
||||
assert.match(refusal, /bb2919f/);
|
||||
});
|
||||
|
||||
test('пустой дифф — тоже отказ, а не тихий успех (#365)', () => {
|
||||
// Публиковать нечего означает, что что-то пошло не так раньше. Прежняя
|
||||
// редакция шага в таком случае выходила с нулём, и вердикт ревью оставался
|
||||
// без артефакта (#171).
|
||||
assert.match(reviewDocPushRefusal([]), /публиковать нечего/);
|
||||
assert.match(reviewDocPushRefusal(['', ' ']), /публиковать нечего/);
|
||||
});
|
||||
|
||||
test('соседний каталог с похожим именем не считается разрешённым (#365)', () => {
|
||||
// Сравнение по префиксу каталога со слэшем: docs/reviews-old подстрокой не
|
||||
// притворяется.
|
||||
assert.deepEqual(
|
||||
pathsOutsideAllowlist(['docs/reviews-old/x.md', 'docs/reviews/y.md']),
|
||||
['docs/reviews-old/x.md'],
|
||||
);
|
||||
assert.deepEqual(pathsOutsideAllowlist(['docs/reviewsx.md']), ['docs/reviewsx.md']);
|
||||
});
|
||||
|
||||
test('allowlist задаётся снаружи и по умолчанию только docs/reviews (#365)', () => {
|
||||
assert.deepEqual(REVIEW_DOC_ALLOWLIST, ['docs/reviews/']);
|
||||
assert.equal(reviewDocPushRefusal(['docs/specs/1.md'], ['docs/specs']), null);
|
||||
assert.match(reviewDocPushRefusal(['docs/specs/1.md']), /docs\/specs\/1\.md/);
|
||||
});
|
||||
|
||||
test('шаг публикации в конвейере проверяет и индекс, и то, что уедет (#365 AC4)', () => {
|
||||
const workflow = readFileSync(
|
||||
new URL('../.github/workflows/process.yml', import.meta.url), 'utf8',
|
||||
);
|
||||
const step = workflow.slice(
|
||||
workflow.indexOf('- name: Опубликовать документ ревью'),
|
||||
workflow.indexOf('- name: Решение по вердикту'),
|
||||
);
|
||||
assert.ok(step.length > 500, 'шаг публикации не найден');
|
||||
// Два рубежа: что проиндексировано и что пуш добавит в ветку. Расходились они
|
||||
// именно тогда, когда база оказывалась не той.
|
||||
assert.equal(
|
||||
(step.match(/git diff --cached --name-only \| node scripts\/review-doc-guard\.mjs/g) || []).length,
|
||||
1, 'индекс проверяется один раз, перед коммитом',
|
||||
);
|
||||
// Дважды: push делается из двух мест — сразу и после ребейза при гонке. Одна
|
||||
// проверка на два пути означала бы, что второй путь не проверен вовсе, а
|
||||
// именно он срабатывает, когда dev ушёл вперёд — то есть в тех самых
|
||||
// условиях, при которых случился bb2919f.
|
||||
assert.equal(
|
||||
(step.match(/git diff --name-only "origin\/\$target\.\.\.HEAD" \| node scripts\/review-doc-guard\.mjs/g) || []).length,
|
||||
2, 'диапазон проверяется перед каждым push',
|
||||
);
|
||||
// Свежая база вместо той, что лежала здесь сорок минут назад.
|
||||
assert.match(step, /git reset -q --hard "origin\/\$target"/);
|
||||
// Форс-пуш запрещён: ветка двигается только вперёд.
|
||||
assert.equal(/--force/.test(step), false, 'в публикации ревью-дока не должно быть force-push');
|
||||
// Индексируется один путь, а не каталог.
|
||||
assert.match(step, /git add -- "\$doc"/);
|
||||
});
|
||||
Reference in New Issue
Block a user