Skip to content

NP-790: Fix bundled action runtime preset loading + add e2e smoke test#5

Open
illiaizotov-dev wants to merge 3 commits intomasterfrom
illia/NP-790
Open

NP-790: Fix bundled action runtime preset loading + add e2e smoke test#5
illiaizotov-dev wants to merge 3 commits intomasterfrom
illia/NP-790

Conversation

@illiaizotov-dev
Copy link
Copy Markdown
Member

Summary

Fixes a runtime crash consumers hit when using StackAdapt/github-tag-action@v7:

Error: TypeError [ERR_INVALID_ARG_TYPE]: The "paths[0]" argument must be of type string. Received undefined
    at resolve (node:path:1257:7)
    at resolveToFileURL (lib/main.js:41844:25)
    at importFrom (lib/main.js:41888:30)
    at load_changelog_config_default (lib/main.js:43164:99)
    at async generateNotes (lib/main.js:43239:50)

Root cause

@semantic-release/release-notes-generator/lib/load-changelog-config.js resolves the preset option at runtime via import-from-esm. That helper walks the filesystem looking for conventional-changelog-<preset> in node_modules — a search that fundamentally cannot succeed in the bundled Action runtime because GitHub Actions does not run npm install on the action dir, and our esbuild bundle is a single self-contained file. The upstream silent-then-fallthrough code path ends up calling path.resolve(undefined, ...) when the context's cwd is unset, which is how the confusing ERR_INVALID_ARG_TYPE surfaces.

Fix

  • Statically import conventional-changelog-conventionalcommits in src/action.ts so esbuild inlines it.
  • Call the preset factory ourselves and hand the resolved parser / writer options to generateNotes as parserOpts / writerOpts. With no preset / config supplied, load-changelog-config.js takes its safe conventional-changelog-angular fallback (also statically imported upstream and therefore bundled) and shallow-merges our options on top — conventionalcommits' keys win.
  • Pass cwd: process.cwd() on the context as belt-and-suspenders.

Integration test that would have caught this

tests/bundle.smoke.test.ts gains a third test case that runs the shipping bundle end-to-end:

  • Spins up a local node:http mock server for /repos/foo/bar/tags and /repos/foo/bar/compare/....
  • Copies lib/main.js into os.tmpdir() (no sibling node_modules anywhere on the resolver walk).
  • Spawns it with GITHUB_API_URL pointed at the mock and INPUT_DRY_RUN=true so it exits cleanly after changelog generation.
  • Asserts no ERR_INVALID_ARG_TYPE / paths\[0\] / module-resolution errors, and positively asserts on Dry run: not performing tag action. to confirm execution walked past generateNotes.

Uses async spawn + Promise-based capture (not spawnSync) because the bundle emits enough startup stdio to deadlock spawnSync on macOS with captured pipes.

Reviewer-hardening follow-up

  • runChildAndCapture listens for child error so a failed spawn surfaces the error through captured stderr rather than hanging until the SIGKILL timer.
  • Mock HTTP server's listen promise attaches a one-shot error handler so bind failures (EADDRINUSE on CI) reject cleanly instead of wedging the test until Vitest's timeout.
  • Documented in src/action.ts that commitOpts always comes from angular when no preset / config is passed — harmless today (defaults match) but flagged for future upstream drift.

Test plan

  • npm run build — bundle produces a 1.7 MB self-contained lib/main.js
  • npm test — 37 tests pass across 5 files
  • npm run lint — clean
  • npm run check — Prettier clean
  • npm run typecheck — clean
  • Against the previously-shipped (buggy) bundle the new smoke test reproduces the user-reported error line-for-line and fails; after the fix it passes
  • Green CI on this PR
  • After merge: move the floating v7 tag to the merged commit so consumers pick up the fix

Made with Cursor

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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant