diff --git a/coderd/x/chatd/toolinput.go b/coderd/x/chatd/toolinput.go index 4aa1a8df6a8..4dbdd3107db 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,15 @@ func malformedToolResult(toolCall fantasy.ToolCallContent) fantasy.ToolResultCon } } -func ambiguousToolResult(toolCall fantasy.ToolCallContent, err error) fantasy.ToolResultContent { +// 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." + 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..6b05416fcdd 100644 --- a/coderd/x/chatd/toolinput_internal_test.go +++ b/coderd/x/chatd/toolinput_internal_test.go @@ -90,6 +90,36 @@ func TestPartitionAmbiguousToolCallsGatesOnBuiltins(t *testing.T) { }) } +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..3943dda6180 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,34 @@ import ( "golang.org/x/xerrors" ) +// 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 +} + +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 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 // 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 +107,13 @@ func validateValue(schema map[string]any, decoder *json.Decoder, token json.Toke } } } + if _, isString := token.(string); isString { + // 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} + } + } 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