From e9dd840111e9e5aa30caa85c712cd4f8d21d7e04 Mon Sep 17 00:00:00 2001 From: Codex Date: Mon, 25 May 2026 06:16:51 +0000 Subject: [PATCH] Unify approval tool message rendering --- internal/telegram/bot.go | 139 +++++++++++++------------------ internal/telegram/render_test.go | 35 ++++++++ 2 files changed, 92 insertions(+), 82 deletions(-) diff --git a/internal/telegram/bot.go b/internal/telegram/bot.go index 3a7a7d4..8879a59 100644 --- a/internal/telegram/bot.go +++ b/internal/telegram/bot.go @@ -1285,10 +1285,10 @@ func renderCodexItemDetailsHTML(item codexThreadItemView) string { switch item.Type { case "commandExecution": - appendField("CWD", item.CWD) if command := strings.TrimSpace(item.Command); command != "" { parts = append(parts, "Command", CodeBlockHTML("bash", command)) } + appendField("CWD", item.CWD) appendInt("Exit code", item.ExitCode) appendInt64("Duration ms", item.DurationMs) if item.AggregatedOutput != nil && strings.TrimSpace(*item.AggregatedOutput) != "" { @@ -1339,16 +1339,21 @@ func renderArgumentsDetailsHTML(raw json.RawMessage) []string { return []string{"Arguments", CodeBlockHTML("json", compactPrettyJSON(value))} } + parts := renderSelectedArgumentDetailsHTML(object, preferredArgumentKeys(object)) + if len(parts) == 0 && len(object) > 0 { + parts = append(parts, "Arguments", CodeBlockHTML("json", compactPrettyJSON(object))) + } + return parts +} + +func renderSelectedArgumentDetailsHTML(object map[string]any, keys []string) []string { var parts []string - for _, key := range preferredArgumentKeys(object) { + for _, key := range keys { part := renderArgumentFieldHTML(key, object[key]) if strings.TrimSpace(part) != "" { parts = append(parts, part) } } - if len(parts) == 0 && len(object) > 0 { - parts = append(parts, "Arguments", CodeBlockHTML("json", compactPrettyJSON(object))) - } return parts } @@ -1764,16 +1769,7 @@ func (b *Bot) handleCodexServerRequest(ctx context.Context, event codexapp.Event } text := renderApprovalHTML(kind, event.Params, "") markup := approvalMarkup(approval.ID) - if msg, ok, err := b.attachApprovalToToolMessage(ctx, threadID, itemID, text, markup); err != nil { - return err - } else if ok { - return b.store.UpdatePendingApprovalMessage(ctx, approval.ID, msg.Chat.ID, msg.MessageID) - } - - msg, err := b.tg.SendMessage(ctx, thread.TelegramUserID, text, SendMessageOptions{ - ParseMode: "HTML", - ReplyMarkup: markup, - }) + msg, err := b.upsertApprovalMessage(ctx, thread.TelegramUserID, threadID, itemID, text, markup) if err != nil { return err } @@ -2111,42 +2107,64 @@ func (b *Bot) upsertToolMessage(ctx context.Context, threadID, itemID, htmlText return nil } -func (b *Bot) attachApprovalToToolMessage(ctx context.Context, threadID, itemID, approvalHTML string, markup *InlineKeyboardMarkup) (Message, bool, error) { +func (b *Bot) upsertApprovalMessage(ctx context.Context, chatID int64, threadID, itemID, approvalHTML string, markup *InlineKeyboardMarkup) (Message, error) { approvalHTML = strings.TrimSpace(approvalHTML) - if threadID == "" || itemID == "" || approvalHTML == "" { - return Message{}, false, nil + if approvalHTML == "" { + return Message{}, errors.New("approval message is empty") + } + if threadID == "" || itemID == "" { + return b.tg.SendMessage(ctx, chatID, approvalHTML, SendMessageOptions{ParseMode: "HTML", ReplyMarkup: markup}) } if err := b.flushAssistantMessage(ctx, threadID); err != nil { - return Message{}, false, err + return Message{}, err + } + chatID, err := b.outputChatID(ctx, threadID) + if err != nil { + return Message{}, err } b.mu.Lock() state := b.outputs[threadID] - if state == nil { - b.mu.Unlock() - return Message{}, false, nil + if state != nil && state.tools == nil { + state.tools = make(map[string]toolMessageState) } - tool, ok := state.tools[itemID] - if !ok || tool.messageID == 0 { - b.mu.Unlock() - return Message{}, false, nil + if state != nil { + tool, ok := state.tools[itemID] + if ok && tool.messageID != 0 { + tool.approvalHTML = approvalHTML + tool.approvalMarkup = markup + tool.editedAt = editedAtTimestamp() + state.tools[itemID] = tool + combined := tool.html() + msg := Message{MessageID: tool.messageID, Chat: Chat{ID: tool.chatID}} + b.mu.Unlock() + _, err := b.tg.EditMessageText(ctx, msg.Chat.ID, msg.MessageID, combined, EditMessageTextOptions{ParseMode: "HTML", ReplyMarkup: editReplyMarkup(markup)}) + if err := ignoreTelegramMessageNotModified(err); err != nil { + b.clearToolApproval(threadID, itemID) + b.logger.Printf("edit tool approval message %s/%s: %v", threadID, itemID, err) + return Message{}, err + } + b.markOutputSent(threadID) + return msg, nil + } } - tool.approvalHTML = approvalHTML - tool.approvalMarkup = markup - tool.editedAt = editedAtTimestamp() - state.tools[itemID] = tool - combined := tool.html() - msg := Message{MessageID: tool.messageID, Chat: Chat{ID: tool.chatID}} b.mu.Unlock() - _, err := b.tg.EditMessageText(ctx, msg.Chat.ID, msg.MessageID, combined, EditMessageTextOptions{ParseMode: "HTML", ReplyMarkup: editReplyMarkup(markup)}) - if err := ignoreTelegramMessageNotModified(err); err != nil { - b.clearToolApproval(threadID, itemID) - b.logger.Printf("edit tool approval message %s/%s: %v", threadID, itemID, err) - return Message{}, false, nil + msg, err := b.sendHTMLMessage(ctx, chatID, approvalHTML, markup) + if err != nil { + return Message{}, err } + b.mu.Lock() + state = b.outputs[threadID] + if state != nil { + if state.tools == nil { + state.tools = make(map[string]toolMessageState) + } + state.tools[itemID] = toolMessageState{chatID: msg.Chat.ID, messageID: msg.MessageID, approvalHTML: approvalHTML, approvalMarkup: markup} + } + b.mu.Unlock() b.markOutputSent(threadID) - return msg, true, nil + return msg, nil } func (b *Bot) clearToolApproval(threadID, itemID string) { @@ -2808,12 +2826,6 @@ func renderApprovalHTML(kind string, raw json.RawMessage, status string) string if reason, _ := params["reason"].(string); reason != "" { lines = append(lines, "", reason) } - for _, key := range []string{"command", "cwd", "grantRoot", "permissions", "fileChanges"} { - if value, ok := params[key]; ok { - lines = append(lines, fmt.Sprintf("%s: %s", argumentLabel(key), conciseValue(value))) - } - } - summary := strings.Join(lines, "\n") details := renderApprovalDetailsHTML(kind, raw) limit := TelegramHTMLMessageLimit @@ -2832,29 +2844,7 @@ func renderApprovalDetailsHTML(kind string, raw json.RawMessage) string { if err := json.Unmarshal(raw, ¶ms); err != nil { return CodeBlockHTML("json", string(raw)) } - var parts []string - appendValue := func(label string, value any) { - text, complex := argumentValueText(value) - if strings.TrimSpace(text) == "" { - return - } - if complex || strings.Contains(text, "\n") || strings.EqualFold(label, "Command") || strings.EqualFold(label, "Permissions") { - language := "text" - if strings.EqualFold(label, "Command") { - language = "bash" - } else if complex || strings.EqualFold(label, "Permissions") || looksLikeJSON(text) { - language = "json" - } - parts = append(parts, ""+EscapeHTML(label)+"", CodeBlockHTML(language, text)) - return - } - parts = append(parts, FieldHTML(label, text)) - } - for _, key := range []string{"command", "cwd", "grantRoot", "permissions", "fileChanges", "parsedCmd", "reason"} { - if value, ok := params[key]; ok { - appendValue(argumentLabel(key), value) - } - } + parts := renderSelectedArgumentDetailsHTML(params, []string{"command", "cwd", "grantRoot", "permissions", "fileChanges", "parsedCmd", "reason"}) if len(parts) == 0 { return CodeBlockHTML("json", prettyJSON(raw)) } @@ -2876,21 +2866,6 @@ func approvalStatusLine(decision string) string { } } -func conciseValue(value any) string { - text := fmt.Sprint(value) - if stringValue, ok := value.(string); ok { - text = stringValue - } else if data, err := json.Marshal(value); err == nil { - text = string(data) - } - text = strings.Join(strings.Fields(text), " ") - runes := []rune(text) - if len(runes) > 180 { - return string(runes[:180]) + "..." - } - return text -} - func prettyJSON(raw json.RawMessage) string { if len(raw) == 0 { return "" diff --git a/internal/telegram/render_test.go b/internal/telegram/render_test.go index 7662e3c..d649e65 100644 --- a/internal/telegram/render_test.go +++ b/internal/telegram/render_test.go @@ -199,6 +199,7 @@ func TestRenderCodexCommandExecutionItem(t *testing.T) { item := codexThreadItemView{ Type: "commandExecution", Command: "go test ./...", + CWD: "/workspace/project", AggregatedOutput: &output, ExitCode: &exitCode, } @@ -208,6 +209,11 @@ func TestRenderCodexCommandExecutionItem(t *testing.T) { t.Fatalf("rendered command item missing %q in %q", want, text) } } + commandAt := strings.Index(text, "Command") + cwdAt := strings.Index(text, "CWD:") + if cwdAt >= 0 && commandAt > cwdAt { + t.Fatalf("command label should render before CWD to avoid Telegram attaching it to the CWD line: %q", text) + } } func TestRenderCodexStartedItems(t *testing.T) { @@ -244,11 +250,40 @@ func TestRenderApprovalDetailsAvoidsRawJSONDump(t *testing.T) { t.Fatalf("approval render missing %q in %q", want, text) } } + summary, _, _ := strings.Cut(text, "
") + for _, unwanted := range []string{"go test ./...", "/workspace/project"} { + if strings.Contains(summary, unwanted) { + t.Fatalf("approval summary should not include %q in %q", unwanted, summary) + } + } if strings.Contains(text, "unused") { t.Fatalf("approval render should omit unused JSON: %q", text) } } +func TestApprovalOnlyToolMessageCanReceiveCompletionDetails(t *testing.T) { + exitCode := 0 + duration := int64(1234) + output := "done" + tool := toolMessageState{ + approvalHTML: SummaryDetailsHTML("Codex requests command approval", "approval details"), + } + tool.toolHTML = renderCodexItemCompleted(codexThreadItemView{ + Type: "commandExecution", + Command: "go test ./...", + CWD: "/workspace/project", + ExitCode: &exitCode, + DurationMs: &duration, + AggregatedOutput: &output, + }) + text := tool.html() + for _, want := range []string{"Tool call: command finished", "Exit code: 0", "Duration ms", "1234", "Codex requests command approval"} { + if !strings.Contains(text, want) { + t.Fatalf("combined approval tool message missing %q in %q", want, text) + } + } +} + func TestToolMessageAddsEditedAtBeforeDetails(t *testing.T) { tool := toolMessageState{ toolHTML: SummaryDetailsHTML("Tool call: command finished\nCommand: go test ./...", "full output"),