Merge pull request #3 from BackendStack21/fix/approval-command-rendering

fix(approval): always render the full command in approval prompts

Author: jkyberneees

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 {

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)

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)

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) {