From 7e8c3b75de40e23b14dadeb870143c3a3e038aaa Mon Sep 17 00:00:00 2001 From: AK Date: Sun, 2 Aug 2026 10:25:08 -0700 Subject: [PATCH] feat: add owner private config reload --- README.md | 5 ++ bot/bot.go | 90 ++++++++++++++++++---- bot/message_test.go | 41 ++++++++++ bot/plugin.go | 6 ++ cmd/irc-bot/main.go | 113 ++++++++++++++++++--------- docs/configuration.md | 19 +++++ docs/plugins.md | 29 ++++++- docs/security.md | 5 ++ plugins/ask.go | 156 ++++++++++++++++++++++++++++++-------- plugins/ask_test.go | 53 ++++++++++++- plugins/wikipedia.go | 80 ++++++++++++++++++- plugins/wikipedia_test.go | 34 +++++++++ 12 files changed, 539 insertions(+), 92 deletions(-) diff --git a/README.md b/README.md index 7b31686..35db53f 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,11 @@ there. | [Security](docs/security.md) | Deployment and runtime security guidance | | [Development and CI](docs/development.md) | Project workflow, tests, CI, CodeQL, and Dependabot | +Owners can send GoBot a private `reload` message to apply reloadable plugin +configuration without dropping the IRC connection. See +[Configuration](docs/configuration.md#owner-only-private-reload) for the +authentication and reload boundaries. + ## Project layout ```text diff --git a/bot/bot.go b/bot/bot.go index 309af5c..5c9db0c 100644 --- a/bot/bot.go +++ b/bot/bot.go @@ -17,22 +17,24 @@ import ( ) type Bot struct { - Config Config - DB *storage.DB - Stats *Stats - Plugins []Plugin - Log *zap.Logger - Queue *Queue - client *irc.Client - mu sync.RWMutex - commandMu sync.Mutex - lastCommands map[string]time.Time - lastWarnings map[string]time.Time - inviteMu sync.Mutex - lastInvites map[string]time.Time - lastInvite time.Time - warmupMu sync.RWMutex - warmupUntil map[string]time.Time + Config Config + DB *storage.DB + Stats *Stats + Plugins []Plugin + Log *zap.Logger + Queue *Queue + client *irc.Client + reloadHandler func(Message) + mu sync.RWMutex + pluginMu sync.RWMutex + commandMu sync.Mutex + lastCommands map[string]time.Time + lastWarnings map[string]time.Time + inviteMu sync.Mutex + lastInvites map[string]time.Time + lastInvite time.Time + warmupMu sync.RWMutex + warmupUntil map[string]time.Time } func New(cfg Config, db *storage.DB, plugins []Plugin, log *zap.Logger) *Bot { @@ -44,6 +46,38 @@ func NewWithStats(cfg Config, db *storage.DB, plugins []Plugin, log *zap.Logger, b.Queue = NewQueue(cfg.RateLimit.MessagesPerSecond, cfg.RateLimit.Burst, func(o Outgoing) { b.sendNow(o.Target, o.Text) }) return b } + +// SetReloadHandler installs the owner-only private-message callback used by +// the process to reload plugin configuration in place. +func (b *Bot) SetReloadHandler(handler func(Message)) { + b.mu.Lock() + b.reloadHandler = handler + b.mu.Unlock() +} + +// ReloadPlugins applies configuration to active plugins that explicitly +// support runtime reloads. Connection, identity, channel, and owner settings +// remain unchanged until the next process start. +func (b *Bot) ReloadPlugins(configs map[string]PluginConfig) (int, error) { + b.pluginMu.Lock() + defer b.pluginMu.Unlock() + count := 0 + var firstErr error + for _, p := range b.Plugins { + reloadable, ok := p.(Reloadable) + if !ok { + continue + } + if err := reloadable.Reload(configs[p.Name()]); err != nil { + if firstErr == nil { + firstErr = fmt.Errorf("%s: %w", p.Name(), err) + } + continue + } + count++ + } + return count, firstErr +} func (b *Bot) Send(target, text string) { if !b.Queue.Enqueue(Outgoing{target, text}) { b.Stats.dropped.Add(1) @@ -428,6 +462,9 @@ func (b *Bot) logIRCEvent(m *irc.Message) { } } func (b *Bot) dispatch(msg Message) { + if b.handlePrivateReload(msg) { + return + } if msg.Command == "PRIVMSG" && msg.IsChannel && b.channelWarming(msg.Target) { return } @@ -440,8 +477,10 @@ func (b *Bot) dispatch(msg Message) { continue } consumed := false + b.pluginMu.RLock() func() { defer func() { + b.pluginMu.RUnlock() if r := recover(); r != nil { b.Log.Error("plugin panic", zap.String("plugin", p.Name()), zap.Any("panic", r)) } @@ -469,8 +508,10 @@ func (b *Bot) dispatchEvent(msg Message) { if !ok { continue } + b.pluginMu.RLock() func() { defer func() { + b.pluginMu.RUnlock() if r := recover(); r != nil { b.Log.Error("plugin event panic", zap.String("plugin", p.Name()), zap.Any("panic", r)) } @@ -479,6 +520,23 @@ func (b *Bot) dispatchEvent(msg Message) { }() } } + +func (b *Bot) handlePrivateReload(msg Message) bool { + if msg.Command != "PRIVMSG" || msg.IsChannel || !b.IsOwner(msg) { + return false + } + text := strings.ToLower(strings.TrimSpace(msg.Text)) + if text != "reload" && text != "!reload" { + return false + } + b.mu.RLock() + handler := b.reloadHandler + b.mu.RUnlock() + if handler != nil { + handler(msg) + } + return true +} func (b *Bot) Run(ctx context.Context) error { backoff := 5 * time.Second for { diff --git a/bot/message_test.go b/bot/message_test.go index 712db4a..bfc017d 100644 --- a/bot/message_test.go +++ b/bot/message_test.go @@ -31,3 +31,44 @@ func TestValidChannelName(t *testing.T) { } } } + +func TestIsOwnerRequiresAuthenticatedAccount(t *testing.T) { + b := &Bot{Config: Config{OwnerAccounts: []string{"Alice"}}} + + if !b.IsOwner(Message{Account: "alice"}) { + t.Fatal("expected matching authenticated account to be an owner") + } + if b.IsOwner(Message{Nick: "alice"}) { + t.Fatal("nickname alone must not prove ownership") + } + if b.IsOwner(Message{Account: "*"}) { + t.Fatal("unidentified account must not prove ownership") + } +} + +func TestPrivateReloadIsOwnerOnly(t *testing.T) { + called := 0 + b := &Bot{ + Config: Config{OwnerAccounts: []string{"alice"}}, + reloadHandler: func(Message) { called++ }, + } + + if !b.handlePrivateReload(Message{Command: "PRIVMSG", Account: "alice", Text: "reload"}) { + t.Fatal("expected an authenticated owner's private reload to be handled") + } + if called != 1 { + t.Fatalf("expected reload handler once, got %d calls", called) + } + if b.handlePrivateReload(Message{Command: "PRIVMSG", Account: "guest", Text: "reload"}) { + t.Fatal("unauthenticated account must not trigger reload") + } + if b.handlePrivateReload(Message{Command: "PRIVMSG", Account: "alice", IsChannel: true, Text: "reload"}) { + t.Fatal("channel messages must not trigger private reload") + } + if b.handlePrivateReload(Message{Command: "PRIVMSG", Account: "alice", Text: "reload now"}) { + t.Fatal("reload must require an exact command") + } + if called != 1 { + t.Fatalf("unexpected reload handler calls: %d", called) + } +} diff --git a/bot/plugin.go b/bot/plugin.go index 9de3ac2..6a94d70 100644 --- a/bot/plugin.go +++ b/bot/plugin.go @@ -19,3 +19,9 @@ type Starter interface { type EventHandler interface { HandleEvent(*Bot, Message) bool } + +// Reloadable lets a long-running plugin apply configuration changes without +// rebuilding its state or dropping the IRC connection. +type Reloadable interface { + Reload(PluginConfig) error +} diff --git a/cmd/irc-bot/main.go b/cmd/irc-bot/main.go index 04af31d..67d1a55 100644 --- a/cmd/irc-bot/main.go +++ b/cmd/irc-bot/main.go @@ -19,42 +19,10 @@ import ( ) func main() { - viper.SetConfigFile("config.yaml") - viper.AutomaticEnv() - viper.SetEnvPrefix("BOT") - viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) - _ = viper.ReadInConfig() - viper.BindEnv("identity.sasl_pass", "BOT_SASL_PASS") - viper.BindEnv("plugins.news.api_key", "BOT_NEWS_API_KEY") - viper.BindEnv("plugins.lastfm.api_key", "BOT_LASTFM_API_KEY") - viper.BindEnv("plugins.github.token", "BOT_GITHUB_TOKEN") - viper.BindEnv("plugins.urltitle.youtube_api_key", "BOT_YOUTUBE_API_KEY") - viper.BindEnv("plugins.ask.ai_rewrite", "BOT_ASK_AI_REWRITE") - viper.BindEnv("plugins.ask.provider", "BOT_ASK_PROVIDER") - viper.BindEnv("plugins.ask.openrouter_api_key", "BOT_OPENROUTER_API_KEY") - viper.BindEnv("plugins.ask.openrouter_model", "BOT_OPENROUTER_MODEL") - viper.BindEnv("plugins.ask.openai_api_key", "BOT_OPENAI_API_KEY") - viper.BindEnv("plugins.ask.openai_model", "BOT_OPENAI_MODEL") - viper.BindEnv("plugins.ask.gemini_api_key", "BOT_GEMINI_API_KEY") - viper.BindEnv("plugins.ask.gemini_model", "BOT_GEMINI_MODEL") - viper.BindEnv("plugins.ask.ollama_url", "BOT_OLLAMA_URL") - viper.BindEnv("plugins.ask.ollama_model", "BOT_OLLAMA_MODEL") - viper.BindEnv("storage.db_path", "BOT_STORAGE_DB_PATH") - viper.BindEnv("stats.listen_address", "BOT_STATS_LISTEN_ADDRESS") - - var cfg bot.Config - if err := viper.Unmarshal(&cfg); err != nil { + cfg, err := loadConfig() + if err != nil { panic(err) } - if cfg.Storage.DBPath == "" { - cfg.Storage.DBPath = "bot.db" - } - if cfg.CommandPrefix == "" { - cfg.CommandPrefix = "!" - } - if cfg.Stats.ListenAddress == "" { - cfg.Stats.ListenAddress = "127.0.0.1" - } networks := cfg.Networks if len(networks) == 0 && cfg.Server.Host != "" { @@ -133,13 +101,37 @@ func main() { } } instances = append(instances, instance) + } + + var reloadMu sync.Mutex + reload := func(current *bot.Bot, msg bot.Message) { + reloadMu.Lock() + defer reloadMu.Unlock() + updated, err := loadConfig() + if err != nil { + current.Send(msg.ReplyTarget(), "reload failed; configuration was not changed") + log.Warn("configuration reload failed", zap.Error(err)) + return + } + count, err := current.ReloadPlugins(updated.Plugins) + if err != nil { + current.Send(msg.ReplyTarget(), "reload failed; configuration was not changed") + log.Warn("configuration reload failed", zap.Error(err)) + return + } + current.Send(msg.ReplyTarget(), fmt.Sprintf("configuration reloaded for %d plugin(s); IRC connection unchanged", count)) + log.Info("configuration reloaded", zap.Int("plugins", count), zap.String("network", current.Config.NetworkName)) + } + for _, instance := range instances { + current := instance + current.SetReloadHandler(func(msg bot.Message) { reload(current, msg) }) wg.Add(1) go func(networkName string, b *bot.Bot) { defer wg.Done() if err := b.Run(ctx); err != nil { log.Error("bot stopped", zap.String("network", networkName), zap.Error(err)) } - }(network.Name, instance) + }(current.Config.NetworkName, current) } wg.Wait() qctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) @@ -149,6 +141,57 @@ func main() { } } +func loadConfig() (bot.Config, error) { + configureViper() + if err := viper.ReadInConfig(); err != nil { + return bot.Config{}, err + } + var cfg bot.Config + if err := viper.Unmarshal(&cfg); err != nil { + return bot.Config{}, err + } + if cfg.Storage.DBPath == "" { + cfg.Storage.DBPath = "bot.db" + } + if cfg.CommandPrefix == "" { + cfg.CommandPrefix = "!" + } + if cfg.Stats.ListenAddress == "" { + cfg.Stats.ListenAddress = "127.0.0.1" + } + return cfg, nil +} + +func configureViper() { + viper.Reset() + viper.SetConfigFile("config.yaml") + viper.AutomaticEnv() + viper.SetEnvPrefix("BOT") + viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) + bind := func(key, env string) { + if err := viper.BindEnv(key, env); err != nil { + panic(err) + } + } + bind("identity.sasl_pass", "BOT_SASL_PASS") + bind("plugins.news.api_key", "BOT_NEWS_API_KEY") + bind("plugins.lastfm.api_key", "BOT_LASTFM_API_KEY") + bind("plugins.github.token", "BOT_GITHUB_TOKEN") + bind("plugins.urltitle.youtube_api_key", "BOT_YOUTUBE_API_KEY") + bind("plugins.ask.ai_rewrite", "BOT_ASK_AI_REWRITE") + bind("plugins.ask.provider", "BOT_ASK_PROVIDER") + bind("plugins.ask.openrouter_api_key", "BOT_OPENROUTER_API_KEY") + bind("plugins.ask.openrouter_model", "BOT_OPENROUTER_MODEL") + bind("plugins.ask.openai_api_key", "BOT_OPENAI_API_KEY") + bind("plugins.ask.openai_model", "BOT_OPENAI_MODEL") + bind("plugins.ask.gemini_api_key", "BOT_GEMINI_API_KEY") + bind("plugins.ask.gemini_model", "BOT_GEMINI_MODEL") + bind("plugins.ask.ollama_url", "BOT_OLLAMA_URL") + bind("plugins.ask.ollama_model", "BOT_OLLAMA_MODEL") + bind("storage.db_path", "BOT_STORAGE_DB_PATH") + bind("stats.listen_address", "BOT_STATS_LISTEN_ADDRESS") +} + func newLogger(format string) *zap.Logger { cfg := zap.NewProductionConfig() if format != "json" { diff --git a/docs/configuration.md b/docs/configuration.md index 71fdf8f..cde5bc6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -169,6 +169,25 @@ owner_accounts: GoBot records the IRCv3 `account` tag when the network provides it. There is intentionally no ownership-claim command. +### Owner-only private reload + +An owner can reload file-backed plugin settings without disconnecting GoBot: + +```text +/msg GoBot reload +/msg GoBot !reload +``` + +The sender must be identified to an IRC account listed in `owner_accounts`; a +nickname alone is not accepted. GoBot replies privately after the reload and +keeps the existing IRC connection open. + +The reload currently applies settings for active plugins that support runtime +reloads, including `ask`. It does not change the server, nickname, channels, +`owner_accounts`, plugin enable/disable flags, or per-channel plugin +overrides. Those changes require a restart. Environment variables loaded from +`.env` are process settings as well, so changing `.env` requires a restart. + Anyone may invite the bot when invitations are enabled: ```yaml diff --git a/docs/plugins.md b/docs/plugins.md index 92b53ac..71700bb 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -688,7 +688,11 @@ plugins: retrieved source and asks it to turn that source into a concise, factual, single-paragraph answer. It does not use an AI provider when `provider: none` is selected, and it falls back to the source summary if the provider is -unavailable. Keep provider credentials out of `config.yaml`: +unavailable. `BOT_ASK_PROVIDER` and `BOT_ASK_AI_REWRITE` override the +corresponding nested config values; this means `provider: none` and +`ai_rewrite: false` can remain in the tracked example while a deployment +enables rewriting through `.env`. Keep provider credentials out of +`config.yaml`: ~~~env BOT_ASK_PROVIDER=openrouter @@ -700,7 +704,28 @@ BOT_OPENROUTER_MODEL=openrouter/free OpenAI uses `BOT_OPENAI_API_KEY` and `BOT_OPENAI_MODEL`; Gemini uses `BOT_GEMINI_API_KEY` and `BOT_GEMINI_MODEL`. For a local Ollama instance, use `BOT_ASK_PROVIDER=ollama`, `BOT_OLLAMA_URL`, and `BOT_OLLAMA_MODEL`. The output -limits still apply regardless of provider. +limits still apply regardless of provider. If a provider returns meta-text +such as “the user asks” or “the source does not”, GoBot rejects it and keeps +the source-grounded answer instead. + +To verify that the key is actually being used, check the service log after one +fresh `!ask` request: + +~~~text +ask AI rewrite requested ... provider=openrouter ... api_key_configured=true +ask AI rewrite used ... provider=openrouter ... +~~~ + +If the provider call fails, GoBot logs `ask AI rewrite unavailable; using source +summary` and still returns the source answer. The API key is never written to +the log. A source-only response is usually faster; an AI rewrite adds one +external request, and free models may take longer when they are cold or busy. +Use `BOT_ASK_AI_REWRITE=false` to compare source-only behavior. + +After changing `config.yaml`, an owner can send GoBot a private `reload` +message to apply reloadable plugin settings without reconnecting. Changes to +`.env` still require a service restart because systemd reads that file when the +process starts. ## Wikipedia diff --git a/docs/security.md b/docs/security.md index 73daef6..397174e 100644 --- a/docs/security.md +++ b/docs/security.md @@ -9,6 +9,11 @@ dependencies, and deployment configuration maintained. - Keep secrets in .env or a deployment secret store, never in Git. - Use authenticated IRC account names for owner controls; nicknames are not authorization proof. +- The private `reload` command is accepted only from an authenticated account + listed in `owner_accounts`; it cannot change ownership or connection + settings. +- Configuration reloads do not reread systemd's `EnvironmentFile`; restart the + service after changing `.env` so API keys and provider settings take effect. - Restrict /stats and /metrics; they have no built-in authentication. - Bind the stats listener to localhost unless another host must scrape it. - If remote scraping is required, use a private/WireGuard address and firewall diff --git a/plugins/ask.go b/plugins/ask.go index 67776a9..ae9e09d 100644 --- a/plugins/ask.go +++ b/plugins/ask.go @@ -17,12 +17,14 @@ import ( "github.com/variablenix/GoBot/bot" "github.com/variablenix/GoBot/storage" + "go.uber.org/zap" ) // Ask answers questions from a small set of public sources. AI rewriting is // deliberately opt-in: source lookup works without a provider or API key. type Ask struct { cfg bot.PluginConfig + cfgMu sync.RWMutex mu sync.Mutex last map[string]time.Time lastWarning map[string]time.Time @@ -35,12 +37,43 @@ func (p *Ask) Help() string { } func (p *Ask) Init(c bot.PluginConfig, _ *storage.DB) error { - p.cfg = withAskEnvironment(c) + p.setConfig(withAskEnvironment(c)) p.last = make(map[string]time.Time) p.lastWarning = make(map[string]time.Time) return nil } +// Reload applies ask configuration without resetting cooldown state. Environment +// values are read from the process environment, which systemd loads at startup. +func (p *Ask) Reload(c bot.PluginConfig) error { + p.setConfig(withAskEnvironment(c)) + p.mu.Lock() + if p.last == nil { + p.last = make(map[string]time.Time) + } + if p.lastWarning == nil { + p.lastWarning = make(map[string]time.Time) + } + p.mu.Unlock() + return nil +} + +func (p *Ask) setConfig(c bot.PluginConfig) { + p.cfgMu.Lock() + p.cfg = c + p.cfgMu.Unlock() +} + +func (p *Ask) configSnapshot() bot.PluginConfig { + p.cfgMu.RLock() + defer p.cfgMu.RUnlock() + cfg := make(bot.PluginConfig, len(p.cfg)) + for key, value := range p.cfg { + cfg[key] = value + } + return cfg +} + // withAskEnvironment applies the ask-specific environment variables after // the config file has been decoded. Viper can read bound scalar environment // values with Get, but nested map values are not reliably reflected when the @@ -90,14 +123,15 @@ func (p *Ask) Handle(b *bot.Bot, m bot.Message) bool { } key := askSenderKey(m) - if !p.allow(key) { + cfg := p.configSnapshot() + if !p.allow(key, cfg.Int("cooldown_seconds", 15)) { if p.allowWarning(key) { b.Send(target, "ask is cooling down — please wait a moment") } return true } - timeout := p.cfg.Int("timeout_seconds", 12) + timeout := cfg.Int("timeout_seconds", 12) if timeout < 1 { timeout = 1 } @@ -107,22 +141,35 @@ func (p *Ask) Handle(b *bot.Bot, m bot.Message) bool { ctx, cancel := context.WithTimeout(context.Background(), time.Duration(timeout)*time.Second) defer cancel() - source, found := p.findSource(ctx, question) + source, found := p.findSource(ctx, question, cfg) if !found { b.Send(target, "I couldn't find a reliable source for that question.") return true } answer := cleanExternalText(source.Summary) - if p.cfg.Bool("ai_rewrite", false) && strings.ToLower(strings.TrimSpace(p.cfg.String("provider", "none"))) != "none" { - if rewritten, ok := p.rewrite(ctx, question, source); ok { - answer = cleanExternalText(rewritten) + provider := strings.ToLower(strings.TrimSpace(cfg.String("provider", "none"))) + if cfg.Bool("ai_rewrite", false) && provider != "none" { + model, keyConfigured := askProviderInfo(provider, cfg) + if b.Log != nil { + b.Log.Info("ask AI rewrite requested", zap.String("provider", provider), zap.String("model", model), zap.Bool("api_key_configured", keyConfigured)) + } + started := time.Now() + if rewritten, ok := p.rewriteWithConfig(ctx, question, source, cfg); ok { + if rewritten = cleanExternalText(rewritten); usableAskRewrite(rewritten) { + answer = rewritten + if b.Log != nil { + b.Log.Info("ask AI rewrite used", zap.String("provider", provider), zap.Duration("duration", time.Since(started))) + } + } + } else if b.Log != nil { + b.Log.Warn("ask AI rewrite unavailable; using source summary", zap.String("provider", provider), zap.String("model", model), zap.Duration("duration", time.Since(started))) } } if answer == "" { answer = "I found a source, but it did not include a usable summary." } - b.Send(target, formatAskResponse(m.Nick, answer, source.URL, p.cfg.Int("max_length", 360), p.cfg.Int("max_response_chars", 240))) + b.Send(target, formatAskResponse(m.Nick, answer, source.URL, cfg.Int("max_length", 360), cfg.Int("max_response_chars", 240))) return true } @@ -130,6 +177,28 @@ func isAskCommand(command string) bool { return command == "ask" || command == "question" || command == "q" } +func askProviderInfo(provider string, cfg bot.PluginConfig) (string, bool) { + provider = strings.ToLower(strings.TrimSpace(provider)) + switch provider { + case "openrouter": + model := firstAskValue(cfg.String("openrouter_model", ""), os.Getenv("BOT_OPENROUTER_MODEL"), "openrouter/free") + key := firstAskValue(cfg.String("openrouter_api_key", ""), os.Getenv("BOT_OPENROUTER_API_KEY")) + return model, key != "" + case "openai": + model := firstAskValue(cfg.String("openai_model", ""), os.Getenv("BOT_OPENAI_MODEL"), "gpt-4o-mini") + key := firstAskValue(cfg.String("openai_api_key", ""), os.Getenv("BOT_OPENAI_API_KEY")) + return model, key != "" + case "gemini": + model := firstAskValue(cfg.String("gemini_model", ""), os.Getenv("BOT_GEMINI_MODEL"), "gemini-2.0-flash") + key := firstAskValue(cfg.String("gemini_api_key", ""), os.Getenv("BOT_GEMINI_API_KEY")) + return model, key != "" + case "ollama": + return firstAskValue(cfg.String("ollama_model", ""), os.Getenv("BOT_OLLAMA_MODEL"), "llama3.2"), true + default: + return "", false + } +} + func askSenderKey(m bot.Message) string { if account := strings.TrimSpace(m.Account); account != "" { return "account:" + strings.ToLower(account) @@ -137,8 +206,7 @@ func askSenderKey(m bot.Message) string { return "sender:" + strings.ToLower(strings.Join([]string{m.Nick, m.User, m.Host}, "\x00")) } -func (p *Ask) allow(key string) bool { - cooldown := p.cfg.Int("cooldown_seconds", 15) +func (p *Ask) allow(key string, cooldown int) bool { if cooldown <= 0 { return true } @@ -163,15 +231,35 @@ func (p *Ask) allowWarning(key string) bool { return true } +func usableAskRewrite(answer string) bool { + answer = strings.ToLower(strings.TrimSpace(answer)) + if answer == "" { + return false + } + for _, phrase := range []string{ + "the user asks", + "the source does not", + "the source is", + "the answer should be", + "according to the source", + "not enough information in this source", + } { + if strings.Contains(answer, phrase) { + return false + } + } + return true +} + type askSource struct { Title string Summary string URL string } -func (p *Ask) findSource(ctx context.Context, question string) (askSource, bool) { - tryWikipedia := p.cfg.Bool("wikipedia_first", true) - tryDuckDuckGo := p.cfg.Bool("duckduckgo_fallback", true) +func (p *Ask) findSource(ctx context.Context, question string, cfg bot.PluginConfig) (askSource, bool) { + tryWikipedia := cfg.Bool("wikipedia_first", true) + tryDuckDuckGo := cfg.Bool("duckduckgo_fallback", true) if tryWikipedia { if source, ok := askWikipedia(ctx, question); ok { return source, true @@ -375,33 +463,37 @@ type askChatResponse struct { } func (p *Ask) rewrite(ctx context.Context, question string, source askSource) (string, bool) { - provider := strings.ToLower(strings.TrimSpace(p.cfg.String("provider", "none"))) - limit := clampAskLength(p.cfg.Int("max_response_chars", 240), 80, 320, 240) - prompt := fmt.Sprintf("Question: %s\nSource title: %s\nSource text: %s\n\nAnswer in one concise plain-text paragraph, using only the source. Do not use markdown, lists, or line breaks. Keep it under %d characters. If the source does not answer the question, say that it cannot be verified from this source.", question, cleanExternalText(source.Title), truncateAsk(source.Summary, 2000), limit) - system := "You are GoBot's concise IRC answer editor. Be factual, clear, and cautious. Never invent details beyond the supplied source." + return p.rewriteWithConfig(ctx, question, source, p.configSnapshot()) +} + +func (p *Ask) rewriteWithConfig(ctx context.Context, question string, source askSource, cfg bot.PluginConfig) (string, bool) { + provider := strings.ToLower(strings.TrimSpace(cfg.String("provider", "none"))) + limit := clampAskLength(cfg.Int("max_response_chars", 240), 80, 320, 240) + prompt := fmt.Sprintf("Question: %s\nSource title: %s\nSource text: %s\n\nAnswer the question directly in one concise plain-text paragraph, using only the source. Do not mention the user, the question, the source, or your instructions. Do not use markdown, lists, or line breaks. Keep it under %d characters. If the source does not answer the question, say only: Not enough information in this source.", question, cleanExternalText(source.Title), truncateAsk(source.Summary, 2000), limit) + system := "You are GoBot's concise IRC answer editor. Start with the answer, not meta-commentary. Be factual, clear, and cautious. Never invent details beyond the supplied source." switch provider { case "openrouter": - return p.openAICompatible(ctx, "openrouter", system, prompt) + return p.openAICompatible(ctx, "openrouter", system, prompt, cfg) case "openai": - return p.openAICompatible(ctx, "openai", system, prompt) + return p.openAICompatible(ctx, "openai", system, prompt, cfg) case "gemini": - return p.gemini(ctx, system, prompt) + return p.gemini(ctx, system, prompt, cfg) case "ollama": - return p.ollama(ctx, system, prompt) + return p.ollama(ctx, system, prompt, cfg) default: return "", false } } -func (p *Ask) openAICompatible(ctx context.Context, provider, system, prompt string) (string, bool) { +func (p *Ask) openAICompatible(ctx context.Context, provider, system, prompt string, cfg bot.PluginConfig) (string, bool) { var key, model, endpoint string if provider == "openrouter" { - key = firstAskValue(p.cfg.String("openrouter_api_key", ""), os.Getenv("BOT_OPENROUTER_API_KEY")) - model = firstAskValue(p.cfg.String("openrouter_model", ""), os.Getenv("BOT_OPENROUTER_MODEL"), "openrouter/free") + key = firstAskValue(cfg.String("openrouter_api_key", ""), os.Getenv("BOT_OPENROUTER_API_KEY")) + model = firstAskValue(cfg.String("openrouter_model", ""), os.Getenv("BOT_OPENROUTER_MODEL"), "openrouter/free") endpoint = "https://openrouter.ai/api/v1/chat/completions" } else { - key = firstAskValue(p.cfg.String("openai_api_key", ""), os.Getenv("BOT_OPENAI_API_KEY")) - model = firstAskValue(p.cfg.String("openai_model", ""), os.Getenv("BOT_OPENAI_MODEL"), "gpt-4o-mini") + key = firstAskValue(cfg.String("openai_api_key", ""), os.Getenv("BOT_OPENAI_API_KEY")) + model = firstAskValue(cfg.String("openai_model", ""), os.Getenv("BOT_OPENAI_MODEL"), "gpt-4o-mini") endpoint = "https://api.openai.com/v1/chat/completions" } if key == "" { @@ -437,9 +529,9 @@ func (p *Ask) openAICompatible(ctx context.Context, provider, system, prompt str return response.Choices[0].Message.Content, strings.TrimSpace(response.Choices[0].Message.Content) != "" } -func (p *Ask) gemini(ctx context.Context, system, prompt string) (string, bool) { - key := firstAskValue(p.cfg.String("gemini_api_key", ""), os.Getenv("BOT_GEMINI_API_KEY")) - model := firstAskValue(p.cfg.String("gemini_model", ""), os.Getenv("BOT_GEMINI_MODEL"), "gemini-2.0-flash") +func (p *Ask) gemini(ctx context.Context, system, prompt string, cfg bot.PluginConfig) (string, bool) { + key := firstAskValue(cfg.String("gemini_api_key", ""), os.Getenv("BOT_GEMINI_API_KEY")) + model := firstAskValue(cfg.String("gemini_model", ""), os.Getenv("BOT_GEMINI_MODEL"), "gemini-2.0-flash") if key == "" { return "", false } @@ -507,9 +599,9 @@ func (p *Ask) gemini(ctx context.Context, system, prompt string) (string, bool) return response.Candidates[0].Content.Parts[0].Text, true } -func (p *Ask) ollama(ctx context.Context, system, prompt string) (string, bool) { - model := firstAskValue(p.cfg.String("ollama_model", ""), os.Getenv("BOT_OLLAMA_MODEL"), "llama3.2") - base := firstAskValue(p.cfg.String("ollama_url", ""), os.Getenv("BOT_OLLAMA_URL"), "http://127.0.0.1:11434") +func (p *Ask) ollama(ctx context.Context, system, prompt string, cfg bot.PluginConfig) (string, bool) { + model := firstAskValue(cfg.String("ollama_model", ""), os.Getenv("BOT_OLLAMA_MODEL"), "llama3.2") + base := firstAskValue(cfg.String("ollama_url", ""), os.Getenv("BOT_OLLAMA_URL"), "http://127.0.0.1:11434") parsed, err := url.Parse(strings.TrimRight(base, "/") + "/api/chat") if err != nil || (parsed.Scheme != "http" && parsed.Scheme != "https") || parsed.Host == "" { return "", false diff --git a/plugins/ask_test.go b/plugins/ask_test.go index 8341075..5fd4c25 100644 --- a/plugins/ask_test.go +++ b/plugins/ask_test.go @@ -3,6 +3,7 @@ package plugins import ( "strings" "testing" + "time" "github.com/variablenix/GoBot/bot" ) @@ -58,10 +59,11 @@ func TestAskEnvironmentOverridesNestedConfig(t *testing.T) { }, nil); err != nil { t.Fatalf("init failed: %v", err) } - if got := p.cfg.String("provider", "none"); got != "openrouter" { + cfg := p.configSnapshot() + if got := cfg.String("provider", "none"); got != "openrouter" { t.Fatalf("provider = %q, want openrouter", got) } - if !p.cfg.Bool("ai_rewrite", false) { + if !cfg.Bool("ai_rewrite", false) { t.Fatal("ai_rewrite remained disabled despite BOT_ASK_AI_REWRITE=true") } } @@ -73,14 +75,59 @@ func TestAskInvalidEnvironmentBooleanLeavesConfigUnchanged(t *testing.T) { if err := p.Init(bot.PluginConfig{"ai_rewrite": true}, nil); err != nil { t.Fatalf("init failed: %v", err) } - if !p.cfg.Bool("ai_rewrite", false) { + if !p.configSnapshot().Bool("ai_rewrite", false) { t.Fatal("invalid environment boolean unexpectedly changed config") } } +func TestAskProviderInfoUsesEnvironmentCredentials(t *testing.T) { + t.Setenv("BOT_OPENROUTER_API_KEY", "secret") + t.Setenv("BOT_OPENROUTER_MODEL", "nvidia/test:free") + model, configured := askProviderInfo("openrouter", bot.PluginConfig{"provider": "openrouter"}) + if model != "nvidia/test:free" || !configured { + t.Fatalf("provider info = (%q, %v), want configured test model", model, configured) + } +} + +func TestAskReloadUpdatesConfigAndPreservesCooldownState(t *testing.T) { + t.Setenv("BOT_ASK_PROVIDER", "") + t.Setenv("BOT_ASK_AI_REWRITE", "") + p := &Ask{} + if err := p.Init(bot.PluginConfig{"provider": "none", "ai_rewrite": false}, nil); err != nil { + t.Fatalf("init failed: %v", err) + } + p.last["account:test"] = time.Now() + if err := p.Reload(bot.PluginConfig{"provider": "openrouter", "ai_rewrite": true}); err != nil { + t.Fatalf("reload failed: %v", err) + } + cfg := p.configSnapshot() + if cfg.String("provider", "none") != "openrouter" || !cfg.Bool("ai_rewrite", false) { + t.Fatalf("reload did not update config: %#v", cfg) + } + if _, ok := p.last["account:test"]; !ok { + t.Fatal("reload discarded cooldown state") + } +} + func TestAskSenderKeyUsesAccountWhenAvailable(t *testing.T) { key := askSenderKey(bot.Message{Nick: "Echo", Account: "UserAccount"}) if key != "account:useraccount" { t.Fatalf("got %q", key) } } + +func TestUsableAskRewriteRejectsProviderMetaText(t *testing.T) { + for _, answer := range []string{ + "The user asks: what is Linux?", + "The source does not define that topic.", + "According to the source, the answer is unclear.", + "Not enough information in this source.", + } { + if usableAskRewrite(answer) { + t.Errorf("usableAskRewrite(%q) = true, want false", answer) + } + } + if !usableAskRewrite("Linux is a family of open-source operating systems.") { + t.Fatal("usableAskRewrite rejected a direct factual answer") + } +} diff --git a/plugins/wikipedia.go b/plugins/wikipedia.go index 359a542..e6c8621 100644 --- a/plugins/wikipedia.go +++ b/plugins/wikipedia.go @@ -7,6 +7,7 @@ import ( "net/url" "strings" "time" + "unicode" "github.com/variablenix/GoBot/bot" "github.com/variablenix/GoBot/storage" @@ -59,12 +60,19 @@ type wikipediaSummaryResult struct { } func wikipediaSummary(ctx context.Context, query string) (wikipediaSummaryResult, bool) { - result, ok := wikipediaSummaryPage(ctx, query) - if ok { + searchTerm := wikipediaSearchTerm(query) + if searchTerm == "" { + searchTerm = strings.TrimSpace(query) + } + + // Try the cleaned topic first. This avoids sending a full conversational + // question to the page endpoint, where an accidental redirect can look like + // a successful but unrelated answer. + if result, ok := wikipediaSummaryPage(ctx, searchTerm); ok && wikipediaTitleMatches(searchTerm, result.Title) { return result, true } - searchURL := "https://en.wikipedia.org/w/api.php?action=query&list=search&srsearch=" + url.QueryEscape(query) + "&format=json&utf8=1&srlimit=1" + searchURL := "https://en.wikipedia.org/w/api.php?action=query&list=search&srsearch=" + url.QueryEscape(searchTerm) + "&format=json&utf8=1&srlimit=8" req, err := wikipediaRequest(ctx, searchURL) if err != nil { return wikipediaSummaryResult{}, false @@ -87,7 +95,71 @@ func wikipediaSummary(ctx context.Context, query string) (wikipediaSummaryResult if err := json.NewDecoder(res.Body).Decode(&search); err != nil || len(search.Query.Search) == 0 { return wikipediaSummaryResult{}, false } - return wikipediaSummaryPage(ctx, search.Query.Search[0].Title) + for _, candidate := range search.Query.Search { + if !wikipediaTitleMatches(searchTerm, candidate.Title) { + continue + } + if result, ok := wikipediaSummaryPage(ctx, candidate.Title); ok { + return result, true + } + } + return wikipediaSummaryResult{}, false +} + +// wikipediaSearchTerm turns common conversational question forms into a +// focused topic. Wikipedia's search endpoint otherwise tends to rank an +// incidental word in a question above the article the user meant. +func wikipediaSearchTerm(query string) string { + words := strings.FieldsFunc(strings.ToLower(strings.TrimSpace(query)), func(r rune) bool { + return !unicode.IsLetter(r) && !unicode.IsNumber(r) + }) + stop := map[string]struct{}{ + "a": {}, "about": {}, "an": {}, "and": {}, "are": {}, "can": {}, + "could": {}, "current": {}, "currently": {}, "define": {}, "did": {}, + "do": {}, "does": {}, "exactly": {}, "explain": {}, "for": {}, + "happening": {}, "how": {}, "in": {}, "is": {}, "it": {}, "latest": {}, + "me": {}, "more": {}, "now": {}, "of": {}, "on": {}, "please": {}, + "recent": {}, "tell": {}, "the": {}, "to": {}, "today": {}, "was": {}, + "what": {}, "when": {}, "where": {}, "who": {}, "why": {}, "would": {}, + "you": {}, "know": {}, + } + filtered := make([]string, 0, len(words)) + for _, word := range words { + if _, ignored := stop[word]; !ignored { + filtered = append(filtered, word) + } + } + return strings.Join(filtered, " ") +} + +// wikipediaTitleMatches prevents a loosely related first search result from +// being presented as an answer. One-word topics must appear in the title. For +// multi-word topics, require two matching words, or the first topic word (the +// usual subject) to match. This keeps useful lookups such as "TLS protect" +// while rejecting "UFO disclosure" -> "Disclosure Day (soundtrack)". +func wikipediaTitleMatches(topic, title string) bool { + topicWords := strings.Fields(wikipediaSearchTerm(topic)) + titleWords := strings.Fields(wikipediaSearchTerm(title)) + if len(topicWords) == 0 || len(titleWords) == 0 { + return false + } + titleSet := make(map[string]struct{}, len(titleWords)) + for _, word := range titleWords { + titleSet[word] = struct{}{} + } + matches := 0 + for _, word := range topicWords { + if _, ok := titleSet[word]; ok { + matches++ + } + } + if len(topicWords) == 1 { + return matches == 1 + } + return matches >= 2 || func() bool { + _, ok := titleSet[topicWords[0]] + return ok + }() } func wikipediaSummaryPage(ctx context.Context, title string) (wikipediaSummaryResult, bool) { diff --git a/plugins/wikipedia_test.go b/plugins/wikipedia_test.go index d45417e..e90575f 100644 --- a/plugins/wikipedia_test.go +++ b/plugins/wikipedia_test.go @@ -17,3 +17,37 @@ func TestWikipediaRequestSetsDescriptiveUserAgent(t *testing.T) { t.Fatalf("got Accept %q", got) } } + +func TestWikipediaSearchTermFocusesConversationalQuestions(t *testing.T) { + tests := map[string]string{ + "what is Linux exactly? Do you know?": "linux", + "What is Ubuntu?": "ubuntu", + "is UFO disclosure happening more now?": "ufo disclosure", + "How does TLS protect web traffic?": "tls protect web traffic", + "tell me about the history of video games": "history video games", + } + for input, want := range tests { + if got := wikipediaSearchTerm(input); got != want { + t.Errorf("wikipediaSearchTerm(%q) = %q, want %q", input, got, want) + } + } +} + +func TestWikipediaTitleMatchesRejectsIncidentalResults(t *testing.T) { + tests := []struct { + topic string + title string + want bool + }{ + {topic: "Linux", title: "Linux", want: true}, + {topic: "Ubuntu", title: "Ubuntu", want: true}, + {topic: "UFO disclosure", title: "Disclosure Day (soundtrack)", want: false}, + {topic: "TLS protect", title: "Transport Layer Security", want: false}, + {topic: "TLS protect", title: "TLS", want: true}, + } + for _, test := range tests { + if got := wikipediaTitleMatches(test.topic, test.title); got != test.want { + t.Errorf("wikipediaTitleMatches(%q, %q) = %v, want %v", test.topic, test.title, got, test.want) + } + } +}