From da322ec694a85ca669451d077126f1eae57329cd Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Sun, 27 Sep 2026 18:04:46 +0200 Subject: [PATCH 1/2] fix: eight validated bug-hunt fixes across approvers, serve, resource --- cmd/odek/serve_runs.go | 22 ++++++-- cmd/odek/wsapprover.go | 9 ++- cmd/odek/wsapprover_trustall_test.go | 61 +++++++++++++++++++++ internal/fsatomic/fsatomic.go | 15 +++-- internal/resource/resource.go | 2 +- internal/telegram/approver.go | 41 ++++++++++---- internal/telegram/approver_deadlock_test.go | 41 ++++++++++++++ 7 files changed, 167 insertions(+), 24 deletions(-) create mode 100644 cmd/odek/wsapprover_trustall_test.go create mode 100644 internal/telegram/approver_deadlock_test.go diff --git a/cmd/odek/serve_runs.go b/cmd/odek/serve_runs.go index f351d6c3..1c5237ca 100644 --- a/cmd/odek/serve_runs.go +++ b/cmd/odek/serve_runs.go @@ -35,6 +35,7 @@ import ( "fmt" "net/http" "sort" + "strconv" "strings" "sync" "sync/atomic" @@ -116,9 +117,17 @@ func handleEvents() http.HandlerFunc { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) return } - limit := parseIntDefault(r.URL.Query().Get("limit"), 100) - if limit < 1 { - limit = 100 + // snapshot() treats limit<=0 as "return everything"; only absent or + // unparseable params get the default. An explicit limit=0 must reach + // snapshot so clients can request the untruncated feed. + limit := 100 + if q := r.URL.Query().Get("limit"); q != "" { + if v, err := strconv.Atoi(q); err == nil { + limit = v + } + } + if limit < 0 { + limit = 0 } if limit > serveEventsCap { limit = serveEventsCap @@ -459,11 +468,14 @@ func (r *serveRun) record(v any) error { r.Status = "running" } case "token", "token_delta": - if c, _ := m["content"].(string); c != "" { + // A still-draining loop must not mutate a terminal run's result — + // the snapshot served by GET /api/runs/{id} would change between + // polls after the run already completed/failed. + if c, _ := m["content"].(string); c != "" && !runStatusTerminal(r.Status) { r.Result += c } case "error": - if msg, _ := m["message"].(string); msg != "" { + if msg, _ := m["message"].(string); msg != "" && !runStatusTerminal(r.Status) { r.Error = msg } case "done": diff --git a/cmd/odek/wsapprover.go b/cmd/odek/wsapprover.go index f662cf93..5a06a604 100644 --- a/cmd/odek/wsapprover.go +++ b/cmd/odek/wsapprover.go @@ -182,7 +182,10 @@ func (a *wsApprover) PromptCommand(cls danger.RiskClass, cmd, description string // the map write below (or SetTrustAll) is a data race that can fatally // crash serve with "concurrent map read and map write". a.mu.Lock() - trusted := a.approveAll[cls] || a.trustAll + // Trust-all honors the same class gate as the per-class trust shortcut: + // destructive/blocked/unknown prompts always need a real user decision, + // even after a blanket trust grant from an earlier benign batch. + trusted := a.approveAll[cls] || (a.trustAll && danger.TrustShortcutAllowed(cls)) a.mu.Unlock() if trusted { return nil @@ -275,6 +278,10 @@ func (a *wsApprover) PromptCommand(cls danger.RiskClass, cmd, description string a.mu.Lock() a.approveAll[cls] = true a.mu.Unlock() + // Trust grants count toward approval friction, same as the TTY + // path — otherwise reflexive trust-tapping never trips the + // fatigue guard on the WebUI. + a.recordApproval(cls) return nil default: return fmt.Errorf("operation denied by user: %s", cmd) diff --git a/cmd/odek/wsapprover_trustall_test.go b/cmd/odek/wsapprover_trustall_test.go new file mode 100644 index 00000000..215b5315 --- /dev/null +++ b/cmd/odek/wsapprover_trustall_test.go @@ -0,0 +1,61 @@ +package main + +import ( + "testing" + "time" + + "github.com/BackendStack21/odek/internal/danger" +) + +// trustAll must not auto-approve classes that never allow trust shortcuts +// (destructive, blocked, unknown, ...). A benign batch approval that grants +// SetTrustAll must still surface a per-call prompt for a destructive command. +func TestWSApprover_TrustAllGatedByTrustShortcutAllowed(t *testing.T) { + a := newWSApprover(func(v any) error { return nil }) + a.SetTrustAll(true) + + prompted := make(chan struct{}, 1) + go func() { + // Wait for the prompt to be registered, then answer it. + deadline := time.After(3 * time.Second) + for { + a.mu.Lock() + var id string + for k := range a.pending { + id = k + } + a.mu.Unlock() + if id != "" { + a.HandleResponse(id, "approve") + close(prompted) + return + } + select { + case <-deadline: + return + default: + time.Sleep(5 * time.Millisecond) + } + } + }() + + err := a.PromptCommand(danger.Destructive, "rm -rf /tmp/x", "test") + if err != nil { + t.Fatalf("PromptCommand returned error: %v", err) + } + + select { + case <-prompted: + // Good: a real prompt was shown and answered. + default: + t.Fatal("destructive command was auto-approved by trustAll without prompting") + } + + // And the destructive class must not have been silently trusted. + a.mu.Lock() + _, trusted := a.approveAll[danger.Destructive] + a.mu.Unlock() + if trusted { + t.Error("destructive class must never be cached as approved") + } +} diff --git a/internal/fsatomic/fsatomic.go b/internal/fsatomic/fsatomic.go index 78f5ece6..238b6c4c 100644 --- a/internal/fsatomic/fsatomic.go +++ b/internal/fsatomic/fsatomic.go @@ -66,11 +66,16 @@ func WriteFile(path string, data []byte, perm os.FileMode) (err error) { tmp = "" // renamed — no longer ours to remove // Make the rename itself durable by fsyncing the parent directory. - if d, derr := os.Open(dir); derr == nil { - defer d.Close() - if err := d.Sync(); err != nil { - return fmt.Errorf("fsatomic: fsync dir: %w", err) - } + // A failure to open the directory means durability cannot be + // guaranteed for the rename — report it instead of silently + // returning success. + d, derr := os.Open(dir) + if derr != nil { + return fmt.Errorf("fsatomic: open dir: %w", derr) + } + defer d.Close() + if err := d.Sync(); err != nil { + return fmt.Errorf("fsatomic: fsync dir: %w", err) } return nil } diff --git a/internal/resource/resource.go b/internal/resource/resource.go index 34200794..c992d7da 100644 --- a/internal/resource/resource.go +++ b/internal/resource/resource.go @@ -29,7 +29,7 @@ func escapeGlob(s string) string { var b strings.Builder for _, r := range s { switch r { - case '*', '?', '[', ']', '^', '{', '}': + case '\\', '*', '?', '[', ']', '^', '{', '}': b.WriteByte('\\') } b.WriteRune(r) diff --git a/internal/telegram/approver.go b/internal/telegram/approver.go index 542b7754..7e5d75b0 100644 --- a/internal/telegram/approver.go +++ b/internal/telegram/approver.go @@ -169,9 +169,11 @@ func allowTrustForClass(cls danger.RiskClass) bool { } func (a *TelegramApprover) PromptCommand(cls danger.RiskClass, cmd, description string) error { - // Check session trust cache + // Check session trust cache. Trust-all is gated by the same class rule + // as the per-class trust shortcut: destructive/blocked/unknown prompts + // always require an explicit user decision. a.mu.Lock() - if a.trusted[cls] || a.trustAll { + if a.trusted[cls] || (a.trustAll && danger.TrustShortcutAllowed(cls)) { a.mu.Unlock() return nil } @@ -222,20 +224,27 @@ func (a *TelegramApprover) PromptCommand(cls danger.RiskClass, cmd, description } markup := InlineKeyboardMarkup{InlineKeyboard: keyboard} - msg, err := a.bot.SendMessage(a.ChatID, text, &SendOpts{ + // Register the pending request BEFORE sending so a fast tap racing the + // SendMessage round-trip is not silently dropped (the callback used to + // find no pending entry yet, acknowledge with a toast, and the approval + // then timed out despite the user having acted). + pr := &pendingRequest{resp: make(chan string, 1), userID: a.userID, class: cls, allowTrust: allowTrust} + a.mu.Lock() + a.pending[id] = pr + a.mu.Unlock() + + if msg, err := a.bot.SendMessage(a.ChatID, text, &SendOpts{ ParseMode: ParseModeMarkdownV2, ReplyMarkup: &markup, - }) - if err != nil { + }); err != nil { + a.mu.Lock() + delete(a.pending, id) + a.mu.Unlock() return fmt.Errorf("telegram approver: send prompt: %w", err) + } else { + pr.messageID = msg.ID } - // Register the pending request with message ID and originating user. - pr := &pendingRequest{resp: make(chan string, 1), messageID: msg.ID, userID: a.userID, class: cls, allowTrust: allowTrust} - a.mu.Lock() - a.pending[id] = pr - a.mu.Unlock() - defer func() { a.mu.Lock() delete(a.pending, id) @@ -345,7 +354,15 @@ func (a *TelegramApprover) HandleCallback(data string, userID int64) bool { if pr.userID != 0 && pr.userID != userID { return true } - pr.resp <- action + // Non-blocking send: the buffered channel has a single-shot reader. + // A duplicate callback (double-tap, Telegram retry) arriving after + // the reader consumed the action must not block HandleCallback — + // it runs on the serialized update loop, so a blocking send would + // freeze the whole bot for every chat. + select { + case pr.resp <- action: + default: + } } return true diff --git a/internal/telegram/approver_deadlock_test.go b/internal/telegram/approver_deadlock_test.go new file mode 100644 index 00000000..4d73e454 --- /dev/null +++ b/internal/telegram/approver_deadlock_test.go @@ -0,0 +1,41 @@ +package telegram + +import ( + "testing" + "time" +) + +// A duplicate approval callback (double-tap, Telegram retry) must never +// block HandleCallback: the reader of pr.resp consumes at most one action, +// so a second send on the buffered channel used to block the serialized +// update loop forever, freezing the whole bot. +func TestHandleCallback_DuplicateDoesNotBlock(t *testing.T) { + ts := testServer(t, nil) + defer ts.Close() + bot := testBot(t, ts) + + a := NewTelegramApprover(bot, 1, 0) + id := a.newID() + pr := &pendingRequest{resp: make(chan string, 1)} + a.mu.Lock() + a.pending[id] = pr + a.mu.Unlock() + + done := make(chan struct{}) + go func() { + a.HandleCallback(cbPrefixApprove+id, 0) // first — fills the buffer + a.HandleCallback(cbPrefixApprove+id, 0) // duplicate — must not block + close(done) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("HandleCallback blocked on duplicate callback (deadlock)") + } + + action := <-pr.resp + if action != "approve" { + t.Errorf("action = %q, want %q", action, "approve") + } +} From 2113061733588075e7381369021eacebf5c18f06 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Sun, 27 Sep 2026 18:09:43 +0200 Subject: [PATCH 2/2] fix: pin events limit contract with test; document limit=0 semantics --- cmd/odek/serve_runs_events_limit_test.go | 49 ++++++++++++++++++++++++ docs/WEBUI.md | 7 +++- 2 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 cmd/odek/serve_runs_events_limit_test.go diff --git a/cmd/odek/serve_runs_events_limit_test.go b/cmd/odek/serve_runs_events_limit_test.go new file mode 100644 index 00000000..069dede1 --- /dev/null +++ b/cmd/odek/serve_runs_events_limit_test.go @@ -0,0 +1,49 @@ +package main + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/BackendStack21/odek/internal/events" +) + +// An explicit limit=0 must reach the snapshot untruncated (documented +// "return everything"), and negatives are treated as 0 — previously both +// were silently rewritten to the default 100. +func TestHandleEvents_ExplicitZeroLimitReturnsAll(t *testing.T) { + serveEvents.reset() + t.Cleanup(serveEvents.reset) + now := time.Now().UTC() + for i := 0; i < 3; i++ { + serveEvents.add(events.Event{Type: "e", RunID: fmt.Sprintf("r%d", i), Timestamp: now}) + } + + decode := func(req *http.Request) (count int) { + w := httptest.NewRecorder() + handleEvents()(w, req) + var body struct { + Count int `json:"count"` + } + if err := json.NewDecoder(w.Body).Decode(&body); err != nil { + t.Fatal(err) + } + return body.Count + } + + if got := decode(httptest.NewRequest(http.MethodGet, "/api/events?limit=0", nil)); got != 3 { + t.Errorf("limit=0 returned %d events, want all 3", got) + } + if got := decode(httptest.NewRequest(http.MethodGet, "/api/events?limit=-5", nil)); got != 3 { + t.Errorf("limit=-5 returned %d events, want all 3 (treated as 0)", got) + } + if got := decode(httptest.NewRequest(http.MethodGet, "/api/events?limit=2", nil)); got != 2 { + t.Errorf("limit=2 returned %d events, want 2", got) + } + if got := decode(httptest.NewRequest(http.MethodGet, "/api/events", nil)); got != 3 { + t.Errorf("absent limit returned %d events, want default (all 3 fit)", got) + } +} diff --git a/docs/WEBUI.md b/docs/WEBUI.md index 39700bc1..9e945d25 100644 --- a/docs/WEBUI.md +++ b/docs/WEBUI.md @@ -527,8 +527,11 @@ classifications — never prompt or completion content. ### `GET /api/events?limit=&run_id=&session_id=` Recent `odek.event/v1` runtime events (ring of 500, oldest-first, filtered). -Event payloads carry SHA-256 arg hashes and redacted fields only — never raw -tool arguments. Every WS prompt and REST run feeds the same ring. +`limit` defaults to 100; an explicit `limit=0` returns the full filtered +ring (negative values are treated as 0); values above the ring size clamp +to the ring. Event payloads carry SHA-256 arg hashes and redacted fields +only — never raw tool arguments. Every WS prompt and REST run feeds the +same ring. ### `GET /api/usage`