feat: expand verify-release to macOS and Windows#18145
Conversation
|
Hi there! Thank you for your contribution to Gemini CLI. To improve our contribution process and better track changes, we now require all pull requests to be associated with an existing issue, as announced in our recent discussion and as detailed in our CONTRIBUTING.md. This pull request is being closed because it is not currently linked to an issue. Once you have updated the description of this PR to link an issue (e.g., by adding How to link an issue: Thank you for your understanding and for being a part of our community! |
|
Hi @yunaseoul, thank you so much for your contribution to Gemini CLI! We really appreciate the time and effort you've put into this. We're making some updates to our contribution process to improve how we track and review changes. Please take a moment to review our recent discussion post: Improving Our Contribution Process & Introducing New Guidelines. Key Update: Starting January 26, 2026, the Gemini CLI project will require all pull requests to be associated with an existing issue. Any pull requests not linked to an issue by that date will be automatically closed. Thank you for your understanding and for being a part of our community! |
Summary of ChangesHello @yunaseoul, 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 enhances the robustness of the release verification process by extending the Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
There was a problem hiding this comment.
Code Review
This pull request aims to expand the verify-release workflow to support macOS and Windows. The change in packages/test-utils/src/test-rig.ts correctly handles the binary name for Windows. However, it introduces a dependency on the os module without importing it, which will cause a runtime error. This is a critical issue that needs to be addressed.
|
Size Change: -2 B (0%) Total Size: 23.7 MB ℹ️ View Unchanged
|
d294814 to
2b955cc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully expands the verify-release workflow to support macOS and Windows by updating the test rig to handle platform-specific command names. The changes are logical and directly address the goal. However, one of the changes introduces an inconsistency in how Node.js built-in modules are imported compared to the rest of the codebase. While this was done for compatibility, it's a maintainability risk. I've left a comment with suggestions to either apply the change consistently or add documentation to prevent future issues.
packages/test-utils/src/test-rig.ts
Outdated
| import * as pty from '@lydell/node-pty'; | ||
| import stripAnsi from 'strip-ansi'; | ||
| import * as os from 'node:os'; | ||
| import * as os from 'os'; |
There was a problem hiding this comment.
This change from import * as os from 'node:os'; to import * as os from 'os'; introduces an inconsistency. Other files in the project, such as packages/cli/src/gemini.tsx and packages/core/src/utils/shell-utils.ts, still use the recommended node: prefix for built-in modules.
If this change is required for compatibility with a specific tool in the CI pipeline, it's important to either:
- Apply this change consistently across the codebase for all Node.js built-in imports.
- Add a comment explaining why this specific file needs to deviate from the project's standard import style. This will prevent other developers from reverting it and accidentally breaking the build.
Without consistency or a comment, this creates a maintainability issue that could lead to hard-to-debug problems in the future.
a6b4f26 to
c4eb567
Compare
Co-authored-by: Yuna Seol <yunaseol@google.com>
Co-authored-by: Yuna Seol <yunaseol@google.com>
Co-authored-by: Yuna Seol <yunaseol@google.com>
feat: expand verify-release to macOS and Windows
Summary
Updates the
verify-releaseworkflow to run on a matrix of OSes (Ubuntu, macOS, Windows) to ensure cross-platform compatibility of the release verification process.Details
.github/workflows/verify-release.ymlto use a build matrix foros.packages/test-utils/src/test-rig.tsto correctly handle the binary name (gemini.cmd) on Windows when running NPM release tests.osimport to usefrom 'os'for broader compatibility.Related Issues
Fixes #18140
How to Validate
verify-releaseworkflow manually from the Actions tab.Pre-Merge Checklist