-
Notifications
You must be signed in to change notification settings - Fork 39
Handle nested field schema formatting for tool description #1001
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
Coverage Report •
|
||||||||||||||||||||
xingyaoww
left a comment
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.
Did you try to migrate stuff from this PR? OpenHands/OpenHands#11497
enyst
left a comment
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.
Thank you for this! LGTM and I'm happy we continue unit testing this converter, it really helps.
I'd suggest maybe we can get first Edward's fix ported from a two-weeks old PR, if that's okay, which seems like a part of this?
|
@OpenHands We just merged a PR doing something like this PR is doing. Try to merge main into this PR branch ht/handle-nested-params-for-tools-desc, and fix the conflicts, then tell me what you think: do you think we need this PR as follow-up? |
|
I'm on it! enyst can track my progress at all-hands.dev |
…nflicts in fn_call_converter and tests - Reconcile schema formatting logic with new recursive formatter - Preserve legacy labels for task_list to satisfy existing tests - Clean up conflict markers and fix imports Co-authored-by: openhands <[email protected]>
|
Final summary What I did
Validation
Checklist (per your instructions)
Answer to “Do we still need this PR as follow-up?” |
… and adapt tests - Drop special-case label for task_list; rely on generic recursive formatter - Update test expectations to match nested array/object formatting Co-authored-by: openhands <[email protected]>
enyst
left a comment
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.
LGTM in general, but please take another look @ryanhoangt , I'm not sure the agent merge didn't miss something, though I checked some. You know best the details here!

Fix #1074
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:f7e54e7-pythonRun
All tags pushed for this build
About Multi-Architecture Support
f7e54e7-python) is a multi-arch manifest supporting both amd64 and arm64f7e54e7-python-amd64) are also available if needed