-
Notifications
You must be signed in to change notification settings - Fork 78
fix: Rename tool name, sent notifications/tools/list_changed
#37
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
MQ37
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.
I think we should handle the case where the tool name > 64 chars, see my comments. Otherwise good job 👍
src/examples/clientSse.ts
Outdated
| const SERVER_URL = 'https://actors-mcp-server.apify.actor/sse'; | ||
| // We need to change forward slash / to underscore -- in the tool name as Anthropic does not allow forward slashes in the tool name | ||
| const SELECTED_TOOL = 'apify--rag-web-browser'; | ||
| const SELECTED_TOOL = 'apify-slash-rag-web-browser'; |
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 use the actorNameToToolName('apify/rag-web-browser')?
src/examples/clientStdio.ts
Outdated
|
|
||
| const TOOLS = 'apify/rag-web-browser,lukaskrivka/google-maps-with-contact-details'; | ||
| const SELECTED_TOOL = 'apify--rag-web-browser'; // We need to change / to _ in the tool name | ||
| const SELECTED_TOOL = 'apify-slash-rag-web-browser'; |
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.
same here
src/actors.ts
Outdated
| return actorName.replace('/', '--'); | ||
| return actorName | ||
| .replace(/\//g, '-slash-') | ||
| .replace(/\./g, '-dot-'); |
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.
what if the tool name is longer that 64 chars after the replace? Based on errors I got from working on 5ire.app MCP supports 64 chars at max. We should maybe use hash of the whole Actor name, slice to the say 60 chars and last 4 replace with the chars from hex digest of the hash.
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.
Yeah, the problem I was trying to avoid is to have this lookup
const tool = Array.from(this.tools.values()).find((t) => t.name === name || t.actorFullName === name);
I faced issue with tool names vs Actor names
But I tested it now, with correct tool description it is working fine
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've decided to ignore name collision as of now. I don't think it will happen
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.
sure, we can ignore that for now 👍
Closes apify/tester-mcp-client#11 apify/tester-mcp-client#12