diff --git a/.claude/rules/logging-conventions.md b/.claude/rules/logging-conventions.md index db58834..65cd5ed 100644 --- a/.claude/rules/logging-conventions.md +++ b/.claude/rules/logging-conventions.md @@ -119,12 +119,12 @@ The one documented exception is `MemorySearch`'s `query` field — it's small, u ## 5. Nil-Logger Safety ★ -Library code — anything in `pkg/` — must accept a `nil` logger and behave silently. Two project conventions for the fallback: +Library code — anything in `pkg/` — must accept a `nil` logger and behave silently. The two project conventions for the fallback are encoded once in `internal/loghelper`; **call the helper, never inline the `if logger == nil` block**: -| Where | Fallback | Why | +| Where | Helper | Why | |---|---|---| -| `pkg/` libraries (`mcp`, `temperature`) | `slog.New(slog.DiscardHandler)` | Tests and embedders may not want output | -| CLI-time code (`pkg/compiler` when invoked directly) | `slog.Default()` | Users running `remindb compile` want feedback | +| `pkg/` libraries (`mcp`, `temperature`) | `loghelper.OrDiscard(l)` → `slog.New(slog.DiscardHandler)` | Tests and embedders may not want output | +| CLI-time code (`pkg/compiler` when invoked directly) | `loghelper.OrDefault(l)` → `slog.Default()` | Users running `remindb compile` want feedback | ```go // Bad — assumes the caller passed a non-nil logger @@ -134,7 +134,7 @@ func NewServer(st *store.Store, ..., logger *slog.Logger) *Server { return s } -// Good — pkg/ library default +// Bad — inlining the fallback; it now lives in one place func NewServer(st *store.Store, ..., logger *slog.Logger) *Server { if logger == nil { logger = slog.New(slog.DiscardHandler) @@ -142,18 +142,20 @@ func NewServer(st *store.Store, ..., logger *slog.Logger) *Server { return &Server{logger: logger, ...} } +// Good — pkg/ library default +func NewServer(st *store.Store, ..., logger *slog.Logger) *Server { + return &Server{logger: loghelper.OrDiscard(logger), ...} +} + // Good — CLI-time default (used only when the call is the entry point) func Run(ctx context.Context, st *store.Store, opts ...Option) error { o := applyOptions(opts...) - logger := o.logger - if logger == nil { - logger = slog.Default() - } + logger := loghelper.OrDefault(o.logger) ... } ``` -See `pkg/mcp/server.go:26-28` and `pkg/temperature/tracker.go:30-32` for the discard pattern; `pkg/compiler/compiler.go:63-66` for the default pattern. +`internal/loghelper/loghelper.go` is the canonical implementation of both conventions; every `pkg/` site resolves through it (`OrDiscard` for libraries, `OrDefault` for CLI-entry code). No `pkg/` file should still inline `if logger == nil { logger = slog.New(...) }`. --- diff --git a/internal/loghelper/loghelper.go b/internal/loghelper/loghelper.go new file mode 100644 index 0000000..2d4cedb --- /dev/null +++ b/internal/loghelper/loghelper.go @@ -0,0 +1,17 @@ +package loghelper + +import "log/slog" + +func OrDiscard(l *slog.Logger) *slog.Logger { + if l == nil { + return slog.New(slog.DiscardHandler) + } + return l +} + +func OrDefault(l *slog.Logger) *slog.Logger { + if l == nil { + return slog.Default() + } + return l +} diff --git a/internal/loghelper/loghelper_test.go b/internal/loghelper/loghelper_test.go new file mode 100644 index 0000000..9caea7d --- /dev/null +++ b/internal/loghelper/loghelper_test.go @@ -0,0 +1,29 @@ +package loghelper + +import ( + "log/slog" + "testing" +) + +func TestOrDiscard(t *testing.T) { + if got := OrDiscard(nil); got == nil { + t.Fatal("OrDiscard(nil) returned nil") + } + OrDiscard(nil).Info("must not panic") + + l := slog.New(slog.DiscardHandler) + if got := OrDiscard(l); got != l { + t.Errorf("OrDiscard(l) = %p, want identity %p", got, l) + } +} + +func TestOrDefault(t *testing.T) { + if got := OrDefault(nil); got != slog.Default() { + t.Errorf("OrDefault(nil) = %p, want slog.Default() %p", got, slog.Default()) + } + + l := slog.New(slog.DiscardHandler) + if got := OrDefault(l); got != l { + t.Errorf("OrDefault(l) = %p, want identity %p", got, l) + } +} diff --git a/pkg/compiler/compiler.go b/pkg/compiler/compiler.go index 63be22f..735c137 100644 --- a/pkg/compiler/compiler.go +++ b/pkg/compiler/compiler.go @@ -14,6 +14,7 @@ import ( "github.com/radimsem/remindb/internal/fileext" "github.com/radimsem/remindb/internal/ignore" + "github.com/radimsem/remindb/internal/loghelper" "github.com/radimsem/remindb/internal/redaction" "github.com/radimsem/remindb/internal/tempfile" "github.com/radimsem/remindb/pkg/config" @@ -131,10 +132,7 @@ func Compile(ctx context.Context, st *store.Store, opts ...Option) (*Result, err opt(&o) } - logger := o.logger - if logger == nil { - logger = slog.Default() - } + logger := loghelper.OrDefault(o.logger) ctx, cancel := withTimeout(ctx, o.timeout) defer cancel() diff --git a/pkg/mcp/initial.go b/pkg/mcp/initial.go index 41c7826..61956a6 100644 --- a/pkg/mcp/initial.go +++ b/pkg/mcp/initial.go @@ -5,15 +5,14 @@ import ( "fmt" "log/slog" + "github.com/radimsem/remindb/internal/loghelper" "github.com/radimsem/remindb/pkg/compiler" "github.com/radimsem/remindb/pkg/store" ) // Run an initial compile when the store is empty; no-op otherwise. func MaybeInitialCompile(ctx context.Context, st *store.Store, dir string, logger *slog.Logger) error { - if logger == nil { - logger = slog.New(slog.DiscardHandler) - } + logger = loghelper.OrDiscard(logger) stats, err := st.GetStats(ctx) if err != nil { diff --git a/pkg/mcp/ledger/ledger.go b/pkg/mcp/ledger/ledger.go index d60c301..40b9c36 100644 --- a/pkg/mcp/ledger/ledger.go +++ b/pkg/mcp/ledger/ledger.go @@ -15,6 +15,7 @@ import ( "sync" "github.com/radimsem/remindb/internal/contentid" + "github.com/radimsem/remindb/internal/loghelper" "github.com/radimsem/remindb/pkg/config" "github.com/radimsem/remindb/pkg/mcp/jsonlsink" ) @@ -64,9 +65,7 @@ type Ledger struct { // New opens (and compacts) the ledger under /.remindb/sessions. func New(workspace string, logger *slog.Logger) (*Ledger, error) { - if logger == nil { - logger = slog.New(slog.DiscardHandler) - } + logger = loghelper.OrDiscard(logger) dir := filepath.Join(workspace, config.DirName, subDir) // growth bounded by compact(), not rotation diff --git a/pkg/mcp/notify/notify.go b/pkg/mcp/notify/notify.go index 72b02d7..2e3694a 100644 --- a/pkg/mcp/notify/notify.go +++ b/pkg/mcp/notify/notify.go @@ -9,6 +9,7 @@ import ( "time" mcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/radimsem/remindb/internal/loghelper" "github.com/radimsem/remindb/pkg/config" "github.com/radimsem/remindb/pkg/mcp/resources" ) @@ -31,9 +32,7 @@ type Publisher struct { } func NewPublisher(cfg config.ResourcesConfig, logger *slog.Logger) (*Publisher, error) { - if logger == nil { - logger = slog.New(slog.DiscardHandler) - } + logger = loghelper.OrDiscard(logger) def := DefaultDebounce if cfg.Debounce != nil { diff --git a/pkg/mcp/rescan.go b/pkg/mcp/rescan.go index 90b1726..0f34d60 100644 --- a/pkg/mcp/rescan.go +++ b/pkg/mcp/rescan.go @@ -15,6 +15,7 @@ import ( "github.com/radimsem/remindb/internal/contentid" "github.com/radimsem/remindb/internal/fileext" "github.com/radimsem/remindb/internal/ignore" + "github.com/radimsem/remindb/internal/loghelper" "github.com/radimsem/remindb/pkg/compiler" "github.com/radimsem/remindb/pkg/config" "github.com/radimsem/remindb/pkg/diff" @@ -64,9 +65,7 @@ func NewRescanLoop(st *store.Store, dir string, interval time.Duration, cc confi if interval <= 0 { interval = defaultRescanInterval } - if logger == nil { - logger = slog.New(slog.DiscardHandler) - } + logger = loghelper.OrDiscard(logger) if status == nil { status = rescanstat.New() } diff --git a/pkg/mcp/server.go b/pkg/mcp/server.go index f8d1386..4cf0176 100644 --- a/pkg/mcp/server.go +++ b/pkg/mcp/server.go @@ -8,6 +8,7 @@ import ( "time" "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/radimsem/remindb/internal/loghelper" "github.com/radimsem/remindb/internal/redaction" "github.com/radimsem/remindb/pkg/config" "github.com/radimsem/remindb/pkg/logbuf" @@ -102,10 +103,7 @@ func NewServer(st *store.Store, tracker *temperature.Tracker, cfg temperature.Co opt(&o) } - logger := o.logger - if logger == nil { - logger = slog.New(slog.DiscardHandler) - } + logger := loghelper.OrDiscard(o.logger) transport := o.transport if transport == "" { diff --git a/pkg/mcp/session/registry.go b/pkg/mcp/session/registry.go index 0358853..f1fffbb 100644 --- a/pkg/mcp/session/registry.go +++ b/pkg/mcp/session/registry.go @@ -11,6 +11,7 @@ import ( gomcp "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/radimsem/remindb/internal/contentid" + "github.com/radimsem/remindb/internal/loghelper" "github.com/radimsem/remindb/pkg/mcp/ledger" "github.com/radimsem/remindb/pkg/mcp/sessionlog" ) @@ -59,9 +60,7 @@ type Registry struct { // NewRegistry tracks live sessions; l (may be nil) persists their metrics. func NewRegistry(srv *gomcp.Server, transport, listen string, l *ledger.Ledger, logger *slog.Logger) *Registry { - if logger == nil { - logger = slog.New(slog.DiscardHandler) - } + logger = loghelper.OrDiscard(logger) return &Registry{ srv: srv, diff --git a/pkg/temperature/tracker.go b/pkg/temperature/tracker.go index 6cbd745..5804282 100644 --- a/pkg/temperature/tracker.go +++ b/pkg/temperature/tracker.go @@ -11,6 +11,7 @@ import ( "time" "github.com/radimsem/remindb/internal/contentid" + "github.com/radimsem/remindb/internal/loghelper" "github.com/radimsem/remindb/pkg/config" "github.com/radimsem/remindb/pkg/store" ) @@ -49,9 +50,7 @@ func NewTracker(s NodeStore, dir string, bootstrap Config, logger *slog.Logger) return nil, fmt.Errorf("invalid config: %w", err) } - if logger == nil { - logger = slog.New(slog.DiscardHandler) - } + logger = loghelper.OrDiscard(logger) return &Tracker{ store: s, bootstrap: bootstrap,