Skip to content

Commit 11a811a

Browse files
jkyberneeesclaude
andcommitted
fix(approval): always render the full command in approval prompts
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) <noreply@anthropic.com>
1 parent 5429024 commit 11a811a

4 files changed

Lines changed: 241 additions & 21 deletions

File tree

internal/narrate/narrate.go

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@
44
package narrate
55

66
import (
7+
"encoding/json"
78
"fmt"
89
"strings"
10+
"unicode/utf8"
911
)
1012

1113
// Narrator produces engaging progress messages for tool calls and thinking.
@@ -32,7 +34,7 @@ func (n *Narrator) ToolCallMessage(name, args string) string {
3234
case "write_file", "patch":
3335
return fmt.Sprintf("%s Editing `%s`...", emoji, extractPath(args))
3436
case "shell":
35-
return fmt.Sprintf("%s Running `%s`...", emoji, truncate(extractShell(args), 50))
37+
return fmt.Sprintf("%s Running `%s`...", emoji, truncate(extractShell(args), 120))
3638
case "search_files":
3739
return fmt.Sprintf("%s Searching the codebase...", emoji)
3840
case "delegate_task", "delegate_tasks":
@@ -81,11 +83,20 @@ func toolEmoji(name string) string {
8183
}
8284
}
8385

