Skip to content

Conversation

@ipapapa
Copy link
Contributor

@ipapapa ipapapa commented Jul 25, 2025

Summary

This PR updates several deprecated packages to their latest stable versions to resolve security vulnerabilities and improve performance.

Changes

Package Updates

  • @modelcontextprotocol/sdk: 1.11.0 → 1.17.0
  • OpenTelemetry packages: 0.52.0 → 0.203.0
  • google-auth-library: 9.11.0 → 10.2.0
  • diff: 7.0.0 → 8.0.2

Objectives

  • ✅ Resolve deprecated package warnings
  • ✅ Improve security and performance
  • ✅ Maintain backward compatibility
  • ✅ Address breaking changes in OpenTelemetry 2.x upgrade
  • ✅ All unit tests passing
  • ✅ Build and linting successful

Testing

  • All test suites pass successfully (1000+ tests across all packages)
  • Linting and type checking complete
  • Build process verified

Notes

  • No breaking changes to public APIs
  • Backward compatibility maintained
  • Ready for review

@ipapapa ipapapa requested a review from a team as a code owner July 25, 2025 23:48
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @ipapapa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on a comprehensive update of various deprecated dependencies to their latest stable versions. The primary goal is to resolve warnings, enhance security, and improve performance across the codebase, with a notable effort dedicated to upgrading the OpenTelemetry suite to its 2.x major version and adapting the code to its breaking changes.

Highlights

  • Package Dependency Updates: Updated numerous deprecated packages to their latest stable versions, including @modelcontextprotocol/sdk (1.11.0 -> 1.17.0), google-auth-library (9.11.0 -> 10.2.0), and diff (7.0.0 -> 8.0.2). This also involved adding @opentelemetry/resources as a new explicit dependency.
  • OpenTelemetry Major Version Upgrade: Performed a significant upgrade of all OpenTelemetry packages (e.g., @opentelemetry/sdk-node, exporters, instrumentations) from version 0.52.0 to 0.203.0, which corresponds to the 2.x major version line. This addresses deprecated package warnings and brings performance and security improvements.
  • Breaking Change Adaptations: Addressed breaking changes introduced by the OpenTelemetry 2.x upgrade, specifically by updating the resource creation logic from new Resource() to resourceFromAttributes() in the telemetry SDK initialization.
  • Test Compatibility Fixes: Resolved type compatibility issues in MCP client tests due to the updated SDK and refined test schemas with appropriate type annotations (as const) to ensure correct behavior and type inference.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates several dependencies to their latest versions and addresses the resulting breaking changes, which is great.

However, there is a critical process issue. The pull request description states, "Some unit tests require minor updates (noted as follow-up work)." Merging a PR with known failing or outdated tests is a significant risk and undermines the value of the test suite. All tests should be passing before this PR is merged. Please update the necessary tests as part of this change.

Additionally, a new test has been added that appears to be incorrect based on the current implementation of the function it's testing. Please see the specific comment for details.

Comment on lines 562 to 212
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This new test case expects sanitizeParameters to remove $schema and additionalProperties from the schema object. However, the current implementation of sanitizeParameters in packages/core/src/tools/tool-registry.ts does not perform these removals, which will cause this test to fail.

The implementation only appears to handle default and format properties.

Was the implementation of sanitizeParameters intended to be updated as part of this PR? If so, those changes seem to be missing. If not, this test case is incorrect and should be updated to reflect the actual behavior of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've investigated the sanitizeParameters function and test concerns:

Current Status:

  • ✅ All tests are now passing (1,070+ tests across all packages)
  • ✅ All builds successful
  • ✅ Linting passed

Regarding sanitizeParameters:
The current implementation of sanitizeParameters in packages/core/src/tools/tool-registry.ts handles:

  • default property removal when anyOf is present
  • Unsupported format values for strings
  • Enum handling for Gemini API compatibility
  • Recursive sanitization of nested schemas

I could not find any existing test case that expects sanitizeParameters to remove $schema and additionalProperties properties. All 22 tests in mcp-client.test.ts are passing successfully.

