Skip to content

Commit 9b052a3

Browse files
committed
fix: acknowledge approval/clarify/cancel responses in Telegram and WebUI
- Clarify: echo user's choice ('Yes'/'No') instead of generic 'Got it, thanks!' - Approval: AnswerCallbackQuery now shows toast (Approved/Denied/Trusted) - WebUI: send approval_ack event after user responds for UI feedback - Cancel: approvers now listen for Cancel() to interrupt blocking PromptCommand (fixes 120s hang when /stop fires during approval) - Cancel path: send cancellation summary instead of silent return (partial session saved + stop summary + recovery hint) - WebSocket disconnect: approver.Cancel() called to release pending approvals - New: TelegramApprover.Cancel(), wsApprover.Cancel() (idempotent) - New: approvalToast() helper in handler.go - Tests: Cancel interrupts PromptCommand, idempotent, ack not sent on cancel
1 parent 1fca557 commit 9b052a3

7 files changed

Lines changed: 161 additions & 8 deletions

File tree

cmd/odek/serve.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,9 @@ func handleWS(store *session.Store, resources *resource.Registry, resolved confi
303303
return
304304
}
305305
defer agent.Close()
306+
if approver != nil {
307+
defer approver.Cancel() // release any pending approval on disconnect
308+
}
306309
if sandboxCleanup != nil {
307310
defer sandboxCleanup()
308311
}

cmd/odek/telegram.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,7 @@ func telegramCmd(args []string) error {
392392
// Channel full or closed — clarify already resolved.
393393
}
394394
}
395-
return "✅ Got it, thanks!", nil
395+
return "✅ You chose **" + answer + "**", nil
396396
}
397397

398398
// Route skill suggestion callbacks — Save or Skip.
@@ -1228,15 +1228,23 @@ func handleChatMessage(
12281228
traceMsgID = 0
12291229
}
12301230

1231-
// If the context was cancelled (by /stop or restart), don't send
1232-
// a redundant error message — the canceller already notified the
1233-
// user with a summary or restart notification.
1231+
// If the context was cancelled (by /stop or restart), save partial
1232+
// state and notify the user with a cancellation summary.
12341233
if errors.Is(err, context.Canceled) {
12351234
// Save partial session state so the conversation isn't lost.
12361235
cs.LastActive = time.Now()
12371236
if saveErr := sessionManager.Save(chatID, cs.Messages); saveErr != nil {
12381237
log.Error("save session after cancel", "chat_id", chatID, "error", saveErr)
12391238
}
1239+
// Send a cancellation summary so the user knows what happened.
1240+
cancelMsg := "⏹️ *Task cancelled.*"
1241+
if infoVal, ok := chatRunInfos.LoadAndDelete(chatID); ok {
1242+
info := infoVal.(loop.IterationInfo)
1243+
cancelMsg += "\n\n" + formatStopSummary(info)
1244+
}
1245+
cancelMsg += "\n\n_Session state saved. Send a new message to continue._"
1246+
sendAsync(bot, chatID, cancelMsg,
1247+
&telegram.SendOpts{ParseMode: telegram.ParseModeMarkdownV2, ReplyToMessageID: messageID})
12401248
return
12411249
}
12421250

cmd/odek/wsapprover.go

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ type wsApprover struct {
3737
pending map[string]chan string // request ID → response channel
3838
mu sync.Mutex
3939
approveAll map[danger.RiskClass]bool // trust-cached risk classes
40+
cancel chan struct{} // closed by Cancel() to interrupt waiting PromptCommand
4041
}
4142

4243
// newWSApprover creates a wsApprover that sends requests via sendFn.
@@ -45,6 +46,7 @@ func newWSApprover(sendFn func(v any) error) *wsApprover {
4546
sendFn: sendFn,
4647
pending: make(map[string]chan string),
4748
approveAll: make(map[danger.RiskClass]bool),
49+
cancel: make(chan struct{}),
4850
}
4951
}
5052

@@ -79,9 +81,15 @@ func (a *wsApprover) PromptCommand(cls danger.RiskClass, cmd, description string
7981
return fmt.Errorf("approval: send failed: %w", err)
8082
}
8183

82-
// Wait for response (60s timeout prevents agent deadlock)
84+
// Wait for response, cancellation, or timeout (60s).
8385
select {
8486
case action := <-resp:
87+
// Ack the user's choice back to the browser for UI feedback.
88+
a.sendFn(map[string]any{
89+
"type": "approval_ack",
90+
"id": id,
91+
"action": action,
92+
})
8593
switch action {
8694
case "approve":
8795
return nil
@@ -91,6 +99,8 @@ func (a *wsApprover) PromptCommand(cls danger.RiskClass, cmd, description string
9199
default:
92100
return fmt.Errorf("operation denied by user: %s", cmd)
93101
}
102+
case <-a.cancel:
103+
return fmt.Errorf("approval cancelled: %s", cmd)
94104
case <-time.After(60 * time.Second):
95105
return fmt.Errorf("approval timeout: %s", cmd)
96106
}
@@ -118,3 +128,14 @@ func (a *wsApprover) newID() string {
118128
rand.Read(b)
119129
return "apr-" + hex.EncodeToString(b)
120130
}
131+
132+
// Cancel interrupts any pending PromptCommand by closing the cancel channel.
133+
// Safe to call multiple times — subsequent calls are no-ops.
134+
func (a *wsApprover) Cancel() {
135+
select {
136+
case <-a.cancel:
137+
// Already closed.
138+
default:
139+
close(a.cancel)
140+
}
141+
}

cmd/odek/wsapprover_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"errors"
55
"strings"
66
"testing"
7+
"time"
78

89
"github.com/BackendStack21/kode/internal/danger"
910
)
@@ -267,3 +268,47 @@ func TestWSApprover_PromptOperation_SendError(t *testing.T) {
267268
t.Fatal("expected error for send failure in operation")
268269
}
269270
}
271+
272+
// ── Test Cancel ────────────────────────────────────────────────────────
273+
274+
func TestWSApprover_Cancel_InterruptsPrompt(t *testing.T) {
275+
var acked bool
276+
sendFn := func(v any) error {
277+
if m, ok := v.(map[string]any); ok && m["type"] == "approval_ack" {
278+
acked = true
279+
}
280+
return nil
281+
}
282+
a := newWSApprover(sendFn)
283+
284+
done := make(chan error, 1)
285+
go func() {
286+
done <- a.PromptCommand(danger.Safe, "test", "")
287+
}()
288+
289+
// Cancel immediately.
290+
a.Cancel()
291+
292+
select {
293+
case err := <-done:
294+
if err == nil {
295+
t.Error("expected error from cancelled PromptCommand")
296+
}
297+
if !strings.Contains(err.Error(), "cancelled") {
298+
t.Errorf("expected 'cancelled' in error, got: %v", err)
299+
}
300+
case <-time.After(3 * time.Second):
301+
t.Fatal("PromptCommand did not return after Cancel() within 3s")
302+
}
303+
304+
// Ack event should NOT be sent on cancel — the user didn't respond.
305+
if acked {
306+
t.Error("approval_ack should not be sent on cancel")
307+
}
308+
}
309+
310+
func TestWSApprover_Cancel_Idempotent(t *testing.T) {
311+
a := newWSApprover(func(v any) error { return nil })
312+
a.Cancel()
313+
a.Cancel() // second call should not panic
314+
}

internal/telegram/approver.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ type TelegramApprover struct {
4545
mu sync.Mutex
4646
trusted map[danger.RiskClass]bool
4747
log Logger
48+
cancel chan struct{} // closed by Cancel() to interrupt waiting PromptCommand
4849

4950
// ChatID is the Telegram chat where approval prompts are sent.
5051
ChatID int64
@@ -58,6 +59,7 @@ func NewTelegramApprover(bot *Bot, chatID int64) *TelegramApprover {
5859
pending: make(map[string]chan string),
5960
trusted: make(map[danger.RiskClass]bool),
6061
log: NewNopLogger(),
62+
cancel: make(chan struct{}),
6163
}
6264
}
6365

@@ -70,6 +72,17 @@ func (a *TelegramApprover) SetLogger(l Logger) {
7072
a.log = l
7173
}
7274

