-
Notifications
You must be signed in to change notification settings - Fork 112
feat: add namespace to MCP tools #1647
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: master
Are you sure you want to change the base?
Conversation
WalkthroughThe changes introduce a new required string property, Changes
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Caution
Changes requested ❌
Reviewed everything up to acd6ea0 in 1 minute and 26 seconds. Click for details.
- Reviewed
144
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
5
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/mcp/server.ts:97
- Draft comment:
Consistently add the namespace 'dbt' to all MCP tools. Ensure that future tool additions also include this property to maintain consistency. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. src/mcp/types.ts:10
- Draft comment:
Added 'namespace' property in McpTool type. This improves clarity for tool categorization. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. src/mcp/types.ts:10
- Draft comment:
Good update: Added an explicit 'namespace' property to the McpTool type. Optionally, consider using a union type for allowed namespace values for stronger type safety. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. src/mcp/server.ts:437
- Draft comment:
Typographical error: 'indivdual' should be corrected to 'individual' in the RUN_TEST tool description. - Reason this comment was not posted:
Comment was on unchanged code.
5. src/mcp/server.ts:506
- Draft comment:
Typographical error: In the INSTALL_DEPS tool description, consider revising "dbt projects's packages.yml file" to "dbt project's packages.yml file" to improve clarity. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_vE4sLKIMtmoVmoCp
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -94,6 +94,7 @@ export class DbtPowerUserMcpServerTools implements Disposable { | |||
|
|||
private tools: McpTool[] = [ | |||
{ | |||
namespace: "dbt", |
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.
Consider extracting the constant dbt
namespace into a shared constant (e.g. DBT_NAMESPACE
) to reduce duplication and avoid potential typos.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mcp/server.ts (1)
95-575
: Consider documenting the purpose of the namespaceThe namespace property has been consistently added to all tools with the value "dbt". However, there's no documentation explaining the purpose or usage of this property.
Consider adding a comment or documentation that explains how the namespace property is intended to be used (e.g., for grouping tools, disambiguation, filtering, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mcp/server.ts
(16 hunks)src/mcp/types.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (windows-latest)
- GitHub Check: test (windows-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
src/mcp/types.ts (1)
9-17
: LGTM: Type definition updated to include namespace propertyThe
McpTool
type definition has been correctly extended with a requirednamespace
string property, which aligns with the PR's objective of adding namespace support to MCP tools.src/mcp/server.ts (15)
95-207
: Namespace property consistently added to the first toolThe "dbt" namespace has been correctly added to the tool definition, maintaining the required structure from the updated type definition.
208-230
: Namespace property correctly added to GET_COLUMNS_OF_MODEL toolThe "dbt" namespace has been appropriately applied to this tool.
231-256
: Namespace property correctly added to GET_COLUMNS_OF_SOURCE toolThe "dbt" namespace has been appropriately applied to this tool.
257-316
: Namespace property added to conditional toolsThe "dbt" namespace has been consistently applied to the tools that are conditionally included based on workspace configuration.
317-337
: Namespace property correctly added to COMPILE_MODEL toolThe "dbt" namespace has been appropriately applied to this tool.
338-361
: Namespace property correctly added to COMPILE_QUERY toolThe "dbt" namespace has been appropriately applied to this tool.
362-386
: Namespace property correctly added to RUN_MODEL toolThe "dbt" namespace has been appropriately applied to this tool.
387-411
: Namespace property correctly added to BUILD_MODEL toolThe "dbt" namespace has been appropriately applied to this tool.
412-432
: Namespace property correctly added to BUILD_PROJECT toolThe "dbt" namespace has been appropriately applied to this tool.
433-455
: Namespace property correctly added to RUN_TEST toolThe "dbt" namespace has been appropriately applied to this tool.
456-478
: Namespace property correctly added to RUN_MODEL_TEST toolThe "dbt" namespace has been appropriately applied to this tool.
479-501
: Namespace property correctly added to ADD_DBT_PACKAGES toolThe "dbt" namespace has been appropriately applied to this tool.
502-524
: Namespace property correctly added to INSTALL_DEPS toolThe "dbt" namespace has been appropriately applied to this tool.
525-549
: Namespace property correctly added to GET_CHILDREN_MODELS toolThe "dbt" namespace has been appropriately applied to this tool.
550-574
: Namespace property correctly added to GET_PARENT_MODELS toolThe "dbt" namespace has been appropriately applied to this tool.
Overview
Problem
Describe the problem you are solving. Mention the ticket/issue if applicable.
Solution
Describe the implemented solution. Add external references if needed.
Screenshot/Demo
A picture is worth a thousand words. Please highlight the changes if applicable.
How to test
Checklist
README.md
updated and added information about my changeImportant
Add
namespace
property toMcpTool
type and update tools inserver.ts
withnamespace: "dbt"
.namespace
property toMcpTool
type intypes.ts
.namespace: "dbt"
to each tool intools
array inserver.ts
.GET_PROJECTS
,GET_COLUMNS_OF_MODEL
,EXECUTE_SQL
, and others.This description was created by
for acd6ea0. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit