-
Notifications
You must be signed in to change notification settings - Fork 9
Devrel/roles, workflows #58
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
4bb8a33 to
742376a
Compare
src/tools/upsert-asset-mapi.ts
Outdated
|
|
||
| export const registerTool = (server: McpServer): void => { | ||
| server.tool( | ||
| "upsert-asset-mapi", |
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.
Honestly, the MAPI contract for assets is kinda weird. Should the MCP client "craft" the internal ID when creating the asset? Just look at the docs...
I would simplify things for the MCP server: one tool for creating and one tool for updating.
| reference: referenceObjectSchema, | ||
| }); | ||
|
|
||
| export const upsertAssetDataSchema = z.object({ |
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 don't think it should belong to "patch" schemas. Move it under the "schemas" folder and name it simply assetSchemas.ts.
src/tools/list-roles-mapi.ts
Outdated
| export const registerTool = (server: McpServer): void => { | ||
| server.tool( | ||
| "list-roles-mapi", | ||
| "Get all Kontent.ai roles from Management API", |
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.
IMHO we don't need to say that we use management API all the time. Actually, most tools don't talk about it at all :)
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 agree we don't, but it's mentioned in kontent-tool-descriptions.mdc:
- API source: Always specify the API being used:
- "from Management API"
- "via Management API"
- "from Delivery API"
I mainly used variant endpoints for reference -- coincidentally, all of them state the API :) might be worth updating the doc and unifying this across the codebase
src/tools/get-role-mapi.ts
Outdated
| export const registerTool = (server: McpServer): void => { | ||
| server.tool( | ||
| "get-role-mapi", | ||
| "Get Kontent.ai role by ID from Management API", |
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.
Again, remove "from Management API".
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.
decided to ditch getRole altogether -- list roles is sufficient and we save a bit of context
src/tools/get-role-mapi.ts
Outdated
| "get-role-mapi", | ||
| "Get Kontent.ai role by ID from Management API", | ||
| { | ||
| identifier: z.string().describe("Role ID (UUID)"), |
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.
IMHO "(UUID)" can be removed.
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.
endpoint removed
src/tools/delete-workflow-mapi.ts
Outdated
| "delete-workflow-mapi", | ||
| "Delete Kontent.ai workflow", | ||
| { | ||
| identifier: z.string().describe("Workflow ID (UUID) to delete"), |
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 would remove "(UUID) to delete". Also, use z.uuid() instead of z.string() and get rid of the description because it's self-explanatory. Also, rename the pro from identifier to id.
src/schemas/workflowSchemas.ts
Outdated
| }); | ||
|
|
||
| // Schema for the list workflows response | ||
| export const listWorkflowsResponseSchema = z |
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.
The "listWorkflowsResponseSchema" is unused. I know that this is not your doing but removing it (and all the related code) would simplify this file. Boy scout rule.
src/schemas/workflowSchemas.ts
Outdated
| }); | ||
|
|
||
| // Workflow scope input schema | ||
| export const workflowScopeInputSchema = z.object({ |
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 (and not just this) should not be needed to export.
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.
removed exports and also removed previous impls of workflows and its subschemas (scopes were outdated and the schemas were not used)
src/schemas/workflowSchemas.ts
Outdated
| .string() | ||
| .uuid() | ||
| .optional() | ||
| .describe("Step ID (required when updating existing steps)"), |
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.
Just a suggestion: I think we should make the schemas as easy to understand as possible. What about splitting create and update schema?
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 don't think it adds any value, but feel free to challenge that. unless I'm missing something, the only difference is ID and scheduled_step, both of which are readonly. we're not using response schemas anywhere, are we?
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.
wait a sec, I might actually rethink that, once I figure out the API reference.
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.
alright, thought about it some more. API reference is a bit confusing, mentioning IDs in POST/PUT request body for workflows, even though you cannot set them. anyway, even though update workflow is a PUT request, pretty much all of the fields are required, so the bodies are identical IMO. I don't see value in splitting them.
src/schemas/workflowSchemas.ts
Outdated
| .describe("Codename for API operations (auto-generated if omitted)"), | ||
| scopes: z | ||
| .array(workflowScopeInputSchema) | ||
| .describe("Array of scopes defining which content types use this workflow"), |
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.
Scopes are currently content type and collection. You mention content types only here. Also, this might change in the future. Please simplify the description. It might be worth it to mention what empty array means?
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 adjusted the description to mention collections too and also mention the behavior of setting both to empty. can't say I simplified anything, I mostly copied the description from the api reference
src/tools/delete-workflow-mapi.ts
Outdated
|
|
||
| return createMcpToolSuccessResponse({ | ||
| message: `Workflow '${identifier}' deleted successfully`, | ||
| deletedWorkflow: response.rawData, |
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.
According to our docs, the response is empty :/
be0edc2 to
d764466
Compare
|
Simplified the schemas by removing redundant fields. This includes removal of ID from workflow step schema, which effectively means the MCP will not have an option to change codename of an existing workflow step (=> nothing to reference the step with). whether or not we want to have this feature in the MCP is for consideration. |
never mind, I added the ID as a readonly optional field for referencing -> codename update now possible |
huluvu21
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 have one final note but the PR can be merged as is :)
d04762c to
8a27ee2
Compare
8a27ee2 to
db4fc9f
Compare
Motivation
Adds support for available roles and workflow MAPI endpoints
Checklist
How to test
If manual testing is required, what are the steps?