-
Notifications
You must be signed in to change notification settings - Fork 3.3k
feat: mcp prompts as slash commands #5451
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
base: dev
Are you sure you want to change the base?
Conversation
|
Yeah ill merge it, maybe have some nits for you but this is good! |
|
/review |
| const is_mcp_prompt = !!mcp_prompts[input.command] | ||
|
|
||
| let template = "" | ||
| if (is_mcp_prompt) { |
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.
Minor style suggestion: The let template = "" followed by the if/else assignment could potentially be refactored using an async IIFE pattern to avoid the let statement (see packages/opencode/src/util/iife.ts). However, this is a minor point and the current approach is acceptable given the complexity of the async logic in each branch.
|
Do we need the dynamic import for mcp? |
|
also I wonder if instead of this pattern we should expose ways to add/remove commands on the command namespace and then mcp can just add slash commands / remove them through that? We kinda have a similar thing for MCP this is me just glancing over but it's just the way the domains overlap so curious your opinion |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
In case we don't have any MCP the slash command will always be a normal command. With the dynamic import we will avoid importing the module all together. Not necessary and I'm willing to refactor but seems like a nice addition |
We could also do that but it would still require custom logic because the template of the prompt is "special". But yeah I guess it's just a matter of who owns what. You definitely have more experience in the codebase so up to you to take the call. Just let me know and I'll refactor it eventually. |
|
@rekram1-node shouln not have trusted the review 😅 I actually removed the snake_case now without type errors |
|
@rekram1-node @paoloricciuti i've tested the original pr against basicmemory and it worked great so far. dependent on an mcp server properly handling prompts in terms of error handling but thats not the problem of the pr implementation. this and getting resources in would be amazing. |
|
@kziemski glad to hear... I'm gonna work on resources immediately after this gets merged |
|
planning to merge today |
|
Btw I think I have the resource PR basically ready... I'll wait anyway, so this doesn't become a huge PR and there will be fewer conflicts. |
Partially addresses #806
This adds every prompt in the mcp servers as a slash command...the slashes are named
mcp.[servername].[promptname]Here's how it looks like using it.
Screen.Recording.2025-12-12.at.19.28.11.mov
I've added a "cache" layer so that prompts are only fetched when an MCP server is connected. The value of the prompt is fetched lazily when the user selects it.
If this get's merged, I'll probably work on resources too.