feat: harden API client, align with mcp-ecosystem standards#15
Merged
Conversation
Introduces a new tool for validating EDD API connection, enforces MCP stdio protocol safety by redirecting console output to stderr, and adds detailed HTTP error diagnostics with actionable hints. Updates manifest and server metadata, normalizes EDD API URLs, and expands tests for error handling and stdio cleanliness. Also adds brand assets and screenshots.
- Customer ID fallback logic (tries customer, id, user_id, user params) - Robust stats date-range parsing with value coercion and range filtering - Discount code normalization across list/get sale responses - Add edd_validate_connection tool to README - 3 new unit tests (26 total, all passing) Standards alignment: - Add eslint-config-prettier integration - Add tsconfig strict flags (noUnusedLocals, noUnusedParameters, noImplicitReturns) - Fix prepublishOnly to run tests before publish - Fix ESLint errors in stdio.ts console patching - Fix manifest.json screenshots to reference actual asset - npm audit fix (0 vulnerabilities) Release infrastructure: - Release-please manifest mode with config files - OIDC Trusted Publishing (no NPM_TOKEN needed, Node 24) - Auto-build .mcpb Desktop Extension on release - Slack announce job Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR enhances error handling with a structured HTTP error type, adds API URL validation/normalization, implements console output redirection for MCP stdio compatibility, introduces a new connection validation tool, and enriches existing tool outputs. The release workflow is updated to use manifest-based versioning with separate build and publish jobs. ESLint and TypeScript configurations are tightened. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MCP as MCP Server
participant Validator as edd_validate_connection
participant Client as EDD Client
participant API as EDD API
User->>MCP: Call edd_validate_connection
MCP->>Validator: Execute validation tool
Validator->>Client: listProducts()
Client->>API: GET /edd-api/products/
API-->>Client: Response (or error)
Client-->>Validator: Products list
Validator->>Client: listSales()
Client->>API: GET /edd-api/sales/
API-->>Client: Response (or error)
Client-->>Validator: Sales list
Validator->>Validator: Compile checks result
Validator-->>MCP: {ok: bool, checks: {...}}
MCP-->>User: Validation status & diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/env.ts (1)
42-79: Consider using Zod for URL validation.The manual URL validation works correctly, but coding guidelines specify using Zod schemas for input validation. This would provide consistent error formatting and integrate with the existing Zod infrastructure.
As per coding guidelines: "Use Zod schemas for input validation of tool parameters" - while this isn't a tool parameter, using Zod for env validation would maintain consistency.
♻️ Optional Zod-based alternative
import { z } from 'zod'; const EddApiUrlSchema = z.string().url().transform((rawUrl) => { const parsedUrl = new URL(rawUrl); if (parsedUrl.protocol !== 'https:' && parsedUrl.protocol !== 'http:') { throw new Error('EDD_API_URL must start with https:// or http://'); } parsedUrl.search = ''; parsedUrl.hash = ''; // ... rest of normalization logic return parsedUrl.toString(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/env.ts` around lines 42 - 79, Replace the manual URL validation in normalizeEddApiUrl with a Zod schema: create an EddApiUrlSchema using z.string().url().transform that parses the URL, enforces protocol is 'https:' or 'http:' (throwing the same protocol error), strips search and hash, and performs the same pathname normalization logic (edd-api prefix handling and trailing slashes) before returning the normalized string; then refactor normalizeEddApiUrl to either use EddApiUrlSchema.parse(rawUrl) or delegate to the schema's transform so validation/error formatting is consistent with the project's Zod infrastructure.src/stdio.ts (1)
5-9: Suspicious type cast inwriteStderr.The cast
args as []is incorrect—it casts the array to an empty tuple type, which doesn't match theformatfunction's variadic signature. While this works at runtime due to JavaScript's spread behavior, it's misleading and could hide type errors.♻️ Suggested fix
function writeStderr(prefix: string | null, args: unknown[]): void { - const message = format(...(args as [])); + const message = format(...args); const line = prefix ? `${prefix}${message}\n` : `${message}\n`; process.stderr.write(line); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stdio.ts` around lines 5 - 9, The cast `args as []` in writeStderr is incorrect and misleading; instead remove that empty-tuple cast and spread args with a correct type assertion compatible with format (e.g., use format(...(args as Parameters<typeof format>)) or change the writeStderr signature to accept args: Parameters<typeof format>) so the call format(...args) is properly typed; update the writeStderr function to use that correct assertion and keep the rest of the logic (prefix, line, process.stderr.write) unchanged.src/edd-client.ts (1)
296-314: Sequential API calls may cause performance issues.
getCustomerByIdtries up to 4 different query parameters sequentially. Combined with the 3-retry logic, this could result in up to 12 API calls in the worst case, potentially causing slowdowns or rate-limiting issues.Consider documenting this behavior or adding a flag to limit fallback attempts when performance is critical.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/edd-client.ts` around lines 296 - 314, The getCustomerById function makes up to four sequential API calls (each subject to the existing 3-retry logic) which can cause high latency and rate-limit pressure; modify getCustomerById to accept an optional parameter (e.g., tryAll?: boolean or maxAttempts?: number) that defaults to the current behavior but when false/1 will only try the preferred param ({ customer: customerId }) and return early on miss, or alternatively implement parallel querying using Promise.any on the candidate URLs (constructed via buildUrl) and then call this.request on each candidate in parallel while preserving the existing retry wrapper; update references to getCustomerById and add the new parameter to callers that need the faster/limited behavior, and ensure errors are handled exactly as current (return null if none found).src/index.ts (1)
115-134: Deduplicate discount-code normalization.The same normalization logic is repeated for
discountCodesanddiscounts, and again inedd_get_sale. Extract a small helper and reuse the computed value to keep behavior consistent and reduce maintenance.♻️ Suggested refactor
const edd = new EDDClient(config); +const normalizeDiscountCodes = (raw: unknown): string[] | null => { + if (Array.isArray(raw)) return raw.filter((v) => typeof v === 'string'); + if (typeof raw === 'string' && raw.trim().length > 0) return [raw]; + return null; +}; + // Create MCP server const server = new McpServer({ name: 'io.github.verygoodplugins/mcp-edd', version: packageJson.version, }); @@ - const summary = sales.map((s) => ({ - discountCodes: (() => { - const raw = (s as unknown as { discount?: unknown }).discount ?? s.discounts ?? null; - if (Array.isArray(raw)) return raw.filter((v) => typeof v === 'string'); - if (typeof raw === 'string' && raw.trim().length > 0) return [raw]; - return null; - })(), + const summary = sales.map((s) => { + const discountCodes = normalizeDiscountCodes( + (s as unknown as { discount?: unknown }).discount ?? s.discounts ?? null + ); + + return { + discountCodes, id: s.ID, email: s.email, total: s.total, date: s.date, gateway: s.gateway, products: s.products.map((p) => p.name), hasLicenses: (s.licenses?.length ?? 0) > 0, // Back-compat: older clients look for `discounts`. - discounts: (() => { - const raw = (s as unknown as { discount?: unknown }).discount ?? s.discounts ?? null; - if (Array.isArray(raw)) return raw.filter((v) => typeof v === 'string'); - if (typeof raw === 'string' && raw.trim().length > 0) return [raw]; - return null; - })(), - })); + discounts: discountCodes, + }; + }); @@ - const discountCodes = (() => { - const raw = - (sale as unknown as { discount?: unknown }).discount ?? sale.discounts ?? null; - if (Array.isArray(raw)) return raw.filter((v) => typeof v === 'string'); - if (typeof raw === 'string' && raw.trim().length > 0) return [raw]; - return null; - })(); + const discountCodes = normalizeDiscountCodes( + (sale as unknown as { discount?: unknown }).discount ?? sale.discounts ?? null + );Also applies to: 178-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 115 - 134, Duplicate discount normalization logic appears in the object mapping for discountCodes and discounts (and again in edd_get_sale); extract a small helper function (e.g., normalizeDiscounts or parseDiscounts) that accepts the raw field and returns either string[] or null, replace the inline IIFE blocks in the mapping to call that helper once and reuse its result for both discountCodes and discounts, and update edd_get_sale to call the same helper so all places share the same behavior.manifest.json (1)
57-101: Consider consistent formatting in tools array.Lines 74, 79, and 92 use inline single-line format while other entries use multi-line format. For consistency and easier diffs in future edits, consider using the same multi-line format throughout.
♻️ Optional: Consistent multi-line formatting
- { "name": "edd_get_sale", "description": "Get complete sale details by ID or purchase key" }, + { + "name": "edd_get_sale", + "description": "Get complete sale details by ID or purchase key" + }, { "name": "edd_list_customers", "description": "List customers with purchase history and lifetime value" }, - { "name": "edd_get_customer", "description": "Get customer details by ID or email address" }, + { + "name": "edd_get_customer", + "description": "Get customer details by ID or email address" + }, ... - { "name": "edd_list_discounts", "description": "List all discount codes with usage stats" }, + { + "name": "edd_list_discounts", + "description": "List all discount codes with usage stats" + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@manifest.json` around lines 57 - 101, In manifest.json's "tools" array, some tool entries (e.g., the objects with "name": "edd_get_sale", "name": "edd_get_customer", and "name": "edd_get_discount") use single-line inline formatting while the rest use multi-line objects; update those single-line entries to the same multi-line key/value format as the other tool objects (preserve the same "name" and "description" values and ordering) so the entire "tools" array is consistently formatted for clearer diffs and future edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@manifest.json`:
- Around line 16-18: The manifest.json references an "icon" asset at
assets/icon.png which is missing; either add the icon file to the repository at
that path or remove the "icon": "assets/icon.png" entry from manifest.json so
the manifest no longer references a non-existent asset. Locate the "icon" key in
manifest.json (near the "screenshots" entry) and either commit a valid PNG to
assets/icon.png or delete the "icon" property from the JSON to resolve the
manifest error.
In `@tests/unit/stdio-clean.test.ts`:
- Around line 23-48: The test currently only waits 200ms then asserts stdout is
empty, which can false-pass if the spawned process exited early; capture stderr
(child.stderr) like stdout and after awaiting sleep(200) check child.exitCode
(or child.killed) and if it is not null/has exited, fail the test with the
captured stderr/exit info before asserting stdout; otherwise proceed to
expect(stdout.trim()).toBe(''). Update the test around the spawn/await block
(symbols: child, child.stdout, child.stderr, sleep, terminate) to collect stderr
and guard with an exitCode check prior to the stdout assertion.
---
Nitpick comments:
In `@manifest.json`:
- Around line 57-101: In manifest.json's "tools" array, some tool entries (e.g.,
the objects with "name": "edd_get_sale", "name": "edd_get_customer", and "name":
"edd_get_discount") use single-line inline formatting while the rest use
multi-line objects; update those single-line entries to the same multi-line
key/value format as the other tool objects (preserve the same "name" and
"description" values and ordering) so the entire "tools" array is consistently
formatted for clearer diffs and future edits.
In `@src/edd-client.ts`:
- Around line 296-314: The getCustomerById function makes up to four sequential
API calls (each subject to the existing 3-retry logic) which can cause high
latency and rate-limit pressure; modify getCustomerById to accept an optional
parameter (e.g., tryAll?: boolean or maxAttempts?: number) that defaults to the
current behavior but when false/1 will only try the preferred param ({ customer:
customerId }) and return early on miss, or alternatively implement parallel
querying using Promise.any on the candidate URLs (constructed via buildUrl) and
then call this.request on each candidate in parallel while preserving the
existing retry wrapper; update references to getCustomerById and add the new
parameter to callers that need the faster/limited behavior, and ensure errors
are handled exactly as current (return null if none found).
In `@src/env.ts`:
- Around line 42-79: Replace the manual URL validation in normalizeEddApiUrl
with a Zod schema: create an EddApiUrlSchema using z.string().url().transform
that parses the URL, enforces protocol is 'https:' or 'http:' (throwing the same
protocol error), strips search and hash, and performs the same pathname
normalization logic (edd-api prefix handling and trailing slashes) before
returning the normalized string; then refactor normalizeEddApiUrl to either use
EddApiUrlSchema.parse(rawUrl) or delegate to the schema's transform so
validation/error formatting is consistent with the project's Zod infrastructure.
In `@src/index.ts`:
- Around line 115-134: Duplicate discount normalization logic appears in the
object mapping for discountCodes and discounts (and again in edd_get_sale);
extract a small helper function (e.g., normalizeDiscounts or parseDiscounts)
that accepts the raw field and returns either string[] or null, replace the
inline IIFE blocks in the mapping to call that helper once and reuse its result
for both discountCodes and discounts, and update edd_get_sale to call the same
helper so all places share the same behavior.
In `@src/stdio.ts`:
- Around line 5-9: The cast `args as []` in writeStderr is incorrect and
misleading; instead remove that empty-tuple cast and spread args with a correct
type assertion compatible with format (e.g., use format(...(args as
Parameters<typeof format>)) or change the writeStderr signature to accept args:
Parameters<typeof format>) so the call format(...args) is properly typed; update
the writeStderr function to use that correct assertion and keep the rest of the
logic (prefix, line, process.stderr.write) unchanged.
- Deleted the assets/README.md file, which contained brand asset guidelines. - Updated the stdio-clean unit test to capture and assert stderr output, ensuring better error diagnostics during process execution.
- Updated the `terminate` function to check for both `exitCode` and `signalCode` before returning. - Refactored the promise handling for process exit to improve reliability. - Adjusted the test timeout for better performance during execution.
This was referenced Feb 17, 2026
6 tasks
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
1 participant
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
Pre-1.0.0 hardening pass to align with mcp-ecosystem standards and fix real-world API edge cases.
API Client Hardening
getCustomerByIdnow triescustomer,id,user_id, anduserparams sequentially (EDD API is inconsistent about which ID field it respects)discountCodesarray across list/get sale responses (handles string, array, and missing values)Standards Alignment
eslint-config-prettierintegrationnoUnusedLocals,noUnusedParameters,noImplicitReturns)prepublishOnlyruns tests before publishstdio.tsconsole patchingmanifest.jsonscreenshots point to actual assetnpm audit fix— 0 vulnerabilitiesRelease Infrastructure
NPM_TOKENsecret needed, Node 24).mcpbDesktop Extension and attach to GitHub ReleasesTests
Test plan
npm run build— cleannpm run lint— cleannpm test— 26/26 passingnpm audit— 0 vulnerabilitiesPost-merge
git tag v1.0.0 main && git push origin v1.0.0npm publish --access public(first-time)gh release create v1.0.0 --title "v1.0.0" --notes "Initial release"npmGitHub environment~/mcp-registry/bin/mcp-publisher publish server.jsonMade with Cursor