diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..6dc01d2 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,102 @@ +# AGENTS.md + +## Purpose +This repository uses Codex for full-project Go code review, not diff-only review. + +When asked to review code, inspect the entire repository and use repository-wide context. Do not limit analysis to the latest commit, pull request diff, or recently changed files. + +## Review priorities +Review the codebase with focus on: +- correctness and reliability; +- maintainability and architecture; +- idiomatic Go; +- testability; +- performance where justified by code evidence; +- security; +- godoc quality. + +## Scope rules +- Always review the whole repository unless the prompt explicitly narrows scope. +- Check cross-package interactions, public APIs, package boundaries, and shared patterns. +- Prefer concrete fixes over generic advice. +- When feasible, make small, high-confidence improvements directly. +- When uncertain, state confidence level and evidence. + +## Go review expectations +Check for: +- bugs, fragile logic, invalid assumptions, nil handling issues, resource leaks; +- poor error handling; +- misuse of context, cancellation, timeouts, retries, and cleanup; +- race risks, deadlocks, blocking hazards, unsafe shared state; +- non-idiomatic naming, APIs, interfaces, package structure, and error patterns; +- unnecessary complexity, duplication, or weak abstractions; +- obvious performance problems supported by the code; +- security risks such as unsafe input handling, secret leakage, insecure logging, injection risks, and risky file or network operations. + +## Godoc rules +Review comments for all declarations. + +### Exported declarations +Exported types, funcs, methods, vars, and consts must have godoc comments. + +Each exported godoc comment must: +- start with the identifier name; +- explain the purpose or behavior; +- be as short as possible without losing important meaning; +- avoid repeating the signature mechanically; +- stay high-signal and informative. + +### Unexported declarations +Unexported types, funcs, methods, vars, and consts should generally not have godoc-style comments unless there is a strong reason. + +### Always report +- missing godoc on exported declarations; +- unnecessary godoc on unexported declarations; +- comments that are too long, vague, redundant, or low-value; +- comments that should be shortened or rewritten. + +When feasible, rewrite bad godoc into better versions. + +## Testing expectations +Treat tests as a required part of review. + +- Assess existing test quality, not only test presence. +- Add or propose as many useful tests as reasonably possible. +- Prioritize public APIs, critical flows, edge cases, negative paths, boundary conditions, and concurrency-sensitive logic. +- Prefer table-driven tests where appropriate. +- Add regression tests for bugs you find. +- If a case is hard to test directly, explain the gap and the best test strategy. + +## Commands +Before finalizing changes, run the relevant project checks when available: +- build +- tests +- lint +- static analysis + +Prefer the repository’s documented commands. If multiple choices exist, use the most standard and least destructive ones first. + +## Output format +For repo-wide review tasks, structure the result as: + +1. Overall summary +2. Critical findings +3. Major findings +4. Minor findings +5. Godoc issues +6. Test gaps and added/proposed tests +7. Good decisions worth keeping +8. Summary of concrete changes made + +For each finding include: +- location; +- issue; +- why it matters; +- recommended fix. + +## Working style +- Be direct, specific, and action-oriented. +- Do not stop at style-only feedback. +- Use full repository context before drawing conclusions. +- Prefer minimal, high-confidence patches. +- Preserve behavior unless intentionally fixing a bug. diff --git a/bot.go b/bot.go index f36bb71..40dccae 100644 --- a/bot.go +++ b/bot.go @@ -242,9 +242,10 @@ func (bot *Bot[T]) initLoggers(opts *BotOpts) { path := fmt.Sprintf("%s/main.log", strings.TrimRight(opts.LoggerBasePath, "/")) logger, err := utils.CreateFileLogger("BOT", level, path) if err != nil { - bot.logger.Fatal(err) + bot.logger.Errorln(err) + } else { + bot.logger = logger } - bot.logger = logger } if opts.UseRequestLogger { @@ -253,9 +254,10 @@ func (bot *Bot[T]) initLoggers(opts *BotOpts) { path := fmt.Sprintf("%s/requests.log", strings.TrimRight(opts.LoggerBasePath, "/")) logger, err := utils.CreateFileLogger("REQUESTS", level, path) if err != nil { - bot.logger.Fatal(err) + bot.logger.Errorln(err) + } else { + bot.RequestLogger = logger } - bot.RequestLogger = logger } } } @@ -275,7 +277,9 @@ func (bot *Bot[T]) SetUpdateOffset(offset int) { } // GetUpdateTypes returns the list of update types the bot is configured to receive. -func (bot *Bot[T]) GetUpdateTypes() []tgapi.UpdateType { return bot.updateTypes } +func (bot *Bot[T]) GetUpdateTypes() []tgapi.UpdateType { + return append([]tgapi.UpdateType(nil), bot.updateTypes...) +} // GetLogger returns the main bot logger. func (bot *Bot[T]) GetLogger() *slog.Logger { return bot.logger } @@ -383,12 +387,12 @@ func (bot *Bot[T]) Debug(debug bool) *Bot[T] { func (bot *Bot[T]) AddPlugins(plugin ...*Plugin[T]) *Bot[T] { level := bot.GetLoggerLevel() for _, p := range plugin { - if p.logger == nil { - logger := utils.CreateLogger(p.name, level) - p.SetLogger(logger) + cloned := clonePlugin(p) + if cloned.logger == nil { + cloned.logger = utils.CreateLogger(cloned.name, level) } - bot.plugins = append(bot.plugins, *p) - bot.logger.Debugln(fmt.Sprintf("plugins with name \"%s\" registered", p.name)) + bot.plugins = append(bot.plugins, cloned) + bot.logger.Debugln(fmt.Sprintf("plugins with name \"%s\" registered", cloned.name)) } return bot } @@ -535,12 +539,12 @@ func (bot *Bot[T]) AddDatabaseLoggerWriter(writer DbLogger[T]) *Bot[T] { // _ = bot.Close(context.Background()) func (bot *Bot[T]) RunWithContext(ctx context.Context) { if len(bot.prefixes) == 0 { - bot.logger.Fatalln("no prefixes defined") + bot.logger.Errorln("no prefixes defined") return } if len(bot.plugins) == 0 { - bot.logger.Fatalln("no plugins defined") + bot.logger.Errorln("no plugins defined") return } @@ -604,3 +608,35 @@ func (bot *Bot[T]) RunWithContext(ctx context.Context) { func (bot *Bot[T]) Run() { bot.RunWithContext(context.Background()) } + +func clonePlugin[T DbContext](p *Plugin[T]) Plugin[T] { + cloned := Plugin[T]{ + name: p.name, + commands: make(map[string]*Command[T], len(p.commands)), + payloads: make(map[string]*Command[T], len(p.payloads)), + middlewares: append(extypes.Slice[Middleware[T]](nil), p.middlewares...), + skipAutoCmd: p.skipAutoCmd, + logger: p.logger, + onClose: p.onClose, + } + + for name, command := range p.commands { + cloned.commands[name] = cloneCommand(command) + } + for name, command := range p.payloads { + cloned.payloads[name] = cloneCommand(command) + } + + return cloned +} + +func cloneCommand[T DbContext](command *Command[T]) *Command[T] { + if command == nil { + return nil + } + + cloned := *command + cloned.args = append(extypes.Slice[CommandArg](nil), command.args...) + cloned.middlewares = append(extypes.Slice[Middleware[T]](nil), command.middlewares...) + return &cloned +} diff --git a/bot_test.go b/bot_test.go new file mode 100644 index 0000000..0d4d671 --- /dev/null +++ b/bot_test.go @@ -0,0 +1,70 @@ +package laniakea + +import ( + "path/filepath" + "reflect" + "testing" + + "git.nix13.pw/scuroneko/laniakea/tgapi" + "git.nix13.pw/scuroneko/slog" +) + +func TestGetUpdateTypesReturnsCopy(t *testing.T) { + bot := &Bot[NoDB]{updateTypes: []tgapi.UpdateType{tgapi.UpdateTypeMessage}} + + got := bot.GetUpdateTypes() + got[0] = tgapi.UpdateTypeCallbackQuery + + if want := []tgapi.UpdateType{tgapi.UpdateTypeMessage}; !reflect.DeepEqual(bot.updateTypes, want) { + t.Fatalf("GetUpdateTypes exposed internal slice: got %v want %v", bot.updateTypes, want) + } +} + +func TestAddPluginsSnapshotsConfiguration(t *testing.T) { + bot := &Bot[NoDB]{logger: slog.CreateLogger()} + plugin := NewPlugin[NoDB]("demo") + + cmd := plugin.NewCommand(func(ctx *MsgContext, db *NoDB) {}, "start") + plugin.AddMiddleware(*NewMiddleware("base", func(ctx *MsgContext, db *NoDB) bool { return true })) + + bot.AddPlugins(plugin) + + cmd.SetDescription("mutated after registration") + plugin.NewCommand(func(ctx *MsgContext, db *NoDB) {}, "late") + plugin.AddMiddleware(*NewMiddleware("late", func(ctx *MsgContext, db *NoDB) bool { return true })) + + registered := bot.plugins[0] + if _, exists := registered.commands["late"]; exists { + t.Fatal("late command leaked into registered plugin snapshot") + } + if registered.commands["start"].description != "" { + t.Fatalf("registered command description unexpectedly mutated: %q", registered.commands["start"].description) + } + if len(registered.middlewares) != 1 { + t.Fatalf("registered middlewares unexpectedly mutated: got %d want 1", len(registered.middlewares)) + } +} + +func TestInitLoggersFallsBackToStdoutLoggerOnFileError(t *testing.T) { + bot := &Bot[NoDB]{} + + bot.initLoggers(&BotOpts{ + Debug: true, + WriteToFile: true, + UseRequestLogger: true, + LoggerBasePath: filepath.Join(t.TempDir(), "missing", "nested"), + }) + + if bot.logger == nil { + t.Fatal("expected main logger fallback") + } + if bot.RequestLogger == nil { + t.Fatal("expected request logger fallback") + } + if err := bot.RequestLogger.Close(); err != nil { + t.Fatalf("failed to close request logger: %v", err) + } + if err := bot.logger.Close(); err != nil { + t.Fatalf("failed to close main logger: %v", err) + } +} diff --git a/drafts.go b/drafts.go index ff49b59..168adca 100644 --- a/drafts.go +++ b/drafts.go @@ -38,12 +38,9 @@ func (g *LinearDraftIdGenerator) Next() uint64 { return g.lastId.Add(1) } -// DraftProvider manages a collection of Drafts and provides methods to create and -// configure them. It holds shared configuration (chat, parse mode, entities) and -// a draft ID generator. +// DraftProvider manages a collection of Drafts and a shared draft ID generator. // -// DraftProvider is NOT thread-safe. Concurrent access from multiple goroutines -// requires external synchronization. +// DraftProvider is safe for concurrent use. type DraftProvider struct { mu sync.RWMutex api *tgapi.API @@ -133,10 +130,7 @@ type Draft struct { // NewDraft creates a new draft with the provided parse mode. // -// The draft inherits the provider's chatID, messageThreadID, and entities. -// If parseMode is zero, the provider's default parseMode is used. -// -// Panics if chatID is zero — call SetChat() on the provider first. +// The caller must set a chat with SetChat before Push or Flush. func (p *DraftProvider) NewDraft(parseMode tgapi.ParseMode) *Draft { id := p.generator.Next() draft := &Draft{ @@ -224,6 +218,9 @@ func (d *Draft) Flush() error { if d.Message == "" { return nil } + if d.chatID == 0 { + return ErrDraftChatIDZero + } params := tgapi.SendMessageP{ ChatID: d.chatID, diff --git a/drafts_test.go b/drafts_test.go new file mode 100644 index 0000000..5e10497 --- /dev/null +++ b/drafts_test.go @@ -0,0 +1,36 @@ +package laniakea + +import ( + "testing" + + "git.nix13.pw/scuroneko/laniakea/tgapi" + "git.nix13.pw/scuroneko/slog" +) + +func TestDraftFlushRequiresChatID(t *testing.T) { + draft := NewRandomDraftProvider(&tgapi.API{}).NewDraft(tgapi.ParseNone) + draft.Message = "hello" + + if err := draft.Flush(); err != ErrDraftChatIDZero { + t.Fatalf("expected ErrDraftChatIDZero, got %v", err) + } +} + +func TestMsgContextNewDraftWorksWithoutLimiter(t *testing.T) { + ctx := &MsgContext{ + Api: &tgapi.API{}, + Msg: &tgapi.Message{ + Chat: &tgapi.Chat{ID: 42, Type: string(tgapi.ChatTypePrivate)}, + }, + Logger: slog.CreateLogger(), + draftProvider: NewRandomDraftProvider(&tgapi.API{}), + } + + draft := ctx.NewDraft() + if draft == nil { + t.Fatal("expected draft") + } + if draft.chatID != 42 { + t.Fatalf("unexpected chat id: %d", draft.chatID) + } +} diff --git a/msg_context.go b/msg_context.go index 4dbf52d..43e640c 100644 --- a/msg_context.go +++ b/msg_context.go @@ -422,13 +422,23 @@ func (ctx *MsgContext) newDraft(parseMode tgapi.ParseMode) *Draft { ctx.Logger.Errorln("can't create draft: ctx.Msg is nil") return nil } - - c, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - if err := ctx.Api.Limiter.Wait(c, ctx.Msg.Chat.ID); err != nil { - ctx.Logger.Errorln(err) + if ctx.Api == nil { + ctx.Logger.Errorln("can't create draft: ctx.Api is nil") return nil } + if ctx.draftProvider == nil { + ctx.Logger.Errorln("can't create draft: ctx.draftProvider is nil") + return nil + } + + if ctx.Api.Limiter != nil { + c, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := ctx.Api.Limiter.Wait(c, ctx.Msg.Chat.ID); err != nil { + ctx.Logger.Errorln(err) + return nil + } + } draft := ctx.draftProvider.NewDraft(parseMode).SetChat(ctx.Msg.Chat.ID, ctx.Msg.MessageThreadID) return draft diff --git a/tgapi/methods_types.go b/tgapi/methods_types.go index 7e8a103..ea4294a 100644 --- a/tgapi/methods_types.go +++ b/tgapi/methods_types.go @@ -10,8 +10,8 @@ const ( ParseHTML ParseMode = "HTML" // ParseMD enables legacy Markdown style parsing. ParseMD ParseMode = "Markdown" - // ParseNone disables any parsing. - ParseNone ParseMode = "None" + // ParseNone disables parse_mode and leaves plain-text requests unannotated. + ParseNone ParseMode = "" ) // EmptyParams is a placeholder for methods that take no parameters. diff --git a/tgapi/parse_mode_test.go b/tgapi/parse_mode_test.go new file mode 100644 index 0000000..c2ec740 --- /dev/null +++ b/tgapi/parse_mode_test.go @@ -0,0 +1,37 @@ +package tgapi + +import ( + "encoding/json" + "strings" + "testing" +) + +func TestParseNoneOmitsParseModeInJSON(t *testing.T) { + data, err := json.Marshal(SendMessageP{ + ChatID: 42, + Text: "hello", + ParseMode: ParseNone, + }) + if err != nil { + t.Fatalf("Marshal returned error: %v", err) + } + + if strings.Contains(string(data), `"parse_mode"`) { + t.Fatalf("expected parse_mode to be omitted, got %s", string(data)) + } +} + +func TestParseModeStillSerializesExplicitModes(t *testing.T) { + data, err := json.Marshal(SendMessageP{ + ChatID: 42, + Text: "hello", + ParseMode: ParseMDV2, + }) + if err != nil { + t.Fatalf("Marshal returned error: %v", err) + } + + if !strings.Contains(string(data), `"parse_mode":"MarkdownV2"`) { + t.Fatalf("expected MarkdownV2 parse_mode, got %s", string(data)) + } +} diff --git a/tgapi/uploader_api.go b/tgapi/uploader_api.go index c244020..7e87723 100644 --- a/tgapi/uploader_api.go +++ b/tgapi/uploader_api.go @@ -7,6 +7,7 @@ import ( "mime/multipart" "net/http" "path/filepath" + "strings" "time" "git.nix13.pw/scuroneko/laniakea/utils" @@ -241,7 +242,7 @@ func prepareMultipart[P any](files []UploaderFile, params P) (*bytes.Buffer, str // uploaderTypeByExt infers the Telegram upload field name from a file extension. // Falls back to UploaderDocumentType for unrecognized extensions. func uploaderTypeByExt(filename string) UploaderFileType { - ext := filepath.Ext(filename) + ext := strings.ToLower(filepath.Ext(filename)) switch ext { case ".jpg", ".jpeg", ".png", ".webp", ".bmp": return UploaderPhotoType diff --git a/tgapi/uploader_api_test.go b/tgapi/uploader_api_test.go index cbbfebc..54f0d3c 100644 --- a/tgapi/uploader_api_test.go +++ b/tgapi/uploader_api_test.go @@ -104,6 +104,27 @@ func TestUploaderEncodesJSONFieldsAndLeavesAcceptEncodingToHTTPTransport(t *test } } +func TestNewUploaderFileDetectsFileTypeCaseInsensitively(t *testing.T) { + tests := []struct { + name string + filename string + want UploaderFileType + }{ + {name: "uppercase photo", filename: "PHOTO.JPG", want: UploaderPhotoType}, + {name: "uppercase voice", filename: "voice.OGG", want: UploaderVoiceType}, + {name: "unknown defaults to document", filename: "archive.BIN", want: UploaderDocumentType}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + file := NewUploaderFile(tt.filename, []byte("x")) + if file.field != tt.want { + t.Fatalf("unexpected uploader field: got %q want %q", file.field, tt.want) + } + }) + } +} + func readMultipartRequest(req *http.Request) (map[string]string, string, []byte, error) { _, params, err := mime.ParseMediaType(req.Header.Get("Content-Type")) if err != nil { diff --git a/utils/limiter_test.go b/utils/limiter_test.go new file mode 100644 index 0000000..ab69db9 --- /dev/null +++ b/utils/limiter_test.go @@ -0,0 +1,41 @@ +package utils + +import ( + "context" + "errors" + "testing" + "time" +) + +func TestRateLimiterCheckDropOverflowHonorsGlobalLock(t *testing.T) { + rl := NewRateLimiter() + rl.SetGlobalLock(1) + + if err := rl.Check(context.Background(), true, 0); !errors.Is(err, ErrDropOverflow) { + t.Fatalf("expected ErrDropOverflow, got %v", err) + } +} + +func TestRateLimiterChatLocksAreScopedPerChat(t *testing.T) { + rl := NewRateLimiter() + rl.SetChatLock(42, 1) + + if rl.Allow(42) { + t.Fatal("expected locked chat to be rejected") + } + if !rl.Allow(7) { + t.Fatal("expected unrelated chat to remain allowed") + } +} + +func TestRateLimiterGlobalWaitRespectsContextCancellation(t *testing.T) { + rl := NewRateLimiter() + rl.SetGlobalLock(1) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + + if err := rl.GlobalWait(ctx); !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected DeadlineExceeded, got %v", err) + } +}