From 84043381987cba35d37a6b732bf579b5769dc340 Mon Sep 17 00:00:00 2001 From: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Mon, 19 Oct 2026 15:29:08 -0400 Subject: [PATCH 1/2] test(bundle): drive the non-Error String(e) arms of runEventHandlers, normalizeError and label-sync's write through dist/index.js A preload hands the bundle one Response whose status getter throws a plain string, the one read @octokit/request makes outside its try/catch, so the string rejects the client call unwrapped. Three runs pin that the step still fails once naming the string: pull_request synchronize (events.ts:50), /milestone (handleIssueComment.ts:217) and the label-sync job (labelSync.ts:107). Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> --- .../bundle/nonErrorRejectionArms.test.ts | 107 ++++++++++++++++++ __tests__/bundle/nonErrorRejectionPreload.cjs | 36 ++++++ 2 files changed, 143 insertions(+) create mode 100644 __tests__/bundle/nonErrorRejectionArms.test.ts create mode 100644 __tests__/bundle/nonErrorRejectionPreload.cjs diff --git a/__tests__/bundle/nonErrorRejectionArms.test.ts b/__tests__/bundle/nonErrorRejectionArms.test.ts new file mode 100644 index 00000000..95cc9dfb --- /dev/null +++ b/__tests__/bundle/nonErrorRejectionArms.test.ts @@ -0,0 +1,107 @@ +import type { FakeGithub } from './fakeGithub' +import path from 'node:path' +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' + +import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' +import { start } from './fakeGithub' +import { comment, helpersFor, repo, token } from './helpers' +import { runBundle } from './runBundle' + +vi.setConfig({ testTimeout: 30_000 }) + +const preload = path.resolve(__dirname, 'nonErrorRejectionPreload.cjs') + +// The `e instanceof Error ? e.message : String(e)` arms of the dispatchers and of label-sync's per-label +// write (events.ts:50, handleIssueComment.ts:217, labelSync.ts:107) only run when a client call rejects +// with something that is not an Error. Octokit wraps every fetch rejection in a RequestError, so the +// preload hands the bundle one Response whose `status` getter throws a plain string, the one read that +// @octokit/request makes outside its try/catch. The run must still fail once with that string in the +// message, after the same request list as the Error-shaped failure, with no unhandled rejection. +describe('dist/index.js when an api call rejects with a non-Error value', () => { + let gh: FakeGithub + const { calls } = helpersFor(() => gh) + + beforeAll(async () => { + gh = await start() + }) + afterEach(() => gh.reset()) + afterAll(() => gh.close()) + + function env(target: string) { + return { NODE_OPTIONS: `--require "${preload}"`, NON_ERROR_REJECT: target } + } + + describe('pull_request synchronize: runEventHandlers folds the string into the one failure', () => { + beforeEach(() => gh.mergeQueueFallback({ pullRequestId: 'PR_none', headOid: pullReqOpenedEvent.pull_request.head.sha, enabled: false })) + + it('owners-label\'s pull request read rejects with a string: the step fails naming it and the later handlers still run', async () => { + const result = await runBundle({ + eventName: 'pull_request', + payload: { ...pullReqOpenedEvent, action: 'synchronize' }, + inputs: token, + apiUrl: gh.url, + env: env(`GET ${repo}/pulls/1`), + }) + + expect(result.status, result.stdout).toBe(1) + expect(result.errors).toHaveLength(1) + expect(result.errors[0]).toMatch(/^error handling pull_request event: /) + expect(result.errors[0]).toContain(`GET ${repo}/pulls/1 rejected with a string, not an Error`) + expect(result.errors[0]).not.toContain('[object') + expect(result.stderr).not.toMatch(/Unhandled|unhandledRejection/) + // the faulted read reached the fake and was recorded like any other + expect(calls()[0]).toBe(`GET ${repo}/pulls/1`) + }) + }) + + describe('issue_comment: the per-command normalizeError coerces the string', () => { + it('/milestone\'s milestone list rejects with a string: the step fails with "error handling issue comment: Error: "', async () => { + gh.route('GET', `${repo}/collaborators/Codertocat`, { status: 204 }) + gh.route('GET', `${repo}/milestones`, { status: 200, body: [{ number: 7, title: 'Sprint 2' }] }) + + const result = await runBundle({ + eventName: 'issue_comment', + payload: comment('/milestone Sprint 2'), + inputs: { ...token, 'prow-commands': '/milestone' }, + apiUrl: gh.url, + env: env(`GET ${repo}/milestones`), + }) + + expect(result.status, result.stdout).toBe(1) + expect(result.errors).toEqual([ + `TypeError: error handling issue comment: Error: GET ${repo}/milestones rejected with a string, not an Error`, + ]) + expect(result.stderr).not.toMatch(/Unhandled|unhandledRejection/) + // the milestone is never set once the list is lost + expect(calls()).toEqual([`GET ${repo}/collaborators/Codertocat`, `GET ${repo}/milestones`]) + }) + }) + + describe('workflow_dispatch label-sync: a non-Error write refusal is recorded like an Error one', () => { + it('the first create rejects with a string: it is logged and recorded, the remaining labels are still created, and the run fails once', async () => { + gh.route('GET', `${repo}/labels`, { status: 200, body: [] }) + gh.route('POST', `${repo}/labels`, { status: 201, body: {} }) + + const result = await runBundle({ + eventName: 'workflow_dispatch', + payload: {}, + inputs: { ...token, jobs: 'label-sync' }, + apiUrl: gh.url, + env: env(`POST ${repo}/labels`), + }) + + expect(result.status, result.stdout).toBe(1) + expect(result.errors).toHaveLength(2) + expect(result.errors[0]).toBe(`label-sync: could not sync approved: POST ${repo}/labels rejected with a string, not an Error`) + expect(result.errors[1]).toBe(`TypeError: error handling cron job: Error: 1 label(s) could not be synced: approved (POST ${repo}/labels rejected with a string, not an Error)`) + expect(result.stderr).not.toMatch(/Unhandled|unhandledRejection/) + + // every other built-in label is still created after the string refusal + const posts = gh.requestsMatching('POST', /\/labels$/) + expect(posts.length).toBeGreaterThan(1) + expect((posts[0].body as { name: string }).name).toBe('approved') + expect(result.stdout).toContain(`label-sync: created ${posts.length - 1} [`) + expect(result.stdout).toContain(', unchanged 0, failed 1') + }) + }) +}) diff --git a/__tests__/bundle/nonErrorRejectionPreload.cjs b/__tests__/bundle/nonErrorRejectionPreload.cjs new file mode 100644 index 00000000..b38a00c9 --- /dev/null +++ b/__tests__/bundle/nonErrorRejectionPreload.cjs @@ -0,0 +1,36 @@ +// Preloaded into the bundle child with `NODE_OPTIONS=--require` by the non-Error rejection e2e test. +// The first request whose `METHOD /path` equals NON_ERROR_REJECT still reaches the fake GitHub (so it is +// recorded and answered) but the bundle is handed a Response whose `status` getter throws a plain string. +// `status` is the first thing @octokit/request reads after its fetch try/catch, so the string rejects the +// `octokit.*` call as-is instead of being wrapped in a RequestError: the only way a non-Error reaches the +// bundle's `e instanceof Error ? e.message : String(e)` arms through the real client. Every other request, +// and the matching request's second occurrence, is untouched. Fails closed: without NON_ERROR_REJECT the +// preload throws instead of silently running the test against the real client. +'use strict' + +const target = process.env.NON_ERROR_REJECT +if (!target) + throw new Error('nonErrorRejectionPreload: NON_ERROR_REJECT is unset') + +const realFetch = globalThis.fetch +let armed = true + +globalThis.fetch = async (input, init) => { + const href = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url + const method = (init?.method ?? (typeof input === 'object' && 'method' in input ? input.method : 'GET')).toUpperCase() + const response = await realFetch(input, init) + if (!armed || `${method} ${new URL(href).pathname}` !== target) + return response + armed = false + await response.body?.cancel() + return new Proxy(response, { + get(resp, prop) { + if (prop === 'status') + // a non-Error is the point: the bundle's String(e) arms are what this preload exists to reach + // eslint-disable-next-line no-throw-literal + throw `${target} rejected with a string, not an Error` + const value = Reflect.get(resp, prop, resp) + return typeof value === 'function' ? value.bind(resp) : value + }, + }) +} From f0e2394e8ea81a736758cf6dd1214f0a58eedb8a Mon Sep 17 00:00:00 2001 From: Bob Killen Date: Wed, 21 Oct 2026 09:20:16 -0500 Subject: [PATCH 2/2] test(bundle): cover primitive auth rejection without ignoring it Exercise getErrorDetails through /retitle, asserting the warning and denied write while keeping the action green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Killen --- .../bundle/nonErrorRejectionArms.test.ts | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/__tests__/bundle/nonErrorRejectionArms.test.ts b/__tests__/bundle/nonErrorRejectionArms.test.ts index 95cc9dfb..e9a3fd47 100644 --- a/__tests__/bundle/nonErrorRejectionArms.test.ts +++ b/__tests__/bundle/nonErrorRejectionArms.test.ts @@ -15,8 +15,8 @@ const preload = path.resolve(__dirname, 'nonErrorRejectionPreload.cjs') // write (events.ts:50, handleIssueComment.ts:217, labelSync.ts:107) only run when a client call rejects // with something that is not an Error. Octokit wraps every fetch rejection in a RequestError, so the // preload hands the bundle one Response whose `status` getter throws a plain string, the one read that -// @octokit/request makes outside its try/catch. The run must still fail once with that string in the -// message, after the same request list as the Error-shaped failure, with no unhandled rejection. +// @octokit/request makes outside its try/catch. Each path must retain its Error-shaped failure behavior +// and report the string without an unhandled rejection; collaborator checks warn and deny the write. describe('dist/index.js when an api call rejects with a non-Error value', () => { let gh: FakeGithub const { calls } = helpersFor(() => gh) @@ -104,4 +104,30 @@ describe('dist/index.js when an api call rejects with a non-Error value', () => expect(result.stdout).toContain(', unchanged 0, failed 1') }) }) + + describe('issue_comment: a non-Error collaborator rejection denies the write', () => { + it('/retitle warns with the primitive error details and does not update the issue', async () => { + const collaboratorPath = `${repo}/collaborators/Codertocat` + const target = `GET ${collaboratorPath}` + gh.route('GET', collaboratorPath, { status: 204 }) + gh.route('PATCH', `${repo}/issues/1`, { status: 200, body: {} }) + + const result = await runBundle({ + eventName: 'issue_comment', + payload: comment('/retitle A better title'), + inputs: { ...token, 'prow-commands': '/retitle' }, + apiUrl: gh.url, + env: env(target), + }) + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(result.stdout.split(/\r?\n/).filter(line => line.startsWith('::warning::'))).toEqual([ + `::warning::encountered unexpected error checking collaborator status: status=unknown, message=${target} rejected with a string, not an Error`, + ]) + expect(result.stderr).not.toMatch(/Unhandled|unhandledRejection/) + expect(calls()).toEqual([target]) + expect(gh.requestsMatching('PATCH', /\/issues\/1$/)).toEqual([]) + }) + }) })