Improve approval and sandbox flows
This commit is contained in:
Codex
@@ -6,6 +6,7 @@ import (
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"html"
|
||||
"log"
|
||||
"mime"
|
||||
"os"
|
||||
@@ -219,7 +220,7 @@ func botCommands() []BotCommand {
|
||||
{Command: "cancel", Description: "Interrupt the active turn"},
|
||||
{Command: "workspace", Description: "Select workspace"},
|
||||
{Command: "model", Description: "Choose model"},
|
||||
{Command: "sandbox", Description: "Show or set sandbox"},
|
||||
{Command: "sandbox", Description: "Choose sandbox"},
|
||||
{Command: "pic", Description: "Generate images"},
|
||||
{Command: "diff", Description: "Show latest diff"},
|
||||
{Command: "help", Description: "Show help"},
|
||||
@@ -332,7 +333,7 @@ func (b *Bot) sendHelp(ctx context.Context, chatID int64) error {
|
||||
"/workspaces - list workspaces",
|
||||
"/workspace [ID] - select workspace",
|
||||
"/model - choose model and reasoning effort",
|
||||
"/sandbox [read-only|workspace-write|danger-full-access] - show or set sandbox",
|
||||
"/sandbox - choose sandbox",
|
||||
"/pic PROMPT - generate image(s) from a prompt",
|
||||
"/diff - show the latest streamed diff",
|
||||
"",
|
||||
@@ -776,19 +777,43 @@ func (b *Bot) handleEffortCallback(ctx context.Context, callback *CallbackQuery,
|
||||
}
|
||||
|
||||
func (b *Bot) handleSandboxCommand(ctx context.Context, userID, chatID int64, session store.Session, args []string) error {
|
||||
if len(args) == 0 {
|
||||
_, err := b.tg.SendMessage(ctx, chatID, "Current sandbox: "+session.Sandbox, SendMessageOptions{})
|
||||
_ = userID
|
||||
if len(args) > 0 {
|
||||
_, err := b.tg.SendMessage(ctx, chatID, "Use /sandbox and choose from the buttons.", SendMessageOptions{})
|
||||
return err
|
||||
}
|
||||
sandbox, err := codexapp.NormalizeSandbox(args[0])
|
||||
return b.sendSandboxChoices(ctx, chatID, session)
|
||||
}
|
||||
|
||||
func (b *Bot) sendSandboxChoices(ctx context.Context, chatID int64, session store.Session) error {
|
||||
text := sandboxStatusText(session.Sandbox) + "\nChoose sandbox:"
|
||||
message, err := b.tg.SendMessage(ctx, chatID, EscapeHTML(text), SendMessageOptions{
|
||||
ParseMode: "HTML",
|
||||
ReplyMarkup: sandboxMarkup(session.Sandbox),
|
||||
})
|
||||
if err != nil {
|
||||
_, sendErr := b.tg.SendMessage(ctx, chatID, "Use one of: read-only, workspace-write, danger-full-access.", SendMessageOptions{})
|
||||
return sendErr
|
||||
}
|
||||
if err := b.store.SetSessionSandbox(ctx, userID, sandbox); err != nil {
|
||||
return err
|
||||
}
|
||||
_, err = b.tg.SendMessage(ctx, chatID, "Sandbox set to "+sandbox+".", SendMessageOptions{})
|
||||
b.rememberSettingsMessage(ctx, session.TelegramUserID, chatID, message.MessageID)
|
||||
return nil
|
||||
}
|
||||
|
||||
func (b *Bot) handleSandboxCallback(ctx context.Context, callback *CallbackQuery, sandbox string) error {
|
||||
normalized, err := codexapp.NormalizeSandbox(sandbox)
|
||||
if err != nil {
|
||||
return b.tg.AnswerCallbackQuery(ctx, callback.ID, "Unsupported sandbox.")
|
||||
}
|
||||
if err := b.store.SetSessionSandbox(ctx, callback.From.ID, normalized); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := b.tg.AnswerCallbackQuery(ctx, callback.ID, "Sandbox selected."); err != nil {
|
||||
return err
|
||||
}
|
||||
text := sandboxStatusText(normalized)
|
||||
message, err := b.tg.EditMessageText(ctx, callback.Message.Chat.ID, callback.Message.MessageID, EscapeHTML(text), EditMessageTextOptions{ParseMode: "HTML"})
|
||||
if err == nil {
|
||||
b.rememberSettingsMessage(ctx, callback.From.ID, callback.Message.Chat.ID, message.MessageID)
|
||||
}
|
||||
return err
|
||||
}
|
||||
|
||||
@@ -1186,6 +1211,9 @@ func (b *Bot) handleCallback(ctx context.Context, callback *CallbackQuery) error
|
||||
if effort, ok := ParseEffortCallbackData(callback.Data); ok {
|
||||
return b.handleEffortCallback(ctx, callback, effort)
|
||||
}
|
||||
if sandbox, ok := ParseSandboxCallbackData(callback.Data); ok {
|
||||
return b.handleSandboxCallback(ctx, callback, sandbox)
|
||||
}
|
||||
if approvalID, decision, ok := ParseApprovalCallbackData(callback.Data); ok {
|
||||
return b.handleApprovalCallback(ctx, callback, approvalID, decision)
|
||||
}
|
||||
@@ -2202,10 +2230,76 @@ func combineToolApprovalHTML(toolHTML, approvalHTML string) string {
|
||||
case approvalHTML == "":
|
||||
return toolHTML
|
||||
default:
|
||||
return toolHTML + "\n\n" + approvalHTML
|
||||
return toolHTML + "\n\n" + compactApprovalHTMLForTool(toolHTML, approvalHTML)
|
||||
}
|
||||
}
|
||||
|
||||
func compactApprovalHTMLForTool(toolHTML, approvalHTML string) string {
|
||||
return removeDuplicateLabeledQuotes(approvalHTML, labelsPresentInToolHTML(toolHTML))
|
||||
}
|
||||
|
||||
func labelsPresentInToolHTML(toolHTML string) map[string]bool {
|
||||
labels := make(map[string]bool)
|
||||
for _, label := range []string{"CWD", "Command"} {
|
||||
if strings.Contains(toolHTML, "<b>"+EscapeHTML(label)+":</b>") {
|
||||
labels[label] = true
|
||||
}
|
||||
}
|
||||
return labels
|
||||
}
|
||||
|
||||
func removeDuplicateLabeledQuotes(htmlText string, labels map[string]bool) string {
|
||||
if len(labels) == 0 || strings.TrimSpace(htmlText) == "" {
|
||||
return htmlText
|
||||
}
|
||||
const open = "<blockquote expandable>"
|
||||
const close = "</blockquote>"
|
||||
var out strings.Builder
|
||||
searchFrom := 0
|
||||
for {
|
||||
start := strings.Index(htmlText[searchFrom:], open)
|
||||
if start < 0 {
|
||||
out.WriteString(htmlText[searchFrom:])
|
||||
break
|
||||
}
|
||||
start += searchFrom
|
||||
contentStart := start + len(open)
|
||||
end := strings.Index(htmlText[contentStart:], close)
|
||||
if end < 0 {
|
||||
out.WriteString(htmlText[searchFrom:])
|
||||
break
|
||||
}
|
||||
end += contentStart
|
||||
content := htmlText[contentStart:end]
|
||||
label := quoteLabel(content)
|
||||
if labels[label] {
|
||||
out.WriteString(strings.TrimRight(htmlText[searchFrom:start], "\n"))
|
||||
searchFrom = end + len(close)
|
||||
for strings.HasPrefix(htmlText[searchFrom:], "\n") {
|
||||
searchFrom++
|
||||
}
|
||||
continue
|
||||
}
|
||||
out.WriteString(htmlText[searchFrom : end+len(close)])
|
||||
searchFrom = end + len(close)
|
||||
}
|
||||
return strings.TrimSpace(out.String())
|
||||
}
|
||||
|
||||
func quoteLabel(content string) string {
|
||||
const boldOpen = "<b>"
|
||||
const labelClose = ":</b>"
|
||||
content = strings.TrimSpace(content)
|
||||
if !strings.HasPrefix(content, boldOpen) {
|
||||
return ""
|
||||
}
|
||||
end := strings.Index(content, labelClose)
|
||||
if end < 0 {
|
||||
return ""
|
||||
}
|
||||
return html.UnescapeString(content[len(boldOpen):end])
|
||||
}
|
||||
|
||||
func addEditedAtLine(htmlText, editedAt string) string {
|
||||
htmlText = strings.TrimSpace(htmlText)
|
||||
if htmlText == "" || editedAt == "" {
|
||||
@@ -2907,6 +3001,36 @@ func effortMarkup(model codexapp.Model) *InlineKeyboardMarkup {
|
||||
return &InlineKeyboardMarkup{InlineKeyboard: keyboard}
|
||||
}
|
||||
|
||||
func sandboxMarkup(current string) *InlineKeyboardMarkup {
|
||||
options := []struct {
|
||||
Value string
|
||||
Label string
|
||||
}{
|
||||
{Value: "read-only", Label: "Read only"},
|
||||
{Value: "workspace-write", Label: "Workspace write"},
|
||||
{Value: "danger-full-access", Label: "Danger full access"},
|
||||
}
|
||||
keyboard := make([][]InlineKeyboardButton, 0, len(options))
|
||||
for _, option := range options {
|
||||
label := option.Label
|
||||
if option.Value == current {
|
||||
label += " current"
|
||||
}
|
||||
keyboard = append(keyboard, []InlineKeyboardButton{{
|
||||
Text: label,
|
||||
CallbackData: SandboxCallbackData(option.Value),
|
||||
}})
|
||||
}
|
||||
return &InlineKeyboardMarkup{InlineKeyboard: keyboard}
|
||||
}
|
||||
|
||||
func sandboxStatusText(sandbox string) string {
|
||||
if strings.TrimSpace(sandbox) == "" {
|
||||
sandbox = "(Codex default)"
|
||||
}
|
||||
return "Current sandbox: " + sandbox
|
||||
}
|
||||
|
||||
func modelSupportsEffort(model codexapp.Model, effort string) bool {
|
||||
for _, option := range model.SupportedReasoningEfforts {
|
||||
if option.ReasoningEffort == effort {
|
||||
@@ -3281,13 +3405,140 @@ func renderApprovalDetailSectionsHTML(kind string, raw json.RawMessage) []string
|
||||
if err := json.Unmarshal(raw, ¶ms); err != nil {
|
||||
return []string{CodeBlockHTML("json", string(raw))}
|
||||
}
|
||||
parts := renderSelectedArgumentDetailsHTML(params, []string{"cwd", "command", "additionalPermissions", "networkApprovalContext", "proposedExecpolicyAmendment", "proposedNetworkPolicyAmendments", "availableDecisions", "grantRoot", "permissions", "fileChanges", "parsedCmd", "reason"})
|
||||
parts := renderApprovalPayloadDetailsHTML(raw, params)
|
||||
if len(parts) == 0 {
|
||||
return []string{CodeBlockHTML("json", prettyJSON(raw))}
|
||||
}
|
||||
return nonEmptyHTML(parts)
|
||||
}
|
||||
|
||||
func renderApprovalPayloadDetailsHTML(raw json.RawMessage, params map[string]any) []string {
|
||||
var parts []string
|
||||
appendPart := func(part string) {
|
||||
if strings.TrimSpace(part) != "" {
|
||||
parts = append(parts, part)
|
||||
}
|
||||
}
|
||||
|
||||
appendPart(renderApprovalFieldHTML("cwd", params["cwd"]))
|
||||
appendPart(renderApprovalFieldHTML("command", params["command"]))
|
||||
appendPart(renderApprovalFieldHTML("parsedCmd", params["parsedCmd"]))
|
||||
appendPart(renderApprovalFieldHTML("additionalPermissions", params["additionalPermissions"]))
|
||||
appendPart(renderNetworkApprovalContextHTML(params["networkApprovalContext"]))
|
||||
appendPart(renderExecpolicyAmendmentHTML(params["proposedExecpolicyAmendment"]))
|
||||
appendPart(renderNetworkPolicyAmendmentsHTML(params["proposedNetworkPolicyAmendments"]))
|
||||
appendPart(renderAvailableDecisionsHTML(raw))
|
||||
appendPart(renderApprovalFieldHTML("grantRoot", params["grantRoot"]))
|
||||
appendPart(renderApprovalFieldHTML("permissions", params["permissions"]))
|
||||
appendPart(renderApprovalFieldHTML("fileChanges", params["fileChanges"]))
|
||||
return parts
|
||||
}
|
||||
|
||||
func renderApprovalFieldHTML(key string, value any) string {
|
||||
if strings.EqualFold(key, "reason") {
|
||||
return ""
|
||||
}
|
||||
return renderArgumentFieldHTML(key, value)
|
||||
}
|
||||
|
||||
func renderExecpolicyAmendmentHTML(value any) string {
|
||||
items := stringSliceValue(value)
|
||||
if len(items) == 0 {
|
||||
return ""
|
||||
}
|
||||
return "<b>Proposed command rule:</b> " + CodeBlockHTML("bash", strings.Join(items, " "))
|
||||
}
|
||||
|
||||
func renderNetworkPolicyAmendmentsHTML(value any) string {
|
||||
entries, ok := value.([]any)
|
||||
if !ok || len(entries) == 0 {
|
||||
return ""
|
||||
}
|
||||
var lines []string
|
||||
for _, entry := range entries {
|
||||
object, ok := entry.(map[string]any)
|
||||
if !ok {
|
||||
continue
|
||||
}
|
||||
action := strings.TrimSpace(fmt.Sprint(object["action"]))
|
||||
host := strings.TrimSpace(fmt.Sprint(object["host"]))
|
||||
if action == "" && host == "" {
|
||||
continue
|
||||
}
|
||||
lines = append(lines, strings.TrimSpace(strings.Join(nonEmptyStrings(action, host), " ")))
|
||||
}
|
||||
if len(lines) == 0 {
|
||||
return ""
|
||||
}
|
||||
return FieldHTML("Proposed network rules", strings.Join(lines, "\n"))
|
||||
}
|
||||
|
||||
func renderNetworkApprovalContextHTML(value any) string {
|
||||
object, ok := value.(map[string]any)
|
||||
if !ok || len(object) == 0 {
|
||||
return ""
|
||||
}
|
||||
var fields []string
|
||||
for _, key := range []string{"host", "port", "protocol", "url"} {
|
||||
if text := strings.TrimSpace(fmt.Sprint(object[key])); text != "" && text != "<nil>" {
|
||||
fields = append(fields, key+"="+text)
|
||||
}
|
||||
}
|
||||
if len(fields) == 0 {
|
||||
return renderArgumentFieldHTML("networkApprovalContext", value)
|
||||
}
|
||||
return FieldHTML("Network request", strings.Join(fields, " "))
|
||||
}
|
||||
|
||||
func renderAvailableDecisionsHTML(raw json.RawMessage) string {
|
||||
options := approvalDecisionOptions(raw)
|
||||
if len(options) == 0 {
|
||||
return ""
|
||||
}
|
||||
seen := make(map[string]bool, len(options))
|
||||
var labels []string
|
||||
for _, option := range options {
|
||||
label := strings.TrimSpace(option.Label)
|
||||
if label == "" || seen[label] {
|
||||
continue
|
||||
}
|
||||
labels = append(labels, label)
|
||||
seen[label] = true
|
||||
}
|
||||
if len(labels) == 0 {
|
||||
return ""
|
||||
}
|
||||
return FieldHTML("Available decisions", strings.Join(labels, ", "))
|
||||
}
|
||||
|
||||
func stringSliceValue(value any) []string {
|
||||
switch v := value.(type) {
|
||||
case []string:
|
||||
return nonEmptyStrings(v...)
|
||||
case []any:
|
||||
var out []string
|
||||
for _, item := range v {
|
||||
text := strings.TrimSpace(fmt.Sprint(item))
|
||||
if text != "" && text != "<nil>" {
|
||||
out = append(out, text)
|
||||
}
|
||||
}
|
||||
return out
|
||||
default:
|
||||
return nil
|
||||
}
|
||||
}
|
||||
|
||||
func nonEmptyStrings(values ...string) []string {
|
||||
var out []string
|
||||
for _, value := range values {
|
||||
if trimmed := strings.TrimSpace(value); trimmed != "" {
|
||||
out = append(out, trimmed)
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
func approvalStatusLine(decision string) string {
|
||||
if strings.HasPrefix(decision, "networkPolicy") {
|
||||
return "Applied network rule."
|
||||
|
||||
@@ -443,6 +443,23 @@ func ParseApprovalCallbackData(data string) (int64, string, bool) {
|
||||
}
|
||||
}
|
||||
|
||||
func SandboxCallbackData(sandbox string) string {
|
||||
return "sandbox:" + sandbox
|
||||
}
|
||||
|
||||
func ParseSandboxCallbackData(data string) (string, bool) {
|
||||
if !strings.HasPrefix(data, "sandbox:") {
|
||||
return "", false
|
||||
}
|
||||
sandbox := strings.TrimPrefix(data, "sandbox:")
|
||||
switch sandbox {
|
||||
case "read-only", "workspace-write", "danger-full-access":
|
||||
return sandbox, true
|
||||
default:
|
||||
return "", false
|
||||
}
|
||||
}
|
||||
|
||||
func WorkspaceCallbackData(id int64) string {
|
||||
return fmt.Sprintf("workspace:%d", id)
|
||||
}
|
||||
|
||||
@@ -371,6 +371,57 @@ func TestRenderApprovalDetailsAvoidsRawJSONDump(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestRenderApprovalDetailsSummarizesPolicyFields(t *testing.T) {
|
||||
raw := json.RawMessage(`{
|
||||
"command":"git push gitea master",
|
||||
"cwd":"/workspace/project",
|
||||
"reason":"Need to publish changes",
|
||||
"proposedExecpolicyAmendment":["git","push","gitea","master"],
|
||||
"availableDecisions":[
|
||||
{"acceptWithExecpolicyAmendment":{"execpolicy_amendment":["git","push","gitea","master"]}},
|
||||
"decline"
|
||||
]
|
||||
}`)
|
||||
text := renderApprovalHTML("item/commandExecution/requestApproval", raw, "")
|
||||
for _, want := range []string{"Proposed command rule", "git push gitea master", "Available decisions", "Approve rule", "Deny"} {
|
||||
if !strings.Contains(text, want) {
|
||||
t.Fatalf("approval render missing concise field %q in %q", want, text)
|
||||
}
|
||||
}
|
||||
for _, unwanted := range []string{"execpolicy_amendment", "acceptWithExecpolicyAmendment", "availableDecisions"} {
|
||||
if strings.Contains(text, unwanted) {
|
||||
t.Fatalf("approval render still contains verbose field %q in %q", unwanted, text)
|
||||
}
|
||||
}
|
||||
if strings.Count(text, "Need to publish changes") != 1 {
|
||||
t.Fatalf("reason should only appear once in summary: %q", text)
|
||||
}
|
||||
}
|
||||
|
||||
func TestCombinedCommandApprovalOmitsDuplicateCommandDetails(t *testing.T) {
|
||||
toolHTML := renderCodexItemStarted(codexThreadItemView{
|
||||
Type: "commandExecution",
|
||||
Command: "go test ./...",
|
||||
CWD: "/workspace/project",
|
||||
})
|
||||
approvalHTML := renderApprovalHTML("item/commandExecution/requestApproval", json.RawMessage(`{
|
||||
"command":"go test ./...",
|
||||
"cwd":"/workspace/project",
|
||||
"reason":"Need to run tests"
|
||||
}`), "")
|
||||
text := combineToolApprovalHTML(toolHTML, approvalHTML)
|
||||
for _, want := range []string{"Tool call: command started", "Codex requests command approval", "Need to run tests"} {
|
||||
if !strings.Contains(text, want) {
|
||||
t.Fatalf("combined approval missing %q in %q", want, text)
|
||||
}
|
||||
}
|
||||
for _, duplicate := range []string{"<b>CWD:</b>", "<b>Command:</b>"} {
|
||||
if got := strings.Count(text, duplicate); got != 1 {
|
||||
t.Fatalf("%s count = %d, want 1 in %q", duplicate, got, text)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestApprovalOnlyToolMessageCanReceiveCompletionDetails(t *testing.T) {
|
||||
exitCode := 0
|
||||
duration := int64(1234)
|
||||
@@ -464,7 +515,7 @@ func TestResumeThreadListText(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestModelAndEffortCallbackData(t *testing.T) {
|
||||
func TestModelEffortAndSandboxCallbackData(t *testing.T) {
|
||||
modelID := strings.Join([]string{"server", "model", "id"}, "-")
|
||||
data, ok := ModelCallbackData(modelID)
|
||||
if !ok {
|
||||
@@ -479,4 +530,11 @@ func TestModelAndEffortCallbackData(t *testing.T) {
|
||||
if !ok || effort != effortName {
|
||||
t.Fatalf("unexpected effort callback parse: effort=%q ok=%v", effort, ok)
|
||||
}
|
||||
sandbox, ok := ParseSandboxCallbackData(SandboxCallbackData("workspace-write"))
|
||||
if !ok || sandbox != "workspace-write" {
|
||||
t.Fatalf("unexpected sandbox callback parse: sandbox=%q ok=%v", sandbox, ok)
|
||||
}
|
||||
if _, ok := ParseSandboxCallbackData(SandboxCallbackData("bad")); ok {
|
||||
t.Fatal("bad sandbox callback should not parse")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user