Read API error details without an undeclared import, fix a null crash, add tests

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: <your-user>/set-commit-status-action@<that-branch>`. Open the pull
+request from a branch without that commit.

eslint.config.mjs

@@ -1,9 +1,20 @@
 import MyrotvoretsConfig from '@myrotvorets/eslint-config-myrotvorets-ts';
+import globals from 'globals';
 
 /** @type {import('eslint').Linter.Config[]} */
 export default [
     {
         ignores: ['dist/**', 'lib/**'],
     },
     ...MyrotvoretsConfig,
+    {
+        // Tests are plain CommonJS running under Node's built-in test runner,
+        // 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: globals.node,
+        },
+    },
 ];

package.json

@@ -8,8 +8,9 @@
     "lint": "eslint .",
     "lint:fix": "eslint . --fix",
     "package": "ncc build --minify",
-    "test": "exit 0",
-    "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 <support@myrotvorets.center> (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": {

src/main.ts

@@ -1,8 +1,8 @@
 import { getInput, info, setFailed, warning } from '@actions/core';
 import { getOctokit } from '@actions/github';
-import { 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<void> {
             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));
     }
 }
 

src/utils.ts

@@ -3,6 +3,48 @@ 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 {
+    message?: string;
+    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 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 String(error);
+}
+
 function isPullRequest(): boolean {
     return context.eventName === 'pull_request';
 }

tests/utils.test.js

@@ -0,0 +1,76 @@
+// 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('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');
+    });
+
+    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 ['<html>Bad Gateway</html>', 502]) {
+            const error = Object.assign(new Error('Bad Gateway'), { response: { data } });
+
+            strictEqual(getApiErrorMessage(error), 'Bad Gateway');
+        }
+    });
+});

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"]
 }