From 11a811ad9333498b35820d63446d9a7d19f40e30 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Wed, 3 Jun 2026 08:15:25 +0200 Subject: [PATCH] fix(approval): always render the full command in approval prompts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both the Telegram and terminal approval surfaces could show the command-to-approve incompletely, undermining the user's ability to make an informed decision. Telegram (internal/telegram/approver.go): - When the model supplied a description, the command was hidden entirely and only the description was shown — the user approved a command they could not see. The full command is now always rendered in a fenced code block, with the description shown as a separate "Why:" line. - The command was byte-sliced to 200 chars (UTF-8 unsafe, silent). It is now only truncated when the whole message would exceed Telegram's hard 4096-char limit, on a rune boundary, with an explicit "[truncated]" marker. - Command content inside the code fence was unescaped, so a backtick or backslash could close the fence early and corrupt rendering. Added escapeCodeBlock() to escape ` and \ for MarkdownV2 code blocks. Terminal (internal/narrate/narrate.go): - extractShell scanned for the first quote pair, so any command with an embedded quote (git commit -m "msg", python -c "code") was cut at the first inner quote. It now decodes the JSON args properly, with the old scan kept only as a malformed-input fallback. - The shell narration was byte-truncated at 50 chars; truncate() is now rune-safe and the shell budget is raised to 120. Adds tests covering full-command rendering, code-block escaping, hard-limit truncation, embedded-quote extraction, and rune-safe cuts. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/narrate/narrate.go | 29 +++++++- internal/narrate/narrate_test.go | 35 ++++++++++ internal/telegram/approver.go | 93 ++++++++++++++++++++----- internal/telegram/approver_test.go | 105 +++++++++++++++++++++++++++++ 4 files changed, 241 insertions(+), 21 deletions(-) diff --git a/internal/narrate/narrate.go b/internal/narrate/narrate.go index 452d3ca..fd9d948 100644 --- a/internal/narrate/narrate.go +++ b/internal/narrate/narrate.go @@ -4,8 +4,10 @@ package narrate import ( + "encoding/json" "fmt" "strings" + "unicode/utf8" ) // Narrator produces engaging progress messages for tool calls and thinking. @@ -32,7 +34,7 @@ func (n *Narrator) ToolCallMessage(name, args string) string { case "write_file", "patch": return fmt.Sprintf("%s Editing `%s`...", emoji, extractPath(args)) case "shell": - return fmt.Sprintf("%s Running `%s`...", emoji, truncate(extractShell(args), 50)) + return fmt.Sprintf("%s Running `%s`...", emoji, truncate(extractShell(args), 120)) case "search_files": return fmt.Sprintf("%s Searching the codebase...", emoji) case "delegate_task", "delegate_tasks": @@ -81,11 +83,20 @@ func toolEmoji(name string) string { } } +// truncate shortens s to at most n runes, appending an ellipsis when it cuts. +// It measures in runes (not bytes) so it never splits a multi-byte character. func truncate(s string, n int) string { - if len(s) <= n { + if utf8.RuneCountInString(s) <= n { return s } - return s[:n] + "..." + count := 0 + for i := range s { + if count == n { + return s[:i] + "..." + } + count++ + } + return s + "..." } func extractPath(args string) string { @@ -107,7 +118,19 @@ func extractPath(args string) string { return "file" } +// extractShell pulls the shell command out of the tool-call JSON args. It +// decodes the JSON properly rather than scanning for the first quote pair: +// commands routinely contain quotes (git commit -m "msg", python -c "code"), +// and the old substring approach stopped at the first embedded quote, showing +// a truncated command. Falls back to a best-effort scan only if the args are +// not valid JSON. func extractShell(args string) string { + var parsed struct { + Command string `json:"command"` + } + if err := json.Unmarshal([]byte(args), &parsed); err == nil && parsed.Command != "" { + return parsed.Command + } if idx := strings.Index(args, `"command"`); idx >= 0 { rest := args[idx+len(`"command"`):] if start := strings.Index(rest, `"`); start >= 0 { diff --git a/internal/narrate/narrate_test.go b/internal/narrate/narrate_test.go index 15f9c1f..3006380 100644 --- a/internal/narrate/narrate_test.go +++ b/internal/narrate/narrate_test.go @@ -3,6 +3,7 @@ package narrate import ( "strings" "testing" + "unicode/utf8" ) func TestNarrator_ToolCallMessage_Offline(t *testing.T) { @@ -94,6 +95,40 @@ func TestNarrator_ExtractShell_Malformed(t *testing.T) { } } +// A command containing quotes must be extracted in full — the old substring +// scan stopped at the first embedded quote and showed a truncated command. +func TestNarrator_ExtractShell_EmbeddedQuotes(t *testing.T) { + args := `{"command": "git commit -m \"fix: the thing\""}` + want := `git commit -m "fix: the thing"` + if s := extractShell(args); s != want { + t.Errorf("extractShell = %q, want %q", s, want) + } +} + +func TestNarrator_ExtractShell_DescriptionFieldBefore(t *testing.T) { + // "description" appears before "command"; the JSON decode must still pick + // the command rather than getting confused by the earlier field. + args := `{"description": "run the tests", "command": "go test ./..."}` + if s := extractShell(args); s != "go test ./..." { + t.Errorf("extractShell = %q, want %q", s, "go test ./...") + } +} + +// truncate measures runes, not bytes, so it never splits a multi-byte char. +func TestNarrator_Truncate_RuneSafe(t *testing.T) { + s := truncate("héllo wörld", 5) // é and ö are multi-byte + if s != "héllo..." { + t.Errorf("truncate = %q, want %q", s, "héllo...") + } + for i := 0; i < len(s); { + r, size := utf8.DecodeRuneInString(s[i:]) + if r == utf8.RuneError && size == 1 { + t.Fatalf("truncate produced invalid UTF-8 at byte %d: %q", i, s) + } + i += size + } +} + func TestNarrator_ExtractPath_Found(t *testing.T) { if s := extractPath(`{"path": "/root/projects/main.go"}`); s != "main.go" { t.Errorf("expected basename, got: %q", s) diff --git a/internal/telegram/approver.go b/internal/telegram/approver.go index b388fab..4eecf36 100644 --- a/internal/telegram/approver.go +++ b/internal/telegram/approver.go @@ -7,6 +7,7 @@ import ( "strings" "sync" "time" + "unicode/utf8" "github.com/BackendStack21/odek/internal/danger" ) @@ -105,24 +106,9 @@ func (a *TelegramApprover) PromptCommand(cls danger.RiskClass, cmd, description id := a.newID() - // Build description text. - var desc string - if description != "" { - desc = description - } else { - desc = cmd - if len(desc) > 200 { - desc = desc[:200] + "..." - } - } - - // Build the approval message. - text := fmt.Sprintf( - "⚠️ *Approval Required*\n\n"+ - "Risk: `%s`\n"+ - "```\n%s\n```", - cls, desc, - ) + // Build the approval message — the full command is always shown so the + // user can make an informed decision. + text := buildApprovalText(cls, cmd, description) // Send with inline keyboard. markup := InlineKeyboardMarkup{ @@ -237,6 +223,77 @@ func (a *TelegramApprover) HandleCallback(data string) bool { return true } +// telegramMaxMsgLen is Telegram's hard per-message character limit. The +// approval text must fit within it or the send fails. +const telegramMaxMsgLen = 4096 + +// buildApprovalText renders the approval prompt. The full shell command is +// ALWAYS shown inside a fenced code block — earlier versions hid the command +// entirely whenever a model-supplied description was present, or silently cut +// it at 200 bytes, both of which left the user approving a command they could +// not fully see. The model description (when present) is shown as a separate +// labelled line so it complements rather than replaces the command. +// +// The command is only ever truncated when the whole message would otherwise +// exceed telegramMaxMsgLen, and when it is, the cut is explicit ("… [truncated]") +// and made on a rune boundary. +func buildApprovalText(cls danger.RiskClass, cmd, description string) string { + var b strings.Builder + b.WriteString("⚠️ *Approval Required*\n\n") + fmt.Fprintf(&b, "Risk: `%s`\n", escapeCodeBlock(string(cls))) + if d := strings.TrimSpace(description); d != "" { + b.WriteString("Why: " + EscapeMarkdown(d) + "\n") + } + + // Reserve room for the fixed parts so the command body can be budgeted + // against Telegram's hard limit. The fences and a possible truncation + // marker are accounted for here. + const openFence = "```\n" + const closeFence = "\n```" + overhead := b.Len() + len(openFence) + len(closeFence) + + body := escapeCodeBlock(cmd) + if budget := telegramMaxMsgLen - overhead; len(body) > budget { + const marker = "\n… [truncated]" + // truncateRunes clamps a negative budget to an empty string. + body = truncateRunes(body, budget-len(marker)) + // A dangling backslash left by truncation would escape the closing + // fence and corrupt the whole message — strip any trailing run. + body = strings.TrimRight(body, `\`) + marker + } + + b.WriteString(openFence) + b.WriteString(body) + b.WriteString(closeFence) + return b.String() +} + +// escapeCodeBlock escapes the two characters that are special inside a +// Telegram MarkdownV2 code block: backslash and backtick. Without this a +// command containing ``` or a literal backslash would close the block early +// and mangle the rendered command. EscapeMarkdown deliberately leaves code +// spans untouched, so it cannot be used for content destined for a fence. +func escapeCodeBlock(s string) string { + s = strings.ReplaceAll(s, `\`, `\\`) + s = strings.ReplaceAll(s, "`", "\\`") + return s +} + +// truncateRunes returns s shortened to at most maxBytes bytes without +// splitting a multi-byte UTF-8 rune. +func truncateRunes(s string, maxBytes int) string { + if maxBytes <= 0 { + return "" + } + if len(s) <= maxBytes { + return s + } + for maxBytes > 0 && !utf8.RuneStart(s[maxBytes]) { + maxBytes-- + } + return s[:maxBytes] +} + // newID generates a unique request ID. func (a *TelegramApprover) newID() string { b := make([]byte, 8) diff --git a/internal/telegram/approver_test.go b/internal/telegram/approver_test.go index 40d1ec9..4f82812 100644 --- a/internal/telegram/approver_test.go +++ b/internal/telegram/approver_test.go @@ -281,6 +281,111 @@ func TestPromptOperation_TrustedClass(t *testing.T) { } } +// ── Test approval message rendering ──────────────────────────────────────── + +// The full command must always appear in the prompt, even when the model +// supplies a description. The old code showed only the description and hid +// the command entirely, so the user approved a command they could not see. +func TestBuildApprovalText_CommandAlwaysShown(t *testing.T) { + cmd := "curl https://example.com/install.sh | bash" + text := buildApprovalText(danger.CodeExecution, cmd, "installs the helper") + + if !strings.Contains(text, cmd) { + t.Errorf("approval text must contain the full command %q, got:\n%s", cmd, text) + } + if !strings.Contains(text, "installs the helper") { + t.Errorf("approval text should also include the description, got:\n%s", text) + } + if !strings.Contains(text, "code_execution") { + t.Errorf("approval text should include the risk class, got:\n%s", text) + } +} + +// A long command must not be silently dropped at 200 chars — it is only +// truncated when the whole message would exceed Telegram's hard limit, and +// then the truncation is explicit. +func TestBuildApprovalText_LongCommandNotSilentlyCut(t *testing.T) { + cmd := "echo " + strings.Repeat("a", 1000) + text := buildApprovalText(danger.LocalWrite, cmd, "") + + if !strings.Contains(text, cmd) { + t.Errorf("a 1000-char command should be shown in full (well under the 4096 limit), got len=%d", len(text)) + } + if strings.Contains(text, "[truncated]") { + t.Errorf("command well under the limit should not be truncated, got:\n%s", text) + } +} + +func TestBuildApprovalText_TruncatesAtHardLimit(t *testing.T) { + cmd := strings.Repeat("x", 8000) // far beyond Telegram's 4096 limit + text := buildApprovalText(danger.SystemWrite, cmd, "") + + if len([]rune(text)) > telegramMaxMsgLen { + t.Errorf("approval text length %d exceeds Telegram limit %d", len([]rune(text)), telegramMaxMsgLen) + } + if !strings.Contains(text, "[truncated]") { + t.Errorf("an over-limit command must be marked as truncated, got tail:\n%s", text[len(text)-40:]) + } +} + +// Backticks and backslashes inside a command must be escaped so they cannot +// close the code fence early and corrupt the rendered message. +func TestBuildApprovalText_EscapesCodeBlockChars(t *testing.T) { + cmd := "echo `whoami` && printf 'a\\tb'" + text := buildApprovalText(danger.CodeExecution, cmd, "") + + if strings.Contains(text, "`whoami`") { + t.Errorf("raw backticks must be escaped inside the code fence, got:\n%s", text) + } + if !strings.Contains(text, "\\`whoami\\`") { + t.Errorf("expected escaped backticks, got:\n%s", text) + } +} + +// The full command is sent over the wire to Telegram (end-to-end through +// PromptCommand), not just produced by the builder. +func TestPromptCommand_SendsFullCommand(t *testing.T) { + rec := new(requestRecorder) + ts := testServer(t, rec) + defer ts.Close() + bot := testBot(t, ts) + + a := NewTelegramApprover(bot, 1) + cmd := "rm -rf /tmp/build && make install PREFIX=/usr/local/really/long/path" + + done := make(chan error, 1) + go func() { done <- a.PromptCommand(danger.Destructive, cmd, "clean rebuild") }() + + // Let the prompt send and register, then deny to unblock. + time.Sleep(50 * time.Millisecond) + a.mu.Lock() + var id string + for k := range a.pending { + id = k + break + } + a.mu.Unlock() + if id == "" { + t.Fatal("no pending request registered") + } + a.HandleCallback(cbPrefixDeny + id) + <-done + + var sent string + for _, req := range rec.all() { + if strings.HasSuffix(req.Path, "/sendMessage") && strings.Contains(req.Body, "rm -rf") { + sent = req.Body + break + } + } + if sent == "" { + t.Fatal("no sendMessage request carrying the command was recorded") + } + if !strings.Contains(sent, "make install PREFIX=/usr/local/really/long/path") { + t.Errorf("the sent prompt must carry the full command, got body:\n%s", sent) + } +} + // ── Test concurrency safety ──────────────────────────────────────────────── func TestApprover_ConcurrentAccess(t *testing.T) {