From ba24b9751dae77d87b877711cb65661830655e35 Mon Sep 17 00:00:00 2001 From: Dalton Lima Date: Sat, 29 Aug 2026 17:22:34 +0100 Subject: [PATCH 1/5] chore: declare @octokit/request-error and import it as a type --- package-lock.json | 1 + package.json | 1 + src/main.ts | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/package-lock.json b/package-lock.json index 0a8f2588..fad8f740 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,6 +12,7 @@ "@actions/core": "^2.0.0", "@actions/github": "^7.0.0", "@myrotvorets/eslint-config-myrotvorets-ts": "^3.0.0", + "@octokit/request-error": "^5.1.1", "@octokit/webhooks-types": "^7.3.1", "@types/node": "^24.0.0", "@vercel/ncc": "^0.45.0", diff --git a/package.json b/package.json index d5a7dc58..63aab840 100644 --- a/package.json +++ b/package.json @@ -18,6 +18,7 @@ "@actions/core": "^2.0.0", "@actions/github": "^7.0.0", "@myrotvorets/eslint-config-myrotvorets-ts": "^3.0.0", + "@octokit/request-error": "^5.1.1", "@octokit/webhooks-types": "^7.3.1", "@types/node": "^24.0.0", "@vercel/ncc": "^0.45.0", diff --git a/src/main.ts b/src/main.ts index 529e540b..c6c74f4e 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1,6 +1,6 @@ import { getInput, info, setFailed, warning } from '@actions/core'; import { getOctokit } from '@actions/github'; -import { RequestError } from '@octokit/request-error'; +import type { RequestError } from '@octokit/request-error'; import { CommitStatusState, getCommitHash, From 73a47fc687cb0a9afe30b699c474fb12416e0542 Mon Sep 17 00:00:00 2001 From: Dalton Lima Date: Sat, 29 Aug 2026 18:41:14 +0100 Subject: [PATCH 2/5] refactor: read API error details structurally and add tests --- eslint.config.mjs | 12 ++++++++++ package-lock.json | 1 - package.json | 3 +-- src/main.ts | 19 ++-------------- src/utils.ts | 33 ++++++++++++++++++++++++++++ tests/utils.test.js | 53 +++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 101 insertions(+), 20 deletions(-) create mode 100644 tests/utils.test.js diff --git a/eslint.config.mjs b/eslint.config.mjs index 6dff86f0..19c8b80e 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -6,4 +6,16 @@ export default [ ignores: ['dist/**', 'lib/**'], }, ...MyrotvoretsConfig, + { + // Tests are plain CommonJS running under Node's built-in test runner, + // not part of the TypeScript build. + files: ['tests/**/*.js'], + languageOptions: { + sourceType: 'commonjs', + globals: { + module: 'writable', + require: 'readonly', + }, + }, + }, ]; diff --git a/package-lock.json b/package-lock.json index fad8f740..0a8f2588 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,7 +12,6 @@ "@actions/core": "^2.0.0", "@actions/github": "^7.0.0", "@myrotvorets/eslint-config-myrotvorets-ts": "^3.0.0", - "@octokit/request-error": "^5.1.1", "@octokit/webhooks-types": "^7.3.1", "@types/node": "^24.0.0", "@vercel/ncc": "^0.45.0", diff --git a/package.json b/package.json index 63aab840..b2d1af5e 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,7 @@ "lint": "eslint .", "lint:fix": "eslint . --fix", "package": "ncc build --minify", - "test": "exit 0", + "test": "npm run build && node --test tests/*.test.js", "all": "npm run build && npm run lint:fix && npm run package && npm test" }, "keywords": [], @@ -18,7 +18,6 @@ "@actions/core": "^2.0.0", "@actions/github": "^7.0.0", "@myrotvorets/eslint-config-myrotvorets-ts": "^3.0.0", - "@octokit/request-error": "^5.1.1", "@octokit/webhooks-types": "^7.3.1", "@types/node": "^24.0.0", "@vercel/ncc": "^0.45.0", diff --git a/src/main.ts b/src/main.ts index c6c74f4e..6eb573d3 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1,8 +1,8 @@ import { getInput, info, setFailed, warning } from '@actions/core'; import { getOctokit } from '@actions/github'; -import type { RequestError } from '@octokit/request-error'; import { CommitStatusState, + getApiErrorMessage, getCommitHash, isForeignPullRequest, parseRepoName, @@ -21,10 +21,6 @@ interface Inputs { context?: string; } -interface ValidationFailedError { - errors: object | string; -} - function getInputs(): Inputs { const token = getInput('token', { required: true }); const state = validateCommitStatusState(getInput('status', { required: true })); @@ -71,18 +67,7 @@ async function run(): Promise { context: inputs.context, }); } catch (error) { - let foundMoreElaborateError = false; - const reqError = error as RequestError; - if (reqError.response !== undefined) { - const errorData = reqError.response.data as ValidationFailedError; - if (typeof errorData.errors === 'string') { - foundMoreElaborateError = true; - setFailed(errorData.errors); - } - } - if (!foundMoreElaborateError) { - setFailed((error as Error).message); - } + setFailed(getApiErrorMessage(error)); } } diff --git a/src/utils.ts b/src/utils.ts index 8af84684..5f559a61 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -3,6 +3,39 @@ import type { PullRequest } from '@octokit/webhooks-types'; export type CommitStatusState = 'error' | 'failure' | 'pending' | 'success'; +/** + * The shape we read off a failed API call. Only the one field we actually use, + * described locally rather than imported from `@octokit/request-error`. + * + * Every level is optional because none of it is guaranteed: the throw may not be + * a request error at all, and `data` has been observed as `null`. Reading it + * structurally also avoids `instanceof RequestError`, which fails silently when + * more than one copy of that package ends up in the tree. + */ +interface RequestErrorLike { + response?: { + data?: { + errors?: unknown; + }; + }; +} + +/** + * Returns the most specific message available for a failed API call. + * + * The GitHub API sometimes puts a useful explanation in `response.data.errors` + * while `error.message` is only "Validation Failed". Prefer the former when it + * is a string, and fall back to the error's own message otherwise. + */ +export function getApiErrorMessage(error: unknown): string { + const data = (error as RequestErrorLike | null)?.response?.data; + if (typeof data?.errors === 'string') { + return data.errors; + } + + return error instanceof Error ? error.message : String(error); +} + function isPullRequest(): boolean { return context.eventName === 'pull_request'; } diff --git a/tests/utils.test.js b/tests/utils.test.js new file mode 100644 index 00000000..32879187 --- /dev/null +++ b/tests/utils.test.js @@ -0,0 +1,53 @@ +// Plain CommonJS against the compiled output in lib/, which is what `npm run +// build` produces and what ncc then bundles into dist/. Node's built-in test +// runner is used deliberately: this repo ships a bundle to other people's +// runners, so not adding a test framework is worth a little verbosity. + +const { strictEqual } = require('node:assert'); +const { describe, it } = require('node:test'); + +const { getApiErrorMessage } = require('../lib/utils.js'); + +describe('getApiErrorMessage', () => { + it('prefers response.data.errors when it is a string', () => { + const error = Object.assign(new Error('Validation Failed'), { + response: { data: { errors: 'Sha is not a valid commit' } }, + }); + + strictEqual(getApiErrorMessage(error), 'Sha is not a valid commit'); + }); + + it('falls back to the message when errors is an array', () => { + // The shape the API returns most of the time. Deliberately not flattened, + // so this documents that the elaborate path stays dormant for it. + const error = Object.assign(new Error('Validation Failed'), { + response: { data: { errors: [{ resource: 'Status', code: 'custom' }] } }, + }); + + strictEqual(getApiErrorMessage(error), 'Validation Failed'); + }); + + it('falls back to the message when there is no response', () => { + strictEqual(getApiErrorMessage(new Error('Bad credentials')), 'Bad credentials'); + }); + + it('does not throw when data is null', () => { + const error = Object.assign(new Error('Not Found'), { response: { data: null } }); + + strictEqual(getApiErrorMessage(error), 'Not Found'); + }); + + it('does not throw when the response has no data', () => { + const error = Object.assign(new Error('Server Error'), { response: {} }); + + strictEqual(getApiErrorMessage(error), 'Server Error'); + }); + + it('handles a thrown value that is not an Error', () => { + strictEqual(getApiErrorMessage('boom'), 'boom'); + }); + + it('handles a thrown null', () => { + strictEqual(getApiErrorMessage(null), 'null'); + }); +}); From afc626ae015ab859e868607c4a46d0f1e5dd125b Mon Sep 17 00:00:00 2001 From: Dalton Lima Date: Sat, 29 Aug 2026 21:36:18 +0100 Subject: [PATCH 3/5] fix: address review findings on error handling and test config --- eslint.config.mjs | 9 ++++----- package-lock.json | 2 +- package.json | 6 ++++-- src/utils.ts | 17 +++++++++++++---- tests/utils.test.js | 4 ++++ tsconfig.json | 6 +++++- 6 files changed, 31 insertions(+), 13 deletions(-) diff --git a/eslint.config.mjs b/eslint.config.mjs index 19c8b80e..301768b0 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -1,4 +1,5 @@ import MyrotvoretsConfig from '@myrotvorets/eslint-config-myrotvorets-ts'; +import globals from 'globals'; /** @type {import('eslint').Linter.Config[]} */ export default [ @@ -8,14 +9,12 @@ export default [ ...MyrotvoretsConfig, { // Tests are plain CommonJS running under Node's built-in test runner, - // not part of the TypeScript build. + // not part of the TypeScript build. no-undef stays on for them, unlike + // for .ts where tsc covers it, so the globals have to be declared. files: ['tests/**/*.js'], languageOptions: { sourceType: 'commonjs', - globals: { - module: 'writable', - require: 'readonly', - }, + globals: globals.node, }, }, ]; diff --git a/package-lock.json b/package-lock.json index 0a8f2588..a8928080 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15,6 +15,7 @@ "@octokit/webhooks-types": "^7.3.1", "@types/node": "^24.0.0", "@vercel/ncc": "^0.45.0", + "globals": "^17.11.0", "typescript": "^5.3.3" }, "engines": { @@ -2895,7 +2896,6 @@ "integrity": "sha512-Z2I8hM+PbJDXQDq3Icgpzv+mPdwr68iZUU9d5WW4FuXfDUQfkZaZuvjMv42/5crNyw154+9+VWXbYrUgDXbxNw==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=18" }, diff --git a/package.json b/package.json index b2d1af5e..1de824a0 100644 --- a/package.json +++ b/package.json @@ -8,8 +8,9 @@ "lint": "eslint .", "lint:fix": "eslint . --fix", "package": "ncc build --minify", - "test": "npm run build && node --test tests/*.test.js", - "all": "npm run build && npm run lint:fix && npm run package && npm test" + "test": "npm run build && npm run test:unit", + "all": "npm run build && npm run lint:fix && npm run package && npm run test:unit", + "test:unit": "node --test \"tests/*.test.js\"" }, "keywords": [], "author": "Myrotvorets (https://myrotvorets.center/)", @@ -21,6 +22,7 @@ "@octokit/webhooks-types": "^7.3.1", "@types/node": "^24.0.0", "@vercel/ncc": "^0.45.0", + "globals": "^17.11.0", "typescript": "^5.3.3" }, "repository": { diff --git a/src/utils.ts b/src/utils.ts index 5f559a61..f883866f 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -13,6 +13,7 @@ export type CommitStatusState = 'error' | 'failure' | 'pending' | 'success'; * more than one copy of that package ends up in the tree. */ interface RequestErrorLike { + message?: string; response?: { data?: { errors?: unknown; @@ -28,12 +29,20 @@ interface RequestErrorLike { * is a string, and fall back to the error's own message otherwise. */ export function getApiErrorMessage(error: unknown): string { - const data = (error as RequestErrorLike | null)?.response?.data; - if (typeof data?.errors === 'string') { - return data.errors; + const errorLike = error as RequestErrorLike | null; + + if (typeof errorLike?.response?.data?.errors === 'string') { + return errorLike.response.data.errors; + } + + // Read `message` structurally rather than testing `instanceof Error`, for the + // same reason as above, and so an error-like object that carries a message + // still reports it instead of being stringified to "[object Object]". + if (typeof errorLike?.message === 'string') { + return errorLike.message; } - return error instanceof Error ? error.message : String(error); + return String(error); } function isPullRequest(): boolean { diff --git a/tests/utils.test.js b/tests/utils.test.js index 32879187..63c4714d 100644 --- a/tests/utils.test.js +++ b/tests/utils.test.js @@ -43,6 +43,10 @@ describe('getApiErrorMessage', () => { strictEqual(getApiErrorMessage(error), 'Server Error'); }); + it('uses the message from an error-like object', () => { + strictEqual(getApiErrorMessage({ message: 'Bad credentials' }), 'Bad credentials'); + }); + it('handles a thrown value that is not an Error', () => { strictEqual(getApiErrorMessage('boom'), 'boom'); }); diff --git a/tsconfig.json b/tsconfig.json index 8dacd5ff..7b70c001 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -2,6 +2,10 @@ "compilerOptions": { "target": "es2019", "module": "CommonJS", + // Pinned so the emit layout cannot shift. Without it tsc infers rootDir + // from the inputs, and any file added outside src/ would relocate the + // output to lib/src/, breaking the "main" entry that ncc bundles from. + "rootDir": "./src", "noEmitOnError": true, "outDir": "./lib", "sourceMap": false, @@ -18,5 +22,5 @@ ] }, "compileOnSave": true, - "include": ["src/**.ts", "tests/**.ts"] + "include": ["src/**.ts"] } From 97e0b7a07d2dea7ec889690afc2a99f9a7ca5422 Mon Sep 17 00:00:00 2001 From: Dalton Lima Date: Sun, 11 Oct 2026 17:29:25 +0100 Subject: [PATCH 4/5] docs: explain how to test a change against a built dist/ branch --- CONTRIBUTING.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3d347dc7..9d9bc4e3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -44,3 +44,8 @@ npm run all # all of the above Running `npm run all` is a good way to confirm your change compiles and lints. It will modify `dist/` as a side effect. That is fine, just leave those changes out of your commits. + +To try your change in a real workflow, commit the built `dist/` to a separate +branch in your fork and point a workflow at it with +`uses: /set-commit-status-action@`. Open the pull +request from a branch without that commit. From 4d9d4fa16de250a8d808425d56656dd00aa287d7 Mon Sep 17 00:00:00 2001 From: Dalton Lima Date: Sun, 11 Oct 2026 21:47:44 +0100 Subject: [PATCH 5/5] test: cover null response, primitive data and thrown undefined --- tests/utils.test.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/utils.test.js b/tests/utils.test.js index 63c4714d..65cc8f87 100644 --- a/tests/utils.test.js +++ b/tests/utils.test.js @@ -54,4 +54,23 @@ describe('getApiErrorMessage', () => { it('handles a thrown null', () => { strictEqual(getApiErrorMessage(null), 'null'); }); + + it('handles a thrown undefined', () => { + strictEqual(getApiErrorMessage(undefined), 'undefined'); + }); + + it('does not throw when the response is null', () => { + const error = Object.assign(new Error('Not Found'), { response: null }); + + strictEqual(getApiErrorMessage(error), 'Not Found'); + }); + + it('falls back to the message when data is not an object', () => { + // e.g. a proxy returning an HTML error page instead of JSON. + for (const data of ['Bad Gateway', 502]) { + const error = Object.assign(new Error('Bad Gateway'), { response: { data } }); + + strictEqual(getApiErrorMessage(error), 'Bad Gateway'); + } + }); });