MCP improvements for minimal context fetching#1139
Open
MathiasGruber wants to merge 5 commits intomainfrom
Open
MCP improvements for minimal context fetching#1139MathiasGruber wants to merge 5 commits intomainfrom
MathiasGruber wants to merge 5 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughRenames MCP public types/exports to ModelContextProtocol-*, adds response-filtering (select/search/maxLength) parsed from procedureArguments, validates inputs, and wires filters through handler → meta-tools to procedure execution; multiple forum, socials, utils, routing, and parsing modules refactored and new utilities added. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as ModelContextProtocolHandler
participant MetaTools as Meta-tools
participant TRPCCaller as TRPC Caller
Client->>Handler: POST with procedureArguments (input, select/search/maxLength)
Handler->>Handler: validate procedureArguments (isPlainObject)
Handler->>Handler: filters = parseResponseFilters(procedureArguments)
Handler->>MetaTools: handleCallEndpoint(registry, createTrpcCaller, endpointName, input, filters)
MetaTools->>MetaTools: findEndpoint & checkEndpointAuthorization
MetaTools->>TRPCCaller: resolveProcedure & invoke procedure with input
TRPCCaller->>MetaTools: return raw result
MetaTools->>MetaTools: applyFilters (select/search/maxLength) / handleTransformedResponse
MetaTools->>Handler: return formatted response or structured error
Handler->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 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 |
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[MCP Request] --> B{IP Rate Limit}
B -->|Pass| C[Extract Request Context]
C --> D{Token Present?}
D -->|Yes| E{Token Type?}
E -->|oat_| F[Verify Opaque Token via OAuth]
E -->|JWT| G[Verify JWT with Clerk]
F --> H{Valid?}
G --> H
H -->|Yes| I{User Rate Limit}
H -->|No| J[401 Unauthorized]
I -->|Pass| K[Create tRPC Context]
I -->|Fail| L[429 Too Many Requests]
K --> M{Tool Call}
M -->|listGameRouters| N[Return Router List]
M -->|listRouterEndpoints| O[Return Endpoint List]
M -->|getEndpointSchema| P[Return Schema]
M -->|callEndpoint| Q{Auth Check}
Q -->|Pass| R{Mutation?}
R -->|Yes| S{Has Write Scope?}
S -->|Yes| T[Execute Endpoint]
S -->|No| U[403 Forbidden]
R -->|No| T
T --> V{Response Filters?}
V -->|select| W[Extract Paths]
V -->|search| X[Filter by Pattern]
V -->|maxLength| Y[Apply Budget]
V -->|None| Z[Return Raw]
W --> Z
X --> Z
Y --> Z
D -->|No| K
Last reviewed commit: 6a65d85 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/src/libs/mcp/meta-tools.ts (3)
295-339: Minor optimization:lowerPatternis recomputed on each recursive call.The
pattern.toLowerCase()is called on every recursive invocation ofsearchObject. For deeply nested objects with many elements, this could add up. Consider passing the lowercased pattern as a parameter or extracting to a wrapper function.Suggested optimization
-const searchObject = (obj: unknown, pattern: string): unknown => { +const searchObject = (obj: unknown, pattern: string): unknown => { + const lowerPattern = pattern.toLowerCase(); + return searchObjectInternal(obj, lowerPattern); +}; + +const searchObjectInternal = (obj: unknown, lowerPattern: string): unknown => { if (obj === null || obj === undefined) return undefined; - const lowerPattern = pattern.toLowerCase(); - if (typeof obj === "string") { return obj.toLowerCase().includes(lowerPattern) ? obj : undefined; } // ... rest uses searchObjectInternal instead of searchObject🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/mcp/meta-tools.ts` around lines 295 - 339, The function searchObject repeatedly calls pattern.toLowerCase() per recursive invocation (stored in lowerPattern), causing unnecessary work; change the implementation to compute the lowercased pattern once and pass it down into recursive calls (e.g., add a second parameter like lowerPattern or create a wrapper function that calls searchObjectInternal with the precomputed lowerPattern), then update all recursive calls inside searchObject to use that precomputed lowerPattern and remove the per-call pattern.toLowerCase() computation.
460-472: Null-coalescing fallbackfilters.maxLength ?? 0is misleading.On line 466,
filters.maxLength ?? 0is used, but this code path is only entered whenfilters?.maxLengthis truthy (line 460). The?? 0fallback will never execute. While harmless, it adds confusion.Simplify by removing redundant fallback
if ("text" in block && typeof block.text === "string") { return { ...block, - text: truncateText(block.text, filters.maxLength ?? 0), + text: truncateText(block.text, filters.maxLength), }; }Note: This requires
filters.maxLengthto benumber(notnumber | undefined) in the narrowed type, which is guaranteed by the outerif (filters?.maxLength)check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/mcp/meta-tools.ts` around lines 460 - 472, The null-coalescing fallback filters.maxLength ?? 0 is redundant because the outer guard if (filters?.maxLength) guarantees a truthy number; update the mapping inside the if branch to pass filters.maxLength directly to truncateText (e.g., truncateText(block.text, filters.maxLength)), and if TypeScript complains about types, narrow or assert filters.maxLength as number (using a local const maxLength = filters.maxLength) before calling truncateText; reference the truncateText call in the return block and the filters.maxLength usage in this conditional block to locate the change.
341-348: Note: Truncated output exceedsmaxLengthdue to the suffix.The truncation indicator
...[truncated, X more chars]is appended after slicing tomaxLength, so the final string length exceeds the specified limit. If the intent is formaxLengthto cap the actual output length, the suffix length should be accounted for. If the current behavior is intentional (maxLength refers to content length), consider clarifying this in the schema description.Option to respect maxLength strictly
const truncateText = (text: string, maxLength: number): string => { if (text.length <= maxLength) return text; - const remaining = text.length - maxLength; - return `${text.slice(0, maxLength)}...[truncated, ${remaining} more chars]`; + const suffix = "...[truncated]"; + const contentLength = Math.max(0, maxLength - suffix.length); + return `${text.slice(0, contentLength)}${suffix}`; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/mcp/meta-tools.ts` around lines 341 - 348, truncateText currently slices to maxLength then appends the suffix, causing the final string to exceed maxLength; update truncateText to compute the suffix string (`...[truncated, ${remaining} more chars]`), subtract its length from maxLength to get available characters for slicing, handle the case where available <= 0 (e.g., return a truncated suffix or the suffix trimmed to maxLength), then slice the original text to `available` and append the full suffix so the returned string length does not exceed maxLength; refer to the truncateText function and the suffix construction when making the changes.app/src/libs/mcp/handler.ts (1)
131-143: Edge case: emptyselectarray passes as having filters.If
args.selectis an empty array[], after filtering it will still be an empty array assigned tofilters.select. On line 143,filters.select(an empty array) is truthy, sohasFiltersbecomestrue, and an empty filters object withselect: []is passed tohandleCallEndpoint. This is likely harmless sinceselectFieldswith empty paths returns{}, but it's inconsistent with the intent of only passing filters when meaningful filtering is requested.Suggested fix to check array length
const filters: ResponseFilters = {}; if (Array.isArray(args?.select)) { - filters.select = args.select.filter( + const selected = args.select.filter( (s): s is string => typeof s === "string", ); + if (selected.length > 0) { + filters.select = selected; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/mcp/handler.ts` around lines 131 - 143, The code treats an empty args.select array as a meaningful filter because filters.select gets assigned [] and thus makes hasFilters truthy; update the logic in handler.ts so filters.select is only set or considered when it contains at least one string (e.g., assign filters.select = args.select.filter(...) and only treat it as a filter if filters.select.length > 0), and compute hasFilters using the presence/length check (filters.select?.length) instead of a truthy check; ensure the downstream call that uses hasFilters (handleCallEndpoint) receives filters only when select is non-empty or other filters are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/libs/mcp/meta-tools.ts`:
- Around line 456-481: The transformed endpoint branch only applies truncateText
for filters.maxLength and skips select/search; after obtaining result from
foundEndpoint.transformMcpProcedure(output) pass the transformed blocks through
the same filtering pipeline as non-transformed responses (i.e., use applyFilters
or an equivalent block-aware filter function) so select/search are applied, then
map over the filtered blocks to apply truncateText when filters.maxLength is
present; update the logic around foundEndpoint.transformMcpProcedure, procedure,
applyFilters, and truncateText to ensure filtering happens before truncation and
the returned shape remains { content: [...] }.
---
Nitpick comments:
In `@app/src/libs/mcp/handler.ts`:
- Around line 131-143: The code treats an empty args.select array as a
meaningful filter because filters.select gets assigned [] and thus makes
hasFilters truthy; update the logic in handler.ts so filters.select is only set
or considered when it contains at least one string (e.g., assign filters.select
= args.select.filter(...) and only treat it as a filter if filters.select.length
> 0), and compute hasFilters using the presence/length check
(filters.select?.length) instead of a truthy check; ensure the downstream call
that uses hasFilters (handleCallEndpoint) receives filters only when select is
non-empty or other filters are present.
In `@app/src/libs/mcp/meta-tools.ts`:
- Around line 295-339: The function searchObject repeatedly calls
pattern.toLowerCase() per recursive invocation (stored in lowerPattern), causing
unnecessary work; change the implementation to compute the lowercased pattern
once and pass it down into recursive calls (e.g., add a second parameter like
lowerPattern or create a wrapper function that calls searchObjectInternal with
the precomputed lowerPattern), then update all recursive calls inside
searchObject to use that precomputed lowerPattern and remove the per-call
pattern.toLowerCase() computation.
- Around line 460-472: The null-coalescing fallback filters.maxLength ?? 0 is
redundant because the outer guard if (filters?.maxLength) guarantees a truthy
number; update the mapping inside the if branch to pass filters.maxLength
directly to truncateText (e.g., truncateText(block.text, filters.maxLength)),
and if TypeScript complains about types, narrow or assert filters.maxLength as
number (using a local const maxLength = filters.maxLength) before calling
truncateText; reference the truncateText call in the return block and the
filters.maxLength usage in this conditional block to locate the change.
- Around line 341-348: truncateText currently slices to maxLength then appends
the suffix, causing the final string to exceed maxLength; update truncateText to
compute the suffix string (`...[truncated, ${remaining} more chars]`), subtract
its length from maxLength to get available characters for slicing, handle the
case where available <= 0 (e.g., return a truncated suffix or the suffix trimmed
to maxLength), then slice the original text to `available` and append the full
suffix so the returned string length does not exceed maxLength; refer to the
truncateText function and the suffix construction when making the changes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/libs/mcp/meta-tools.ts`:
- Around line 388-392: truncateText currently appends an indicator after slicing
to maxLength, so the returned string can exceed the requested cap; change
truncateText to ensure the final returned string length is <= maxLength by
computing the indicator (e.g., "...[truncated, N more chars]"), and if
truncation is needed slice the input to maxLength - indicator.length (handling
the edge case when maxLength is smaller than the indicator by returning a
trimmed indicator or a hard-shortened fallback) so the combined slice+indicator
never exceeds maxLength.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/src/layout/FancyForumThreads.tsx (1)
4-8: Constant defined between import statements.The
FANCY_FORUM_THREADS_PER_PAGEconstant is placed between thelucide-reactimport and other imports, which is unconventional. Consider moving it after all imports for cleaner organization.📝 Suggested: move constant after imports
"use client"; import { MessagesSquare, SquarePen } from "lucide-react"; - -// Number of forum threads to display per page in fancy forum component -const FANCY_FORUM_THREADS_PER_PAGE = 10; - import Link from "next/link"; import { useState } from "react"; import { api } from "@/app/_trpc/client"; import { Button } from "@/components/ui/button"; import ContentBox from "@/layout/ContentBox"; import Image from "@/layout/Image"; import Loader from "@/layout/Loader"; import type { InfiniteThreads } from "@/libs/forum"; import { useInfinitePagination } from "@/libs/pagination"; import { parseHtml } from "@/utils/parse"; + +// Number of forum threads to display per page in fancy forum component +const FANCY_FORUM_THREADS_PER_PAGE = 10;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/layout/FancyForumThreads.tsx` around lines 4 - 8, Move the top-level constant FANCY_FORUM_THREADS_PER_PAGE out of the import block and place it after all import statements (e.g., below the last import in FancyForumThreads.tsx); locate the constant definition and cut/paste it so that all imports (including lucide-react, next/link, etc.) are grouped together at the top and then declare FANCY_FORUM_THREADS_PER_PAGE before the FancyForumThreads component or related functions.app/src/libs/forum.ts (1)
78-90: Fragile error detection via string matching.The check
error.message?.includes("not found")is brittle—message text could change or vary. Consider checking error type/code or using a more robust pattern.♻️ Suggested: check error code if available
const userDataPromise = userId ? fetchUser(client, userId).catch((error) => { - // Check if it's an expected "not found" error - if (error.message?.includes("not found")) { + // Check if it's an expected "not found" error (TRPCError with NOT_FOUND code) + if (error.code === "NOT_FOUND" || error.message?.includes("not found")) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/forum.ts` around lines 78 - 90, The current fragile check in the fetchUser catch block (inside userDataPromise in fetchUser/client flow) uses error.message?.includes("not found"); replace this with a robust check: inspect a standard error property such as error.code (e.g., "NOT_FOUND" or "404") or check the error class (e.g., instanceof NotFoundError) returned by fetchUser, and only treat those cases as a missing user returning null; for all other errors keep the Sentry.captureException and console.error behavior. If fetchUser can return different error shapes, add a small helper (e.g., isNotFoundError) used in that catch to centralize checks (code, status, or class) and avoid brittle string matching.app/src/utils/string.ts (1)
46-54: Edge case: output can exceedmaximumLengthwhen indicator is longer.When
maximumLength < indicator.length, the function returns just the indicator, which exceeds the specified max. Consider whether callers expect strict adherence to the limit.💡 Optional: truncate indicator itself when it exceeds maximumLength
export const truncateString = ( text: string, maximumLength: number, indicator: string = "...", ): string => { if (text.length <= maximumLength) return text; + if (maximumLength <= 0) return ""; + if (indicator.length >= maximumLength) return indicator.slice(0, maximumLength); const maxTextLength = Math.max(0, maximumLength - indicator.length); return `${text.slice(0, maxTextLength)}${indicator}`; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/utils/string.ts` around lines 46 - 54, The truncateString implementation can produce output longer than maximumLength when indicator.length > maximumLength; update truncateString to ensure the final string never exceeds maximumLength by first computing an allowedIndicator (if indicator.length > maximumLength then use indicator.slice(0, maximumLength) else use indicator), then compute maxTextLength = Math.max(0, maximumLength - allowedIndicator.length) and return `${text.slice(0, maxTextLength)}${allowedIndicator}` (this also covers negative/zero maximumLength cases by returning an appropriate truncated indicator or empty string).app/src/server/api/routers/forum.ts (1)
94-134: Misleading variable name:databaseMutationsincludes non-database operations.The
databaseMutationsarray includes social media API calls (callDiscordNews,callFacebookNews,callRedditNews,callTwitterNews,callInstagramNews), which are not database mutations. This naming is misleading and could confuse future maintainers.♻️ Suggested rename for clarity
- const databaseMutations: Promise<unknown>[] = [ + const pendingOperations: Promise<unknown>[] = [ moderateContent(ctx.drizzle, { content: sanitized, userId: ctx.userId, @@ -119,16 +119,16 @@ .where(eq(forumBoard.id, input.board_id)), ]; if (isNews) { - databaseMutations.push( + pendingOperations.push( callDiscordNews(input.title, input.content, user.avatar), callFacebookNews(input.title, input.content), callRedditNews(input.title, input.content), callTwitterNews(input.title, input.content), ); // Only post to Instagram if an image is attached if (input.image) { - databaseMutations.push( + pendingOperations.push( callInstagramNews(input.title, input.content, input.image), ); } } - await Promise.all(databaseMutations); + await Promise.all(pendingOperations);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/forum.ts` around lines 94 - 134, The variable databaseMutations currently mixes real DB operations (moderateContent, ctx.drizzle.insert/update of forumThread/forumPost/forumBoard) with external API calls (callDiscordNews, callFacebookNews, callRedditNews, callTwitterNews, callInstagramNews); rename and split responsibilities by creating two arrays (e.g., databaseMutations for moderateContent and ctx.drizzle.* calls and externalCalls for callDiscordNews/callFacebookNews/callRedditNews/callTwitterNews/callInstagramNews), push the social media calls into externalCalls (only include callInstagramNews when input.image exists), then await them appropriately (e.g., await Promise.all(databaseMutations) and await Promise.all(externalCalls) or await Promise.all([...databaseMutations, ...externalCalls]) depending on desired concurrency).app/src/app/forum/[boardid]/page.tsx (1)
4-6: Consider moving constant to a shared location.The
FORUM_BOARD_THREADS_PER_PAGEconstant is defined inline. If this value is used in other forum-related pages or could be reused, consider moving it to@/drizzle/constants.tsfor consistency with the coding guidelines that prefer importing constants for game-related values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/app/forum/`[boardid]/page.tsx around lines 4 - 6, FORUM_BOARD_THREADS_PER_PAGE is defined inline; move it into the shared forum/game constants file (create or update constants.ts used by forum-related modules) and export it, then replace the inline constant with an import of FORUM_BOARD_THREADS_PER_PAGE in page.tsx (ensure the exported name matches and update any other forum pages to import the constant instead of redefining it).app/src/libs/mcp/meta-tools.ts (2)
479-505: Consider adding a recursion depth limit tosearchObject.While
resolvePathWithWildcardshas a depth limit of 20,searchObjectrecurses without explicit bounds. Deeply nested response objects could cause stack overflow. This is lower risk since tRPC responses are typically bounded, but adding a depth parameter would provide consistent protection.♻️ Suggested enhancement
-const searchObject = (value: unknown, pattern: string): unknown => { +const MAXIMUM_SEARCH_DEPTH = 20; + +const searchObject = ( + value: unknown, + pattern: string, + depth: number = 0, +): unknown => { + if (depth > MAXIMUM_SEARCH_DEPTH) return undefined; + const lowercasePattern = pattern.toLowerCase(); // ... rest of function if (Array.isArray(value)) { const filteredElements = value - .map((element) => searchObject(element, pattern)) + .map((element) => searchObject(element, pattern, depth + 1)) .filter((filtered) => filtered !== undefined); // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/mcp/meta-tools.ts` around lines 479 - 505, searchObject currently recurses without a depth bound which risks stack overflows on deeply nested inputs; add an optional depth parameter (e.g., depth = 0) and a maxDepth constant (match resolvePathWithWildcards' 20) to the searchObject signature, increment depth on each recursive call (including when recursing into Array elements and when calling filterObjectEntries for objects), and short-circuit to undefined if depth >= maxDepth; update any internal calls (map -> searchObject(element, depth + 1), searchObject(value, depth + 1)) and ensure matchesPrimitive and filterObjectEntries usage remains unchanged except passing/incrementing the depth where searchObject is invoked.
289-291: Consider adding a length limit for the search pattern.The
searchparameter accepts any non-empty string without a maximum length constraint. Very long search patterns could impact performance during recursive filtering. Consider adding a reasonable limit similar toMAXIMUM_SELECT_PATHS.♻️ Suggested enhancement
+const MAXIMUM_SEARCH_LENGTH = 256; + if (typeof procedureArguments?.search === "string" && procedureArguments.search) { - filters.search = procedureArguments.search; + filters.search = procedureArguments.search.slice(0, MAXIMUM_SEARCH_LENGTH); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/mcp/meta-tools.ts` around lines 289 - 291, The search string assignment currently accepts any non-empty string (procedureArguments?.search) which can be arbitrarily long; add a length guard using a defined limit (reuse or define a constant similar to MAXIMUM_SELECT_PATHS, e.g. MAXIMUM_SEARCH_PATTERN_LENGTH) and either truncate or reject values exceeding that limit before assigning to filters.search. Update the check around procedureArguments?.search to enforce the limit and ensure any error or truncation behaviour is consistent with existing validation patterns in this module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/app/api/mcp/`[[...transport]]/route.ts:
- Around line 86-95: The domain validation for frontendApiHost in the
isValidClerkDomain check only covers Clerk-hosted dev and local patterns and
mislabels the first regex as "Production"; update the validation to include a
production/custom-domain pattern (e.g., a regex that allows subdomains like
clerk.mycompany.com) and correct the comment. Specifically, adjust the
isValidClerkDomain logic (the variable named isValidClerkDomain used with
frontendApiHost) to OR in a third pattern that matches production custom domains
(for example a /^clerk\.[a-z0-9.-]+$/-style pattern), and update the surrounding
comment to clearly document which regex covers production, dev, and local
development.
---
Nitpick comments:
In `@app/src/app/forum/`[boardid]/page.tsx:
- Around line 4-6: FORUM_BOARD_THREADS_PER_PAGE is defined inline; move it into
the shared forum/game constants file (create or update constants.ts used by
forum-related modules) and export it, then replace the inline constant with an
import of FORUM_BOARD_THREADS_PER_PAGE in page.tsx (ensure the exported name
matches and update any other forum pages to import the constant instead of
redefining it).
In `@app/src/layout/FancyForumThreads.tsx`:
- Around line 4-8: Move the top-level constant FANCY_FORUM_THREADS_PER_PAGE out
of the import block and place it after all import statements (e.g., below the
last import in FancyForumThreads.tsx); locate the constant definition and
cut/paste it so that all imports (including lucide-react, next/link, etc.) are
grouped together at the top and then declare FANCY_FORUM_THREADS_PER_PAGE before
the FancyForumThreads component or related functions.
In `@app/src/libs/forum.ts`:
- Around line 78-90: The current fragile check in the fetchUser catch block
(inside userDataPromise in fetchUser/client flow) uses
error.message?.includes("not found"); replace this with a robust check: inspect
a standard error property such as error.code (e.g., "NOT_FOUND" or "404") or
check the error class (e.g., instanceof NotFoundError) returned by fetchUser,
and only treat those cases as a missing user returning null; for all other
errors keep the Sentry.captureException and console.error behavior. If fetchUser
can return different error shapes, add a small helper (e.g., isNotFoundError)
used in that catch to centralize checks (code, status, or class) and avoid
brittle string matching.
In `@app/src/libs/mcp/meta-tools.ts`:
- Around line 479-505: searchObject currently recurses without a depth bound
which risks stack overflows on deeply nested inputs; add an optional depth
parameter (e.g., depth = 0) and a maxDepth constant (match
resolvePathWithWildcards' 20) to the searchObject signature, increment depth on
each recursive call (including when recursing into Array elements and when
calling filterObjectEntries for objects), and short-circuit to undefined if
depth >= maxDepth; update any internal calls (map -> searchObject(element, depth
+ 1), searchObject(value, depth + 1)) and ensure matchesPrimitive and
filterObjectEntries usage remains unchanged except passing/incrementing the
depth where searchObject is invoked.
- Around line 289-291: The search string assignment currently accepts any
non-empty string (procedureArguments?.search) which can be arbitrarily long; add
a length guard using a defined limit (reuse or define a constant similar to
MAXIMUM_SELECT_PATHS, e.g. MAXIMUM_SEARCH_PATTERN_LENGTH) and either truncate or
reject values exceeding that limit before assigning to filters.search. Update
the check around procedureArguments?.search to enforce the limit and ensure any
error or truncation behaviour is consistent with existing validation patterns in
this module.
In `@app/src/server/api/routers/forum.ts`:
- Around line 94-134: The variable databaseMutations currently mixes real DB
operations (moderateContent, ctx.drizzle.insert/update of
forumThread/forumPost/forumBoard) with external API calls (callDiscordNews,
callFacebookNews, callRedditNews, callTwitterNews, callInstagramNews); rename
and split responsibilities by creating two arrays (e.g., databaseMutations for
moderateContent and ctx.drizzle.* calls and externalCalls for
callDiscordNews/callFacebookNews/callRedditNews/callTwitterNews/callInstagramNews),
push the social media calls into externalCalls (only include callInstagramNews
when input.image exists), then await them appropriately (e.g., await
Promise.all(databaseMutations) and await Promise.all(externalCalls) or await
Promise.all([...databaseMutations, ...externalCalls]) depending on desired
concurrency).
In `@app/src/utils/string.ts`:
- Around line 46-54: The truncateString implementation can produce output longer
than maximumLength when indicator.length > maximumLength; update truncateString
to ensure the final string never exceeds maximumLength by first computing an
allowedIndicator (if indicator.length > maximumLength then use
indicator.slice(0, maximumLength) else use indicator), then compute
maxTextLength = Math.max(0, maximumLength - allowedIndicator.length) and return
`${text.slice(0, maxTextLength)}${allowedIndicator}` (this also covers
negative/zero maximumLength cases by returning an appropriate truncated
indicator or empty string).
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+140
to
+155
| const counterUpdates: Promise<unknown>[] = []; | ||
| if (isNews) { | ||
| counterUpdates.push( | ||
| ctx.drizzle | ||
| .update(userData) | ||
| .set({ unreadNews: sql`LEAST(unreadNews + 1, 1000)` }) | ||
| .where(ne(userData.userId, effectiveUserId)), | ||
| ...publishNewsToSocialMedia( | ||
| input.title, | ||
| input.content, | ||
| user.avatar, | ||
| input.image, | ||
| ), | ||
| ); | ||
| } | ||
| await Promise.all(counterUpdates); |
There was a problem hiding this comment.
unreadNews counter increment was removed but never restored
The old code incremented unreadNews for all users when news was created. This was removed during refactoring and not re-added in the new counterUpdates block. Now when news posts are created, users will no longer see their unreadNews counter increment, breaking the notification badge in the navbar.
Suggested change
| const counterUpdates: Promise<unknown>[] = []; | |
| if (isNews) { | |
| counterUpdates.push( | |
| ctx.drizzle | |
| .update(userData) | |
| .set({ unreadNews: sql`LEAST(unreadNews + 1, 1000)` }) | |
| .where(ne(userData.userId, effectiveUserId)), | |
| ...publishNewsToSocialMedia( | |
| input.title, | |
| input.content, | |
| user.avatar, | |
| input.image, | |
| ), | |
| ); | |
| } | |
| await Promise.all(counterUpdates); | |
| const counterUpdates: Promise<unknown>[] = []; | |
| if (isNews) { | |
| counterUpdates.push( | |
| ctx.drizzle | |
| .update(userData) | |
| .set({ unreadNews: sql`LEAST(unreadNews + 1, 1000)` }) | |
| .where(ne(userData.userId, effectiveUserId)), | |
| ...publishNewsToSocialMedia( | |
| input.title, | |
| input.content, | |
| user.avatar, | |
| input.image, | |
| ), | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/server/api/routers/forum.ts
Line: 140-155
Comment:
`unreadNews` counter increment was removed but never restored
The old code incremented `unreadNews` for all users when news was created. This was removed during refactoring and not re-added in the new `counterUpdates` block. Now when news posts are created, users will no longer see their unreadNews counter increment, breaking the notification badge in the navbar.
```suggestion
const counterUpdates: Promise<unknown>[] = [];
if (isNews) {
counterUpdates.push(
ctx.drizzle
.update(userData)
.set({ unreadNews: sql`LEAST(unreadNews + 1, 1000)` })
.where(ne(userData.userId, effectiveUserId)),
...publishNewsToSocialMedia(
input.title,
input.content,
user.avatar,
input.image,
),
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/src/server/api/routers/forum.ts (1)
121-127: Redundant WHERE condition doesn't provide the stated guard.The condition
eq(forumBoard.id, board.id)is always true whenboardwas fetched usinginput.board_id(line 75). This doesn't actually guard against the board being deleted between fetch and update—if the board is deleted, the UPDATE simply won't match any rows regardless of this extra condition.The
rowsAffected === 0check on line 132 is the actual guard here and works correctly. Consider simplifying the WHERE clause or updating the comment to reflect what actually happens.Suggested simplification
ctx.drizzle .update(forumBoard) .set({ nThreads: sql`nThreads + 1`, updatedAt: new Date() }) - .where( - and( - eq(forumBoard.id, input.board_id), - // Guard: Only update if board still exists (id matches the pre-check) - eq(forumBoard.id, board.id), - ), - ), + .where(eq(forumBoard.id, input.board_id)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/forum.ts` around lines 121 - 127, The WHERE clause in the update using forumBoard contains a redundant condition eq(forumBoard.id, board.id) because board was previously fetched by input.board_id; remove that redundant eq condition from the .where(and(...)) call (leave only eq(forumBoard.id, input.board_id)), and update the inline comment near that .where invocation to state that the actual concurrency guard is the subsequent rowsAffected === 0 check (which detects no-rows-updated if the board was deleted concurrently) rather than the removed equality. Ensure references to forumBoard, input.board_id, board, and the rowsAffected check remain intact so the intent is clear.app/src/libs/mcp/tools.ts (1)
13-19: Prefer arrow-function exports for these helpers.
The guidelines call for arrow functions over function declarations in TS files; behavior stays the same.As per coding guidelines: "Prefer arrow functions over function declarations (e.g., `const fn = () => {}` instead of `function fn() {}`)".♻️ Suggested refactor
-export function mergeInputs( - inputs: z.ZodObject<z.core.$ZodLooseShape>[], -): z.ZodObject<z.core.$ZodLooseShape, z.core.$strip> { +export const mergeInputs = ( + inputs: z.ZodObject<z.core.$ZodLooseShape>[], +): z.ZodObject<z.core.$ZodLooseShape, z.core.$strip> => { return inputs.reduce((acc, input) => { return acc.extend(input.shape); }, z.object({})); -} +};-export function extractToolsFromProcedures< +export const extractToolsFromProcedures = < TRoot extends AnyRootTypes, TRecord extends RouterRecord, >( appRouter: Router<TRoot, TRecord>, currentPath: string[] = [], -): ModelContextProtocolTool[] { +): ModelContextProtocolTool[] => { const tools: ModelContextProtocolTool[] = []; const procedures = Object.entries(appRouter._def.procedures); // ... -} +};Also applies to: 29-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/mcp/tools.ts` around lines 13 - 19, Convert the exported function declarations to arrow-function exports: replace the function mergeInputs(...) { ... } declaration with a const mergeInputs = (...) => { ... } exported (export const mergeInputs = ...), preserving the implementation (reduce, z.object({}), acc.extend(input.shape)) and types (inputs: z.ZodObject<...>[], return type z.ZodObject<...>). Do the same refactor for the other helper(s) in this file mentioned in the comment (the helper(s) in the later block around the second helper, currently declared with function ...), keeping their names, parameter types, return types, and exported visibility unchanged. Ensure no behavioral changes and update any references/imports if necessary.app/src/libs/mcp/meta-tools.ts (1)
79-101: Consider arrow function style.The coding guidelines prefer arrow functions over function declarations. This is a minor style consistency issue.
♻️ Suggested refactor
-export function buildToolRegistry(tools: ModelContextProtocolTool[]): ToolRegistry { +export const buildToolRegistry = (tools: ModelContextProtocolTool[]): ToolRegistry => { const routers = new Map<string, Map<string, EndpointData>>(); // ... rest of function -} +};As per coding guidelines: "Prefer arrow functions over function declarations (e.g.,
const fn = () => {}instead offunction fn() {})"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/mcp/meta-tools.ts` around lines 79 - 101, The function buildToolRegistry is declared with a function declaration rather than the project's preferred arrow function style; rewrite it as a const initialized arrow function (e.g., const buildToolRegistry = (tools: ModelContextProtocolTool[]): ToolRegistry => { ... }) while preserving existing logic and return shape (routers Map and EndpointData entries), keeping the same exported name and types (ToolRegistry, ModelContextProtocolTool) and ensuring all references to buildToolRegistry continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/app/api/daily-link-cleaner/route.ts`:
- Around line 22-52: processContentUrls currently skips removing text
occurrences when the same URL also appears inside an <img> because
isWithinImgTag only triggers the fullTag replacement and the plain url
replacement may be skipped; modify the loop in processContentUrls so that for
each UrlCheckResult where keep is false you (1) if isImg && fullTag replace the
fullTag in newContent, and then (2) always remove any remaining plain
occurrences of the URL in newContent by replacing all instances (use a global
replacement, e.g. build a safe RegExp from the URL via an escapeRegExp helper)
instead of a single .replace; reference processContentUrls, isWithinImgTag,
UrlCheckResult and newContent when making this change.
In `@app/src/utils/url.ts`:
- Around line 9-66: isUrlSafe currently only checks textual hostnames and simple
dotted IPv4/IPv6 prefixes, allowing IP-literal variants (e.g.
[::ffff:127.0.0.1]), integer/hex IPv4 forms (e.g. 2130706433), and hostnames
that DNS-resolve to private addresses to bypass checks. Update isUrlSafe to:
normalize the URL host by removing IPv6 brackets and decoding numeric IPv4 forms
(use a robust IP parser like ipaddr.js or equivalent to canonicalize IPv4/IPv6
literals and detect mapped IPv4-in-IPv6); use net.isIP/ipaddr to detect literals
and validate against private/loopback/cloud ranges; for non-literal hostnames
perform DNS resolution (dns.promises.lookup or lookupAll) and validate every
resolved address is public before allowing; make isUrlSafe async if you perform
DNS lookups and name the function isUrlSafe (or export an async variant) so
callers are updated accordingly. Ensure checks still enforce only http/https
protocols.
---
Nitpick comments:
In `@app/src/libs/mcp/meta-tools.ts`:
- Around line 79-101: The function buildToolRegistry is declared with a function
declaration rather than the project's preferred arrow function style; rewrite it
as a const initialized arrow function (e.g., const buildToolRegistry = (tools:
ModelContextProtocolTool[]): ToolRegistry => { ... }) while preserving existing
logic and return shape (routers Map and EndpointData entries), keeping the same
exported name and types (ToolRegistry, ModelContextProtocolTool) and ensuring
all references to buildToolRegistry continue to work.
In `@app/src/libs/mcp/tools.ts`:
- Around line 13-19: Convert the exported function declarations to
arrow-function exports: replace the function mergeInputs(...) { ... }
declaration with a const mergeInputs = (...) => { ... } exported (export const
mergeInputs = ...), preserving the implementation (reduce, z.object({}),
acc.extend(input.shape)) and types (inputs: z.ZodObject<...>[], return type
z.ZodObject<...>). Do the same refactor for the other helper(s) in this file
mentioned in the comment (the helper(s) in the later block around the second
helper, currently declared with function ...), keeping their names, parameter
types, return types, and exported visibility unchanged. Ensure no behavioral
changes and update any references/imports if necessary.
In `@app/src/server/api/routers/forum.ts`:
- Around line 121-127: The WHERE clause in the update using forumBoard contains
a redundant condition eq(forumBoard.id, board.id) because board was previously
fetched by input.board_id; remove that redundant eq condition from the
.where(and(...)) call (leave only eq(forumBoard.id, input.board_id)), and update
the inline comment near that .where invocation to state that the actual
concurrency guard is the subsequent rowsAffected === 0 check (which detects
no-rows-updated if the board was deleted concurrently) rather than the removed
equality. Ensure references to forumBoard, input.board_id, board, and the
rowsAffected check remain intact so the intent is clear.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (text.length <= maximumLength) return text; | ||
| const maxTextLength = Math.max(0, maximumLength - indicator.length); | ||
| return `${text.slice(0, maxTextLength)}${indicator}`; | ||
| }; |
There was a problem hiding this comment.
truncateString can exceed maxLength when budget is small
Medium Severity
truncateString violates its documented contract ("Maximum length including the indicator") when maximumLength is less than indicator.length. For example, with "...[truncated]" (14 chars) as the indicator and a remaining budget of 10, maxTextLength becomes max(0, 10-14) = 0, producing just "...[truncated]" — 14 chars, exceeding the 10-char budget. This is realistic in applyLengthBudgetToBlocks where earlier blocks consume most of the budget, leaving a small remainder for the final block.
Additional Locations (1)
| ctx.drizzle | ||
| .update(userData) | ||
| .set({ unreadNews: sql`LEAST(unreadNews + 1, 1000)` }) | ||
| .where(ne(userData.userId, effectiveUserId)), |
There was a problem hiding this comment.
Unread news exclusion uses sender instead of poster
Low Severity
The unread news counter update excludes effectiveUserId (the display sender, which could be an AI account) instead of ctx.userId (the actual human who performed the action). When a staff member posts news as an AI user, the AI account gets excluded from the notification bump while the actual poster — who already knows about the news — still receives the unread increment.
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.
Pull Request
Please fill: what was implemented, why, possibly include screenshots, are any of the changes breaking, etc.
License
By making this pull request, I confirm that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the Studie-Tech ApS organization has the copyright to use and modify my contribution for perpetuity.
Note
High Risk
Touches auth/token verification and remote-calling surfaces (MCP, webhook posting, URL fetching), where small mistakes can cause security issues or break integrations. Also changes forum API inputs and pagination behavior, which can impact multiple pages/clients.
Overview
Improves the MCP API by hardening token verification (JWT + opaque), adding IP and per-user rate limiting, tightening scope handling for mutations, and expanding
callEndpointto support response filters (select,search,maxLength) with safer input/path validation and better error reporting (incl. Sentry capture for unexpected failures).Refactors forum thread fetching into a new
libs/forummodule, standardizes pagination limits via constants, updatesgetThreadsinput naming (boardId/boardName), and tweaks forum UI to better reflect pending mutation states (disabled actions, empty state). Adds safer URL accessibility checks with SSRF protection and sharedfetchWithTimeout/getClientIputilities, and strengthens social integrations with Discord webhook validation plus shared markdown conversion/truncation helpers.Written by Cursor Bugbot for commit 6a65d85. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements