Skip to content

proposal #1

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.

Sign up for GitHub

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

Jump to bottom
Closed
eyurtsev wants to merge 1 commit into from
Closed

proposal #1

eyurtsev wants to merge 1 commit into from

Conversation

@eyurtsev
Copy link

@eyurtsev eyurtsev commented Mar 4, 2025 •
edited
Loading

I added some inline comments / questions for the existing APIs.

The main things:

  1. What is the tool discovery API?
  2. Could we separate the request and response for tool creation?
  3. Potentially use JSONSchema for inputs / outputs on tool definition?
  4. Mark things as explicitly required to cut down implementation surface

nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
RFC.md
- **Toolkit Information:**

[//]: # (On the request side, could the client specify a toolkit ID/name to use?)
[//]: # (On the request side, could the toolkit ID be optional? With server interpreting this some default toolkit?)
Copy link
Contributor

@nbarbettini nbarbettini Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, we are thinking of keeping the toolkit ID required, because it reduces the complexity of implementing a server. (no need to build a heuristic to determine what belongs to the default toolkit)

nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
nbarbettini
nbarbettini reviewed Mar 5, 2025
View reviewed changes
@nbarbettini
Copy link
Contributor

nbarbettini commented Mar 5, 2025

Thanks for all of your detailed comments @eyurtsev. This PR addresses them: #2

@nbarbettini
Copy link
Contributor

nbarbettini commented Mar 12, 2025

Thanks again @eyurtsev! All of this feedback has been incorporated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nbarbettini nbarbettini nbarbettini left review comments

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@eyurtsev @nbarbettini