Repository navigation
Adjust MCP search response sizing defaults - #107
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 44 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThese changes add a recursive schema compressor for LLM-friendly payloads, integrate it into capability search rows, and refactor the search tool to serialize and trim results by serialized payload size with a configurable maxResponseSize and dynamic limit defaulting to detail mode. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
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. Comment |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
adf7b8b to
dca8447
Compare
|
🔎 Preview deployed: https://kody-pr-107.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/worker/src/mcp/capabilities/schema-compression.node.test.ts (1)
4-70: Consider adding edge case tests for nested structures and null inputs.The current tests cover the primary use cases well. For more robust coverage, consider adding tests for:
- Nested object schemas (properties containing objects with their own properties)
- Array schemas with
itemsnull/undefinedinput passthrough- Composition keywords (
allOf,anyOf)These can be deferred if the current coverage is sufficient for your needs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/schema-compression.node.test.ts` around lines 4 - 70, Add additional unit tests for compressSchemaForLlm to cover edge cases: add a test for nested object schemas (a property whose value is an object with its own properties and required fields), a test for array schemas (properties with type: 'array' and an items schema that should be compressed), a test that passing null/undefined returns the input unchanged or handles it gracefully, and a test for composition keywords (e.g., an allOf/anyOf array containing schemas) to assert compression behaves correctly; reference compressSchemaForLlm in the existing schema-compression.node.test.ts and create one focused test case per bullet to ensure each edge case is validated.packages/worker/src/mcp/capabilities/schema-compression.ts (1)
29-133: Consider extending support for schema composition keywords.The recursive traversal correctly handles
properties,definitions,$defs,patternProperties, anditems. However, schema composition keywords likeallOf,anyOf,oneOf,not,if,then, andelseare processed by the generic recursive path (lines 125-129), which means nested schemas within these keywords won't have theirpropertyNamecontext set.This is unlikely to cause issues in practice since these nested schemas rarely have redundant titles matching a parent property name, but worth noting for completeness if you encounter uncompressed metadata in complex schemas later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/schema-compression.ts` around lines 29 - 133, The compressSchemaNode traversal currently treats composition keywords (allOf, anyOf, oneOf, not, if, then, else) via the generic fallback, so nested schemas there never get the correct propertyName context; update compressSchemaNode to explicitly handle those keys: for array-valued keywords (allOf, anyOf, oneOf) map each item through compressSchemaNode with { isRoot: false, propertyName: undefined, stripRootObjectType: false }, and for object-valued keywords (not, if, then, else) call compressSchemaNode on their value with the same context; keep using the existing context fields (isRoot, propertyName, stripRootObjectType) and the same pattern used for items/properties handling so titles/descriptions inside composed schemas are correctly compressed.packages/worker/src/mcp/tools/search.ts (1)
59-79: Use binary search formatchestrimming to avoid repeated full serialization.Current trimming removes one match at a time and re-serializes on every iteration. With large detailed schemas, that can be unnecessarily expensive. A binary search over prefix length keeps behavior the same and reduces serialization passes.
♻️ Proposed refactor
function applyMaxResponseSize( payload: SearchPayload, maxResponseSize: number, ): { payload: SearchPayload; serialized: string } { if (!Number.isFinite(maxResponseSize) || maxResponseSize <= 0) { const serialized = stringifySearchPayload(payload) return { payload, serialized } } - let matches = payload.matches - let trimmedPayload: SearchPayload = { ...payload, matches } - let serialized = stringifySearchPayload(trimmedPayload) - - while (serialized.length > maxResponseSize && matches.length > 0) { - matches = matches.slice(0, -1) - trimmedPayload = { ...payload, matches } - serialized = stringifySearchPayload(trimmedPayload) - } - - return { payload: trimmedPayload, serialized } + const allMatches = payload.matches + let bestPayload: SearchPayload = { ...payload, matches: allMatches } + let bestSerialized = stringifySearchPayload(bestPayload) + if (bestSerialized.length <= maxResponseSize) { + return { payload: bestPayload, serialized: bestSerialized } + } + + let low = 0 + let high = allMatches.length + while (low <= high) { + const mid = Math.floor((low + high) / 2) + const candidatePayload: SearchPayload = { + ...payload, + matches: allMatches.slice(0, mid), + } + const candidateSerialized = stringifySearchPayload(candidatePayload) + + if (candidateSerialized.length <= maxResponseSize) { + bestPayload = candidatePayload + bestSerialized = candidateSerialized + low = mid + 1 + } else { + high = mid - 1 + } + } + + return { payload: bestPayload, serialized: bestSerialized } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search.ts` around lines 59 - 79, The current applyMaxResponseSize function trims payload.matches one at a time and re-serializes each loop; replace that linear trimming with a binary search over the prefix length of payload.matches to find the largest prefix whose serialized size is <= maxResponseSize. Keep using stringifySearchPayload for size checks, operate on trimmedPayload = { ...payload, matches: payload.matches.slice(0, k) } during checks, and return the final trimmed payload and its serialized string; ensure you still handle non-finite/<=0 maxResponseSize the same way and that edge cases (0 matches fit or none fit) are handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/schema-compression.node.test.ts`:
- Around line 4-70: Add additional unit tests for compressSchemaForLlm to cover
edge cases: add a test for nested object schemas (a property whose value is an
object with its own properties and required fields), a test for array schemas
(properties with type: 'array' and an items schema that should be compressed), a
test that passing null/undefined returns the input unchanged or handles it
gracefully, and a test for composition keywords (e.g., an allOf/anyOf array
containing schemas) to assert compression behaves correctly; reference
compressSchemaForLlm in the existing schema-compression.node.test.ts and create
one focused test case per bullet to ensure each edge case is validated.
In `@packages/worker/src/mcp/capabilities/schema-compression.ts`:
- Around line 29-133: The compressSchemaNode traversal currently treats
composition keywords (allOf, anyOf, oneOf, not, if, then, else) via the generic
fallback, so nested schemas there never get the correct propertyName context;
update compressSchemaNode to explicitly handle those keys: for array-valued
keywords (allOf, anyOf, oneOf) map each item through compressSchemaNode with {
isRoot: false, propertyName: undefined, stripRootObjectType: false }, and for
object-valued keywords (not, if, then, else) call compressSchemaNode on their
value with the same context; keep using the existing context fields (isRoot,
propertyName, stripRootObjectType) and the same pattern used for
items/properties handling so titles/descriptions inside composed schemas are
correctly compressed.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 59-79: The current applyMaxResponseSize function trims
payload.matches one at a time and re-serializes each loop; replace that linear
trimming with a binary search over the prefix length of payload.matches to find
the largest prefix whose serialized size is <= maxResponseSize. Keep using
stringifySearchPayload for size checks, operate on trimmedPayload = {
...payload, matches: payload.matches.slice(0, k) } during checks, and return the
final trimmed payload and its serialized string; ensure you still handle
non-finite/<=0 maxResponseSize the same way and that edge cases (0 matches fit
or none fit) are handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dd07527-a932-4b84-9374-8ff342cec83c
📒 Files selected for processing (4)
packages/worker/src/mcp/capabilities/capability-search.tspackages/worker/src/mcp/capabilities/schema-compression.node.test.tspackages/worker/src/mcp/capabilities/schema-compression.tspackages/worker/src/mcp/tools/search.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/mcp/tools/search.ts (1)
363-389:⚠️ Potential issue | 🟠 MajorSurface when size trimming removed matches.
Lines 363-389 have both the original and trimmed match counts, but the returned text and
structuredContent.resultnever say that low-ranked hits were dropped to satisfymaxResponseSize. With the new default 4,000-character budget,detail: truesearches can look complete even when they were trimmed. Please return an omitted-count or warning so callers know to retry with a larger budget when needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search.ts` around lines 363 - 389, When applyMaxResponseSize trims matches, surface the omitted count and a warning: compare payload.matches.length with trimmedPayload.matches.length after calling applyMaxResponseSize in search.ts and, if trimmed down, add an omittedCount (payload.matches.length - trimmedPayload.matches.length) to structuredContent.result and push a descriptive warning into the warnings array (or add an omitted warning field) so the returned content and structuredContent indicate low-ranked hits were dropped to satisfy maxResponseSize; update the returned text payload (prependToolMetadataContent) to reflect that a trim occurred by including the same warning/omitted-count message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/capabilities/schema-compression.node.test.ts`:
- Around line 250-257: The test expectation includes a redundant description
"Repo" for the property repo which conflicts with the suite's redundant-label
normalization (repo/Repo normalize to the same token) and so will be removed by
the compressor; update the test in schema-compression.node.test.ts to match the
compressor output by either removing the description field from the expected
schema for the repo property or changing the expected description to the
normalized token form used elsewhere, ensuring the expectation for the anyOf ->
properties -> repo entry (the 'repo' property) matches the compressor's output.
In `@packages/worker/src/mcp/capabilities/schema-compression.ts`:
- Around line 55-60: The current conditional that strips "type: 'object'" is
gated by context.isRoot, causing nested/composed object schemas to keep their
type and breaking tests; update the check in the schema-compression logic (the
block using context.isRoot, context.stripRootObjectType, key and value) to
remove the context.isRoot requirement so that when context.stripRootObjectType
is true and key === 'type' and value === 'object' the type is stripped for all
objects (not only root), ensuring the condition reads effectively:
context.stripRootObjectType && key === 'type' && value === 'object'.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 59-90: The binary search in applyMaxResponseSize can return an
over-budget empty-match payload when even the payload with zero matches
serializes larger than maxResponseSize; before the loop, compute the serialized
form of the empty-match payload (using stringifySearchPayload on { ...payload,
matches: [] }) and if its length > maxResponseSize handle the “nothing fits”
case explicitly by returning the empty-match payload but with the serialized
string truncated to maxResponseSize (or another deterministic capped
representation) instead of returning an over-budget serialized value; ensure you
reference applyMaxResponseSize, stringifySearchPayload and payload.matches when
implementing this check.
---
Outside diff comments:
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 363-389: When applyMaxResponseSize trims matches, surface the
omitted count and a warning: compare payload.matches.length with
trimmedPayload.matches.length after calling applyMaxResponseSize in search.ts
and, if trimmed down, add an omittedCount (payload.matches.length -
trimmedPayload.matches.length) to structuredContent.result and push a
descriptive warning into the warnings array (or add an omitted warning field) so
the returned content and structuredContent indicate low-ranked hits were dropped
to satisfy maxResponseSize; update the returned text payload
(prependToolMetadataContent) to reflect that a trim occurred by including the
same warning/omitted-count message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62990e8b-fb53-4b63-bc2e-9d9c25239aa0
📒 Files selected for processing (3)
packages/worker/src/mcp/capabilities/schema-compression.node.test.tspackages/worker/src/mcp/capabilities/schema-compression.tspackages/worker/src/mcp/tools/search.ts
| if ( | ||
| context.isRoot && | ||
| context.stripRootObjectType && | ||
| key === 'type' && | ||
| value === 'object' | ||
| ) { |
There was a problem hiding this comment.
Object type stripping is root-only, which breaks intended compression and current tests.
Because Line 56 requires context.isRoot, nested/composed object schemas retain type: 'object'. That’s the direct cause of the failures reported at Line 95, Line 138, and Line 239 in the test suite.
🛠️ Suggested fix
- if (
- context.isRoot &&
- context.stripRootObjectType &&
- key === 'type' &&
- value === 'object'
- ) {
- continue
- }
+ if (key === 'type' && value === 'object') {
+ const shouldStripObjectType =
+ !context.isRoot || context.stripRootObjectType
+ if (shouldStripObjectType) {
+ continue
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/schema-compression.ts` around lines 55 -
60, The current conditional that strips "type: 'object'" is gated by
context.isRoot, causing nested/composed object schemas to keep their type and
breaking tests; update the check in the schema-compression logic (the block
using context.isRoot, context.stripRootObjectType, key and value) to remove the
context.isRoot requirement so that when context.stripRootObjectType is true and
key === 'type' and value === 'object' the type is stripped for all objects (not
only root), ensuring the condition reads effectively:
context.stripRootObjectType && key === 'type' && value === 'object'.
| function applyMaxResponseSize( | ||
| payload: SearchPayload, | ||
| maxResponseSize: number, | ||
| ): { payload: SearchPayload; serialized: string } { | ||
| if (!Number.isFinite(maxResponseSize) || maxResponseSize <= 0) { | ||
| const serialized = stringifySearchPayload(payload) | ||
| return { payload, serialized } | ||
| } | ||
|
|
||
| const total = payload.matches.length | ||
| let low = 0 | ||
| let high = total | ||
| let bestPayload: SearchPayload = { ...payload, matches: [] } | ||
| let bestSerialized = stringifySearchPayload(bestPayload) | ||
|
|
||
| while (low <= high) { | ||
| const mid = Math.floor((low + high) / 2) | ||
| const trimmedPayload: SearchPayload = { | ||
| ...payload, | ||
| matches: payload.matches.slice(0, mid), | ||
| } | ||
| const serialized = stringifySearchPayload(trimmedPayload) | ||
| if (serialized.length <= maxResponseSize) { | ||
| bestPayload = trimmedPayload | ||
| bestSerialized = serialized | ||
| low = mid + 1 | ||
| } else { | ||
| high = mid - 1 | ||
| } | ||
| } | ||
|
|
||
| return { payload: bestPayload, serialized: bestSerialized } |
There was a problem hiding this comment.
maxResponseSize can still be exceeded for valid inputs.
Line 63 accepts any positive integer, but Lines 71-90 fall back to the empty-match payload even when that payload already serializes above the requested cap (for example maxResponseSize: 1, or unexpectedly long warnings). That breaks the new contract for a valid input. Please handle the “nothing fits” case explicitly instead of returning an over-budget payload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/tools/search.ts` around lines 59 - 90, The binary
search in applyMaxResponseSize can return an over-budget empty-match payload
when even the payload with zero matches serializes larger than maxResponseSize;
before the loop, compute the serialized form of the empty-match payload (using
stringifySearchPayload on { ...payload, matches: [] }) and if its length >
maxResponseSize handle the “nothing fits” case explicitly by returning the
empty-match payload but with the serialized string truncated to maxResponseSize
(or another deterministic capped representation) instead of returning an
over-budget serialized value; ensure you reference applyMaxResponseSize,
stringifySearchPayload and payload.matches when implementing this check.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/worker/src/mcp/capabilities/schema-compression.node.test.ts (1)
257-260:⚠️ Potential issue | 🟡 MinorTest expectation conflicts with case-insensitive redundancy rule.
The test expects
description: 'Repo'to be preserved at Line 259, but the compressor correctly removes it as redundant with the property keyrepo(case-insensitive match). This is consistent with the behavior demonstrated in test 3 (lines 72-113) wheretitle: 'Owner'is removed for propertyowner, and confirmed by the pipeline failure.✅ Proposed fix to align expectation with compressor output
anyOf: [ { type: 'object', properties: { repo: { type: 'string', - description: 'Repo', }, }, }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/schema-compression.node.test.ts` around lines 257 - 260, The test expectation is wrong: the schema compressor's case-insensitive redundancy rule removes property descriptions that match the property key (e.g., property `repo` with description `'Repo'`), so update the test assertion in schema-compression.node.test.ts to expect the `description` to be removed for the `repo` property (or change the description to a non-redundant value) so it matches the compressor's output; look for the test block referencing `repo` and the compressor behavior demonstrated with `owner`/`Owner` to align the expected result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/worker/src/mcp/capabilities/schema-compression.node.test.ts`:
- Around line 257-260: The test expectation is wrong: the schema compressor's
case-insensitive redundancy rule removes property descriptions that match the
property key (e.g., property `repo` with description `'Repo'`), so update the
test assertion in schema-compression.node.test.ts to expect the `description` to
be removed for the `repo` property (or change the description to a non-redundant
value) so it matches the compressor's output; look for the test block
referencing `repo` and the compressor behavior demonstrated with `owner`/`Owner`
to align the expected result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bbc63f9-ee91-4e30-bf46-fe354f271912
📒 Files selected for processing (1)
packages/worker/src/mcp/capabilities/schema-compression.node.test.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
2 participants
Summary
Testing
Summary by CodeRabbit
New Features
Improvements
limitdefault: 5 when detail is enabled, otherwise 15.Tests