feat(infra): бот за заявяване на Issue-та (/assign) срещу дублирана работа (#230) - #240
feat(infra): бот за заявяване на Issue-та (/assign) срещу дублирана работа (#230)#240cefothe wants to merge 7 commits into
Conversation
Pure, dependency-free logic for the /assign issue-claiming bot (midt-bg#230): command parsing, claim marker read/write/strip, idle-day computation, linked-PR awareness, and nudge decisioning. All parse paths guarded (malformed marker never throws); strip is global (anti-spoof). 84 tests under node --test, auto-run by scripts-test.yml.
assign-issue.yml: /assign and /unassign comment commands record a claim as a body marker + status: in-progress label (GitHub refuses to assign non-collaborators; @rustbot precedent). stale-assignment-check.yml: daily sweep nudges at 14 days idle and auto-releases at 21, skipping issues with a still-open linked PR. Both are thin github-script glue over scripts/issue-claim.mjs; actions SHA-pinned, least-privilege permissions, timeout + concurrency set. Refs midt-bg#230.
Link docs/implementation-plans/230-issue-claim-bot.md from docs/README.md so the docs-integrity gate (check:docs) passes — every doc must be reachable from the index.
The workflow if: gate uses GitHub Actions startsWith, which is case-insensitive, so /ASSIGN runs the job — but parseCommand matched case-sensitively and returned null, silently no-opping. Lowercase the token so /ASSIGN, /Assign, etc. take effect. The exact-word guard still rejects /assignee, /assign-me (even uppercased).
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR #230 — бот за заявяване на Issue-та (/assign) срещу дублирана работа
Какво прави PR-ът
Добавя GitHub Actions работен поток и помощен скрипт (scripts/issue-claim.mjs), които позволяват на контрибутор да заяви Issue чрез команда /assign в коментар. Заявката се записва като маркер в тялото на Issue-то, а дневен sweep напомня (nudge) и по-късно освобождава изоставени заявки. Целта е да се избегне дублирана работа, без да се блокират други хора, когато заявителят замълчи. Пакетът включва и обстоен тестов файл (scripts/issue-claim.test.mjs, ~50 случая).
Обща оценка
Кодът е с високо качество и е добре закален срещу типичните рискове на issue_comment работни потоци. Фаза-0 сканирането за сигурност и в двете партиди е ЧИСТО: няма твърдо кодирани тайни, няма злонамерени шаблони или нови зависимости, ненадежденото github.event.comment.body се подава през env и се чете от process.env (без израз-инжекция), действията са пинати към SHA, правата са least-privilege по job, sweep-ът е устойчив на едно „отровено“ Issue, а bot-author guard предпазва от feedback-loop.
Най-важна забележка (средна — целостта на данните)
computeIdleDays нулира часовника за неактивност при всяко човешко събитие в timeline-а, а не само при активност на самия заявител. Ако заявителят е замълчал, но други хора коментират/лейбълват Issue-то, то никога няма да бъде авто-освободено — което противоречи на заявената цел на функцията. Препоръка: измервай неактивността спрямо събитията на claim.user.
Други наблюдения (ниски / за потвърждение)
- Spoof на самоличност (ниско): маркерът за заявка живее в редактируемото тяло на Issue-то, така че може да бъде предварително поставен/подправен. Планът предвижда tamper-proof claim-of-record в коментар от бота (D2) — приемливо за ниско-рискова функция, но заслужава followup.
parseClaimMarkerчете само първия маркер: по-рано поставен фалшив маркер може да засенчи истинския.stripClaimмаха всички (глобален regex), така че sticky-claim е предотвратен, но „> 1 маркер = невалиден“ би затворило и този ъгъл. Да се потвърди, че изходният код не се доверява на маркера за авторизация на/unassign.hasOpenLinkedPrпри PR от форк (O3): всеки отворен cross-referenced PR (вкл. от чужд форк) връщаtrue— потенциален вектор за „заключване“ на Issue от външен потребител. Струва си потвърждение спрямо изходния код.- Анти-spoof пропуск за
shouldNudge: липсва тест, че коментар от човек с вмъкнат<!-- sigma-nudge -->НЕ потиска напомнянето (иначе е възможно вечно избягване на авто-освобождаване). parseCommandпокритие: липсват тестове за команда, предшествана от текст/цитат (напр.> /assign), която трябва да върнеnull.- UX несъответствие (незначително):
if:гейтът използваstartsWith(comment.body, '/assign'), докатоparseCommandправи.trim()— коментар с водещи интервали няма да задейства job-а, а/assignee//assign-meпускат job без ефект (хабене на runner).
Тестове
Тестовият пакет е силен и възпроизводим (фиксиран NOW вместо Date.now()): покрива граничните случаи 14/21 дни, null-safety, устойчивост към malformed JSON (S2), анти-spoof при дублирани маркери (S3), валидация на потребителско име (S5) и нулиране от бот (O1). Няма cheater-тестове, TODO-та или мъртъв код. Наблюденията по-горе са предимно въпроси за покритие/дизайн.
Вердикт
VERDICT: COMMENT — няма блокиращи проблеми в сигурността в нито една партида. Преди финално одобрение / merge:
- решение по средната забележка за
computeIdleDays; - потвърждение на 4-те наблюдения за покритие/дизайн спрямо изходния код;
- решение на maintainer-ите по модела
needs-decisionза #230 — не мърджвай преди това решение.
| const events = timeline ?? []; | ||
| let lastHumanMs = NaN; | ||
| for (const e of events) { | ||
| if (e?.actor?.type === 'Bot') continue; |
There was a problem hiding this comment.
Средна — цялост на данните. computeIdleDays изключва само Bot-събитията, така че часовникът за неактивност се нулира при всяко човешко събитие в timeline-а — включително коментари/лейбъли от хора, различни от заявителя. Ако заявителят (claim.user) е замълчал, но други коментират или триажират issue-то, idle остава малко и задачата никога не се освобождава — което директно противоречи на заявената цел на sweep-а („contributors who picked up an issue and went quiet don't block others").
Препоръка: измервай неактивността спрямо събитията на самия заявител, напр. подай claimedUser и брой само събития, при които e.actor.login === claimedUser (с fallback към createdAt). Ако това е съзнателен избор, добави коментар/тест, който го фиксира като поведение.
| if (!body) return null; | ||
| // Reset lastIndex before exec (MARKER_RE is global; reset avoids stale state). | ||
| MARKER_RE.lastIndex = 0; | ||
| const match = MARKER_RE.exec(body); |
There was a problem hiding this comment.
Ниско. parseClaimMarker чете само ПЪРВИЯ маркер (MARKER_RE.exec). Тъй като тялото на issue-то е редактируемо от автора му, по-рано поставен фалшив <!-- sigma-claim: ... --> може да засенчи легитимния. stripClaim маха всички маркери (глобален regex, S3), така че sticky-claim е предотвратен, но идентификацията чете първия. Обмисли нормализация „> 1 маркер → третирай като невалиден/без claim", за да затвориш и този griefing вектор (свързано с D2 — claim-of-record в коментар от бота).
| if: >- | ||
| github.event.issue.pull_request == null && | ||
| github.event.comment.user.type != 'Bot' && | ||
| (startsWith(github.event.comment.body, '/assign') || |
There was a problem hiding this comment.
Незначително. Гейтът startsWith(github.event.comment.body, '/assign') работи върху суровото тяло, докато parseCommand прави body.trim(). Коментар с водещи интервали (напр. /assign) няма да задейства job-а изобщо, въпреки че parseCommand би го приел — лек UX разнобой. Освен това startsWith пуска job и за /assignee, /assign-me (после безобиден no-op през exact-word guard-а), т.е. се хаби runner. Не е блокиращо; помисли за подравняване на условието (trim/regex) с parseCommand.
| ); | ||
| }); | ||
|
|
||
| it('human comment containing the words "auto-release" does NOT suppress nudge — O1', () => { |
There was a problem hiding this comment.
Анти-spoof пропуск в покритието на shouldNudge. Тук се проверява, че свободен текст „auto-release“ не потиска напомнянето, а тестът на ред ~436 проверява потискане само от коментар на бот, съдържащ NUDGE_MARKER. Липсва обаче тестът за обратния анти-spoof случай: коментар от човек (actor.type === 'User'), който съдържа вмъкнат <!-- sigma-nudge -->, НЕ бива да потиска напомнянето. Ако изходният shouldNudge прави само substring-съвпадение по тялото, без да проверява типа на автора, всеки потребител може да постави невидимия маркер и да блокира напомнянията/авто-освобождаването завинаги. Предлагам да се добави тест, който затвърждава, че само маркер от бот потиска (аналогично на S3 анти-spoof тестовете за claim маркерите).
| assert.equal(hasOpenLinkedPr(timeline), false); | ||
| }); | ||
|
|
||
| it('returns true for a fork-source PR that is still open — O3', () => { |
There was a problem hiding this comment.
Този тест затвърждава като „правилно“ поведение, че hasOpenLinkedPr връща true за всеки отворен cross-referenced PR, включително от чужд форк (fork-user/sigma). Ако при откриване на „свързан отворен PR“ stale-sweep-ът пропуска авто-освобождаването, това е потенциален вектор за отказ/заключване: външен потребител може да отвори PR от своя форк, който реферира едно idle Issue, и така да държи заявката (claim) заключена безсрочно. Струва си да се потвърди спрямо изходния код и, ако е нужно, да се стесни условието (напр. само PR-и от базовото репо или авторът да съвпада със заявителя). Ако поведението е умишлено — добавете тест/коментар, който явно документира решението за форк-PR-ите.
| }); | ||
|
|
||
| it('returns /assign when followed by trailing text', () => { | ||
| assert.equal(parseCommand('/assign please'), '/assign'); |
There was a problem hiding this comment.
Покритието на parseCommand обхваща водещо празно пространство и завършващ текст, но липсва негативен тест за команда, предшествана от текст или цитат — напр. parseCommand('yes /assign') или quoted reply parseCommand('> /assign') трябва да върнат null. GitHub често цитира предишни коментари с префикс >, така че без такъв тест не е гарантирано, че цитиран/вграден /assign няма да задейства неволна заявка. Предлагам да се добавят 1–2 такива case-а.
| assert.equal(parseClaimMarker(body), null); | ||
| }); | ||
|
|
||
| it('uses first marker when duplicates are present', () => { |
There was a problem hiding this comment.
Семантиката „използва първия маркер при дубликати“ е тествана, но си струва да се потвърди спрямо изходния код, че parseClaimMarker не се използва като единствен източник на истина за авторизация на /unassign. Тялото на Issue е редактируемо от неговия автор — ако той вмъкне <!-- sigma-claim: {"user":"жертва"} --> преди легитимния маркер, canUnassign би могъл да бъде подведен. Ако решението за авторизация се взима от labels/assignees, а не от маркера в тялото — идеално; предлагам кратък тест/коментар, който да затвърди това.
…review) Addresses ydimitrof's review: - computeIdleDays now measures inactivity from the CLAIMER's own events, not any human's, so others commenting can't keep an abandoned claim alive (MEDIUM). - hasOpenLinkedPr counts only the claimer's own open PR, so a stranger's fork PR referencing an idle issue can't lock the claim (griefing guard). - shouldNudge only treats a BOT-authored nudge marker as a prior nudge, so a human can't plant the marker to suppress nudges. - parseClaimMarker treats >1 marker as no valid claim (planted-marker fail-safe). Adds tests for each, plus quoted/prefixed parseCommand negatives.
|
Благодаря за подробния преглед, @ydimitrof! 🙏 Всички забележки са адресирани в Средна — Ниско — Ниско — Ниско — Покритие — Незначително — Авторизация на Статус: 96/96 теста минават ( |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
API Error: Connection closed mid-response. The response above may be incomplete.
| // Only a BOT-authored comment carrying the marker counts as a prior nudge. Checking the author | ||
| // type stops a human from planting <!-- sigma-nudge --> in a comment to suppress nudges forever | ||
| // (anti-spoof, sibling to the claim-marker hardening). | ||
| const alreadyNudged = events.some( |
There was a problem hiding this comment.
[Среден] Nudge-детекцията е глобална върху цялата timeline → счупен nudge при пре-заявяване.
alreadyNudged търси какъвто и да е bot коментар с nudgeMarker в цялата timeline. Проблем при повторно заявяване: след пълен цикъл заявяване→nudge→release, старият nudge коментар остава завинаги в timeline-а. Когато нов contributor заяви същото issue и стои неактивен ≥14 дни, тази проверка вижда стария <!-- sigma-nudge -->, връща false, и новият заявител бива освободен на 21-я ден без нито едно предупреждение.
O1 реши false-positive-а от споделена проза, но не и този случай. Предложение: ограничете търсенето до събития след последното sigma-release (или след последната claim активност на текущия заявител), за да е nudge-ът относителен спрямо текущото заемане, а не спрямо цялата история на issue-то. Струва си и изричен тестов случай за re-claim в партида 2.
|
|
||
| // ── parseCommand ─────────────────────────────────────────────────────────────── | ||
|
|
||
| describe('parseCommand', () => { |
There was a problem hiding this comment.
Покритието на parseCommand е за команди в един ред (/assign please, /assign), но липсва тест за многоредово тяло на коментар. GitHub коментарите често са многоредови, напр. /assign\n\nЩе го поема. Предлагам да добавите тест, който фиксира поведението, когато /assign е първият токен на първия ред, а тялото продължава на следващи редове — за да е ясно дали командата се разпознава спрямо първия ред или спрямо цялото тяло.
| }); | ||
|
|
||
| it('returns null when user contains invalid characters — S5', () => { | ||
| const body = '<!-- sigma-claim: {"user":"bad user!"} -->'; |
There was a problem hiding this comment.
Има отличен тест за премахване на ДУБЛИРАНИ банери/маркери, но липсва обратната защита: stripClaim НЕ трябва да премахва легитимен потребителски текст, който случайно съдържа реда **🔧 Claimed by:** или емоджито 🔧. Ако логиката е базирана на широк regex, това е реален риск от загуба на съдържание. Предлагам защитен тест, напр. stripClaim('Виж 🔧 в кода\n**🔧 Claimed by:** пример') да запазва потребителския ред.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах стриктно — одобрявам на връх ff51dff. Ботът е solid и адресира точно рисковете на този клас automation.
- Actions injection: чисто.
comment.bodyминава като env var (COMMENT_BODY), не се интерполира вscript:блок — правилният безопасен модел (интерполация вrun:/script:= RCE от недоверен коментар). - Exact-command parse.
parseCommandвзима първия токен и сравнява=== '/assign'(case-insensitive), тъй че/assignee,/assign-me,/assignedне задействат заявка. CoarsestartsWithgate-ът в YAML-а е само trigger; скриптът пре-парсва точно. - Idle от активността на самия claimer (
computeIdleDays,e.actor.login === claimedUser), не отupdated_at— чужд коментар не нулира таймера.hasOpenLinkedPrпази от release при отворен свързан PR; нъджовете са идемпотентни (маркер). - Permissions минимални (
issues: write,contents: read); per-issue concurrency; изключване на bot-коментари. 777-редовият тест покрива логиката реално — no cheater tests.
FYI (не блокира): status: in-progress ще се auto-create-не с default цвят при първото addLabels — може да го пре-стилизирате с цвят/описание.
Implements #230. Одобрено; blocked на required CI.
nedda76
left a comment
nedda76
left a comment
There was a problem hiding this comment.
Прегледах workflow-ите и glue-логиката. Добре обмислена, сигурностно-съзнателна реализация на #230.
Червеното CI е несвързано — пребазиране го оправя
Падналата проверка check е таймаут на тест в packages/db/src/refresh-slice.test.ts (sqlite-cli интеграционната сюита, „Test timed out in 5000ms"), а не в нищо, което този PR пипа (само .github/workflows/*, scripts/issue-claim.mjs, docs). Точно това вече е поправено на main в bf8b956 (test(db): raise vitest timeout for the sqlite-cli integration suite, #243). Пребазиране върху актуалния main би вдигнало таймаута и CI-то би минало зелено. Тоест не е нужна промяна по кода тук.
Сигурност — потвърждава се
- Инжекционната защита е направена правилно: недоверемото тяло на коментара идва през
env: COMMENT_BODYи се чете сprocess.env, никога не се интерполира в стринга наgithub-script(класическият GitHub Actions script-injection вектор).parseCommandправи авторитетния разбор. issue_comment(неpull_request_target), права стеснени доissues: write, contents: read, без тайни отвъдGITHUB_TOKEN.- Action-ите са пиннати по commit SHA (checkout/setup-node/github-script) — supply-chain хигиена.
- Guard срещу ботове (feedback loop),
concurrencyгрупа на Issue, retry на 409/422 с пре-четене на тялото.@${actor}е GitHub username → безопасно в markdown.
Бележка — това е продуктово решение, не само код
Самата механика на заемане беше предложена в #230, който още стои с status: needs-decision. Преди мърдж струва да се вземе решение по механизма (маркер в тялото на Issue-то + етикет status: in-progress, огледало на @rustbot claim), защото това е нова постоянна конвенция за контрибуторите (CONTRIBUTING.md се променя).
Кодът е издържан. Гейтовете са два: (1) пребазиране за зелено CI, (2) решение по #230 за самата механика. Само преглед — самият мърдж не е мой.
4 participants
Какво и защо
Въвежда бота за заявяване на Issue-та, предложен в #230: контрибуторите заявяват задача с коментар
/assign(и/unassign, за да я освободят) — без нужда от достъп до репото. Дневна проверка автоматично освобождава „застояли" заемания. Целта е двама души да не тръгват по един и същ Issue.Реализацията са два тънки GitHub Actions workflow-а над един тестван модул с чиста логика:
.github/workflows/assign-issue.yml— обработва/assignи/unassign. Тъй като GitHub не позволява не-колаборатор да е нативен Assignee, заемането се пази като маркер в тялото на Issue-то (<!-- sigma-claim -->+ видим ред🔧 Claimed by: @user) плюс етикетstatus: in-progress. Същият модел като Rust@rustbot claim..github/workflows/stale-assignment-check.yml— дневно: подсеща при 14 дни без активност, освобождава при 21; пропуска Issue-та със свързан още отворен PR.scripts/issue-claim.mjs— цялата логика (парсване на командите, четене/писане/чистене на маркера, изчисляване на неактивност, разпознаване на свързан PR, решения за подсещане), покрита с 88node:testтеста (scripts/issue-claim.test.mjs).Спрямо черновата в #230 тук е втвърдено: недовереното тяло на коментара минава през
env(без интерполация в скрипта); защитенJSON.parse(повреден маркер не чупи дневната проверка); глобален strip срещу подправяне на маркера; валидация на потребителското име; невидими<!-- sigma-nudge -->/<!-- sigma-release -->маркери за идемпотентни повторни минавания; retry при 409/422 при запис на тялото; разпознаване на fork PR;timeout-minutes+concurrency; SHA-пинати actions; least-privilege права per job;/unassignна чужд claim само от maintainer (проверка на реалното ниво на достъп, не наauthor_association).Свързан issue
Част от #230 (
status: needs-decision). Моделът все още чака решение от поддръжниците — този PR е реализацията, ако бъде приет. Пипа.github/, което се застъпва с CI/branch-protection нишката: нужна е координация с @cefothe. Отворените въпроси в #230 (отделен workflow за застояли PR-и; различни 14/21 прозорци поpriority; кога си струва Triage роля) остават извън обхвата на този PR.Вид промяна
feat— нова функционалност (+ciза workflow-ите,docsза плана и CONTRIBUTING)Как е тествано
node --test scripts/issue-claim.test.mjs→ 88/88 минават.pnpm lint(prettier--check) — чисто;pnpm check:docs— минава (планът е индексиран вdocs/README.md).cefothe/sigma, живи workflow-и) — покрити сценарии:/assign,/unassign, оспорено заемане (втори потребител), неоторизиран/unassign(не-колаборатор само сread), maintainer override, повторно заемане, празно тяло, подправен маркер, повреден маркер (без грешка),/assignвърху PR (пропуска се), race при едновременно заемане (без двойно заемане), case-insensitive команди (/ASSIGN). Дневната проверка е пусната ръчно (workflow_dispatch) и коректно не прави нищо за пресни заемания.Нужни етикети (действие за поддръжника)
Трябва да се създаде един нов етикет преди/при merge:
status: in-progress— в стила на съществуващитеstatus:етикети. Защо: и двата workflow-а зависят от него —assign-issue.ymlго слага при заемане, аstale-assignment-check.ymlфилтрира списъка с Issue-та по него. Ако не съществува,addLabelsще го създаде автоматично с произволен цвят и без описание — затова е по-добре да се създаде предварително:Останалите използвани етикети вече съществуват (
infra,status: needs-decision). Настройки на Actions: не е нужно да се пипа общото „Read and write" право — per-jobpermissions: issues: writeе достатъчно (проверено на живо).Чеклист
Co-Authored-By:trailermidt-bg/sigma:mainpnpm typecheck— не засяга типове (само.mjs/.yml); ще се провери в CIpnpm test— минава (unit тестовете чрезnode --test)pnpm lintе чисто.env*или.dev.varsdocs/е обновена (план + индекс + CONTRIBUTING)