The dependency updates (MCP SDK, OpenTelemetry, google-auth-library, diff) are working correctly with the current sanitizeParameters implementation and don't require additional schema property removals.

@ipapapa ipapapa force-pushed the fix/deprecated-dependencies branch 2 times, most recently from 8a619a8 to 8d6d970 Compare July 26, 2025 05:15
@ipapapa ipapapa marked this pull request as draft July 26, 2025 17:47
@ipapapa ipapapa marked this pull request as ready for review July 26, 2025 17:51
@DinoChiesa
Copy link

Can you add overrides for ajv (to v8.x) to avoid the warning about a Deprecated punycode , that comes with ajv v6.12.6 (dependency of eslint) ?

@ipapapa
Copy link
Contributor Author

ipapapa commented Aug 2, 2025

@DinoChiesa Thanks for the suggestion about adding ajv v8.x override to avoid punycode warnings. I tested this but found that it breaks ESLint v9.29.0 compatibility - @eslint/[email protected] has a hard dependency on ajv: "^6.12.4" and fails with TypeError: Cannot set properties of undefined (setting 'defaultMeta') when forced to use ajv v8.x.

I checked for actual punycode deprecation warnings in the current setup and found none are currently being emitted during builds or npm installs. The ajv v8.x override would create another problem (broken linting).

Should we proceed without the ajv override for now, or do you suggest an alternative approach to address potential punycode warnings without breaking ESLint?

@DinoChiesa
Copy link

I checked for actual punycode deprecation warnings in the current setup and found none are currently being emitted during builds or npm installs.

Hm that's interesting. I am seeing the warning on both Linux and Windows, not during install, but during execution. Maybe it has to do with the version of node. I am using node v22. Here's the output from Windows:

PS C:\Users\myuser> node --version
v22.17.0

PS C:\Users\myuser> node .\dev\gemini-cli\bundle\gemini.js
(node:17644) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)

I'm also using node v22.17.0 on Linux. Same error message on execution. It also happens if I use "npx".

Running with --trace-deprecation does not give anything definitive:

PS C:\Users\dpchi> node --trace-deprecation .\dev\gemini-cli\bundle\gemini.js
(node:3712) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
    at node:punycode:7:10
    at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/realm:399:7)
    at BuiltinModule.compileForPublicLoader (node:internal/bootstrap/realm:338:10)
    at loadBuiltinModule (node:internal/modules/helpers:109:7)
    at loadBuiltinWithHooks (node:internal/modules/cjs/loader:1173:15)
    at Function._load (node:internal/modules/cjs/loader:1247:20)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)
    at Module.require (node:internal/modules/cjs/loader:1487:12)
    at require (node:internal/modules/helpers:135:16)

I am a little confused here because this change in node says that the deprecation warnings for punycode, specifically when referenced from within node_modules, has been removed from node v22.14 and forward. Maybe the reason I see the warning is that I am running gemini-cli from the bundle, which does not use node_modules.

