Repository navigation
[quality] approve.ts: ten mutation survivors — any-Bot /approve, user-token bot review, require_self_approval break, approver sort, label casing, isBot notifier match #521
Description
Current scope and disposition (2026-10-21)
This issue tracks only its original finding below, not every unrelated finding folded into its comments. The table is the current routing record. Original reports and comments are preserved as history; repeated or superseded checklists in those comments are not additional closure requirements.
| Finding | Canonical owner / disposition |
|---|---|
| Original approve mutation assertions (bot handling, ordering, label casing, notifier identity) | This issue; #522 |
| Config org-tier merge and parsing assertion gaps | #534 / #535; duplicate here, not part of #522 |
Each open PR remains responsible for its own work even after this issue closes. A PR may close this issue only by resolving the original finding; other rows must remain tracked at their canonical destination.
Original report (preserved)
Finding
A single-operator mutation probe over src/plugins/approve.ts on main@88241f4 (123 mutants, each run against __tests__/plugins/{approve,approveErrorPaths,approveEvents,approveReview}.test.ts + __tests__/issueCommentTest/approve.test.ts, survivors re-run against the full npx vitest related src/plugins/approve.ts set: 46 files / 920 tests) leaves ten non-equivalent survivors. Line coverage is 100% for this file in both the unit run (npm run test:coverage: 163 files / 2206 tests, 100 % lines / 99.75 % branches) and the bundle e2e run (npm run test:coverage:e2e: 83 files / 502 tests, approve.ts 100 % lines), so this is an assertion-strength gap, not a coverage gap.
| # | site | mutant that survives | behaviour the suite does not pin |
|---|---|---|---|
| 1 | approvalEvents approve.ts:118 |
login === undefined || submitted_at == null → && |
With a user token (tokenLogin set), a Bot's APPROVED review and a pending (submitted_at: null) review must still be skipped. Today the bot case only passes because login === tokenLogin is undefined === undefined in every test. |
| 2 | humanLogin approve.ts:144 |
user.login === '' || user.type === 'Bot' → && |
A comment whose user.login is '' must yield no event. |
| 3 | humanLogin approve.ts:144 |
user.type === 'Bot' || login === 'github-actions[bot]' → && |
Any Bot user's /approve (e.g. renovate[bot]) must be ignored, not just github-actions[bot]. Only the latter is ever used in the suite. |
| 4 | computeApproval approve.ts:172 |
continue → break |
With require_self_approval, skipping the author's own event must not stop later events: the suite always puts the author's event last. |
| 5 | computeApproval approve.ts:191 |
drop .sort() on per-file approvers |
coveredFiles approver lists are documented "sorted"; every test approves in alphabetical order already. |
| 6 | computeApproval approve.ts:213 |
drop .sort() on approvers |
Same for the PR-level approvers set (drives the notifier's "approved by" order). |
| 7 | syncLabel approve.ts:396 |
drop .toLowerCase() |
An existing Approved label (any casing) must count as present; otherwise approved is re-added. |
| 8–10 | isBot approve.ts:430 |
===→!== (both sides), ||→&& |
The notifier is found by `user.type === 'Bot' |
Equivalent / not worth pinning (recorded for the next probe so it is not re-derived): :202 debug text, :208 &&→|| (author in approving already covers every file they could be suggested for), :243 size > 0 → >= 0 (loop exits on coverage.size === 0), :347 sort of a union of identical per-OWNERS lists, :400 removeLabels([]) is a no-op, :416 .trim() either side.
Recommendation
Add six cases to __tests__/plugins/approve.test.ts (approvalEvents with a tokenLogin; any-Bot and empty-login comments; require_self_approval author-first ordering; non-alphabetical approval order across one and two files) and four to __tests__/plugins/approveEvents.test.ts (another App's bot notifier is patched; a human quoting the marker is not; Approved label casing). Each of the ten mutants then fails at least one test; the whole set stays green on main.
Priority
- Impact: medium (3 and 8–10 are authorization/idempotency behaviour of the approve plugin; the rest are ordering/idempotency contracts the docs promise)
- Effort: low
Decision taken by hive: pin behaviour with tests only; no production change.
Filed by quality agent (hold-gated mode)
🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5q9t | SHA: 88241f4
— hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88
Activity
github-actions commentedon Oct 19, 2026
Please add a kind label with
/kind feature,/kind failing-testor/kind cleanup.hivecommons-hive commentedon Oct 20, 2026
Duplicate tracking record. Canonical config regression-test issue: #534; implementation: #535. This is not a demonstrated production config-loss bug and is not part of #521/#522.
Folded finding. This was filed as a separate issue, but 1 open agent-filed issue (plus 2 closed as completed in the last 14 days) already cite
test.ts, so it was folded into the oldest of them instead of opening another. Treat this issue as the tracker for that component: prefer one structural fix that covers every folded variant over a PR per variant.[quality] config.ts: mergeProwConfig drops an organization lgtm/sweep section when the repository tier has none, and values-without-exclusive / object-without-color parse only by accident — 5 mutation survivors
Finding
src/utils/config.tsis 100 % line- and branch-covered bynpm run test:coverageand, apart from theString(e)arm at line 657 that #528 records as unreachable from the bundle, bynpm run test:coverage:e2eas well (main@ 88241f4, local runs). A single-operator mutation probe over the file (258 mutants:===/!==,&&/||,??→||,=== undefined→=== null, comparison bounds,.every/.some,.find/.findLast,Math.min/max, spread-source swaps inmergeProwConfig,true/false) against the six direct config suites left 33 alive; re-run againstnpx vitest related src/utils/config.ts --run(70 files / 1498 tests) two non-equivalent mutants still survive, and three more are killed by exactly one unrelated test:lgtm: { ...base.lgtm, ...over.lgtm }→{ ...over.lgtm, ...over.lgtm }lgtm.bind_to_commit: truemust survive a repositoryprow.yamlthat has nolgtmsection. ThemergeProwConfigtest putslgtmonly in the repo tier, so the org tier is never the one that has to survive.sweep: { ...base.sweep, ...over.sweep }→{ ...over.sweep, ...over.sweep }sweep.lookback: no test merges asweepsection at all (sweep: {}is the only asserted value).section.exclusive === undefined→=== nullkind: { values: [...] }withoutexclusive— the documented optional form — must parse. Everyvalues:mapping in the config suites setsexclusive; onlylabelCatalog.test.ts› "de-duplicates case-insensitively" happens to parse one.value.color === undefined→!== undefined;value.color !== undefined→!== null{ name: bug }and{ name: docs, description: … }without acolormust parse (and must not be run through the color check). Every object value in the config suites carriescolor; again only thelabelCatalogde-dup test pins it.unknown.length > 0→>= 0core.debug('…ignoring unknown top level keys: ')line. Debug-only, but free to pin next to the existing "logs them once" case.Equivalent survivors, not actionable:
.trim()on theconfiginput (core.getInputalready trims); every??→||on a value the normalizers already reject when falsy (tide.*,hold.label,authorization.*,require_matching_label,users,sweep.lookback);max_request_count !== undefined→!== null(undefined < nis false either way);fetchRepoFile's||→&&folds on the not-a-file guard (nogetContentshape distinguishes them);.find→.findLaston the unknown-authorization-key message.ms <= 0(line 659) and the 24 h cap (line 720) are pinned by__tests__/cronJobTest/sweep.test.ts;merge_on_events ?? true(line 787) bymergeGate.test.ts.Recommendation
Add to
__tests__/utils/config.test.ts:mergeProwConfig› orglgtm/sweepsurvive a repo tier without them, and are overridden when the repo tier has them.label values› an object value withoutcolorin a list, and avalues:mapping withoutexclusive(assertingexclusiveis absent).parseProwConfig›core.debugnot called when every top-level key is known.Test-only; no
src/,dist/or docs change. Files claimed:__tests__/utils/config.test.ts. Disjoint from every open held PR (#510–#533): none touches__tests__/utils/config*.test.tsorsrc/utils/config.ts.Evidence
npm run test:coverage→ 100 / 99.75 / 100 / 100 (main@ 88241f4, local run; the four reported branches are theawaitartifact docs(contributing): record the four negative-count BRDA branches as a v8 await artifact, not a gap #511 documents)npm run test:coverage:e2e→ 99.96 / 98.79 / 100 / 99.95 (same revision, local run;config.tsshows only line 657, see test(bundle): cover non-Error rejection paths including auth #528). The two reports come from different transforms and are not merged.src/utils/config.ts; direct suitesnpx vitest run __tests__/utils/{config,configLoader,configSweep}.test.ts __tests__/{configurationDocs,dogfoodConfig,docsYamlExamples}.test.ts(241 tests, ~6 s); survivors re-run withnpx vitest related src/utils/config.ts --run→ 1498 passed for lines 740, 741, 421; 1 failed (labelCatalog.test.ts) for 451, 469, 472.Priority
mergeProwConfigis the org/repo tiering rule every plugin reads through; a regression there silently turns off an organization-widelgtm.bind_to_commitor sweep window for every repository that ships its ownprow.yamlFiled by quality agent (hold-gated mode)
🐝 Hive Agent:
quality| Instance:hosted-available-lke648397-260827-5q9t| SHA:88241f4— hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88