From 7740e05361f1563f61356c3f0e5dff61438b93a6 Mon Sep 17 00:00:00 2001 From: Illia Izotov Date: Thu, 23 Apr 2026 12:41:24 -0600 Subject: [PATCH 1/3] NP-790: Harden bundle smoke test and document commitOpts coupling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses ts-node-reviewer findings on the previous commit (4de84b0): * `runChildAndCapture` now listens for `error` on the child so a failed spawn (e.g. ENOENT on the node binary) folds the error into the captured stderr and resolves immediately instead of hanging until the SIGKILL timer fires. A `settled` guard protects against the close/error race. * The mock HTTP server's `listen` promise now attaches a one-shot `error` handler so a failed bind (EADDRINUSE on a constrained CI host, etc.) rejects with a useful message instead of letting the test silently wedge until Vitest's test timeout. * `src/action.ts` gains an explicit comment next to the preset pre- resolution explaining that upstream `load-changelog-config.js` never merges a plugin-supplied `commits` field, so `commitOpts` always comes from angular when no `preset` / `config` is passed. Harmless today (angular and conventionalcommits both default to `{ ignore: undefined, merges: false }`) but worth flagging for future upstream drift. No behavioural change to the shipping bundle — `esbuild` strips comments and the smoke test's runtime changes only affect error paths that previously caused hangs, not the happy path. All 37 tests across 5 files still pass; lint, prettier, and typecheck clean. Made-with: Cursor --- src/action.ts | 8 +++++++ tests/bundle.smoke.test.ts | 49 ++++++++++++++++++++++++++++++-------- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/action.ts b/src/action.ts index 6ee3fadfa..ceba0365d 100644 --- a/src/action.ts +++ b/src/action.ts @@ -216,6 +216,14 @@ export default async function main() { // `preset: 'conventionalcommits'` would trigger a dynamic // `import-from-esm` lookup that fails on the Actions runner // where no `node_modules` directory exists. + // + // Note: upstream `load-changelog-config.js` only reads + // `loadedConfig.commits` (never merging a plugin-supplied `commits` + // field), so when no `preset` / `config` is passed `commitOpts` + // always comes from the angular default preset. That is harmless + // today — angular and conventionalcommits both default to + // `{ ignore: undefined, merges: false }` — but is worth knowing if + // those defaults ever diverge upstream. const resolvedPreset = conventionalCommitsPreset({ types: mergeWithDefaultChangelogRules(mappedReleaseRules), }); diff --git a/tests/bundle.smoke.test.ts b/tests/bundle.smoke.test.ts index d8903b34f..d2b6dc679 100644 --- a/tests/bundle.smoke.test.ts +++ b/tests/bundle.smoke.test.ts @@ -54,7 +54,8 @@ interface ChildResult { * Drain stdout/stderr asynchronously while the child runs so large * bursts of startup output can't deadlock the parent. Always resolves * (never rejects) so the caller can assert on the captured streams even - * if the child crashed, exited non-zero, or had to be SIGKILLed. + * if the child crashed, exited non-zero, had to be SIGKILLed, or failed + * to spawn in the first place. */ function runChildAndCapture( child: ChildProcess, @@ -63,21 +64,39 @@ function runChildAndCapture( return new Promise((resolve) => { const stdoutChunks: Buffer[] = []; const stderrChunks: Buffer[] = []; + let settled = false; + + const finish = (overrides: Partial): void => { + if (settled) return; + settled = true; + clearTimeout(timer); + resolve({ + stdout: Buffer.concat(stdoutChunks).toString('utf8'), + stderr: Buffer.concat(stderrChunks).toString('utf8'), + status: null, + signal: null, + ...overrides, + }); + }; + child.stdout?.on('data', (chunk: Buffer) => stdoutChunks.push(chunk)); child.stderr?.on('data', (chunk: Buffer) => stderrChunks.push(chunk)); + // If spawn itself fails (e.g. ENOENT for the node binary), Node emits + // `error` and never a `close`. Fold the error into stderr so the + // caller still sees a useful message instead of hanging until the + // SIGKILL timer fires. + child.on('error', (err) => { + stderrChunks.push(Buffer.from(`\n${String(err)}\n`)); + finish({}); + }); + const timer = setTimeout(() => { child.kill('SIGKILL'); }, timeoutMs); child.on('close', (status, signal) => { - clearTimeout(timer); - resolve({ - stdout: Buffer.concat(stdoutChunks).toString('utf8'), - stderr: Buffer.concat(stderrChunks).toString('utf8'), - status, - signal, - }); + finish({ status, signal }); }); }); } @@ -209,8 +228,18 @@ describe('bundled action artefact', () => { }; const server = createServer(handleRequest); - await new Promise((resolve) => { - server.listen(0, '127.0.0.1', resolve); + await new Promise((resolve, reject) => { + // Without an `error` listener, a failed `listen` (e.g. EADDRINUSE + // on a constrained CI host) would never reject and the test would + // silently hang until vitest's test timeout. + const onError = (err: Error): void => { + reject(err); + }; + server.once('error', onError); + server.listen(0, '127.0.0.1', () => { + server.off('error', onError); + resolve(); + }); }); try { From 75da0bb75d1239d351ce1923e10c6f12f5551e8d Mon Sep 17 00:00:00 2001 From: Illia Izotov Date: Thu, 23 Apr 2026 15:04:51 -0600 Subject: [PATCH 2/3] NP-790: CI guard against committed-bundle / source drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GitHub Actions runner executes whatever `lib/main.js` is checked into the repo, not the bundle CI rebuilds during `npm run build`. Without a drift check, a PR could change `src/` and rebuild locally but commit only the `src/` hunk — CI would stay green while every consumer of `StackAdapt/github-tag-action@vN` keeps running the old bundle. Add a `git diff --exit-code -- lib/` step immediately after `npm run build` that fails the job if the freshly rebuilt bundle diverges from the committed artefact, with a `::error::` line telling the author to run `npm run build` and commit the result. Verified locally: running the guard with a clean tree passes; after a real source change + rebuild it prints the diff and exits non-zero. (Surfaced by the reviewer's final pass on PR #5 as the highest-value process gap remaining.) Made-with: Cursor --- .github/workflows/test.yml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c986df911..4c22f5f87 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -22,4 +22,17 @@ jobs: # surfaces missing-file / bundler-config regressions before the # (slower) full Vitest suite runs. - run: npm run build + # Guard against committed-bundle drift: GitHub's Actions runner + # executes the `lib/main.js` that is *checked in*, not whatever CI + # rebuilds. Without this check a PR could change `src/` + rebuild + # locally, commit only the `src/` change, and still pass CI while + # shipping a stale bundle to every consumer. `git diff --exit-code` + # fails the job if the freshly-built bundle diverges from the + # committed one, forcing the author to commit the rebuild. + - name: Ensure lib/main.js is up to date with src/ + run: | + if ! git diff --exit-code -- lib/; then + echo "::error::\`lib/main.js\` is stale. Run \`npm run build\` locally and commit the updated bundle." + exit 1 + fi - run: npm test From 21a1872cf2810ce4a05df3773b50c7263b66c9d7 Mon Sep 17 00:00:00 2001 From: Illia Izotov Date: Thu, 23 Apr 2026 15:33:31 -0600 Subject: [PATCH 3/3] NP-790: Harden drift guard to also catch untracked lib/ outputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer pointed out that `git diff --exit-code -- lib/` only compares tracked paths to the index, so if the bundler ever starts emitting additional artefacts under `lib/` (sourcemaps, split chunks, a `lib/main.js.LICENSE.txt`, etc.) a future `src/` change could introduce them without CI noticing — the runner would then run the committed `lib/main.js` but ship missing sibling files. Not a concern today (esbuild output is a single `lib/main.js` by design) but the guard should cover the class, not just the current instance. Added a complementary `git ls-files --others --exclude-standard -- lib/` check that fails the job if any new untracked files show up under `lib/` after `npm run build`, with an `::error::` line that tells the author to either commit the new artefact or add it to `.gitignore`. Also: * Renamed the step from "Ensure lib/main.js is up to date with src/" to "Ensure lib/ is up to date with src/" now that it covers the whole directory. * Added a non-fatal `git diff --stat -- lib/ || true` ahead of the strict check so CI logs get a one-line summary of what drifted before the full patch gets printed by the failing `diff`. Verified locally: clean tree passes both checks; modifying a tracked `lib/` file is caught by check 1; adding an untracked `lib/foo.map` is caught by check 2. Made-with: Cursor --- .github/workflows/test.yml | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 4c22f5f87..f81d7f6d7 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -23,16 +23,30 @@ jobs: # (slower) full Vitest suite runs. - run: npm run build # Guard against committed-bundle drift: GitHub's Actions runner - # executes the `lib/main.js` that is *checked in*, not whatever CI + # executes whatever is *checked in* under `lib/`, not whatever CI # rebuilds. Without this check a PR could change `src/` + rebuild # locally, commit only the `src/` change, and still pass CI while - # shipping a stale bundle to every consumer. `git diff --exit-code` - # fails the job if the freshly-built bundle diverges from the - # committed one, forcing the author to commit the rebuild. - - name: Ensure lib/main.js is up to date with src/ + # shipping a stale bundle to every consumer. + # + # Two complementary checks: + # 1. `git diff --exit-code -- lib/` catches modifications to + # tracked files (today: `lib/main.js`). + # 2. `git ls-files --others --exclude-standard -- lib/` catches + # brand-new untracked files the bundler might start emitting + # (e.g. sourcemaps, additional chunks) that a future `src/` + # change could introduce without the author noticing. + - name: Ensure lib/ is up to date with src/ run: | + git diff --stat -- lib/ || true if ! git diff --exit-code -- lib/; then - echo "::error::\`lib/main.js\` is stale. Run \`npm run build\` locally and commit the updated bundle." + echo "::error::Files under \`lib/\` are stale. Run \`npm run build\` locally and commit the updated bundle." + exit 1 + fi + untracked=$(git ls-files --others --exclude-standard -- lib/) + if [ -n "$untracked" ]; then + echo "::error::The build produced new files under \`lib/\` that are not committed:" + printf ' %s\n' $untracked + echo "::error::Commit the new artefact(s) or add them to \`.gitignore\`." exit 1 fi - run: npm test