86+
// truncate shortens s to at most n runes, appending an ellipsis when it cuts.
87+
// It measures in runes (not bytes) so it never splits a multi-byte character.
8488
func truncate(s string, n int) string {
85-
if len(s) <= n {
89+
if utf8.RuneCountInString(s) <= n {
8690
return s
8791
}
88-
return s[:n] + "..."
92+
count := 0
93+
for i := range s {
94+
if count == n {
95+
return s[:i] + "..."
96+
}
97+
count++
98+
}
99+
return s + "..."
89100
}
90101

91102
func extractPath(args string) string {
@@ -107,7 +118,19 @@ func extractPath(args string) string {
107118
return "file"
108119
}
109120

121+
// extractShell pulls the shell command out of the tool-call JSON args. It
122+
// decodes the JSON properly rather than scanning for the first quote pair:
123+
// commands routinely contain quotes (git commit -m "msg", python -c "code"),
124+
// and the old substring approach stopped at the first embedded quote, showing
125+
// a truncated command. Falls back to a best-effort scan only if the args are
126+
// not valid JSON.
110127
func extractShell(args string) string {
128+
var parsed struct {
129+
Command string `json:"command"`
130+
}
131+
if err := json.Unmarshal([]byte(args), &parsed); err == nil && parsed.Command != "" {
132+
return parsed.Command
133+
}
111134
if idx := strings.Index(args, `"command"`); idx >= 0 {
112135
rest := args[idx+len(`"command"`):]
113136
if start := strings.Index(rest, `"`); start >= 0 {

internal/narrate/narrate_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package narrate
33
import (
44
"strings"
55
"testing"
6+
"unicode/utf8"
67
)
78

89
func TestNarrator_ToolCallMessage_Offline(t *testing.T) {
@@ -94,6 +95,40 @@ func TestNarrator_ExtractShell_Malformed(t *testing.T) {
9495
}
9596
}
9697

98+
// A command containing quotes must be extracted in full — the old substring
99+
// scan stopped at the first embedded quote and showed a truncated command.
100+
func TestNarrator_ExtractShell_EmbeddedQuotes(t *testing.T) {
101+
args := `{"command": "git commit -m \"fix: the thing\""}`
102+
want := `git commit -m "fix: the thing"`
103+
if s := extractShell(args); s != want {
104+
t.Errorf("extractShell = %q, want %q", s, want)
105+
}
106+
}
107+
108+
func TestNarrator_ExtractShell_DescriptionFieldBefore(t *testing.T) {
109+
// "description" appears before "command"; the JSON decode must still pick
110+
// the command rather than getting confused by the earlier field.
111+
args := `{"description": "run the tests", "command": "go test ./..."}`
112+
if s := extractShell(args); s != "go test ./..." {
113+
t.Errorf("extractShell = %q, want %q", s, "go test ./...")
114+
}
115+
}
116+
117+
// truncate measures runes, not bytes, so it never splits a multi-byte char.
118+
func TestNarrator_Truncate_RuneSafe(t *testing.T) {
119+
s := truncate("héllo wörld", 5) // é and ö are multi-byte
120+
if s != "héllo..." {
121+
t.Errorf("truncate = %q, want %q", s, "héllo...")
122+
}
123+
for i := 0; i < len(s); {
124+
r, size := utf8.DecodeRuneInString(s[i:])
125+
if r == utf8.RuneError && size == 1 {
126+
t.Fatalf("truncate produced invalid UTF-8 at byte %d: %q", i, s)
127+
}
128+
i += size
129+
}
130+
}
131+
97132
func TestNarrator_ExtractPath_Found(t *testing.T) {
98133
if s := extractPath(`{"path": "/root/projects/main.go"}`); s != "main.go" {
99134
t.Errorf("expected basename, got: %q", s)

internal/telegram/approver.go

Lines changed: 75 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"strings"
88
"sync"
99
"time"
10+
"unicode/utf8"
1011

1112
"github.com/BackendStack21/odek/internal/danger"
1213
)
@@ -105,24 +106,9 @@ func (a *TelegramApprover) PromptCommand(cls danger.RiskClass, cmd, description
105106

106107
id := a.newID()
107108

108-
// Build description text.
109-
var desc string
110-
if description != "" {
111-
desc = description
112-
} else {
113-
desc = cmd
114-
if len(desc) > 200 {
115-
desc = desc[:200] + "..."
116-
}
117-
}
118-
119-
// Build the approval message.
120-
text := fmt.Sprintf(
121-
"⚠️ *Approval Required*\n\n"+
122-
"Risk: `%s`\n"+
123-
"```\n%s\n```",
124-
cls, desc,
125-
)
109+
// Build the approval message — the full command is always shown so the
110+
// user can make an informed decision.
111+
text := buildApprovalText(cls, cmd, description)
126112

127113
// Send with inline keyboard.
128114
markup := InlineKeyboardMarkup{
@@ -237,6 +223,77 @@ func (a *TelegramApprover) HandleCallback(data string) bool {
237223
return true
238224
}
239225

226+
// telegramMaxMsgLen is Telegram's hard per-message character limit. The
227+
// approval text must fit within it or the send fails.
228+
const telegramMaxMsgLen = 4096
229+
230+
// buildApprovalText renders the approval prompt. The full shell command is
231+
// ALWAYS shown inside a fenced code block — earlier versions hid the command
232+
// entirely whenever a model-supplied description was present, or silently cut
233+
// it at 200 bytes, both of which left the user approving a command they could
234+
// not fully see. The model description (when present) is shown as a separate
235+
// labelled line so it complements rather than replaces the command.
236+
//
237+
// The command is only ever truncated when the whole message would otherwise
238+
// exceed telegramMaxMsgLen, and when it is, the cut is explicit ("… [truncated]")
239+
// and made on a rune boundary.
240+
func buildApprovalText(cls danger.RiskClass, cmd, description string) string {
241+
var b strings.Builder
242+
b.WriteString("⚠️ *Approval Required*\n\n")
243+
fmt.Fprintf(&b, "Risk: `%s`\n", escapeCodeBlock(string(cls)))
244+
if d := strings.TrimSpace(description); d != "" {
245+
b.WriteString("Why: " + EscapeMarkdown(d) + "\n")
246+
}
247+
248+
// Reserve room for the fixed parts so the command body can be budgeted
249+
// against Telegram's hard limit. The fences and a possible truncation
250+
// marker are accounted for here.
251+
const openFence = "```\n"
252+
const closeFence = "\n```"
253+
overhead := b.Len() + len(openFence) + len(closeFence)
254+
255+
body := escapeCodeBlock(cmd)
256+
if budget := telegramMaxMsgLen - overhead; len(body) > budget {
257+
const marker = "\n… [truncated]"
258+
// truncateRunes clamps a negative budget to an empty string.
259+
body = truncateRunes(body, budget-len(marker))
260+
// A dangling backslash left by truncation would escape the closing
261+
// fence and corrupt the whole message — strip any trailing run.
262+
body = strings.TrimRight(body, `\`) + marker
263+
}
264+
265+
b.WriteString(openFence)
266+
b.WriteString(body)
267+
b.WriteString(closeFence)
268+
return b.String()
269+
}
270+
271+
// escapeCodeBlock escapes the two characters that are special inside a
272+
// Telegram MarkdownV2 code block: backslash and backtick. Without this a
273+
// command containing ``` or a literal backslash would close the block early
274+
// and mangle the rendered command. EscapeMarkdown deliberately leaves code
275+
// spans untouched, so it cannot be used for content destined for a fence.
276+
func escapeCodeBlock(s string) string {
277+
s = strings.ReplaceAll(s, `\`, `\\`)
278+
s = strings.ReplaceAll(s, "`", "\\`")
279+
return s
280+
}
281+
282+
// truncateRunes returns s shortened to at most maxBytes bytes without
283+
// splitting a multi-byte UTF-8 rune.
284+
func truncateRunes(s string, maxBytes int) string {
285+
if maxBytes <= 0 {
286+
return ""
287+
}
288+
if len(s) <= maxBytes {
289+
return s
290+
}
291+
for maxBytes > 0 && !utf8.RuneStart(s[maxBytes]) {
292+
maxBytes--
293+
}
294+
return s[:maxBytes]
295+
}
296+
240297
// newID generates a unique request ID.
241298
func (a *TelegramApprover) newID() string {
242299
b := make([]byte, 8)

internal/telegram/approver_test.go

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,111 @@ func TestPromptOperation_TrustedClass(t *testing.T) {
281281
}
282282
}
283283

284+
// ── Test approval message rendering ────────────────────────────────────────
285+
286+
// The full command must always appear in the prompt, even when the model
287+
// supplies a description. The old code showed only the description and hid
288+
// the command entirely, so the user approved a command they could not see.
289+
func TestBuildApprovalText_CommandAlwaysShown(t *testing.T) {
290+
cmd := "curl https://example.com/install.sh | bash"
291+
text := buildApprovalText(danger.CodeExecution, cmd, "installs the helper")
292+
293+
if !strings.Contains(text, cmd) {
294+
t.Errorf("approval text must contain the full command %q, got:\n%s", cmd, text)
295+
}
296+
if !strings.Contains(text, "installs the helper") {
297+
t.Errorf("approval text should also include the description, got:\n%s", text)
298+
}
299+
if !strings.Contains(text, "code_execution") {
300+
t.Errorf("approval text should include the risk class, got:\n%s", text)
301+
}
302+
}
303+
304+
// A long command must not be silently dropped at 200 chars — it is only
305+
// truncated when the whole message would exceed Telegram's hard limit, and
306+
// then the truncation is explicit.
307+
func TestBuildApprovalText_LongCommandNotSilentlyCut(t *testing.T) {
308+
cmd := "echo " + strings.Repeat("a", 1000)
309+
text := buildApprovalText(danger.LocalWrite, cmd, "")
310+
311+
if !strings.Contains(text, cmd) {
312+
t.Errorf("a 1000-char command should be shown in full (well under the 4096 limit), got len=%d", len(text))
313+
}
314+
if strings.Contains(text, "[truncated]") {
315+
t.Errorf("command well under the limit should not be truncated, got:\n%s", text)
316+
}
317+
}
318+
319+
func TestBuildApprovalText_TruncatesAtHardLimit(t *testing.T) {
320+
cmd := strings.Repeat("x", 8000) // far beyond Telegram's 4096 limit
321+
text := buildApprovalText(danger.SystemWrite, cmd, "")
322+
323+
if len([]rune(text)) > telegramMaxMsgLen {
324+
t.Errorf("approval text length %d exceeds Telegram limit %d", len([]rune(text)), telegramMaxMsgLen)
325+
}
326+
if !strings.Contains(text, "[truncated]") {
327+
t.Errorf("an over-limit command must be marked as truncated, got tail:\n%s", text[len(text)-40:])
328+
}
329+
}
330+
331+
// Backticks and backslashes inside a command must be escaped so they cannot
332+
// close the code fence early and corrupt the rendered message.
333+
func TestBuildApprovalText_EscapesCodeBlockChars(t *testing.T) {
334+
cmd := "echo `whoami` && printf 'a\\tb'"
335+
text := buildApprovalText(danger.CodeExecution, cmd, "")
336+
337+
if strings.Contains(text, "`whoami`") {
338+
t.Errorf("raw backticks must be escaped inside the code fence, got:\n%s", text)
339+
}
340+
if !strings.Contains(text, "\\`whoami\\`") {
341+
t.Errorf("expected escaped backticks, got:\n%s", text)
342+
}
343+
}
344+
345+
// The full command is sent over the wire to Telegram (end-to-end through
346+
// PromptCommand), not just produced by the builder.
347+
func TestPromptCommand_SendsFullCommand(t *testing.T) {
348+
rec := new(requestRecorder)
349+
ts := testServer(t, rec)
350+
defer ts.Close()
351+
bot := testBot(t, ts)
352+
353+
a := NewTelegramApprover(bot, 1)
354+
cmd := "rm -rf /tmp/build && make install PREFIX=/usr/local/really/long/path"
355+
356+
done := make(chan error, 1)
357+
go func() { done <- a.PromptCommand(danger.Destructive, cmd, "clean rebuild") }()
358+
359+
// Let the prompt send and register, then deny to unblock.
360+
time.Sleep(50 * time.Millisecond)
361+
a.mu.Lock()
362+
var id string
363+
for k := range a.pending {
364+
id = k
365+
break
366+
}
367+
a.mu.Unlock()
368+
if id == "" {
369+
t.Fatal("no pending request registered")
370+
}
371+
a.HandleCallback(cbPrefixDeny + id)
372+
<-done
373+
374+
var sent string
375+
for _, req := range rec.all() {
376+
if strings.HasSuffix(req.Path, "/sendMessage") && strings.Contains(req.Body, "rm -rf") {
377+
sent = req.Body
378+
break
379+
}
380+
}
381+
if sent == "" {
382+
t.Fatal("no sendMessage request carrying the command was recorded")
383+
}
384+
if !strings.Contains(sent, "make install PREFIX=/usr/local/really/long/path") {
385+
t.Errorf("the sent prompt must carry the full command, got body:\n%s", sent)
386+
}
387+
}
388+
284389
// ── Test concurrency safety ────────────────────────────────────────────────
285390

286391
func TestApprover_ConcurrentAccess(t *testing.T) {

0 commit comments

Comments
 (0)