Skip to content

[quality] sweep.ts delayed-reviewers OWNERS guard (line 80 false arm) is covered by neither the unit nor the bundle suite #560

Open

Description

@hivecommons-hive

Finding

src/cronJobs/sweep.ts guards the delayed-reviewers loop with branchHasOwners(octokit, context, pr.base.ref) before calling requestReviewersIfDue. The false arm of that guard — a pull request that is due for delayed blunderbuss but whose base branch has no OWNERS files — is exercised by neither suite:

  • Unit, npm run test:coverage on main@e3119ad (167 files / 2330 tests, 99.92 % lines / 99.35 % branches): coverage/lcov.info records SF:src/cronJobs/sweep.ts BRDA:80,4,1,0 (zero hits). __tests__/cronJobTest/sweep.test.ts has a sweep on a repository without OWNERS files block, but none of its cases configure blunderbuss.delay, so no pull request ever reaches the delayed loop there; the sweep with blunderbuss.delay block serves an OWNERS file on master in every case.
  • End-to-end, npm run test:coverage:e2e on main@e3119ad (86 files / 528 tests, 100 % lines / 99.46 % branches): coverage-e2e/lcov.info records the same BRDA:80,4,1,0; every scenario in __tests__/bundle/blunderbussDelay.test.ts routes git/trees/master with an OWNERS blob.

Both reports were produced locally from the same checkout at e3119ad; per docs/contributing.md the two runs are deliberately not merged (vite vs. tsc+webpack maps), so they are read separately above. The nearby zero-hit records at sweep.ts:85 (non-Error throw in the same loop) and :208 are claimed by #558 and #559 respectively and are not part of this issue.

Why it matters: this is the only path by which a repository that adopts blunderbuss.delay without OWNERS files is protected from the sweep attempting reviewer selection against an empty OWNERS set. A one-character mutant that drops the guard (|| true) is accepted by the whole current sweep.test.ts and by the whole current bundle suite — verified by applying it, rebuilding the coverage bundle, and running both.

Recommendation

Add one case per suite, in new files so the held PRs #558 (__tests__/cronJobTest/sweep.test.ts) and #559 / #555 (__tests__/bundle/blunderbussDelay.test.ts) are not touched:

  • Unit: __tests__/cronJobTest/sweepDelayWithoutOwners.test.ts — blunderbuss.delay: 1h, an empty master tree, one pull request created and updated 70 min ago; assert sweep resolves with no candidates and no failures, the tree was read, and the timeline, reviews and requested_reviewers endpoints were never called.
  • Bundle: __tests__/bundle/blunderbussDelayWithoutOwners.test.ts — the same scenario through dist/index.js via the schedule event; assert exit 0, sweep: 1 more checked for delayed reviewers, exactly one git/trees/master?recursive=true read, zero timeline/review reads, zero non-GET requests, and that the only non-config requests are the pull list and the tree read.

Both cases must fail against the || true mutant (the unit case via the unhandled-request guard, the bundle case via a could not read the timeline: HttpError: Not Found sweep failure).

Priority

  • Impact: medium (guard protects the delayed blunderbuss path on OWNERS-less repositories; covered by neither suite → coverage-gap priority 1 under the hive evidence rules)
  • Effort: low

Decision taken by hive: new test files rather than edits to the two files held PRs already occupy, to keep every open test PR rebase-free.


Filed by quality agent (hold-gated mode)


🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5q9t | SHA: e3119ad

— hive: agent=quality backend=copilot model=claude-fable-5.1

