From f6446bb30c6eeb6b6e02a736cf5565c30d38bf90 Mon Sep 17 00:00:00 2001 From: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Mon, 26 Oct 2026 19:27:57 -0400 Subject: [PATCH 1/2] test: pin sweep's delayed-reviewer guards, meow's image validation edges, the tilde-fence opener and /approve cancel's review author Ten Stryker survivors of a probe over the dispatcher, cron and command modules change an observable effect yet no test distinguishes them: - sweep.ts lastReadyForReview: any later timeline event moved the review clock; a draft was timed through the timeline before being skipped; the delayed path lost ignore_authors when called as explicit - meow.ts: a foreign host carrying the provider's path prefix was rendered; a url with only a username or only a password passed; the 4096 bound - command.ts fenceOpener: a ~~~ fence with a backtick in its info string - approve.ts cancel: the last APPROVED review of anyone, not the bot's Each new case fails under its mutant and passes on main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> --- .../cronJobTest/sweepDelayGuards.test.ts | 153 ++++++++++++++++++ .../approveCancelAuthor.test.ts | 58 +++++++ .../issueCommentTest/meowImageEdges.test.ts | 105 ++++++++++++ __tests__/utils/commandTildeFence.test.ts | 22 +++ 4 files changed, 338 insertions(+) create mode 100644 __tests__/cronJobTest/sweepDelayGuards.test.ts create mode 100644 __tests__/issueCommentTest/approveCancelAuthor.test.ts create mode 100644 __tests__/issueCommentTest/meowImageEdges.test.ts create mode 100644 __tests__/utils/commandTildeFence.test.ts diff --git a/__tests__/cronJobTest/sweepDelayGuards.test.ts b/__tests__/cronJobTest/sweepDelayGuards.test.ts new file mode 100644 index 00000000..4904e081 --- /dev/null +++ b/__tests__/cronJobTest/sweepDelayGuards.test.ts @@ -0,0 +1,153 @@ +import { Buffer } from 'node:buffer' +import { http } from 'msw' +import { setupServer } from 'msw/node' +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' + +import { sweep } from '../../src/cronJobs/sweep' +import { resetOwnersCaches } from '../../src/utils/owners' +import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' +import pullReqListPulls from '../fixtures/pullReq/pullReqListPulls.json' +import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' +import * as utils from '../testUtils' +import { blobSha } from '../utils/ownersFixtures' + +const server = setupServer() +beforeAll(() => server.listen(utils.failOnUnhandledRequest)) +afterEach(() => server.resetHandlers()) +afterAll(() => server.close()) + +const repo = `${utils.api}/repos/Codertocat/Hello-World` +const now = new Date('2026-09-16T12:00:00Z') +const owners = 'approvers:\n- alice\nreviewers:\n- carol\n' + +function ago(minutes: number): string { + return new Date(now.getTime() - minutes * 60_000).toISOString() +} + +function prowYaml(text: string) { + const file = structuredClone(labelFileContents) + file.content = Buffer.from(text).toString('base64') + return http.get(utils.contentsUrl('.github/prow.yaml'), utils.mockResponse(200, file)) +} + +interface Spec { + number: number + createdMinutesAgo: number + updatedMinutesAgo?: number + draft?: boolean + author?: string +} + +function pull(spec: Spec) { + return { + ...structuredClone(pullReqListPulls[0]), + number: spec.number, + draft: spec.draft ?? false, + labels: [], + created_at: ago(spec.createdMinutesAgo), + updated_at: ago(spec.updatedMinutesAgo ?? spec.createdMinutesAgo), + requested_reviewers: [], + requested_teams: [], + assignees: [], + user: { login: spec.author ?? 'some-author' }, + head: { sha: `sha${spec.number}` }, + base: { ref: 'master', sha: 'basesha' }, + } +} + +function json(body: unknown, status = 200) { + return new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } }) +} + +// Pins three guards of `requestReviewersIfDue` (src/cronJobs/sweep.ts) that +// the other delayed-blunderbuss suites reach but never distinguish: the +// `ready_for_review` filter on the timeline, the draft skip before any +// timeline read, and `explicit: false` carrying `ignore_authors` into the +// delayed path. +describe('sweep with blunderbuss.delay: guards of requestReviewersIfDue', () => { + let context: utils.MockContext + let requested: number[] + let timelineReads: number[] + let timeline: Record> + + function servePulls(pulls: ReturnType[]) { + server.use( + http.get(`${repo}/pulls`, utils.mockResponse(200, pulls)), + http.get(`${repo}/pulls/:number`, ({ params }) => { + const item = pulls.find(pr => pr.number === Number(params.number)) + return json(item ?? { message: 'Not Found' }, item ? 200 : 404) + }), + ) + } + + beforeEach(() => { + utils.setupJobsEnv('sweep') + vi.useFakeTimers({ now, toFake: ['Date'] }) + resetOwnersCaches() + context = new utils.MockContext(pullReqOpenedEvent) + requested = [] + timelineReads = [] + timeline = {} + server.use( + prowYaml('blunderbuss:\n delay: 1h\n'), + ...utils.noOrgOrRepoConfigExcept(), + utils.defaultBranchTree(['OWNERS']), + http.get(`${repo}/branches/master`, utils.mockResponse(200, { commit: { sha: 'basesha' } })), + http.get(`${repo}/git/trees/basesha`, utils.mockResponse(200, { sha: 'basesha', truncated: false, tree: [{ path: 'OWNERS', type: 'blob', sha: blobSha('OWNERS') }] })), + http.get(`${repo}/git/blobs/${blobSha('OWNERS')}`, utils.mockResponse(200, { encoding: 'base64', content: Buffer.from(owners).toString('base64') })), + http.get(`${repo}/pulls/:number/files`, utils.mockResponse(200, [{ filename: 'x.go', status: 'modified' }])), + http.get(`${repo}/pulls/:number/reviews`, utils.mockResponse(200, [])), + http.get(`${repo}/issues/:number/timeline`, ({ params }) => { + timelineReads.push(Number(params.number)) + return json(timeline[Number(params.number)] ?? []) + }), + http.get(`${repo}/issues/:number`, utils.mockResponse(200, { labels: [] })), + http.get(`${repo}/issues/:number/comments`, utils.mockResponse(200, [])), + http.post(`${repo}/issues/:number/comments`, utils.mockResponse(201, {})), + http.post(`${repo}/pulls/:number/requested_reviewers`, ({ params }) => { + requested.push(Number(params.number)) + return json({}, 201) + }), + utils.repoHasLabels(['approved', 'lgtm']), + ) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('times a former draft from its last ready_for_review, not from a later unrelated timeline event', async () => { + // opened long before the due window, marked ready inside it, then labeled after it; + // only the ready_for_review event moves the review clock + servePulls([pull({ number: 1, createdMinutesAgo: 5 * 60, updatedMinutesAgo: 70 })]) + timeline[1] = [ + { event: 'ready_for_review', created_at: ago(70) }, + { event: 'labeled', created_at: ago(10) }, + { event: 'ready_for_review' }, + ] + + await expect(sweep(context)).resolves.toEqual({ candidates: [], merged: [], enqueued: [], failures: [] }) + expect(timelineReads).toEqual([1]) + expect(requested).toEqual([1]) + }) + + it('leaves a draft alone before reading its timeline when ignore_drafts is on', async () => { + servePulls([pull({ number: 1, createdMinutesAgo: 70, draft: true })]) + timeline[1] = [{ event: 'ready_for_review', created_at: ago(70) }] + + await expect(sweep(context)).resolves.toEqual({ candidates: [], merged: [], enqueued: [], failures: [] }) + expect(timelineReads).toEqual([]) + expect(requested).toEqual([]) + }) + + it('leaves a due pull request by an author in blunderbuss.ignore_authors alone', async () => { + server.use(prowYaml('blunderbuss:\n delay: 1h\n ignore_authors:\n - Dave\n')) + servePulls([ + pull({ number: 1, createdMinutesAgo: 70, author: 'dave' }), + pull({ number: 2, createdMinutesAgo: 70, author: 'erin' }), + ]) + + await expect(sweep(context)).resolves.toEqual({ candidates: [], merged: [], enqueued: [], failures: [] }) + expect(requested).toEqual([2]) + }) +}) diff --git a/__tests__/issueCommentTest/approveCancelAuthor.test.ts b/__tests__/issueCommentTest/approveCancelAuthor.test.ts new file mode 100644 index 00000000..76d11580 --- /dev/null +++ b/__tests__/issueCommentTest/approveCancelAuthor.test.ts @@ -0,0 +1,58 @@ +import { Buffer } from 'node:buffer' +import { http } from 'msw' +import { setupServer } from 'msw/node' +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest' + +import { handleIssueComment } from '../../src/issueComment/handleIssueComment' +import issueCommentEventAssignFixture from '../fixtures/issues/assign/issueCommentEventAssign.json' +import pullReqListReviews from '../fixtures/pullReq/pullReqListReviews.json' +import * as utils from '../testUtils' + +const server = setupServer(utils.noMergeQueue) +utils.failTestOnUnhandledRequests(server) +beforeAll(() => server.listen(utils.failOnUnhandledRequest)) +beforeEach(() => server.use(...utils.noOrgOrRepoConfigExcept(), utils.defaultBranchTree())) +afterEach(() => server.resetHandlers()) +afterAll(() => server.close()) + +const repo = `${utils.api}/repos/Codertocat/Hello-World` + +// Pins the author check of `cancel` (src/issueComment/approve.ts): /approve +// cancel dismisses the bot's own latest APPROVED review, not a later approval +// left by a human reviewer. The fixture list carries only the bot's review, +// so approve.test.ts cannot tell the two apart. +describe('/approve cancel with a human approval after the bot review', () => { + let issueCommentEventAssign: typeof issueCommentEventAssignFixture + + beforeEach(() => { + utils.setupActionsEnv('/approve') + issueCommentEventAssign = structuredClone(issueCommentEventAssignFixture) + const owners = Buffer.from('approvers:\n- some-user\n').toString('base64') + server.use( + http.get(`${repo}/contents/OWNERS`, utils.mockResponse(200, { type: 'file', encoding: 'base64', size: 4096, name: 'OWNERS', path: 'OWNERS', content: owners })), + ) + }) + + it('dismisses the bot review and leaves the human approval in place', async () => { + const botReview = structuredClone(pullReqListReviews[0]) + const humanReview = { ...structuredClone(pullReqListReviews[0]), id: 81, user: { ...pullReqListReviews[0].user, login: 'octocat', id: 2, type: 'User' } } + expect(botReview.user.login).toBe('github-actions[bot]') + expect(botReview.state).toBe('APPROVED') + server.use(http.get(`${repo}/pulls/1/reviews`, utils.mockResponse(200, [botReview, humanReview]))) + + const dismissBot = new utils.ObserveRequest() + const dismissHuman = new utils.ObserveRequest() + server.use( + http.put(`${repo}/pulls/1/reviews/80/dismissals`, utils.mockResponse(200, null, dismissBot)), + http.put(`${repo}/pulls/1/reviews/81/dismissals`, utils.mockResponse(200, null, dismissHuman)), + ) + + issueCommentEventAssign.comment.body = '/approve cancel' + issueCommentEventAssign.comment.user.login = 'some-user' + + await handleIssueComment(new utils.MockContext(issueCommentEventAssign)) + + await expect(dismissBot.called()).resolves.toBe('called') + await expect(dismissHuman.notCalled()).resolves.toBe('not called') + }) +}) diff --git a/__tests__/issueCommentTest/meowImageEdges.test.ts b/__tests__/issueCommentTest/meowImageEdges.test.ts new file mode 100644 index 00000000..b2e811e4 --- /dev/null +++ b/__tests__/issueCommentTest/meowImageEdges.test.ts @@ -0,0 +1,105 @@ +import type { MockInstance } from 'vitest' +import dns from 'node:dns' +import * as core from '@actions/core' +import { http, HttpResponse } from 'msw' +import { setupServer } from 'msw/node' +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' + +import { handleIssueComment } from '../../src/issueComment/handleIssueComment' +import { meowConfig } from '../../src/issueComment/meow' +import * as comments from '../../src/utils/comments' +import issueCommentEvent from '../fixtures/issues/issueCommentEvent.json' +import * as utils from '../testUtils' + +const catApi = 'https://api.thecatapi.com/v1/images/search' + +const server = setupServer() +utils.failTestOnUnhandledRequests(server) + +// as in meow.test.ts: a socket the interceptor passed through to the real network is the only way to reach dns +let lookup: MockInstance + +beforeAll(() => server.listen(utils.failOnUnhandledRequest)) +beforeEach(() => { + lookup = vi.spyOn(dns, 'lookup') +}) +afterEach(async () => { + server.resetHandlers() + await new Promise(resolve => setImmediate(resolve)) + await new Promise(resolve => setImmediate(resolve)) + expect(lookup.mock.calls.map(call => call[0])).toEqual([]) +}) +afterAll(() => server.close()) + +function contextFor(body: string) { + issueCommentEvent.comment.body = body + return new utils.MockContext(issueCommentEvent) +} + +// Pins the edges of `parseCatImage` / `isTrustedImageSource` +// (src/issueComment/meow.ts) that meow.test.ts reaches but never +// distinguishes: the s3 fallback must check the host and not only the path +// prefix, a url with only a username or only a password is refused, and the +// length bound is inclusive at 4096. +describe('/meow image validation edges', () => { + let createComment: ReturnType> + + beforeEach(() => { + utils.setupActionsEnv('/meow') + meowConfig.timeoutMs = 10_000 + meowConfig.retryDelayMs = 0 + meowConfig.maxAttempts = 3 + createComment = vi.spyOn(comments, 'createComment').mockResolvedValue() + vi.spyOn(core, 'warning').mockImplementation(() => {}) + }) + + function expectNote() { + expect(createComment).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + 1, + 'The cat API is unavailable right now.', + ) + } + + it('does not trust the provider path prefix on a foreign host', async () => { + server.use(http.get(catApi, () => + HttpResponse.json([{ url: 'https://evil.example/cdn2.thecatapi.com/images/cat.jpg' }]))) + + await handleIssueComment(contextFor('/meow')) + + expect(core.warning).toHaveBeenCalledWith(expect.stringContaining('unexpected host')) + expectNote() + }) + + it.each([ + ['a username only', 'https://user@cdn2.thecatapi.com/images/cat.jpg'], + ['a password only', 'https://:secret@cdn2.thecatapi.com/images/cat.jpg'], + ])('degrades to a note for a url that embeds %s', async (_, url) => { + server.use(http.get(catApi, () => HttpResponse.json([{ url }]))) + + await handleIssueComment(contextFor('/meow')) + + expect(core.warning).toHaveBeenCalledWith(expect.stringContaining('unusable image url')) + expectNote() + }) + + it('renders a url of exactly 4096 characters and refuses one character more', async () => { + const prefix = 'https://cdn2.thecatapi.com/images/' + const atBound = `${prefix}${'a'.repeat(4096 - prefix.length - 4)}.jpg` + expect(atBound).toHaveLength(4096) + server.use(http.get(catApi, () => HttpResponse.json([{ url: atBound }]))) + + await handleIssueComment(contextFor('/meow')) + + expect(createComment).toHaveBeenCalledWith(expect.anything(), expect.anything(), 1, `![cat](<${atBound}>)`) + + createComment.mockClear() + server.use(http.get(catApi, () => HttpResponse.json([{ url: `${atBound}g` }]))) + + await handleIssueComment(contextFor('/meow')) + + expect(core.warning).toHaveBeenCalledWith(expect.stringContaining('excessively long')) + expectNote() + }) +}) diff --git a/__tests__/utils/commandTildeFence.test.ts b/__tests__/utils/commandTildeFence.test.ts new file mode 100644 index 00000000..0eef20a8 --- /dev/null +++ b/__tests__/utils/commandTildeFence.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from 'vitest' + +import { commandLines, hasCommand } from '../../src/utils/command' + +// Pins the backtick restriction of `fenceOpener` (src/utils/command.ts) to +// backtick fences only: CommonMark forbids a backtick in the info string of a +// ``` fence, but a ~~~ fence may carry one and still opens a code block. +describe('tilde fence with a backtick in its info string', () => { + it('opens a code block, so a command inside it is not matched', () => { + const body = '~~~ `code` ~~~\n/approve\n~~~\n/lgtm' + + expect(hasCommand('/approve', body)).toBe(false) + expect(hasCommand('/lgtm', body)).toBe(true) + expect(commandLines(body)).toEqual(['/lgtm']) + }) + + it('is still not an opener when the fence character is a backtick', () => { + const body = '``` `code` ```\n/approve' + + expect(commandLines(body)).toEqual(['``` `code` ```', '/approve']) + }) +}) From d578c8c477e4bc2ada1012fd4b552dca07e632c6 Mon Sep 17 00:00:00 2001 From: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Thu, 29 Oct 2026 08:47:26 -0400 Subject: [PATCH 2/2] test(sweep): start each delayed-reviewer guard case with utils.newRun() instead of the removed resetOwnersCaches() #607 moved the OWNERS caches into the run's RunState and deleted the test-only resetOwnersCaches export; the sibling sweepDelay*.test.ts files were migrated in that change, this one was still open and kept importing it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> --- __tests__/cronJobTest/sweepDelayGuards.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/__tests__/cronJobTest/sweepDelayGuards.test.ts b/__tests__/cronJobTest/sweepDelayGuards.test.ts index 4904e081..0eb1d497 100644 --- a/__tests__/cronJobTest/sweepDelayGuards.test.ts +++ b/__tests__/cronJobTest/sweepDelayGuards.test.ts @@ -4,7 +4,6 @@ import { setupServer } from 'msw/node' import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' import { sweep } from '../../src/cronJobs/sweep' -import { resetOwnersCaches } from '../../src/utils/owners' import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' import pullReqListPulls from '../fixtures/pullReq/pullReqListPulls.json' import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' @@ -83,7 +82,7 @@ describe('sweep with blunderbuss.delay: guards of requestReviewersIfDue', () => beforeEach(() => { utils.setupJobsEnv('sweep') vi.useFakeTimers({ now, toFake: ['Date'] }) - resetOwnersCaches() + utils.newRun() context = new utils.MockContext(pullReqOpenedEvent) requested = [] timelineReads = []