Skip to content
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

Adding cody mcp #6589

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

Adding cody mcp #6589

wants to merge 2 commits into from

Conversation

arafatkatze
Copy link
Contributor

@arafatkatze arafatkatze commented Jan 10, 2025

Related OpenContext PR

Purposely waiting to do the final steps of the PR AFTER the base merge conflicts from Bee's PR is resolved and a demo is shared and discussed properly.

Test plan

@arafatkatze arafatkatze force-pushed the adding-cody-mcp branch 2 times, most recently from 05f7fc9 to e0bc155 Compare January 11, 2025 17:52
@arafatkatze arafatkatze marked this pull request as ready for review January 14, 2025 16:37
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

I just noticed you are on an old branch so probably unaware of the changes we have on main where we automatically build the tool from openCtx providers via setupOpenCtxProviderListener. In that method we can get the mentions from the MCP and convert them using openCtxProviderMetadata so that we can create the tools for all openCtx providers using the same createOpenCtxTools method.

Let me know if this is confusing or if you have any concerns about this approach!

Comment on lines +249 to +316
public parse(): string[] {
try {
JSON.parse(this.unprocessedText)
} catch {
return []
}
const unparsedText = this.unprocessedText
this.reset()
return [unparsedText]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public parse(): string[] {
try {
JSON.parse(this.unprocessedText)
} catch {
return []
}
const unparsedText = this.unprocessedText
this.reset()
return [unparsedText]
}

As discussed, this is for parsing the sub XML tag where the queries are returned by the LLM through stream. we can move this into execute instead

return [unparsedText]
}

public async execute(span: Span, queries: string[]): Promise<ContextItem[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like the queries param is not being used here. How does it get the items in this case? 👀

Comment on lines +1252 to +1256
"openctx.mcp.enable": {
"type": "boolean",
"markdownDescription": "Enable OpenCtx providers for Cody.",
"default": true
},
"openctx.mcp.uri": {
"type": "string",
"markdownDescription": "URI for the MCP provider tools in OpenCtx.",
"default": ""
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: What's the reason behind having a separated field for these instead of using the existing openctx.providers that handles provider changes?

@@ -235,6 +237,56 @@ class SearchTool extends CodyTool {
}
}

export class ModelContextProviderTool extends CodyTool {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use OpenCtxTool to handle this?

arafatkatze added a commit that referenced this pull request Jan 15, 2025
…ovider (#6650)

- Asynchronously create OpenCtxTools for model context protocol
providers
- Implement `createModelContextConfig` to handle the specific
configuration for model context protocol providers -


## Test plan

<!-- Required. See
https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles.
-->

Needs to test it with the changes in
#6589

---------

Co-authored-by: arafatkatze <arafat.da.khan@gmail.com>
Copy link

‼️ Hey @sourcegraph/cody-security, please review this PR carefully as it introduces the usage of an unsafe_ function or abuses PromptString.

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.

2 participants