Fixes for #8900 #8901
Fixes for #8900 #8901
Conversation
Add parentheses to ensure skipAutoAuth check happens before fallback to "Application Default Credentials" string. Also add unit tests to cover this logic and ensure correct behavior.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @joehan, 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 primarily focuses on enhancing the robustness and consistency of the MCP server by standardizing its module import styles. It includes a critical bug fix for handling Application Default Credentials during user authentication and introduces new tests to validate this improved behavior, alongside minor documentation and changelog updates.
Highlights
- Import Style Consistency: Standardized import paths across numerous MCP server files by removing the
.jsextension for local module imports (e.g.,import { foo } from './bar.js'becameimport { foo } from './bar'). This change aims to resolve a mixed ESM/CJS import style issue. - Authentication Logic Fix: Corrected a logical error in
src/mcp/index.tswithin theFirebaseMcpServer.getAuthenticatedUsermethod. The fix addresses an operator precedence issue that prevented the MCP server from correctly identifying 'Application Default Credentials' when no explicit user was authenticated. - New Test Coverage: Added a new test file,
src/mcp/index.spec.ts, to provide comprehensive unit test coverage for theFirebaseMcpServer.getAuthenticatedUsermethod, ensuring the corrected authentication logic functions as expected under various scenarios. - Documentation and Changelog Updates: Updated
src/mcp/CONTRIBUTING.mdto reflect the new import style in code examples. A new entry was also added toCHANGELOG.mdto document the fix for the Application Default Credentials issue.
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
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to standardize import styles to resolve issues highlighted by new tests. The changes primarily involve removing the .js extension from relative imports, which aligns with CJS conventions. However, for imports from @modelcontextprotocol/sdk/types, the .js extension is being added. This creates an inconsistent import pattern. My review includes suggestions to address this for better code clarity and maintainability. Additionally, I've noted a potential typo in the CHANGELOG.md issue reference. The bug fix for operator precedence in getAuthenticatedUser is a good catch and is well-tested by the new unit tests.
| @@ -1,4 +1,4 @@ | |||
| import { CallToolResult } from "@modelcontextprotocol/sdk/types"; | |||
| import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; | |||
There was a problem hiding this comment.
The PR description states the goal is to move all imports to CJS style, which typically means removing file extensions like .js. However, this change adds the .js extension to the import from @modelcontextprotocol/sdk/types. This appears to contradict the stated goal and introduces an inconsistency in import styles throughout the project.
Could you clarify why this change is needed? If it's not strictly necessary for CJS compatibility, please consider removing the extension to maintain a consistent style.
| import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; | |
| import { CallToolResult } from "@modelcontextprotocol/sdk/types"; |
| @@ -1,4 +1,4 @@ | |||
| import { CallToolResult } from "@modelcontextprotocol/sdk/types"; | |||
| import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; | |||
There was a problem hiding this comment.
Similar to other files in this PR, the .js extension is being added to this import path. The PR description mentions a move to CJS-style imports, which usually don't include file extensions. This change appears inconsistent with that goal.
To maintain a uniform import style, please consider removing the .js extension if it's not strictly required.
| import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; | |
| import { CallToolResult } from "@modelcontextprotocol/sdk/types"; |
| isGraphQLResponse, | ||
| } from "../../../dataconnect/types"; | ||
| import { CallToolResult } from "@modelcontextprotocol/sdk/types"; | ||
| import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; |
There was a problem hiding this comment.
This change adds a .js extension to the import, which is inconsistent with the PR's goal of standardizing on CJS-style imports (which typically omit file extensions). For consistency with the other changes in this PR that remove the extension, could you remove it here as well, unless it's required for a specific reason?
| import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; | |
| import { CallToolResult } from "@modelcontextprotocol/sdk/types"; |
| @@ -1,4 +1,4 @@ | |||
| import { CallToolResult } from "@modelcontextprotocol/sdk/types"; | |||
| import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; | |||
There was a problem hiding this comment.
The addition of the .js extension here is inconsistent with the PR's stated goal of moving to CJS-style imports. Most other changes in this PR remove this extension from relative imports. To ensure consistency, please remove the extension from this import path as well.
| import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; | |
| import { CallToolResult } from "@modelcontextprotocol/sdk/types"; |
github-project-automation
bot
moved this to Approved [PR]
in [Cloud] Extensions + Functions
github-project-automation
bot
moved this from Approved [PR]
to Done
in [Cloud] Extensions + Functions
3 participants
Description
Builds on #8900 , which added some tests which highlighted that we are doing a weird mix of ESM and CJS style imports in MCP server. To get the new tests working correctly, I've moved all these imports to CJS style. I also added a CHANGELOG entry.