fix(loop): deliver recovered tool-panic message to the LLM + review cleanups (#26)

* fix(loop): deliver recovered tool-panic message to the LLM + review cleanups

A panicking tool's recovered error message was discarded: the goroutine
returned before results[idx] was assigned, so the LLM received an empty
tool result and the consecutive-error tracker never counted the failure.
Drop the early return so the panic message flows through like any other
tool error. The existing TestToolPanic_DoesNotKillAgent never exercised
this path (its message-count trigger never matched with no system
prompt); switch it to a call counter and assert the panic message
reaches the LLM.

Review cleanups from the #25 follow-up:
- loop: name the thrice-repeated anonymous interface as trustAllSetter
- telegram: parse ODEK_TELEGRAM_ALLOW_ALL with strconv.ParseBool for a
  consistent truthy grammar (malformed values fail closed)
- telegram: add TelegramConfig.HasAllowlist() and use it in both
  ValidateConfig and the open-bot startup warning
- telegram: correct HandlerConfig allowlist comments (AllowAllUsers is
  only consulted when BOTH lists are empty) and document on HandleUpdate
  that every routed handler must enforce isAllowed

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* test(telegram): pin strict ALLOW_ALL grammar and HasAllowlist behavior

Verification-protocol remediation for #26: the claim that malformed or
extended truthy values ("yes", "on", "junk") fail closed had no
corresponding test, and HasAllowlist had no direct unit test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: jkyberneees

cmd/odek/telegram.go

@@ -169,7 +169,7 @@ func telegramCmd(args []string) error {
 	// Loud warning when running without an allowlist (only reachable when the
 	// operator explicitly set ODEK_TELEGRAM_ALLOW_ALL; ValidateConfig blocks
 	// the accidental case).
-	if cfg.AllowAllUsers && len(cfg.AllowedChats) == 0 && len(cfg.AllowedUsers) == 0 {
+	if cfg.AllowAllUsers && !cfg.HasAllowlist() {
 		handlerLog.Warn("telegram bot is running with NO allowlist — ANY user can drive the agent (ODEK_TELEGRAM_ALLOW_ALL=true)")
 	}
 

internal/loop/loop.go

@@ -520,6 +520,10 @@ func (e *Engine) RunWithMessages(ctx context.Context, messages []llm.Message) (s
 	return e.runLoop(ctx, messages)
 }
 
+// trustAllSetter is implemented by approvers (wsApprover, TelegramApprover)
+// whose tool-level prompts auto-pass while a batch approval grant is active.
+type trustAllSetter interface{ SetTrustAll(bool) }
+
 // runLoop is the shared core of Run and RunWithMessages.
 // It runs the ReAct loop on the given messages and returns the final
 // answer plus the complete updated message history.
@@ -534,7 +538,7 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [
 	// the same approver (wsApprover is per-connection, TelegramApprover is
 	// per-chat). The per-iteration reset below is the primary mechanism; this
 	// defer only covers abnormal exits mid-iteration.
-	if ta, ok := e.approver.(interface{ SetTrustAll(bool) }); ok {
+	if ta, ok := e.approver.(trustAllSetter); ok {
 		defer ta.SetTrustAll(false)
 	}
 
@@ -847,7 +851,7 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [
 		// (not via defer, which only fires when runLoop returns) — otherwise a
 		// single approved batch would auto-approve every dangerous tool for the
 		// remainder of the run.
-		var trustAllApprover interface{ SetTrustAll(bool) }
+		var trustAllApprover trustAllSetter
 		if e.approver != nil && len(result.ToolCalls) > 1 {
 			// Classify each tool call and filter to only those needing approval.
 			type riskyCall struct {
@@ -901,7 +905,7 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [
 				// Approved: set trustAll on the approver if supported, so
 				// individual tool-level PromptCommand calls auto-pass.
 				if !batchDenied {
-					if ta, ok := e.approver.(interface{ SetTrustAll(bool) }); ok {
+					if ta, ok := e.approver.(trustAllSetter); ok {
 						ta.SetTrustAll(true)
 						trustAllApprover = ta
 					}
@@ -939,12 +943,13 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [
 							ctxTool.SetContext(ctx)
 						}
 						// Capture any panic from the tool so it does not kill the agent.
-						var toolPanicked bool
+						// The recovered message falls through to results[idx] like any
+						// other tool error, so the LLM sees it and the consecutive-error
+						// tracking counts it.
 						func() {
 							defer func() {
 								if r := recover(); r != nil {
 									output = fmt.Sprintf("error: tool %q panicked: %v", tcRef.Function.Name, r)
-									toolPanicked = true
 								}
 							}()
 							res, err := t.Call(tcRef.Function.Arguments)
@@ -954,9 +959,6 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [
 								output = redact.RedactSecrets(res)
 							}
 						}()
-						if toolPanicked {
-							return
-						}
 					}
 					results[idx] = execResult{output: output}
 				}(i, tc)

internal/loop/loop_test.go

@@ -10,6 +10,7 @@ import (
 	"sort"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -2007,8 +2008,12 @@ func (p *panicTool) Call(args string) (string, error) {
 }
 
 // TestToolPanic_DoesNotKillAgent verifies that a panicking tool call
-// does not crash the agent. The agent should recover and continue.
+// does not crash the agent. The agent should recover and continue, and the
+// recovered panic message must reach the LLM as the tool result (regression:
+// it used to be discarded, leaving an empty tool result).
 func TestToolPanic_DoesNotKillAgent(t *testing.T) {
+	var toolResult atomic.Value // string: content of the tool message the LLM saw
+	var callNum atomic.Int32
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		var body struct {
 			Messages []llm.Message `json:"messages"`
@@ -2017,11 +2022,16 @@ func TestToolPanic_DoesNotKillAgent(t *testing.T) {
 			t.Fatal(err)
 		}
 		// First call: return tool calls with panic tool
-		if len(body.Messages) == 2 {
+		if callNum.Add(1) == 1 {
 			fmt.Fprint(w, `{"choices":[{"index":0,"message":{"role":"assistant","content":"","tool_calls":[{"id":"call_1","type":"function","function":{"name":"panic_tool","arguments":"{}"}}]},"finish_reason":"tool_calls"}]}`)
 			return
 		}
-		// Second call (after tool panic): return content
+		// Second call (after tool panic): capture the tool result, return content
+		for _, m := range body.Messages {
+			if m.Role == "tool" {
+				toolResult.Store(m.Content)
+			}
+		}
 		fmt.Fprint(w, `{"choices":[{"index":0,"message":{"role":"assistant","content":"Agent survived the panic!"},"finish_reason":"stop"}]}`)
 	}))
 	defer server.Close()
@@ -2035,6 +2045,10 @@ func TestToolPanic_DoesNotKillAgent(t *testing.T) {
 	if !strings.Contains(result, "Agent survived") {
 		t.Errorf("agent result = %q, want 'Agent survived'", result)
 	}
+	tr, _ := toolResult.Load().(string)
+	if !strings.Contains(tr, "panicked") {
+		t.Errorf("tool result sent to LLM = %q, want the recovered panic message (containing %q)", tr, "panicked")
+	}
 }
 
 // ── Context Length Error Detection ────────────────────────────────────

internal/telegram/config.go

@@ -58,7 +58,10 @@ func ConfigFromEnv(base TelegramConfig) TelegramConfig {
 		cfg.AllowedUsers = parseInt64List(v)
 	}
 	if v := os.Getenv("ODEK_TELEGRAM_ALLOW_ALL"); v != "" {
-		cfg.AllowAllUsers = parseBool(v)
+		// strconv.ParseBool keeps the truthy grammar consistent with the rest
+		// of the config surface; a malformed value fails closed (false).
+		b, err := strconv.ParseBool(strings.TrimSpace(v))
+		cfg.AllowAllUsers = err == nil && b
 	}
 	if v := os.Getenv("ODEK_TELEGRAM_BOT_USERNAME"); v != "" {
 		cfg.BotUsername = v
@@ -139,23 +142,19 @@ func ValidateConfig(cfg TelegramConfig) error {
 	// operator explicitly opts in. An empty allowlist would otherwise let ANY
 	// Telegram user drive the agent (and its shell/file tools). Checked last so
 	// field-level validation errors surface first.
-	if len(cfg.AllowedChats) == 0 && len(cfg.AllowedUsers) == 0 && !cfg.AllowAllUsers {
+	if !cfg.HasAllowlist() && !cfg.AllowAllUsers {
 		return fmt.Errorf("telegram: no allowlist configured — set ODEK_TELEGRAM_ALLOWED_CHATS " +
 			"and/or ODEK_TELEGRAM_ALLOWED_USERS to restrict access, or set " +
 			"ODEK_TELEGRAM_ALLOW_ALL=true to explicitly run an open bot (NOT recommended)")
 	}
 	return nil
 }
 
-// parseBool parses common truthy string values ("true", "1", "yes", "on",
-// case-insensitive). Anything else is false.
-func parseBool(s string) bool {
-	switch strings.ToLower(strings.TrimSpace(s)) {
-	case "true", "1", "yes", "on":
-		return true
-	default:
-		return false
-	}
+// HasAllowlist reports whether at least one allowlist (chats or users) is
+// configured. With no allowlist the bot is open to every Telegram user, which
+// requires the explicit AllowAllUsers opt-in (see ValidateConfig).
+func (c TelegramConfig) HasAllowlist() bool {
+	return len(c.AllowedChats) > 0 || len(c.AllowedUsers) > 0
 }
 
 // parseInt64List parses a comma-separated string of integers into a slice of int64.

internal/telegram/config_test.go

@@ -462,6 +462,59 @@ func TestConfigFromEnv_allowAll(t *testing.T) {
 	}
 }
 
+// TestConfigFromEnv_allowAllStrictGrammar pins the strconv.ParseBool grammar:
+// only its truthy forms enable the open-bot flag; anything else — including
+// the formerly-accepted "yes"/"on" and malformed values — fails closed.
+func TestConfigFromEnv_allowAllStrictGrammar(t *testing.T) {
+	tests := []struct {
+		value string
+		want  bool
+	}{
+		{"true", true},
+		{"TRUE", true},
+		{"1", true},
+		{" true ", true}, // surrounding whitespace is trimmed
+		{"false", false},
+		{"0", false},
+		{"yes", false}, // not part of strconv.ParseBool — fails closed
+		{"on", false},  // not part of strconv.ParseBool — fails closed
+		{"junk", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.value, func(t *testing.T) {
+			unsetAllEnvVars(t)
+			t.Setenv("ODEK_TELEGRAM_ALLOW_ALL", tt.value)
+			cfg := ConfigFromEnv(DefaultConfig())
+			if cfg.AllowAllUsers != tt.want {
+				t.Errorf("ODEK_TELEGRAM_ALLOW_ALL=%q → AllowAllUsers=%v, want %v",
+					tt.value, cfg.AllowAllUsers, tt.want)
+			}
+		})
+	}
+}
+
+func TestHasAllowlist(t *testing.T) {
+	tests := []struct {
+		name  string
+		chats []int64
+		users []int64
+		want  bool
+	}{
+		{"both empty", nil, nil, false},
+		{"chats only", []int64{1}, nil, true},
+		{"users only", nil, []int64{2}, true},
+		{"both set", []int64{1}, []int64{2}, true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cfg := TelegramConfig{AllowedChats: tt.chats, AllowedUsers: tt.users}
+			if got := cfg.HasAllowlist(); got != tt.want {
+				t.Errorf("HasAllowlist() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
 func TestValidateConfig_emptyToken(t *testing.T) {
 	cfg := DefaultConfig()
 	err := ValidateConfig(cfg)

internal/telegram/handler.go

@@ -11,10 +11,10 @@ import (
 
 // HandlerConfig controls which messages the Handler processes.
 type HandlerConfig struct {
-	AllowedChats []int64 // restricts by chat ID; empty + AllowAllUsers required to allow any
+	AllowedChats []int64 // non-empty: only these chat IDs pass; empty: no chat filter
 	BotUsername  string  // for @mention detection in groups (without @)
 	MaxMsgLength int     // default: 4096
-	AllowedUsers []int64 // restricts by user ID; empty + AllowAllUsers required to allow any
+	AllowedUsers []int64 // non-empty: only these user IDs pass; empty: no user filter
 	// AllowAllUsers must be true to permit access when BOTH allowlists are
 	// empty. Default false = fail-closed (deny everyone) so an unconfigured
 	// handler never silently allows all users. See ValidateConfig.
@@ -184,6 +184,11 @@ func defaultDocumentHandler(bot *Bot) func(int64, int, string, string) (string,
 // HandleUpdate routes an incoming Telegram update to the appropriate handler.
 // Recovers from panics in handler callbacks to prevent a single bad update
 // from crashing the entire bot loop.
+//
+// SECURITY: every handler routed here must enforce isAllowed itself before
+// acting (handleMessage and handleCallback both do). When adding a new update
+// type, gate it the same way — callback queries once bypassed authorization
+// because their handler skipped this check.
 func (h *Handler) HandleUpdate(upd Update) {
 	defer h.recoverFromPanic("HandleUpdate", upd.ID)
 	switch {