Activity

  1. added
    testingCreated by Hive for agent-filed issue provenance
    agent/qualityCreated by Hive for agent-filed issue provenance
    on Oct 22, 2026
  2. github-actions commented on Oct 22, 2026

    @github-actions
    Contributor

    Please add a kind label with /kind feature, /kind failing-test or /kind cleanup.

  3. hivecommons-hive commented on Oct 22, 2026

    @hivecommons-hive
    ContributorAuthor

    Folded finding. This was filed as a separate issue, but 1 open agent-filed issue (plus 4 closed as completed in the last 14 days) already cite docs/contributing.md, 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] sweep: delayed-only pull listed without requested_reviewers/requested_teams keys is untested (sweep.ts:208)

    Finding

    requestReviewersIfDue in src/cronJobs/sweep.ts (line 208) guards the delayed-reviewers decision with (pr.requested_reviewers ?? []).length > 0 || (pr.requested_teams ?? []).length > 0, but no test lists a delayed-only pull request (updated before the normal lookback, inside the blunderbuss.delay due window) whose list entry omits those keys. Both ?? fallback arms are the only remaining non-phantom branches in sweep.ts that neither suite reaches, once open PRs #558 (timeline-read failure, :85/:244) and #561 (OWNERS guard, :80) land.

    The existing sweep.test.ts case treats a pull request listed without labels or requested_reviewers as unlabelled with no reviewers exercises the bare-listing shape only on the candidate path (sweepPullRequest), not on the delayed path.

    Evidence (revision e3119ad, run locally)

    Suite Command Result
    Unit npm run test:coverage 2330 passed; coverage/lcov.info → SF:src/cronJobs/sweep.ts BRDA:208,27,1,0 and BRDA:208,28,1,0
    E2E bundle npm run test:coverage:e2e 528 passed; coverage-e2e/lcov.info → SF:src/cronJobs/sweep.ts BRDA:208,27,1,0 and BRDA:208,28,1,0

    Per docs/contributing.md the two coverage runs are deliberately not merged (vite vs. tsc+webpack maps), so they were analysed separately; both miss the same two arms. These are zero-hit arms, not the documented negative-count v8 phantoms (the awk recipe in docs/contributing.md lists only close.ts:39, reopen.ts:36, approve.ts:504, auth.ts:220 as phantoms).

    Residuals deliberately not filed: the e instanceof Error ? e.message : String(e) non-Error arms at sweep.ts:85 and config.ts:618 are unreachable through the code paths that feed them (parseDuration and octokit always throw Errors).

    Recommendation

    Add one unit test and one bundle (e2e) test, in new files so they do not collide with #558's edits to __tests__/cronJobTest/sweep.test.ts or #559's edits to __tests__/bundle/blunderbussDelay.test.ts:

    • __tests__/cronJobTest/sweepDelayBareListing.test.ts: a delayed-only due pull served from GET /pulls with requested_reviewers and requested_teams removed from the list item still gets reviewers requested (POST /pulls/1/requested_reviewers called, failures: []).
    • __tests__/bundle/blunderbussDelayBareListing.test.ts: the same scenario against dist/index.js via the fake GitHub API.

    Priority

    • Impact: low (defensive fallback; GitHub normally sends the arrays)
    • Effort: low

    Filed by quality agent (hold-gated mode)


    🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5q9t | SHA: unknown

    — hive: agent=quality backend=copilot model=claude-fable-5.1


    Folded finding. This was filed as a separate issue, but 1 open agent-filed issue (plus 4 closed as completed in the last 14 days) already cite docs/contributing.md, 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] normalizeBlunderbuss: the non-Error parseDuration catch arm (config.ts:618) is uncovered by both suites

    Finding

    normalizeBlunderbuss in src/utils/config.ts rewraps a parseDuration failure for blunderbuss.delay as

    throw new Error(`${source}: ${e instanceof Error ? e.message : e}`)

    (line 618 at main@e3119ad). The non-Error arm of that ternary is the single remaining unit-uncovered branch in config.ts, and the same line is listed as uncovered by the bundle run.

    The sibling sweep.lookback arm (config.ts:677) already has the repository's treatment for this shape: __tests__/utils/configSweep.test.ts drives the arm by spying parseDuration to throw a string, and the arm carries a /* v8 ignore next -- non-Error arm unreachable from dist/index.js; … drives it */ hint for the bundle run (same pattern as #549). blunderbuss.delay arrived in #530 without either.

    Coverage evidence

    • Unit: npm run test:coverage on main@e3119ad (2330 tests, 99.92 % lines / 99.35 % branches) — config.ts 100 / 99.61, uncovered 618; coverage/lcov.info records BRDA:618,76,1,0.
    • End-to-end: npm run test:coverage:e2e on main@e3119ad (528 bundle tests, 100 % lines / 99.46 % branches) — config.ts 100 / 99.61, uncovered 618. parseDuration only ever throws Error, so the arm is unreachable from dist/index.js.
    • The two runs are read separately on purpose (docs/contributing.md: their statement/branch maps come from different transforms and do not combine).

    Not one of the four documented phantom branches (close.ts:39, reopen.ts:36, approve.ts:504, auth.ts:220): the record is a plain zero, not negative.

    Recommendation

    Mirror the sweep.lookback treatment:

    • add __tests__/utils/configBlunderbussDelay.test.ts that spies parseDuration (vi.mock('../../src/utils/duration', { spy: true })) to throw a string and asserts parseProwConfig('x', 'blunderbuss:\n delay: 1h\n') throws x: clock skew
    • add the v8 ignore next hint on the non-Error alternate at config.ts:618 naming that test, and repack dist/

    Decision taken by hive: follow the existing in-repo pattern for this arm shape (#549, config.ts:677) rather than introduce a new one.

    Priority

    • Impact: medium (last unit-uncovered branch in config.ts; keeps both coverage floors honest)
    • Effort: low

    Filed by quality agent (hold-gated mode)


    🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5q9t | SHA: e3119ad

    — hive: agent=quality backend=copilot model=claude-fable-5.1


    Folded finding. This was filed as a separate issue, but 1 open agent-filed issue (plus 4 closed as completed in the last 14 days) already cite src/cronJobs/sweep.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] sweep.ts:85 — the delayed-reviewers catch of sweep has no unit test and its String(e) arm no test at all

    Finding

    sweep (src/cronJobs/sweep.ts) runs branchHasOwners + requestReviewersIfDue for every delayed-only pull request inside a try/catch that turns a throw into { number, message: blunderbuss: ${e instanceof Error ? e.message : String(e)} } (line 85 at main@e3119ad), so one failing pull request does not stop the loop and the sweep fails at the end with every failure listed.

    Evidence at main@e3119ad, local runs:

    • Unit — npm run test:coverage (2330 tests, 99.91 % lines / 99.35 % branches): coverage/lcov.info has BRDA:85,5,0,0 and BRDA:85,5,1,0. Neither arm of the catch is reached: no unit test makes a delayed pull request fail. (sweep.ts 98.33 / 92.3, uncovered 85,244.)
    • End-to-end — npm run test:coverage:e2e (528 tests, 100 % lines / 99.46 % branches): coverage-e2e/lcov.info has BRDA:85,5,0,1 and BRDA:85,5,1,0. The Error arm is hit once by the bundle suite; the String(e) arm never.
    • The two runs are analysed separately on purpose: their statement/branch maps come from different transforms and do not combine (see vitest.e2e-coverage.config.mjs).

    Held PRs #558, #561 and #562 each state that line 85 is out of their scope, so nothing open claims it.

    Recommendation

    Add a unit test file (new file, so it does not collide with #558's edits to __tests__/cronJobTest/sweep.test.ts) with two due delayed-only pull requests on a base branch with OWNERS:

    • the reviews read of one answers 500 → sweep rejects with sweep: 1 pull request(s) failed: #1 (blunderbuss: <message>) and the other pull request still receives its reviewer request (Error arm; also pins that the loop continues past a failure);
    • requestOwnersReviewers rejects once with a non-Error → the failure reads blunderbuss: <String(e)> (String(e) arm; same vi.mock(..., { spy: true }) technique as __tests__/utils/configSweep.test.ts).

    The String(e) arm stays unreachable from the bundle (octokit only ever rejects with RequestError); whether to v8-ignore it for the e2e run the way #537/#544/#549 did is a separate, production-file change and is not part of this issue.

    Priority

    • Impact: high (the only error path of the delayed-reviewers loop; a regression that let one bad pull request abort the loop or lose the failure list would go unnoticed)
    • Effort: low

    Filed by quality agent (hold-gated mode)


    🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5q9t | SHA: e3119ad

    — hive: agent=quality backend=copilot model=claude-fable-5.1


    Folded finding. This was filed as a separate issue, but 1 open agent-filed issue (plus 4 closed as completed in the last 14 days) already cite docs/contributing.md, 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] sweep: the ignore_drafts=false arm of delayed reviewers (sweep.ts:218) has no unit test

    Finding

    requestReviewersIfDue in src/cronJobs/sweep.ts branches on blunderbuss.ignore_drafts at line 218. When it is false, a due draft is not skipped at line 204, and the sweep times the pull request from created_at alone instead of reading its timeline for the last ready_for_review. The unit suite never takes that arm: every sweep with blunderbuss.delay test in __tests__/cronJobTest/sweep.test.ts runs with the default ignore_drafts: true.

    Evidence (both at e3119ad, run locally):

    • Unit: npm run test:coverage → coverage/lcov.info BRDA:218,30,1,0 (the ignore_drafts false arm, 0 hits).
    • E2E: npm run test:coverage:e2e → coverage-e2e/lcov.info BRDA:218,30,1,1, driven by __tests__/bundle/blunderbussDelay.test.ts ("uses creation time and requests on a due draft when ignore_drafts is false").

    The two runs are analyzed separately on purpose: vitest.e2e-coverage.config.mjs documents that their branch maps come from different transforms and do not combine. So this arm is covered end-to-end but not by a unit test; the per-request behaviour (a draft is requested, the timeline is never read) is only pinned through the bundle.

    Not in scope: close.ts:39, reopen.ts:36, approve.ts:504, auth.ts:220 — these are the phantom negative-count branches docs/contributing.md documents (negative BRDA hits in both runs), not misses. sweep.ts:80-85, :208, :244 and config.ts:618 are already claimed by open PRs #558, #561, #562, #563 and #565.

    Recommendation

    Add a unit test, in its own file so it does not collide with #558's edits to the sweep with blunderbuss.delay block, that configures blunderbuss: { delay: 1h, ignore_drafts: false } and asserts:

    • a due draft (opened 70m ago) gets reviewers requested;
    • the issues/:number/timeline endpoint is never called;
    • a draft opened after the due window is still left alone (the created_at gate at line 214 still applies).

    Priority

    • Impact: low (covered by the e2e suite; unit-only miss)
    • Effort: low

    Filed by quality agent (hold-gated mode)


    🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5q9t | SHA: unknown

    — hive: agent=quality backend=copilot model=claude-fable-5.1


    Folded finding. This was filed as a separate issue, but 1 open agent-filed issue (plus 4 closed as completed in the last 14 days) already cite docs/contributing.md, 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] Mutation runner for the #554 harness box: Stryker 10 vitest-runner runs 0 tests per mutant under Vitest 5.0.4; config ready to land once upstream fixes selection

    Finding

    Tracker #554 (section P1: Strengthen assertions and the test harness) asks for "bounded, repeatable mutation checks for selected high-risk behavior, with reproducible commands and reviewed equivalent-mutant exclusions". The repository has no mutation runner (grep -ri stryker . finds nothing outside node_modules), so every mutation check so far has been done by hand, one edit at a time, in PR descriptions (#557 tide.ts:203, #558 and #562 sweep.ts, #532).

    The obvious tool, StrykerJS with its Vitest runner, does not work against this repository today: @stryker-mutator/vitest-runner 10.0.0 (latest, 2026-08-14) is broken under the Vitest 5.0.4 this repository pins. Reproduced on main @ e3119ad, Node 26.12.0, after npm install --save-dev --save-exact @stryker-mutator/core@10.0.0 @stryker-mutator/vitest-runner@10.0.0 and the config below:

    Run Result
    npx stryker run --mutate src/plugins/tide.ts --checkers "" (defaults: related: true, coverageAnalysis: perTest) dry run reports 1233 tests (the suite has 1802 non-bundle tests); 538 mutants instrumented; 396 survived, 10 killed, 1 timeout, "Ran 0.00 tests per mutant on average"; 24 min 34 s. Mutants such as removing the throw in tide.ts:580 survive although the clear-text report lists tideOnPullRequest throws when the payload has no pull request as the test "run" for it.
    same with vitest.related: false dry run now reports the full 1802 tests; per-mutant runs still execute 0 tests (6/6 survived on --mutate "src/plugins/tide.ts:578-581").
    related: false + --coverageAnalysis off (no per-test name filter at all) each mutant run still finishes in ~5 s with 0 tests executed; 6/6 survived.

    Two separate upstream defects are involved, both open at stryker-mutator/stryker-js:

    1. The per-test filter never matches a test inside a describe on Vitest 5, because the runner joins suite and test names with a space while Vitest 5 matches testNamePattern against names joined with > (vitest-runner: on Vitest 5 the per-test name filter matches nothing, so every covered mutant survives (testNamePattern now joins the chain with ' > ') stryker-mutator/stryker-js#6210, #6262, #6263, #6265; fix PRs #6214, #6220, #6247, #6260 — none merged as of 2026-10-22).
    2. Even unfiltered, a second ctx.start() on the runner's single reused Vitest instance executes nothing under Vitest 5.0.4 (the runner only clears ctx.state.filesMap between runs; I did not isolate why Vitest 5 then runs no files). This is the reason the coverageAnalysis: off row above also runs 0 tests; I did not find it filed upstream.

    Because every mutant "survives" with zero tests executed, a test:mutation script added now would report a 0 % score against a suite that in fact kills these mutants by hand (#557 shows the tide.ts:203 mutant fails on one case). That is worse than no runner: it would teach readers to ignore the report. I therefore did not open a PR.

    Recommendation

    Adopt StrykerJS once a @stryker-mutator/vitest-runner release carries the Vitest 5 selection fix (watch stryker-mutator/stryker-js#6260 and the vitest-runner changelog) and re-run the reproduction above; both failure modes must be gone (Ran N tests per mutant with N > 0, and the tide.ts:580 throw-removal mutant killed). The files to land are ready and need no further design — this is the exact text that was validated to instrument, dry-run and sandbox correctly here, only the per-mutant run is blocked:

    • package.json: devDependencies @stryker-mutator/core and @stryker-mutator/vitest-runner (exact-pinned to the fixed release), script "test:mutation": "stryker run".
    • .gitignore: append
      # Sandboxes and report written by `npm run test:mutation`
      .stryker-tmp
      reports/mutation
      
    • stryker.config.mjs (new):
      // `npm run test:mutation`: a bounded mutation check of the behaviour that near-100 %
      // coverage has repeatedly failed to protect (merge gating, delayed reviewer sweeps,
      // OWNERS group expansion and authorization). It is not a repository-wide gate and is
      // not wired into CI: it runs on demand before changing one of the files below, or
      // when a review asks whether a test actually distinguishes a behaviour.
      //
      // Scope is a list of files, not line ranges, so it does not rot as the files move;
      // widen it one file at a time and record the baseline score in docs/contributing.md.
      // Only the unit suite runs against the mutants. `__tests__/bundle/` executes the
      // committed dist/index.js as a child process, which Stryker cannot instrument, so
      // those tests would pass on every mutant and only add time.
      //
      // A mutant that survives is either a missing assertion (add the test) or an
      // equivalent mutant (record it under `// Stryker disable next-line <mutator>: <why>`
      // in the source, with the reason on the same line, so the exclusion is reviewed with
      // the code it describes). Do not widen `ignorePatterns` or lower `thresholds.break`
      // to make a run pass.
      
      /** @type {import('@stryker-mutator/api/core').PartialStrykerOptions} */
      export default {
        mutate: [
          'src/plugins/tide.ts',
          'src/cronJobs/sweep.ts',
          'src/utils/owners.ts',
          'src/utils/auth.ts',
        ],
        testRunner: 'vitest',
        vitest: {
          configFile: 'vitest.config.mjs',
          // Vitest's related mode drops roughly a third of the suite here (1233 of 1802
          // tests found in the dry run); perTest coverage analysis already runs only the
          // tests that reach each mutant.
          related: false,
        },
        // Stryker copies the project into .stryker-tmp/ sandboxes; skip what the unit
        // suite never reads so the copy stays small.
        ignorePatterns: ['dist', 'lib', 'coverage', 'coverage-e2e', '.coverage-bundle', '.vitest', 'reports', '__tests__/bundle'],
        checkers: ['typescript'],
        tsconfigFile: 'tsconfig.json',
        reporters: ['clear-text', 'progress', 'html'],
        htmlReporter: { fileName: 'reports/mutation/index.html' },
        // Logging strings, debug lines and the text of thrown errors are not behaviour the
        // suite is asked to pin; mutating them only produces noise.
        mutator: {
          excludedMutations: ['StringLiteral', 'ObjectLiteral'],
        },
        coverageAnalysis: 'perTest',
        // `break` is set just under the score recorded at the current baseline so a change
        // that stops a test from distinguishing a behaviour fails the run. Update it, with
        // the baseline, when the score moves for a reviewed reason.
        thresholds: { high: 90, low: 80, break: null },
      }
      Do not set vitest.dir: '__tests__': the config's include: ['__tests__/**/*.test.ts'] is then resolved relative to that directory and the dry run finds no tests.
    • docs/contributing.md: a ### Mutation checks subsection after Phantom "uncovered" branches recording the command, the four-file scope, the first measured baseline score per file, and the // Stryker disable next-line convention for reviewed equivalent mutants. (Added as a new subsection, not a row in the Testing table, so it does not collide with chore(coverage): raise the e2e branch floor to 98 after the pending coverage work landed #556's edit of that table.)
    • After the first green run, set thresholds.break just under the measured score and record the figure in the doc comment, mirroring how vitest.config.mjs documents its coverage floor.

    Decision taken by hive: wait for the upstream runner fix rather than (a) pinning a second Vitest 4 copy under an npm alias for the mutation run — the runner resolves vitest from the project root, so this would need a separate node_modules — or (b) writing a bespoke "apply these N hand mutants and expect a red test" script. Option (b) is what #557/#558/#562 already do in prose; it is reproducible but not bounded in the sense #554 asks for (it only re-checks mutants someone already thought of). If the upstream fix has not shipped by the time the P1 harness section of #554 is otherwise complete, (b) is the fallback and this issue should be re-scoped to it.

    No CI gate is proposed; #554 states a repository-wide mutation gate is not required, and .github/workflows/ changes are out of reach for this agent in any case.

    Priority

    Related

    Refs #554 — this is the focused child issue for its "bounded, repeatable mutation checks" box; the tracker stays open for its other sections.


    Filed by quality agent (hold-gated mode)


    🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5q9t | SHA: e3119ad

    — hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88


    Folded finding. This was filed as a separate issue, but 1 open agent-filed issue (plus 3 closed as completed in the last 14 days) already cite docs/contributing.md, 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] Gate proposal: per-site v8-ignore markers on bundle-unreachable non-Error catch arms — one errorMessage() helper + ESLint selector

    Finding

    The repository keeps re-handling one class of coverage miss site by site: a catch that formats the error as e instanceof Error ? e.message : String(e) (or : e), whose non-Error arm is unit-covered but never reachable from dist/index.js because octokit only rejects with RequestError and the local callee only throws Error. Each time the e2e floor in vitest.e2e-coverage.config.mjs notices one, the fix has been a per-site /* v8 ignore next -- non-Error arm unreachable from dist/index.js; <test> drives it */ marker plus a dist/ rebuild. Instances so far:

    # site treatment
    1 src/labels/lgtm.ts:68 #537 (merged)
    2 src/run.ts:42, src/utils/command.ts:25, src/utils/config.ts:678 #544 (merged)
    3 src/plugins/requireMatchingLabel.ts:169 #549 (merged)
    4 src/utils/config.ts:618 (normalizeBlunderbuss) #563 (open, hold)
    5 src/cronJobs/sweep.ts:85 (delayed-reviewers catch) #565 (open, hold) covers the unit side and explicitly defers the marker as "a production-file change outside this test PR"

    Evidence at main@e3119ad (npm run test:coverage:e2e, 528 tests, 100 % lines / 99.46 % branches): coverage-e2e/lcov.info reports BRDA:85,5,1,0 for sweep.ts and BRDA:618,76,1,0 for config.ts — the String(e) arms with zero bundle hits. The unit run (npm run test:coverage, 2330 tests, 99.92 % / 99.35 %) is read separately as docs/contributing.md requires.

    Sites of the same shape that have not yet been flagged and will be next: src/cronJobs/labelSync.ts:107, src/cronJobs/sweep.ts:162, src/plugins/tide.ts:138. The repository also already has two private copies of the helper this proposal asks for: errorMessage() at src/plugins/approveReview.ts:229 and the fallback line of errorMessage() at src/utils/mergeQueue.ts:190.

    Three or more instances of one class means the gate is missing, so this issue proposes the gate instead of filing instance #6.

    Recommendation

    Decision requested: accept or decline the following gate.

    1. One shared helper. Add src/utils/errors.ts exporting errorMessage(e: unknown): string (the body of approveReview.ts:229), with a single unit test in __tests__/utils/errors.test.ts driving both arms. The one bundle-unreachable String(e) arm then lives in exactly one place and carries exactly one v8 ignore marker; the per-site markers at the five sites above become plain errorMessage(e) calls and are deleted. mergeQueue.ts's errorMessage keeps its errors[]-array handling and falls through to the shared helper.

    2. Lint rule that stops the class from coming back. In eslint.config.mjs, for src/** except src/utils/errors.ts, add:

      'no-restricted-syntax': ['error', {
        selector: 'ConditionalExpression > BinaryExpression.test[operator="instanceof"][right.name="Error"]',
        message: 'Format unknown errors with errorMessage() from src/utils/errors.ts; do not inline the instanceof-Error ternary (each copy adds a bundle-unreachable branch).',
      }],

      npm run lint already runs in .github/workflows/test.yml (line 36), so no workflow file changes and the gate is enforced on every PR.

    Risk: governance — this changes how contributors must write error handling in src/, and it is a production-code refactor, which the quality lane is not permitted to PR. Decision taken by hive: proposed the simplest additive option (one helper, one lint selector, existing lint step) rather than a lower e2e branch floor or a blanket coverage.ignoreClassMethods/path exclusion, both of which would hide real misses on the same lines.

    Priority

    • Impact: medium (every new catch in src/ otherwise costs a follow-up coverage PR plus a dist/ rebuild)
    • Effort: low (helper ≈ 5 lines, one lint entry, five mechanical call-site edits)

    Filed by quality agent (hold-gated mode)


    🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5q9t | SHA: e3119ad

    — hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    agent/qualityCreated by Hive for agent-filed issue provenanceneeds-kindstage/040-covered-by-prAn open PR covers this issuetestingCreated by Hive for agent-filed issue provenance

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions