chore: improved good defaults#34
Conversation
📝 WalkthroughWalkthroughThe PR changes default logging environment handling: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
config/config.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-12-28T22:10:31.364ZApplied to files:
📚 Learning: 2026-01-15T23:07:37.652ZApplied to files:
⏰ 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). (1)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/config.go (1)
233-241: AlignOnlyModelInteractionsdefault across config sources (and fix CI unused helper).Line 241 now uses
getEnvBool, so when the env var is unset the env-only path defaults to false, whileviper.SetDefault("logging.only_model_interactions", true)(Line 189) and the LogConfig comment (Line 70) still indicate a true default. This makes behavior depend on whether a config file exists and can unexpectedly broaden logging when enabled. Also, CI reportsgetEnvBoolOrDefaultunused (Line 436).If the intended default is still true, switch back to
getEnvBoolOrDefault(fixes the lint error). If the intended default is now false, update the viper default and comment and remove the helper.✅ Suggested fix (keep default true)
- OnlyModelInteractions: getEnvBool("LOGGING_ONLY_MODEL_INTERACTIONS"), + OnlyModelInteractions: getEnvBoolOrDefault("LOGGING_ONLY_MODEL_INTERACTIONS", true),
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
config/config.godocker-compose.yaml
💤 Files with no reviewable changes (1)
- docker-compose.yaml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: All request/response payloads must use strongly-typed structs instead of interfaces{} or map[string]interface{} to catch errors at compile time
Use goroutines and channels for high-throughput scenarios (10k+ concurrent connections) to maintain concurrency design principles
Use the module namegomodelfor all imports and package references
Files:
config/config.go
config/config.go
📄 CodeRabbit inference engine (CLAUDE.md)
config/config.go: UseViperfor loading configuration from environment variables and .env files in the config package
At least one provider API key is required in the environment configuration to start the server
Files:
config/config.go
🧠 Learnings (2)
📚 Learning: 2025-12-28T22:10:31.364Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T22:10:31.364Z
Learning: Applies to config/config.go : Use `Viper` for loading configuration from environment variables and .env files in the config package
Applied to files:
config/config.go
📚 Learning: 2026-01-15T23:07:37.652Z
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/factory.go:112-143
Timestamp: 2026-01-15T23:07:37.652Z
Learning: Guideline: Do not use interface{} or map[string]interface{} for API request/response payload types. Prefer strongly-typed structs for API payload definitions to improve type safety, serialization, and documentation. Allow interface{} only in internal infrastructure code paths where pragmatic flexibility is necessary (e.g., to avoid import cycles or to handle highly dynamic internal contracts). In internal/auditlog/factory.go and similar non-API implementation files, applying this restriction is optional and should be evaluated on a case-by-case basis based on whether the type remains internal and does not define API boundary shapes.
Applied to files:
config/config.go
🪛 GitHub Actions: CI
config/config.go
[error] 436-436: golangci-lint: config/config.go:436:6: func getEnvBoolOrDefault is unused (unused)
⏰ 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). (1)
- GitHub Check: Analyze (go)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai rerun |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/config.go (1)
233-241: AlignOnlyModelInteractionsdefault across config paths.In the env-only path,
OnlyModelInteractionsnow defaults tofalsewhenLOGGING_ONLY_MODEL_INTERACTIONSis unset, but the struct comment and viper defaults still indicatetrue. This makes behavior dependent on whether a config file exists and can unintentionally broaden audit logging scope. Please align the defaults (either restoretruehere or update the viper defaults + doc comment tofalse).🔧 Option A: restore the documented default (true) in env-only path
} else { + onlyModelInteractions := true + if v, ok := os.LookupEnv("LOGGING_ONLY_MODEL_INTERACTIONS"); ok { + onlyModelInteractions = strings.EqualFold(v, "true") || v == "1" + } // No config file, use environment variables (legacy support) cfg = Config{ Server: ServerConfig{ Port: viper.GetString("PORT"), @@ Logging: LogConfig{ Enabled: getEnvBool("LOGGING_ENABLED"), StorageType: getEnvOrDefault("LOGGING_STORAGE_TYPE", "sqlite"), LogBodies: getEnvBool("LOGGING_LOG_BODIES"), LogHeaders: getEnvBool("LOGGING_LOG_HEADERS"), BufferSize: getEnvIntOrDefault("LOGGING_BUFFER_SIZE", 1000), FlushInterval: getEnvIntOrDefault("LOGGING_FLUSH_INTERVAL", 5), RetentionDays: getEnvIntOrDefault("LOGGING_RETENTION_DAYS", 30), - OnlyModelInteractions: getEnvBool("LOGGING_ONLY_MODEL_INTERACTIONS"), + OnlyModelInteractions: onlyModelInteractions, },
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
config/config.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: All request/response payloads must use strongly-typed structs instead of interfaces{} or map[string]interface{} to catch errors at compile time
Use goroutines and channels for high-throughput scenarios (10k+ concurrent connections) to maintain concurrency design principles
Use the module namegomodelfor all imports and package references
Files:
config/config.go
config/config.go
📄 CodeRabbit inference engine (CLAUDE.md)
config/config.go: UseViperfor loading configuration from environment variables and .env files in the config package
At least one provider API key is required in the environment configuration to start the server
Files:
config/config.go
🧠 Learnings (2)
📚 Learning: 2025-12-28T22:10:31.364Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T22:10:31.364Z
Learning: Applies to config/config.go : Use `Viper` for loading configuration from environment variables and .env files in the config package
Applied to files:
config/config.go
📚 Learning: 2026-01-15T23:07:37.652Z
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/factory.go:112-143
Timestamp: 2026-01-15T23:07:37.652Z
Learning: Guideline: Do not use interface{} or map[string]interface{} for API request/response payload types. Prefer strongly-typed structs for API payload definitions to improve type safety, serialization, and documentation. Allow interface{} only in internal infrastructure code paths where pragmatic flexibility is necessary (e.g., to avoid import cycles or to handle highly dynamic internal contracts). In internal/auditlog/factory.go and similar non-API implementation files, applying this restriction is optional and should be evaluated on a case-by-case basis based on whether the type remains internal and does not define API boundary shapes.
Applied to files:
config/config.go
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Good defaults have been added to config.go files for env variables.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/config.go`:
- Around line 236-237: The config defaults for LogBodies and LogHeaders are
currently set via getEnvBoolOrDefault("LOGGING_LOG_BODIES", true) and
getEnvBoolOrDefault("LOGGING_LOG_HEADERS", true) which contradicts the struct
comments and viper defaults and risks logging sensitive data; change the boolean
defaults to false (i.e., use getEnvBoolOrDefault(..., false)) so both LogBodies
and LogHeaders default to false unless explicitly enabled, and ensure any
related viper default logic (the Viper defaults that set these to false) remains
consistent with these changes.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
config/config.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: All request/response payloads must use strongly-typed structs instead of interfaces{} or map[string]interface{} to catch errors at compile time
Use goroutines and channels for high-throughput scenarios (10k+ concurrent connections) to maintain concurrency design principles
Use the module namegomodelfor all imports and package references
Files:
config/config.go
config/config.go
📄 CodeRabbit inference engine (CLAUDE.md)
config/config.go: UseViperfor loading configuration from environment variables and .env files in the config package
At least one provider API key is required in the environment configuration to start the server
Files:
config/config.go
🧠 Learnings (2)
📚 Learning: 2025-12-28T22:10:31.364Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T22:10:31.364Z
Learning: Applies to config/config.go : Use `Viper` for loading configuration from environment variables and .env files in the config package
Applied to files:
config/config.go
📚 Learning: 2026-01-15T23:07:37.652Z
Learnt from: SantiagoDePolonia
Repo: ENTERPILOT/GOModel PR: 33
File: internal/auditlog/factory.go:112-143
Timestamp: 2026-01-15T23:07:37.652Z
Learning: Guideline: Do not use interface{} or map[string]interface{} for API request/response payload types. Prefer strongly-typed structs for API payload definitions to improve type safety, serialization, and documentation. Allow interface{} only in internal infrastructure code paths where pragmatic flexibility is necessary (e.g., to avoid import cycles or to handle highly dynamic internal contracts). In internal/auditlog/factory.go and similar non-API implementation files, applying this restriction is optional and should be evaluated on a case-by-case basis based on whether the type remains internal and does not define API boundary shapes.
Applied to files:
config/config.go
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
config/config.go (1)
435-436: LGTM!The documentation update accurately describes the function behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| LogBodies: getEnvBoolOrDefault("LOGGING_LOG_BODIES", true), | ||
| LogHeaders: getEnvBoolOrDefault("LOGGING_LOG_HEADERS", true), |
There was a problem hiding this comment.
Defaulting to log bodies/headers raises security and consistency concerns.
-
Privacy/Compliance risk: The struct comment at line 46 explicitly warns that
LogBodies"may contain sensitive data (PII, API keys in prompts)". Defaulting this totruemeans sensitive user data is logged without explicit opt-in, which could violate GDPR/CCPA requirements. -
Documentation mismatch: The struct comments (lines 47-48, 52-53) state "Default: false" for both fields, but this code now defaults them to
true. -
Inconsistent behavior: When a config file exists, viper defaults (lines 184-185) set these to
false. When no config file exists (this code path), they now default totrue. This creates unpredictable behavior depending on config file presence.
Consider either:
- Keep defaults as
falsefor security-by-default, requiring explicit opt-in - If
trueis intentional, update struct comments and viper defaults to match
🔒 Suggested fix for security-by-default approach
- LogBodies: getEnvBoolOrDefault("LOGGING_LOG_BODIES", true),
- LogHeaders: getEnvBoolOrDefault("LOGGING_LOG_HEADERS", true),
+ LogBodies: getEnvBool("LOGGING_LOG_BODIES"),
+ LogHeaders: getEnvBool("LOGGING_LOG_HEADERS"),📝 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.
| LogBodies: getEnvBoolOrDefault("LOGGING_LOG_BODIES", true), | |
| LogHeaders: getEnvBoolOrDefault("LOGGING_LOG_HEADERS", true), | |
| LogBodies: getEnvBool("LOGGING_LOG_BODIES"), | |
| LogHeaders: getEnvBool("LOGGING_LOG_HEADERS"), |
🤖 Prompt for AI Agents
In `@config/config.go` around lines 236 - 237, The config defaults for LogBodies
and LogHeaders are currently set via getEnvBoolOrDefault("LOGGING_LOG_BODIES",
true) and getEnvBoolOrDefault("LOGGING_LOG_HEADERS", true) which contradicts the
struct comments and viper defaults and risks logging sensitive data; change the
boolean defaults to false (i.e., use getEnvBoolOrDefault(..., false)) so both
LogBodies and LogHeaders default to false unless explicitly enabled, and ensure
any related viper default logic (the Viper defaults that set these to false)
remains consistent with these changes.
There was a problem hiding this comment.
Pull request overview
This PR improves default configuration values for logging by enabling body and header logging by default when the corresponding environment variables are not set. This allows the docker-compose configuration to be simplified by removing redundant environment variable declarations.
Changes:
- Updated
LogBodiesandLogHeadersto default totruewhen environment variables are unset - Removed now-redundant logging environment variables from docker-compose.yaml
- Improved documentation for the
getEnvBoolOrDefaulthelper function
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docker-compose.yaml | Removed 5 logging-related environment variables that now use sensible defaults |
| config/config.go | Changed LogBodies and LogHeaders to use getEnvBoolOrDefault with true defaults; improved helper documentation |
Comments suppressed due to low confidence (2)
config/config.go:47
- The documentation comment states 'Default: false' but the code now sets the default to
true(line 236). This should be updated to 'Default: true' to reflect the actual behavior.
// LogBodies enables logging of full request/response bodies
// WARNING: May contain sensitive data (PII, API keys in prompts)
// Default: false
config/config.go:52
- The documentation comment states 'Default: false' but the code now sets the default to
true(line 237). This should be updated to 'Default: true' to reflect the actual behavior.
// LogHeaders enables logging of request/response headers
// Sensitive headers (Authorization, Cookie, etc.) are auto-redacted
// Default: false
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| viper.SetDefault("logging.storage_type", "sqlite") | ||
| viper.SetDefault("logging.log_bodies", false) | ||
| viper.SetDefault("logging.log_headers", false) | ||
| viper.SetDefault("logging.log_bodies", true) |
There was a problem hiding this comment.
Changing the default for LogBodies from false to true increases the risk of logging sensitive data (PII, API keys in prompts) as noted in the WARNING comment on line 46. While this may be intentional for "good defaults" in development, this change should be carefully considered for production environments. Consider whether the .env.template file should be updated to reflect this new default to prevent user confusion.
| viper.SetDefault("logging.log_bodies", true) | |
| viper.SetDefault("logging.log_bodies", false) |
Summary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.