refactor: corvus env uppercase#18
Conversation
|
Thank you for contributing to this project with this PR, welcome to the community and the amazing world of open source! |
📝 WalkthroughWalkthroughThe changes introduce a new Bolt performance agent documentation and standardize environment variable naming conventions across the agent-runtime codebase from lowercase Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-02-17 to 2026-02-17 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/agent-runtime/src/config/schema.rs (1)
1635-1701:⚠️ Potential issue | 🟡 MinorInconsistent legacy fallback strategy —
MODEL,WORKSPACE, andTEMPERATURElost their non-prefixed fallbacks.
API_KEY,PROVIDER,GATEWAY_PORT, andGATEWAY_HOSTcorrectly fall back to their non-prefixed variants (API_KEY,PROVIDER,PORT,HOST), butMODEL,WORKSPACE, andTEMPERATUREonly checkCORVUS_*— no legacy fallback. This is a breaking change for users who previously setMODEL=gpt-4o(or similar) in their environment.If removing these fallbacks is intentional, please document it (e.g., in a migration note). Otherwise, add fallbacks for consistency:
Proposed fix for consistent fallback
- // Model: CORVUS_MODEL - if let Ok(model) = corvus_env("MODEL") { + // Model: CORVUS_MODEL or MODEL + if let Ok(model) = corvus_env("MODEL").or_else(|_| std::env::var("MODEL")) { if !model.is_empty() { self.default_model = Some(model); } } - // Workspace directory: CORVUS_WORKSPACE - if let Ok(workspace) = corvus_env("WORKSPACE") { + // Workspace directory: CORVUS_WORKSPACE or WORKSPACE + if let Ok(workspace) = corvus_env("WORKSPACE").or_else(|_| std::env::var("WORKSPACE")) { if !workspace.is_empty() { self.workspace_dir = PathBuf::from(workspace); } } // ... - // Temperature: CORVUS_TEMPERATURE - if let Ok(temp_str) = corvus_env("TEMPERATURE") { + // Temperature: CORVUS_TEMPERATURE or TEMPERATURE + if let Ok(temp_str) = corvus_env("TEMPERATURE").or_else(|_| std::env::var("TEMPERATURE")) {Based on learnings: "Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/config/schema.rs` around lines 1635 - 1701, The code only checks CORVUS_MODEL, CORVUS_WORKSPACE, and CORVUS_TEMPERATURE but not their legacy unprefixed env vars, causing a breaking change; update the environment lookups to use the same fallback pattern as other settings by using the corvus_env closure with .or_else(|_| std::env::var("MODEL")) / .or_else(|_| std::env::var("WORKSPACE")) / .or_else(|_| std::env::var("TEMPERATURE")) respectively, then parse/assign into self.default_model, self.workspace_dir (PathBuf::from), and self.default_temperature (parse f64 and validate range) just like the existing blocks for provider/port/host so legacy MODEL, WORKSPACE, and TEMPERATURE env vars are respected.
🧹 Nitpick comments (4)
.agents/command/bolt.agent.md (1)
145-145: Fix code span spacing instable/immutable.There’s a space inside the code span, which triggers MD038 and renders oddly.
Proposed fix
-- Missing `stable`/` immutable` annotations for better Compose compiler optimization +- Missing `stable`/`immutable` annotations for better Compose compiler optimization🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/command/bolt.agent.md at line 145, The markdown contains code spans with extra leading spaces in the annotations so change the spans from ` stable`/` immutable` to `stable`/`immutable` (remove the spaces inside the backticks) so MD038 is not triggered and the annotations render correctly; update the occurrences of the `stable` and `immutable` code spans in .agents/command/bolt.agent.md, ensuring there are no internal spaces inside the backticks.clients/agent-runtime/src/providers/mod.rs (1)
132-132: Ollama base URL resolution piggybacked onresolve_api_keyis a semantic mismatch.
resolve_api_keyreturns what callers treat as an "API key", but for Ollama, it now resolves a base URL. This works today becauseresolve_ollama_base_urlon line 200–207 validates the result withnormalize_http_url, but a future maintainer could reasonably assume the return value is a credential.Consider extracting a dedicated
resolve_ollama_base_url_from_envthat readsCORVUS_OLLAMA_BASE_URL/OLLAMA_BASE_URLdirectly, rather than routing through the API-key resolution path. This would make the intent clearer and avoid the generic fallback (CORVUS_API_KEY/API_KEY) from accidentally being interpreted as a base URL for Ollama.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/providers/mod.rs` at line 132, The current code routes Ollama's base URL lookup through resolve_api_key which semantically returns API keys; create a dedicated resolver resolve_ollama_base_url_from_env that reads only CORVUS_OLLAMA_BASE_URL and OLLAMA_BASE_URL from the environment (no generic API_KEY fallbacks), remove the "ollama" entry from the resolve_api_key mapping, and change resolve_ollama_base_url to call resolve_ollama_base_url_from_env and then validate the result with normalize_http_url so the base-URL intent is explicit and cannot be confused with credentials.clients/agent-runtime/src/onboard/wizard.rs (2)
757-795:CORVUS_API_KEY/API_KEYfallbacks in the Ollama base-URL resolver are semantically misleading.
resolve_ollama_base_url_for_fetchfalls back toCORVUS_API_KEYandAPI_KEYenv vars (lines 784-793), which conventionally hold bearer tokens, not HTTP base URLs. Althoughnormalize_http_urlsafely rejects non-URL strings, including these in a URL resolution chain is confusing for maintainers and creates a silent semantic mismatch: any user who happens to setCORVUS_API_KEYto a URL-like string would unexpectedly have it used as an Ollama endpoint.Consider removing the
CORVUS_API_KEYandAPI_KEYfallbacks to keep this function's intent clear — it resolves a base URL, not an API key.Proposed simplification
fn resolve_ollama_base_url_for_fetch(api_key: Option<&str>) -> String { api_key .and_then(normalize_http_url) .or_else(|| { std::env::var("CORVUS_OLLAMA_BASE_URL") .ok() .and_then(|value| normalize_http_url(&value)) }) .or_else(|| { std::env::var("OLLAMA_BASE_URL") .ok() .and_then(|value| normalize_http_url(&value)) }) - .or_else(|| { - std::env::var("CORVUS_API_KEY") - .ok() - .and_then(|value| normalize_http_url(&value)) - }) - .or_else(|| { - std::env::var("API_KEY") - .ok() - .and_then(|value| normalize_http_url(&value)) - }) .unwrap_or_else(|| OLLAMA_DEFAULT_BASE_URL.to_string()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/onboard/wizard.rs` around lines 757 - 795, The function resolve_ollama_base_url_for_fetch currently falls back to CORVUS_API_KEY and API_KEY environment variables (via normalize_http_url), which is semantically incorrect because those vars are tokens not base URLs; remove the two or_else branches that read CORVUS_API_KEY and API_KEY so the resolver only checks api_key, CORVUS_OLLAMA_BASE_URL and OLLAMA_BASE_URL before defaulting to OLLAMA_DEFAULT_BASE_URL, and update any related tests or callers to stop relying on API key env vars as URL fallbacks; reference resolve_ollama_base_url_for_fetch and normalize_http_url when making the change.
1827-1829:provider_env_var("ollama")now returns a URL env var instead of an API-key env var — trace the downstream impact.Every other provider arm returns an
*_API_KEYvariable, but Ollama now returnsOLLAMA_BASE_URL. Callers infetch_live_models_for_provider(line 960) read this env var into a local namedapi_key, which then flows intofetch_ollama_modelsas theapi_keyparameter. The indirection works becauseresolve_ollama_base_url_for_fetchtries to parse the value as a URL, but the variable naming (api_key) throughout the call chain is misleading.This also affects the "Skipped" message in
setup_provider(line 1523-1527), which tells users to set the env var fromprovider_env_var(...). For Ollama, this would say "Set OLLAMA_BASE_URL" — which is reasonable but different in nature from "Set YOUR_API_KEY".No functional bug, but worth a short inline comment at line 1829 to clarify the intentional semantic difference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/agent-runtime/src/onboard/wizard.rs` around lines 1827 - 1829, provider_env_var("ollama") intentionally returns OLLAMA_BASE_URL (a URL) rather than an API key; add a short inline comment next to the "ollama" arm in provider_env_var clarifying that Ollama uses a base URL, callers (fetch_live_models_for_provider -> fetch_ollama_models) still read it into a variable named `api_key` for historical reasons and rely on resolve_ollama_base_url_for_fetch to parse the URL, and that setup_provider’s "Set ..." message intentionally points to OLLAMA_BASE_URL. Mention the specific symbols provider_env_var, fetch_live_models_for_provider, fetch_ollama_models, resolve_ollama_base_url_for_fetch, and setup_provider in the comment so future readers understand the semantic difference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/command/bolt.agent.md:
- Around line 204-208: Update the three bullets under "Choose an optimization
that:" so each is a complete sentence by adding a clear subject (e.g., "The
optimization" or "It") to each bullet; specifically change "- **Has clear and
measurable performance impact** (can you benchmark it?)" to "- The optimization
has clear and measurable performance impact (can you benchmark it?)", change "-
**Can be implemented cleanly with low risk** (low regression risk)" to "- The
optimization can be implemented cleanly with low risk (low regression risk)",
and change "- Does not compromise code readability or maintainability" to "- It
does not compromise code readability or maintainability".
---
Outside diff comments:
In `@clients/agent-runtime/src/config/schema.rs`:
- Around line 1635-1701: The code only checks CORVUS_MODEL, CORVUS_WORKSPACE,
and CORVUS_TEMPERATURE but not their legacy unprefixed env vars, causing a
breaking change; update the environment lookups to use the same fallback pattern
as other settings by using the corvus_env closure with .or_else(|_|
std::env::var("MODEL")) / .or_else(|_| std::env::var("WORKSPACE")) /
.or_else(|_| std::env::var("TEMPERATURE")) respectively, then parse/assign into
self.default_model, self.workspace_dir (PathBuf::from), and
self.default_temperature (parse f64 and validate range) just like the existing
blocks for provider/port/host so legacy MODEL, WORKSPACE, and TEMPERATURE env
vars are respected.
---
Nitpick comments:
In @.agents/command/bolt.agent.md:
- Line 145: The markdown contains code spans with extra leading spaces in the
annotations so change the spans from ` stable`/` immutable` to
`stable`/`immutable` (remove the spaces inside the backticks) so MD038 is not
triggered and the annotations render correctly; update the occurrences of the
`stable` and `immutable` code spans in .agents/command/bolt.agent.md, ensuring
there are no internal spaces inside the backticks.
In `@clients/agent-runtime/src/onboard/wizard.rs`:
- Around line 757-795: The function resolve_ollama_base_url_for_fetch currently
falls back to CORVUS_API_KEY and API_KEY environment variables (via
normalize_http_url), which is semantically incorrect because those vars are
tokens not base URLs; remove the two or_else branches that read CORVUS_API_KEY
and API_KEY so the resolver only checks api_key, CORVUS_OLLAMA_BASE_URL and
OLLAMA_BASE_URL before defaulting to OLLAMA_DEFAULT_BASE_URL, and update any
related tests or callers to stop relying on API key env vars as URL fallbacks;
reference resolve_ollama_base_url_for_fetch and normalize_http_url when making
the change.
- Around line 1827-1829: provider_env_var("ollama") intentionally returns
OLLAMA_BASE_URL (a URL) rather than an API key; add a short inline comment next
to the "ollama" arm in provider_env_var clarifying that Ollama uses a base URL,
callers (fetch_live_models_for_provider -> fetch_ollama_models) still read it
into a variable named `api_key` for historical reasons and rely on
resolve_ollama_base_url_for_fetch to parse the URL, and that setup_provider’s
"Set ..." message intentionally points to OLLAMA_BASE_URL. Mention the specific
symbols provider_env_var, fetch_live_models_for_provider, fetch_ollama_models,
resolve_ollama_base_url_for_fetch, and setup_provider in the comment so future
readers understand the semantic difference.
In `@clients/agent-runtime/src/providers/mod.rs`:
- Line 132: The current code routes Ollama's base URL lookup through
resolve_api_key which semantically returns API keys; create a dedicated resolver
resolve_ollama_base_url_from_env that reads only CORVUS_OLLAMA_BASE_URL and
OLLAMA_BASE_URL from the environment (no generic API_KEY fallbacks), remove the
"ollama" entry from the resolve_api_key mapping, and change
resolve_ollama_base_url to call resolve_ollama_base_url_from_env and then
validate the result with normalize_http_url so the base-URL intent is explicit
and cannot be confused with credentials.
…ity considerations
1 participant
This pull request standardizes environment variable naming by switching from lower-case
corvus_*to upper-caseCORVUS_*throughout the codebase, and improves the handling of Ollama model fetching by allowing the base URL to be configurable via environment variables. It also introduces utility functions for environment variable access and HTTP URL normalization, making the configuration logic more robust and maintainable.Environment variable naming and config standardization:
corvus_*format to the upper-caseCORVUS_*format throughout the codebase, including in config schema, gateway, memory, main, and onboarding wizard modules. This affects both code and tests. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16]Ollama model fetching improvements:
CORVUS_OLLAMA_BASE_URLorOLLAMA_BASE_URLenvironment variables, falling back to the default if unset. This is handled by new utility functionsnormalize_http_urlandresolve_ollama_base_url_for_fetch. [1] [2] [3] [4]Utility and code cleanup:
Minor formatting:
These changes improve configuration consistency, make environment variable handling more robust, and enhance support for flexible deployments, especially with Ollama.
Summary by CodeRabbit
New Features
Documentation
Chores