Skip to content

Commit aa81f7a

Browse files
committed
Fix 5 compatibility issues in Telegram Phase 4
Issues found and fixed during compatibility review: CRITICAL: 1. Handler.Approver data race — goroutines racing on shared pointer → Replaced single *TelegramApprover with sync.Map keyed by chatID → Added SetApprover/GetApprover/DeleteApprover methods on Handler → handleCallback now looks up approver by callback's chat ID 2. /new command NPE — nil Approver dereference when no message processed yet → Now uses handler.GetApprover(chatID) which returns nil gracefully MEDIUM: 3. Same-chat message racing — two rapid messages corrupt session history → Added per-chat sync.Mutex serialization via chatMu sync.Map → handleChatMessage locks per-chat before processing MINOR: 4. Dead os import + dead code in approver.go — removed CONFIRMED OK: 5. Project-level config — LoadConfig always loads ./odek.json, confirmed
1 parent aaa68a9 commit aa81f7a

3 files changed

Lines changed: 56 additions & 20 deletions

File tree

cmd/odek/telegram.go

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"os"
77
"os/signal"
88
"strings"
9+
"sync"
910
"syscall"
1011
"time"
1112

@@ -17,6 +18,17 @@ import (
1718
"github.com/BackendStack21/kode/internal/telegram"
1819
)
1920

21+
// chatMu serializes agent processing per chat to prevent same-chat message
22+
// racing. Each chat gets its own mutex; messages from the same chat are
23+
// processed sequentially, preserving session history integrity.
24+
var chatMu sync.Map // map[int64]*sync.Mutex
25+
26+
// getChatMutex returns the per-chat mutex for the given chat ID.
27+
func getChatMutex(chatID int64) *sync.Mutex {
28+
v, _ := chatMu.LoadOrStore(chatID, &sync.Mutex{})
29+
return v.(*sync.Mutex)
30+
}
31+
2032
// telegramCmd is the entry point for "odek telegram".
2133
func telegramCmd(args []string) error {
2234
// 1. Load config from all sources (file → env).
@@ -85,8 +97,8 @@ func telegramCmd(args []string) error {
8597
// Handle /new — clear session and reset trust in the approver.
8698
if cmdName == "new" {
8799
sessionManager.Delete(chatID)
88-
if handler.Approver != nil {
89-
handler.Approver.ResetTrust()
100+
if a := handler.GetApprover(chatID); a != nil {
101+
a.ResetTrust()
90102
}
91103
}
92104

@@ -164,9 +176,16 @@ func handleChatMessage(
164176
resolved config.ResolvedConfig,
165177
systemMessage string,
166178
) {
179+
// Serialize per chat: only one agent loop runs per chat at a time.
180+
// Prevents same-chat message racing that would corrupt session history.
181+
mu := getChatMutex(chatID)
182+
mu.Lock()
183+
defer mu.Unlock()
184+
167185
// Create a per-chat TelegramApprover for inline keyboard approval.
168186
approver := telegram.NewTelegramApprover(bot, chatID)
169-
handler.Approver = approver
187+
handler.SetApprover(chatID, approver)
188+
defer handler.DeleteApprover(chatID)
170189

171190
// Get or create the session for this chat.
172191
cs, err := sessionManager.GetOrCreate(chatID)

internal/telegram/approver.go

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"crypto/rand"
55
"encoding/hex"
66
"fmt"
7-
"os"
87
"strings"
98
"sync"
109
"time"
@@ -218,12 +217,3 @@ func (a *TelegramApprover) ResetTrust() {
218217
a.trusted = make(map[danger.RiskClass]bool)
219218
a.mu.Unlock()
220219
}
221-
222-
// ── File descriptor check ──────────────────────────────────────────────────
223-
224-
// ensureApproverHasFD is a no-op on Telegram (no TTY needed).
225-
// Kept for interface compatibility — never called directly.
226-
var _ = func() string {
227-
// Satisfy the os import
228-
return os.DevNull
229-
}

internal/telegram/handler.go

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"os"
66
"strings"
7+
"sync"
78
)
89

910
// ─── Config ────────────────────────────────────────────────────────────────
@@ -24,11 +25,10 @@ type Handler struct {
2425
Bot *Bot
2526
Config HandlerConfig
2627

27-
// Approver handles inline keyboard approval requests for dangerous
28-
// operations. When non-nil, callback queries with approval prefixes
29-
// (apr:/den:/trs:) are routed to Approver.HandleCallback before
30-
// reaching OnCallbackQuery.
31-
Approver *TelegramApprover
28+
// approvers maps chatID → *TelegramApprover for inline keyboard approval
29+
// requests. Protected by sync.Map for concurrent read/write from the
30+
// update loop (reads) and agent goroutines (writes).
31+
approvers sync.Map
3232

3333
// OnTextMessage is called when a plain text message is received.
3434
// Returns the response text (may be empty).
@@ -61,6 +61,29 @@ type Handler struct {
6161
OnError func(chatID int64, err error)
6262
}
6363

64+
// SetApprover stores a TelegramApprover for the given chat ID.
65+
// Thread-safe: safe to call from any goroutine.
66+
func (h *Handler) SetApprover(chatID int64, a *TelegramApprover) {
67+
h.approvers.Store(chatID, a)
68+
}
69+
70+
// GetApprover retrieves the TelegramApprover for the given chat ID.
71+
// Returns nil if no approver is registered. Thread-safe.
72+
func (h *Handler) GetApprover(chatID int64) *TelegramApprover {
73+
v, ok := h.approvers.Load(chatID)
74+
if !ok {
75+
return nil
76+
}
77+
a, _ := v.(*TelegramApprover)
78+
return a
79+
}
80+
81+
// DeleteApprover removes the TelegramApprover for the given chat ID.
82+
// Thread-safe. Used when a session is reset or ends.
83+
func (h *Handler) DeleteApprover(chatID int64) {
84+
h.approvers.Delete(chatID)
85+
}
86+
6487
// NewHandler creates a Handler with the given bot and default settings.
6588
func NewHandler(bot *Bot) *Handler {
6689
return &Handler{
@@ -233,8 +256,12 @@ func (h *Handler) handleCommand(msg *Message) {
233256

234257
// handleCallback processes a callback query from an inline keyboard.
235258
func (h *Handler) handleCallback(cq *CallbackQuery) {
236-
// Route approval callbacks to the TelegramApprover first.
237-
if h.Approver != nil && h.Approver.HandleCallback(cq.Data) {
259+
if cq.Message == nil || cq.Message.Chat == nil {
260+
return
261+
}
262+
263+
// Route approval callbacks to the per-chat TelegramApprover.
264+
if a := h.GetApprover(cq.Message.Chat.ID); a != nil && a.HandleCallback(cq.Data) {
238265
// Answer the callback (remove loading state on button).
239266
if err := h.Bot.AnswerCallbackQuery(cq.ID, "", false); err != nil {
240267
if h.OnError != nil {

0 commit comments

Comments
 (0)