Skip to content

Conversation

@2witstudios
Copy link
Owner

@2witstudios 2witstudios commented Jan 15, 2026

Summary by CodeRabbit

  • Improvements

    • Enhanced AI tool compatibility and reliability across different AI providers through improved tool name standardization.
  • Tests

    • Added comprehensive test coverage for tool name handling, including edge cases, nested namespaces, and round-trip validation.

✏️ Tip: You can customize this high-level summary in your review settings.

The tool name sanitization (replacing colons with underscores) was only
applied when using Google Gemini. However, Azure via OpenRouter also
requires tool names to match the pattern ^[a-zA-Z0-9_.-]+$ (no colons).

This caused intermittent errors like "Invalid 'tools[24].name'" when
MCP tools with names like "mcp:servername:toolname" were sent to Azure.

Now sanitization happens for all providers since underscores are
universally supported and parseMCPToolName already handles both formats.

Fixed in both:
- /api/ai/chat (page chat)
- /api/ai/global/[id]/messages (global assistant)
The tool name sanitization (replacing colons with underscores) was only
applied when using Google Gemini. However, Azure via OpenRouter and
other providers also require tool names to match ^[a-zA-Z0-9_.-]+$.

This caused errors like "Invalid 'tools[24].name'" when MCP tools with
names like "mcp:servername:toolname" were sent to these providers.

Changes:
- Add sanitizeToolName() and sanitizeToolNamesForProvider() utilities
  to mcp-tool-converter.ts with full JSDoc documentation
- Refactor both chat routes to use the centralized utility
- Add comprehensive tests for sanitization and round-trip parsing

The colon format is preserved internally (mcp:server:tool) to support
nested namespaces in tool names. Sanitization to underscore format
(mcp__server__tool) happens at the API boundary before sending to
providers. parseMCPToolName() handles both formats for execution.
@chatgpt-codex-connector
Copy link

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

This PR extracts and centralizes tool name sanitization logic into reusable utility functions (sanitizeToolName and sanitizeToolNamesForProvider) for AI provider compatibility. The changes replace inline sanitization across route handlers with calls to the new utilities, converting colon characters to double underscores to ensure provider-compatible tool names.

Changes

Cohort / File(s) Summary
New Utility Functions
apps/web/src/lib/ai/core/mcp-tool-converter.ts
Added sanitizeToolName() to replace colons with double underscores and sanitizeToolNamesForProvider<T>() to apply sanitization across all keys in a tool record, providing reusable provider compatibility helpers.
Route Handlers Using Sanitization
apps/web/src/app/api/ai/chat/route.ts, apps/web/src/app/api/ai/global/[id]/messages/route.ts
Replaced previous manual tool merging and inline sanitization logic with calls to sanitizeToolNamesForProvider(), consolidating provider-specific name normalization into a centralized step.
Test Coverage
apps/web/src/lib/ai/core/__tests__/mcp-tool-name-validation.test.ts
Added comprehensive tests for sanitizeToolName() and sanitizeToolNamesForProvider(), including colon handling, nested namespaces, empty strings, and round-trip parsing scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Colons once lived in tool names free,
But now with underscores—__—they shall be!
Tools merge and sanitize, provider at ease,
A refactor so clean, it's sure to please! 🔧

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is vague and does not clearly describe the actual changes made in the pull request, which involve refactoring tool name sanitization logic. Revise the title to clearly describe the main change, such as 'Refactor AI tool name sanitization for provider compatibility' or 'Extract tool name sanitization logic to dedicated utility functions'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d82f1a7 and d61ea6f.

📒 Files selected for processing (4)
  • apps/web/src/app/api/ai/chat/route.ts
  • apps/web/src/app/api/ai/global/[id]/messages/route.ts
  • apps/web/src/lib/ai/core/__tests__/mcp-tool-name-validation.test.ts
  • apps/web/src/lib/ai/core/mcp-tool-converter.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/web/src/app/**/route.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

