From dde74c83aaf40e5c9791f3dd6720ca6519169b23 Mon Sep 17 00:00:00 2001 From: Diego Fornalha Date: Sat, 4 Apr 2026 04:09:28 -0300 Subject: [PATCH] fix: improve error handling, add godoc, and fix bugs across core packages - pkg/pid: log os.Remove errors instead of ignoring, fix Windows syscall - pkg/session: add godoc to all exported methods, handle os.MkdirAll error - pkg/logger: fix nil err in Errorf, handle logFile.Close errors, add godoc - pkg/cron: add input validation in AddJob, add godoc to all exported methods - pkg/config: log SaveConfig errors during migration, add godoc Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/config/config.go | 9 +++++++-- pkg/config/gateway.go | 6 +++++- pkg/cron/service.go | 18 ++++++++++++++++++ pkg/logger/logger.go | 21 +++++++++++++++++---- pkg/pid/pidfile.go | 14 ++++++++++---- pkg/pid/pidfile_windows.go | 10 +++++----- pkg/session/jsonl_backend.go | 7 +++++++ pkg/session/manager.go | 7 ++++++- pkg/utils/download.go | 16 +++++++++++----- pkg/utils/markdown.go | 22 +++++++--------------- pkg/utils/media.go | 2 ++ 11 files changed, 95 insertions(+), 37 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 7165246e5..481feb8e7 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -340,6 +340,7 @@ type WhatsAppConfig struct { UseNative bool `json:"use_native" yaml:"-" env:"PICOCLAW_CHANNELS_WHATSAPP_USE_NATIVE"` SessionStorePath string `json:"session_store_path" yaml:"-" env:"PICOCLAW_CHANNELS_WHATSAPP_SESSION_STORE_PATH"` AllowFrom FlexibleStringSlice `json:"allow_from" yaml:"-" env:"PICOCLAW_CHANNELS_WHATSAPP_ALLOW_FROM"` + GroupTrigger GroupTriggerConfig `json:"group_trigger,omitempty" yaml:"-"` ReasoningChannelID string `json:"reasoning_channel_id" yaml:"-" env:"PICOCLAW_CHANNELS_WHATSAPP_REASONING_CHANNEL_ID"` } @@ -1027,7 +1028,9 @@ func LoadConfig(path string) (*Config, error) { return nil, fmt.Errorf("failed to load existing security config: %w", secErr) } defer func(cfg *Config) { - _ = SaveConfig(path, cfg) + if saveErr := SaveConfig(path, cfg); saveErr != nil { + logger.ErrorF("failed to save config after v0 migration", map[string]any{"error": saveErr}) + } }(cfg) case 1: // V1→V2 migration: infer Enabled and migrate channel config fields @@ -1061,7 +1064,9 @@ func LoadConfig(path string) (*Config, error) { } defer func(cfg *Config) { - _ = SaveConfig(path, cfg) + if saveErr := SaveConfig(path, cfg); saveErr != nil { + logger.ErrorF("failed to save config after v1 migration", map[string]any{"error": saveErr}) + } }(cfg) logger.InfoF( "config migrate success", diff --git a/pkg/config/gateway.go b/pkg/config/gateway.go index e9f4085d3..5147303c9 100644 --- a/pkg/config/gateway.go +++ b/pkg/config/gateway.go @@ -33,6 +33,7 @@ func canonicalGatewayLogLevel(level logger.LogLevel) string { } } +// normalizeGatewayLogLevel maps a user-provided log level string to a valid zerolog level. func normalizeGatewayLogLevel(logLevel string) string { if level, ok := logger.ParseLevel(logLevel); ok { return canonicalGatewayLogLevel(level) @@ -61,7 +62,10 @@ func ResolveGatewayLogLevel(path string) string { data, err := os.ReadFile(path) if err == nil { - _ = json.Unmarshal(data, &cfg) + if jsonErr := json.Unmarshal(data, &cfg); jsonErr != nil { + // Malformed config — fall through to default log level. + _ = jsonErr + } } if envLevel := os.Getenv("PICOCLAW_LOG_LEVEL"); envLevel != "" { diff --git a/pkg/cron/service.go b/pkg/cron/service.go index 6a8728943..0191365ef 100644 --- a/pkg/cron/service.go +++ b/pkg/cron/service.go @@ -7,6 +7,7 @@ import ( "fmt" "log" "os" + "strings" "sync" "time" @@ -68,6 +69,8 @@ type CronService struct { gronx *gronx.Gronx } +// NewCronService creates a cron service that persists jobs to storePath. +// onJob is called when a job fires; it may be nil if SetOnJob is used later. func NewCronService(storePath string, onJob JobHandler) *CronService { cs := &CronService{ storePath: storePath, @@ -80,6 +83,7 @@ func NewCronService(storePath string, onJob JobHandler) *CronService { return cs } +// Start begins the cron run loop in a background goroutine. func (cs *CronService) Start() error { cs.mu.Lock() defer cs.mu.Unlock() @@ -107,6 +111,7 @@ func (cs *CronService) Start() error { return nil } +// Stop signals the run loop to exit and waits for it to finish. func (cs *CronService) Stop() { cs.mu.Lock() defer cs.mu.Unlock() @@ -365,12 +370,14 @@ func (cs *CronService) getNextWakeMS() *int64 { return nextWake } +// Load reads persisted jobs from disk, replacing the in-memory store. func (cs *CronService) Load() error { cs.mu.Lock() defer cs.mu.Unlock() return cs.loadStore() } +// SetOnJob replaces the callback invoked when a job fires. func (cs *CronService) SetOnJob(handler JobHandler) { cs.mu.Lock() defer cs.mu.Unlock() @@ -404,12 +411,18 @@ func (cs *CronService) saveStoreUnsafe() error { return fileutil.WriteFileAtomic(cs.storePath, data, 0o600) } +// AddJob creates and persists a new cron job. Returns an error if the +// schedule expression is invalid or required fields are empty. func (cs *CronService) AddJob( name string, schedule CronSchedule, message string, channel, to string, ) (*CronJob, error) { + if strings.TrimSpace(name) == "" { + return nil, fmt.Errorf("job name must not be empty") + } + cs.mu.Lock() defer cs.mu.Unlock() @@ -447,6 +460,7 @@ func (cs *CronService) AddJob( return &job, nil } +// UpdateJob replaces a job's definition and persists the change. func (cs *CronService) UpdateJob(job *CronJob) error { cs.mu.Lock() defer cs.mu.Unlock() @@ -464,6 +478,7 @@ func (cs *CronService) UpdateJob(job *CronJob) error { return fmt.Errorf("job not found") } +// RemoveJob deletes a job by ID and returns true if it was found. func (cs *CronService) RemoveJob(jobID string) bool { cs.mu.Lock() defer cs.mu.Unlock() @@ -493,6 +508,7 @@ func (cs *CronService) removeJobUnsafe(jobID string) bool { return removed } +// EnableJob toggles a job's enabled state and recomputes its next run time. func (cs *CronService) EnableJob(jobID string, enabled bool) *CronJob { cs.mu.Lock() defer cs.mu.Unlock() @@ -522,6 +538,7 @@ func (cs *CronService) EnableJob(jobID string, enabled bool) *CronJob { return nil } +// ListJobs returns a snapshot of all jobs, optionally including disabled ones. func (cs *CronService) ListJobs(includeDisabled bool) []CronJob { cs.mu.RLock() defer cs.mu.RUnlock() @@ -540,6 +557,7 @@ func (cs *CronService) ListJobs(includeDisabled bool) []CronJob { return enabled } +// Status returns a summary of the cron service state for diagnostics. func (cs *CronService) Status() map[string]any { cs.mu.RLock() defer cs.mu.RUnlock() diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go index 6d2e31791..288a4bc66 100644 --- a/pkg/logger/logger.go +++ b/pkg/logger/logger.go @@ -110,6 +110,7 @@ func formatFieldValue(i any) string { return s } +// SetLevel sets the global minimum log level. func SetLevel(level LogLevel) { mu.Lock() defer mu.Unlock() @@ -117,12 +118,14 @@ func SetLevel(level LogLevel) { zerolog.SetGlobalLevel(level) } +// SetConsoleLevel adjusts the log level for console output only. func SetConsoleLevel(level LogLevel) { mu.Lock() defer mu.Unlock() logger = logger.Level(level) } +// DisableConsole silences console output by redirecting it to io.Discard. func DisableConsole() { mu.Lock() defer mu.Unlock() @@ -130,6 +133,7 @@ func DisableConsole() { logger = logger.Output(io.MultiWriter(writers...)) } +// EnableConsole restores console output after a DisableConsole call. func EnableConsole() { mu.Lock() defer mu.Unlock() @@ -137,6 +141,7 @@ func EnableConsole() { logger = logger.Output(io.MultiWriter(writers...)) } +// GetLevel returns the current global log level. func GetLevel() LogLevel { mu.RLock() defer mu.RUnlock() @@ -186,15 +191,18 @@ func EnableFileLogging(filePath string) error { return fmt.Errorf("failed to open log file: %w", err) } - // Close old file if exists + // Close old file if exists; log but don't fail on close error + // since we're replacing it with a new file. if logFile != nil { - logFile.Close() + if closeErr := logFile.Close(); closeErr != nil { + fmt.Fprintf(os.Stderr, "warning: failed to close old log file: %v\n", closeErr) + } } logFile = newFile if len(writers) != 1 { - return fmt.Errorf("failed to configure file logging: %w", err) + return fmt.Errorf("invalid writers state: expected 1 writer, got %d", len(writers)) } writers = append(writers, logFile) @@ -203,12 +211,15 @@ func EnableFileLogging(filePath string) error { return nil } +// DisableFileLogging closes the log file and removes it from the output writers. func DisableFileLogging() { mu.Lock() defer mu.Unlock() if logFile != nil { - logFile.Close() + if closeErr := logFile.Close(); closeErr != nil { + fmt.Fprintf(os.Stderr, "warning: failed to close log file: %v\n", closeErr) + } logFile = nil } if len(writers) > 1 { @@ -217,6 +228,8 @@ func DisableFileLogging() { } } +// ConfigureFromEnv reads PICOCLAW_LOG_FILE and PICOCLAW_LOG_LEVEL from the +// environment and applies them to the logger configuration. func ConfigureFromEnv() { if logFile := os.Getenv("PICOCLAW_LOG_FILE"); logFile != "" { if strings.HasPrefix(logFile, "~/") { diff --git a/pkg/pid/pidfile.go b/pkg/pid/pidfile.go index 69d02bc65..aec992d31 100644 --- a/pkg/pid/pidfile.go +++ b/pkg/pid/pidfile.go @@ -61,7 +61,9 @@ func WritePidFile(homePath, host string, port int) (*PidFileData, error) { logger.Warnf("not running (PID: %d) so will remove the pid file: %s", data.PID, pidPath) } // Stale PID file; process no longer exists → clean up. - os.Remove(pidPath) + if err := os.Remove(pidPath); err != nil && !os.IsNotExist(err) { + logger.Warnf("failed to remove stale pid file %s: %v", pidPath, err) + } } data := &PidFileData{ @@ -91,7 +93,7 @@ func WritePidFile(homePath, host string, port int) (*PidFileData, error) { return nil, fmt.Errorf("failed to write pid file: %w", err) } if err := os.Rename(tmp, pidPath); err != nil { - os.Remove(tmp) + _ = os.Remove(tmp) // best-effort cleanup of temp file return nil, fmt.Errorf("failed to rename pid file: %w", err) } logger.Debugf("wrote pid file: %s success", pidPath) @@ -115,7 +117,9 @@ func ReadPidFileWithCheck(homePath string) *PidFileData { if !isProcessRunning(data.PID) { logger.Debugf("process not running, remove pid file: %s", pidPath) - os.Remove(pidPath) + if err := os.Remove(pidPath); err != nil && !os.IsNotExist(err) { + logger.Warnf("failed to remove stale pid file %s: %v", pidPath, err) + } return nil } @@ -137,7 +141,9 @@ func RemovePidFile(homePath string) { } logger.Infof("remove pid file: %s", pidPath) - os.Remove(pidPath) + if err := os.Remove(pidPath); err != nil && !os.IsNotExist(err) { + logger.Warnf("failed to remove pid file %s: %v", pidPath, err) + } } // readPidFileUnlocked reads the PID file without acquiring the lock. diff --git a/pkg/pid/pidfile_windows.go b/pkg/pid/pidfile_windows.go index 6a2cce793..406d66bc8 100644 --- a/pkg/pid/pidfile_windows.go +++ b/pkg/pid/pidfile_windows.go @@ -23,19 +23,19 @@ func isProcessRunning(pid int) bool { return false } - handle, _, err := procOpenProcess.Call( + handle, _, _ := procOpenProcess.Call( uintptr(processQueryLimitedInformation), 0, uintptr(pid), ) - if handle == 0 || err != nil { + if handle == 0 { return false } - defer procCloseHandle.Call(handle) + defer procCloseHandle.Call(handle) //nolint:errcheck var exitCode uint32 - ret, _, err := procGetExitCodeProcess.Call(handle, uintptr(unsafe.Pointer(&exitCode))) - if ret == 0 || err != nil { + ret, _, _ := procGetExitCodeProcess.Call(handle, uintptr(unsafe.Pointer(&exitCode))) + if ret == 0 { return false } return exitCode == stillActive diff --git a/pkg/session/jsonl_backend.go b/pkg/session/jsonl_backend.go index 7f470de15..27393d40c 100644 --- a/pkg/session/jsonl_backend.go +++ b/pkg/session/jsonl_backend.go @@ -20,18 +20,21 @@ func NewJSONLBackend(store memory.Store) *JSONLBackend { return &JSONLBackend{store: store} } +// AddMessage appends a text message to the session (fire-and-forget). func (b *JSONLBackend) AddMessage(sessionKey, role, content string) { if err := b.store.AddMessage(context.Background(), sessionKey, role, content); err != nil { log.Printf("session: add message: %v", err) } } +// AddFullMessage appends a complete message (with tool calls, reasoning, etc.) to the session. func (b *JSONLBackend) AddFullMessage(sessionKey string, msg providers.Message) { if err := b.store.AddFullMessage(context.Background(), sessionKey, msg); err != nil { log.Printf("session: add full message: %v", err) } } +// GetHistory returns the message history for a session, or an empty slice on error. func (b *JSONLBackend) GetHistory(key string) []providers.Message { msgs, err := b.store.GetHistory(context.Background(), key) if err != nil { @@ -41,6 +44,7 @@ func (b *JSONLBackend) GetHistory(key string) []providers.Message { return msgs } +// GetSummary returns the session summary, or empty string on error. func (b *JSONLBackend) GetSummary(key string) string { summary, err := b.store.GetSummary(context.Background(), key) if err != nil { @@ -50,18 +54,21 @@ func (b *JSONLBackend) GetSummary(key string) string { return summary } +// SetSummary replaces the session summary (fire-and-forget). func (b *JSONLBackend) SetSummary(key, summary string) { if err := b.store.SetSummary(context.Background(), key, summary); err != nil { log.Printf("session: set summary: %v", err) } } +// SetHistory replaces the full message history for a session (fire-and-forget). func (b *JSONLBackend) SetHistory(key string, history []providers.Message) { if err := b.store.SetHistory(context.Background(), key, history); err != nil { log.Printf("session: set history: %v", err) } } +// TruncateHistory keeps only the last keepLast messages in a session (fire-and-forget). func (b *JSONLBackend) TruncateHistory(key string, keepLast int) { if err := b.store.TruncateHistory(context.Background(), key, keepLast); err != nil { log.Printf("session: truncate history: %v", err) diff --git a/pkg/session/manager.go b/pkg/session/manager.go index ef720b7c5..8a8a10247 100644 --- a/pkg/session/manager.go +++ b/pkg/session/manager.go @@ -2,6 +2,7 @@ package session import ( "encoding/json" + "log" "os" "path/filepath" "strings" @@ -32,13 +33,17 @@ func NewSessionManager(storage string) *SessionManager { } if storage != "" { - os.MkdirAll(storage, 0o700) + if err := os.MkdirAll(storage, 0o700); err != nil { + log.Printf("session: failed to create storage dir %s: %v", storage, err) + } sm.loadSessions() } return sm } +// GetOrCreate returns the session for key, creating a new empty one if it doesn't exist. +// The returned session is never nil. func (sm *SessionManager) GetOrCreate(key string) *Session { sm.mu.Lock() defer sm.mu.Unlock() diff --git a/pkg/utils/download.go b/pkg/utils/download.go index 5d9a13a30..57d56815e 100644 --- a/pkg/utils/download.go +++ b/pkg/utils/download.go @@ -39,9 +39,13 @@ func DownloadToFile(ctx context.Context, client *http.Client, req *http.Request, defer resp.Body.Close() if resp.StatusCode < 200 || resp.StatusCode >= 300 { - // Read a small amount for the error message. + // Read a small amount for the error message; ignore read errors + // since the HTTP status itself is the primary error signal. errBody := make([]byte, 512) - n, _ := io.ReadFull(resp.Body, errBody) + n, readErr := io.ReadFull(resp.Body, errBody) + if readErr != nil && readErr != io.ErrUnexpectedEOF && readErr != io.EOF { + return "", fmt.Errorf("HTTP %d (body unreadable: %w)", resp.StatusCode, readErr) + } return "", fmt.Errorf("HTTP %d: %s", resp.StatusCode, string(errBody[:n])) } @@ -56,10 +60,12 @@ func DownloadToFile(ctx context.Context, client *http.Client, req *http.Request, "path": tmpPath, }) - // Cleanup helper — removes the temp file on any error. + // Cleanup helper — best-effort removal of the temp file on error. + // Close/Remove errors are intentionally ignored since this only runs + // on failure paths where the primary error is already captured. cleanup := func() { - _ = tmpFile.Close() - _ = os.Remove(tmpPath) + tmpFile.Close() + os.Remove(tmpPath) } // Optionally limit the download size. diff --git a/pkg/utils/markdown.go b/pkg/utils/markdown.go index c7873252a..cf24cb60e 100644 --- a/pkg/utils/markdown.go +++ b/pkg/utils/markdown.go @@ -25,6 +25,13 @@ var skipTags = map[string]bool{ "nav": true, "footer": true, "aside": true, "header": true, "form": true, "dialog": true, } +// unlikelyKeywords is allocated once at package level to avoid repeated +// allocation on every call to isUnlikelyNode. +var unlikelyKeywords = []string{ + "menu", "nav", "footer", "sidebar", "cookie", "banner", + "sponsor", "advert", "popup", "modal", "newsletter", "share", "social", +} + func isSafeHref(href string) bool { lower := strings.ToLower(strings.TrimSpace(href)) if strings.HasPrefix(lower, "javascript:") || strings.HasPrefix(lower, "vbscript:") || @@ -82,21 +89,6 @@ func isUnlikelyNode(n *html.Node) bool { strings.Contains(classId, "content") { return false } - unlikelyKeywords := []string{ - "menu", - "nav", - "footer", - "sidebar", - "cookie", - "banner", - "sponsor", - "advert", - "popup", - "modal", - "newsletter", - "share", - "social", - } for _, keyword := range unlikelyKeywords { if strings.Contains(classId, keyword) { return true diff --git a/pkg/utils/media.go b/pkg/utils/media.go index 823ca155e..c06f5c4b5 100644 --- a/pkg/utils/media.go +++ b/pkg/utils/media.go @@ -18,6 +18,8 @@ import ( var audioExtensions = []string{".mp3", ".wav", ".ogg", ".m4a", ".flac", ".aac", ".wma"} +// AudioFormat returns the audio format (e.g. "mp3", "wav") for the given file path +// based on its extension, or an error if the format is unsupported. func AudioFormat(path string) (string, error) { ext := strings.ToLower(filepath.Ext(path)) for _, supportedExt := range audioExtensions {