Fixed conversion of environment variables to number types with GitHub Copilot 2#155
Fixed conversion of environment variables to number types with GitHub Copilot 2#155ryo8000 wants to merge 1 commit into
Conversation
WalkthroughEnvironment variable handling is refactored to parse and validate numeric values with explicit errors, introduce ordering checks among timeouts, and compute maxDelay and port via validated helpers. Tests are added to assert rejection on invalid integer inputs and out-of-range values for all relevant variables, including PORT beyond safe integer limits. Changes
Sequence Diagram(s)sequenceDiagram
participant Proc as process.env
participant Env as loadEnv (src/env.ts)
participant Parser as toSafeInteger wrappers
participant Validator as Ordering checks
participant Export as environment export
Proc->>Env: Load variables
Env->>Parser: Parse HEADERS_TIMEOUT / REQUEST_TIMEOUT / KEEP_ALIVE_TIMEOUT / MAX_DELAY / PORT
Parser-->>Env: Numbers or throw on invalid
Env->>Validator: Verify keepAlive < headers < request
Validator-->>Env: OK or throw
Env->>Export: Build environment object
Export-->>Caller: Return config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 @ryo8000, 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 significantly enhances the robustness of environment variable parsing by replacing simple Number() conversions with a more secure and error-handling method. The changes ensure that numerical environment variables are strictly validated, throwing explicit errors for invalid inputs. This prevents potential runtime issues caused by malformed configuration values and improves the overall stability of the application's startup process.
Highlights
- Robust Environment Variable Parsing: Replaced direct Number() conversions for HEADERS_TIMEOUT, REQUEST_TIMEOUT, KEEP_ALIVE_TIMEOUT, MAX_DELAY, and PORT with a custom toSafeInteger utility. This new approach includes explicit checks for undefined values and throws detailed errors for non-integer or out-of-range inputs.
- Enhanced Error Handling: Introduced specific and descriptive error messages for each environment variable when an invalid numerical value is provided, making it significantly easier to diagnose configuration issues.
- Comprehensive Unit Tests: Added comprehensive unit tests to env.test.ts covering various invalid scenarios for environment variables, including non-numeric strings, floats, scientific notation, and values exceeding Number.MAX_SAFE_INTEGER, ensuring the new error handling mechanism functions correctly.
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. 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.
String to number, oh! Careful parsing, errors caught, Code runs safe and sound.
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. ↩
ryo8000
changed the title
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of environment variable parsing by validating that numeric values are indeed valid integers and throwing an error if they are not. This is a good improvement over the previous behavior of silently using default values. The added tests effectively cover these new error cases.
My main feedback is on the implementation in src/env.ts, where there is significant code duplication for parsing each numeric environment variable. I've suggested refactoring this into a helper function to improve maintainability and readability.
| const headersTimeoutValue = process.env['HEADERS_TIMEOUT']; | ||
| const requestTimeoutValue = process.env['REQUEST_TIMEOUT']; | ||
| const keepAliveTimeoutValue = process.env['KEEP_ALIVE_TIMEOUT']; | ||
| const maxDelayValue = process.env['MAX_DELAY']; | ||
| const portValue = process.env['PORT']; | ||
|
|
||
| // Convert environment variables with error handling | ||
| const headersTimeout = | ||
| headersTimeoutValue !== undefined | ||
| ? (() => { | ||
| const parsed = toSafeInteger(headersTimeoutValue); | ||
| if (parsed === undefined) { | ||
| throw new Error( | ||
| `Invalid HEADERS_TIMEOUT value: "${headersTimeoutValue}". Must be a valid integer.` | ||
| ); | ||
| } | ||
| return parsed; | ||
| })() | ||
| : 10000; | ||
|
|
||
| const requestTimeout = | ||
| requestTimeoutValue !== undefined | ||
| ? (() => { | ||
| const parsed = toSafeInteger(requestTimeoutValue); | ||
| if (parsed === undefined) { | ||
| throw new Error( | ||
| `Invalid REQUEST_TIMEOUT value: "${requestTimeoutValue}". Must be a valid integer.` | ||
| ); | ||
| } | ||
| return parsed; | ||
| })() | ||
| : 30000; | ||
|
|
||
| const keepAliveTimeout = | ||
| keepAliveTimeoutValue !== undefined | ||
| ? (() => { | ||
| const parsed = toSafeInteger(keepAliveTimeoutValue); | ||
| if (parsed === undefined) { | ||
| throw new Error( | ||
| `Invalid KEEP_ALIVE_TIMEOUT value: "${keepAliveTimeoutValue}". Must be a valid integer.` | ||
| ); | ||
| } | ||
| return parsed; | ||
| })() | ||
| : 5000; | ||
|
|
||
| const maxDelay = | ||
| maxDelayValue !== undefined | ||
| ? (() => { | ||
| const parsed = toSafeInteger(maxDelayValue); | ||
| if (parsed === undefined) { | ||
| throw new Error(`Invalid MAX_DELAY value: "${maxDelayValue}". Must be a valid integer.`); | ||
| } | ||
| return parsed; | ||
| })() | ||
| : 10000; | ||
|
|
||
| const port = | ||
| portValue !== undefined | ||
| ? (() => { | ||
| const parsed = toSafeInteger(portValue); | ||
| if (parsed === undefined) { | ||
| throw new Error(`Invalid PORT value: "${portValue}". Must be a valid integer.`); | ||
| } | ||
| return parsed; | ||
| })() | ||
| : 8000; |
There was a problem hiding this comment.
The logic for parsing and validating numeric environment variables is repeated five times. This code duplication makes the code harder to read and maintain. Any future changes to this logic would need to be applied in all five places, which is error-prone.
To improve this, you can extract the parsing logic into a reusable helper function. This will make the code more concise, readable, and easier to maintain, following the DRY (Don't Repeat Yourself) principle.
| const headersTimeoutValue = process.env['HEADERS_TIMEOUT']; | |
| const requestTimeoutValue = process.env['REQUEST_TIMEOUT']; | |
| const keepAliveTimeoutValue = process.env['KEEP_ALIVE_TIMEOUT']; | |
| const maxDelayValue = process.env['MAX_DELAY']; | |
| const portValue = process.env['PORT']; | |
| // Convert environment variables with error handling | |
| const headersTimeout = | |
| headersTimeoutValue !== undefined | |
| ? (() => { | |
| const parsed = toSafeInteger(headersTimeoutValue); | |
| if (parsed === undefined) { | |
| throw new Error( | |
| `Invalid HEADERS_TIMEOUT value: "${headersTimeoutValue}". Must be a valid integer.` | |
| ); | |
| } | |
| return parsed; | |
| })() | |
| : 10000; | |
| const requestTimeout = | |
| requestTimeoutValue !== undefined | |
| ? (() => { | |
| const parsed = toSafeInteger(requestTimeoutValue); | |
| if (parsed === undefined) { | |
| throw new Error( | |
| `Invalid REQUEST_TIMEOUT value: "${requestTimeoutValue}". Must be a valid integer.` | |
| ); | |
| } | |
| return parsed; | |
| })() | |
| : 30000; | |
| const keepAliveTimeout = | |
| keepAliveTimeoutValue !== undefined | |
| ? (() => { | |
| const parsed = toSafeInteger(keepAliveTimeoutValue); | |
| if (parsed === undefined) { | |
| throw new Error( | |
| `Invalid KEEP_ALIVE_TIMEOUT value: "${keepAliveTimeoutValue}". Must be a valid integer.` | |
| ); | |
| } | |
| return parsed; | |
| })() | |
| : 5000; | |
| const maxDelay = | |
| maxDelayValue !== undefined | |
| ? (() => { | |
| const parsed = toSafeInteger(maxDelayValue); | |
| if (parsed === undefined) { | |
| throw new Error(`Invalid MAX_DELAY value: "${maxDelayValue}". Must be a valid integer.`); | |
| } | |
| return parsed; | |
| })() | |
| : 10000; | |
| const port = | |
| portValue !== undefined | |
| ? (() => { | |
| const parsed = toSafeInteger(portValue); | |
| if (parsed === undefined) { | |
| throw new Error(`Invalid PORT value: "${portValue}". Must be a valid integer.`); | |
| } | |
| return parsed; | |
| })() | |
| : 8000; | |
| function getNumericEnv(name: string, defaultValue: number): number { | |
| const value = process.env[name]; | |
| if (value === undefined) { | |
| return defaultValue; | |
| } | |
| const parsed = toSafeInteger(value); | |
| if (parsed === undefined) { | |
| throw new Error(`Invalid ${name} value: "${value}". Must be a valid integer.`); | |
| } | |
| return parsed; | |
| } | |
| // Convert environment variables with error handling | |
| const headersTimeout = getNumericEnv('HEADERS_TIMEOUT', 10000); | |
| const requestTimeout = getNumericEnv('REQUEST_TIMEOUT', 30000); | |
| const keepAliveTimeout = getNumericEnv('KEEP_ALIVE_TIMEOUT', 5000); | |
| const maxDelay = getNumericEnv('MAX_DELAY', 10000); | |
| const port = getNumericEnv('PORT', 8000); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/ut/env.test.ts (2)
84-90: Prevent test flakiness by clearing numeric env vars in this blockIf CI (or a dev shell) happens to define any of these env vars, these tests could behave unexpectedly. Clearing them at the start of this describe keeps the tests hermetic without relying on global beforeEach.
Apply this diff to insert a local beforeEach:
describe('Invalid environment variable values', () => { + beforeEach(() => { + delete process.env.HEADERS_TIMEOUT; + delete process.env.REQUEST_TIMEOUT; + delete process.env.KEEP_ALIVE_TIMEOUT; + delete process.env.MAX_DELAY; + delete process.env.PORT; + });
84-132: Reduce duplication via table-driven tests (optional)You can collapse the repetitive invalid-input tests into a single parameterized test for readability and maintainability.
Example:
it.each([ ['HEADERS_TIMEOUT', 'invalid'], ['REQUEST_TIMEOUT', '12.5'], ['KEEP_ALIVE_TIMEOUT', 'abc123'], ['MAX_DELAY', '1e5'], ['PORT', 'port8000'], ] as const)('should throw an error for invalid %s value', async (name, value) => { process.env[name] = value; await expect(loadEnv()).rejects.toThrow( `Invalid ${name} value: "${value}". Must be a valid integer.` ); });src/env.ts (2)
71-81: Add basic range validation for timeouts, maxDelay, and portCurrently, negative values for timeouts/maxDelay or out-of-range ports (e.g., 65536 or -1) would pass parsing but are invalid at runtime. Add sanity checks before the ordering assertions.
Apply this diff to insert the checks just before the ordering validations:
- +// Sanity range checks +if (headersTimeout < 0 || keepAliveTimeout < 0 || requestTimeout < 0) { + throw new Error( + 'Invalid timeout configuration: values must be non-negative integers (ms).' + ); +} +if (maxDelay < 0) { + throw new Error( + `Invalid MAX_DELAY value: "${maxDelayValue}". Must be a non-negative integer.` + ); +} +if (port < 0 || port > 65535) { + throw new Error( + `Invalid PORT value: "${portValue}". Must be an integer between 0 and 65535.` + ); +} + if (headersTimeout <= keepAliveTimeout) { throw new Error( `Invalid timeout configuration: headersTimeout (${headersTimeout}ms) must be greater than keepAliveTimeout (${keepAliveTimeout}ms)` ); }If you adopt these checks, consider adding a few targeted tests (e.g., PORT='-1', PORT='65536', MAX_DELAY='-1') to lock behavior.
9-69: Reduce repetition with a small helper to read safe integersThe IIFE pattern repeats the same parse/throw/default logic five times. A tiny helper improves readability and makes future additions cheaper.
Sketch:
const readSafeInt = (name: 'HEADERS_TIMEOUT' | 'REQUEST_TIMEOUT' | 'KEEP_ALIVE_TIMEOUT' | 'MAX_DELAY' | 'PORT', defaultValue: number): number => { const raw = process.env[name]; if (raw === undefined) return defaultValue; const parsed = toSafeInteger(raw); if (parsed === undefined) { throw new Error(`Invalid ${name} value: "${raw}". Must be a valid integer.`); } return parsed; }; // Usage: const headersTimeout = readSafeInt('HEADERS_TIMEOUT', 10000); const requestTimeout = readSafeInt('REQUEST_TIMEOUT', 30000); const keepAliveTimeout = readSafeInt('KEEP_ALIVE_TIMEOUT', 5000); const maxDelay = readSafeInt('MAX_DELAY', 10000); const port = readSafeInt('PORT', 8000);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/env.ts(2 hunks)tests/ut/env.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/ut/env.test.ts (1)
tests/ut/utils/number.test.ts (2)
validIntegerTestCases(4-19)describe(3-39)
src/env.ts (1)
src/utils/number.ts (1)
toSafeInteger(16-28)
🔇 Additional comments (3)
tests/ut/env.test.ts (1)
84-132: Solid negative-path coverage with precise error assertionsThe new "Invalid environment variable values" suite cleanly exercises parsing failures for all numeric env vars and validates exact error messages. Nice job covering the out-of-safe-range case too.
src/env.ts (2)
1-70: Explicit, safe integer parsing with early failures is a clear upgradeParsing via toSafeInteger with explicit errors and sensible defaults improves correctness and eliminates silent coercions. The structure reads well and aligns with the new tests.
83-93: Environment export looks consistent with computed valuesUsing the validated, computed values in the exported object keeps consumers simple and reliable.
1 participant
Summary by CodeRabbit