apps/web/src/app/**/route.{ts,tsx}: In Next.js 15 route handlers, params in dynamic routes are Promise objects and MUST be awaited before destructuring
Use Response.json() or NextResponse.json() for returning JSON from route handlers
Get request body using const body = await request.json();
Get search parameters using const { searchParams } = new URL(request.url);

apps/web/src/app/**/route.{ts,tsx}: In Next.js 15 dynamic routes, params are Promise objects and MUST be awaited before destructuring: const { id } = await context.params;
In Route Handlers, get request body with const body = await request.json();
In Route Handlers, get search parameters with const { searchParams } = new URL(request.url);
In Route Handlers, return JSON using Response.json(data) or NextResponse.json(data)
For permission logic, use centralized functions from @pagespace/lib/permissions: getUserAccessLevel(), canUserEditPage()

Files:

  • apps/web/src/app/api/ai/global/[id]/messages/route.ts
  • apps/web/src/app/api/ai/chat/route.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Never use any types - always use proper TypeScript types
Use camelCase for variable and function names
Use UPPER_SNAKE_CASE for constants
Use PascalCase for type and enum names
Use kebab-case for filenames, except React hooks (camelCase with use prefix), Zustand stores (camelCase with use prefix), and React components (PascalCase)
Lint with Next/ESLint as configured in apps/web/eslint.config.mjs
Message content should always use the message parts structure with { parts: [{ type: 'text', text: '...' }] }
Use centralized permission functions from @pagespace/lib/permissions (e.g., getUserAccessLevel, canUserEditPage) instead of implementing permission logic locally
Always use Drizzle client from @pagespace/db package for database access
Use ESM modules throughout the codebase

**/*.{ts,tsx}: Never use any types - always use proper TypeScript types
Write code that is explicit over implicit and self-documenting

Files:

  • apps/web/src/app/api/ai/global/[id]/messages/route.ts
  • apps/web/src/app/api/ai/chat/route.ts
  • apps/web/src/lib/ai/core/__tests__/mcp-tool-name-validation.test.ts
  • apps/web/src/lib/ai/core/mcp-tool-converter.ts
**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.ts: React hook files should use camelCase matching the exported hook name (e.g., useAuth.ts)
Zustand store files should use camelCase with use prefix (e.g., useAuthStore.ts)

Files:

  • apps/web/src/app/api/ai/global/[id]/messages/route.ts
  • apps/web/src/app/api/ai/chat/route.ts
  • apps/web/src/lib/ai/core/__tests__/mcp-tool-name-validation.test.ts
  • apps/web/src/lib/ai/core/mcp-tool-converter.ts
**/*.{ts,tsx,js,jsx,json}

📄 CodeRabbit inference engine (AGENTS.md)

Format code with Prettier

Files:

  • apps/web/src/app/api/ai/global/[id]/messages/route.ts
  • apps/web/src/app/api/ai/chat/route.ts
  • apps/web/src/lib/ai/core/__tests__/mcp-tool-name-validation.test.ts
  • apps/web/src/lib/ai/core/mcp-tool-converter.ts
apps/web/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/web/src/**/*.{ts,tsx}: Use message parts structure for message content: { parts: [{ type: 'text', text: '...' }] }
For database access, always use Drizzle client from @pagespace/db: import { db, pages } from '@pagespace/db';
Use centralized Drizzle ORM with PostgreSQL for all database operations - no direct SQL or other ORMs
Use Socket.IO for real-time collaboration features - imported from the realtime service at port 3001
Use Vercel AI SDK with async/await for all AI operations and streaming
Use Next.js 15 App Router and TypeScript for all routes and components

Files:

  • apps/web/src/app/api/ai/global/[id]/messages/route.ts
  • apps/web/src/app/api/ai/chat/route.ts
  • apps/web/src/lib/ai/core/__tests__/mcp-tool-name-validation.test.ts
  • apps/web/src/lib/ai/core/mcp-tool-converter.ts
🧠 Learnings (2)
📚 Learning: 2025-12-16T19:03:59.870Z
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 91
File: apps/web/src/components/ai/shared/chat/tool-calls/CompactToolCallRenderer.tsx:253-277
Timestamp: 2025-12-16T19:03:59.870Z
Learning: In apps/web/src/components/ai/shared/chat/tool-calls/CompactToolCallRenderer.tsx (TypeScript/React), use the `getLanguageFromPath` utility from `formatters.ts` to infer syntax highlighting language from file paths instead of hardcoding language values in DocumentRenderer calls.

Applied to files:

  • apps/web/src/app/api/ai/global/[id]/messages/route.ts
  • apps/web/src/app/api/ai/chat/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to apps/processor/**/*.test.ts : Write unit tests for the processor service with test files named `*.test.ts` alongside source or in `__tests__/` directory

Applied to files:

  • apps/web/src/lib/ai/core/__tests__/mcp-tool-name-validation.test.ts
🧬 Code graph analysis (3)
apps/web/src/app/api/ai/global/[id]/messages/route.ts (1)
apps/web/src/lib/ai/core/mcp-tool-converter.ts (1)
  • sanitizeToolNamesForProvider (321-329)
apps/web/src/app/api/ai/chat/route.ts (1)
apps/web/src/lib/ai/core/mcp-tool-converter.ts (1)
  • sanitizeToolNamesForProvider (321-329)
apps/web/src/lib/ai/core/__tests__/mcp-tool-name-validation.test.ts (1)
apps/web/src/lib/ai/core/mcp-tool-converter.ts (4)
  • sanitizeToolName (302-304)
  • sanitizeToolNamesForProvider (321-329)
  • createSafeToolName (72-76)
  • parseMCPToolName (242-276)
🔇 Additional comments (10)
apps/web/src/lib/ai/core/mcp-tool-converter.ts (2)

285-304: LGTM! Well-documented sanitization utility.

The sanitizeToolName function correctly handles the colon-to-double-underscore transformation. The documentation clearly explains the rationale (provider compatibility) and how this integrates with the existing parseMCPToolName function that handles both formats.


306-329: LGTM! Generic helper preserves type safety.

The sanitizeToolNamesForProvider<T> function correctly transforms keys while preserving the tool definitions via reference. The generic type parameter ensures type safety is maintained for callers.

apps/web/src/lib/ai/core/__tests__/mcp-tool-name-validation.test.ts (4)

12-14: LGTM! Test imports updated correctly.

The new imports align with the exported functions from mcp-tool-converter.ts.


99-117: LGTM! Good coverage of sanitizeToolName edge cases.

Tests cover the key scenarios: basic colon replacement, multiple colons, idempotency (already sanitized names), and empty strings.


119-152: LGTM! sanitizeToolNamesForProvider tests verify key transformation and value preservation.

The test at line 146 correctly verifies that tool definitions are preserved by reference (using toBe), confirming no unnecessary object copies are made.


154-187: LGTM! Round-trip tests validate the sanitization/parsing integration.

The nested namespace test (lines 172-186) correctly documents that after sanitization, parseMCPToolName treats everything after the first __ delimiter as part of the tool name. This is expected behavior and ensures MCP tools remain executable after sanitization.

apps/web/src/app/api/ai/chat/route.ts (2)

50-51: LGTM! Import added for centralized sanitization.


579-582: LGTM! Centralized sanitization replaces provider-specific logic.

The change correctly applies sanitizeToolNamesForProvider to the merged tool set. Since sanitizeToolName only replaces colons, PageSpace tools (which use underscores, not colons) pass through unchanged. This ensures universal provider compatibility without affecting existing tool names.

The as any cast is acceptable here given the eslint-disable comment and the complexity of properly typing the merged tool definitions.

apps/web/src/app/api/ai/global/[id]/messages/route.ts (2)

32-33: LGTM! Import added for centralized sanitization.


671-674: LGTM! Consistent sanitization approach with Page AI route.

The implementation mirrors the Page AI route (apps/web/src/app/api/ai/chat/route.ts), ensuring consistent tool name handling across both route handlers. This eliminates provider-specific sanitization logic and centralizes it in the utility function.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants