-
Notifications
You must be signed in to change notification settings - Fork 213
feat: add stdio-based function discovery mode #1711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 @taeold, 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 introduces a significant enhancement to the function discovery process by providing an alternative stdio
-based mechanism. This change aims to improve the reliability and speed of function manifest generation, particularly in environments where traditional HTTP server-based discovery might encounter issues or introduce unnecessary overhead. It ensures backward compatibility while offering a more streamlined approach for future integrations.
Highlights
- New Discovery Mode: Introduced a new
stdio
-based function discovery mechanism that outputs function manifests directly tostderr
. - Improved Reliability & Performance: This new mode avoids the overhead and potential reliability issues associated with starting an HTTP server for function discovery, leading to faster and more robust discovery.
- Conditional Activation: The
stdio
discovery mode is activated by setting theFUNCTIONS_DISCOVERY_MODE=stdio
environment variable, while the existing HTTP-based discovery remains the default. - Structured Output: Function manifests are output as base64-encoded JSON prefixed with
__FIREBASE_FUNCTIONS_MANIFEST__:
, and errors are similarly prefixed with__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:
, facilitating programmatic parsing.
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 is currently in preview and 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 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.
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 introduces a new stdio-based discovery mode, which enhances reliability and performance. Suggestions have been made to improve error handling and reduce the use of magic strings in the new runStdioDiscovery
function.
Adds an alternative discovery mechanism that outputs function manifests via stderr instead of starting an HTTP server. This improves reliability by avoiding issues where module loading blocks the HTTP endpoint from becoming available. When FUNCTIONS_DISCOVERY_MODE=stdio is set: - Outputs base64-encoded manifest to stderr with __FIREBASE_FUNCTIONS_MANIFEST__: prefix - Outputs errors to stderr with __FIREBASE_FUNCTIONS_MANIFEST_ERROR__: prefix - Exits immediately without starting HTTP server - Maintains backward compatibility (HTTP remains default) Includes comprehensive tests that verify both HTTP and stdio discovery work correctly for all test cases (commonjs, esm, various configurations).
- Add test fixture for broken syntax error handling - Refactor tests to use unified DiscoveryResult interface - Parameterize tests with nested loops to test all cases with both discovery methods - Add error handling tests for both HTTP and stdio discovery - All stdio discovery tests passing (16/16)
bf5e751
to
62e37ba
Compare
- Mark runStdioDiscovery() promise with void operator - Move handleQuitquitquit function to program root - Fix TypeScript warnings for error handling - Add eslint-disable comment for Express async handler
/gemini review |
There was a problem hiding this 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 introduces a new stdio
-based function discovery mechanism as an alternative to the existing HTTP-based one. This is a great feature for improving reliability and performance. The implementation looks solid, and the tests are comprehensive, covering both success and error paths for both discovery methods.
My review focuses on improving the robustness of the new tests and addressing a regression in the HTTP discovery test logic. I've identified a few areas for improvement:
- A bug in the test retry logic that could mask failures.
- Fragile regular expressions for parsing
stdio
output. - Some code duplication in the test files.
- Add proper error handling for non-Error objects - Define MANIFEST_PREFIX and MANIFEST_ERROR_PREFIX constants - Fix HTTP discovery retry logic regression in tests - Make regex patterns multiline-safe for manifest and error parsing - Add timeout handling for stdio discovery in tests
38b69b5
to
a5f0b05
Compare
- Remove unnecessary dependencies from package.json - Remove package-lock.json - The npm link is handled by run.sh script
/gemini review |
There was a problem hiding this 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 introduces a new stdio-based function discovery mechanism, improving reliability and performance. The implementation is clean and the tests are thorough. There's a potential race condition with process.exit()
that could lead to truncated output, and some code duplication in the test setup.
src/bin/firebase-functions.ts
Outdated
process.stderr.write(`${MANIFEST_PREFIX}${base64}\n`); | ||
process.exit(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling process.exit()
immediately after process.stderr.write()
can lead to a race condition where the process terminates before the stderr
buffer is flushed. This can result in truncated or missing output, making debugging difficult.
A safer approach is to set process.exitCode
and allow the process to exit naturally once the event loop is clear.
process.stderr.write(`${MANIFEST_PREFIX}${base64}\n`); | |
process.exit(0); | |
process.stderr.write(`${MANIFEST_PREFIX}${base64}\n`); | |
process.exitCode = 0; |
- Use process.exitCode instead of process.exit() to prevent race conditions with stderr buffer flushing - Refactor test to define discoveryMethods array once to reduce code duplication
/gemini review |
There was a problem hiding this 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 introduces a new stdio-based discovery mechanism for functions, improving reliability and performance. The implementation is clean, and backward compatibility is maintained. The tests have been refactored to cover both discovery methods. A potential issue in the test suite's parsing logic could lead to flaky tests, and a suggestion has been provided to make it more robust.
- Replace magic string prefixes with self-documenting XML tags - Add explicit start/end markers to handle OS buffering edge cases - Make regex parsing more robust and less greedy - Addresses review comment about regex being too greedy
/gemini review |
There was a problem hiding this 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 introduces a new stdio-based function discovery mode, which improves performance and reliability by avoiding HTTP server overhead. Identified areas for improvement concern robustness in test code and consistency in error handling.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Description
This PR adds a new stdio-based function discovery mode to the Firebase Functions SDK. This mode allows the Firebase CLI to discover functions by running the user's code in a subprocess and capturing the function manifest through stderr output, without starting an HTTP server.
Key Changes
FUNCTIONS_DISCOVERY_MODE=stdio
environment variableStdio Output Format
The stdio discovery mode outputs function manifests and errors using XML-style tags to ensure robust parsing:
Success Output:
Error Output:
The XML-style tags provide:
Testing
Benefits