-
Notifications
You must be signed in to change notification settings - Fork 2
@W-20591366 Add Tool Adapter Layer for MCP Tools #29
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
patricksullivansf
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.
love it. it's a bit obtuse to grok all at once, but it seems well reasoned.
| async handler(args) { | ||
| const timestamp = new Date().toISOString(); | ||
| console.error(`[${timestamp}] CARTRIDGES tool '${name}' called with:`, args); | ||
| interface CartridgeDeployInput { |
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.
These interfaces are temporary placeholders and might change once we start implement the tool.
| /** | ||
| * Input type for mrt_bundle_push tool. | ||
| */ | ||
| interface MrtBundlePushInput { |
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.
These interfaces are temporary placeholders and might change once we start implement the tool.
packages/b2c-dx-mcp/bin/run.cmd
Outdated
|
|
||
| node "%~dp0\run" %* | ||
|
|
||
|
|
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.
A new line is added after pnpm build
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.
Overall looks good 👍
My comments are around configuration loading. We should push as much of this as possible into the OCLIF and SDK layers so it's consistent (And not repeated) across all tooling.
We can refactor some of this in followup PRs though. I have one coming now where I've identified 2 places we're loading dw.json in the SDK (which is bad). As mentioned I'd like to add an even simpler abstraction for getting configuration for instance, MRT, etc (for instance I don't want the CLIs to have to load the mobify config files themselves). I can help with that in followup PRs. See if there's anything we can simplify now from my comments below.
| parse: async (input) => input.toLowerCase(), | ||
| }), | ||
|
|
||
| // Auth flags |
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.
Note you can get and spread these flags from ...MrtCommand.baseFlags
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.
@clavery That sounds good! For MCP tools, it might need different API keys, do you consider rename api-key to mrt-api-key?
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.
perhaps. although we do want to maximum compatibility with existing conventions (like pwa-kit-dev, etc) which uses --api-key. I'll think about this. Right now we don't have any auth needs that conflict with api-key (all other auths use distinct naming).
One thing that's important is the env variables are the same (which in this case is unambiguous (SFCC_MRT_API_KEY)). So if you don't want to spread from the base command make sure we're keeping those consistent. The idea is that folks can set their configuration once (.env files, dw.json, etc) and ALL of our tools respond in the same way
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.
@clavery I updated to spread from the command and it works well. One thing to note, if config Cursor MCP with .env instead of "env" attribute in mcp.json, .env needs to be under home directory (~) because Cursor runs MCP servers with cwd = home directory.
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.
Both .env and env attribute in mcp.json will be supported.
Yeah I know about cursors weird behavior there. Here's what I told Shaurye a few months ago: Once we move this MCP out of PWA kit we'll want to use a more generic env var AND we should support this as an argument as well (i.e. --project ${workspaceFolder}). Again falling back to cwd when not specified (edited)
Basically we should default everything to assuming the current working directory is our project but allow for a --project (or similar) flag to root (or change our directory) to. The ${workspaceFolder} expansion works in cursor
I think this might be a useful flag at the BaseCommand too rather than just being an MCP concern.
All that being said if they don't configure it that way then we don't know where the project is and configuration will just load what it can (And many tools may be useful but others useless)
| * Environment variables take precedence over dw.json values. | ||
| * Empty strings are treated as undefined (fall back to dw.json). | ||
| */ | ||
| function getEnvironmentOverrides(): Record<string, string | undefined> { |
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.
Why is this function needed? OCLIF supports and resolves environment variables for flags automatically. We should be using the flags from InstanceCommand.baseFlags (or at least duplicating them) for compatibility with the CLI.
| function createInstance(services: Services): B2CInstance { | ||
| const envOverrides = getEnvironmentOverrides(); | ||
|
|
||
| return B2CInstance.fromEnvironment({ |
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. In a CLI B2CInstance.fromEnvironment should be seeded with the values obtained from the CLI entry (i.e. flags) which would automatically have the environment variables applied.
packages/b2c-dx-mcp/src/services.ts
Outdated
| } | ||
|
|
||
| // 2. Check environment variable | ||
| const envApiKey = process.env.SFCC_MRT_API_KEY?.trim(); |
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 should come from the OCLIF flags as well.
| } | ||
|
|
||
| // 3. Check ~/.mobify config file (or ~/.mobify--[hostname] if cloud origin specified) | ||
| const mobifyConfig = loadMobifyConfig(options.cloudOrigin); |
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'd like to improve this so you don't have to call this utility function by having a more consistent "configuration" facade. I will work on that in a separate PR though.
Summary
This PR introduces a Tool Adapter layer for the B2C DX MCP server that standardizes how SDK functions are wrapped as MCP tools. The adapter provides a consistent pattern for:
textResult,jsonResult,errorResult)Additionally, MRT authentication is now resolved once at server startup via
Services.create()and reused for all tool calls, improving performance and consistency.Changes
New Files
packages/b2c-dx-mcp/src/tools/adapter.ts- Core tool adapter implementation with:createToolAdapter<TInput, TOutput>()factory functionToolExecutionContextinterface providingb2cInstance,mrtConfig, andservicestextResult(),jsonResult(),errorResult()requiresInstanceflag for B2C instance injection (WebDAV/OCAPI tools)requiresMrtAuthflag for MRT API authentication injectionpackages/b2c-dx-mcp/test/tools/adapter.test.ts- Comprehensive test suite (789 lines) covering:Modified Files
packages/b2c-dx-mcp/src/services.ts- Enhanced with:Services.create()static factory method for MRT auth resolution at startupresolveMrtAuth()method implementing priority: flag → env var → ~/.mobifyresolveB2CInstance()method implementing priority: flags → env vars → dw.jsonmrtConfigproperty storing pre-resolved MRT auth, project, and environmentb2cInstanceproperty storing pre-resolved B2CInstance (may be undefined)packages/b2c-dx-mcp/src/commands/mcp.ts- Updated to useServices.create()with new auth flagspackages/b2c-dx-mcp/src/registry.ts- Switched fromconsole.errorto structured logging viagetLogger()packages/b2c-dx-mcp/src/tools/index.ts- Re-exports adapter moduleTool implementations migrated to adapter pattern:
packages/b2c-dx-mcp/src/tools/cartridges/index.ts-cartridge_deploytoolpackages/b2c-dx-mcp/src/tools/mrt/index.ts-mrt_bundle_pushtoolpackages/b2c-dx-mcp/src/tools/pwav3/index.ts- PWA v3 toolspackages/b2c-dx-mcp/src/tools/scapi/index.ts- SCAPI toolspackages/b2c-dx-mcp/src/tools/storefrontnext/index.ts- Storefront Next toolspackages/b2c-dx-mcp/README.md- Documentation for:MrtCommand.baseFlags:--api-key,--project,--environment,--cloud-originInstanceCommand.baseFlags:--server,--username,--password,--client-id,--client-secret,--code-version~/.mobify--[hostname])packages/b2c-tooling-sdk/src/cli/index.ts- ExportloadMobifyConfigfor MRT config file loadingTest Updates
packages/b2c-dx-mcp/test/commands/mcp.test.ts- Added test stubs for new flagsTest Plan
test-plan.md
Testing Tip:
Prompt Cursor to perform manual test in the test plan:
"Complete ALL tests in Test Plan. Create any required config files (~/.mobify, dw.json), update mcp.json for each test, wait for my restart, verify, and clean up at the end."
pnpm test)pnpm run format)