Conversation
isAuthenticationError fell back to `message.includes('401')`, which
matched any message containing "401" as a substring — a port like 4012,
an id, or a line number — and misreported them as authentication errors.
Match 401 as a standalone number token (`\b401\b`) instead, so real
"HTTP 401" / "401 Unauthorized" messages still resolve while numeric
substrings do not.
Fixes google-gemini#28203
Summary of ChangesHello, 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 refines the logic used to identify authentication errors within the core package. By tightening the string matching criteria for 401 status codes, the change eliminates false positives that previously triggered unnecessary re-authentication flows when unrelated numbers appeared in error messages. Highlights
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 the 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 counterproductive. 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. Footnotes
|
|
📊 PR Size: size/S
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the authentication error detection in packages/core/src/utils/errors.ts by replacing message.includes('401') with a word-boundary regex /\b401\b/ to prevent false positives on larger numbers (e.g., ports), and adds corresponding test cases. The review feedback suggests further refining the regex to target specific HTTP/authentication contexts (like HTTP 401 or 401 Unauthorized) to avoid false positives on standalone, non-auth '401' values (such as user IDs or counts), and adding tests for those scenarios.
| // number (e.g. a port like 4012 or an id), which produced false positives. | ||
| const message = getErrorMessage(error); | ||
| if (message.includes('401')) { | ||
| if (/\b401\b/.test(message)) { |
There was a problem hiding this comment.
While matching \b401\b prevents matching larger numbers like 4012, it still matches any standalone 401 in the error message (e.g., 'User 401 not found', 'processed 401 records', or 'error at line 401'). Since triggering a 401 authentication error can lead to a highly disruptive spurious re-auth or logout flow, we should make this fallback check more precise by matching only known HTTP/authentication contexts (like HTTP 401, status code: 401, 401 Unauthorized, or exactly 401).
| if (/\b401\b/.test(message)) { | |
| if (/\bHTTP\s+401\b|\bstatus\s+code:\s*401\b|\b401\b[\s:-]*unauthorized\b|^401$/i.test(message)) { |
| it('should not match 401 as a substring of a larger number', () => { | ||
| expect(isAuthenticationError(new Error('listening on port 4012'))).toBe( | ||
| false, | ||
| ); | ||
| expect( | ||
| isAuthenticationError(new Error('connection refused at 127.0.0.1:4015')), | ||
| ).toBe(false); | ||
| expect(isAuthenticationError(new Error('processed 24013 records'))).toBe( | ||
| false, | ||
| ); | ||
| expect(isAuthenticationError(new Error('error at line 1401'))).toBe(false); | ||
| }); |
There was a problem hiding this comment.
To prevent regressions and ensure that standalone numbers like 401 in non-authentication contexts (e.g., user IDs, record counts) are not falsely matched as authentication errors, we should add explicit test cases for these scenarios.
it('should not match 401 as a substring of a larger number or in non-auth contexts', () => {
expect(isAuthenticationError(new Error('listening on port 4012'))).toBe(
false,
);
expect(
isAuthenticationError(new Error('connection refused at 127.0.0.1:4015')),
).toBe(false);
expect(isAuthenticationError(new Error('processed 24013 records'))).toBe(
false,
);
expect(isAuthenticationError(new Error('error at line 1401'))).toBe(false);
expect(isAuthenticationError(new Error('User 401 not found'))).toBe(false);
expect(isAuthenticationError(new Error('processed 401 records'))).toBe(false);
});
gemini-cli
Bot
added
priority/p2
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
|
This one's already triaged |
1 participant
Summary
isAuthenticationErrorfell back tomessage.includes('401'), which matches any error message containing401as a substring — a port number like4012, an id, or a line number — and misreports those as authentication errors. That can trigger a spurious re-auth / logout flow on errors that have nothing to do with auth.Details
The other branches of
isAuthenticationError(numericcode === 401,UnauthorizedError) are precise; only the string fallback was too loose. This changes it to match401as a standalone number token (\b401\b) instead of a raw substring, so a larger number that merely contains401no longer matches, while real messages likeHTTP 401,401 Unauthorized,Status code: 401, and the MCP SDK'sError POSTing to endpoint (HTTP 401): ...still resolve.Related Issues
Fixes #28203
How to Validate
cd packages/core npx vitest run src/utils/errors.test.tsThe
isAuthenticationErrorsuite adds cases that the old substring check failed:401 Unauthorized,HTTP 401,Status code: 401,Error POSTing to endpoint (HTTP 401): deniedlistening on port 4012,connection refused at 127.0.0.1:4015,processed 24013 records,error at line 1401Pre-Merge Checklist