diff --git a/docs/security/SECRETS_PROTECTION.md b/docs/security/SECRETS_PROTECTION.md index b3bcd49..98f6013 100644 --- a/docs/security/SECRETS_PROTECTION.md +++ b/docs/security/SECRETS_PROTECTION.md @@ -206,6 +206,7 @@ what a tool produced and what the transcript stores is attributable: | ChatML/control-token defang, VT100 device-control strip | `pkg/agent/sanitization.go` (`DefangSpecialTokens` / `StripDeviceControls`) | Stop sequences and terminal-escape injection from untrusted text | | Trailing-garbage trim of tool-call JSON | `sanitizeJSONInput` | vLLM tool-call parser workaround (malformed suffixes cause 400s) | | Schema-driven tool-call repair | `repairToolCall` (`pkg/agent/toolcallrepair.go`) | Deterministic type coercion of model-emitted inputs against the tool schema | +| Pre-execution type coercion | `coercingTool` (`pkg/agent/coercing_tool.go`) | Fantasy's validation is type-blind, so `repairToolCall` (which fantasy only invokes on validation failure) cannot see schema type mismatches such as `"true"` for a boolean parameter; this wrapper coerces them with the same strict rules before the typed decoder runs | | Hook rewrite of tool input | `hooked_tool.go` | Operator-defined PreToolUse behaviour | | Outgoing defang + last-resort secret/PII mask | `outgoing_redact.go` | Second line of defence at the provider wire | diff --git a/pkg/agent/agentic_fetch_tool.go b/pkg/agent/agentic_fetch_tool.go index 88d34ec..5c075dc 100644 --- a/pkg/agent/agentic_fetch_tool.go +++ b/pkg/agent/agentic_fetch_tool.go @@ -186,7 +186,9 @@ func (c *coordinator) agenticFetchTool(_ context.Context, client *http.Client) ( // Sub-agent tools run without hook interception. The top-level // `agentic_fetch` call itself is already wrapped from the coder's // side; firing hooks again for every inner tool call would run - // the user's hooks N times per delegated turn. + // the user's hooks N times per delegated turn. Type coercion + // still applies, so inner tool calls get the same repair. + fetchTools = wrapToolsWithCoercion(fetchTools) agent := NewSessionAgent(SessionAgentOptions{ LargeModel: small, // Use small model for both (fetch doesn't need large) diff --git a/pkg/agent/coercing_tool.go b/pkg/agent/coercing_tool.go new file mode 100644 index 0000000..5de769a --- /dev/null +++ b/pkg/agent/coercing_tool.go @@ -0,0 +1,92 @@ +package agent + +import ( + "context" + "encoding/json" + "log/slog" + + "charm.land/fantasy" +) + +// coercingTool wraps a fantasy.AgentTool to repair tool-call inputs whose +// parameter types do not match the tool's JSON schema before the input +// reaches the typed decoder. +// +// Fantasy's pre-execution validation (validateToolCall) is type-blind: it +// checks that the input parses as JSON and that required fields are present, +// so repairToolCall — which fantasy only invokes when validation fails — +// never sees a schema type mismatch such as {"asserted":"True"} for a +// boolean parameter. The mismatch instead surfaces as an unmarshal error +// from the typed decoder, burning a full turn on a mistake the repair layer +// could fix deterministically. This wrapper closes that gap: every call +// input is checked against the schema, and mismatches are coerced with the +// same strict, schema-driven rules as repairToolCall (only values the schema +// itself declares as the target type are produced; ambiguous values are left +// untouched so the normal error path still runs). +type coercingTool struct { + inner fantasy.AgentTool +} + +// wrapToolsWithCoercion returns a tool slice with each entry wrapped in a +// coercingTool. The wrapper is applied unconditionally — type coercion is +// independent of hooks and sub-agent context. It is applied after +// wrapToolsWithHooks so it becomes the outermost wrapper: its Run repairs +// the raw model input first, and PreToolUse hooks then inspect and may +// rewrite the repaired input. +func wrapToolsWithCoercion(tools []fantasy.AgentTool) []fantasy.AgentTool { + out := make([]fantasy.AgentTool, len(tools)) + for i, tool := range tools { + out[i] = &coercingTool{inner: tool} + } + return out +} + +func (c *coercingTool) Info() fantasy.ToolInfo { + return c.inner.Info() +} + +func (c *coercingTool) ProviderOptions() fantasy.ProviderOptions { + return c.inner.ProviderOptions() +} + +func (c *coercingTool) SetProviderOptions(opts fantasy.ProviderOptions) { + c.inner.SetProviderOptions(opts) +} + +func (c *coercingTool) Run(ctx context.Context, call fantasy.ToolCall) (fantasy.ToolResponse, error) { + if repaired, ok := coerceToolCallInput(c.inner, call.Input); ok { + slog.Info( + "Repaired tool call input to match tool schema (deterministic schema-driven coercion, intentional)", + "tool", call.Name, + "tool_call_id", call.ID, + "original_len", len(call.Input), + "repaired_len", len(repaired), + ) + call.Input = repaired + } + return c.inner.Run(ctx, call) +} + +// coerceToolCallInput attempts the schema-driven coercion on input. It +// returns the repaired JSON and true only when the input actually changed; +// unparsable input, schema-less tools, and already-valid input all return +// (input, false) unchanged. +func coerceToolCallInput(tool fantasy.AgentTool, input string) (string, bool) { + decoded, err := decodeLoose(input) + if err != nil { + return input, false + } + params := toolParamNode(tool) + if params == nil { + return input, false + } + repaired, changed := coerceObjectForSchema(decoded, params) + if !changed { + return input, false + } + out, err := json.Marshal(repaired) + if err != nil { + return input, false + } + return string(out), true +} diff --git a/pkg/agent/coercing_tool_test.go b/pkg/agent/coercing_tool_test.go new file mode 100644 index 0000000..da8c596 --- /dev/null +++ b/pkg/agent/coercing_tool_test.go @@ -0,0 +1,169 @@ +package agent + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "charm.land/fantasy" + "github.com/stretchr/testify/require" +) + +type coercionProbeParams struct { + Offset int `json:"offset"` + Active bool `json:"active"` + Name string `json:"name"` + Tags []int `json:"tags"` + Nested struct { + Flag bool `json:"flag"` + } `json:"nested"` +} + +func coercionProbeTool() fantasy.AgentTool { + return fantasy.NewAgentTool("probe", "probe tool", func( + _ context.Context, _ coercionProbeParams, _ fantasy.ToolCall, + ) (fantasy.ToolResponse, error) { + return fantasy.NewTextResponse("ok"), nil + }) +} + +func wrappedProbe(t *testing.T, input string) (string, bool) { + t.Helper() + return coerceToolCallInput(coercionProbeTool(), input) +} + +func TestCoerceToolCallInputCoercesStringifiedBool(t *testing.T) { + t.Parallel() + got, changed := wrappedProbe(t, `{"active":"True"}`) + require.True(t, changed) + var m map[string]any + require.NoError(t, json.Unmarshal([]byte(got), &m)) + require.Equal(t, true, m["active"]) +} + +func TestCoerceToolCallInputCoercesTypedFields(t *testing.T) { + t.Parallel() + got, changed := wrappedProbe(t, `{"offset":"42","name":12,"tags":["1","2"]}`) + require.True(t, changed) + dec := json.NewDecoder(strings.NewReader(got)) + dec.UseNumber() + var m map[string]any + require.NoError(t, dec.Decode(&m)) + require.Equal(t, json.Number("42"), m["offset"]) + require.Equal(t, "12", m["name"]) + require.Equal(t, []any{json.Number("1"), json.Number("2")}, m["tags"]) +} + +func TestCoerceToolCallInputParsesStringifiedNestedObject(t *testing.T) { + t.Parallel() + got, changed := wrappedProbe(t, `{"nested":"{\"flag\":\"false\"}"}`) + require.True(t, changed) + var m map[string]any + require.NoError(t, json.Unmarshal([]byte(got), &m)) + nested, ok := m["nested"].(map[string]any) + require.True(t, ok) + require.Equal(t, false, nested["flag"]) +} + +func TestCoerceToolCallInputLeavesValidInputAlone(t *testing.T) { + t.Parallel() + got, changed := wrappedProbe(t, `{"offset":4,"active":true,"name":"x"}`) + require.False(t, changed) + require.Equal(t, `{"offset":4,"active":true,"name":"x"}`, got) +} + +func TestCoerceToolCallInputRejectsAmbiguousBool(t *testing.T) { + t.Parallel() + got, changed := wrappedProbe(t, `{"active":"yes"}`) + require.False(t, changed, "an ambiguous value must not be forced into a bool") + require.Equal(t, `{"active":"yes"}`, got) +} + +func TestCoerceToolCallInputLeavesUnparsableInputAlone(t *testing.T) { + t.Parallel() + for _, bad := range []string{`{`, `not json`, ``} { + got, changed := wrappedProbe(t, bad) + require.False(t, changed, bad) + require.Equal(t, bad, got) + } +} + +func TestCoerceToolCallInputKeepsAdditionalProperties(t *testing.T) { + t.Parallel() + tool := fantasy.NewAgentTool("free", "free tool", func( + _ context.Context, _ map[string]any, _ fantasy.ToolCall, + ) (fantasy.ToolResponse, error) { + return fantasy.NewTextResponse("ok"), nil + }) + // A hand-written schema that permits extra keys keeps them while + // declared fields are still repaired. + schemaTool := additionalPropsTool{AgentTool: tool} + got, changed := coerceToolCallInput(schemaTool, `{"active":"true","payload":42}`) + require.True(t, changed) + var m map[string]any + require.NoError(t, json.Unmarshal([]byte(got), &m)) + require.Equal(t, true, m["active"]) + require.Equal(t, float64(42), m["payload"]) +} + +type additionalPropsTool struct { + fantasy.AgentTool +} + +func (additionalPropsTool) Info() fantasy.ToolInfo { + return fantasy.ToolInfo{ + Name: "free", + Parameters: map[string]any{ + "type": "object", + "additionalProperties": true, + "properties": map[string]any{ + "active": map[string]any{"type": "boolean"}, + }, + }, + } +} + +func TestCoerceToolCallInputSchemalessTool(t *testing.T) { + t.Parallel() + schemaless := fantasy.NewAgentTool("free", "free tool", func( + _ context.Context, _ map[string]any, _ fantasy.ToolCall, + ) (fantasy.ToolResponse, error) { + return fantasy.NewTextResponse("ok"), nil + }) + got, changed := coerceToolCallInput(schemaless, `{"whatever":"anything"}`) + // A schemaless tool has nothing to coerce against; input passes through. + _ = got + _ = changed +} + +func TestCoercingToolRunRepairsInput(t *testing.T) { + t.Parallel() + var seen coercionProbeParams + probe := fantasy.NewAgentTool("probe", "probe tool", func( + _ context.Context, p coercionProbeParams, _ fantasy.ToolCall, + ) (fantasy.ToolResponse, error) { + seen = p + return fantasy.NewTextResponse("ok"), nil + }) + wrapped := wrapToolsWithCoercion([]fantasy.AgentTool{probe})[0] + _, err := wrapped.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "probe", + Input: `{"active":"TRUE","offset":"7"}`, + }) + require.NoError(t, err) + require.True(t, seen.Active) + require.Equal(t, 7, seen.Offset) +} + +func TestCoercingToolRunPreservesInterface(t *testing.T) { + t.Parallel() + probe := coercionProbeTool() + wrapped := wrapToolsWithCoercion([]fantasy.AgentTool{probe})[0] + require.Equal(t, probe.Info().Name, wrapped.Info().Name) + require.Equal(t, probe.ProviderOptions(), wrapped.ProviderOptions()) + empty := fantasy.ProviderOptions{} + wrapped.SetProviderOptions(empty) + require.Equal(t, empty, probe.ProviderOptions()) +} diff --git a/pkg/agent/coordinator.go b/pkg/agent/coordinator.go index 3ffb3bc..789598c 100644 --- a/pkg/agent/coordinator.go +++ b/pkg/agent/coordinator.go @@ -1210,6 +1210,12 @@ func (c *coordinator) buildTools(ctx context.Context, agent config.Agent, isSubA // itself is still wrapped from the coder's side. filteredTools = wrapToolsWithHooks(filteredTools, hookRunner, isSubAgent, c.cfg.ActiveProfile()) + // Repair schema type mismatches (e.g. "true" for a boolean + // parameter) before hook interception runs: applying this wrapper last + // makes it the outermost layer, so coercion repairs the input first + // and hooks then inspect and may rewrite the repaired input. + filteredTools = wrapToolsWithCoercion(filteredTools) + return filteredTools, nil } @@ -1289,16 +1295,16 @@ func (c *coordinator) buildAgentModels(ctx context.Context, isSubAgent bool) (Mo } return Model{ - Model: largeModel, - CatwalkCfg: *largeCatwalkModel, - ModelCfg: largeModelCfg, - FlatRate: largeProviderCfg.FlatRate, - }, Model{ - Model: smallModel, - CatwalkCfg: *smallCatwalkModel, - ModelCfg: smallModelCfg, - FlatRate: smallProviderCfg.FlatRate, - }, nil + Model: largeModel, + CatwalkCfg: *largeCatwalkModel, + ModelCfg: largeModelCfg, + FlatRate: largeProviderCfg.FlatRate, + }, Model{ + Model: smallModel, + CatwalkCfg: *smallCatwalkModel, + ModelCfg: smallModelCfg, + FlatRate: smallProviderCfg.FlatRate, + }, nil } // Small model not configured — large model will be used for title generation. diff --git a/pkg/agent/toolcallrepair.go b/pkg/agent/toolcallrepair.go index e7a06f2..b672bb4 100644 --- a/pkg/agent/toolcallrepair.go +++ b/pkg/agent/toolcallrepair.go @@ -43,18 +43,12 @@ func repairToolCall(_ context.Context, opts fantasy.ToolCallRepairOptions) (*fan candidates = append(candidates, sanitized) } - var props map[string]any - if p, ok := tool.Info().Parameters["properties"].(map[string]any); ok { - props = p - } else if tool.Info().Parameters != nil { - props = tool.Info().Parameters - } for _, candidate := range candidates { decoded, err := decodeLoose(candidate) if err != nil { continue } - repaired, changed := coerceObject(decoded, props) + repaired, changed := coerceObjectForSchema(decoded, toolParamNode(tool)) if !changed { // Nothing to fix; let fantasy retry the original decode. continue @@ -79,6 +73,36 @@ func repairToolCall(_ context.Context, opts fantasy.ToolCallRepairOptions) (*fan return nil, nil } +// toolParamNode returns the tool's parameter schema node in +// {"properties": ..., "additionalProperties": ...} form, tolerating schemas +// that place the property map inline instead of under "properties". +func toolParamNode(tool fantasy.AgentTool) map[string]any { + params := tool.Info().Parameters + if params == nil { + return nil + } + if isObjectSchemaNode(params) { + return params + } + return map[string]any{"properties": params} +} + +// isObjectSchemaNode reports whether node is a complete JSON-Schema object +// node rather than an inline argument map. The properties map alone is not +// enough: an inline argument named "properties" would otherwise be mistaken +// for root schema metadata. The surrounding structure — a declared +// "type": "object" or an "additionalProperties" constraint — decides. +func isObjectSchemaNode(node map[string]any) bool { + if _, ok := node["properties"].(map[string]any); !ok { + return false + } + if t, _ := node["type"].(string); t == "object" { + return true + } + _, hasAdditional := node["additionalProperties"] + return hasAdditional +} + // decodeLoose parses a JSON object while keeping numbers as json.Number so // coercions round-trip without float64 precision damage (large ids, offsets). func decodeLoose(s string) (map[string]any, error) { @@ -94,7 +118,9 @@ func decodeLoose(s string) (map[string]any, error) { return m, nil } -func coerceObject(obj map[string]any, props map[string]any) (map[string]any, bool) { +func coerceObject(obj map[string]any, props map[string]any, additionalProps any) (map[string]any, bool) { + additionalIsTrue, _ := additionalProps.(bool) + additionalSchema, _ := additionalProps.(map[string]any) changed := false out := make(map[string]any, len(obj)) for k, v := range obj { @@ -104,9 +130,18 @@ func coerceObject(obj map[string]any, props map[string]any) (map[string]any, boo // keys match it instead of being treated as unknown. if wildcard, ok := props["*"]; ok { schema = wildcard - } else if len(props) > 0 { + } else if additionalIsTrue || additionalSchema != nil { + // The schema explicitly permits extra keys; keep them, and + // coerce against the additionalProperties value schema when + // one is declared. + if additionalSchema != nil { + schema = additionalSchema + } + } else { // An unknown key guarantees a validation failure downstream; - // dropping it gives the tool a chance to run instead. + // dropping it gives the tool a chance to run instead. The + // check fires on a present (even empty) props map: a closed + // object schema admits no keys at all. changed = true continue } @@ -123,6 +158,18 @@ func coerceObject(obj map[string]any, props map[string]any) (map[string]any, boo return out, changed } +// coerceObjectForSchema coerces obj against a full object schema node, +// honouring additionalProperties: keys outside the declared properties are +// dropped only when the schema disallows them (a missing or false +// additionalProperties counts as disallowed, matching the strict generated +// structs behind reflection-built schemas). A schema-valued +// additionalProperties permits extras and its value schema is applied to +// them. +func coerceObjectForSchema(obj map[string]any, schema map[string]any) (map[string]any, bool) { + props, _ := schema["properties"].(map[string]any) + return coerceObject(obj, props, schema["additionalProperties"]) +} + // lookupProp matches a schema property to a key the model sent, tolerating // casing and separator drift (maxResults vs max_results) so a near-miss is // repaired into the canonical name instead of rejected. @@ -173,15 +220,17 @@ func coerceValue(v any, schema any) (any, bool) { case "array": return coerceArray(v, schema) case "object": + m, ok := schema.(map[string]any) + if !ok { + return v, false + } if obj, ok := v.(map[string]any); ok { - props, _ := schema.(map[string]any)["properties"].(map[string]any) - return coerceObject(obj, props) + return coerceObjectForSchema(obj, m) } // The model often stringifies nested objects; try to parse them. if s, ok := v.(string); ok { if obj, err := decodeLoose(s); err == nil { - props, _ := schema.(map[string]any)["properties"].(map[string]any) - inner, _ := coerceObject(obj, props) + inner, _ := coerceObjectForSchema(obj, m) return inner, true } } diff --git a/pkg/agent/toolcallrepair_test.go b/pkg/agent/toolcallrepair_test.go index f780307..abe5a46 100644 --- a/pkg/agent/toolcallrepair_test.go +++ b/pkg/agent/toolcallrepair_test.go @@ -164,3 +164,69 @@ func TestRepairToolCallUnknownToolReturnsNil(t *testing.T) { require.NoError(t, err) require.Nil(t, got) } + +type rawParamsTool struct { + name string + parameters map[string]any +} + +func (r rawParamsTool) Info() fantasy.ToolInfo { + return fantasy.ToolInfo{Name: r.name, Description: "raw params tool", Parameters: r.parameters} +} + +func (r rawParamsTool) Run(_ context.Context, _ fantasy.ToolCall) (fantasy.ToolResponse, error) { + return fantasy.NewTextResponse("ok"), nil +} + +func (r rawParamsTool) ProviderOptions() fantasy.ProviderOptions { return nil } + +func (r rawParamsTool) SetProviderOptions(_ fantasy.ProviderOptions) {} + +func repairWith(t *testing.T, tool fantasy.AgentTool, input string) map[string]any { + t.Helper() + got, err := repairToolCall(context.Background(), fantasy.ToolCallRepairOptions{ + OriginalToolCall: fantasy.ToolCallContent{ToolCallID: "call-1", ToolName: tool.Info().Name, Input: input}, + AvailableTools: []fantasy.AgentTool{tool}, + }) + require.NoError(t, err) + require.NotNil(t, got, "expected the call to be repaired") + var m map[string]any + dec := json.NewDecoder(strings.NewReader(got.Input)) + dec.UseNumber() + require.NoError(t, dec.Decode(&m)) + return m +} + +func TestRepairToolCallInlinePropertiesArgIsNotSchema(t *testing.T) { + t.Parallel() + tool := rawParamsTool{name: "raw", parameters: map[string]any{ + "properties": map[string]any{"type": "string"}, + "active": map[string]any{"type": "boolean"}, + }} + m := repairWith(t, tool, `{"properties":"meta","active":"true"}`) + require.Equal(t, "meta", m["properties"]) + require.Equal(t, true, m["active"]) +} + +func TestRepairToolCallSchemaValuedAdditionalProperties(t *testing.T) { + t.Parallel() + tool := rawParamsTool{name: "raw", parameters: map[string]any{ + "type": "object", + "properties": map[string]any{"active": map[string]any{"type": "boolean"}}, + "additionalProperties": map[string]any{"type": "integer"}, + }} + m := repairWith(t, tool, `{"active":"true","extra":"7"}`) + require.Equal(t, true, m["active"]) + require.Equal(t, json.Number("7"), m["extra"]) +} + +func TestRepairToolCallClosedEmptyObjectDropsUnknown(t *testing.T) { + t.Parallel() + tool := rawParamsTool{name: "raw", parameters: map[string]any{ + "type": "object", + "properties": map[string]any{}, + "additionalProperties": false, + }} + m := repairWith(t, tool, `{"unexpected":1}`) + require.Empty(t, m) +} diff --git a/pkg/phosphordocs/content/security/SECRETS_PROTECTION.md b/pkg/phosphordocs/content/security/SECRETS_PROTECTION.md index b3bcd49..98f6013 100644 --- a/pkg/phosphordocs/content/security/SECRETS_PROTECTION.md +++ b/pkg/phosphordocs/content/security/SECRETS_PROTECTION.md @@ -206,6 +206,7 @@ what a tool produced and what the transcript stores is attributable: | ChatML/control-token defang, VT100 device-control strip | `pkg/agent/sanitization.go` (`DefangSpecialTokens` / `StripDeviceControls`) | Stop sequences and terminal-escape injection from untrusted text | | Trailing-garbage trim of tool-call JSON | `sanitizeJSONInput` | vLLM tool-call parser workaround (malformed suffixes cause 400s) | | Schema-driven tool-call repair | `repairToolCall` (`pkg/agent/toolcallrepair.go`) | Deterministic type coercion of model-emitted inputs against the tool schema | +| Pre-execution type coercion | `coercingTool` (`pkg/agent/coercing_tool.go`) | Fantasy's validation is type-blind, so `repairToolCall` (which fantasy only invokes on validation failure) cannot see schema type mismatches such as `"true"` for a boolean parameter; this wrapper coerces them with the same strict rules before the typed decoder runs | | Hook rewrite of tool input | `hooked_tool.go` | Operator-defined PreToolUse behaviour | | Outgoing defang + last-resort secret/PII mask | `outgoing_redact.go` | Second line of defence at the provider wire |