mcp: don't treat pointers differently given an OutputSchema override #692
Conversation
This comment was marked as spam.
This comment was marked as spam.
In the fix for modelcontextprotocol#199, I was overly cautious: I assumed that if the user provided their own schema, we shouldn't fix up typed nils for them. But this was wrong: consider that: 1. If the user provides a schema, it must be an "object" schema. 2. One way that we recommend customizing schemas is by calling jsonschema.For and modifying. It's very confusing that doing this would change the treatment of the zero value. The good news is that any tool that was returning a typed nil is provably wrong, so this is just a bug that we can and should fix. Fixes modelcontextprotocol#691
findleyr
force-pushed
the
outputschema
branch
from
7dc5160 to
669bd34
Compare
| // | ||
| // TODO(rfindley): we really shouldn't ever return 'null' results. Maybe we | ||
| // should have a jsonschema.Zero(schema) helper? | ||
| func setSchema[T any](sfield *any, rfield **jsonschema.Resolved) (zero any, err error) { |
There was a problem hiding this comment.
Maybe add some more documentation around sfield and rfield, and what exactly this function is doing? E.g., it seems like sfield is an inpput, and rfield is an output, in addition to the 2 return variables. I think it might be beneficial to document the type of sfield since it is any but I think this is an any expected to unmarshall into a valid jsonschema object (cf Tool.OutputSchema).
I am also not familiar with the sfield and rfield names but I could be missing some conventions around this.
I could be missing some context around how this function is used, or conventions around these names.
| if err != nil { | ||
| t.Fatal(err) | ||
| t0 := tools.Tools[0] | ||
| for _, t1 := range tools.Tools[1:] { |
There was a problem hiding this comment.
t1 is a confusing name here since it is index 1 or 2.
tn might be slightly better.
maybe it would be even clearer if we iterated over adjacent pairs in this array, then we could use names like i and j and avoid giving explicit indices.
| // should have a jsonschema.Zero(schema) helper? | ||
| func setSchema[T any](sfield *any, rfield **jsonschema.Resolved) (zero any, err error) { | ||
| rt := reflect.TypeFor[T]() | ||
| if rt.Kind() == reflect.Pointer { |
There was a problem hiding this comment.
I'm on the fence, should we put a comment here explaining the importance of setting rt and zero when sfield != nil? As in, the change in this PR is (I think) making it so we never return a typed nil for pointer elements an rather return a element-typed zero. Is there a good reference as to why we should never return typed nils? From the comment in the PR it sounds like this is always a bug to return typed nils, but why?
| if rt.Kind() == reflect.Pointer { | ||
| rt = rt.Elem() | ||
| zero = reflect.Zero(rt).Interface() | ||
| } |
There was a problem hiding this comment.
Somewhat related to my previous comment, should we check if our handling of zero/nil for pointer types is "interfering" with the user provided schema? It sounds like actually the user should never be including the ability to return typed nils in their schema, so maybe this is something we can check at validation time?
its also OK if the answer is "no" and this is too much added complexity, I just wanted to double check if there's any corner cases we could easily identify and flag for the user to make their UX better
jba
left a comment
jba
left a comment
There was a problem hiding this comment.
Address Markus's comments before merging.
4 participants
In the fix for #199, I was overly cautious: I assumed that if the user
provided their own schema, we shouldn't fix up typed nils for them.
But this was wrong: consider that:
jsonschema.For and modifying. It's very confusing that doing
this would change the treatment of the zero value.
The good news is that any tool that was returning a typed nil is
provably wrong, so this is just a bug that we can and should fix.
Fixes #691