toolrunner: validate tool inputs against JSON Schema before handler execution#305
toolrunner: validate tool inputs against JSON Schema before handler execution#305subhashdasyam wants to merge 5 commits intoanthropics:mainfrom
Conversation
…xecution The parse() function claimed to validate inputs against the tool's schema but only called json.Unmarshal. This meant required fields, enum constraints, additionalProperties, pattern, and numeric bounds defined in the schema were never enforced at runtime. This commit adds a zero-dependency schema validator (stdlib only: encoding/json and regexp) that checks inputs before they reach the handler. The schema is parsed once at tool creation time. If validation fails, the handler never runs. Constraints now enforced: - required: missing fields rejected instead of becoming zero values - type: string/number/integer/boolean/array/object checked - enum: values must be in the allowed set - additionalProperties: false rejects unknown keys - pattern: regex matched against string values - minLength/maxLength: string length bounds - minimum/maximum/exclusiveMinimum/exclusiveMaximum: numeric bounds - minItems/maxItems: array length bounds The original JSON bytes are preserved at creation time so fields like additionalProperties survive the BetaToolInputSchemaParam marshal roundtrip. If schema parsing fails for any reason, validation is skipped and behavior falls back to the previous json.Unmarshal-only path. No breaking changes. Includes 5 new test suites (16 total tests pass): - TestToolRunner_SchemaValidation (required, enum, type, empty, optional) - TestToolRunner_AdditionalPropertiesRejected - TestToolRunner_PatternValidation - TestToolRunner_StringLengthValidation - TestToolRunner_NumericBoundsValidation
There was a problem hiding this comment.
Pull request overview
Adds runtime JSON Schema validation to toolrunner tool input parsing so invalid tool arguments returned by the model are rejected before reaching handlers, aligning behavior with the existing documentation and preventing silent zero-value fallthrough.
Changes:
- Introduces a lightweight JSON Schema validator (required, additionalProperties, type, enum, pattern, length/bounds, item counts) and runs it before
json.UnmarshalintoT. - Preserves a raw schema map alongside the typed
BetaToolInputSchemaParamto avoid losing schema fields during marshal/unmarshal roundtrips. - Adds new test suites covering the newly enforced validation constraints and ensuring handlers are not called on invalid input.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
toolrunner/tool.go |
Adds schema validation logic, stores raw schema for validation, and updates constructors to initialize the validator. |
toolrunner/runner_test.go |
Adds test coverage for required fields, enum/type checks, additionalProperties, patterns, string length, and numeric bounds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Validation tests belong in tool_test.go (tests tool.go logic), not runner_test.go (tests runner orchestration loop). Same package, same directory, just the right file.
…ties, enum, regex Fixes 5 issues from code review: 1. newSchemaValidator now recognizes object schemas when type is an array containing "object", or when type is absent but properties, required, or additionalProperties is present. 2. additionalProperties:false now rejects unknown keys even when the properties field is absent entirely (not just empty). 3. Enum comparison uses reflect.DeepEqual instead of fmt.Sprintf, so string "1" no longer matches numeric enum value 1. 4. Invalid regex patterns in schemas now produce a validation error instead of silently passing all inputs. 5. Regex patterns are pre-compiled once at tool creation time and cached in the validator. Compile errors are stored separately and surfaced during validation. New tests cover all 5 fixes: - TestMissingTypeInference (3 subtests) - TestAdditionalPropertiesNoPropsField (2 subtests) - TestEnumCrossTypeMismatch (2 subtests) - TestInvalidRegexPattern
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@stainless-ci-bot and @claude do review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case "integer": | ||
| if f, ok := value.(float64); ok && f == float64(int64(f)) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Integer validation can be bypassed due to float64 rounding during encoding/json unmarshaling (e.g., large numbers with fractional parts can round to an integer), causing non-integer JSON inputs to incorrectly pass the integer check. A more robust approach is to decode validation input using json.Decoder with UseNumber() and validate integers from the original json.Number string representation (or at least guard against precision loss by rejecting values outside the safe integer range).
| func (v *schemaValidator) validateObject(path string, obj map[string]any, schema map[string]any, refStack map[string]bool) error { | ||
| if req, ok := schema["required"].([]any); ok { | ||
| for _, r := range req { | ||
| name, _ := r.(string) | ||
| if name == "" { | ||
| continue | ||
| } | ||
| if _, exists := obj[name]; !exists { | ||
| return fmt.Errorf("missing required property '%s'", joinPath(path, name)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| props, _ := schema["properties"].(map[string]any) | ||
| additional := schema["additionalProperties"] | ||
| for key, val := range obj { | ||
| propPath := joinPath(path, key) |
There was a problem hiding this comment.
Two related issues make validation errors harder to consume/debug: (1) missing-required errors use joinPath(path, name) which yields city at the root rather than consistently prefixing with input like other errors (via displayPath), and (2) iterating for key, val := range obj produces nondeterministic error selection/order when multiple properties are invalid (Go map iteration is randomized). Consider normalizing required error paths (e.g., include displayPath(path) consistently) and iterating object keys in sorted order when validating to produce stable, predictable error messages.
| if variants, ok := schema["oneOf"]; ok { | ||
| if err := v.validateVariants(path, value, variants, true, refStack); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation adds oneOf support, but the new tests only cover anyOf (and not oneOf edge cases). Add tests that assert: (a) exactly one variant matches succeeds, (b) zero matches fails, and (c) multiple matches fails—ideally also verifying the handler is not called when oneOf validation fails.
| // TestSchemaValidation verifies that the tool runner validates inputs | ||
| // against the JSON Schema before executing the handler. This prevents missing | ||
| // required fields, enum violations, and type mismatches from reaching handlers. | ||
| func TestSchemaValidation(t *testing.T) { |
There was a problem hiding this comment.
The PR description lists new test suites named TestToolRunner_SchemaValidation, TestToolRunner_AdditionalPropertiesRejected, etc., but the added tests are named TestSchemaValidation, TestAdditionalPropertiesRejected, etc. Either update the PR description to match the actual test names or rename the tests to match what's documented.
What this fixes
The
parse()function intoolrunner/tool.gohas a comment that says it "validates and parses the input according to the tool's schema." It doesn't. It just callsjson.Unmarshalinto a typed struct.That means the JSON Schema you define for a tool (required fields, enums, patterns, numeric bounds) is sent to the model but never actually checked at runtime. If the model returns bad arguments, whether from a bug, hallucination, or prompt injection, those arguments hit your handler with zero validation. Missing required fields become Go zero values. Enum violations pass through silently.
additionalProperties: falseis ignored entirely.This is a problem because developers read that comment and reasonably assume the SDK is doing what it says. They skip handler-level validation because the SDK told them it already happened.
What this changes
The
parse()method now validates incoming JSON against the tool's schema before callingjson.Unmarshal. If validation fails, the handler never runs and the caller gets a clear error.Constraints now enforced at runtime:
["string", "null"])reflect.DeepEqual)propertiesis empty or absent)utf8.RuneCountInString, per JSON Schema spec)How it works
The schema is parsed into a raw
map[string]anyat tool creation time (not per-request). We keep the original JSON bytes fromNewBetaToolFromBytesandNewBetaToolFromJSONSchemaso that fields likeadditionalPropertiessurvive the roundtrip throughBetaToolInputSchemaParam, which drops them during marshal.At creation time,
prepareSchemawalks the entire schema tree to:regexp.Compileon every request)$ref)Validation runs before
json.Unmarshal, so invalid inputs never reach the handler and never produce misleading zero-value structs.Error handling contract by constructor
The three constructors have different error contracts:
NewBetaToolFromBytes$refNewBetaToolFromJSONSchemaNewBetaToolNewBetaToolFromBytesandNewBetaToolFromJSONSchemaalready returned errors before this change, so surfacing schema definition errors through them is not an API change. However, schemas with previously-ignored issues (e.g., an invalid regex in apatternfield that was never enforced) will now cause construction to fail. This is intentional — these are schema bugs that should be caught early.NewBetaTooldoes not return an error by design (unchanged signature), so it falls back gracefully.What it doesn't change
encoding/json,regexp,unicode/utf8, andreflectfrom the standard library.go.modis untouched.NewBetaTool,NewBetaToolFromBytes,NewBetaToolFromJSONSchema) are unchanged.json.RawMessage,[]byte) skip validation, same as before.Limitations
formatkeyword is not enforced (e.g.,"format": "email"is accepted but not validated).Tests
All existing tests pass. New test suites added:
TestToolRunner_SchemaValidation— required fields, enum, type, empty object, optional omissionTestToolRunner_AdditionalPropertiesRejected— extra keys blocked when schema says noTestToolRunner_PatternValidation— regex enforcement on stringsTestToolRunner_StringLengthValidation— minLength/maxLength boundsTestToolRunner_NumericBoundsValidation— minimum/maximum boundsEach rejection test confirms the handler is never called when validation fails (
handlerCalledflag).The comment fix
Old:
// parse validates and parses the input according to the tool's schema.New: