Add MCPcat #26
Add MCPcat #26
Conversation
WalkthroughAdds mcpcat as a dependency and a new exported MCPCAT_CONFIG constant for telemetry configuration. Wires conditional mcpcat initialization into GlobalpingMCP.init and adds a private getUserIdentification() helper used by mcpcat.identify, gated by an optional MCPCAT_PROJECT_ID env var. Extends environment typings (GlobalpingEnv and Cloudflare.Env) with Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
globalping-mcp-server | 9c38796 | Nov 29 2025, 03:31 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/config/constants.ts (1)
136-143: Clarify usage ofMCPCAT_CONFIG.PROJECT_ID_ENV_VAR
PROJECT_ID_ENV_VARis defined here but not used in the runtime path that readsenv.MCPCAT_PROJECT_ID, which creates duplication and potential drift between config and actual binding names.Either:
- Wire
PROJECT_ID_ENV_VARinto the code that reads the project ID (e.g. index.ts), or- Remove this field until it’s actually needed, to keep configuration minimal and unambiguous.
src/index.ts (1)
55-69: Avoid duplicating theMCPCAT_PROJECT_IDbinding nameHere the env binding name is hard-coded as
this.env.MCPCAT_PROJECT_ID, whileMCPCAT_CONFIG.PROJECT_ID_ENV_VARinconstants.tsalready encodes the same string. That duplication can get out of sync.Consider deriving the project ID from the config constant instead of hard-coding the property, for example:
const projectIdKey = MCPCAT_CONFIG.PROJECT_ID_ENV_VAR as keyof GlobalpingEnv; const projectId = this.env[projectIdKey]; if (projectId && MCPCAT_CONFIG.ENABLED) { mcpcat.track(this.server, projectId, { identify: async () => this.getUserIdentification(), }); }Alternatively, if you don’t plan to use the config indirection, dropping
PROJECT_ID_ENV_VARfromMCPCAT_CONFIGwould also remove the ambiguity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(1 hunks)src/config/constants.ts(1 hunks)src/index.ts(3 hunks)src/types/globalping.ts(1 hunks)worker-configuration.d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/types/globalping.tsworker-configuration.d.tssrc/config/constants.tssrc/index.ts
🧬 Code graph analysis (1)
src/index.ts (2)
src/config/constants.ts (1)
MCPCAT_CONFIG(140-143)src/auth/token-manager.ts (1)
isValidAPIToken(53-63)
🔍 Remote MCP DeepWiki
Perfect! I now have sufficient context. Let me compile the relevant information for the PR review.
Additional Context for PR Review
Project Overview
The Globalping MCP Server bridges AI models with the Globalping API, allowing AI to perform network tests like ping, traceroute, DNS, MTR, and HTTP from various global locations through natural language commands. The project is deployed as a Cloudflare Worker application, leveraging Durable Objects for stateful MCP agent instances and KV namespaces for OAuth data persistence.
Existing Observability
The codebase already includes observability enabled in its wrangler.jsonc configuration file through Cloudflare's logging and analytics infrastructure. The GlobalpingMCP class includes an onStateUpdate method that logs state changes, which can be used for basic monitoring and debugging within the application logic.
Authentication Context for MCPcat Integration
The Globalping MCP Server uses two primary authentication mechanisms: OAuth 2.0 and API Token authentication. For OAuth users, their username obtained from the Globalping OAuth introspect endpoint is used as the userName, while API token users are identified with a generic "API Token User". This context is relevant to the PR's user identification logic for MCPcat tracking, which the AI summary indicates returns a generic, non-PII user identity object.
Configuration Pattern
The init method within the GlobalpingMCP class handles initialization and configuration through tool registration, with the wrangler.jsonc file defining various bindings and configurations that could be used to conditionally enable features. The MCPcat integration follows this established pattern by using environment variable configuration and conditional enablement.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
🔇 Additional comments (4)
src/types/globalping.ts (1)
27-27: LGTM!Clean type addition for the optional MCPcat project ID environment variable.
package.json (1)
38-38: mcpcat version 0.1.8 is valid, stable, and has no known security vulnerabilities.Verification confirms:
- Version 0.1.8 exists on npm and is the latest release
- Package is not deprecated
- No security advisories found in GitHub's advisory database
- Licensed under MIT
The dependency is safe to use as specified.
worker-configuration.d.ts (1)
9-14: Verify MCPCAT_PROJECT_ID is wired in Wrangler config and deployment envThe type addition is fine, but MCPCAT_PROJECT_ID will be
undefinedat runtime unless it’s also declared in your Wrangler config (e.g.,vars/envinwrangler.json(c)) and set in the deployment environment. Double‑check those are in place so MCPcat initialization doesn’t silently skip due to a missing project ID.src/index.ts (1)
5-12: No changes requested for MCPcat-related importsThis import hunk doesn’t surface any specific issues; keeping it as-is is fine from a review standpoint.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
vitest.workers.config.ts (1)
21-21: Reduce risk of test/prod compatibility flag drift
compatibilityFlags: ["nodejs_compat_v2"]is now duplicated here and inwrangler.jsonc. To avoid subtle bugs when one is updated without the other, consider centralizing this (e.g., a small shared config module or a test that asserts both files expose the same flags) or at least adding a short comment noting it must stay in sync withwrangler.jsonc.src/index.ts (2)
5-5: Make MCPcat initialization non-fatal if tracking setup failsRight now, any exception thrown by
mcpcat.track(e.g., misconfigured project ID, unexpected runtime behavior) will bubble out ofinit()and can prevent the MCP server from coming up, even though analytics are non-critical.Consider wrapping the tracking setup in a
try/catchso failures degrade gracefully to “no tracking” rather than breaking initialization:- if (this.env.MCPCAT_PROJECT_ID && MCPCAT_CONFIG.ENABLED) { - mcpcat.track(this.server, this.env.MCPCAT_PROJECT_ID, { - // Identify users with generic labels - identify: async () => { - return this.getUserIdentification(); - }, - }); - - console.log("✓ MCPcat tracking initialized"); - } else { - console.log( - "✗ MCPcat tracking disabled (no project ID or disabled in config)", - ); - } + if (this.env.MCPCAT_PROJECT_ID && MCPCAT_CONFIG.ENABLED) { + try { + mcpcat.track(this.server, this.env.MCPCAT_PROJECT_ID, { + // Identify users with generic labels + identify: async () => this.getUserIdentification(), + }); + console.log("✓ MCPcat tracking initialized"); + } catch (err) { + console.error("✗ MCPcat tracking initialization failed:", err); + } + } else { + console.log( + "✗ MCPcat tracking disabled (no project ID or disabled in config)", + ); + }This keeps analytics from impacting core functionality if the library or config ever misbehaves.
Also applies to: 12-12, 55-69
379-422: MakehasAPITokenexplicitly boolean and reusegetToken()
hasAPITokenis currently derived via short‑circuiting:const hasAPIToken = this.props?.accessToken && isValidAPIToken(this.props.accessToken);This yields a
string | boolean | undefinedat the type level, which can be confusing if reused later. You can tighten this and avoid touchingpropsdirectly by reusinggetToken()and coercing to a boolean:- const isAuth = this.props?.isAuthenticated; - const hasAPIToken = - this.props?.accessToken && isValidAPIToken(this.props.accessToken); + const isAuth = this.props?.isAuthenticated; + const accessToken = this.getToken(); + const hasAPIToken = + !!(accessToken && isValidAPIToken(accessToken));This keeps all token access going through a single helper and makes the intent of the flag clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.ts(3 hunks)vitest.workers.config.ts(1 hunks)wrangler.jsonc(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wrangler.jsonc
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/index.tsvitest.workers.config.ts
🧬 Code graph analysis (1)
src/index.ts (2)
src/config/constants.ts (1)
MCPCAT_CONFIG(140-143)src/auth/token-manager.ts (1)
isValidAPIToken(53-63)
🔍 Remote MCP DeepWiki
Relevant facts from the repository docs useful for reviewing the MCPcat addition:
-
Project entry/architecture: Cloudflare Worker (src/index.ts) routes /mcp and /sse to a GlobalpingMCP Durable Object; auth is dual-path (API token priority, OAuth fallback). This is the integration point where mcpcat initialization in init()/index.ts will run.
-
Tool & init pattern: GlobalpingMCP.init() registers core measurement tools via registerGlobalpingTools(this, tokenProvider) and several utility tools (getMeasurement, compareLocations, help, authStatus). Any new telemetry init (mcpcat) should follow same safe token/provider patterns and avoid blocking init.
-
Auth context: Props passed into Durable Object (accessToken, userName, isAuthenticated) are the canonical way tokens/identity are exposed to tools; mcpcat identify callback should use non-PII values from these props (or same generic "API Token User") and must not leak tokens.
-
Env & types: Additions to env (MCPCAT_PROJECT_ID) must be present in worker types/bindings (worker-configuration.d.ts / src/types/globalping.ts) — PR already updates those files; verify names match wrangler.jsonc bindings and usage sites.
-
Config pattern & constants: Feature flags/config objects are placed under src/config/constants.ts and consumed in init(); new MCPCAT_CONFIG should follow existing constant patterns (as const, grouped with OAUTH_CONFIG / MCP_CONFIG).
-
Token handling & sanitization: All API usage sanitizes tokens (sanitizeToken) and uses validateToken before calling Globalping APIs; mcpcat code should not assume raw Bearer headers — use same sanitize/getToken pipeline.
-
Observability & runtime: Worker uses nodejs_compat (now historically used) and wrangler.jsonc controls compatibility_flags and observability — adding mcpcat dependency that relies on Node APIs must be validated against current compatibility flags in wrangler.jsonc (PR changes miniflare/compat flags noted in ai_summary — confirm runtime compatibility).
-
Durable Object state & privacy: GlobalpingMCP.state persists measurements and oAuth state; mcpcat should not persist PII and must only store/project non-identifying telemetry (PR indicates generic non-PII identify callback). Also ensure removeOAuthData()/KV cleanup not impacted.
Files/areas to pay extra attention to in the PR:
- package.json dependency bump (mcpcat ^0.1.8) — check transitive deps and size impact on Worker bundle.
- src/config/constants.ts — new MCPCAT_CONFIG shape and default values.
- src/index.ts / GlobalpingMCP.init() — placement of mcpcat init, error handling, that init is non-blocking and respects env flag (MCPCAT_PROJECT_ID) and ENABLED flag.
- src/types/globalping.ts and worker-configuration.d.ts — added MCPCAT_PROJECT_ID typing must match runtime binding names.
- wrangler.jsonc / vitest.workers.config.ts changes to nodejs_compat -> nodejs_compat_v2 — ensure mcpcat is compatible and tests still run in the Workers test pool.
Checks I recommend during review
- Verify mcpcat init is guarded by MCPCAT_CONFIG.ENABLED and by presence of MCPCAT_PROJECT_ID env var (no accidental network calls when disabled).
- Ensure identify callback returns only non-PII fields (userName generic, no email/id) and does not log tokens.
- Confirm token usage: mcpcat must not capture or persist raw tokens. If it needs user identity, use the same userName generic rules used elsewhere.
- Bundle size / compatibility: run local wrangler dev build and check bundle size; ensure no forbidden Node APIs used beyond Cloudflare nodejs compatibility.
- Tests: run unit + workers integration tests to ensure miniflare/compat change does not break test pool.
Sources used: DeepWiki repository documentation and code overview pages.,
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
54-82: Reorder MCPcat initialization to occur after all tools are registeredMCPcat documentation explicitly requires tools to be registered before calling
mcpcat.track(), so it can correctly identify and instrument the full tool set. The current code callstrack()beforeregisterGlobalpingTools()(line 78), which means MCPcat initializes with no tools available and cannot inject itscontextfield as intended.Move
mcpcat.track()to afterregisterGlobalpingTools()completes. Additionally, wrapping it in a fire-and-forget async block prevents telemetry initialization from blocking theinit()method.For example:
+ // Register all the Globalping tools first + registerGlobalpingTools(this, () => { + const raw = this.getToken() ?? ""; + return sanitizeToken(raw); + }); + - // Initialize MCPcat tracking if project ID is configured + // Initialize MCPcat tracking after tools are registered if (this.env.MCPCAT_PROJECT_ID && MCPCAT_CONFIG.ENABLED) { - try { - // Dynamic import to avoid loading mcpcat in environments where it's not needed - // This prevents errors when mcpcat dependencies (like node:os) aren't available - const mcpcat = await import("mcpcat"); - mcpcat.track(this.server, this.env.MCPCAT_PROJECT_ID, { - // Identify users with generic labels - identify: async () => { - return this.getUserIdentification(); - }, - }); - - console.log("✓ MCPcat tracking initialized"); - } catch (error) { - console.error("✗ Failed to initialize MCPcat tracking:", error); - } - } else { - console.log( - "✗ MCPcat tracking disabled (no project ID or disabled in config)", - ); - } - - // Register all the Globalping tools - registerGlobalpingTools(this, () => { - const raw = this.getToken() ?? ""; - return sanitizeToken(raw); - }); + (async () => { + try { + const mcpcat = await import("mcpcat"); + mcpcat.track(this.server, this.env.MCPCAT_PROJECT_ID, { + identify: async () => this.getUserIdentification(), + }); + console.log("✓ MCPcat tracking initialized"); + } catch (error) { + console.error("✗ Failed to initialize MCPcat tracking:", error); + } + })(); + } else { + console.log( + "✗ MCPcat tracking disabled (no project ID or disabled in config)", + ); + }
🧹 Nitpick comments (2)
src/index.ts (2)
5-5: Reconfirm whether MCPCAT should be enabled by default in all deploymentsImporting
MCPCAT_CONFIGhere means theENABLEDflag insrc/config/constants.tseffectively controls analytics for every deployment. Right now that flag istrue, so any environment that setsMCPCAT_PROJECT_IDwill send analytics by default.If you expect forks/self-hosters to explicitly opt in to MCPcat (especially for privacy/compliance reasons), consider flipping the default to
falseand requiring an explicit enablement in their config, with docs pointing to how to turn it on.
385-428: TightenhasAPITokentyping and double‑checkclientIdprivacy expectationsTwo small points here:
hasAPITokencurrently relies on the&&short‑circuit result, so its inferred type isstring | boolean | undefined. It behaves correctly at runtime, but making it explicitly boolean is clearer and avoids surprising types:- const hasAPIToken = - this.props?.accessToken && isValidAPIToken(this.props.accessToken); + const hasAPIToken = + !!this.props?.accessToken && isValidAPIToken(this.props.accessToken);
userData.clientIdfor OAuth users will be sent to MCPcat. IfclientIdis considered sensitive in your threat model (e.g., maps to specific tenants or internal app IDs), you might want to omit it or hash/bucket it before sending. If it’s already treated as non‑PII product telemetry, this is fine, but it’s worth an explicit decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/index.ts
🧬 Code graph analysis (1)
src/index.ts (2)
src/config/constants.ts (1)
MCPCAT_CONFIG(140-143)src/auth/token-manager.ts (1)
isValidAPIToken(53-63)
🔍 Remote MCP DeepWiki
Summary of repository docs & areas to check for the "Add MCPcat" PR (searched DeepWiki):
Key repo context (from project docs)
- This is a Cloudflare Worker MCP server: entry at src/index.ts; Durable Object GlobalpingMCP handles /mcp and /sse and registers tools; OAuth flows use OAUTH_KV and @cloudflare/workers-oauth-provider.,
Concrete facts to use while reviewing the PR
- Where mcpcat must be initialized: GlobalpingMCP.init() / src/index.ts — init registers tools and supplies a token-provider closure (sanitizeToken). Any mcpcat init should be non-blocking, use same sanitize/getToken flow, and avoid exposing raw tokens.
- Auth & token handling: token-manager sanitizes and classifies API tokens vs OAuth; isAPITokenRequest() path sets ctx.props with accessToken ("Bearer …") and userName "API Token User". mcpcat identify callback must not leak full tokens and must follow sanitizeToken usage.
- Env / bindings: Worker bindings include globalping_mcp_object (Durable Object) and OAUTH_KV; new MCPCAT_PROJECT_ID env var in PR must match worker bindings (wrangler.jsonc) and types (worker-configuration.d.ts / src/types/globalping.ts).
- OAuth & KV flow: /authorize stores StateData in OAUTH_KV (ttl 10m), /auth/callback reads & deletes — mcpcat init must not depend on ephemeral KV state or block those flows.
- Tool registration pattern: registerGlobalpingTools(this, tokenProvider) is used to register core measurement tools. New telemetry/tracking should be registered similarly (token provider + safe identify callback).
- Tests & runtime: repository uses Vitest workers pool and Wrangler for local/dev. PR also touches miniflare/compat flags (ai summary noted change to nodejs_compat_v2) — verify mcpcat package (native Node APIs) is compatible with the Worker compatibility flag used in wrangler.jsonc and vitest.workers config. Confirm tests run under vitest.workers.config.ts.
- State & privacy: GlobalpingMCP.state persists measurement cache and oAuth; mcpcat identify must return only non-PII (the repo uses "API Token User" for API tokens).
- Packaging/bundle: package.json changes adding mcpcat may increase bundle size; given Workers limits, reviewer should verify bundle size & disallowed Node APIs. DeepWiki notes nodejs_compat flag is present in config.
Concrete review checklist (apply to PR changes)
- Init gating: mcpcat dynamic import and init must be guarded by MCPCAT_CONFIG.ENABLED and presence of MCPCAT_PROJECT_ID env var; must not run when disabled. (Check src/config/constants.ts and src/index.ts changes.)
- Non-blocking init: dynamic import + mcpcat.track should not block init; failures should be caught and logged only (no throw that prevents agent init).
- Token/privacy: identify callback must use getUserIdentification() pattern, return non-PII (userName generic), never include raw tokens or refresh tokens. Use sanitizeToken before passing tokens anywhere.
- Types & bindings: MCPCAT_PROJECT_ID must be added to src/types/globalping.ts and worker-configuration.d.ts and also added to wrangler.jsonc bindings if required — names must match exactly.
- Compatibility & runtime: confirm mcpcat package APIs work under the Workers compatibility_flags used by wrangler.jsonc (if PR changed nodejs_compat→nodejs_compat_v2, run local wrangler dev and vitest workers to surface runtime errors).
- Bundle & size: run a local bundle/build to confirm size stays within Worker limits and no forbidden Node APIs are used.
- Tests: run unit and workers integration tests (vitest) — ensure vitest.workers.config.ts still compatible after changes to compatibility flags.
- Logging: ensure no sensitive values (tokens) are logged by the new code paths.
- Opt-out: confirm MCPCAT_CONFIG default (ENABLED) is safe (default should be false in public repos unless intended) — verify value in src/config/constants.ts.
- Cleanup on failure: if identify/track fails, ensure it doesn't alter Durable Object state or KV.
Tools/sources used
- DeepWiki repository docs and architecture pages: DeepWiki_read_wiki_contents, DeepWiki_read_wiki_structure.,
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
|
Thanks again for flagging this to us. We'll need to publish a new version for this to actually work, so please don't merge this until then. The underlying issue is because Cloudflare is a Node compatible environment that doesn't fully support some of what we import for debug logging. The next version we publish will default to console.log for debugging if the imports for logging are not detected. Thanks for your patience! 😄 -- MCPcat Team |
There was a problem hiding this comment.
@jimaek we just published a new version that should work with your Cloudflare deployment: [email protected] once you've confirmed this change worked for you I will go ahead and publish it as the new official v0.1.9 😄
Some other comments:
-
You should be able to revert the dynamic import statement you made as a result of the
node:oserrors. This is because we moved some of our utils to adjust dynamically as part of MCPcat. -
I believe your
test/integration/mcp-tools.test.tswill fail unless you augment them to call tools with the context parameters MCPcat asks agents to provide. A simple fix would be to just disable MCPcat for that test so it only has to concern itself with the functionality of GlobalPing's expectations. You could simple modifyvitest.workers.config.ts:
kvNamespaces: ["OAUTH_KV"],
bindings: {
GLOBALPING_CLIENT_ID: "test-client-id",
+ // Disable MCPcat during tests - it adds a required 'context' parameter to all tools for agents to understand their use-cases
+ MCPCAT_PROJECT_ID: "",
},
},
isolatedStorage: false,
You can also disable the tool context parameter requirement, but our users find it to be one of the most useful features for reproducing issues and discovering customer use-cases.
The option to disable it is passed in via an options object (reference)
src/index.ts
Outdated
| try { | ||
| // Dynamic import to avoid loading mcpcat in environments where it's not needed | ||
| // This prevents errors when mcpcat dependencies (like node:os) aren't available | ||
| const mcpcat = await import("mcpcat"); |
There was a problem hiding this comment.
You should be able to revert this commit.
This reverts commit b432c6e.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.ts (1)
379-422: Re‑check whetherclientIdis safe to send to MCPcat and adjust if needed
getUserIdentification()correctly avoids tokens and real usernames, but it does forwardthis.props.clientIdintouserData.clientId. IfclientIdis in any way user‑identifying or sensitive in your privacy model, this would violate the “non‑PII only” requirement for analytics.If there’s any doubt, consider either omitting it or hashing/normalizing it before sending, for example:
- clientId: this.props.clientId || "unknown", + // Optionally hash or drop if clientId could be sensitive + clientId: this.props.clientId ? "client_present" : "unknown",(or a stable but non‑reversible hash computed elsewhere if you still need segmentation).
Please confirm how
clientIdis defined in your OAuth provider and whether it’s acceptable to send raw, or adjust the field accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(1 hunks)src/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/index.ts
🧬 Code graph analysis (1)
src/index.ts (2)
src/config/constants.ts (1)
MCPCAT_CONFIG(140-143)src/auth/token-manager.ts (1)
isValidAPIToken(53-63)
🔍 Remote MCP DeepWiki
Summary of additional repository context relevant to reviewing "Add MCPcat" (PR #26):
-
Architecture & where mcpcat belongs
- Globalping MCP server runs in Cloudflare Worker; Durable Object GlobalpingMCP in src/index.ts handles MCP init, tools, token flow and state.
- MCP tools are registered during GlobalpingMCP.init() and token provider used is sanitizeToken(this.getToken()) — any telemetry init must follow same non-leaking token pattern.
-
Token & privacy expectations (must follow PR user requirements)
- Token handling: sanitizeToken/remove "Bearer " prefix, never emit raw tokens; API tokens are 32-char alphanumeric and treated as "API Token User" in props; identify callback must return non-PII only.
- getToolContext() returns { props } and tools rely on this for auth — mcpcat identify must use same (no raw token logging).
-
Env/bindings & types to verify
- Worker env includes GLOBALPING_CLIENT_ID, OAUTH_KV, and durable object binding name globalping_mcp_object; PR adds MCPCAT_PROJECT_ID — ensure exact name and add to wrangler.jsonc/type defs and worker-configuration.d.ts as PR does.
- Wrangler/Miniflare compatibility: repo uses compatibility_flags "nodejs_compat" (docs show this in wrangler.jsonc); PR changes to "nodejs_compat_v2" — ensure mcpcat package uses only APIs supported under that flag and verify vitest.workers config uses same compatibility.
-
Init/gating & non-blocking init guidance
- init() should be non-blocking and not break MCP init; failures must be caught and logged only — registerGlobalpingTools and other init flows illustrate pattern to follow (token-provider closure + safe errors).
-
Tests & runtime verification steps to run locally
- Repo uses Vitest with workers pool for integration; vitest.workers.config.ts and wrangler dev used for local testing — run integration tests after changes to mcpcat to surface runtime import issues under worker compatibility.
-
Other reviewers' comment in PR aligns with known runtime/import issues
- DeepWiki docs note nodejs_compat flag used for Node APIs; PR comment (MCPcat team) warns Cloudflare Node-compatible env may not support some debug imports — corroborates need to verify mcpcat version compatibility and that mcpcat downgrade/upgrade is gated by env checks (PR must avoid imports that fail at runtime).
Files/sections to inspect in PR (check these places in repo):
- src/index.ts init flow & where mcpcat.track/identify is called — ensure wrapped/caught and uses sanitizeToken/getUserIdentification.
- src/config/constants.ts and wrangler.jsonc — MCPCAT_CONFIG.ENABLED default and MCPCAT_PROJECT_ID presence and matching binding/name.
- src/types/globalping.ts and worker-configuration.d.ts — ensure MCPCAT_PROJECT_ID added and matches binding.
- vitest.workers.config.ts and wrangler.jsonc compatibility_flags — run integration tests and wrangler dev to detect import/runtime failures.
Tools/sources used:
- DeepWiki documentation pages for repository structure, architecture, authentication, MCP server, tooling, testing, and deployment.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/index.ts (1)
383-426: TightenhasAPITokentyping and double-check the “no PII” claim
hasAPITokenis currently a truthy check that ends up asstring | false | undefined, which works but is a bit muddy for future readers and type tooling. Also, the doc says “Does not expose PII” while includingclientIdinuserData— that’s probably fine, but it’s worth making explicit.You could clarify both with a small tweak:
- const isAuth = this.props?.isAuthenticated; - const hasAPIToken = - this.props?.accessToken && isValidAPIToken(this.props.accessToken); + const isAuth = this.props?.isAuthenticated; + const hasAPIToken = + !!this.props?.accessToken && + isValidAPIToken(this.props.accessToken);And either adjust the comment or add a short note that
clientIdis not considered user PII in this context (or drop it fromuserDataif that’s not true).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.ts(3 hunks)vitest.workers.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vitest.workers.config.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/index.ts
🧬 Code graph analysis (1)
src/index.ts (2)
src/config/constants.ts (1)
MCPCAT_CONFIG(140-143)src/auth/token-manager.ts (1)
isValidAPIToken(53-63)
🔍 Remote MCP DeepWiki
Summary of additional repository context relevant to reviewing "Add MCPcat" (PR #26)
-
Runtime: Worker uses Node.js compatibility flag; docs and wrangler.jsonc show current project uses "nodejs_compat" (compatibility_date 2025-03-10). PR updates compatibility flag to "nodejs_compat_v2" in wrangler.jsonc and vitest.workers.config.ts — verify mcpcat package runtime requirements under v2 vs v1.,
-
Where to verify safety of mcpcat import:
- GlobalpingMCP.init() registers tools and calls token provider; mcpcat should be initialized in init flow only when MCPCAT_PROJECT_ID env present and guarded (docs show init pattern and token sanitization). Ensure imports won't crash worker at module load time (must be conditional or resilient). See init/tool registration and token-sanitization patterns in src/index.ts and auth token manager.
-
Token/privacy expectations: repository enforces sanitizeToken() and never logging raw tokens; any mcpcat identify callback must follow same pattern (non-PII only, mask tokens). DeepWiki documents getToken()/sanitizeToken() usage for tools and registerGlobalpingTools token-provider closure.
-
Env/type bindings: repo adds MCPCAT_PROJECT_ID to env typings and wrangler bindings — DeepWiki shows correct pattern for Durable Object and KV bindings; ensure PR's name exactly matches binding used in src/index.ts and worker-configuration.d.ts.
-
Tests/local dev: vitest integration uses workers pool and miniflare; PR added MCPCAT_PROJECT_ID to vitest.workers.config.ts — run integration tests to reproduce runtime import failures (as MCPcat team warned). DeepWiki instructs to run integration tests after dependency changes.
-
Init gating & error handling guidance: docs show init should be non-blocking and catch errors — mcpcat initialization must follow that pattern (log on failure, don't throw during init). MCPcat team comment (PR discussion) explicitly requests not to merge until mcpcat fixes debug-import behavior; follow their guidance.
Files/locations to check in the PR
- src/index.ts — ensure mcpcat import/initialization is conditional, wrapped in try/catch, uses sanitizeToken/getUserIdentification, and doesn't leak tokens.
- src/config/constants.ts and wrangler.jsonc / vitest.workers.config.ts — confirm MCPCAT_PROJECT_ID binding name matches and compatibility flag change is intentional.
- src/types/globalping.ts and worker-configuration.d.ts — verify MCPCAT_PROJECT_ID added consistently.
- package.json / dependency bump — confirm mcpcat ^0.1.9-beta.2 compatibility with Cloudflare Worker nodejs_compat_v2.
Tools/sources used
- DeepWiki_read_wiki_structure — repository pages list and structure
- DeepWiki_read_wiki_contents — detailed docs on architecture, auth, GlobalpingMCP init, token handling, wrangler config, tests, and development guidance
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
🔇 Additional comments (1)
src/index.ts (1)
12-73: I'll verify the review comment about lazy-loading themcpcatmodule. Let me examine the current code structure and context.
<function_calls>
#!/bin/bashFirst, let's examine the src/index.ts file to see the current import and init() structure
echo "=== Examining src/index.ts structure ==="
head -100 src/index.ts | cat -necho ""
echo "=== Checking file size ==="
wc -l src/index.ts
</function_calls><function_calls>
#!/bin/bashLook for all mcpcat references in the codebase
echo "=== Searching for mcpcat references ==="
rg -n "mcpcat" --type ts --type jsecho ""
echo "=== Looking for import patterns ==="
rg -n "import.*from" src/index.ts | head -20
</function_calls><function_calls>
#!/bin/bashSearch for any Cloudflare worker or environment configuration context
echo "=== Looking for Cloudflare-related config ==="
rg -n -i "cloudflare|wrangler|worker" --type ts --type js --type json --type tomlecho ""
echo "=== Check for .wrangler or wrangler.toml ==="
fd -e toml -e json | grep -i wrangler | head -5
</function_calls><function_calls>
#!/bin/bashLook at the init() method around lines 55-73
echo "=== Examining init() method and context ==="
sed -n '45,85p' src/index.ts | cat -n
</function_calls><function_calls>
#!/bin/bashCheck package.json to see if mcpcat is listed and what version
echo "=== Checking package dependencies ==="
if [ -f package.json ]; then
jq '.dependencies, .devDependencies' package.json | grep -A5 -B5 "mcpcat" || echo "No mcpcat in package.json"
fiecho ""
echo "=== Looking for any tsconfig or build configuration ==="
cat tsconfig.json 2>/dev/null | head -30
</function_calls><function_calls>
#!/bin/bashCheck for any existing dynamic imports in the codebase as reference
echo "=== Looking for existing dynamic import patterns ==="
rg -n "await import(" --type ts --type jsecho ""
echo "=== Check for try/catch blocks and error handling patterns ==="
rg -n -A5 "try {" src/index.ts | head -40
</function_calls><function_calls>
#!/bin/bashExamine the exact current state of lines 12-73
echo "=== Current state of src/index.ts lines 12-73 ==="
sed -n '12,73p' src/index.ts | cat -n
</function_calls><function_calls>
mcpcat Cloudflare workers compatibility issues node imports
</function_calls><function_calls>
#!/bin/bashCheck if there's any existing documentation about mcpcat or Cloudflare worker setup
echo "=== Looking for README or documentation ==="
if [ -f README.md ]; then
head -100 README.md
fiecho ""
echo "=== Check for any comments about mcpcat or telemetry ==="
rg -n -B2 -A2 "mcpcat|telemetry|MCPCAT" src/index.ts
</function_calls>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/index.ts (1)
380-423: Tighten identification typing and confirmclientIdis non-PIIThe identification helper is logically sound, but a couple of refinements would make telemetry payloads safer and more self-documenting:
- Narrow the return type instead of
Record<string, any>so future changes don’t accidentally leak additional fields or PII:- private getUserIdentification(): { - userId: string; - userName: string; - userData: Record<string, any>; - } { + private getUserIdentification(): { + userId: "api_token_user" | "oauth_user" | "anonymous_user"; + userName: string; + userData: { + authMethod: "api_token" | "oauth" | "none"; + clientId?: string; + }; + } {
- The comment states this “Does not expose PII”, but
clientId: this.props.clientId || "unknown"will be sent for OAuth flows. Please double-check thatclientIdis an application/client identifier and never user-specific. If there’s any risk it could contain user-identifying data, consider either dropping it or replacing it with a coarse boolean flag likehasClientId: booleanto preserve privacy.Please confirm mcpcat’s expected shape for
identifypayloads in its current version and thatclientIdis safe to emit according to your telemetry/privacy guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.ts(3 hunks)src/mcp/tools.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/mcp/tools.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/index.ts
🧬 Code graph analysis (1)
src/index.ts (2)
src/config/constants.ts (1)
MCPCAT_CONFIG(140-143)src/auth/token-manager.ts (1)
isValidAPIToken(53-63)
🔍 Remote MCP DeepWiki
Relevant repository docs and architecture useful for this PR review (pulled from DeepWiki):
-
Runtime & compatibility:
- Worker currently relies on Node.js compatibility flag; PR changing compatibility flag to nodejs_compat_v2 is notable because mcpcat may rely on different Node-compat behavior — confirm mcpcat v^0.1.9-beta.2 compatibility with nodejs_compat_v2 before merging.,
-
Where mcpcat must be safe to import:
- Docs show init/registration pattern in GlobalpingMCP.init() — imports that can fail at module-load must be avoided or guarded; initialization should be non-blocking and errors caught so DO init doesn't crash. Verify mcpcat import/usage is resilient to missing debug logging imports as MCPcat team indicated.
-
Token / privacy handling:
- Token flow: agent.getToken() → sanitizeToken() → API client validateToken(); repo consistently masks/sanitizes tokens and sets ctx.props for API-token requests. Any mcpcat identify callback must use sanitized token/non-PII only and follow existing mask/sanitize patterns.
-
Env & bindings:
- MCPCAT_PROJECT_ID should match the new env binding pattern (MCPCAT_PROJECT_ID added in types and wrangler bindings in PR). Confirm binding name matches everywhere (src/types, worker-configuration.d.ts, wrangler/vitest config).
-
Init gating & error handling guidance:
- Docs require init to be non-blocking and to catch errors; mcpcat initialization must not throw during module load or DO init (log on failure). MCPcat team requested waiting for their patch — heed that request.
-
Tests / local dev:
- Vitest integration uses miniflare/miniflare compatibility flags; PR updates miniflare compatibilityFlags to nodejs_compat_v2 — run integration tests locally to reproduce potential import failures noted by MCPcat team before merging.
Files / behaviors to verify in the PR (actionable checklist):
- src/index.ts
- mcpcat import occurs only inside guarded/try-catch init flow (not at top-level module import), and failures do not crash DO init.
- Identify callback uses sanitizeToken()/non-PII and does not log raw tokens.
- src/config/constants.ts, wrangler.jsonc, vitest.workers.config.ts, src/types/globalping.ts, worker-configuration.d.ts
- MCPCAT_PROJECT_ID binding name is consistent across all locations.
- package.json
- mcpcat bump (-> ^0.1.9-beta.2) — validate mcpcat runtime requirements (nodejs_compat_v2) and consider waiting for MCPcat team's published fix as requested in PR discussion.
- src/mcp/tools.ts
- Removal of empty inputSchema entries is intentional; confirm tools' behavior unchanged.
Sources consulted:
- DeepWiki repository structure and contents for architecture, auth, MCP agent/init patterns, token management, env/bindings, testing and deployment guidance.,
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
🔇 Additional comments (1)
src/index.ts (1)
5-12: Top-levelmcpcatimport poses a potential runtime failure in Cloudflare Workers, though the PII concern aboutclientIdis unfounded.The review correctly identifies that the top-level
import * as mcpcat from "mcpcat"(line 12) can fail at module-evaluation time, before the try/catch ininit()runs. This is technically valid: if themcpcatpackage uses unsupported Node APIs during its module initialization, it will crash the worker regardless of the guardedmcpcat.track()call withininit().However, the concern about
clientIdbeing PII is incorrect. TheclientIdfield inPropsis the OAuth application identifier (set fromGLOBALPING_CLIENT_IDin the environment), not a user identifier. The comment "Does not expose PII - uses generic labels only" is accurate; the three identification buckets (api_token_user,oauth_user,anonymous_user) correctly avoid user-specific identifiers.Recommended actions:
If MCPcat compatibility with Cloudflare Workers is confirmed as broken in the current beta version, migrate the import to dynamic loading inside
init():const mcpcat = await import("mcpcat"); mcpcat.track(/*...*/);This ensures module-load failures are caught and logged as non-fatal warnings.
Verify before merging: Test the exact
[email protected]version in your Cloudflare Workers environment (withnodejs_compat_v2if enabled) to confirm whether module-evaluation failures occur. If the MCPcat team has published a patched version, update the dependency.Retain the try/catch as a second line of defense for any runtime errors during
track()invocation.
|
@naji247 can you fix that? protobufjs 7.0.0 - 7.2.4 3 critical severity vulnerabilities |
|
Will do @eyepokes sorry about that! |
I am not a fan of overrides, but it will fix that. |
refactor: formatting code fix: use Number.isNaN for loss check in formatMeasurementSummary
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)src/index.ts(3 hunks)src/mcp/helpers.ts(1 hunks)src/mcp/tools.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/index.tssrc/mcp/helpers.tssrc/mcp/tools.ts
🧬 Code graph analysis (1)
src/index.ts (2)
src/config/constants.ts (1)
MCPCAT_CONFIG(140-143)src/auth/token-manager.ts (1)
isValidAPIToken(53-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
🔇 Additional comments (3)
src/mcp/helpers.ts (1)
139-141: LGTM!Switching from
isNaN()toNumber.isNaN()is a correctness improvement.Number.isNaN()is more strict and only returnstruefor actualNaNvalues without type coercion, preventing unexpected behavior.src/mcp/tools.ts (1)
13-13: LGTM!The signature formatting change and removal of empty
inputSchemadeclarations from parameter-less tools (locations, limits) are sensible cleanups with no functional impact.src/index.ts (1)
5-5: LGTM!The MCPcat integration implementation is solid:
- Properly gated by environment variable and config flag
- Try/catch ensures telemetry failures don't break the MCP server
getUserIdentification()correctly checks API token first (preventing misclassification), then OAuth, then falls back to anonymous- No PII is exposed in the analytics
However, do not merge this PR until the critical issues with the mcpcat dependency are resolved (see my comment on package.json).
Also applies to: 12-12, 55-71, 378-420
|
Just deployed |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json(1 hunks)
🧰 Additional context used
🔍 Remote MCP DeepWiki
Summary — additional context to help review PR #26 (Add MCPcat)
-
What MCPCat is and where it integrates
- MCPcat = analytics/telemetry SDK for MCP servers; docs show a TypeScript SDK and npm install instructions; intended to be initialized from the MCP server init flow.,
-
Security / dependency risk you must re-check (actionable)
- protobufjs has a critical Prototype Pollution advisory (GHSA-h755-8qp9-cq85; affected versions >=7.0.0 <7.2.5 and >=6.10.0 <6.11.4; fixed in 7.2.5 / 6.11.4). If mcpcat (or its transitive deps like redact-pii → google-gax → protobufjs) pulls an affected protobufjs, that creates a critical vulnerability — run the concrete checks below..
- Concrete local checks for reviewer: in the repo clone run:
- npm ls protobufjs
- npm info mcpcat versions --json && npm view [email protected] version
- if vulnerable versions appear, either require mcpcat@>=the patched release or add an explicit override to force protobufjs >=7.2.5.
- (PR comments already mention [email protected] and an override was used upstream; still verify in your environment).,
-
Where the PR touches the system and what to validate in-code
- Init path: GlobalpingMCP.init() is the place tools and telemetry are registered; confirm MCPcat initialization is guarded by MCPCAT_CONFIG.ENABLED and by presence of MCPCAT_PROJECT_ID so it’s not active in tests/dev unless intended. Verify token/props handling and that identify callback uses only non-PII fields.
- Env/type bindings: PR adds MCPCAT_PROJECT_ID to GlobalpingEnv / worker-configuration.d.ts — ensure you regenerate wrangler types (wrangler types) and that vitest/miniflare config includes that binding for tests.,
- Wrangler compatibility flag change: PR updates compatibility flag to nodejs_compat_v2 (also changed in vitest / miniflare config). Confirm local dev/test (wrangler dev, vitest.workers) works with nodejs_compat_v2.
-
Tool registration / schema change to review
- PR removed empty inputSchema: confirm removal doesn’t change public tool schemas advertised to models (tools are registered via registerGlobalpingTools(this, tokenProvider) and server.tool(name, schema, handler)); missing schema vs explicit {} can affect generated tool metadata — check that the MCP server still advertises the same parameter documentation.,
-
Tests & CI
- PR bumped vitest worker compatibilityFlags → make sure integration tests that use the Workers pool / miniflare run locally and in CI with the updated compatibility flag. Run the full test suite (unit + workers integration) after installing mcpcat to confirm no runtime-only import errors occur.
Quick actionable checklist for reviewers (minimal, run these)
- npm install && npm info mcpcat versions --json && npm view [email protected] version (verify mcpcat release).
- npm ls protobufjs (check for <=7.2.4 in the dependency tree). If present, either (a) ensure mcpcat’s published release pins/overrides protobufjs to >=7.2.5 or (b) use package.json overrides.
- Run wrangler types; run npm run dev; run npm run test (unit + integration) to catch runtime import/logging issues.
- Confirm MCPCAT_CONFIG.ENABLED guard and MCPCAT_PROJECT_ID env presence is required before SDK init (no telemetry in test/dev by accident).
- Verify tool schemas/documentation remain the same (esp. locations/limits/help) after removing empty inputSchema entries.
Tools/sources used
- Project docs/architecture/config from DeepWiki: DeepWiki_read_wiki_structure, DeepWiki_read_wiki_contents, DeepWiki_ask_question.
- Public web searches (mcpcat site, protobufjs advisories / GHSA) via web.run search queries.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
package.json
Outdated
| "agents": "^0.2.21", | ||
| "globalping": "^0.2.0", | ||
| "hono": "^4.10.6", | ||
| "mcpcat": "^0.1.9-beta.3", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Query npm registry for [email protected] dependencies
echo "=== [email protected] published dependencies ==="
npm view [email protected] --json 2>/dev/null | jq '.dependencies'
echo -e "\n=== [email protected] published dependencies (for comparison) ==="
npm view [email protected] --json 2>/dev/null | jq '.dependencies'
# Check npm registry vulnerability data
echo -e "\n=== Checking for vulnerabilities in mcpcat versions ==="
npm view [email protected] --json 2>/dev/null | jq '.vulnerabilities // "No vulnerabilities field"'Repository: jsdelivr/globalping-mcp-server
Length of output: 533
🌐 Web query:
GHSA-h755-8qp9-cq85 protobufjs vulnerability details and fix version
💡 Result:
Summary — GHSA-h755-8qp9-cq85 (CVE-2023-36665)
-
What it is: a Prototype Pollution vulnerability in protobuf.js that lets a crafted protobuf message (or certain parsing/loader inputs) modify Object.prototype (Object.constructor.prototype...), allowing data/function overwrite and leading to high-severity impact. Exploitation vectors include parse(), load()/loadSync(), ReflectionObject.setParsedOption, and util.setProperty. [1][2]
-
Affected versions:
-
= 6.10.0, < 6.11.4
-
= 7.0.0, < 7.2.5. [1][3]
-
-
Fixed versions:
- 6.11.4
- 7.2.5. [1][3]
References
- GitHub Advisory (GHSA-h755-8qp9-cq85 / protobufjs): Jul 5, 2023. [1]
- OSV / CVE-2023-36665 summary. [2]
- CVE details entry for CVE-2023-36665. [3]
🌐 Web query:
mcpcat 0.1.9-beta.3 protobufjs dependency resolution
💡 Result:
Short answer: force the protobufjs version you need with your package manager (npm "overrides" or Yarn "resolutions"/Yarn v2+ manifest). Example snippets and references below.
If using npm (v8+), add an overrides entry in your root package.json and reinstall:
{
"overrides": {
"protobufjs": "7.5.4"
}
}
Then run: npm install. [npm overrides docs][1], protobufjs latest on npm. [2]
If using Yarn (classic or v2+), add a resolutions entry in package.json and run yarn install:
{
"resolutions": {
"protobufjs": "7.5.4"
}
}
(Yarn v2+ uses the manifest/resolutions field likewise.) [3][2]
Notes:
- Pick the exact protobufjs version that is compatible with mcpcat (or the package complaining). Use the protobufjs npm page to confirm available versions. [2]
- If mcpcat itself declares protobufjs as a direct dependency, you may need to align your direct dependency spec (or open an issue/PR against mcpcat). npm overrides cannot override a direct dependency unless specs match (see npm docs). [1]
Sources:
[1] npm package.json "overrides" docs. (docs.npmjs.com)
[2] protobufjs package on npm (versions). (npmjs.com)
[3] Yarn "resolutions" / manifest docs. (yarnpkg.com)
If you want, tell me which package manager and the exact error message and I’ll give the precise package.json snippet and install commands.
Citations:
- 1: https://docs.npmjs.com/cli/v11/configuring-npm/package-json?utm_source=openai
- 2: https://www.npmjs.com/package/protobufjs?utm_source=openai
- 3: https://yarnpkg.com/configuration/manifest?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Trace transitive protobufjs dependency through @opentelemetry/otlp-transformer
echo "=== @opentelemetry/[email protected] dependencies ==="
npm view "@opentelemetry/[email protected]" --json 2>/dev/null | jq '.dependencies'
# Check if protobufjs is directly or transitively required
echo -e "\n=== Searching for protobufjs in @opentelemetry dependencies chain ==="
npm view "@opentelemetry/[email protected]" --json 2>/dev/null | jq '.peerDependencies, .dependencies' | grep -i protobuf || echo "Not found in direct dependencies"
# Check [email protected] as well (another dep that might pull protobufjs)
echo -e "\n=== [email protected] dependencies ==="
npm view "[email protected]" --json 2>/dev/null | jq '.dependencies'Repository: jsdelivr/globalping-mcp-server
Length of output: 542
🌐 Web query:
@opentelemetry/otlp-transformer 0.203.0 protobufjs version transitive dependency
💡 Result:
@opentelemetry/[email protected] pulls in protobufjs as a transitive dependency: "protobufjs": "^7.3.0". [1][2]
Sources:
- mirror of package dependency list showing protobufjs:^7.3.0. [1]
- npm / package listings for the OpenTelemetry packages (package published info). [2]
References:
[1] mirror commit showing dependencies (includes "protobufjs": "^7.3.0"). (git.joshthomas.dev)
[2] @opentelemetry package listing / registry info. (npmjs.com)
Citations:
- 1: https://git.joshthomas.dev/mirrors/opencode/commit/53630ebdce8a3dc64dd11b0fb38205b35fad74fc?utm_source=openai
- 2: https://www.npmjs.com/package/%40opentelemetry/otlp-transformer?utm_source=openai
🏁 Script executed:
npm install
npm ls protobufjs # Should show 7.3.0 or higher
npm audit # Should show no critical protobufjs vulnerabilitiesRepository: jsdelivr/globalping-mcp-server
Length of output: 2079
CRITICAL: Protobufjs vulnerability is confirmed present and unresolved in [email protected].
Local verification reveals a critical protobufjs vulnerability (GHSA-h755-8qp9-cq85) is actively present. While @opentelemetry/otlp-transformer correctly specifies protobufjs: ^7.3.0 (safe), a secondary dependency chain brings in a vulnerable version:
[email protected]
└─ @google-cloud/[email protected]
└─ [email protected]
└─ [email protected] ← CRITICAL (fixed version is 7.2.5)
The [email protected] package pins protobufjs to 7.2.4, which falls within the vulnerable range. This is a blocking issue that must be resolved before merging:
Required:
- Update
redact-piito a version that depends on safegoogle-gax/@google-cloud/dlpversions (or add npmoverridesto forceprotobufjs: ^7.2.5) - Run
npm auditlocally and confirm all 3 critical vulnerabilities are resolved - The upgrade from mcpcat beta.2 to beta.3 does not address this vulnerability—both versions have the same issue via their shared
redact-piidependency
There was a problem hiding this comment.
Confirmed, all 3 vulnerabilities are still there
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Really triple checked this time and should be good with [email protected]. It's Thanksgiving over here and the stuffing must have gone to my head!
Also our main users of the package got back to us so I was able to remove it completely.
There was a problem hiding this comment.
@naji247 mcpcat crashes the server if you call any tool, for example "method": "tools/list"
{
"message": "{\"stack\":\"TypeError: Cannot read properties of undefined (reading 'typeName')\\n at parseDef (index.js:63026:53)\\n at zodToJsonSchema (index.js:63085:16)\\n at index.js:63247:44\\n at Array.map (<anonymous>)\\n at index.js:63242:89\\n at index.js:51486:30\\n at index.js:67346:40\\n at index.js:51486:30\\n at index.js:51292:34\",\"message\":\"Cannot read properties of undefined (reading 'typeName')\"}"
}
There was a problem hiding this comment.
Looks like the new MCP SDK released Zod 4 support and caused a regression. This is now fixed.
Tested the following with the new 0.1.9-beta.5 version:
- All tests passing on this branch
- Confirmed npm audit shows 0 vulns
- Setup the GlobalPing repo locally to run with Claude Code + MCPcat successfully
- Setup the MCP Inspector to test the GlobalPing MCP server locally
We should be good to go now. Thanks for the patience!
|
Looks like its good to merge |
…fix for incorrect user type detection
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/types/app.ts (1)
1-9: ClarifyisOAuthrequiredness and update allPropscreators accordinglyAdding a new non‑optional
isOAuth: booleanto the exportedPropstype will force every existing creator ofPropsto be updated; otherwise TS will fail or callers may end up passingundefinedat runtime if they bypass typings. Review all places that constructProps(including non‑OAuth flows) and either:
- Set
isOAuth: falseexplicitly in those paths, or- If you want to treat the flag as best‑effort context only, relax the type to
isOAuth?: booleanand handle the default (false) where it’s consumed.Right now the type suggests the field is always present, but only the OAuth callback path in this PR is clearly setting it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app.ts(1 hunks)src/index.ts(4 hunks)src/types/app.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{vue,ts,js}
⚙️ CodeRabbit configuration file
We use Nuxt with auto imports enabled. Don't warn about missing imports.
Files:
src/app.tssrc/types/app.ts
🔍 Remote MCP DeepWiki
Summary of additional repository context relevant to reviewing PR #26 (Add MCPcat)
-
Where telemetry should be initialized and guarded
- MCP SDK and telemetry/agent init live in src/index.ts init(); telemetry must only run when config enabled and env var present (init/register tools location) — see MCP Server / GlobalpingMCP init and routing docs.,
-
Environment/binding changes required for new MCPCAT_PROJECT_ID
- Env types, wrangler/miniflare/vitest configs and worker types must include MCPCAT_PROJECT_ID so local/test runs and wrangler dev pick it up (wrangler.jsonc and vitest.workers config referenced in docs). Confirm PR adds these; tests and local miniflare must include empty binding.
-
Token / PII / identification rules
- getUserIdentification must avoid transmitting PII; token and OAuth flows sanitize tokens via sanitizeToken/getToolContext — ensure MCPcat identify callback uses only non-PII fields (API token → masked "API Token User" or OAuth username) per token management and OAuth flow docs.
-
Where to wire mcpcat safely
- Best place is during GlobalpingMCP.init() (MCP server init) or the Durable Object init flow; docs describe registerGlobalpingTools and server.tool registration in init(). Ensure mcpcat wiring follows same pattern and uses token provider/sanitizeToken.
-
Tests / dev-run guidance
- Add MCPCAT_PROJECT_ID to vitest.workers config and miniflare bindings (docs specify running unit + Workers integration tests, and adding env/bindings for tests). Run npm ls / npm audit to inspect transitive deps before merging.
-
Dependency/security note (context for mcpcat dependency)
- Repo guidance recommends checking transitive dependency trees and running npm audit (docs mention running npm ls and tests). Given PR discussion about protobufjs vulnerabilities in mcpcat dependency tree, ensure lockfile / overrides addressed before merge.
Files/areas to double-check in the PR (actionable)
- src/index.ts: ensure MCPCAT init only runs when MCPCAT_CONFIG.ENABLED && env.MCPCAT_PROJECT_ID; follow existing init/token provider patterns.
- src/types/globalping.ts & worker-configuration.d.ts & vitest.workers.config.ts & wrangler.jsonc: confirm MCPCAT_PROJECT_ID binding/type added (for dev/test).
- getUserIdentification implementation: confirm no emails/tokens leaked; align with Token Management and OAuth introspection behavior.
- package.json / lockfile: run npm ls and npm audit to verify mcpcat version (and its transitive protobufjs versions) are safe or overridden per discussion.
- Tests: run unit + Workers integration tests (vitest + workerd/miniflare) to catch runtime import/logging failures in Node-compatible CF runtime (docs note nodejs_compat flags and runtime issues).
Sources used
- DeepWiki pages for repository documentation and architecture, including Overview, MCP Server, Authentication, API Integration, Development and Testing, Configuration, and Deployment.,
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: globalping-mcp-server
🔇 Additional comments (1)
src/app.ts (1)
249-266: Based on my verification, I can now provide the rewritten review comment:
The review comment's concern is unfounded—
isOAuthis already set consistently across all flows.There is only one
completeAuthorizationcall in the codebase (the OAuth callback at line 250). Additionally,isOAuthis already initialized in the non-OAuth flow (API token path atsrc/index.ts:553setsisOAuth: false), and thePropstype definition requiresisOAuth: booleanas a mandatory field. Downstream consumers safely handle it with optional chaining (!this.props?.isOAuthatsrc/index.ts:388). No action is needed.
it was detecting user type incorrectly(always treated the user as an API Token user, even though it should have been an OAuth user). The latest commit fixes this |
|
Thanks everyone, I think it's ready to merge. @naji247 ty for the cooperation |
https://docs.mcpcat.io/quickstart