From bcc3d447a188114a19c7f900be013e578b95d3c9 Mon Sep 17 00:00:00 2001 From: David Siewert Date: Sat, 25 Apr 2026 20:46:16 +0600 Subject: [PATCH 1/8] feat(agent): add pretty_print and disable_escape_html options for tool feedback - Add PrettyPrint and DisableEscapeHTML config options to ToolFeedbackConfig - Add FormatArgsJSON helper function with configurable pretty printing and HTML escaping - Add toolFeedbackArgsPreviewWithOptions to pass formatting options - Update pipeline_execute.go to use new formatting options for tool feedback This fixes the issue where '&&' would be displayed as '\u0026' in tool feedback messages and provides optional pretty-printing for better readability. --- pkg/agent/agent_utils.go | 9 +++++++++ pkg/agent/pipeline_execute.go | 4 ++-- pkg/config/config.go | 8 +++++--- pkg/config/defaults.go | 8 +++++--- pkg/utils/tool_feedback.go | 17 +++++++++++++++++ 5 files changed, 38 insertions(+), 8 deletions(-) diff --git a/pkg/agent/agent_utils.go b/pkg/agent/agent_utils.go index 4ba75cde4..90d9f43b9 100644 --- a/pkg/agent/agent_utils.go +++ b/pkg/agent/agent_utils.go @@ -184,6 +184,15 @@ func toolFeedbackArgsPreview(args map[string]any, maxLen int) string { return utils.Truncate(string(argsJSON), maxLen) } +func toolFeedbackArgsPreviewWithOptions(args map[string]any, maxLen int, prettyPrint, disableEscapeHTML bool) string { + if args == nil { + args = map[string]any{} + } + + argsJSON := utils.FormatArgsJSON(args, prettyPrint, disableEscapeHTML) + return utils.Truncate(argsJSON, maxLen) +} + func shouldPublishToolFeedback(cfg *config.Config, ts *turnState) bool { if ts == nil || ts.channel == "" || ts.opts.SuppressToolFeedback { return false diff --git a/pkg/agent/pipeline_execute.go b/pkg/agent/pipeline_execute.go index 0cf3eaa9a..ec3514f10 100644 --- a/pkg/agent/pipeline_execute.go +++ b/pkg/agent/pipeline_execute.go @@ -91,7 +91,7 @@ toolLoop: feedbackMsg := utils.FormatToolFeedbackMessage( toolName, toolFeedbackExplanation, - toolFeedbackArgsPreview(toolArgs, toolFeedbackMaxLen), + toolFeedbackArgsPreviewWithOptions(toolArgs, toolFeedbackMaxLen, al.cfg.Agents.Defaults.ToolFeedback.PrettyPrint, al.cfg.Agents.Defaults.ToolFeedback.DisableEscapeHTML), ) fbCtx, fbCancel := context.WithTimeout(turnCtx, 3*time.Second) _ = al.bus.PublishOutbound(fbCtx, outboundMessageForTurnWithKind(ts, feedbackMsg, messageKindToolFeedback)) @@ -373,7 +373,7 @@ toolLoop: feedbackMsg := utils.FormatToolFeedbackMessage( toolName, toolFeedbackExplanation, - toolFeedbackArgsPreview(toolArgs, toolFeedbackMaxLen), + toolFeedbackArgsPreviewWithOptions(toolArgs, toolFeedbackMaxLen, al.cfg.Agents.Defaults.ToolFeedback.PrettyPrint, al.cfg.Agents.Defaults.ToolFeedback.DisableEscapeHTML), ) fbCtx, fbCancel := context.WithTimeout(turnCtx, 3*time.Second) _ = al.bus.PublishOutbound(fbCtx, outboundMessageForTurnWithKind(ts, feedbackMsg, messageKindToolFeedback)) diff --git a/pkg/config/config.go b/pkg/config/config.go index 6bb8d3ce6..0cbc6fead 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -247,9 +247,11 @@ type SubTurnConfig struct { } type ToolFeedbackConfig struct { - Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"` - MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` - SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` + Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"` + MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` + SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` + PrettyPrint bool `json:"pretty_print" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_PRETTY_PRINT"` + DisableEscapeHTML bool `json:"disable_escape_html" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_DISABLE_ESCAPE_HTML"` } type AgentDefaults struct { diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index f3aaca7ab..7725b040e 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -35,9 +35,11 @@ func DefaultConfig() *Config { SummarizeTokenPercent: 75, SteeringMode: "one-at-a-time", ToolFeedback: ToolFeedbackConfig{ - Enabled: false, - MaxArgsLength: 300, - SeparateMessages: false, + Enabled: false, + MaxArgsLength: 300, + SeparateMessages: false, + PrettyPrint: true, + DisableEscapeHTML: true, }, SplitOnMarker: false, }, diff --git a/pkg/utils/tool_feedback.go b/pkg/utils/tool_feedback.go index de7cb467e..4e80e57f3 100644 --- a/pkg/utils/tool_feedback.go +++ b/pkg/utils/tool_feedback.go @@ -1,12 +1,29 @@ package utils import ( + "bytes" + "encoding/json" "fmt" "strings" ) const ToolFeedbackContinuationHint = "Continuing the current task." +func FormatArgsJSON(args map[string]any, prettyPrint, disableEscapeHTML bool) string { + var buf bytes.Buffer + enc := json.NewEncoder(&buf) + if prettyPrint { + enc.SetIndent("", " ") + } + if disableEscapeHTML { + enc.SetEscapeHTML(false) + } + if err := enc.Encode(args); err != nil { + return "{}" + } + return strings.TrimSpace(buf.String()) +} + // FormatToolFeedbackMessage renders a tool feedback message for chat channels. // It keeps the tool name on the first line for animation and can include both // a human explanation and the serialized tool arguments in the body. From fc89fea319bb8d8cde70ea488965836edf0caac8 Mon Sep 17 00:00:00 2001 From: David Siewert Date: Sat, 25 Apr 2026 21:14:06 +0600 Subject: [PATCH 2/8] test(utils): add unit tests for FormatArgsJSON Add tests for FormatArgsJSON covering: - Default compact JSON output - Pretty print formatting - HTML escape disabling (preserves &&, <, >) - Combined pretty print and escape disable - Default HTML escaping behavior - Nil args handling --- pkg/utils/tool_feedback_test.go | 100 +++++++++++++++++++++++++++++++- 1 file changed, 99 insertions(+), 1 deletion(-) diff --git a/pkg/utils/tool_feedback_test.go b/pkg/utils/tool_feedback_test.go index c30f53827..5fabd3d83 100644 --- a/pkg/utils/tool_feedback_test.go +++ b/pkg/utils/tool_feedback_test.go @@ -1,6 +1,9 @@ package utils -import "testing" +import ( + "encoding/json" + "testing" +) func TestFormatToolFeedbackMessage(t *testing.T) { got := FormatToolFeedbackMessage( @@ -56,3 +59,98 @@ func TestFitToolFeedbackMessage_TruncatesSingleLineMessage(t *testing.T) { t.Fatalf("FitToolFeedbackMessage() = %q, want %q", got, want) } } + +func TestFormatArgsJSON_Defaults(t *testing.T) { + args := map[string]any{"path": "README.md", "line": 42} + got := FormatArgsJSON(args, false, false) + var gotVal, wantVal any + if err := json.Unmarshal([]byte(got), &gotVal); err != nil { + t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) + } + want := `{"path":"README.md","line":42}` + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("invalid test want JSON: %v", err) + } + if !jsonValEq(gotVal, wantVal) { + t.Fatalf("FormatArgsJSON() = %q, want %q", got, want) + } +} + +func TestFormatArgsJSON_PrettyPrint(t *testing.T) { + args := map[string]any{"path": "README.md", "line": 42} + got := FormatArgsJSON(args, true, false) + var gotVal any + if err := json.Unmarshal([]byte(got), &gotVal); err != nil { + t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) + } + want := `{"path":"README.md","line":42}` + var wantVal any + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("invalid test want JSON: %v", err) + } + if !jsonValEq(gotVal, wantVal) { + t.Fatalf("FormatArgsJSON() prettyPrint = %q, want structure %q", got, want) + } +} + +func TestFormatArgsJSON_DisableEscapeHTML(t *testing.T) { + args := map[string]any{"msg": "a < b && c > d"} + got := FormatArgsJSON(args, false, true) + var gotVal, wantVal any + want := `{"msg":"a < b && c > d"}` + if err := json.Unmarshal([]byte(got), &gotVal); err != nil { + t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) + } + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("invalid test want JSON: %v", err) + } + if !jsonValEq(gotVal, wantVal) { + t.Fatalf("FormatArgsJSON() disableEscapeHTML = %q, want %q", got, want) + } +} + +func TestFormatArgsJSON_PrettyPrintAndDisableEscapeHTML(t *testing.T) { + args := map[string]any{"msg": "a < b && c > d"} + got := FormatArgsJSON(args, true, true) + var gotVal, wantVal any + want := `{"msg":"a < b && c > d"}` + if err := json.Unmarshal([]byte(got), &gotVal); err != nil { + t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) + } + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("invalid test want JSON: %v", err) + } + if !jsonValEq(gotVal, wantVal) { + t.Fatalf("FormatArgsJSON() combined = %q, want %q", got, want) + } +} + +func TestFormatArgsJSON_EscapeHTMLByDefault(t *testing.T) { + args := map[string]any{"msg": "a < b && c > d"} + got := FormatArgsJSON(args, false, false) + var gotVal, wantVal any + want := `{"msg":"a \u003c b \u0026\u0026 c \u003e d"}` + if err := json.Unmarshal([]byte(got), &gotVal); err != nil { + t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) + } + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("invalid test want JSON: %v", err) + } + if !jsonValEq(gotVal, wantVal) { + t.Fatalf("FormatArgsJSON() default escape = %q, want %q", got, want) + } +} + +func TestFormatArgsJSON_NilArgs(t *testing.T) { + got := FormatArgsJSON(nil, false, false) + want := `null` + if got != want { + t.Fatalf("FormatArgsJSON() nil = %q, want %q", got, want) + } +} + +func jsonValEq(a, b any) bool { + aJSON, _ := json.Marshal(a) + bJSON, _ := json.Marshal(b) + return string(aJSON) == string(bJSON) +} From bdaff5cb693f2c5efbc0105c19d9c8459e7dab2c Mon Sep 17 00:00:00 2001 From: David Siewert Date: Sat, 25 Apr 2026 22:27:01 +0600 Subject: [PATCH 3/8] Add pretty_print and disable_escape_html to tool_feedback defaults --- config/config.example.json | 4 +- go.mod | 2 +- pkg/config/config.go | 2 +- pkg/config/defaults.go | 2 +- tool_feedback.go | 86 +++++++++++++++++++++++++++++++ tool_feedback_test.go | 101 +++++++++++++++++++++++++++++++++++++ 6 files changed, 193 insertions(+), 4 deletions(-) create mode 100644 tool_feedback.go create mode 100644 tool_feedback_test.go diff --git a/config/config.example.json b/config/config.example.json index 30460c231..665a6fd11 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -14,7 +14,9 @@ "tool_feedback": { "enabled": false, "max_args_length": 300, - "separate_messages": false + "separate_messages": false, + "pretty_print": true, + "disable_escape_html": true } } }, diff --git a/go.mod b/go.mod index 4afbe9d85..e4326d8ab 100644 --- a/go.mod +++ b/go.mod @@ -122,7 +122,7 @@ require ( github.com/github/copilot-sdk/go v0.2.0 github.com/go-resty/resty/v2 v2.17.1 // indirect github.com/gogo/protobuf v1.3.2 // indirect - github.com/google/jsonschema-go v0.4.2 // indirect + github.com/google/jsonschema-go v0.4.2 github.com/grbit/go-json v0.11.0 // indirect github.com/klauspost/compress v1.18.4 // indirect github.com/klauspost/cpuid/v2 v2.3.0 // indirect diff --git a/pkg/config/config.go b/pkg/config/config.go index 0cbc6fead..d4908aca3 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -251,7 +251,7 @@ type ToolFeedbackConfig struct { MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` PrettyPrint bool `json:"pretty_print" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_PRETTY_PRINT"` - DisableEscapeHTML bool `json:"disable_escape_html" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_DISABLE_ESCAPE_HTML"` + DisableEscapeHTML bool `json:"disable_escape_html" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_DISABLE_ESCAPE_HTML"` } type AgentDefaults struct { diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 7725b040e..32e3cbbe1 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -39,7 +39,7 @@ func DefaultConfig() *Config { MaxArgsLength: 300, SeparateMessages: false, PrettyPrint: true, - DisableEscapeHTML: true, + DisableEscapeHTML: true, }, SplitOnMarker: false, }, diff --git a/tool_feedback.go b/tool_feedback.go new file mode 100644 index 000000000..b8cf3c384 --- /dev/null +++ b/tool_feedback.go @@ -0,0 +1,86 @@ +package utils + +import ( + "bytes" + "encoding/json" + "fmt" + "strings" +) + +const ToolFeedbackContinuationHint = "Continuing the current task." + +func FormatArgsJSON(args map[string]any, prettyPrint, disableEscapeHTML bool) string { + var buf bytes.Buffer + enc := json.NewEncoder(&buf) + if prettyPrint { + enc.SetIndent("", " ") + } + if disableEscapeHTML { + enc.SetEscapeHTML(false) + } + if err := enc.Encode(args); err != nil { + return "{}" + } + return strings.TrimSpace(buf.String()) +} + +func FormatToolFeedbackMessage(toolName, explanation, argsPreview string) string { + toolName = strings.TrimSpace(toolName) + explanation = strings.TrimSpace(explanation) + argsPreview = strings.TrimSpace(argsPreview) + + bodyLines := make([]string, 0, 2) + if explanation != "" { + bodyLines = append(bodyLines, explanation) + } + if argsPreview != "" { + bodyLines = append(bodyLines, "```json\n"+argsPreview+"\n```") + } + body := strings.Join(bodyLines, "\n") + + if toolName == "" { + return body + } + if body == "" { + return fmt.Sprintf("\U0001f527 `%s`", toolName) + } + + return fmt.Sprintf("\U0001f527 `%s`\n%s", toolName, body) +} + +func FitToolFeedbackMessage(content string, maxLen int) string { + content = strings.TrimSpace(content) + if content == "" || maxLen <= 0 { + return "" + } + if len([]rune(content)) <= maxLen { + return content + } + + firstLine, rest, hasRest := strings.Cut(content, "\n") + firstLine = strings.TrimSpace(firstLine) + rest = strings.TrimSpace(rest) + + if !hasRest || rest == "" { + return Truncate(firstLine, maxLen) + } + + if len([]rune(firstLine)) >= maxLen { + return Truncate(firstLine, maxLen) + } + + remaining := maxLen - len([]rune(firstLine)) - 1 + if remaining <= 0 { + return Truncate(firstLine, maxLen) + } + + return firstLine + "\n" + Truncate(rest, remaining) +} + +func Truncate(s string, maxLen int) string { + runes := []rune(s) + if len(runes) <= maxLen { + return s + } + return string(runes[:maxLen]) +} diff --git a/tool_feedback_test.go b/tool_feedback_test.go new file mode 100644 index 000000000..ef76c1506 --- /dev/null +++ b/tool_feedback_test.go @@ -0,0 +1,101 @@ +package utils + +import ( + "encoding/json" + "testing" +) + +func TestFormatArgsJSON_Defaults(t *testing.T) { + args := map[string]any{"path": "README.md", "line": 42} + got := FormatArgsJSON(args, false, false) + var gotVal, wantVal any + if err := json.Unmarshal([]byte(got), &gotVal); err != nil { + t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) + } + want := `{"path":"README.md","line":42}` + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("invalid test want JSON: %v", err) + } + if !jsonValEq(gotVal, wantVal) { + t.Fatalf("FormatArgsJSON() = %q, want %q", got, want) + } +} + +func TestFormatArgsJSON_PrettyPrint(t *testing.T) { + args := map[string]any{"path": "README.md", "line": 42} + got := FormatArgsJSON(args, true, false) + var gotVal any + if err := json.Unmarshal([]byte(got), &gotVal); err != nil { + t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) + } + want := `{"path":"README.md","line":42}` + var wantVal any + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("invalid test want JSON: %v", err) + } + if !jsonValEq(gotVal, wantVal) { + t.Fatalf("FormatArgsJSON() prettyPrint = %q, want structure %q", got, want) + } +} + +func TestFormatArgsJSON_DisableEscapeHTML(t *testing.T) { + args := map[string]any{"msg": "a < b && c > d"} + got := FormatArgsJSON(args, false, true) + var gotVal, wantVal any + want := `{"msg":"a < b && c > d"}` + if err := json.Unmarshal([]byte(got), &gotVal); err != nil { + t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) + } + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("invalid test want JSON: %v", err) + } + if !jsonValEq(gotVal, wantVal) { + t.Fatalf("FormatArgsJSON() disableEscapeHTML = %q, want %q", got, want) + } +} + +func TestFormatArgsJSON_PrettyPrintAndDisableEscapeHTML(t *testing.T) { + args := map[string]any{"msg": "a < b && c > d"} + got := FormatArgsJSON(args, true, true) + var gotVal, wantVal any + want := `{"msg":"a < b && c > d"}` + if err := json.Unmarshal([]byte(got), &gotVal); err != nil { + t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) + } + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("invalid test want JSON: %v", err) + } + if !jsonValEq(gotVal, wantVal) { + t.Fatalf("FormatArgsJSON() combined = %q, want %q", got, want) + } +} + +func TestFormatArgsJSON_EscapeHTMLByDefault(t *testing.T) { + args := map[string]any{"msg": "a < b && c > d"} + got := FormatArgsJSON(args, false, false) + var gotVal, wantVal any + want := `{"msg":"a \u003c b \u0026\u0026 c \u003e d"}` + if err := json.Unmarshal([]byte(got), &gotVal); err != nil { + t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) + } + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("invalid test want JSON: %v", err) + } + if !jsonValEq(gotVal, wantVal) { + t.Fatalf("FormatArgsJSON() default escape = %q, want %q", got, want) + } +} + +func TestFormatArgsJSON_NilArgs(t *testing.T) { + got := FormatArgsJSON(nil, false, false) + want := `null` + if got != want { + t.Fatalf("FormatArgsJSON() nil = %q, want %q", got, want) + } +} + +func jsonValEq(a, b any) bool { + aJSON, _ := json.Marshal(a) + bJSON, _ := json.Marshal(b) + return string(aJSON) == string(bJSON) +} From 9bc702ebaf5570fd10e16b87b2d683d1e2cd93b6 Mon Sep 17 00:00:00 2001 From: David Siewert Date: Sat, 25 Apr 2026 23:09:40 +0600 Subject: [PATCH 4/8] fix test: enable pretty_print in tool feedback test --- pkg/agent/agent_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go index 01657d43a..6e5bc8191 100644 --- a/pkg/agent/agent_test.go +++ b/pkg/agent/agent_test.go @@ -4519,7 +4519,8 @@ func TestRun_PicoToolFeedbackSuppressesDuplicateInterimAssistantContent(t *testi MaxTokens: 4096, MaxToolIterations: 10, ToolFeedback: config.ToolFeedbackConfig{ - Enabled: true, + Enabled: true, + PrettyPrint: true, }, }, }, From 4ddd650be44d6e0ec4724437d0a16c64e6fa2d54 Mon Sep 17 00:00:00 2001 From: David Siewert Date: Sun, 26 Apr 2026 07:06:36 +0600 Subject: [PATCH 5/8] align ToolFeedbackConfig field spacing --- pkg/config/config.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index d4908aca3..b31c2b63d 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -248,10 +248,10 @@ type SubTurnConfig struct { type ToolFeedbackConfig struct { Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"` - MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` - SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` - PrettyPrint bool `json:"pretty_print" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_PRETTY_PRINT"` - DisableEscapeHTML bool `json:"disable_escape_html" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_DISABLE_ESCAPE_HTML"` + MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` + SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` + PrettyPrint bool `json:"pretty_print" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_PRETTY_PRINT"` + DisableEscapeHTML bool `json:"disable_escape_html" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_DISABLE_ESCAPE_HTML"` } type AgentDefaults struct { From 97b1c3efecc2d5f9050566f4e15596263a743ff7 Mon Sep 17 00:00:00 2001 From: David Siewert Date: Mon, 27 Apr 2026 15:42:18 +0600 Subject: [PATCH 6/8] fix duplicate toolFeedbackArgsPreview function declaration --- config/config.example.json | 4 +- pkg/agent/agent_test.go | 3 +- pkg/agent/agent_utils.go | 15 +---- pkg/agent/pipeline_execute.go | 4 +- pkg/config/config.go | 8 +-- pkg/config/defaults.go | 8 +-- tool_feedback.go | 86 ----------------------------- tool_feedback_test.go | 101 ---------------------------------- 8 files changed, 11 insertions(+), 218 deletions(-) delete mode 100644 tool_feedback.go delete mode 100644 tool_feedback_test.go diff --git a/config/config.example.json b/config/config.example.json index 665a6fd11..30460c231 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -14,9 +14,7 @@ "tool_feedback": { "enabled": false, "max_args_length": 300, - "separate_messages": false, - "pretty_print": true, - "disable_escape_html": true + "separate_messages": false } } }, diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go index 6e5bc8191..01657d43a 100644 --- a/pkg/agent/agent_test.go +++ b/pkg/agent/agent_test.go @@ -4519,8 +4519,7 @@ func TestRun_PicoToolFeedbackSuppressesDuplicateInterimAssistantContent(t *testi MaxTokens: 4096, MaxToolIterations: 10, ToolFeedback: config.ToolFeedbackConfig{ - Enabled: true, - PrettyPrint: true, + Enabled: true, }, }, }, diff --git a/pkg/agent/agent_utils.go b/pkg/agent/agent_utils.go index 90d9f43b9..8e42c1493 100644 --- a/pkg/agent/agent_utils.go +++ b/pkg/agent/agent_utils.go @@ -4,7 +4,6 @@ package agent import ( "context" - "encoding/json" "fmt" "maps" "path/filepath" @@ -177,19 +176,7 @@ func toolFeedbackArgsPreview(args map[string]any, maxLen int) string { args = map[string]any{} } - argsJSON, err := json.MarshalIndent(args, "", " ") - if err != nil { - return utils.Truncate(fmt.Sprintf("%v", args), maxLen) - } - return utils.Truncate(string(argsJSON), maxLen) -} - -func toolFeedbackArgsPreviewWithOptions(args map[string]any, maxLen int, prettyPrint, disableEscapeHTML bool) string { - if args == nil { - args = map[string]any{} - } - - argsJSON := utils.FormatArgsJSON(args, prettyPrint, disableEscapeHTML) + argsJSON := utils.FormatArgsJSON(args, true, false) return utils.Truncate(argsJSON, maxLen) } diff --git a/pkg/agent/pipeline_execute.go b/pkg/agent/pipeline_execute.go index ec3514f10..e1be2a74b 100644 --- a/pkg/agent/pipeline_execute.go +++ b/pkg/agent/pipeline_execute.go @@ -91,7 +91,7 @@ toolLoop: feedbackMsg := utils.FormatToolFeedbackMessage( toolName, toolFeedbackExplanation, - toolFeedbackArgsPreviewWithOptions(toolArgs, toolFeedbackMaxLen, al.cfg.Agents.Defaults.ToolFeedback.PrettyPrint, al.cfg.Agents.Defaults.ToolFeedback.DisableEscapeHTML), +toolFeedbackArgsPreview(toolArgs, toolFeedbackMaxLen), ) fbCtx, fbCancel := context.WithTimeout(turnCtx, 3*time.Second) _ = al.bus.PublishOutbound(fbCtx, outboundMessageForTurnWithKind(ts, feedbackMsg, messageKindToolFeedback)) @@ -373,7 +373,7 @@ toolLoop: feedbackMsg := utils.FormatToolFeedbackMessage( toolName, toolFeedbackExplanation, - toolFeedbackArgsPreviewWithOptions(toolArgs, toolFeedbackMaxLen, al.cfg.Agents.Defaults.ToolFeedback.PrettyPrint, al.cfg.Agents.Defaults.ToolFeedback.DisableEscapeHTML), + toolFeedbackArgsPreview(toolArgs, toolFeedbackMaxLen), ) fbCtx, fbCancel := context.WithTimeout(turnCtx, 3*time.Second) _ = al.bus.PublishOutbound(fbCtx, outboundMessageForTurnWithKind(ts, feedbackMsg, messageKindToolFeedback)) diff --git a/pkg/config/config.go b/pkg/config/config.go index b31c2b63d..48a1cccde 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -247,11 +247,9 @@ type SubTurnConfig struct { } type ToolFeedbackConfig struct { - Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"` - MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` - SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` - PrettyPrint bool `json:"pretty_print" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_PRETTY_PRINT"` - DisableEscapeHTML bool `json:"disable_escape_html" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_DISABLE_ESCAPE_HTML"` + Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"` + MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` + SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` } type AgentDefaults struct { diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 32e3cbbe1..f3aaca7ab 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -35,11 +35,9 @@ func DefaultConfig() *Config { SummarizeTokenPercent: 75, SteeringMode: "one-at-a-time", ToolFeedback: ToolFeedbackConfig{ - Enabled: false, - MaxArgsLength: 300, - SeparateMessages: false, - PrettyPrint: true, - DisableEscapeHTML: true, + Enabled: false, + MaxArgsLength: 300, + SeparateMessages: false, }, SplitOnMarker: false, }, diff --git a/tool_feedback.go b/tool_feedback.go deleted file mode 100644 index b8cf3c384..000000000 --- a/tool_feedback.go +++ /dev/null @@ -1,86 +0,0 @@ -package utils - -import ( - "bytes" - "encoding/json" - "fmt" - "strings" -) - -const ToolFeedbackContinuationHint = "Continuing the current task." - -func FormatArgsJSON(args map[string]any, prettyPrint, disableEscapeHTML bool) string { - var buf bytes.Buffer - enc := json.NewEncoder(&buf) - if prettyPrint { - enc.SetIndent("", " ") - } - if disableEscapeHTML { - enc.SetEscapeHTML(false) - } - if err := enc.Encode(args); err != nil { - return "{}" - } - return strings.TrimSpace(buf.String()) -} - -func FormatToolFeedbackMessage(toolName, explanation, argsPreview string) string { - toolName = strings.TrimSpace(toolName) - explanation = strings.TrimSpace(explanation) - argsPreview = strings.TrimSpace(argsPreview) - - bodyLines := make([]string, 0, 2) - if explanation != "" { - bodyLines = append(bodyLines, explanation) - } - if argsPreview != "" { - bodyLines = append(bodyLines, "```json\n"+argsPreview+"\n```") - } - body := strings.Join(bodyLines, "\n") - - if toolName == "" { - return body - } - if body == "" { - return fmt.Sprintf("\U0001f527 `%s`", toolName) - } - - return fmt.Sprintf("\U0001f527 `%s`\n%s", toolName, body) -} - -func FitToolFeedbackMessage(content string, maxLen int) string { - content = strings.TrimSpace(content) - if content == "" || maxLen <= 0 { - return "" - } - if len([]rune(content)) <= maxLen { - return content - } - - firstLine, rest, hasRest := strings.Cut(content, "\n") - firstLine = strings.TrimSpace(firstLine) - rest = strings.TrimSpace(rest) - - if !hasRest || rest == "" { - return Truncate(firstLine, maxLen) - } - - if len([]rune(firstLine)) >= maxLen { - return Truncate(firstLine, maxLen) - } - - remaining := maxLen - len([]rune(firstLine)) - 1 - if remaining <= 0 { - return Truncate(firstLine, maxLen) - } - - return firstLine + "\n" + Truncate(rest, remaining) -} - -func Truncate(s string, maxLen int) string { - runes := []rune(s) - if len(runes) <= maxLen { - return s - } - return string(runes[:maxLen]) -} diff --git a/tool_feedback_test.go b/tool_feedback_test.go deleted file mode 100644 index ef76c1506..000000000 --- a/tool_feedback_test.go +++ /dev/null @@ -1,101 +0,0 @@ -package utils - -import ( - "encoding/json" - "testing" -) - -func TestFormatArgsJSON_Defaults(t *testing.T) { - args := map[string]any{"path": "README.md", "line": 42} - got := FormatArgsJSON(args, false, false) - var gotVal, wantVal any - if err := json.Unmarshal([]byte(got), &gotVal); err != nil { - t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) - } - want := `{"path":"README.md","line":42}` - if err := json.Unmarshal([]byte(want), &wantVal); err != nil { - t.Fatalf("invalid test want JSON: %v", err) - } - if !jsonValEq(gotVal, wantVal) { - t.Fatalf("FormatArgsJSON() = %q, want %q", got, want) - } -} - -func TestFormatArgsJSON_PrettyPrint(t *testing.T) { - args := map[string]any{"path": "README.md", "line": 42} - got := FormatArgsJSON(args, true, false) - var gotVal any - if err := json.Unmarshal([]byte(got), &gotVal); err != nil { - t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) - } - want := `{"path":"README.md","line":42}` - var wantVal any - if err := json.Unmarshal([]byte(want), &wantVal); err != nil { - t.Fatalf("invalid test want JSON: %v", err) - } - if !jsonValEq(gotVal, wantVal) { - t.Fatalf("FormatArgsJSON() prettyPrint = %q, want structure %q", got, want) - } -} - -func TestFormatArgsJSON_DisableEscapeHTML(t *testing.T) { - args := map[string]any{"msg": "a < b && c > d"} - got := FormatArgsJSON(args, false, true) - var gotVal, wantVal any - want := `{"msg":"a < b && c > d"}` - if err := json.Unmarshal([]byte(got), &gotVal); err != nil { - t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) - } - if err := json.Unmarshal([]byte(want), &wantVal); err != nil { - t.Fatalf("invalid test want JSON: %v", err) - } - if !jsonValEq(gotVal, wantVal) { - t.Fatalf("FormatArgsJSON() disableEscapeHTML = %q, want %q", got, want) - } -} - -func TestFormatArgsJSON_PrettyPrintAndDisableEscapeHTML(t *testing.T) { - args := map[string]any{"msg": "a < b && c > d"} - got := FormatArgsJSON(args, true, true) - var gotVal, wantVal any - want := `{"msg":"a < b && c > d"}` - if err := json.Unmarshal([]byte(got), &gotVal); err != nil { - t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) - } - if err := json.Unmarshal([]byte(want), &wantVal); err != nil { - t.Fatalf("invalid test want JSON: %v", err) - } - if !jsonValEq(gotVal, wantVal) { - t.Fatalf("FormatArgsJSON() combined = %q, want %q", got, want) - } -} - -func TestFormatArgsJSON_EscapeHTMLByDefault(t *testing.T) { - args := map[string]any{"msg": "a < b && c > d"} - got := FormatArgsJSON(args, false, false) - var gotVal, wantVal any - want := `{"msg":"a \u003c b \u0026\u0026 c \u003e d"}` - if err := json.Unmarshal([]byte(got), &gotVal); err != nil { - t.Fatalf("FormatArgsJSON() returned invalid JSON: %v", err) - } - if err := json.Unmarshal([]byte(want), &wantVal); err != nil { - t.Fatalf("invalid test want JSON: %v", err) - } - if !jsonValEq(gotVal, wantVal) { - t.Fatalf("FormatArgsJSON() default escape = %q, want %q", got, want) - } -} - -func TestFormatArgsJSON_NilArgs(t *testing.T) { - got := FormatArgsJSON(nil, false, false) - want := `null` - if got != want { - t.Fatalf("FormatArgsJSON() nil = %q, want %q", got, want) - } -} - -func jsonValEq(a, b any) bool { - aJSON, _ := json.Marshal(a) - bJSON, _ := json.Marshal(b) - return string(aJSON) == string(bJSON) -} From 8dca2a13195fc5fc6bb3546914e12061dada4cd0 Mon Sep 17 00:00:00 2001 From: David Siewert Date: Mon, 27 Apr 2026 16:05:10 +0600 Subject: [PATCH 7/8] fix: improve error handling and nil consistency in FormatArgsJSON - Use fmt.Sprintf fallback instead of {} on encoding errors - Normalize nil args to {} in FormatArgsJSON for consistent output - Update tests to expect {} instead of null for nil args Based on PR #2670 review feedback from afjcjsbx --- pkg/agent/agent_utils.go | 4 ---- pkg/agent/pipeline_execute.go | 2 +- pkg/utils/tool_feedback.go | 8 +++++++- pkg/utils/tool_feedback_test.go | 2 +- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg/agent/agent_utils.go b/pkg/agent/agent_utils.go index 8e42c1493..67524b79c 100644 --- a/pkg/agent/agent_utils.go +++ b/pkg/agent/agent_utils.go @@ -172,10 +172,6 @@ func toolFeedbackExplanationFromMessages(messages []providers.Message) string { } func toolFeedbackArgsPreview(args map[string]any, maxLen int) string { - if args == nil { - args = map[string]any{} - } - argsJSON := utils.FormatArgsJSON(args, true, false) return utils.Truncate(argsJSON, maxLen) } diff --git a/pkg/agent/pipeline_execute.go b/pkg/agent/pipeline_execute.go index e1be2a74b..0cf3eaa9a 100644 --- a/pkg/agent/pipeline_execute.go +++ b/pkg/agent/pipeline_execute.go @@ -91,7 +91,7 @@ toolLoop: feedbackMsg := utils.FormatToolFeedbackMessage( toolName, toolFeedbackExplanation, -toolFeedbackArgsPreview(toolArgs, toolFeedbackMaxLen), + toolFeedbackArgsPreview(toolArgs, toolFeedbackMaxLen), ) fbCtx, fbCancel := context.WithTimeout(turnCtx, 3*time.Second) _ = al.bus.PublishOutbound(fbCtx, outboundMessageForTurnWithKind(ts, feedbackMsg, messageKindToolFeedback)) diff --git a/pkg/utils/tool_feedback.go b/pkg/utils/tool_feedback.go index 4e80e57f3..1834d7f78 100644 --- a/pkg/utils/tool_feedback.go +++ b/pkg/utils/tool_feedback.go @@ -10,6 +10,11 @@ import ( const ToolFeedbackContinuationHint = "Continuing the current task." func FormatArgsJSON(args map[string]any, prettyPrint, disableEscapeHTML bool) string { + // Normalize nil to empty map for consistent output + if args == nil { + args = map[string]any{} + } + var buf bytes.Buffer enc := json.NewEncoder(&buf) if prettyPrint { @@ -19,7 +24,8 @@ func FormatArgsJSON(args map[string]any, prettyPrint, disableEscapeHTML bool) st enc.SetEscapeHTML(false) } if err := enc.Encode(args); err != nil { - return "{}" + // Fallback to fmt.Sprintf to preserve visibility of problematic args + return fmt.Sprintf("%v", args) } return strings.TrimSpace(buf.String()) } diff --git a/pkg/utils/tool_feedback_test.go b/pkg/utils/tool_feedback_test.go index 5fabd3d83..da4accce4 100644 --- a/pkg/utils/tool_feedback_test.go +++ b/pkg/utils/tool_feedback_test.go @@ -143,7 +143,7 @@ func TestFormatArgsJSON_EscapeHTMLByDefault(t *testing.T) { func TestFormatArgsJSON_NilArgs(t *testing.T) { got := FormatArgsJSON(nil, false, false) - want := `null` + want := `{}` if got != want { t.Fatalf("FormatArgsJSON() nil = %q, want %q", got, want) } From 38baf1ccd0ef0f357653747d27519476ff18e26b Mon Sep 17 00:00:00 2001 From: David Siewert Date: Mon, 27 Apr 2026 16:30:51 +0600 Subject: [PATCH 8/8] fix(agent): normalize nil args and improve error handling in FormatArgsJSON - Return fmt.Sprintf fallback instead of {} on encoding errors to preserve visibility - Normalize nil to empty map in FormatArgsJSON for consistent output - Remove redundant nil check in toolFeedbackArgsPreview wrapper - Update test expectation: nil args now return {} not null --- pkg/config/config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 48a1cccde..6bb8d3ce6 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -247,8 +247,8 @@ type SubTurnConfig struct { } type ToolFeedbackConfig struct { - Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"` - MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` + Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"` + MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` }