Conform list tools #90
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a conformance test for the server’s support of Anthropic’s list/tools method while updating the codebase to use camelCase identifiers (e.g., "perPage", "pullNumber", "autoInit", etc.) instead of snake_case. The changes span across multiple files including tests, resource templates, API request builders, and documentation to ensure consistency with the MPC spec.
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/search_test.go | Changed "per_page" to "perPage" in schema property assertions |
| pkg/github/search.go | Updated schema builder from "per_page" to "perPage" |
| pkg/github/repository_resource_test.go | Modified resource template to use "prNumber" instead of "pr_number" |
| pkg/github/repository_resource.go | Adjusted parameter key from "pr_number" to "prNumber" in route handling |
| pkg/github/repositories_test.go | Updated tests to assert camelCase for keys such as "autoInit" and "perPage" |
| pkg/github/repositories.go | Modified parameter naming to use camelCase (e.g., "perPage", "autoInit") |
| pkg/github/pullrequests_test.go | Replaced "pull_number" by "pullNumber" and "commit_id" by "commitId" |
| pkg/github/pullrequests.go | Updated parameter names to use camelCase consistently |
| pkg/github/code_scanning_test.go | Changed "alert_number" to "alertNumber" in tests |
| pkg/github/code_scanning.go | Updated schema key from "alert_number" to "alertNumber" |
| conformance/conformance_test.go | Added new test for tools/list and updated request/response handling |
| README.md | Updated documentation to reflect camelCase keys for parameter names |
Files not reviewed (1)
- go.mod: Language not supported
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
| return k == "description" | ||
| })) | ||
|
|
||
| if diff := cmp.Diff(tool.InputSchema, sup.InputSchema, ignoreDescOpt); diff != "" { |
There was a problem hiding this comment.
The current subset check using cmp.Diff requires an exact match between input schemas. To correctly enforce that the GitHub response is a subset of the Anthropic response, consider modifying this comparison to validate that every key-value pair in the GitHub schema exists in the Anthropic schema, allowing additional properties in the latter.
Copilot uses AI. Check for mistakes.
|
This is really great work - I'm sure it was pretty painstaking, but it's great to see how we stand up - and make it just that bit closer, so hopefully people really can hot swap to our server! ✨ |
williammartin
force-pushed
the
wm/missing-milestone
branch
2 times, most recently
from
254be46 to
135754a
Compare
williammartin
dismissed stale reviews from juruen and SamMorrowDrums
The base branch was changed.
williammartin
force-pushed
the
wm/list-tools-conformance
branch
from
131d9f7 to
69076dc
Compare
williammartin
force-pushed
the
wm/list-tools-conformance
branch
from
69076dc to
e3dc62a
Compare
Page size tool parameter names were changed to `perPage` within github#90 while GitHub API uses `per_page` parameter name. This change fixes overlooked inconsistencies. Follow up on github#90 Follow up on github#129 Fixes github#136 Signed-off-by: Alexander Yastrebov <[email protected]>
Page size tool parameter names were changed to `perPage` within github#90 while GitHub API uses `per_page` parameter name. This change fixes overlooked inconsistencies. Follow up on github#90 Follow up on github#129 Fixes github#136 Signed-off-by: Alexander Yastrebov <[email protected]>
Page size tool parameter names were changed to `perPage` within #90 while GitHub API uses `per_page` parameter name. This change fixes overlooked inconsistencies. Follow up on #90 Follow up on #129 Fixes #136 Signed-off-by: Alexander Yastrebov <[email protected]>
Page size tool parameter names were changed to `perPage` within github#90 while GitHub API uses `per_page` parameter name. This change fixes overlooked inconsistencies. Follow up on github#90 Follow up on github#129 Fixes github#136 Signed-off-by: Alexander Yastrebov <[email protected]>
Description
In this PR you will find a conformance test to check this server against Anthropic's
list/toolsmethod. This is gated by aconformancebuild tag because it's really a point in time test that we would expect to remove later. It intentionally ignores any fields nameddescriptionbecause there were too many minor differences in text.Here is the test output:
You should be able to see the only difference in the schema after applying the stack of PRs this is on top is the addition of a
sortproperty onsearch_code.Reviewer Notes
The stack of PRs this is based on handle the more interesting missing cases, and this one, aside from introducing the test itself adjusts all the casing to conform. Unfortunately, the anthropic implementation is not always consistent see:
per_pagevsperPage, or the occasional snake case likeissue_number.Although this PR gives us a baseline conformance, I would suggest following up to make everything camel case since that is what the JSON RPC and Protocol examples from MPC spec lean towards.
Of all the PRs, this is probably the one I care least about getting merged, though I think the test itself has provided value.