75+
// Cancel interrupts any pending PromptCommand by closing the cancel channel.
76+
// Safe to call multiple times — subsequent calls are no-ops.
77+
func (a *TelegramApprover) Cancel() {
78+
select {
79+
case <-a.cancel:
80+
// Already closed.
81+
default:
82+
close(a.cancel)
83+
}
84+
}
85+
7386
// PromptCommand sends an approval request with inline keyboard and waits
7487
// for the user to respond. Returns nil on approve/trust, error on deny/timeout.
7588
func (a *TelegramApprover) PromptCommand(cls danger.RiskClass, cmd, description string) error {
@@ -135,7 +148,7 @@ func (a *TelegramApprover) PromptCommand(cls danger.RiskClass, cmd, description
135148
a.mu.Unlock()
136149
}()
137150

138-
// Wait for response or timeout.
151+
// Wait for response, cancellation, or timeout.
139152
select {
140153
case action := <-resp:
141154
switch action {
@@ -157,6 +170,8 @@ func (a *TelegramApprover) PromptCommand(cls danger.RiskClass, cmd, description
157170
default:
158171
return fmt.Errorf("operation denied: %s", cmd)
159172
}
173+
case <-a.cancel:
174+
return fmt.Errorf("approval cancelled: %s", cmd)
160175
case <-time.After(approvalTimeout):
161176
return fmt.Errorf("approval timeout: %s", cmd)
162177
}

internal/telegram/approver_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ import (
44
"fmt"
55
"net/http"
66
"net/http/httptest"
7+
"strings"
78
"testing"
9+
"time"
810

911
"github.com/BackendStack21/kode/internal/danger"
1012
)
@@ -306,3 +308,46 @@ func TestApprover_ConcurrentAccess(t *testing.T) {
306308
t.Error("IsTrusted should be true after concurrent sets")
307309
}
308310
}
311+
312+
// ── Test Cancel ────────────────────────────────────────────────────────
313+
314+
func TestTelegramApprover_Cancel_InterruptsPrompt(t *testing.T) {
315+
// Cancel() should cause a blocked PromptCommand to return immediately
316+
// with a cancellation error, not hang for the full timeout.
317+
ts := testServer(t, nil)
318+
defer ts.Close()
319+
bot := testBot(t, ts)
320+
321+
a := NewTelegramApprover(bot, 1)
322+
323+
done := make(chan error, 1)
324+
go func() {
325+
done <- a.PromptCommand(danger.SystemWrite, "rm -rf /tmp/test", "test cancel")
326+
}()
327+
328+
// Cancel immediately.
329+
a.Cancel()
330+
331+
select {
332+
case err := <-done:
333+
if err == nil {
334+
t.Error("expected error from cancelled PromptCommand, got nil")
335+
}
336+
if !strings.Contains(err.Error(), "cancelled") {
337+
t.Errorf("expected 'cancelled' in error, got: %v", err)
338+
}
339+
case <-time.After(3 * time.Second):
340+
t.Fatal("PromptCommand did not return after Cancel() within 3s")
341+
}
342+
}
343+
344+
func TestTelegramApprover_Cancel_Idempotent(t *testing.T) {
345+
ts := testServer(t, nil)
346+
defer ts.Close()
347+
bot := testBot(t, ts)
348+
349+
a := NewTelegramApprover(bot, 1)
350+
a.Cancel()
351+
a.Cancel() // second call should not panic
352+
// If we get here without panic, it's idempotent.
353+
}

internal/telegram/handler.go

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -331,8 +331,9 @@ func (h *Handler) handleCallback(cq *CallbackQuery) {
331331

332332
// Route approval callbacks to the per-chat TelegramApprover.
333333
if a := h.GetApprover(cq.Message.Chat.ID); a != nil && a.HandleCallback(cq.Data) {
334-
// Answer the callback (remove loading state on button).
335-
if err := h.Bot.AnswerCallbackQuery(cq.ID, "", false); err != nil {
334+
// Show a toast acknowledging the user's choice.
335+
ack := approvalToast(cq.Data)
336+
if err := h.Bot.AnswerCallbackQuery(cq.ID, ack, false); err != nil {
336337
h.log.Error("answer callback query (approval) failed", "chat_id", cq.Message.Chat.ID, "error", err)
337338
if h.OnError != nil {
338339
h.OnError(cq.Message.Chat.ID, err)
@@ -577,3 +578,18 @@ func extractCommand(text string) (cmd string, args string) {
577578
return cmdPart, args
578579
}
579580

581+
582+
// approvalToast returns a toast message for an approval callback action.
583+
// Parses the callback data prefix to determine the user's choice.
584+
func approvalToast(data string) string {
585+
switch {
586+
case strings.HasPrefix(data, cbPrefixApprove):
587+
return "✅ Approved"
588+
case strings.HasPrefix(data, cbPrefixDeny):
589+
return "❌ Denied"
590+
case strings.HasPrefix(data, cbPrefixTrust):
591+
return "🔒 Trusted for this session"
592+
default:
593+
return ""
594+
}
595+
}

0 commit comments

Comments
 (0)