-
Notifications
You must be signed in to change notification settings - Fork 4
Add environment variable expansion to llmconfig.yml api_key and extra_headers #70
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
Conversation
Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
Co-authored-by: garland3 <[email protected]>
…ansion details Add comprehensive documentation for configuring OpenRouter-powered LLMs, including environment variable expansion in API keys and headers for security and flexibility. Include examples, best practices, and error handling to guide admins on secure setup, preventing direct key storage in configs.
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
Modified the GitHub Actions workflow to include a new step that sets up Node.js version 20, using actions/setup-node@v4. This change ensures that Node.js is available in the CI environment, likely necessary for subsequent build steps involving JavaScript or Node-based tools. Placed after the checkout step to maintain logical order in the job execution.
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.
Pull Request Overview
This PR unifies environment variable expansion for LLM configurations by replacing os.path.expandvars() with resolve_env_var(), aligning the implementation with MCP server authentication handling. The change provides explicit error messages when required environment variables are unset and extends environment variable expansion support to the extra_headers field in LLM model configurations.
Key changes:
- Replace silent
os.path.expandvars()withresolve_env_var()that raises clear errors for missing variables - Add environment variable expansion to
extra_headersdictionary values - Remove unused CLI module (
backend/modules/llm/cli.py)
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
backend/modules/llm/litellm_caller.py |
Implements resolve_env_var() for api_key and extra_headers with proper error handling |
backend/modules/llm/cli.py |
Entire file deleted (323 lines) - CLI module removed |
backend/tests/test_llm_env_expansion.py |
Adds 6 integration tests covering env var expansion for api_key and extra_headers |
backend/tests/test_config_manager.py |
Adds 6 unit tests validating ModelConfig with environment variables |
docs/02_admin_guide.md |
Extensive documentation update (86 new lines) covering env var expansion patterns, security best practices, and examples |
| compliance_level: "External" | ||
| ``` | ||
|
|
||
| **Note**: The second example demonstrates environment variable expansion in `extra_headers`, which is useful for services like OpenRouter that require site identification headers. |
Copilot
AI
Nov 14, 2025
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.
The phrase 'The second example' is ambiguous since both configuration examples in lines 386-403 are OpenRouter configurations with environment variables. Consider replacing with 'The OpenRouter example above' or 'This example' to be more precise.
| **Note**: The second example demonstrates environment variable expansion in `extra_headers`, which is useful for services like OpenRouter that require site identification headers. | |
| **Note**: The OpenRouter example above demonstrates environment variable expansion in `extra_headers`, which is useful for services like OpenRouter that require site identification headers. |
| - **Never**: Commit API keys to `config/defaults/llmconfig.yml` or any version-controlled files | ||
|
|
||
| This environment variable expansion system works identically to the MCP server `auth_token` field, providing consistent behavior across all authentication and configuration mechanisms in the application. | ||
|
|
Copilot
AI
Nov 14, 2025
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.
The documentation claims the behavior is 'identical' to MCP server auth_token, but doesn't mention the key limitation that resolve_env_var() only supports exact ${VAR} patterns at the beginning of strings (using re.match()), not partial substitution like 'prefix-${VAR}-suffix'. Consider adding a note about this limitation or clarifying that only full-value environment variable substitution is supported.
| > **Limitation:** Only full-value environment variable substitution is supported. The value must be exactly in the form `"${VAR_NAME}"` (with nothing else before or after). Partial substitution, such as `"prefix-${VAR}-suffix"`, is **not** supported and will not be expanded. This applies to all fields that support environment variable expansion, including `api_key`, `auth_token`, and custom headers. |
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
1 similar comment
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
…urce - Upgrade Node.js from default dnf package to v20.x for improved compatibility and features - Remove npm explicit install as it's bundled with Node.js - Apply consistent changes to both Dockerfile and Dockerfile-test for standardized builds - Use NodeSource repository for reliable, up-to-date Node.js provisioning
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
1 similar comment
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
Uncomment the entire GitHub Actions workflow file to activate continuous integration and delivery, including steps for Node.js setup, Docker image building for tests and production, running tests in containers, and pushing images with metadata tagging. This enables automated processes for branches like main and develop, improving development efficiency and deployment reliability.
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
…api-key-expansion
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
1 similar comment
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
Changed require('@tailwindcss/typography') to ES6 import syntax (import tailwindTypography from '@tailwindcss/typography') in tailwind.config.js for better module consistency and modern JavaScript practices.
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
🔒 Security Scan ResultsSecurity Scan SummaryScan ResultsPython SAST (Bandit)Recommendations
|
LLM configurations currently use
os.path.expandvars()for api_key expansion, which silently returns unexpanded strings when environment variables are missing. This differs from MCP server auth_token handling, which usesresolve_env_var()to provide explicit error messages.Changes
Unified environment variable expansion:
os.path.expandvars()withresolve_env_var()inlitellm_caller.pyandcli.pyValueErrorwith clear message when required environment variables are unsetextra_headersfieldExample usage:
Test coverage:
test_config_manager.pyfor ModelConfig validationtest_llm_env_expansion.pyfor LiteLLMCaller behaviorOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.