From aafead960a28d3a55be8fd982d7302e2450c02fa Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 11 Aug 2026 22:53:12 +0000 Subject: [PATCH 1/2] fix(coderd/x/chatd): reject double-encoded tool arguments with actionable errors Some models occasionally send a structured tool argument as a JSON-encoded string, for example {"files": "[...]"} instead of {"files": [...]}. The Go decoder rejects that with an unmarshal error naming internal Go types, which models struggle to act on. Reject such input during the existing pre-hook schema walk instead: when a property whose advertised schema declares an array or object carries a string, return a retryable tool error telling the model to send the value as JSON directly. Every builtin tool decodes those properties into slices, structs, or maps, so nothing that would have executed is rejected. --- coderd/x/chatd/toolinput.go | 14 ++++++-- coderd/x/chatd/toolinput_internal_test.go | 34 ++++++++++++++++++++ coderd/x/chatd/toolschema/toolschema.go | 33 +++++++++++++++++-- coderd/x/chatd/toolschema/toolschema_test.go | 27 ++++++++++++++-- 4 files changed, 100 insertions(+), 8 deletions(-) diff --git a/coderd/x/chatd/toolinput.go b/coderd/x/chatd/toolinput.go index 4aa1a8df6a8..2e9b441e4cb 100644 --- a/coderd/x/chatd/toolinput.go +++ b/coderd/x/chatd/toolinput.go @@ -2,6 +2,7 @@ package chatd import ( "encoding/json" + "errors" "charm.land/fantasy" "golang.org/x/xerrors" @@ -30,7 +31,7 @@ func partitionAmbiguousToolCalls( continue } if err := validateBuiltinToolInput(prepared, toolCall.ToolName, []byte(toolCall.Input)); err != nil { - rejected = append(rejected, ambiguousToolResult(toolCall, err)) + rejected = append(rejected, invalidInputToolResult(toolCall, err)) continue } allowed = append(allowed, toolCall) @@ -70,7 +71,7 @@ func validateBuiltinToolInput(prepared generationPrepared, toolName string, inpu if info.Name != toolName { continue } - return toolschema.ValidateUnambiguous(info.Parameters, input) + return toolschema.Validate(info.Parameters, input) } return nil } @@ -89,9 +90,16 @@ func malformedToolResult(toolCall fantasy.ToolCallContent) fantasy.ToolResultCon } } -func ambiguousToolResult(toolCall fantasy.ToolCallContent, err error) fantasy.ToolResultContent { +// invalidInputToolResult picks retry advice matching the validation failure, +// because the ambiguous-key advice would misdirect a model that +// double-encoded a structured argument. +func invalidInputToolResult(toolCall fantasy.ToolCallContent, err error) fantasy.ToolResultContent { message := "This tool call was not executed because its input is ambiguous: " + err.Error() + ". Retry with the exact property names from the tool schema, each key used once." + if _, ok := errors.AsType[*toolschema.StringifiedError](err); ok { + message = "This tool call was not executed because " + err.Error() + + ". Retry with the value provided as JSON directly, not wrapped in a string." + } return fantasy.ToolResultContent{ ToolCallID: toolCall.ToolCallID, ToolName: toolCall.ToolName, diff --git a/coderd/x/chatd/toolinput_internal_test.go b/coderd/x/chatd/toolinput_internal_test.go index e19bbe48a9d..87093f4b29d 100644 --- a/coderd/x/chatd/toolinput_internal_test.go +++ b/coderd/x/chatd/toolinput_internal_test.go @@ -90,6 +90,40 @@ func TestPartitionAmbiguousToolCallsGatesOnBuiltins(t *testing.T) { }) } +// TestPartitionStringifiedToolCallInput pins the retry advice for the +// double-encoding failure mode: the ambiguous-key advice would misdirect the +// model, and letting the decoder report it yields an unactionable Go +// unmarshal error. +func TestPartitionStringifiedToolCallInput(t *testing.T) { + t.Parallel() + + type input struct { + URLs []string `json:"urls"` + } + tool := fantasy.NewAgentTool("fetch_many", "", + func(context.Context, input, fantasy.ToolCall) (fantasy.ToolResponse, error) { + return fantasy.ToolResponse{}, nil + }) + prepared := generationPrepared{ + Tools: []fantasy.AgentTool{tool}, + BuiltinToolNames: map[string]bool{"fetch_many": true}, + } + stringified := fantasy.ToolCallContent{ + ToolCallID: "call_stringified", + ToolName: "fetch_many", + Input: `{"urls":"[\"https://example.test\"]"}`, + } + + allowed, rejected := partitionAmbiguousToolCalls(prepared, []fantasy.ToolCallContent{stringified}) + require.Empty(t, allowed) + require.Len(t, rejected, 1) + result, ok := rejected[0].Result.(fantasy.ToolResultOutputContentError) + require.True(t, ok) + require.ErrorContains(t, result.Error, + `input property "urls" is a string, but the schema declares an array`) + require.ErrorContains(t, result.Error, "not wrapped in a string") +} + func TestValidateOverriddenToolInputs(t *testing.T) { t.Parallel() diff --git a/coderd/x/chatd/toolschema/toolschema.go b/coderd/x/chatd/toolschema/toolschema.go index 67f7ea5c2ae..4623376359e 100644 --- a/coderd/x/chatd/toolschema/toolschema.go +++ b/coderd/x/chatd/toolschema/toolschema.go @@ -1,10 +1,12 @@ // Package toolschema rejects tool inputs whose object keys the Go decoder -// and a case-sensitive reader resolve differently. +// and a case-sensitive reader resolve differently, and inputs that +// double-encode a structured property as a string. package toolschema import ( "bytes" "encoding/json" + "fmt" "maps" "slices" "strings" @@ -12,21 +14,38 @@ import ( "golang.org/x/xerrors" ) +// StringifiedError reports a property whose schema declares an array or +// object but whose value is a string, the double-encoding mistake some +// models make. The Go decoder rejects such a value with an unmarshal error +// naming internal Go types, so it is caught here where the message can tell +// the model how to correct the call. +type StringifiedError struct { + Path string + SchemaType string +} + +func (e *StringifiedError) Error() string { + return fmt.Sprintf("input property %q is a string, but the schema declares an %s", e.Path, e.SchemaType) +} + // freeFormPropertyName is the property name fantasy generates for // map[string]T inputs. Its keys are data, so they are checked against the // value schema behind this name rather than against a fixed property set. const freeFormPropertyName = "*" -// ValidateUnambiguous reports an error when input holds an object key that +// Validate reports an error when input holds an object key that // encoding/json folds into a declared property but a case-sensitive reader // treats as distinct, or when one object repeats a key. Either lets code // inspecting the raw input read one value while the tool executes another. +// It also reports a StringifiedError when a declared array or object +// property carries a string, which the tool decoder would reject with a +// less actionable message. // // properties is a fantasy ToolInfo.Parameters map, keyed by property name. // Keys matching no property are ignored because a generated struct decoder // drops them. A tool with a hand-written decoder that reads undeclared keys // has to reject ambiguous spellings of those keys itself. -func ValidateUnambiguous(properties map[string]any, input []byte) error { +func Validate(properties map[string]any, input []byte) error { decoder := json.NewDecoder(bytes.NewReader(input)) token, err := decoder.Token() // Input that does not parse here does not decode for the tool either, @@ -92,6 +111,14 @@ func validateValue(schema map[string]any, decoder *json.Decoder, token json.Toke } } } + if _, isString := token.(string); isString { + // Only string values are flagged: they are how models double-encode + // structures, and other scalar mismatches have not needed a better + // message than the decoder's. + if schemaType, _ := schema["type"].(string); schemaType == "array" || schemaType == "object" { + return &StringifiedError{Path: path, SchemaType: schemaType} + } + } return nil } diff --git a/coderd/x/chatd/toolschema/toolschema_test.go b/coderd/x/chatd/toolschema/toolschema_test.go index ebe599e9111..f84d546c988 100644 --- a/coderd/x/chatd/toolschema/toolschema_test.go +++ b/coderd/x/chatd/toolschema/toolschema_test.go @@ -13,7 +13,7 @@ import ( "github.com/coder/coder/v2/coderd/x/chatd/toolschema" ) -func TestValidateUnambiguous(t *testing.T) { +func TestValidate(t *testing.T) { t.Parallel() readFile := chattool.ReadFile(chattool.ReadFileOptions{}) @@ -78,6 +78,29 @@ func TestValidateUnambiguous(t *testing.T) { tool: readFile, input: `{"path":"/allowed","xyzzy":"b"}`, }, + { + name: "array property double-encoded as a string", + tool: editFiles, + input: `{"files":"[{\"path\":\"foo.go\",\"edits\":[]}]"}`, + wantErr: `input property "files" is a string, but the schema declares an array`, + }, + { + name: "nested array property double-encoded as a string", + tool: editFiles, + input: `{"files":[{"path":"a","edits":"[{\"old_text\":\"x\",\"new_text\":\"y\"}]"}]}`, + wantErr: `input property "files[].edits" is a string, but the schema declares an array`, + }, + { + name: "object property double-encoded as a string", + tool: createWorkspace, + input: `{"template_id":"t","parameters":"{\"foo\":\"1\"}"}`, + wantErr: `input property "parameters" is a string, but the schema declares an object`, + }, + { + name: "non-string scalar for an array property", + tool: editFiles, + input: `{"files":3}`, + }, { name: "exact keys", tool: editFiles, @@ -98,7 +121,7 @@ func TestValidateUnambiguous(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := toolschema.ValidateUnambiguous(tt.tool.Info().Parameters, []byte(tt.input)) + err := toolschema.Validate(tt.tool.Info().Parameters, []byte(tt.input)) if tt.wantErr == "" { require.NoError(t, err) return From f638c7652f8eeacf8f5e39ae37262341a2708cf0 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 11 Aug 2026 22:59:51 +0000 Subject: [PATCH 2/2] fix(coderd/x/chatd): tighten comments from cleanup review --- coderd/x/chatd/toolinput.go | 5 ++--- coderd/x/chatd/toolinput_internal_test.go | 4 ---- coderd/x/chatd/toolschema/toolschema.go | 17 ++++++----------- 3 files changed, 8 insertions(+), 18 deletions(-) diff --git a/coderd/x/chatd/toolinput.go b/coderd/x/chatd/toolinput.go index 2e9b441e4cb..4dbdd3107db 100644 --- a/coderd/x/chatd/toolinput.go +++ b/coderd/x/chatd/toolinput.go @@ -90,9 +90,8 @@ func malformedToolResult(toolCall fantasy.ToolCallContent) fantasy.ToolResultCon } } -// invalidInputToolResult picks retry advice matching the validation failure, -// because the ambiguous-key advice would misdirect a model that -// double-encoded a structured argument. +// Ambiguous-key guidance misdirects models that double-encode structured +// arguments. func invalidInputToolResult(toolCall fantasy.ToolCallContent, err error) fantasy.ToolResultContent { message := "This tool call was not executed because its input is ambiguous: " + err.Error() + ". Retry with the exact property names from the tool schema, each key used once." diff --git a/coderd/x/chatd/toolinput_internal_test.go b/coderd/x/chatd/toolinput_internal_test.go index 87093f4b29d..6b05416fcdd 100644 --- a/coderd/x/chatd/toolinput_internal_test.go +++ b/coderd/x/chatd/toolinput_internal_test.go @@ -90,10 +90,6 @@ func TestPartitionAmbiguousToolCallsGatesOnBuiltins(t *testing.T) { }) } -// TestPartitionStringifiedToolCallInput pins the retry advice for the -// double-encoding failure mode: the ambiguous-key advice would misdirect the -// model, and letting the decoder report it yields an unactionable Go -// unmarshal error. func TestPartitionStringifiedToolCallInput(t *testing.T) { t.Parallel() diff --git a/coderd/x/chatd/toolschema/toolschema.go b/coderd/x/chatd/toolschema/toolschema.go index 4623376359e..3943dda6180 100644 --- a/coderd/x/chatd/toolschema/toolschema.go +++ b/coderd/x/chatd/toolschema/toolschema.go @@ -14,11 +14,8 @@ import ( "golang.org/x/xerrors" ) -// StringifiedError reports a property whose schema declares an array or -// object but whose value is a string, the double-encoding mistake some -// models make. The Go decoder rejects such a value with an unmarshal error -// naming internal Go types, so it is caught here where the message can tell -// the model how to correct the call. +// StringifiedError reports a string value for a property whose schema declares +// an array or object, so callers can provide double-encoding retry advice. type StringifiedError struct { Path string SchemaType string @@ -37,9 +34,8 @@ const freeFormPropertyName = "*" // encoding/json folds into a declared property but a case-sensitive reader // treats as distinct, or when one object repeats a key. Either lets code // inspecting the raw input read one value while the tool executes another. -// It also reports a StringifiedError when a declared array or object -// property carries a string, which the tool decoder would reject with a -// less actionable message. +// It also reports StringifiedError for a string value whose schema declares +// an array or object. // // properties is a fantasy ToolInfo.Parameters map, keyed by property name. // Keys matching no property are ignored because a generated struct decoder @@ -112,9 +108,8 @@ func validateValue(schema map[string]any, decoder *json.Decoder, token json.Toke } } if _, isString := token.(string); isString { - // Only string values are flagged: they are how models double-encode - // structures, and other scalar mismatches have not needed a better - // message than the decoder's. + // String values can be double-encoded structures. Other scalar mismatches + // are left to the tool decoder. if schemaType, _ := schema["type"].(string); schemaType == "array" || schemaType == "object" { return &StringifiedError{Path: path, SchemaType: schemaType} }