-
Notifications
You must be signed in to change notification settings - Fork 558
Add property binding to tree-agent #25870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
| } | ||
| \`\`\``; | ||
|
|
||
| const helperPropertyExplanation = details.hasHelperProperties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just drop this. We already expose fields on our nodes as simple properties to the LLM. It actually can't even tell the difference between fields and "additional" properties - they both just look like properties. I also expect that if it sees readonly, it will just respect it without any additional attention being called to it.
The reason we want to have special instructions for the methods is because we want to guide the LLM to prefer methods over properties.
| } | ||
|
|
||
| // Add exposed properties directly on object nodes | ||
| for (const [name, zodTypeAny] of getBoundProperties(definition, bindableSchemas)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You updated this file by essentially duplicating the "method"-related code and creating the "property"-version of it. Most of the code seems like it's essentially exactly the same. For example, this loop here is more or less identical to the loop above. There's code above where the same thing is happening - the code is pretty much identical, most of the logic doesn't do anything different for the method case vs. the property case.
Do you think things would fall out more simply if we processed methods and properties together? Instead of doing everything once for getBoundMethods and once again for getBoundProperties, we could have one pass that iterates over both. Or maybe getBoundProperties should just include methods, since methods are technically properties. For the small places where the difference matters, I think we can tell if we're dealing with a property vs a method by checking the type of the class holding the metadata.
I'm not sure how far it can go, but I do get the sense that a lot of the new code in here can be collapsed in this way.
| // TODO: yuck, this entire file has too many statics. we should rewrite it as a generic zod schema walk. | ||
| let detailsI: SchemaDetails = { | ||
| hasHelperMethods: false, | ||
| hasHelperProperties: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these used for?
Description
This PR allows tree-agent to expose properties to the llm.