Repository navigation
feat(skill): Trello MCP skill bundle (Epic 1) + client ergonomics - #84
Merged
Merged
Conversation
…mics
Bundles the Trello MCP skill so it can be installed and consumed as a
self-contained package. Closes Epic 1 (stories 1-1 through 1-4) of the
skill-package sprint.
Epic 1 contents:
- 1-1 Skill directory structure (skill/assets/, skill/references/)
- 1-2 SKILL.md authored as the skill workflow entry point
- 1-3 install.sh hardened for bundle installation; build-skill-assets.sh
added under scripts/ to refresh the bundled source snapshot
- 1-4 README updated to document skill installation and usage
Tooling:
- mise.toml: adds `package` task (refresh skill assets) and `ci` task
(npm test + asset refresh)
- .gitignore: excludes local Codex state and Claude Flow data caches
Client ergonomics (small, ships with this PR):
- src/trello-client.ts: updateChecklistItem now accepts a string state
shortcut (e.g. "complete") in addition to the full update object,
reducing call-site boilerplate for the most common case
Sprint status flipped to done across epic-1 and all four stories;
implementation artifacts (1-1..1-4 story records) committed for
traceability.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the Trello MCP server into a self-contained, BMAD-compatible skill package, adding necessary documentation, asset-bundling scripts, and health monitoring endpoints. It also updates the updateChecklistItem method to support state updates. The review identified several actionable issues: an incorrect rate-limiter calculation, a proxy configuration bypass in attachment downloads, redundant rate-limiter calls, a version mismatch between the server and package.json, unused dead code in validators.ts, and an unused property in health-monitor.ts.
| await this.rateLimiter.waitForAvailableToken(); | ||
| await this.rateLimiter.waitForAvailableToken(); | ||
| const response = await axios.get(downloadUrl, { | ||
| headers: { |
Contributor
There was a problem hiding this comment.
Using the global axios instance here bypasses the proxy configuration established in the constructor (lines 72-78). This will cause attachment downloads to fail in environments requiring an HTTPS proxy. It is recommended to use this.axiosInstance (which handles absolute URLs correctly) or ensure the proxy agent is passed to this request.
|
|
||
| this.server = new McpServer({ | ||
| name: 'trello-server', | ||
| version: '1.0.0', |
Contributor
There was a problem hiding this comment.
The version string here ('1.0.0') does not match the version in package.json ('1.7.1'). It is better to keep these in sync, ideally by importing the version from package.json.
Owner
Author
There was a problem hiding this comment.
@claude Implement a 'version_bump' mise task that is invoked via git tag action
Comment on lines
+1
to
+259
| import { McpError, ErrorCode } from '@modelcontextprotocol/sdk/types.js'; | ||
|
|
||
| export function validateString(value: unknown, field: string): string { | ||
| if (typeof value !== 'string') { | ||
| throw new McpError(ErrorCode.InvalidParams, `${field} must be a string`); | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| export function validateOptionalString(value: unknown): string | undefined { | ||
| if (value === undefined) return undefined; | ||
| return validateString(value, 'value'); | ||
| } | ||
|
|
||
| export function validateNumber(value: unknown, field: string): number { | ||
| if (typeof value !== 'number') { | ||
| throw new McpError(ErrorCode.InvalidParams, `${field} must be a number`); | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| export function validateOptionalNumber(value: unknown): number | undefined { | ||
| if (value === undefined) return undefined; | ||
| return validateNumber(value, 'value'); | ||
| } | ||
|
|
||
| export function validateStringArray(value: unknown): string[] { | ||
| if (!Array.isArray(value) || !value.every(item => typeof item === 'string')) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'Value must be an array of strings'); | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| export function validateOptionalStringArray(value: unknown): string[] | undefined { | ||
| if (value === undefined) return undefined; | ||
| return validateStringArray(value); | ||
| } | ||
|
|
||
| export function validateBoolean(value: unknown, field: string): boolean { | ||
| if (typeof value !== 'boolean') { | ||
| throw new McpError(ErrorCode.InvalidParams, `${field} must be a boolean`); | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| export function validateOptionalBoolean(value: unknown): boolean | undefined { | ||
| if (value === undefined) return undefined; | ||
| return validateBoolean(value, 'value'); | ||
| } | ||
|
|
||
| export function validateOptionalDateString(value: unknown): string | undefined { | ||
| if (value === undefined) return undefined; | ||
| const dateStr = validateString(value, 'date'); | ||
| if (!/^\d{4}-\d{2}-\d{2}$/.test(dateStr)) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'date must be in YYYY-MM-DD format'); | ||
| } | ||
| return dateStr; | ||
| } | ||
|
|
||
| export function validateGetCardsListRequest(args: Record<string, unknown>): { | ||
| boardId?: string; | ||
| listId: string; | ||
| } { | ||
| if (!args.listId) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'listId is required'); | ||
| } | ||
| return { | ||
| boardId: args.boardId ? validateString(args.boardId, 'boardId') : undefined, | ||
| listId: validateString(args.listId, 'listId'), | ||
| }; | ||
| } | ||
|
|
||
| export function validateGetListsRequest(args: Record<string, unknown>): { boardId?: string } { | ||
| return { | ||
| boardId: args.boardId ? validateString(args.boardId, 'boardId') : undefined, | ||
| }; | ||
| } | ||
|
|
||
| export function validateGetRecentActivityRequest(args: Record<string, unknown>): { | ||
| boardId?: string; | ||
| limit?: number; | ||
| } { | ||
| return { | ||
| boardId: args.boardId ? validateString(args.boardId, 'boardId') : undefined, | ||
| limit: validateOptionalNumber(args.limit), | ||
| }; | ||
| } | ||
|
|
||
| export function validateAddCardRequest(args: Record<string, unknown>): { | ||
| boardId?: string; | ||
| listId: string; | ||
| name: string; | ||
| description?: string; | ||
| dueDate?: string; | ||
| start?: string; | ||
| labels?: string[]; | ||
| } { | ||
| if (!args.listId || !args.name) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'listId and name are required'); | ||
| } | ||
|
|
||
| return { | ||
| boardId: args.boardId ? validateString(args.boardId, 'boardId') : undefined, | ||
| listId: validateString(args.listId, 'listId'), | ||
| name: validateString(args.name, 'name'), | ||
| description: validateOptionalString(args.description), | ||
| dueDate: validateOptionalString(args.dueDate), | ||
| start: validateOptionalDateString(args.start), | ||
| labels: validateOptionalStringArray(args.labels), | ||
| }; | ||
| } | ||
|
|
||
| export function validateUpdateCardRequest(args: Record<string, unknown>): { | ||
| boardId?: string; | ||
| cardId: string; | ||
| name?: string; | ||
| description?: string; | ||
| dueDate?: string; | ||
| start?: string; | ||
| dueComplete?: boolean; | ||
| labels?: string[]; | ||
| } { | ||
| if (!args.cardId) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'cardId is required'); | ||
| } | ||
| return { | ||
| boardId: args.boardId ? validateString(args.boardId, 'boardId') : undefined, | ||
| cardId: validateString(args.cardId, 'cardId'), | ||
| name: validateOptionalString(args.name), | ||
| description: validateOptionalString(args.description), | ||
| dueDate: validateOptionalString(args.dueDate), | ||
| start: validateOptionalDateString(args.start), | ||
| dueComplete: validateOptionalBoolean(args.dueComplete), | ||
| labels: validateOptionalStringArray(args.labels), | ||
| }; | ||
| } | ||
|
|
||
| export function validateArchiveCardRequest(args: Record<string, unknown>): { | ||
| boardId?: string; | ||
| cardId: string; | ||
| } { | ||
| if (!args.cardId) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'cardId is required'); | ||
| } | ||
| return { | ||
| boardId: args.boardId ? validateString(args.boardId, 'boardId') : undefined, | ||
| cardId: validateString(args.cardId, 'cardId'), | ||
| }; | ||
| } | ||
|
|
||
| export function validateAddListRequest(args: Record<string, unknown>): { | ||
| boardId?: string; | ||
| name: string; | ||
| } { | ||
| if (!args.name) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'name is required'); | ||
| } | ||
| return { | ||
| boardId: args.boardId ? validateString(args.boardId, 'boardId') : undefined, | ||
| name: validateString(args.name, 'name'), | ||
| }; | ||
| } | ||
|
|
||
| export function validateArchiveListRequest(args: Record<string, unknown>): { | ||
| boardId?: string; | ||
| listId: string; | ||
| } { | ||
| if (!args.listId) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'listId is required'); | ||
| } | ||
| return { | ||
| boardId: args.boardId ? validateString(args.boardId, 'boardId') : undefined, | ||
| listId: validateString(args.listId, 'listId'), | ||
| }; | ||
| } | ||
|
|
||
| export function validateMoveCardRequest(args: Record<string, unknown>): { | ||
| boardId?: string; | ||
| cardId: string; | ||
| listId: string; | ||
| } { | ||
| if (!args.cardId || !args.listId) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'cardId and listId are required'); | ||
| } | ||
| return { | ||
| boardId: args.boardId ? validateString(args.boardId, 'boardId') : undefined, | ||
| cardId: validateString(args.cardId, 'cardId'), | ||
| listId: validateString(args.listId, 'listId'), | ||
| }; | ||
| } | ||
|
|
||
| export function validateAttachImageRequest(args: Record<string, unknown>): { | ||
| boardId?: string; | ||
| cardId: string; | ||
| imageUrl: string; | ||
| name?: string; | ||
| } { | ||
| if (!args.cardId || !args.imageUrl) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'cardId and imageUrl are required'); | ||
| } | ||
|
|
||
| const imageUrl = validateString(args.imageUrl, 'imageUrl'); | ||
| try { | ||
| new URL(imageUrl); | ||
| } catch (e) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'imageUrl must be a valid URL'); | ||
| } | ||
|
|
||
| return { | ||
| boardId: args.boardId ? validateString(args.boardId, 'boardId') : undefined, | ||
| cardId: validateString(args.cardId, 'cardId'), | ||
| imageUrl: imageUrl, | ||
| name: validateOptionalString(args.name), | ||
| }; | ||
| } | ||
|
|
||
| export function validateSetActiveBoardRequest(args: Record<string, unknown>): { boardId: string } { | ||
| if (!args.boardId) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'boardId is required'); | ||
| } | ||
| return { | ||
| boardId: validateString(args.boardId, 'boardId'), | ||
| }; | ||
| } | ||
|
|
||
| export function validateSetActiveWorkspaceRequest(args: Record<string, unknown>): { | ||
| workspaceId: string; | ||
| } { | ||
| if (!args.workspaceId) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'workspaceId is required'); | ||
| } | ||
| return { | ||
| workspaceId: validateString(args.workspaceId, 'workspaceId'), | ||
| }; | ||
| } | ||
|
|
||
| export function validateListBoardsInWorkspaceRequest(args: Record<string, unknown>): { | ||
| workspaceId: string; | ||
| } { | ||
| if (!args.workspaceId) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'workspaceId is required'); | ||
| } | ||
| return { | ||
| workspaceId: validateString(args.workspaceId, 'workspaceId'), | ||
| }; | ||
| } | ||
|
|
||
| export function validateGetCardRequest(args: Record<string, unknown>): { | ||
| cardId: string; | ||
| includeMarkdown?: boolean; | ||
| } { | ||
| if (!args.cardId) { | ||
| throw new McpError(ErrorCode.InvalidParams, 'cardId is required'); | ||
| } | ||
| return { | ||
| cardId: validateString(args.cardId, 'cardId'), | ||
| includeMarkdown: validateOptionalBoolean(args.includeMarkdown), | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
This file appears to be unused dead code. The server implementation in index.ts uses zod for request validation, and no other files in the source bundle import these manual validator functions. Removing it would improve project cleanliness.
| private performanceTracker: PerformanceTracker; | ||
| private lastHealthCheck?: SystemHealthReport; | ||
| private readonly trelloClient: TrelloClient; | ||
| private rateLimiter: any; // Will get injected from TrelloClient |
Contributor
There was a problem hiding this comment.
The rateLimiter property is unused and the comment about injection is not reflected in the current implementation. This property should be removed to avoid confusion.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d6a31241b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mkdir -p "$INSTALL_DIR/build" | ||
| cat >"$BUILD_FILE" <<'EOF' | ||
| #!/usr/bin/env node | ||
| import { spawn } from 'node:child_process'; |
There was a problem hiding this comment.
When Bun is unavailable, the installer writes build/index.js using ESM syntax (import { spawn } ...) but does not create a nearby package.json with "type": "module" (or use .mjs). In environments where node still interprets .js as CommonJS (notably older Node LTS setups), running node .../build/index.js fails before startup with an import-syntax error, so the fallback install path cannot launch the MCP server at all.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
2026-05-07-14:58 - Node Fallback Compatibility Fix
Objective
Ensure the Node.js fallback path for the MCP server successfully launches in environments without Bun by resolving ESM/CommonJS syntax friction.
Strategy
Update the installer to [insert choice: emit .mjs extension OR emit CJS require syntax] to ensure native Node LTS compatibility without requiring a localized package.json.
Acceptance Criteria
- Executing the generated file in a Node LTS environment (v18/v20) successfully initializes the MCP server.
- The process does not throw
SyntaxError: Cannot use import statement outside a module. - The primary Bun execution path remains unaffected.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes “Epic 1” of the skill-package sprint by packaging Trello MCP as a self-contained BMAD-compatible skill bundle (docs, references, bundled source, and install workflow), and includes a small Trello client ergonomics tweak.
Changes:
- Add
skill/bundle structure (SKILL entrypoint, references, bundled server source) and a hardened first-run installer. - Add packaging/CI tasks (
mise run package,mise run ci) and an asset refresh script to rebuildskill/assets/source/. - Extend
updateChecklistItemto accept a bare state string ('complete' | 'incomplete') in addition to an update object.
Reviewed changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/trello-client.ts | Accept checklist item state strings by normalizing to an update object. |
| skill/SKILL.md | Defines skill activation flow, routing, tool grouping, and agent rules. |
| skill/scripts/install.sh | Installs/builds bundled server into user data dir, with registry fallback. |
| skill/references/trello-mcp/README.md | Entry-point router for loading the right reference doc. |
| skill/references/trello-mcp/configuration.md | Install + MCP client configuration guidance for the skill bundle. |
| skill/references/trello-mcp/api.md | Tool-name reference grouped by capability. |
| skill/references/trello-mcp/patterns.md | Suggested operational workflows for common Trello tasks. |
| skill/references/trello-mcp/gotchas.md | Constraints and pitfalls (IDs, dates, rate limits, destructive actions). |
| skill/assets/source/tsconfig.json | Bundled server TypeScript compiler configuration. |
| skill/assets/source/src/validators.ts | Bundled server input validation helpers. |
| skill/assets/source/src/types.ts | Bundled server type definitions (Trello + MCP-facing types). |
| skill/assets/source/src/trello-client.ts | Bundled server Trello client implementation (snapshot). |
| skill/assets/source/src/rate-limiter.ts | Bundled server rate limiter implementation (snapshot). |
| skill/assets/source/src/index.ts | Bundled server MCP tool registration and stdio server entrypoint (snapshot). |
| skill/assets/source/src/health/health-monitor.ts | Bundled health monitor logic (snapshot). |
| skill/assets/source/src/health/health-endpoints.ts | Bundled MCP health tools/endpoints (snapshot). |
| skill/assets/source/src/evals/evals.ts | Bundled eval configuration (snapshot). |
| skill/assets/source/smithery.yaml | Bundled Smithery manifest for registry-based distribution path. |
| skill/assets/source/package.json | Bundled server package manifest used for local install/build. |
| skill/assets/source/LICENSE | Bundled license file included in source snapshot. |
| scripts/build-skill-assets.sh | Script to refresh skill/assets/source/ from repo source. |
| README.md | Documents skill installation/usage and packaging workflow for maintainers. |
| mise.toml | Adds package and ci tasks (refresh assets; tests + refresh). |
| .gitignore | Ignores local Codex/Claude Flow caches. |
| _bmad-output/implementation-artifacts/sprint-status.yaml | Marks epic/stories as done. |
| _bmad-output/implementation-artifacts/1-1-initialize-skill-directory-structure.md | Story record for traceability. |
| _bmad-output/implementation-artifacts/1-2-create-skill-md-workflow.md | Story record for traceability. |
| _bmad-output/implementation-artifacts/1-3-bundle-install-script.md | Story record for traceability. |
| _bmad-output/implementation-artifacts/1-4-update-documentation.md | Story record for traceability. |
Comment on lines
+31
to
+53
| cat >"$BUILD_FILE" <<'EOF' | ||
| #!/usr/bin/env node | ||
| import { spawn } from 'node:child_process'; | ||
|
|
||
| const child = spawn('npx', ['-y', '@delorenj/mcp-server-trello', ...process.argv.slice(2)], { | ||
| stdio: 'inherit', | ||
| env: process.env, | ||
| }); | ||
|
|
||
| child.on('exit', (code, signal) => { | ||
| if (signal) { | ||
| process.kill(process.pid, signal); | ||
| return; | ||
| } | ||
|
|
||
| process.exit(code ?? 1); | ||
| }); | ||
|
|
||
| child.on('error', (error) => { | ||
| console.error(`Failed to start @delorenj/mcp-server-trello via npx: ${error.message}`); | ||
| process.exit(1); | ||
| }); | ||
| EOF |
Contributor
There was a problem hiding this comment.
This feedback is already applied in commit 4e939e2: the fallback wrapper generated by skill/scripts/install.sh now uses CommonJS (require) so node build/index.js works in default Node LTS CommonJS environments. No UI changes were involved.
| } else { | ||
| // Calculate time until next token is available | ||
| const tokensNeeded = 1 - this.tokens; | ||
| const msToWait = (tokensNeeded / this.refillRate) * 1000; |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Owner
Author
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/delorenj/mcp-server-trello/sessions/8ff1268b-914b-4b67-a6e3-f28adaf63f87 Co-authored-by: delorenj <242611+delorenj@users.noreply.github.com>
Contributor
Applied the review-thread fixes in commit
No UI changes were involved. |
- Route downloadAttachment through axiosInstance to honor HTTPS_PROXY configuration and the rate-limiting interceptor (drops the redundant duplicate waitForAvailableToken call that bypassed the proxy via raw axios.get). - Sync server version string to 1.7.1 to match package.json. - Remove unused validators.ts module (no callers in current src/index.ts; superseded by inline zod schemas) and its now-orphaned unit test. - Refresh skill/assets/source bundle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mise package,mise ci) for bundle refresh and a combined test+refresh CI run.updateChecklistItemso a bare state string (e.g."complete") is accepted in addition to the full update object.What's Included
Epic 1 stories
skill/assets/,skill/references/scaffoldedskill/SKILL.mdauthored as the skill entry pointskill/scripts/install.shhardened;scripts/build-skill-assets.shadded to refresh the bundled source snapshotREADME.mdupdated for skill install and usageTooling
mise.toml:packagetask (refresh skill assets),citask (npm test + asset refresh).gitignore: excludes local Codex state and Claude Flow data cachesClient ergonomics (small, included here)
src/trello-client.ts:updateChecklistItemnow accepts a state string shortcut alongside the full update object. Reduces boilerplate for the most common case.Bookkeeping
_bmad-output/implementation-artifacts/sprint-status.yaml: epic-1 and stories 1-1..1-4 flipped todone_bmad-output/implementation-artifacts/1-1..1-4*.md: story records committed for traceabilityTest Plan
mise run packageand confirmskill/assets/source/regenerates cleanlymise run ciend to end (test + asset refresh)skill/scripts/install.shagainst a fresh target directory and verify the skill package installs and SKILL.md is discoverableupdateChecklistItem(cardId, itemId, "complete")string shortcut and confirm it produces the same payload as the object formOut of Scope
🤖 Generated with Claude Code