implement $ENV:[env-var]:[fallback] config replacements #2293
Conversation
|
Warning Rate limit exceeded@sawka has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
✨ 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 (
|
![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
🧹 Nitpick comments (3)
pkg/wconfig/settingsconfig.go (3)
330-347: Handle nested MetaMapType and prefer any over interface{} for consistencyToday this only recurses into map[string]interface{} and []interface{}. If a nested value ever arrives as waveobj.MetaMapType (e.g., produced programmatically before serialization) it will be skipped. Also, the codebase already uses any; aligning here avoids mixed idioms.
Apply this diff (pairs with the next comment that updates resolveEnvArray to []any):
- for key, value := range m { + for key, value := range m { switch v := value.(type) { case string: if resolved, ok := resolveEnvValue(v); ok { m[key] = resolved } - case map[string]interface{}: - resolveEnvReplacements(waveobj.MetaMapType(v)) - case []interface{}: - resolveEnvArray(v) + case map[string]any: + resolveEnvReplacements(waveobj.MetaMapType(v)) + case waveobj.MetaMapType: + resolveEnvReplacements(v) + case []any: + resolveEnvArray(v) } }
349-362: Harmonize array handling to []any and include waveobj.MetaMapType in recursionJSON unmarshal yields []any; switching the signature and cases makes both functions consistent. Adding the MetaMapType arm improves robustness for programmatically constructed configs.
-func resolveEnvArray(arr []interface{}) { +func resolveEnvArray(arr []any) { for i, value := range arr { switch v := value.(type) { case string: if resolved, ok := resolveEnvValue(v); ok { arr[i] = resolved } - case map[string]interface{}: - resolveEnvReplacements(waveobj.MetaMapType(v)) - case []interface{}: - resolveEnvArray(v) + case map[string]any: + resolveEnvReplacements(waveobj.MetaMapType(v)) + case waveobj.MetaMapType: + resolveEnvReplacements(v) + case []any: + resolveEnvArray(v) } } }
415-420: Ensure correct type conversion for environment‐variable overridesWe’ve confirmed that utilfn.ReUnmarshal simply does
barr, _ := json.Marshal(in) json.Unmarshal(barr, out)and that resolveEnvReplacements only replaces “$ENV:…” tokens with raw strings. The Go JSON package does not automatically convert JSON strings into numeric or boolean Go fields (e.g.
float64,int64,bool, or their pointer variants). As a result, any$ENV:…override on a non‐string setting will either cause the unmarshal to fail (and be silently ignored, leaving a zero‐value) or never apply at all.Affected fields include (but aren’t limited to):
- Boolean flags: e.g.
AppClear,TelemetryEnabled,AutoUpdateEnabled- Numeric values: e.g.
AiMaxTokens,EditorFontSize,AutoUpdateIntervalMs- Pointer types: e.g.
TermScrollback *int64,TermTransparency *float64,WindowTileGapSize *int64Recommendations:
- Document that
$ENV:substitutions currently only work for string‐typed settings.- Optionally extend
resolveEnvValue(or post‐unmarshal logic) to parse the resolved string into the target Go type—e.g. usingstrconv.ParseBool,strconv.ParseInt,strconv.ParseFloat—before callingjson.Unmarshal.- Alternatively, introduce typed placeholders like
$ENV:VAR|intor$ENV:VAR|booland handle those inresolveEnvValue.Let me know if you’d like help drafting the conversion logic or updating the documentation in a follow-up PR.
📜 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 (1)
pkg/wconfig/settingsconfig.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/wconfig/settingsconfig.go (1)
pkg/waveobj/metamap.go (1)
MetaMapType(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
pkg/wconfig/settingsconfig.go
Outdated
| func resolveEnvValue(value string) (string, bool) { | ||
| if !strings.HasPrefix(value, "$ENV:") { | ||
| return "", false | ||
| } | ||
|
|
||
| envSpec := value[5:] // Remove "$ENV:" prefix | ||
| parts := strings.SplitN(envSpec, ":", 2) | ||
| envVar := parts[0] | ||
| var fallback string | ||
| if len(parts) > 1 { | ||
| fallback = parts[1] | ||
| } | ||
|
|
||
| // Get the environment variable value | ||
| if envValue := os.Getenv(envVar); envValue != "" { | ||
| return envValue, true | ||
| } | ||
|
|
||
| // Return fallback if provided, otherwise return empty string | ||
| if fallback != "" { | ||
| return fallback, true | ||
| } | ||
| return "", true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use os.LookupEnv to distinguish “unset” vs “set to empty string”; optionally trim whitespace
With os.Getenv you can’t tell whether a variable is unset or explicitly set to an empty string. Current logic treats empty as “unset” and prefers the fallback, which is surprising if users deliberately export VAR="". Using os.LookupEnv preserves the conventional semantics: if it’s set (even to empty), prefer the env over the fallback. Trimming whitespace around the parsed parts also avoids accidental spaces in JSON.
-func resolveEnvValue(value string) (string, bool) {
+func resolveEnvValue(value string) (string, bool) {
if !strings.HasPrefix(value, "$ENV:") {
return "", false
}
- envSpec := value[5:] // Remove "$ENV:" prefix
- parts := strings.SplitN(envSpec, ":", 2)
- envVar := parts[0]
+ envSpec := value[5:] // Remove "$ENV:" prefix
+ parts := strings.SplitN(envSpec, ":", 2)
+ envVar := strings.TrimSpace(parts[0])
var fallback string
if len(parts) > 1 {
- fallback = parts[1]
+ fallback = strings.TrimSpace(parts[1])
}
// Get the environment variable value
- if envValue := os.Getenv(envVar); envValue != "" {
- return envValue, true
- }
+ if envValue, exists := os.LookupEnv(envVar); exists {
+ return envValue, true // prefer the env value even if it's an empty string
+ }
// Return fallback if provided, otherwise return empty string
if fallback != "" {
return fallback, true
}
return "", true
}If you prefer the old behavior (fallback wins over empty env), keep Getenv but please document it explicitly as it’s non-standard.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func resolveEnvValue(value string) (string, bool) { | |
| if !strings.HasPrefix(value, "$ENV:") { | |
| return "", false | |
| } | |
| envSpec := value[5:] // Remove "$ENV:" prefix | |
| parts := strings.SplitN(envSpec, ":", 2) | |
| envVar := parts[0] | |
| var fallback string | |
| if len(parts) > 1 { | |
| fallback = parts[1] | |
| } | |
| // Get the environment variable value | |
| if envValue := os.Getenv(envVar); envValue != "" { | |
| return envValue, true | |
| } | |
| // Return fallback if provided, otherwise return empty string | |
| if fallback != "" { | |
| return fallback, true | |
| } | |
| return "", true | |
| } | |
| func resolveEnvValue(value string) (string, bool) { | |
| if !strings.HasPrefix(value, "$ENV:") { | |
| return "", false | |
| } | |
| envSpec := value[5:] // Remove "$ENV:" prefix | |
| parts := strings.SplitN(envSpec, ":", 2) | |
| envVar := strings.TrimSpace(parts[0]) | |
| var fallback string | |
| if len(parts) > 1 { | |
| fallback = strings.TrimSpace(parts[1]) | |
| } | |
| // Get the environment variable value | |
| if envValue, exists := os.LookupEnv(envVar); exists { | |
| return envValue, true // prefer the env value even if it's an empty string | |
| } | |
| // Return fallback if provided, otherwise return empty string | |
| if fallback != "" { | |
| return fallback, true | |
| } | |
| return "", true | |
| } |
🤖 Prompt for AI Agents
In pkg/wconfig/settingsconfig.go around lines 364-387, replace os.Getenv with
os.LookupEnv and trim whitespace on parsed parts so you can tell the difference
between an env var that is unset versus set to an empty string and avoid
accidental spaces: parse envSpec, strings.TrimSpace on envVar and fallback, call
os.LookupEnv(envVar) and if found (even empty) return that value with true; if
not found and fallback != "" return trimmed fallback with true; otherwise return
"" and true (preserving the function's resolved flag semantics).
No description provided.