Upgrade NodeJS dependencies & upgrade for LTS - #328
Merged
Conversation
Runtime / engines: - Pin .node-version to v24.18.0 (Node 24 "Krypton" Active LTS). - Set engines.node to "^24.15.0 || >=26.0.0" in root, server, and extension manifests: support both LTS lines while excluding non-LTS Node 25, which ini@7 (via markdownlint-cli -> run-con) rejects and which broke `npm ci` under engine-strict. Type definitions: - Align @types/node to ^24.13.3 (server, extension) and add a root "overrides" entry so the whole tree resolves a single Node 24 types version (transitive deps otherwise pulled @types/node@26 to the root). - Remove now-redundant @types/js-yaml and @types/adm-zip: js-yaml@5 and adm-zip@0.6 ship their own type declarations. Go client: - Upgrade module dependencies (mcp-go 0.57.0, x/sys 0.47.0, x/text 0.40.0, x/term 0.45.0, cast 1.10.0, pflag 1.0.10, httpretty 0.2.0, go-isatty 0.0.24, go-colorful 1.4.0, jsonschema-go 0.4.3, safeexec 1.0.1) and run `go mod tidy`. CI: - Matrix the "build and test" workflows (build-server, build-and-test-extension, and the build-and-test-client integration/CODEQL_PATH jobs) over the pinned .node-version and the latest Node 26.x. "Use Node" workflows (copilot-setup, lint-and-format, query-unit-tests, releases) stay on node-version-file. Build artifacts: - Regenerate the committed server/dist bundle so it reflects the upgraded bundled deps (fast-uri 3.1.4 URI-authority hardening), keeping the CI "no uncommitted changes" gate green. Bundle is byte-deterministic.
Contributor
Dependency ReviewThe following issues were found:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request updates the repository’s Node.js compatibility to align with LTS-focused versions (Node 24 and 26), upgrades a set of Node/Go dependencies, and expands CI coverage to run workflows across both Node versions.
Changes:
- Update Node.js engine constraints across packages and set
.node-versionto Node 24.x. - Upgrade Node and Go dependencies (including adding root
overridesandallowScriptshardening). - Add Node 24/26 matrix testing to key GitHub Actions workflows and surface the matrix value in job names/summaries.
Show a summary per file
| File | Description |
|---|---|
server/package.json |
Updates Node engine range and upgrades server dependencies/devDependencies. |
package.json |
Updates root Node engine range; adds overrides for @types/node and allowScripts. |
package-lock.json |
Refreshes lockfile to reflect upgraded dependencies and new constraints. |
extensions/vscode/package.json |
Updates Node engine range and upgrades extension devDependencies. |
client/go.mod |
Bumps Go module dependencies to newer versions. |
client/go.sum |
Updates checksums for the upgraded Go module set. |
.node-version |
Sets the repo default Node version to v24.18.0. |
.github/workflows/build-server.yml |
Adds Node matrix (24 via .node-version + 26) and includes Node in job naming. |
.github/workflows/build-and-test-extension.yml |
Adds Node matrix (24 via .node-version + 26) and includes Node in job naming. |
.github/workflows/build-and-test-client.yml |
Adds Node matrix (24 via .node-version + 26) for test jobs and updates summaries to include Node. |
Review details
Comments suppressed due to low confidence (1)
.github/workflows/build-and-test-client.yml:223
- Same issue as the other setup-node step: providing
node-version(even as an empty string) can overridenode-version-file, so.node-versionmay be ignored and the wrong Node version used. Use two guarded setup-node steps instead of passing an emptynode-version.
- name: Setup Node.js
uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
with:
cache: 'npm'
node-version: ${{ matrix.node != '.node-version' && matrix.node || '' }}
node-version-file: '.node-version'
- Files reviewed: 8/12 changed files
- Comments generated: 3
- Review effort level: Low
actions/setup-node prioritizes node-version over node-version-file. When the matrix value is '.node-version', the previous expression set node-version to an empty string, which could cause the action to ignore node-version-file entirely. Split into two steps with if: guards so node-version is never set to an empty value. Fixes all instances across build-server.yml, build-and-test-extension.yml, and build-and-test-client.yml (integration-tests and codeql-path-tests jobs). Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
package.json:37
- The PR description says it “Introduced an
allowScriptsfield in the rootpackage.jsonto allow specific postinstall scripts”, but the current rootpackage.jsonhas noallowScriptsentry. If you intended to adopt npm’s script allowlist, please addallowScripts(ideally pinned to the versions inpackage-lock.json) so installs/rebuilds don’t unexpectedly skip required native/binary setup scripts under newer npm behavior.
"overrides": {
"@types/node": "^24.13.3"
},
"scripts": {
CHANGELOG.md:23
- Several user-facing docs still state a minimum Node.js version of v25.6.0 (e.g. README.md, server/README.md, client/README.md, docs/getting-started.md, docs/public.md, extensions/vscode/README.md). With this PR changing supported Node to
^24.15.0 || >=26.0.0and.node-versionto v24.18.0, those docs should be updated too; otherwise users will keep being told to install a now-unsupported Node 25.
- **Realigned the supported Node.js versions with the Node.js LTS release line.** The `engines.node` constraint in the root, `server`, and `extensions/vscode` `package.json` files moved from `>=25.6.0` to `^24.15.0 || >=26.0.0`, and `.node-version` moved from `v25.6.0` to `v24.18.0`. Node.js 25 is no longer supported: it is a non-LTS ("Current") line, and transitive dependencies such as `ini` have already dropped support for it. ([#328](https://github.com/advanced-security/codeql-development-mcp-server/pull/328))
- **Added Node.js matrix testing to the build workflows.** `build-server.yml`, `build-and-test-client.yml`, and `build-and-test-extension.yml` now run against both Node 24 (via `.node-version`) and Node 26, with the Node version surfaced in job names and `$GITHUB_STEP_SUMMARY` output. Each matrix entry uses two `if:`-guarded `actions/setup-node` steps so `node-version` is never passed as an empty string (which would otherwise take precedence over `node-version-file` and silently select the runner's default Node). ([#328](https://github.com/advanced-security/codeql-development-mcp-server/pull/328))
- **Pinned `@types/node` repo-wide via a root `overrides` entry** (`^24.13.3`) so every workspace type-checks against a single Node typings version that matches the supported Node 24 LTS baseline. ([#328](https://github.com/advanced-security/codeql-development-mcp-server/pull/328))
- Files reviewed: 9/13 changed files
- Comments generated: 0 new
- Review effort level: Low
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (3)
.github/workflows/build-and-test-client.yml:350
- This row reports
matrix.node, which is.node-versionfor one matrix entry, so the summary table doesn't actually show the Node version used. Usenode -vto capture the real runtime version.
echo "| Node | ${{ matrix.node }} |" >> $GITHUB_STEP_SUMMARY
.github/workflows/build-and-test-client.yml:208
- The summary prints
Node ${{ matrix.node }}, but for the.node-versionmatrix entry this renders asNode .node-versionrather than the actual runtime version used. Prefer reporting the real Node version (e.g.node -v) so CI summaries are accurate.
This issue also appears on line 350 of the same file.
echo "## Integration Tests (${{ matrix.os }}, ${{ matrix.mcp-mode }}, Node ${{ matrix.node }})" >> $GITHUB_STEP_SUMMARY
echo "✅ MCP server integration tests passed on ${{ matrix.os }} with ${{ matrix.mcp-mode }} transport (Node ${{ matrix.node }})" >> $GITHUB_STEP_SUMMARY
package.json:36
- PR description mentions introducing a root-level
allowScriptsfield inpackage.json, but the file currently only addsoverridesand does not defineallowScriptsanywhere. Either add the intendedallowScriptsallowlist here or update the PR description to avoid documenting a change that isn't present.
"overrides": {
"@types/node": "^24.13.3"
},
- Files reviewed: 9/13 changed files
- Comments generated: 0 new
- Review effort level: Low
data-douser
marked this pull request as ready for review
July 29, 2026 02:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request:
ini) having already dropped such support for Node.js 25;Node.js Version Updates:
.node-versionfromv25.6.0tov24.18.0to ensure compatibility with the updated engines and CI matrix.engines.nodefield inpackage.json,server/package.json, andextensions/vscode/package.jsonto support Node.js^24.15.0or>=26.0.0, replacing the previous requirement for Node.js 25+. [1] [2] [3]CI/CD Workflow Improvements:
build-server.yml,build-and-test-client.yml,build-and-test-extension.yml) to use a matrix strategy for Node.js versions (.node-versionand26), ensuring tests run on both Node 24 and 26. Workflow job names and summaries now include the Node.js version. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Dependency Upgrades:
client/go.mod,server/package.json,extensions/vscode/package.json, and the rootpackage.jsonfor improved compatibility, security, and bug fixes. Notable upgrades includeadm-zip,js-yaml,@types/node,eslint,prettier, and several others. [1] [2] [3] [4]Tooling and Configuration:
overridessection inpackage.jsonto pin@types/nodeto^24.13.3for consistent type checking.allowScriptsfield in the rootpackage.jsonto allow specific postinstall scripts, improving security and reproducibility.These changes collectively improve compatibility with current and future Node.js versions and enhance the reliability of the CI pipeline.