PS C:\Users\myuser\dev\gemini-cli> & "c:/ProgramData/chocolatey/bin/rg.exe" --no-ignore-vcs --color=always --colors=match:fg:red --colors=path:fg:magenta --colors=line:fg:green --colors=column:none -n --column -i --heading --no-config -e "punycode" .
.\package-lock.json
8833:19:    "node_modules/punycode": {
8835:47:      "resolved": "https://registry.npmjs.org/punycode/-/punycode-2.3.1.tgz",
10473:10:        "punycode": "^2.3.1"
10915:10:        "punycode": "^2.1.0"

.\packages\vscode-ide-companion\package-lock.json
3940:19:    "node_modules/punycode": {
3942:47:      "resolved": "https://registry.npmjs.org/punycode/-/punycode-2.3.1.tgz",
4804:10:        "punycode": "^2.1.0"

.\bundle\gemini.js
39539:9:    var punycode = __require("punycode");
39621:17:        label = punycode.toUnicode(label);
39664:18:          return punycode.toASCII(l2);
39700:9:    var punycode = __require("punycode");
39713:14:      return punycode.ucs2.decode(str).length;
39882:15:      input = punycode.ucs2.decode(input);
40035:23:      const decoded = punycode.ucs2.decode(input);
40141:20:      this.input = punycode.ucs2.decode(this.input);
40742:23:      const decoded = punycode.ucs2.decode(username);
40749:23:      const decoded = punycode.ucs2.decode(password);
47700:9:    var punycode = __require("punycode");
47782:17:        label = punycode.toUnicode(label);
47825:18:          return punycode.toASCII(l2);
47861:9:    var punycode = __require("punycode");
47874:14:      return punycode.ucs2.decode(str).length;
48043:15:      input = punycode.ucs2.decode(input);
48196:23:      const decoded = punycode.ucs2.decode(input);
48302:20:      this.input = punycode.ucs2.decode(this.input);
48903:23:      const decoded = punycode.ucs2.decode(username);
48910:23:      const decoded = punycode.ucs2.decode(password);
53049:9:    var punycode = __require("punycode");
53131:17:        label = punycode.toUnicode(label);
53174:18:          return punycode.toASCII(l2);
53210:9:    var punycode = __require("punycode");
53223:14:      return punycode.ucs2.decode(str).length;
53392:15:      input = punycode.ucs2.decode(input);
53545:23:      const decoded = punycode.ucs2.decode(input);
53651:20:      this.input = punycode.ucs2.decode(this.input);
54252:23:      const decoded = punycode.ucs2.decode(username);
54259:23:      const decoded = punycode.ucs2.decode(password);
104895:16:      var regexPunycode = /^xn--/;
105155:23:          return regexPunycode.test(string) ? decode(string.slice(4).toLowerCase()) : string;
105163:11:      var punycode = {
105165:46:         * A string representing the current Punycode.js version number.
105166:22:         * @memberOf punycode
105174:22:         * @memberOf punycode
105352:35:                components.host = punycode.toASCII(components.host.replace(protocol.PCT_ENCODED, pctDecChars).toLowerCase());
105354:110:                components.error = components.error || "Host's domain name can not be converted to ASCII via punycode: " + e2;
105426:49:              components.host = !options2.iri ? punycode.toASCII(components.host.replace(protocol.PCT_ENCODED, pctDecChars).toLowerCase()) : punycode.toUnicode(components.host);
105428:148:              components.error = components.error || "Host's domain name can not be converted to " + (!options2.iri ? "ASCII" : "Unicode") + " via punycode: " + e2;
105674:27:                addr[1] = punycode.toASCII(unescapeComponent(addr[1], options2).toLowerCase());
105676:131:                mailtoComponents.error = mailtoComponents.error || "Email address's domain name can not be converted to ASCII via punycode: " + e2;
105695:42:                domain = !options2.iri ? punycode.toASCII(unescapeComponent(domain, options2).toLowerCase()) : punycode.toUnicode(domain);
105697:159:                components.error = components.error || "Email address's domain name can not be converted to " + (!options2.iri ? "ASCII" : "Unicode") + " via punycode: " + e2;
204417:7:      punycode: ">= 0.5",
204418:13:      "node:punycode": [">= 14.18 && < 15", ">= 16"],
204943:7:      punycode: ">= 0.5",
204944:13:      "node:punycode": [">= 14.18 && < 15", ">= 16"],
...

We're sort of in a chicken/egg situation here. eslint has in the past taken a stance that they're going to just ignore the deprecation and stay on ajv v6 because it works for them and they don't want to upset their ecosystem. This decision by the eslint maintainers pre-dated the hard deprecation warnings added to node more recently. It's a little more frustrating because eslintrc (the source of the error you observed) is described as "legacy" since 4 years ago. It's marked as frozen .

Longer history here

I don't know what the right solution is. Maybe in the bundle process, special case this to always use require("puncode/") (note trailing slash).

tl;dr This is why we cannot have nice nodejs things.

@ipapapa
Copy link
Contributor Author

ipapapa commented Aug 4, 2025

There is a catch-22. From one side, we have the warnings and from the other side the lint errors, so I will remove ajv overall from this PR.

@ipapapa ipapapa force-pushed the fix/deprecated-dependencies branch from 95638c1 to 847b117 Compare August 4, 2025 11:45
@ipapapa
Copy link
Contributor Author

ipapapa commented Aug 4, 2025

The problematic ajv override has been successfully removed, and the PR now contains only the dependency updates without any ESLint compatibility issues. The concerns have been addressed by keeping the focus on the OpenTelemetry, MCP SDK, and similar updates.

@ipapapa ipapapa force-pushed the fix/deprecated-dependencies branch from 847b117 to a5d556a Compare August 14, 2025 00:23
@ipapapa
Copy link
Contributor Author

ipapapa commented Aug 14, 2025

@google-gemini/gemini-cli-askmode-approvers Could you please review this PR? All conflicts have been resolved and the build is passing.

@swissspidy
Copy link
Collaborator

Looks like there are now some conflicts that need to be resolved

package.json Outdated
Copy link
Collaborator

@swissspidy swissspidy Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be updated in the individual package where they're used, not here

Copy link
Contributor Author

@ipapapa ipapapa Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point:

  • Dependencies are now updated only in individual packages where they're used:
    • @modelcontextprotocol/sdk: ^1.11.0 → ^1.17.0 in packages/core/package.json
    • @modelcontextprotocol/sdk: ^1.15.1 → ^1.17.0 in packages/cli/package.json
    • @modelcontextprotocol/sdk: ^1.15.1 → ^1.17.0 in packages/vscode-ide-companion/package.json
    • google-auth-library: ^9.11.0 → ^10.2.0 in packages/core/package.json
  • NO dependencies added to root package.json

@ipapapa ipapapa force-pushed the fix/deprecated-dependencies branch 2 times, most recently from 25a30b7 to 739141f Compare August 29, 2025 20:47
- Update @modelcontextprotocol/sdk from ^1.11.0 to ^1.17.0 in packages/core
- Update @modelcontextprotocol/sdk from ^1.15.1 to ^1.17.0 in packages/cli
- Update @modelcontextprotocol/sdk from ^1.15.1 to ^1.17.0 in packages/vscode-ide-companion
- Update google-auth-library from ^9.11.0 to ^10.2.0 in packages/core
- Keep OpenTelemetry packages at ^0.203.0 (already updated in upstream)
- Keep diff at ^7.0.0 (v8 has breaking API changes requiring code updates)

Dependencies are now updated in individual packages where they're used,
addressing the review comment about not updating them in root package.json.
@ipapapa ipapapa force-pushed the fix/deprecated-dependencies branch from 739141f to 2416ee3 Compare August 29, 2025 20:57
@ipapapa
Copy link
Contributor Author

ipapapa commented Aug 29, 2025

Looks like there are now some conflicts that need to be resolved

@swissspidy yes this has been fixed as well. It has been standing for a month so conflicts naturally arise.

@ipapapa
Copy link
Contributor Author

ipapapa commented Aug 29, 2025

I have also created an issue per the @gemini-cli-robot flag. #7434

@ipapapa ipapapa force-pushed the fix/deprecated-dependencies branch from ef3ee0a to 69e019b Compare August 29, 2025 22:42
"yargs": "^17.7.2",
"zod": "^3.23.8"
"zod": "^3.23.8",
"google-auth-library": "^10.2.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this newly added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The google-auth-library dependency was added to packages/cli/package.json to fix a TypeScript compilation error that occurred after the dependency updates, but let me check if removing it would actually not cause an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested removing the google-auth-library dependency from the CLI package and the compilation error returns:

The dependency is needed because:

  • CLI package imports OAuth2Client from google-auth-library in test files
  • Without explicit dependency, it resolves to v9.x (lacks these properties)
  • Core package uses v10.x (has these properties)
  • TypeScript sees two different OAuth2Client types and they're incompatible

@chrstnb
Copy link
Collaborator

chrstnb commented Oct 29, 2025

Thank you for the contribution! We're closing this as there haven't been updates in 60 days. If you'd like to re-open, please ensure there is issue attached that has been approved by and discussed with a maintainer and merge conflicts and comments resolved.

@chrstnb chrstnb closed this Oct 29, 2025
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.

4 participants