feat: per-component jsonl logs #613
Conversation
|
If you would like me to review this PR, either tag me in a comment or check this box:
You can disable automatic comments on my settings page |
WalkthroughThis set of changes introduces a comprehensive, component-aware logging system to the Go backend. A new Sequence Diagram(s)Logger Creation and Component RegistrationsequenceDiagram
participant App as Application
participant Config as Config Loader
participant Logger as LoggerFactory
participant Registry as ComponentRegistry
App->>Config: LoadConfig()
App->>Logger: NewLoggerFactory(baseLogger, componentLogLevels)
Logger->>Registry: RegisterComponent(componentID, type, metadata)
App->>Logger: ForService("ai.completions")
Logger->>Registry: GetComponentLogLevel("ai.completions")
Logger->>Logger: Create logger with component context and log level
Logger-->>App: Component-scoped logger
Listing Registered Logging Components via CLIsequenceDiagram
participant User as User
participant CLI as Main.go
participant Logger as LoggerFactory
participant Registry as ComponentRegistry
User->>CLI: Run with --list-components
CLI->>Logger: NewLoggerFactory(baseLogger)
Logger->>Registry: ListComponents()
CLI->>User: Output components, log levels, enabled/disabled status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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 comments)
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: 10
🔭 Outside diff range comments (6)
backend/golang/pkg/bootstrap/fx/server.go (1)
118-129: PII leakage risk: raw GraphQL query and variables logged at error levelLogging oc.RawQuery and oc.Variables can expose secrets and user data. Keep error logs high-level and move detailed context to Debug with sanitization and truncation.
- logger.Error( - "gql error", - "operation_name", - oc.OperationName, - "raw_query", - oc.RawQuery, - "variables", - oc.Variables, - "errors", - resp.Errors, - ) + logger.Error( + "gql error", + "operation_name", oc.OperationName, + "errors", resp.Errors, + ) + // Emit detailed context at debug level with redaction and truncation + logger.Debug( + "gql error context", + "raw_query", truncateString(oc.RawQuery, 1024), + "variables", sanitizeVariables(oc.Variables), + )Add helper functions in this file (or a shared logging utils package):
// truncateString returns s truncated to max characters with an added ellipsis. func truncateString(s string, max int) string { if max <= 0 || len(s) <= max { return s } if max <= 3 { return s[:max] } return s[:max-3] + "..." } // sanitizeVariables redacts values for known sensitive keys (case-insensitive). func sanitizeVariables(v any) any { switch t := v.(type) { case map[string]any: out := make(map[string]any, len(t)) for k, val := range t { lk := strings.ToLower(k) if strings.Contains(lk, "password") || strings.Contains(lk, "token") || strings.Contains(lk, "secret") || strings.Contains(lk, "authorization") || strings.Contains(lk, "apikey") || strings.Contains(lk, "api_key") { out[k] = "***REDACTED***" } else { out[k] = sanitizeVariables(val) } } return out case []any: out := make([]any, len(t)) for i := range t { out[i] = sanitizeVariables(t[i]) } return out default: return t } }backend/golang/pkg/bootstrap/fx/ai.go (2)
118-126: Migrate ProvideAnonymizerManager to LoggerFactory for consistencyThis provider still injects and uses a raw
*log.Logger, diverging from the new per-component logging pattern. Recommend injecting*bootstrap.LoggerFactoryand deriving a scoped logger, e.g.ai.anonymizer, for uniformity, centralized log level control, and JSONL structure consistency.Apply this refactor within the function:
func ProvideAnonymizerManager( - lc fx.Lifecycle, - logger *log.Logger, + lc fx.Lifecycle, + loggerFactory *bootstrap.LoggerFactory, envs *config.Config, store *db.Store, completionsService *ai.Service, ) AnonymizerManagerResult { - logger.Info("Initializing anonymizer", "type", envs.AnonymizerType) + logger := loggerFactory.ForAI("ai.anonymizer") + logger.Info("Initializing anonymizer", "type", envs.AnonymizerType) @@ - localAnonymizer = ollama.NewOllamaClient("http://localhost:11435", "qwen3-0.6b-q4_k_m", logger) + localAnonymizer = ollama.NewOllamaClient("http://localhost:11435", "qwen3-0.6b-q4_k_m", logger) @@ - anonymizerManager = ai.NewLocalAnonymizerManager(localAnonymizer, store.DB().DB, logger) + anonymizerManager = ai.NewLocalAnonymizerManager(localAnonymizer, store.DB().DB, logger) @@ - anonymizerManager = ai.NewLLMAnonymizerManager(completionsService, envs.CompletionsModel, store.DB().DB, logger) + anonymizerManager = ai.NewLLMAnonymizerManager(completionsService, envs.CompletionsModel, store.DB().DB, logger)Note: Ensure the DI graph provides
LoggerFactoryhere (it appears available elsewhere).
189-197: Use LoggerFactory in PrivateCompletionsService to avoid mixed logging styles
PrivateCompletionsServiceParamsinjects a raw*log.Loggerand the provider uses it directly. Switch toLoggerFactoryand derive a scoped logger (e.g.,ai.private) to keep all AI-related logs component-aware.type PrivateCompletionsServiceParams struct { fx.In Lifecycle fx.Lifecycle - Logger *log.Logger + LoggerFactory *bootstrap.LoggerFactory CompletionsService *ai.Service AnonymizerManager *ai.AnonymizerManager `optional:"true"` } @@ func ProvidePrivateCompletionsService(params PrivateCompletionsServiceParams) (PrivateCompletionsServiceResult, error) { // Create private completions service if anonymizer is enabled var privateCompletionsService *ai.PrivateCompletionsService + logger := params.LoggerFactory.ForAI("ai.private") if params.AnonymizerManager != nil { var err error privateCompletionsService, err = ai.NewPrivateCompletionsService(ai.PrivateCompletionsConfig{ CompletionsService: params.CompletionsService, AnonymizerManager: params.AnonymizerManager, ExecutorWorkers: 1, - Logger: params.Logger, + Logger: logger, }) if err != nil { - params.Logger.Error("Failed to create private completions service", "error", err) + logger.Error("Failed to create private completions service", "error", err) return PrivateCompletionsServiceResult{}, err } @@ - params.Logger.Info("Private completions service enabled") + logger.Info("Private completions service enabled") @@ params.Lifecycle.Append(fx.Hook{ OnStop: func(ctx context.Context) error { - params.Logger.Info("Shutting down private completions service") + logger.Info("Shutting down private completions service") privateCompletionsService.Shutdown() return nil }, }) } else { - params.Logger.Info("Private completions service disabled (no anonymizer)") + logger.Info("Private completions service disabled (no anonymizer)") }Follow-up: Update any wiring expecting
Loggerto instead provideLoggerFactoryhere.Also applies to: 198-233
backend/golang/cmd/server/main.go (1)
106-114: Avoid logging full user profile (PII risk)
logger.Info("User profile", "profile", userProfile)may emit sensitive PII. Prefer logging a minimal set of non-sensitive fields (e.g., user ID) or a redacted summary.- logger.Info("User profile", "profile", userProfile) + logger.Info("User profile", "user_id", userProfile.ID)If a redaction helper exists, apply it here instead.
backend/golang/pkg/bootstrap/fx/temporal.go (2)
92-106: Remove raw logger from TemporalWorkerParams and use component-scoped loggers throughout
TemporalWorkerParamsstill injects*log.Loggerand multiple subsystems use it directly. Migrate toLoggerFactory-derived loggers for each sub-component to ensure proper component scoping and centralized level control.Suggested changes:
type TemporalWorkerParams struct { fx.In Lifecycle fx.Lifecycle - Logger *log.Logger LoggerFactory *bootstrap.LoggerFactory ... } func ProvideTemporalWorker(params TemporalWorkerParams) (TemporalWorkerResult, error) { logger := params.LoggerFactory.ForTemporal("temporal.worker") logger.Info("Creating Temporal worker") @@ - authActivities := auth.NewOAuthActivities(params.Store, params.Logger) + authLogger := params.LoggerFactory.ForComponent("auth.activities") + authActivities := auth.NewOAuthActivities(params.Store, authLogger) @@ - aiAgent := agent.NewAgent(params.Logger, params.NATSConn, params.CompletionsService, params.Config.CompletionsModel, params.Config.ReasoningModel, nil, nil) + agentLogger := params.LoggerFactory.ForComponent("agent.core") + aiAgent := agent.NewAgent(agentLogger, params.NATSConn, params.CompletionsService, params.Config.CompletionsModel, params.Config.ReasoningModel, nil, nil) - schedulerActivities := scheduler.NewTaskSchedulerActivities( - params.Logger, + schedulerLogger := params.LoggerFactory.ForComponent("scheduler.activities") + schedulerActivities := scheduler.NewTaskSchedulerActivities( + schedulerLogger, params.CompletionsService, aiAgent, params.ToolRegistry, params.Config.CompletionsModel, params.Store, params.NotificationsService, nil, ) @@ - identityActivities := identity.NewIdentityActivities(logger, params.Memory, params.CompletionsService, params.Config.CompletionsModel) + identityActivities := identity.NewIdentityActivities( + params.LoggerFactory.ForComponent("identity.activities"), + params.Memory, + params.CompletionsService, + params.Config.CompletionsModel, + )This retains a primary
temporal.workerlogger while giving submodules their own component IDs.Also applies to: 108-171
92-106: *Remove Raw log.Logger Injection in Fx ProvidersThe
TemporalWorkerParamsstruct in
backend/golang/pkg/bootstrap/fx/temporal.go(around line 96) still includes a
Logger *log.Loggerfield. After migrating to*bootstrap.LoggerFactory, all
downstream providers and structs should stop accepting raw*log.Logger—instead
callparams.LoggerFactory.For…(...)inside the constructor.Occurrences of raw
*log.Loggerthat need migration (onlyProvideLoggerand
ProvideLoggerFactoryininfrastructure.goshould keep accepting the raw logger):
- backend/golang/pkg/bootstrap/fx/logger.go
• line 13:logger *log.Logger
• line 17:func NewCharmLoggerWithComponent(logger *log.Logger, ...)- backend/golang/pkg/bootstrap/fx/temporal.go
• line 96:Logger *log.LoggerinTemporalWorkerParams- backend/golang/pkg/bootstrap/fx/ai.go
• line 121:logger *log.Logger
• line 193:Logger *log.Logger- backend/golang/pkg/bootstrap/fx/database.go
• line 111:logger *log.Logger
• line 155:logger *log.Logger- backend/golang/pkg/bootstrap/fx/infrastructure.go
• line 29:func ProvideLogger() (*log.Logger, error)
• line 48:func ProvideLoggerFactory(logger *log.Logger) (*bootstrap.LoggerFactory, error)Please remove all other raw-logger parameters and update those providers to use
the injectedLoggerFactoryinstead.
🧹 Nitpick comments (22)
backend/golang/pkg/bootstrap/component_registry.go (3)
216-226: Deterministic output: consider sorting component typesReturning types in map iteration order is nondeterministic. Sorting improves UX for CLI listing and tests.
You can collect the keys then sort.Slice or sort.SliceStable before returning. This adds a small import: sort.
285-300: Clarify semantics of “disabled” componentsSetting ErrorLevel still emits error logs. If “disabled” should suppress all logs, consider:
- Using a no-op logger wrapper, or
- Documenting that disabled components still emit errors.
Do you intend to keep error logs when disabled? If not, I can draft a small no-op logger adapter to drop all events.
302-310: Trim whitespace when parsing levelsMinor hardening to handle values like " info".
func parseLogLevel(levelStr string) log.Level { - level, err := log.ParseLevel(levelStr) + level, err := log.ParseLevel(strings.TrimSpace(levelStr)) if err != nil { // Default to InfoLevel for invalid levels return log.InfoLevel } return level }backend/golang/pkg/bootstrap/fx/logger.go (1)
60-71: Consider dialing down verbosity for Provided/InvokedFx Provided/Invoked events can be noisy at info level in large graphs. Consider Debug for these two to keep startup logs lean.
backend/golang/pkg/bootstrap/fx/tools.go (2)
82-83: Success log: consider adding component count details for repeat callsOptional: include previous vs new count if re-registering in different lifecycles.
51-53: Ensure ENV-compatible component IDsAll
ForComponentcalls use dot-separated IDs (e.g.tools.core,infrastructure.main, etc.). Since environment variables can’t contain dots, confirm that your bootstrap registry correctly translates between ENV names (which must use underscores) and these dot-notation IDs. If it doesn’t already, add or document a mapping that replaces underscores with dots when reading ENV values.– Example check: if you set “LOG_LEVEL=tools_core=debug” in your shell, the registry should map
tools_core→tools.corebefore passing it to the logger.backend/golang/pkg/holon/activities.go (1)
29-39: Optional: inject LoggerFactory and create a scoped logger hereTo fully align with the factory approach, accept a *bootstrap.LoggerFactory and derive a component logger (e.g., "holon.activities") inside the constructor.
Example:
func NewHolonSyncActivities(factory *bootstrap.LoggerFactory, manager *Manager) *HolonSyncActivities { logger := factory.ForComponent("holon.activities") return &HolonSyncActivities{logger: logger, manager: manager} }backend/golang/pkg/holon/service.go (4)
45-48: Add nil-guard and doc comment for the new constructorConsider guarding against a nil LoggerFactory to avoid panics and add a brief doc comment noting preferred usage vs NewServiceWithLogger.
func NewServiceWithLoggerFactory(store *db.Store, loggerFactory *bootstrap.LoggerFactory) *Service { - logger := loggerFactory.ForService("holon.service.main") + if loggerFactory == nil { + // Fallback to nil logger; service code handles nil loggers via checks. + return NewServiceWithConfig(store, nil, "") + } + logger := loggerFactory.ForService("holon.service.main") return NewServiceWithConfig(store, logger, "") }
221-244: Authentication refresh logs are clear; consider rate-limiting noisy pathsThe added Debug/Error logs are good. If this path can be noisy under token expiry, consider rate-limiting or aggregating to prevent log spam.
266-281: Thread processor init logging LGTM; consider adding component tagLooks good. If LoggerFactory embeds component metadata automatically, fine; otherwise consider attaching a constant component field to ensure consistent filtering.
310-311: Add basic thread metadata for troubleshootingInclude thread state or message count with the thread_id to speed up diagnosis.
- s.logger.Debug("Thread has no messages, processing anyway", "thread_id", threadID) + s.logger.Debug("Thread has no messages, processing anyway", "thread_id", threadID, "message_count", 0)backend/golang/pkg/bootstrap/fx/memory.go (1)
30-37: Switch to LoggerFactory is correct; align component naming conventionUsing loggerFactory.ForMemory is consistent. For consistency across the codebase, consider standardizing component IDs (e.g., "memory.evolving" vs "evolving.memory").
backend/golang/pkg/bootstrap/logger.go (1)
34-71: Improve defaults and resiliency in configurable logger
- Default formatter to JSON to match PR goal (per-component jsonl) unless explicitly set.
- Guard cfg nil to avoid panics.
- Emit a warning on invalid LogLevel (via fmt.Fprintf to stderr) since logger isn’t available yet.
- Replace TODO for file output with a rotating file writer (lumberjack) when LogOutput == "file".
func NewLogger(cfg *config.Config) *log.Logger { - var writer io.Writer + if cfg == nil { + cfg = &config.Config{} + } + var writer io.Writer if cfg.LogOutput == "file" { - // TODO: Implement file writer with rotation - writer = &customLogWriter{} + // TODO: make path configurable (e.g., cfg.LogOutputPath); fallback to stdout until added + writer = &customLogWriter{} } else { writer = &customLogWriter{} } level := log.InfoLevel if cfg.LogLevel != "" { if parsedLevel, err := log.ParseLevel(cfg.LogLevel); err == nil { level = parsedLevel + } else { + // no logger yet; emit minimal warning + _, _ = io.WriteString(os.Stderr, "WARN: invalid LogLevel '"+cfg.LogLevel+"', defaulting to INFO\n") } } - formatter := log.TextFormatter - switch cfg.LogFormat { - case "json": - formatter = log.JSONFormatter - case "logfmt": - formatter = log.LogfmtFormatter - } + // Default to JSON to ensure JSONL logs unless explicitly set otherwise + formatter := log.JSONFormatter + switch cfg.LogFormat { + case "text": + formatter = log.TextFormatter + case "logfmt": + formatter = log.LogfmtFormatter + } logger := log.NewWithOptions(writer, log.Options{ ReportCaller: true, ReportTimestamp: true, Level: level, TimeFormat: time.RFC3339, Formatter: formatter, }) if formatter == log.TextFormatter { logger.SetColorProfile(lipgloss.ColorProfile()) } return logger }If you’d like, I can also add a LogOutputPath to config and wire in lumberjack for rotation.
backend/golang/pkg/bootstrap/fx/server.go (1)
160-181: HTTP server logs LGTM; consider adding server timeoutsStartup/shutdown/error logs look good. Consider setting timeouts to reduce DoS risk.
Add on server creation:
server = &http.Server{ Addr: ":" + params.Config.GraphqlPort, Handler: params.Router, ReadTimeout: 15 * time.Second, WriteTimeout: 15 * time.Second, IdleTimeout: 60 * time.Second, ReadHeaderTimeout: 5 * time.Second, }backend/golang/pkg/bootstrap/fx/database.go (1)
56-63: Memory storage provider: LoggerFactory integration LGTMGood switch to factory. Minor: Consistent naming with other components (e.g., “memory.storage”) is fine as-is; ensure documented in component registry.
backend/golang/pkg/config/config.go (1)
170-176: Default to the global log level when no component override existsCurrent fallback is hard-coded "info". More intuitive is to use the configured global level (
c.LogLevel) when present.func (c *Config) GetComponentLogLevel(componentID string) string { if level, exists := c.ComponentLogLevels[componentID]; exists { return level } - return "info" + if c.LogLevel != "" { + return c.LogLevel + } + return "info" }backend/golang/pkg/bootstrap/logger_test.go (3)
68-85: Stabilize tests by explicitly using a text-format base loggerSeveral tests assert string containment on text output but construct the base logger with
NewBootstrapLogger(). If its default format changes, tests will become flaky. Prefer constructing a text logger explicitly for these tests.Example helper for tests:
func newTextBaseLogger() *log.Logger { return NewLogger(&config.Config{LogFormat: "text", LogLevel: "info"}) }And replace:
- baseLogger := NewBootstrapLogger() + baseLogger := newTextBaseLogger()Applies to tests that assert
assert.Contains(t, output, ...)on non-JSON output.Also applies to: 86-103, 104-129, 131-149, 151-169, 171-189
224-256: Environment override test: align comment and behaviorThe test sets env vars but then manually sets
cfg.LogFormat/cfg.LogLevel. Either:
- call
LoadConfigWithAutoDetection()and assert overrides, or- keep manual settings and update the comment to reflect that env vars aren’t used here.
297-335: Component stats/type assertions can be brittleRelying on
stats["total_components"]and presence of a specific type may be tightly coupled to registry internals. Prefer assertinglen(factory.GetComponentRegistry().ListComponents())for count, and explicitly registering/validating the expected type if needed.backend/golang/cmd/server/main.go (2)
81-83: Documentation example: prefer underscore-to-dot mapping clarificationThe example uses
LOG_LEVEL_holon_managerwhile components are registered with dot IDs (e.g.,holon.manager). Add a short note that underscores in env var names map to dots in component IDs.
129-139: Use component-aware logger for shutdown messages for consistencyThese lines use the package-level
log.Info/Error, which may diverge from configured format/fields. Prefer a component-scoped or base logger.Option: create a base logger early and reuse here, or derive:
shutdownLogger := bootstrap.NewBootstrapLogger() // or use a factory-scoped logger if accessible shutdownLogger.Info("Server shutting down...")backend/golang/pkg/bootstrap/fx/infrastructure.go (1)
29-46: Good error handling with bootstrap logger.The function properly handles configuration loading failures. The temporary LoggerFactory creation on lines 41-42 is used only for a single log message - consider whether this is necessary during bootstrap or if the base logger would suffice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (20)
backend/golang/README.md(1 hunks)backend/golang/cmd/server/main.go(1 hunks)backend/golang/pkg/bootstrap/component_registry.go(1 hunks)backend/golang/pkg/bootstrap/component_registry_test.go(1 hunks)backend/golang/pkg/bootstrap/fx/ai.go(3 hunks)backend/golang/pkg/bootstrap/fx/database.go(2 hunks)backend/golang/pkg/bootstrap/fx/infrastructure.go(6 hunks)backend/golang/pkg/bootstrap/fx/logger.go(1 hunks)backend/golang/pkg/bootstrap/fx/memory.go(2 hunks)backend/golang/pkg/bootstrap/fx/server.go(8 hunks)backend/golang/pkg/bootstrap/fx/services.go(17 hunks)backend/golang/pkg/bootstrap/fx/temporal.go(6 hunks)backend/golang/pkg/bootstrap/fx/tools.go(3 hunks)backend/golang/pkg/bootstrap/logger.go(2 hunks)backend/golang/pkg/bootstrap/logger_factory.go(1 hunks)backend/golang/pkg/bootstrap/logger_test.go(1 hunks)backend/golang/pkg/config/config.go(3 hunks)backend/golang/pkg/holon/activities.go(2 hunks)backend/golang/pkg/holon/manager.go(7 hunks)backend/golang/pkg/holon/service.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
backend/golang/pkg/holon/activities.go (1)
backend/golang/pkg/holon/manager.go (1)
Manager(24-38)
backend/golang/pkg/bootstrap/fx/server.go (2)
backend/golang/pkg/bootstrap/logger_factory.go (1)
LoggerFactory(8-11)backend/golang/pkg/config/config.go (1)
Config(14-43)
backend/golang/pkg/bootstrap/fx/infrastructure.go (3)
backend/golang/pkg/config/config.go (2)
LoadConfigWithAutoDetection(104-108)Config(14-43)backend/golang/pkg/bootstrap/logger.go (2)
NewBootstrapLogger(20-32)NewLogger(34-71)backend/golang/pkg/bootstrap/logger_factory.go (2)
NewLoggerFactoryWithConfig(22-30)LoggerFactory(8-11)
backend/golang/pkg/bootstrap/logger.go (1)
backend/golang/pkg/config/config.go (1)
Config(14-43)
backend/golang/pkg/bootstrap/fx/ai.go (2)
backend/golang/pkg/bootstrap/logger_factory.go (1)
LoggerFactory(8-11)backend/golang/pkg/config/config.go (1)
Config(14-43)
backend/golang/pkg/bootstrap/logger_factory.go (1)
backend/golang/pkg/bootstrap/component_registry.go (38)
ComponentRegistry(77-82)NewComponentRegistry(85-91)ComponentTypeUtility(27-27)ComponentTypeService(18-18)ComponentTypeManager(19-19)ComponentTypeHandler(20-20)ComponentTypeResolver(21-21)ComponentTypeRepository(22-22)ComponentTypeWorker(23-23)ComponentTypeClient(24-24)ComponentTypeServer(25-25)ComponentTypeMiddleware(26-26)ComponentTypeAI(30-30)ComponentTypeAnonymizer(31-31)ComponentTypeEmbedding(32-32)ComponentTypeCompletions(33-33)ComponentTypeProcessor(36-36)ComponentTypeWorkflow(37-37)ComponentTypeIntegration(38-38)ComponentTypeParser(39-39)ComponentTypeTelegram(42-42)ComponentTypeWhatsApp(43-43)ComponentTypeSlack(44-44)ComponentTypeGmail(45-45)ComponentTypeMCP(46-46)ComponentTypeDatabase(49-49)ComponentTypeNATS(50-50)ComponentTypeTemporal(51-51)ComponentTypeDirectory(52-52)ComponentTypeIdentity(55-55)ComponentTypeAuth(56-56)ComponentTypeOAuth(57-57)ComponentTypeChat(60-60)ComponentTypeMemory(61-61)ComponentTypeTwinChat(62-62)ComponentTypeTTS(63-63)ComponentType(14-14)ComponentInfo(67-74)
backend/golang/pkg/bootstrap/component_registry_test.go (1)
backend/golang/pkg/bootstrap/component_registry.go (36)
NewComponentRegistry(85-91)ComponentTypeManager(19-19)ComponentTypeAnonymizer(31-31)ComponentTypeService(18-18)ComponentTypeUtility(27-27)ComponentType(14-14)ComponentTypeHandler(20-20)ComponentTypeResolver(21-21)ComponentTypeRepository(22-22)ComponentTypeWorker(23-23)ComponentTypeClient(24-24)ComponentTypeServer(25-25)ComponentTypeMiddleware(26-26)ComponentTypeAI(30-30)ComponentTypeEmbedding(32-32)ComponentTypeCompletions(33-33)ComponentTypeProcessor(36-36)ComponentTypeWorkflow(37-37)ComponentTypeIntegration(38-38)ComponentTypeParser(39-39)ComponentTypeTelegram(42-42)ComponentTypeWhatsApp(43-43)ComponentTypeSlack(44-44)ComponentTypeGmail(45-45)ComponentTypeMCP(46-46)ComponentTypeDatabase(49-49)ComponentTypeNATS(50-50)ComponentTypeTemporal(51-51)ComponentTypeDirectory(52-52)ComponentTypeIdentity(55-55)ComponentTypeAuth(56-56)ComponentTypeOAuth(57-57)ComponentTypeChat(60-60)ComponentTypeMemory(61-61)ComponentTypeTwinChat(62-62)ComponentTypeTTS(63-63)
⏰ 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: lint-build-test
🔇 Additional comments (45)
backend/golang/pkg/bootstrap/fx/logger.go (1)
16-19: LGTM: fx logger now carries component contextDeriving a component-scoped logger up-front is clean and consistent with the new logging model.
backend/golang/pkg/bootstrap/fx/tools.go (3)
41-47: LoggerFactory injection: good migrationSwitching from raw *log.Logger to *bootstrap.LoggerFactory aligns with the new architecture.
55-67: Logger propagation to tools: OKPassing the component logger into memory search and scheduler tool is correct.
70-79: Telegram tool construction and registration logging: OKError paths log context; success path logs registry count. Looks good.
backend/golang/pkg/holon/activities.go (2)
8-10: LGTM: unify logger import to github.com/charmbracelet/logAlias removal improves consistency.
54-93: Activity logs are well-structuredGood separation of Info/Debug and error contexts.
backend/golang/README.md (1)
45-61: Great jq examplesFiltering by component and prefix is clear and matches the structured JSON output.
backend/golang/pkg/holon/service.go (3)
133-135: Good: structured error logging with contextSwitching to structured logging for failed view increments is clear and actionable.
319-341: Bootstrap processing logs read well; approveStart/finish/error logs with context are appropriate here.
362-365: Background processor init log LGTMGood visibility with the interval field.
backend/golang/pkg/bootstrap/logger.go (1)
20-32: Bootstrap logger: sensible defaults for JSONL; approveFixed Info level, RFC3339, JSON is appropriate for early bootstrap.
backend/golang/pkg/bootstrap/fx/server.go (5)
68-70: Scoped logger for GraphQL server: LGTMComponent name “graphql.server” is clear and consistent with “graphql.http” below.
80-92: Passing resolver logger: goodStructured logs at resolver-level will help correlate request flow.
137-139: Server creation message: approveGood to have a clear success log after wiring.
152-156: fx params changes: approveLifecycle, LoggerFactory, Config, Router injection looks correct.
52-64: LoggerFactory is properly provided by the InfrastructureModuleI’ve confirmed that backend/golang/pkg/bootstrap/fx/infrastructure.go registers
ProvideLoggerFactoryin itsfx.Providecall, which returns a*bootstrap.LoggerFactory. The DI swap is correctly wired and no further changes are needed.backend/golang/pkg/bootstrap/fx/database.go (1)
35-46: Embedding wrapper uses component-scoped logger: approveClear component id and structured logs on success/error.
backend/golang/pkg/bootstrap/fx/ai.go (2)
56-61: Scoped AI completions logger via LoggerFactory: LGTMGood use of
ForAI("ai.completions")to ensure consistent component context.
81-86: Scoped AI embeddings logger via LoggerFactory: LGTMConsistent with the new per-component logging approach.
backend/golang/pkg/config/config.go (1)
37-43: Logging config fields and initialization: LGTMGlobal log format/level/output and component overrides are correctly added and initialized.
Also applies to: 138-143
backend/golang/cmd/server/main.go (1)
96-104: fx event logger scoping: LGTMScoping fx events under
fx.frameworkimproves traceability.backend/golang/pkg/bootstrap/fx/temporal.go (3)
50-54: Temporal client logger via LoggerFactory: LGTMCorrectly scopes Temporal client logs with
ForTemporal("temporal.client").
188-201: Scheduler setup with component-scoped logger: LGTMGood use of
ForTemporal("temporal.scheduler")and structured error handling for schedule deletion/creation.Also applies to: 205-221
110-116: Lifecycle logs and worker start: LGTMLifecycle hooks and error logging follow the new scoped logger pattern. No issues.
Also applies to: 157-170
backend/golang/pkg/bootstrap/component_registry_test.go (9)
11-64: Well-structured test for flat hierarchy component registration.The test comprehensively covers component registration, retrieval, duplicate detection, and type filtering. Good use of metadata and various component types.
66-90: Comprehensive log level management testing.The test properly validates log level setting, retrieval, and default behavior for non-existent components.
92-115: Good test coverage for component enable/disable functionality.The test properly validates state changes and error handling for non-existent components.
117-137: Proper environment variable testing with good isolation.Excellent use of
t.Setenvfor test isolation. The environment variable naming conventionLOG_LEVEL_<component_id>is clear and consistent.
139-160: Clean test for config-based log level loading.The test effectively validates loading log levels from a configuration map.
162-180: Good test for component statistics functionality.The test validates statistics collection including totals and per-type breakdowns. Consider adding a check for the type assertion success on line 177 for robustness.
182-195: Effective test for component-aware logger creation.Good approach using nil writer for testing logger configuration without actual I/O.
197-208: Good test for invalid log level handling.The test properly validates that invalid log levels gracefully fall back to InfoLevel.
210-265: Comprehensive test coverage for all component types.Excellent test that validates all 34 defined component types work correctly with the registry. The systematic approach ensures no component type is overlooked.
backend/golang/pkg/holon/manager.go (3)
76-86: Proper integration with the new LoggerFactory pattern.The transition to LoggerFactory is well-implemented, maintaining backward compatibility with the
EnableLoggingconfiguration option.
136-169: Excellent structured logging with operation context.The addition of
"operation"fields to all log calls provides excellent traceability. The operation names are clear and follow a consistent naming pattern.
199-235: Comprehensive OAuth event logging with proper context.The OAuth token refresh event handling includes detailed logging with operation context, making it easier to debug authentication flows.
backend/golang/pkg/bootstrap/fx/infrastructure.go (3)
48-56: Acceptable approach to avoid circular dependencies.The comment clearly explains why config is loaded again. This is a reasonable trade-off to maintain clean dependency injection without circular dependencies.
58-68: Consistent component-aware logging pattern.The function properly uses LoggerFactory to create a scoped logger for config operations.
77-162: Excellent consistent adoption of LoggerFactory pattern.All infrastructure providers now use component-specific loggers with clear, descriptive component IDs. The lifecycle hooks properly use these scoped loggers for all operations.
backend/golang/pkg/bootstrap/fx/services.go (2)
57-316: Consistent LoggerFactory adoption across all service providers.All service providers properly use LoggerFactory to create component-specific loggers. The component IDs are descriptive and follow a consistent naming pattern.
406-500: Well-structured logging for background services and tools.Background services and tool registration use appropriate component loggers with comprehensive logging at suitable levels.
backend/golang/pkg/bootstrap/logger_factory.go (4)
7-30: Well-designed LoggerFactory with flexible initialization.The dual constructor pattern provides flexibility for initialization with or without pre-configured component log levels.
32-219: Comprehensive component-specific logger methods.The extensive set of ForXxx methods provides type-safe logger creation for all component types. Ignoring registration errors is appropriate for idempotent operations.
221-247: Well-designed context augmentation methods.Good defensive programming in
WithErrorwith nil checking. The field names follow logging best practices for searchability and consistency.
249-287: Clean API for component registry management.The methods provide convenient access to registry features while maintaining good encapsulation. The API is intuitive and consistent.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
backend/golang/pkg/bootstrap/logger_factory.go (4)
41-57: Reduce duplication across ForX helpers by delegating to a single helperAll ForX functions share the exact same pattern. Delegate each wrapper to forType for better maintainability and consistency.
Apply this representative refactor (repeat for the remaining ForX methods):
@@ func (lf *LoggerFactory) ForService(id string) *log.Logger { - _ = lf.componentRegistry.RegisterComponent(id, ComponentTypeService, nil) - return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id) + return lf.forType(id, ComponentTypeService, nil) } @@ func (lf *LoggerFactory) ForManager(id string) *log.Logger { - _ = lf.componentRegistry.RegisterComponent(id, ComponentTypeManager, nil) - return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id) + return lf.forType(id, ComponentTypeManager, nil) } @@ func (lf *LoggerFactory) ForHandler(id string) *log.Logger { - _ = lf.componentRegistry.RegisterComponent(id, ComponentTypeHandler, nil) - return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id) + return lf.forType(id, ComponentTypeHandler, nil) } @@ func (lf *LoggerFactory) ForResolver(id string) *log.Logger { - _ = lf.componentRegistry.RegisterComponent(id, ComponentTypeResolver, nil) - return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id) + return lf.forType(id, ComponentTypeResolver, nil) } @@ func (lf *LoggerFactory) ForRepository(id string) *log.Logger { - _ = lf.componentRegistry.RegisterComponent(id, ComponentTypeRepository, nil) - return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id) + return lf.forType(id, ComponentTypeRepository, nil) }If you foresee metadata being useful (db name, queue, workflow, model), consider adding sibling helpers like ForServiceWithMeta(id, meta) to pass through metadata now that the hook exists.
Also applies to: 71-75, 77-87, 89-99, 101-109, 111-120, 122-130, 132-135, 137-146, 148-156, 158-161, 163-172, 174-182, 184-193, 195-198, 200-209, 211-219
21-30: Load env-defined log levels after config to establish sane precedenceThe PR claims dynamic config via env. Consider applying env overrides by default in the WithConfig ctor.
Apply this diff:
func NewLoggerFactoryWithConfig(baseLogger *log.Logger, componentLogLevels map[string]string) *LoggerFactory { registry := NewComponentRegistry() registry.LoadLogLevelsFromConfig(componentLogLevels) + // Allow environment variables to override config-provided levels + registry.LoadLogLevelsFromEnv() return &LoggerFactory{ baseLogger: baseLogger, componentRegistry: registry, } }If you need different precedence, expose an option flag or document the current behavior explicitly.
231-234: PII caution for user_idIf userID can be an email/phone or other PII, ensure it’s a stable pseudonymous identifier or is gated by environment to avoid leaking PII in shared logs.
274-277: Prefer a typed stats struct over map[string]interface{}Returning a concrete struct (e.g., ComponentStats) improves discoverability and compiler assistance. Keep a map for JSON serialization if needed, but expose typed API internally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
backend/golang/README.md(1 hunks)backend/golang/cmd/server/main.go(1 hunks)backend/golang/pkg/bootstrap/component_registry.go(1 hunks)backend/golang/pkg/bootstrap/logger_factory.go(1 hunks)backend/golang/pkg/config/config.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/golang/README.md
- backend/golang/cmd/server/main.go
- backend/golang/pkg/config/config.go
- backend/golang/pkg/bootstrap/component_registry.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/golang/pkg/bootstrap/logger_factory.go (1)
backend/golang/pkg/bootstrap/component_registry.go (38)
ComponentRegistry(77-82)NewComponentRegistry(85-91)ComponentTypeUtility(27-27)ComponentTypeService(18-18)ComponentTypeManager(19-19)ComponentTypeHandler(20-20)ComponentTypeResolver(21-21)ComponentTypeRepository(22-22)ComponentTypeWorker(23-23)ComponentTypeClient(24-24)ComponentTypeServer(25-25)ComponentTypeMiddleware(26-26)ComponentTypeAI(30-30)ComponentTypeAnonymizer(31-31)ComponentTypeEmbedding(32-32)ComponentTypeCompletions(33-33)ComponentTypeProcessor(36-36)ComponentTypeWorkflow(37-37)ComponentTypeIntegration(38-38)ComponentTypeParser(39-39)ComponentTypeTelegram(42-42)ComponentTypeWhatsApp(43-43)ComponentTypeSlack(44-44)ComponentTypeGmail(45-45)ComponentTypeMCP(46-46)ComponentTypeDatabase(49-49)ComponentTypeNATS(50-50)ComponentTypeTemporal(51-51)ComponentTypeDirectory(52-52)ComponentTypeIdentity(55-55)ComponentTypeAuth(56-56)ComponentTypeOAuth(57-57)ComponentTypeChat(60-60)ComponentTypeMemory(61-61)ComponentTypeTwinChat(62-62)ComponentTypeTTS(63-63)ComponentType(14-14)ComponentInfo(67-74)
⏰ 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: lint-build-test
🔇 Additional comments (2)
backend/golang/pkg/bootstrap/logger_factory.go (2)
7-11: Solid factory abstraction centralizing component-aware loggingThe core idea is sound and aligns with per-component JSONL goals: single place to create scoped loggers, consistent fields, registry-backed level control. Nice foundation.
14-19: Validate baseLogger is non-nil (fail fast)A nil base logger would panic later. Either document the precondition or add a guard.
Apply this minimal guard if acceptable:
func NewLoggerFactory(baseLogger *log.Logger) *LoggerFactory { + if baseLogger == nil { + panic("NewLoggerFactory: baseLogger must not be nil") + } return &LoggerFactory{ baseLogger: baseLogger, componentRegistry: NewComponentRegistry(), } }If panicking is undesirable, return a default logger or change the constructor to return (*LoggerFactory, error).
|
|
||
| // ForComponent creates a logger for a specific component. | ||
| func (lf *LoggerFactory) ForComponent(id string) *log.Logger { | ||
| // Register the component if not already registered | ||
| _ = lf.componentRegistry.RegisterComponent(id, ComponentTypeUtility, nil) | ||
|
|
||
| // Get logger with proper level and context | ||
| return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t ignore registration errors; centralize register+get and support metadata
Right now, registration errors are dropped and the pattern is duplicated across all ForX methods. Centralize this path and at least warn on registration failures so you don’t silently lose component semantics/levels.
Apply this diff to add a private helper and use it in ForComponent:
@@
+// forType centralizes registration + logger retrieval and handles errors consistently.
+func (lf *LoggerFactory) forType(id string, t ComponentType, metadata map[string]interface{}) *log.Logger {
+ if err := lf.componentRegistry.RegisterComponent(id, t, metadata); err != nil {
+ // Best-effort: still return a logger, but surface the failure once.
+ lf.baseLogger.With(
+ "component", id,
+ "component_type", t,
+ "err", err,
+ ).Warn("logger_factory: failed to register component")
+ }
+ return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id)
+}
@@
-// ForComponent creates a logger for a specific component.
+// ForComponent creates a logger for a specific component (defaults to utility type).
func (lf *LoggerFactory) ForComponent(id string) *log.Logger {
- // Register the component if not already registered
- _ = lf.componentRegistry.RegisterComponent(id, ComponentTypeUtility, nil)
-
- // Get logger with proper level and context
- return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id)
+ return lf.forType(id, ComponentTypeUtility, nil)
}This eliminates error ignoring and creates one place to evolve behavior (e.g., metadata, disabled handling).
🤖 Prompt for AI Agents
In backend/golang/pkg/bootstrap/logger_factory.go around lines 31 to 39, the
ForComponent method currently ignores errors from component registration and
duplicates this pattern across similar methods. To fix this, create a private
helper method that centralizes the registration and logger retrieval logic,
capturing and logging any registration errors instead of ignoring them. Then
refactor ForComponent to use this helper, ensuring consistent error handling and
easier future enhancements like metadata support.
| // ForComponent creates a logger for a specific component. | ||
| func (lf *LoggerFactory) ForComponent(id string) *log.Logger { | ||
| // Register the component if not already registered | ||
| _ = lf.componentRegistry.RegisterComponent(id, ComponentTypeUtility, nil) | ||
|
|
||
| // Get logger with proper level and context | ||
| return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Repo-wide verification: ensure everything uses LoggerFactory and not raw charm loggers
To fully realize per-component logs, direct instantiation of charm loggers should be eliminated in favor of LoggerFactory. Run this to find stragglers.
Also applies to: 41-87, 89-219, 221-293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for direct charm logger instantiations (should be none):"
rg -n -S 'charmbracelet/log' -A 3 | rg -n -S 'log\.New\(|log\.NewWithOptions\(' || true
echo
echo "Searching for loggers created without LoggerFactory in Go files:"
rg -n -S 'log\.New\(|log\.NewWithOptions\(' --glob '!**/vendor/**' --glob '!**/node_modules/**' || true
echo
echo "Searching for components registered without LoggerFactory (if any custom registry usage exists):"
rg -n -S 'RegisterComponent\(' || trueLength of output: 19057
Found direct charm logger instantiations outside LoggerFactory
The grep run uncovered multiple calls to log.New(…) and log.NewWithOptions(…) that bypass your new LoggerFactory. In production code you still have:
- backend/golang/pkg/bootstrap/logger.go (lines 21 & 58)
- backend/golang/pkg/ai/anonymizer_manager.go (line 81)
- backend/golang/cmd/tee-api/main.go (line 22)
Additionally, there are dozens of test files (e.g., *.go under pkg/testing, pkg/dataprocessing, pkg/agent, etc.) instantiating raw loggers.
Please replace these with calls to your LoggerFactory (or, for tests, inject a test‐scoped factory) so all components get the correct per-component logger.
🤖 Prompt for AI Agents
In backend/golang/pkg/bootstrap/logger.go at lines 21 and 58,
backend/golang/pkg/ai/anonymizer_manager.go at line 81, and
backend/golang/cmd/tee-api/main.go at line 22, replace all direct calls to
log.New and log.NewWithOptions with calls to LoggerFactory.ForComponent to
ensure consistent logger creation. For test files under pkg/testing,
pkg/dataprocessing, pkg/agent, and others, inject a test-scoped LoggerFactory
and use it to create loggers instead of direct instantiations. This will
centralize logger creation and maintain proper per-component logging
configuration.
| // WithError adds error context to a logger. | ||
| func (lf *LoggerFactory) WithError(logger *log.Logger, err error) *log.Logger { | ||
| if err != nil { | ||
| return logger.With("error", err.Error(), "error_type", "error") | ||
| } | ||
| return logger | ||
| } |
There was a problem hiding this comment.
Preserve error type and structure in logs; avoid static error_type
Current WithError serializes only err.Error() and sets a constant error_type. Log the error itself and include its dynamic type to keep structure and triage fidelity.
Apply this diff:
@@
-import (
- "github.com/charmbracelet/log"
-)
+import (
+ "fmt"
+ "github.com/charmbracelet/log"
+)
@@
func (lf *LoggerFactory) WithError(logger *log.Logger, err error) *log.Logger {
if err != nil {
- return logger.With("error", err.Error(), "error_type", "error")
+ return logger.With(
+ "err", err, // keep structured error value
+ "error_kind", fmt.Sprintf("%T", err), // concrete error type
+ )
}
return logger
}Optional: if you have a global scrubbing policy for PII/secrets in error messages, integrate it here to centralize redaction.
Also applies to: 3-5
🤖 Prompt for AI Agents
In backend/golang/pkg/bootstrap/logger_factory.go around lines 236 to 242, the
WithError method currently logs only the error message string and sets a static
"error_type" value, losing the original error's type and structure. Modify the
method to log the entire error object itself and dynamically capture the error's
actual type for the "error_type" field. Additionally, if there is a global
policy for scrubbing PII or secrets from error messages, integrate that
redaction logic here to ensure centralized and consistent sanitization of error
logs.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
backend/golang/cmd/server/main.go (1)
28-30: Previous concern addressed: env-based overrides now loadedCalling
loggerFactory.LoadLogLevelsFromEnv()ensuresLOG_LEVEL_*overrides affect output of--list-components. Thanks for adding this.
🧹 Nitpick comments (6)
backend/golang/cmd/server/main.go (3)
63-67: Clarify env var format: underscores map to dots in component IDsEnvironment variable names can’t contain dots, so users must replace dots in component IDs with underscores. Make this explicit in the help text and show the mapping.
Apply:
- fmt.Println("\nTo set component log levels:") - fmt.Println(" export LOG_LEVEL_<COMPONENT_NAME>=<LEVEL>") - fmt.Println(" Example: export LOG_LEVEL_HOLON_MANAGER=debug") + fmt.Println("\nTo set component log levels via environment variables:") + fmt.Println(" export LOG_LEVEL_<COMPONENT_ID_WITH_DOTS_REPLACED_BY_UNDERSCORES>=<LEVEL>") + fmt.Println(" Example: export LOG_LEVEL_HOLON_MANAGER=debug # maps to component ID 'holon.manager'")
92-97: Bound the identity profile call with a timeoutAvoid potential hangs during startup by using a short timeout context.
Apply:
- userProfile, err := identityService.GetUserProfile(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + userProfile, err := identityService.GetUserProfile(ctx)Add import:
import "time"
115-115: Emit shutdown log with component contextKeep logs component-scoped even outside the DI boundary. If a factory-scoped logger isn’t available here, at least add the component field.
- log.Info("Server shutting down...") + log.Info("Server shutting down...", "component", "main.shutdown")backend/golang/Makefile (1)
70-73: Add formatting and declare as PHONY for reliability
- Run gofmt on the generated file for consistency.
- Declare the target as .PHONY to avoid name collisions.
Apply:
components-generate: @echo "Generating component registry from codebase..." go run tools/component-generator/main.go . + gofmt -s -w pkg/bootstrap/components_generated.goOutside these lines, also add:
.PHONY: components-generate buildbackend/golang/tools/component-generator/main.go (2)
246-253: Format generated code before writing for stable, idiomatic outputEnsures deterministic diffs and nicer output.
- // Write to file - err = os.WriteFile(outputFile, []byte(builder.String()), 0o644) + // Format and write to file + src := []byte(builder.String()) + if formatted, errFmt := format.Source(src); errFmt == nil { + src = formatted + } + err = os.WriteFile(outputFile, src, 0o644) if err != nil { return fmt.Errorf("failed to write file: %w", err) }
116-121: Receiver name ‘loggerFactory’ is too brittle; consider type-checkingRequiring the receiver identifier to be literally “loggerFactory” will miss calls like lf.ForService("…"). Consider using go/packages to type-check that sel.X is *bootstrap.LoggerFactory instead of matching identifier names.
Happy to provide a typed discovery snippet using golang.org/x/tools/go/packages if you want to make this robust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
.github/workflows/golang-ci.yml(1 hunks)backend/golang/Makefile(2 hunks)backend/golang/cmd/server/main.go(1 hunks)backend/golang/pkg/bootstrap/components_generated.go(1 hunks)backend/golang/pkg/bootstrap/components_generated_test.go(1 hunks)backend/golang/tools/component-generator/main.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/golang/pkg/bootstrap/components_generated.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: lint-build-test
🔇 Additional comments (5)
backend/golang/cmd/server/main.go (2)
22-27: Good, minimal and self-contained component listerThe bootstrap logger + factory approach for
showComponents()is clean and keeps this path independent of full app config.
28-33: I’m gathering how components get default log levels during registration. Running a quick search forNewLoggerFactoryandForAI…backend/golang/Makefile (1)
12-12: Good: build depends on components generationEnsures the registry is regenerated before building.
backend/golang/pkg/bootstrap/components_generated_test.go (2)
9-24: Solid sanity check of generated registrationsCovers basic presence and metadata; good guard against empty or broken generation.
104-134: Good idempotency testVerifies re-registration is a no-op with respect to count and type.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/golang/pkg/bootstrap/components_generated_test.go (1)
44-51: Good: guard against self-referential source paths to the generated fileThis addresses the prior review feedback and prevents discovery loops.
🧹 Nitpick comments (3)
backend/golang/pkg/bootstrap/components_generated_test.go (3)
32-34: Nit: make the empty-type check explicit to the ComponentType typeUse a typed zero value for clarity and future-proofing.
- if comp.Type == "" { + if comp.Type == ComponentType("") { t.Errorf("Component %s has empty Type", comp.ID) }
68-81: Small consistency improvement: assert sizes match before key-by-key checksQuickly surfaces gross mismatches and speeds triage.
func TestComponentCatalogConsistency(t *testing.T) { // Ensure ComponentCatalog and ComponentSources have the same keys + if len(ComponentCatalog) != len(ComponentSources) { + t.Errorf("Catalog/Sources length mismatch: catalog=%d sources=%d", len(ComponentCatalog), len(ComponentSources)) + } for id := range ComponentCatalog { if _, exists := ComponentSources[id]; !exists { t.Errorf("Component %s exists in ComponentCatalog but not in ComponentSources", id) } }
97-104: Improve diagnostics when expected types are missingLog the set of present types to make failures actionable.
- for _, expectedType := range expectedTypes { + // Build a histogram of present types for better error messages. + presentTypes := map[ComponentType]int{} + for _, c := range registry.ListComponents() { + presentTypes[c.Type]++ + } + + for _, expectedType := range expectedTypes { components := registry.ListComponentsByType(expectedType) if len(components) == 0 { - t.Errorf("Expected to find components of type %s, but found none", expectedType) + t.Errorf("Expected to find components of type %s, but found none. Present types: %+v", expectedType, presentTypes) } else { t.Logf("Found %d components of type %s", len(components), expectedType) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
backend/golang/pkg/bootstrap/components_generated.go(1 hunks)backend/golang/pkg/bootstrap/components_generated_test.go(1 hunks)backend/golang/tools/component-generator/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/golang/pkg/bootstrap/components_generated.go
- backend/golang/tools/component-generator/main.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: lint-build-test
🔇 Additional comments (2)
backend/golang/pkg/bootstrap/components_generated_test.go (2)
53-63: Default level assertion can be configuration-sensitiveIf the factory honors env/config (global or per-component), this can fail under certain CI/job settings. Consider explicitly fixing test config or overriding env for deterministic behavior.
If env is used, wrap the test with t.Setenv for the relevant variables (once identified). See the discovery script in my earlier comment.
10-16: No action needed: default log level is deterministic in testsThe tests use NewBootstrapLogger (which hardcodes InfoLevel) and NewLoggerFactory (which does not call LoadLogLevelsFromEnv) so environment variables cannot affect the default log level in components_generated_test.go. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
backend/golang/pkg/agent/interface_test.go (1)
24-24: Adopt test logger consistently across this fileNow that tests use the centralized test logger here, consider migrating the remaining direct logger initializations to testutils.GetTestLogger for consistency and to keep test output quiet (see Line 115 and Line 148).
backend/golang/pkg/dataprocessing/misc/misc_test.go (1)
32-33: Use test logger across all subtests for consistencyNice move to GetTestLogger. Consider updating the other logger initializations in this file to use the test logger too to reduce noise and align with the new logging approach (e.g., Line 88-89, Line 142-144, Line 214-216).
backend/golang/pkg/logging/logger.go (3)
14-18: Drop the custom writer; use os.Stdout directlyThe customLogWriter simply forwards to os.Stdout. You can simplify by passing os.Stdout directly to log.NewWithOptions.
-type customLogWriter struct{} - -func (w *customLogWriter) Write(p []byte) (n int, err error) { - return os.Stdout.Write(p) -} +// Directly use os.Stdout for output; a dedicated writer can be added when file rotation is implemented.
21-33: Simplify NewBootstrapLogger writerPass os.Stdout directly.
-func NewBootstrapLogger() *log.Logger { - logger := log.NewWithOptions(&customLogWriter{}, log.Options{ +func NewBootstrapLogger() *log.Logger { + logger := log.NewWithOptions(os.Stdout, log.Options{
36-44: Clarify behavior for cfg.LogOutput == "file"Currently the "file" branch falls back to stdout silently. Consider adding a TODO comment clarifying the fallback and/or returning an error from a higher layer if a file path isn’t supported yet.
backend/golang/pkg/logging/component_registry.go (1)
379-384: Clarify “disable logging” behaviorSetting Error level doesn’t fully disable logging; it suppresses debug/info/warn but still allows errors. Update the comment to reflect actual behavior or set a stricter level if true disable is desired.
- if !enabled { - // Set to highest level to effectively disable logging - logger.SetLevel(log.ErrorLevel) + if !enabled { + // Suppress lower-severity logs for disabled components; errors may still be emitted. + logger.SetLevel(log.ErrorLevel)backend/golang/pkg/logging/factory.go (3)
213-216: Naming nit: WithContext may be confused with context.ContextProvide a non-ambiguous alias to avoid confusion with Go’s context.
func (lf *Factory) WithContext(logger *log.Logger, key string, value interface{}) *log.Logger { return logger.With(key, value) } + +// WithField is an alias for WithContext to avoid confusion with context.Context semantics. +func (lf *Factory) WithField(logger *log.Logger, key string, value interface{}) *log.Logger { + return logger.With(key, value) +}
229-240: Standardize error field key to "err"Charmbracelet/log commonly uses the "err" field. Also keep type for filtering.
- return logger.With("error", err, "error_type", errorType) + return logger.With("err", err, "error_type", errorType)Optional follow-up: include unwrapped cause type if available.
247-251: Encapsulation: avoid exposing the raw registryReturning the concrete *ComponentRegistry leaks internals. Prefer exposing a narrow interface or keep only the wrapper methods you already provide.
Would you accept an interface (read-only where possible) and keep Set/Get/Enable/Is methods here to preserve invariants?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (14)
backend/golang/cmd/tee-api/main.go(3 hunks)backend/golang/pkg/agent/interface_test.go(2 hunks)backend/golang/pkg/ai/anonymizer_manager.go(3 hunks)backend/golang/pkg/bootstrap/component_registry.go(1 hunks)backend/golang/pkg/bootstrap/components_generated.go(1 hunks)backend/golang/pkg/bootstrap/logger.go(1 hunks)backend/golang/pkg/bootstrap/logger_factory.go(1 hunks)backend/golang/pkg/bootstrap/logger_test.go(1 hunks)backend/golang/pkg/dataprocessing/misc/misc_test.go(2 hunks)backend/golang/pkg/logging/component_registry.go(1 hunks)backend/golang/pkg/logging/factory.go(1 hunks)backend/golang/pkg/logging/logger.go(1 hunks)backend/golang/pkg/testing/personalities/personality_test.go(1 hunks)backend/golang/pkg/testing/test_logger.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- backend/golang/cmd/tee-api/main.go
- backend/golang/pkg/bootstrap/logger.go
- backend/golang/pkg/bootstrap/logger_factory.go
- backend/golang/pkg/bootstrap/component_registry.go
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/golang/pkg/bootstrap/components_generated.go
- backend/golang/pkg/bootstrap/logger_test.go
🧰 Additional context used
🧬 Code Graph Analysis (7)
backend/golang/pkg/agent/interface_test.go (1)
backend/golang/pkg/testing/test_logger.go (1)
GetTestLogger(16-18)
backend/golang/pkg/dataprocessing/misc/misc_test.go (1)
backend/golang/pkg/testing/test_logger.go (1)
GetTestLogger(16-18)
backend/golang/pkg/ai/anonymizer_manager.go (2)
backend/golang/pkg/bootstrap/logger_factory.go (1)
LoggerFactory(11-11)backend/golang/pkg/logging/factory.go (1)
Factory(10-13)
backend/golang/pkg/logging/logger.go (1)
backend/golang/pkg/bootstrap/logger.go (2)
NewBootstrapLogger(11-13)NewLogger(16-18)
backend/golang/pkg/testing/test_logger.go (1)
backend/golang/pkg/logging/factory.go (2)
Factory(10-13)NewFactory(16-21)
backend/golang/pkg/testing/personalities/personality_test.go (1)
backend/golang/pkg/testing/test_logger.go (1)
GetTestLogger(16-18)
backend/golang/pkg/logging/component_registry.go (2)
backend/golang/pkg/bootstrap/component_registry.go (4)
ComponentType(11-11)ComponentInfo(14-14)NewComponentRegistry(17-19)ComponentRegistry(8-8)backend/golang/pkg/dataprocessing/chatgpt/chatgpt.go (1)
Metadata(55-55)
⏰ 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: lint-build-test
🔇 Additional comments (7)
backend/golang/pkg/agent/interface_test.go (1)
12-12: Good switch to centralized test logger importImporting test logger utils with an alias avoids name collisions and keeps call sites explicit.
backend/golang/pkg/dataprocessing/misc/misc_test.go (1)
13-13: Centralized test logger import looks goodUsing the shared test logger utility will prevent noisy output during tests.
backend/golang/pkg/ai/anonymizer_manager.go (2)
24-29: Addition of LoggerFactory is good — ensure it’s actually wired by callersThe new LoggerFactory field enables component-scoped logging. Verify that constructors or callers supply this when used in production paths so the fallback to log.New(nil) doesn’t silently drop per-component config.
84-89: Ignore theForAnonymizerchange—they’re using the correct factory methodThe
logging.Factorytype defines and exercises aForAnonymizer(id string)method (see factory.go:116), and the bootstrap code even invokes it in components_generated.go. No replacement withForComponentis required here.Likely an incorrect or invalid review comment.
backend/golang/pkg/logging/factory.go (3)
46-55: Entry points for Utility look goodThe Utility-scoped helpers are clear and consistent.
252-266: Per-component level APIs look goodWrapper methods keep the factory as the single façade for logging configuration.
287-291: Env loader passthrough is fineStraightforward delegation to the registry.
| func NewFactory(baseLogger *log.Logger) *Factory { | ||
| return &Factory{ | ||
| baseLogger: baseLogger, | ||
| componentRegistry: NewComponentRegistry(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against nil baseLogger to prevent panics
Constructors should fail fast if a nil logger is passed.
Apply this diff:
func NewFactory(baseLogger *log.Logger) *Factory {
+ if baseLogger == nil {
+ panic("logging.Factory: baseLogger cannot be nil")
+ }
return &Factory{
baseLogger: baseLogger,
componentRegistry: NewComponentRegistry(),
}
}📝 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 NewFactory(baseLogger *log.Logger) *Factory { | |
| return &Factory{ | |
| baseLogger: baseLogger, | |
| componentRegistry: NewComponentRegistry(), | |
| } | |
| } | |
| // file: backend/golang/pkg/logging/factory.go | |
| func NewFactory(baseLogger *log.Logger) *Factory { | |
| if baseLogger == nil { | |
| panic("logging.Factory: baseLogger cannot be nil") | |
| } | |
| return &Factory{ | |
| baseLogger: baseLogger, | |
| componentRegistry: NewComponentRegistry(), | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/golang/pkg/logging/factory.go around lines 16 to 21, the NewFactory
constructor does not check if the baseLogger argument is nil, which can cause
panics later. Add a nil check at the start of the function and return an error
or panic immediately if baseLogger is nil to fail fast and prevent downstream
issues.
| func NewFactoryWithConfig(baseLogger *log.Logger, componentLogLevels map[string]string) *Factory { | ||
| registry := NewComponentRegistry() | ||
| registry.LoadLogLevelsFromConfig(componentLogLevels) | ||
|
|
||
| return &Factory{ | ||
| baseLogger: baseLogger, | ||
| componentRegistry: registry, | ||
| } | ||
| } |
There was a problem hiding this comment.
Also validate baseLogger in NewFactoryWithConfig
Mirror the nil check here for consistency.
func NewFactoryWithConfig(baseLogger *log.Logger, componentLogLevels map[string]string) *Factory {
+ if baseLogger == nil {
+ panic("logging.Factory: baseLogger cannot be nil")
+ }
registry := NewComponentRegistry()
registry.LoadLogLevelsFromConfig(componentLogLevels)🤖 Prompt for AI Agents
In backend/golang/pkg/logging/factory.go around lines 24 to 32, the function
NewFactoryWithConfig lacks a nil check for the baseLogger parameter. Add a
validation at the start of the function to check if baseLogger is nil, and
handle this case appropriately (e.g., return nil or an error) to maintain
consistency with other factory creation functions that perform this check.
| func (lf *Factory) getLoggerForComponent(id string, componentType ComponentType, metadata map[string]interface{}) *log.Logger { | ||
| if err := lf.componentRegistry.RegisterComponent(id, componentType, metadata); err != nil { | ||
| // Log the registration error but continue to provide a logger | ||
| // This ensures the application doesn't crash due to registration issues | ||
| lf.baseLogger.Error("failed to register component", "component_id", id, "component_type", componentType, "error", err) | ||
| } | ||
|
|
||
| return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Component registration: idempotency and error noise
If RegisterComponent isn’t idempotent, repeated calls will log errors on every ForX invocation. Either ensure registration is idempotent in the registry or avoid re-registering already known IDs. Also include metadata and standardize the field key to "err".
Proposed minimal logging tweak:
- lf.baseLogger.Error("failed to register component", "component_id", id, "component_type", componentType, "error", err)
+ lf.baseLogger.Error("failed to register component", "component_id", id, "component_type", componentType, "metadata", metadata, "err", err)To verify idempotency and duplicate semantics in the registry, run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate component_registry.go:"
fd -a 'component_registry.go'
echo "Show RegisterComponent and GetLoggerForComponent definitions with context:"
rg -n --no-heading 'type ComponentRegistry|func \(.*\*ComponentRegistry\) RegisterComponent\(|func \(.*\*ComponentRegistry\) GetLoggerForComponent\(' -A 15 -B 2
echo "Check for idempotency cues (e.g., checks on existing IDs, map lookups, or sentinel duplicate errors):"
rg -n --no-heading 'duplicate|exists|already|idempot' -A 2 -B 2Length of output: 167982
Prevent noisy duplicate component registrations and standardize error logging
RegisterComponent is not idempotent—it returns an error on every duplicate call—so invoking getLoggerForComponent repeatedly will flood your logs with “component already registered” errors. You can address this by either making registration idempotent or filtering out the duplicate-registration error in the factory:
Files to update:
- backend/golang/pkg/logging/factory.go (lines ~36–44)
- OR backend/golang/pkg/logging/component_registry.go to adjust RegisterComponent
Proposed filtering approach in Factory.getLoggerForComponent:
func (lf *Factory) getLoggerForComponent(id string, componentType ComponentType, metadata map[string]interface{}) *log.Logger {
- if err := lf.componentRegistry.RegisterComponent(id, componentType, metadata); err != nil {
- lf.baseLogger.Error("failed to register component",
- "component_id", id,
- "component_type", componentType,
- "error", err,
- )
- }
+ if err := lf.componentRegistry.RegisterComponent(id, componentType, metadata); err != nil && !strings.Contains(err.Error(), "already registered") {
+ lf.baseLogger.Error("failed to register component",
+ "component_id", id,
+ "component_type", componentType,
+ "metadata", metadata,
+ "err", err,
+ )
+ }
return lf.componentRegistry.GetLoggerForComponent(lf.baseLogger, id)
}Suggestions:
- Use
"err"instead of"error"for consistency with structured-logging conventions. - Include the full
metadatamap in the log context. - Optionally, make RegisterComponent return nil when a component is already registered, turning it into a no-op.
- If metadata never changes after the first registration, consider moving component registration to initialization rather than per-call.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/golang/pkg/logging/factory.go around lines 36 to 44, the
getLoggerForComponent method logs an error every time a component is already
registered, causing noisy duplicate logs. To fix this, modify the error handling
to check if the error indicates a duplicate registration and suppress logging in
that case. Also, change the log key from "error" to "err" for consistency and
include the full metadata map in the log context. Alternatively, consider making
RegisterComponent idempotent by returning nil on duplicate registrations or move
component registration to initialization to avoid repeated calls.
| // ForHandler creates a logger for handler components. | ||
| func (lf *Factory) ForHandler(id string) *log.Logger { | ||
| return lf.getLoggerForComponent(id, ComponentTypeHandler, nil) | ||
| } | ||
|
|
||
| // ForResolver creates a logger for GraphQL resolver components. | ||
| func (lf *Factory) ForResolver(id string) *log.Logger { | ||
| return lf.getLoggerForComponent(id, ComponentTypeResolver, nil) | ||
| } | ||
|
|
||
| // ForRepository creates a logger for repository components. | ||
| func (lf *Factory) ForRepository(id string) *log.Logger { | ||
| return lf.getLoggerForComponent(id, ComponentTypeRepository, nil) | ||
| } | ||
|
|
||
| // ForWorker creates a logger for worker components. | ||
| func (lf *Factory) ForWorker(id string) *log.Logger { | ||
| return lf.getLoggerForComponent(id, ComponentTypeWorker, nil) | ||
| } | ||
|
|
||
| // ForClient creates a logger for client components. | ||
| func (lf *Factory) ForClient(id string) *log.Logger { | ||
| return lf.getLoggerForComponent(id, ComponentTypeClient, nil) | ||
| } | ||
|
|
||
| // ForServer creates a logger for server components. | ||
| func (lf *Factory) ForServer(id string) *log.Logger { | ||
| return lf.getLoggerForComponent(id, ComponentTypeServer, nil) | ||
| } | ||
|
|
||
| // ForMiddleware creates a logger for middleware components. | ||
| func (lf *Factory) ForMiddleware(id string) *log.Logger { | ||
| return lf.getLoggerForComponent(id, ComponentTypeMiddleware, nil) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a generic WithMetadata API to cover all component types
Only Service/Manager have WithMetadata variants. For parity and to prevent API sprawl, expose a single generic helper.
Add this method (placed near other public helpers):
+// ForTypeWithMetadata creates a logger for any component type with metadata.
+func (lf *Factory) ForTypeWithMetadata(componentType ComponentType, id string, metadata map[string]interface{}) *log.Logger {
+ return lf.getLoggerForComponent(id, componentType, metadata)
+}This preserves existing ForX methods while enabling metadata for all types without duplicating methods.
Also applies to: 111-212, 128-144, 145-165, 166-182, 183-195, 196-212
🤖 Prompt for AI Agents
In backend/golang/pkg/logging/factory.go around lines 76 to 110 and also in the
ranges 111-212, 128-144, 145-165, 166-182, 183-195, 196-212, add a new generic
public method named WithMetadata that accepts a component ID, component type,
and metadata parameters. This method should call the internal
getLoggerForComponent function passing the metadata, enabling metadata support
for all component types without duplicating existing ForX methods. Place this
new method near the other public helper methods to maintain consistency and API
parity.
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/EternisAI/enchanted-twin/pkg/ai" | ||
| "github.com/EternisAI/enchanted-twin/pkg/testing" |
There was a problem hiding this comment.
Import name collision with the stdlib testing package — alias required
This import name shadows the standard library testing package already imported above. The file won’t compile. Alias this package to avoid the collision.
Apply this diff to fix the import:
- "github.com/EternisAI/enchanted-twin/pkg/testing"
+ testutils "github.com/EternisAI/enchanted-twin/pkg/testing"🤖 Prompt for AI Agents
In backend/golang/pkg/testing/personalities/personality_test.go at line 20, the
import of "github.com/EternisAI/enchanted-twin/pkg/testing" conflicts with the
standard library "testing" package already imported. To fix this, alias the
imported package with a unique name (e.g., "etesting") to avoid the name
collision and ensure the file compiles correctly.
| logger := testing.GetTestLogger("personality.test") | ||
| logger.SetLevel(log.InfoLevel) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Update call site to use the alias
After aliasing, update the usage accordingly.
- logger := testing.GetTestLogger("personality.test")
+ logger := testutils.GetTestLogger("personality.test")Run this to spot any other unaliased imports of the internal testing package:
🏁 Script executed:
#!/bin/bash
rg -n --no-heading $'^\\s*"github.com/EternisAI/enchanted-twin/pkg/testing"\\s*$' -A1 -B1Length of output: 379
Update import alias and usage in personality tests
The only occurrence of the unaliased pkg/testing import is in backend/golang/pkg/testing/personalities/personality_test.go. Please:
- Alias the import on line 20:
- "github.com/EternisAI/enchanted-twin/pkg/testing" + testutils "github.com/EternisAI/enchanted-twin/pkg/testing"
- Update the logger call on line 24:
- logger := testing.GetTestLogger("personality.test") + logger := testutils.GetTestLogger("personality.test")
🤖 Prompt for AI Agents
In backend/golang/pkg/testing/personalities/personality_test.go around lines 20
to 25, the import of pkg/testing is unaliased and the logger call uses the
unaliased import. Alias the import of pkg/testing to a shorter name (e.g.,
testpkg) on line 20, then update the logger call on line 24 to use this alias
(e.g., testpkg.GetTestLogger) to maintain consistency and clarity.
| @@ -0,0 +1,18 @@ | |||
| package testing | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Package name testing invites collisions with the stdlib — consider renaming or enforce aliasing
Keeping the package name as testing means every import must be aliased to avoid clashing with the standard testing package. If feasible, rename this package (e.g., testutils or testlog). If renaming is out of scope, enforce aliasing at all call sites.
🏁 Script executed:
#!/bin/bash
# Search for all import statements referencing the local testing package
rg -n '"[^"]*pkg/testing"' -A2Length of output: 715
Enforce aliasing for the local pkg/testing imports to avoid stdlib collisions
The package name testing conflicts with Go’s standard library testing package, causing ambiguous imports. You can either:
- Rename this package (e.g. to
testlogortestutils) and update all import paths and thepackagedeclaration, or - At a minimum, alias every import of
github.com/EternisAI/enchanted-twin/pkg/testing
Files needing an alias update:
- backend/golang/pkg/testing/personalities/personality_test.go
Change:- import "github.com/EternisAI/enchanted-twin/pkg/testing" + import testutils "github.com/EternisAI/enchanted-twin/pkg/testing"
- backend/golang/pkg/dataprocessing/misc/misc_test.go ✅ already uses
testutils - backend/golang/pkg/agent/interface_test.go ✅ already uses
testutils
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/golang/pkg/testing/test_logger.go at line 1, the package name
`testing` conflicts with Go's standard library `testing` package, causing
ambiguous imports. Rename the package to a unique name like `testutils` or
`testlog` and update the package declaration accordingly. Then, update all
import paths in the codebase that reference this package to use the new name or
alias the imports as `testutils` to avoid collisions.
| baseLogger := log.New(nil) // Use nil writer for testing | ||
| return logging.NewFactory(baseLogger) | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use io.Discard instead of a nil writer
Passing nil to log.New relies on implementation details. Use io.Discard for an explicit no-op writer in tests.
-import (
- "github.com/charmbracelet/log"
-
- "github.com/EternisAI/enchanted-twin/pkg/logging"
-)
+import (
+ "io"
+ "github.com/charmbracelet/log"
+
+ "github.com/EternisAI/enchanted-twin/pkg/logging"
+)
@@
var TestLoggerFactory = func() *logging.Factory {
- baseLogger := log.New(nil) // Use nil writer for testing
+ baseLogger := log.New(io.Discard) // Explicitly discard test logs
return logging.NewFactory(baseLogger)
}()📝 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.
| baseLogger := log.New(nil) // Use nil writer for testing | |
| return logging.NewFactory(baseLogger) | |
| }() | |
| import ( | |
| "io" | |
| "github.com/charmbracelet/log" | |
| "github.com/EternisAI/enchanted-twin/pkg/logging" | |
| ) | |
| var TestLoggerFactory = func() *logging.Factory { | |
| baseLogger := log.New(io.Discard) // Explicitly discard test logs | |
| return logging.NewFactory(baseLogger) | |
| }() |
🤖 Prompt for AI Agents
In backend/golang/pkg/testing/test_logger.go around lines 11 to 13, replace the
nil writer passed to log.New with io.Discard to explicitly provide a no-op
writer for testing. Import the io package if necessary and update the log.New
call to use io.Discard instead of nil.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests