-
Notifications
You must be signed in to change notification settings - Fork 5
Style comparison #8
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
Conversation
| private async ensurePublicToken(providedToken?: string): Promise<string> { | ||
| // If no token provided, try to get one from the account | ||
| if (!providedToken) { | ||
| const fetchedToken = await this.fetchPublicToken(); |
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’m wondering if we really need to fetch a token here, since that would increase this tool’s complexity. If the client already knows we have token tools, and calling this tool requires a token, won’t the agent try to obtain the token by itself? If so, we probably don’t need this fallback solution. Another drawback is that if we make changes to the token tool, it’s quite easy to forget to update this part accordingly.
What do you think?
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.
When I was testing this tool, the challenge was that the model would always try to call this tool with the configured MAPBOX_ACCESS_TOKEN, which is a secret token. If we don't have this logic in the tool, this is what would usually happen:
- prompt "compare minimalistic and halloween theme"
-> calls tool with the configuredMAPBOX_ACCESS_TOKEN, which is ansk.token - open the URL -> fail to load GL JS map, because
sk.token is not allowed in GL JS map
I think for the same reason we have this kind of logic in preview style tool:
| const tokensResult = await listTokensTool.run({ |
It's a dependency, just to avoid the model choosing a secret token instead of public one.
But in this situation, we are producing a URL, and the model doesn't know what happens when we open the URL. The user would have to prompt again "hey create another url using public token"
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.
Maybe we can try to describe the input in the schema, to always use a .pk token. I will see if that works.
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.
|
|
||
| // If it's a secret token, try to get a public token instead | ||
| if (providedToken.startsWith('sk.')) { | ||
| const publicToken = await this.fetchPublicToken(); |
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.
🤔 When this tool is called with a private token, it switches the private token to a public token internally, and the caller won’t notice. Is that really the design we want?
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.
This is related to my comment above. .sk token will never work with this tool, because GL JS map will fail to load with a secret token. It is just a guard so that the model doesn't run in to errors
| params.append('before', beforeStyleId); | ||
| params.append('after', afterStyleId); | ||
|
|
||
| const url = `https://agent.mapbox.com/tools/style-compare?${params.toString()}`; |
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.
[Nit] A demo url is, access_token=pk.xxx&before=mapbox/streets-v11&after=mapbox/outdoors-v12#10/37.7/-122.4 we could also consider to add zoom(i.e. 10 in this case), latitude, latitude as an optional parameter for this tool

Description
TilesetComparisonTooltoStyleComparisonToolfor better clarityNew Feature: Style Comparison Tool
API Integration:
https://agent.mapbox.com/tools/style-compareendpointSchema Simplification:
Test Updates:
Breaking Changes
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Testing
Checklist
Additional Notes