Skip to content

First Asserts tool #105

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

First Asserts tool #105

wants to merge 11 commits into from

Conversation

xujiaxj
Copy link

@xujiaxj xujiaxj commented Apr 21, 2025

Adding the first Asserts tool for an API call. It's not quite important what this tool does. It will probably go through more iterations in the future and we will add more tools. This is just a simple PR as the first step.

Tested with Claude Desktop:

image

@xujiaxj xujiaxj requested a review from a team as a code owner April 21, 2025 21:07
Copy link
Contributor

@ioanarm ioanarm left a comment

Choose a reason for hiding this comment

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

Looks good, a couple of comments on the time format.
I am trying to reproduce using ops and no luck so far.

Maybe adding a cloud test along with a couple unit tests could do it. Happy to help!

@xujiaxj
Copy link
Author

xujiaxj commented Apr 22, 2025

Not sure how to do the cloud test against mcptests stack as it does not have any metric.

tools/asserts.go Outdated
Comment on lines 33 to 34
StartTime int64 `json:"startTime" jsonschema:"description=The start time of the assertion status in unix milliseconds format"`
EndTime int64 `json:"endTime" jsonschema:"description=The end time of the assertion status in unix milliseconds format"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW elsewhere in the server we use time.Time and mention that the timestamp should be in RFC3339 in the description, which tends to work quite well (it's easier for a model to get an RFC3339 timestamp correct than a unix timestamp, I guess)

Copy link
Author

Choose a reason for hiding this comment

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

Oh. If that's the case, I can revert back to use RFC3339 and do the *1000 when calling Asserts API.

@xujiaxj xujiaxj requested review from sd2k and ioanarm April 23, 2025 17:31
@xujiaxj
Copy link
Author

xujiaxj commented Apr 23, 2025

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you un-export all of the structs (other than GetAssertionsParams and GetAssertions) so we keep the tools package clean when imported? 🙏

tools/asserts.go Outdated

type GetAssertionsParams struct {
StartTime int64 `json:"startTime" jsonschema:"description=The start time of the assertion status in RFC3339 format"`
EndTime int64 `json:"endTime" jsonschema:"description=The end time of the assertion status in RFC3339 format"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm I don't think the jsonschema description matches the type any more; should the type be time.Time instead so the model can provide an RFC3339 string? (you can convert it to millis using t.UnixMilli)

Copy link
Author

Choose a reason for hiding this comment

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

Eventually the API expects Millis. Which way do you think will work better? Ask the LLM to come up with Millis or use RFC3399 format and convert to Millis in our code?

tools/asserts.go Outdated
Comment on lines 35 to 39
EntityType string `json:"entityType" jsonschema:"description=The type of the entity to list"`
EntityName string `json:"entityName" jsonschema:"description=The name of the entity to list"`
Env string `json:"env" jsonschema:"description=The env of the entity to list"`
Site string `json:"site" jsonschema:"description=The site of the entity to list"`
Namespace string `json:"namespace" jsonschema:"description=The namespace of the entity to list"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

for each of these string types, is the model expected to know the possible options in advance? If it's a closed set (e.g. entity type) we should provide a list of options in the description really; for the others, perhaps we should tell the LLM how to obtain valid values either in the field description or tool description.

Copy link
Author

Choose a reason for hiding this comment

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

We can supply a list of know types. This will help LLM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants