From f9d72d3f456b8ca7762e490cf9c3d7e07e747640 Mon Sep 17 00:00:00 2001 From: Salman Chishti Date: Wed, 8 Apr 2026 21:39:12 +0000 Subject: [PATCH] fix: rename binding to createOctokit and harden option merging - Rename context binding from getOctokit to createOctokit to avoid SyntaxError when users write const { getOctokit } = require(...) in their scripts (~10 public workflows affected) - Strip undefined values from user options to prevent clobbering defaults (e.g. GHES baseUrl) - Deep-merge retry options alongside request options - Use nullish coalescing (??) instead of logical OR (||) - Shallow-copy opts to prevent shared reference mutation - Add tests: undefined stripping, retry merge, falsy value preservation, no mutation of defaults - 32 tests passing, lint clean, dist rebuilt --- __test__/async-function.test.ts | 58 +++++++------- __test__/create-configured-getoctokit.test.ts | 80 ++++++++++++++++++- __test__/getoctokit-integration.test.ts | 24 +++--- dist/index.js | 29 ++++--- src/async-function.ts | 2 +- src/create-configured-getoctokit.ts | 33 ++++++-- src/main.ts | 4 +- types/async-function.d.ts | 2 +- 8 files changed, 169 insertions(+), 63 deletions(-) diff --git a/__test__/async-function.test.ts b/__test__/async-function.test.ts index ffa0bed..6400dc7 100644 --- a/__test__/async-function.test.ts +++ b/__test__/async-function.test.ts @@ -8,26 +8,28 @@ describe('callAsyncFunction', () => { expect(result).toEqual('bar') }) - test('passes getOctokit through the script context', async () => { - const getOctokit = jest.fn().mockReturnValue('secondary-client') + test('passes createOctokit through the script context', async () => { + const createOctokit = jest.fn().mockReturnValue('secondary-client') const result = await callAsyncFunction( - {getOctokit} as any, - "return getOctokit('token')" + {createOctokit} as any, + "return createOctokit('token')" ) - expect(getOctokit).toHaveBeenCalledWith('token') + expect(createOctokit).toHaveBeenCalledWith('token') expect(result).toEqual('secondary-client') }) - test('getOctokit creates client independent from github', async () => { + test('createOctokit creates client independent from github', async () => { const github = {rest: {issues: 'primary'}} - const getOctokit = jest.fn().mockReturnValue({rest: {issues: 'secondary'}}) + const createOctokit = jest + .fn() + .mockReturnValue({rest: {issues: 'secondary'}}) const result = await callAsyncFunction( - {github, getOctokit} as any, + {github, createOctokit} as any, ` - const secondary = getOctokit('other-token') + const secondary = createOctokit('other-token') return { primary: github.rest.issues, secondary: secondary.rest.issues, @@ -41,32 +43,32 @@ describe('callAsyncFunction', () => { secondary: 'secondary', different: true }) - expect(getOctokit).toHaveBeenCalledWith('other-token') + expect(createOctokit).toHaveBeenCalledWith('other-token') }) - test('getOctokit passes options through', async () => { - const getOctokit = jest.fn().mockReturnValue('client-with-opts') + test('createOctokit passes options through', async () => { + const createOctokit = jest.fn().mockReturnValue('client-with-opts') const result = await callAsyncFunction( - {getOctokit} as any, - `return getOctokit('my-token', { baseUrl: 'https://ghes.example.com/api/v3' })` + {createOctokit} as any, + `return createOctokit('my-token', { baseUrl: 'https://ghes.example.com/api/v3' })` ) - expect(getOctokit).toHaveBeenCalledWith('my-token', { + expect(createOctokit).toHaveBeenCalledWith('my-token', { baseUrl: 'https://ghes.example.com/api/v3' }) expect(result).toEqual('client-with-opts') }) - test('getOctokit supports plugins', async () => { - const getOctokit = jest.fn().mockReturnValue('client-with-plugins') + test('createOctokit supports plugins', async () => { + const createOctokit = jest.fn().mockReturnValue('client-with-plugins') const result = await callAsyncFunction( - {getOctokit} as any, - `return getOctokit('my-token', { previews: ['v3'] }, 'pluginA', 'pluginB')` + {createOctokit} as any, + `return createOctokit('my-token', { previews: ['v3'] }, 'pluginA', 'pluginB')` ) - expect(getOctokit).toHaveBeenCalledWith( + expect(createOctokit).toHaveBeenCalledWith( 'my-token', {previews: ['v3']}, 'pluginA', @@ -75,24 +77,24 @@ describe('callAsyncFunction', () => { expect(result).toEqual('client-with-plugins') }) - test('multiple getOctokit calls produce independent clients', async () => { - const getOctokit = jest + test('multiple createOctokit calls produce independent clients', async () => { + const createOctokit = jest .fn() .mockReturnValueOnce({id: 'client-a'}) .mockReturnValueOnce({id: 'client-b'}) const result = await callAsyncFunction( - {getOctokit} as any, + {createOctokit} as any, ` - const a = getOctokit('token-a') - const b = getOctokit('token-b') + const a = createOctokit('token-a') + const b = createOctokit('token-b') return { a: a.id, b: b.id, different: a !== b } ` ) - expect(getOctokit).toHaveBeenCalledTimes(2) - expect(getOctokit).toHaveBeenNthCalledWith(1, 'token-a') - expect(getOctokit).toHaveBeenNthCalledWith(2, 'token-b') + expect(createOctokit).toHaveBeenCalledTimes(2) + expect(createOctokit).toHaveBeenNthCalledWith(1, 'token-a') + expect(createOctokit).toHaveBeenNthCalledWith(2, 'token-b') expect(result).toEqual({a: 'client-a', b: 'client-b', different: true}) }) diff --git a/__test__/create-configured-getoctokit.test.ts b/__test__/create-configured-getoctokit.test.ts index 2572bc5..0c6f94f 100644 --- a/__test__/create-configured-getoctokit.test.ts +++ b/__test__/create-configured-getoctokit.test.ts @@ -76,6 +76,23 @@ describe('createConfiguredGetOctokit', () => { ) }) + test('deep-merges retry so partial overrides preserve existing settings', () => { + const raw = makeMockGetOctokit() + const defaults = { + retry: {enabled: true, retries: 3} + } + + const wrapped = createConfiguredGetOctokit(raw as any, defaults) + wrapped('tok' as any, {retry: {retries: 5}} as any) + + expect(raw).toHaveBeenCalledWith( + 'tok', + expect.objectContaining({ + retry: {enabled: true, retries: 5} + }) + ) + }) + test('user can override request.retries explicitly', () => { const raw = makeMockGetOctokit() const defaults = {request: {retries: 3}} @@ -158,15 +175,35 @@ describe('createConfiguredGetOctokit', () => { test('baseUrl: undefined from user does not clobber default', () => { const raw = makeMockGetOctokit() - const defaults = {baseUrl: 'https://api.github.com'} + const defaults = {baseUrl: 'https://ghes.example.com/api/v3'} const wrapped = createConfiguredGetOctokit(raw as any, defaults) wrapped('tok' as any, {baseUrl: undefined} as any) - // undefined spread still overwrites — this documents current behavior. - // The `baseUrl` key is present but value is undefined. const calledOpts = raw.mock.calls[0][1] - expect(calledOpts).toHaveProperty('baseUrl') + expect(calledOpts.baseUrl).toBe('https://ghes.example.com/api/v3') + }) + + test('undefined values in nested request are stripped', () => { + const raw = makeMockGetOctokit() + const defaults = {request: {retries: 3, agent: 'proxy'}} + + const wrapped = createConfiguredGetOctokit(raw as any, defaults) + wrapped('tok' as any, {request: {retries: undefined, timeout: 5000}} as any) + + const calledOpts = raw.mock.calls[0][1] + expect(calledOpts.request).toEqual({retries: 3, agent: 'proxy', timeout: 5000}) + }) + + test('undefined values in nested retry are stripped', () => { + const raw = makeMockGetOctokit() + const defaults = {retry: {enabled: true, retries: 3}} + + const wrapped = createConfiguredGetOctokit(raw as any, defaults) + wrapped('tok' as any, {retry: {enabled: undefined, retries: 5}} as any) + + const calledOpts = raw.mock.calls[0][1] + expect(calledOpts.retry).toEqual({enabled: true, retries: 5}) }) test('each call creates an independent client', () => { @@ -183,4 +220,39 @@ describe('createConfiguredGetOctokit', () => { expect(b).toBe('client-b') expect(raw).toHaveBeenCalledTimes(2) }) + + test('does not mutate defaultOptions between calls', () => { + const raw = makeMockGetOctokit() + const defaults = { + request: {retries: 3}, + retry: {enabled: true} + } + const originalDefaults = JSON.parse(JSON.stringify(defaults)) + + const wrapped = createConfiguredGetOctokit(raw as any, defaults) + wrapped('tok' as any, {request: {timeout: 5000}, retry: {retries: 10}} as any) + wrapped('tok' as any, {request: {timeout: 9000}} as any) + + expect(defaults).toEqual(originalDefaults) + }) + + test('falsy-but-valid values are preserved, only undefined is stripped', () => { + const raw = makeMockGetOctokit() + const defaults = {baseUrl: 'https://ghes.example.com/api/v3'} + + const wrapped = createConfiguredGetOctokit(raw as any, defaults) + wrapped('tok' as any, { + log: null, + retries: 0, + debug: false, + userAgent: '' + } as any) + + const calledOpts = raw.mock.calls[0][1] + expect(calledOpts.log).toBeNull() + expect(calledOpts.retries).toBe(0) + expect(calledOpts.debug).toBe(false) + expect(calledOpts.userAgent).toBe('') + expect(calledOpts.baseUrl).toBe('https://ghes.example.com/api/v3') + }) }) diff --git a/__test__/getoctokit-integration.test.ts b/__test__/getoctokit-integration.test.ts index 8475cf0..df15475 100644 --- a/__test__/getoctokit-integration.test.ts +++ b/__test__/getoctokit-integration.test.ts @@ -3,12 +3,12 @@ import {getOctokit} from '@actions/github' import {callAsyncFunction} from '../src/async-function' -describe('getOctokit integration via callAsyncFunction', () => { +describe('createOctokit integration via callAsyncFunction', () => { test('real getOctokit creates a functional Octokit client in script scope', async () => { const result = await callAsyncFunction( - {getOctokit} as any, + {createOctokit: getOctokit} as any, ` - const client = getOctokit('fake-token-for-test') + const client = createOctokit('fake-token-for-test') return { hasRest: typeof client.rest === 'object', hasGraphql: typeof client.graphql === 'function', @@ -32,9 +32,9 @@ describe('getOctokit integration via callAsyncFunction', () => { const primary = getOctokit('primary-token') const result = await callAsyncFunction( - {github: primary, getOctokit} as any, + {github: primary, createOctokit: getOctokit} as any, ` - const secondary = getOctokit('secondary-token') + const secondary = createOctokit('secondary-token') return { bothHaveRest: typeof github.rest === 'object' && typeof secondary.rest === 'object', areDistinct: github !== secondary @@ -48,11 +48,11 @@ describe('getOctokit integration via callAsyncFunction', () => { }) }) - test('getOctokit accepts options for GHES base URL', async () => { + test('createOctokit accepts options for GHES base URL', async () => { const result = await callAsyncFunction( - {getOctokit} as any, + {createOctokit: getOctokit} as any, ` - const client = getOctokit('fake-token', { + const client = createOctokit('fake-token', { baseUrl: 'https://ghes.example.com/api/v3' }) return typeof client.rest === 'object' @@ -62,12 +62,12 @@ describe('getOctokit integration via callAsyncFunction', () => { expect(result).toBe(true) }) - test('multiple getOctokit calls produce independent clients with different tokens', async () => { + test('multiple createOctokit calls produce independent clients with different tokens', async () => { const result = await callAsyncFunction( - {getOctokit} as any, + {createOctokit: getOctokit} as any, ` - const clientA = getOctokit('token-a') - const clientB = getOctokit('token-b') + const clientA = createOctokit('token-a') + const clientB = createOctokit('token-b') return { aHasRest: typeof clientA.rest === 'object', bHasRest: typeof clientB.rest === 'object', diff --git a/dist/index.js b/dist/index.js index 07a6e85..5274ca7 100644 --- a/dist/index.js +++ b/dist/index.js @@ -36189,13 +36189,22 @@ function callAsyncFunction(args, source) { } ;// CONCATENATED MODULE: ./src/create-configured-getoctokit.ts +/** + * Strip keys whose value is `undefined` so they don't clobber defaults + * during object spread (e.g. `{baseUrl: undefined}` would wipe a GHES URL). + */ +function stripUndefined(obj) { + return Object.fromEntries(Object.entries(obj).filter(([, v]) => v !== undefined)); +} /** * Creates a wrapped getOctokit that inherits default options and plugins. * Secondary clients created via the wrapper get the same retry, logging, * orchestration ID, and retries count as the pre-built `github` client. * - * - `request` is deep-merged so partial overrides (e.g. `{request: {timeout: 5000}}`) - * don't clobber inherited `retries`, proxy agent, or fetch defaults. + * - `request` and `retry` are deep-merged so partial overrides + * (e.g. `{request: {timeout: 5000}}`) don't clobber inherited values. + * - `undefined` values in user options are stripped to prevent accidental + * clobbering of defaults (e.g. GHES `baseUrl`). * - Default plugins (retry, requestLog) are always included; duplicates are skipped. */ function createConfiguredGetOctokit(rawGetOctokit, defaultOptions, @@ -36203,15 +36212,17 @@ function createConfiguredGetOctokit(rawGetOctokit, defaultOptions, ...defaultPlugins) { // eslint-disable-next-line @typescript-eslint/no-explicit-any return ((token, options, ...plugins) => { - var _a, _b; - const userOpts = options || {}; + var _a, _b, _c, _d; + const userOpts = stripUndefined(options !== null && options !== void 0 ? options : {}); const defaultRequest = (_a = defaultOptions.request) !== null && _a !== void 0 ? _a : {}; - const userRequest = (_b = userOpts.request) !== null && _b !== void 0 ? _b : {}; + const userRequest = stripUndefined((_b = userOpts.request) !== null && _b !== void 0 ? _b : {}); + const defaultRetry = (_c = defaultOptions.retry) !== null && _c !== void 0 ? _c : {}; + const userRetry = stripUndefined((_d = userOpts.retry) !== null && _d !== void 0 ? _d : {}); const merged = { ...defaultOptions, ...userOpts, - // Deep-merge `request` to preserve retries, proxy agent, and fetch - request: { ...defaultRequest, ...userRequest } + request: { ...defaultRequest, ...userRequest }, + retry: { ...defaultRetry, ...userRetry } }; // Deduplicate: default plugins first, then user plugins that aren't already present const allPlugins = [...defaultPlugins]; @@ -36322,14 +36333,14 @@ async function main() { const script = core.getInput('script', { required: true }); // Wrap getOctokit so secondary clients inherit retry, logging, // orchestration ID, and the action's retries input. - const configuredGetOctokit = createConfiguredGetOctokit(lib_github.getOctokit, opts, plugin_retry_dist_node.retry, dist_node.requestLog); + const configuredGetOctokit = createConfiguredGetOctokit(lib_github.getOctokit, { ...opts }, plugin_retry_dist_node.retry, dist_node.requestLog); // Using property/value shorthand on `require` (e.g. `{require}`) causes compilation errors. const result = await callAsyncFunction({ require: wrapRequire, __original_require__: require, github, octokit: github, - getOctokit: configuredGetOctokit, + createOctokit: configuredGetOctokit, context: lib_github.context, core: core, exec: exec, diff --git a/src/async-function.ts b/src/async-function.ts index 7f928b4..37f7c91 100644 --- a/src/async-function.ts +++ b/src/async-function.ts @@ -13,7 +13,7 @@ export declare type AsyncFunctionArguments = { core: typeof core github: InstanceType octokit: InstanceType - getOctokit: typeof getOctokit + createOctokit: typeof getOctokit exec: typeof exec glob: typeof glob io: typeof io diff --git a/src/create-configured-getoctokit.ts b/src/create-configured-getoctokit.ts index e1b4c2d..b596414 100644 --- a/src/create-configured-getoctokit.ts +++ b/src/create-configured-getoctokit.ts @@ -1,12 +1,26 @@ import {getOctokit} from '@actions/github' +/** + * Strip keys whose value is `undefined` so they don't clobber defaults + * during object spread (e.g. `{baseUrl: undefined}` would wipe a GHES URL). + */ +function stripUndefined( + obj: Record +): Record { + return Object.fromEntries( + Object.entries(obj).filter(([, v]) => v !== undefined) + ) +} + /** * Creates a wrapped getOctokit that inherits default options and plugins. * Secondary clients created via the wrapper get the same retry, logging, * orchestration ID, and retries count as the pre-built `github` client. * - * - `request` is deep-merged so partial overrides (e.g. `{request: {timeout: 5000}}`) - * don't clobber inherited `retries`, proxy agent, or fetch defaults. + * - `request` and `retry` are deep-merged so partial overrides + * (e.g. `{request: {timeout: 5000}}`) don't clobber inherited values. + * - `undefined` values in user options are stripped to prevent accidental + * clobbering of defaults (e.g. GHES `baseUrl`). * - Default plugins (retry, requestLog) are always included; duplicates are skipped. */ export function createConfiguredGetOctokit( @@ -17,18 +31,25 @@ export function createConfiguredGetOctokit( ): typeof getOctokit { // eslint-disable-next-line @typescript-eslint/no-explicit-any return ((token: string, options?: any, ...plugins: any[]) => { - const userOpts = options || {} + const userOpts = stripUndefined(options ?? {}) const defaultRequest = (defaultOptions.request as Record | undefined) ?? {} - const userRequest = + const userRequest = stripUndefined( (userOpts.request as Record | undefined) ?? {} + ) + + const defaultRetry = + (defaultOptions.retry as Record | undefined) ?? {} + const userRetry = stripUndefined( + (userOpts.retry as Record | undefined) ?? {} + ) const merged = { ...defaultOptions, ...userOpts, - // Deep-merge `request` to preserve retries, proxy agent, and fetch - request: {...defaultRequest, ...userRequest} + request: {...defaultRequest, ...userRequest}, + retry: {...defaultRetry, ...userRetry} } // Deduplicate: default plugins first, then user plugins that aren't already present diff --git a/src/main.ts b/src/main.ts index 6f78b44..f31f74a 100644 --- a/src/main.ts +++ b/src/main.ts @@ -64,7 +64,7 @@ async function main(): Promise { // orchestration ID, and the action's retries input. const configuredGetOctokit = createConfiguredGetOctokit( getOctokit, - opts, + {...opts}, retry, requestLog ) @@ -76,7 +76,7 @@ async function main(): Promise { __original_require__: __non_webpack_require__, github, octokit: github, - getOctokit: configuredGetOctokit, + createOctokit: configuredGetOctokit, context, core, exec, diff --git a/types/async-function.d.ts b/types/async-function.d.ts index 3c2de8e..f42fd07 100644 --- a/types/async-function.d.ts +++ b/types/async-function.d.ts @@ -11,7 +11,7 @@ export declare type AsyncFunctionArguments = { core: typeof core; github: InstanceType; octokit: InstanceType; - getOctokit: typeof getOctokit; + createOctokit: typeof getOctokit; exec: typeof exec; glob: typeof glob; io: typeof io;