fix: report registered MCP tool count - #504
Conversation
![gitstream-cm[bot]](https://avatars.githubusercontent.com/in/230441?s=60&v=4){kind=small}
There was a problem hiding this comment.
✨ PR Review
LGTM
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #504 +/- ##
==========================================
- Coverage 90.55% 90.53% -0.03%
==========================================
Files 24 24
Lines 1091 1109 +18
Branches 281 283 +2
==========================================
+ Hits 988 1004 +16
- Misses 45 46 +1
- Partials 58 59 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merge Queue Status
This pull request spent 12 minutes 2 seconds in the queue, with no time running CI. ReasonPull request #504 has been dequeued Queue conditions are not satisfied:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Requeued — the merge queue status continues in this comment ↓. |
|
@Mergifyio refresh |
✅ Pull request refreshed |
|
The — charlied/pr-check-repair |
Unit Test Results 1 files 22 suites 1s ⏱️ Results for commit 66d06e5. ♻️ This comment has been updated with latest results. |
|
🥷 Code experts: chrisdoc, CharlieHelps chrisdoc, CharlieHelps have most 👩💻 activity in the files. ✨ Comment |
Merge Queue Status
This pull request spent 4 minutes 53 seconds in the queue, with no time running CI. ReasonThe pull request #504 has been manually updated Requeued — the merge queue status continues in this comment ↓. |
|
The latest Please inspect the gitStream integration or required-check configuration. I did not modify the branch. — charlied/pr-check-repair |
Merge Queue Status
This pull request spent 6 minutes 42 seconds in the queue, including 1 minute 17 seconds running CI. Required conditions to merge
|
There was a problem hiding this comment.
✨ PR Review
The PR correctly replaces the hardcoded tool count with a Proxy-based counting mechanism and includes a matching changeset. The implementation avoids double-counting through careful placement on the proxy boundary. One count-accuracy risk and a test correctness issue are worth addressing before merge.
2 issues detected:
🐞 Bug - Typing the interceptor with `Parameters` collapses all overloads to a single signature, which could silently miss new overloads added by SDK upgrades. 🛠️
Details: In the tool proxy handler, count += 1 executes after target.tool(...args) returns, so if the call throws the count is not incremented — that is intentional and correct. However, for the registerTool proxy the same pattern is used, and it is also correct. The real risk is in the tool handler: the spread type Parameters<McpServer["tool"]> covers only a single overload selected by TypeScript. If the SDK exposes tool as an overloaded function, the spread will only match one overload signature at compile time but will forward all runtime argument shapes to target.tool. If the SDK adds a new overload that the inferred Parameters<> does not cover, calls will silently pass through without being counted (TypeScript will error, but the runtime behavior depends on whether the SDK is updated before the types). This is low-risk today but fragile.
File: src/index.ts (92-96)
🛠️ A suggested code correction is included in the review comments.
🐞 Bug - Using `testDoubles.registerTool.mock.calls.length` as the expected value creates a circular assertion — it checks that the span received the same number that was recorded, not that the number is correct.
Details: The test at line 162-171 asserts that setAttribute was called with "mcp.tools.count" equal to testDoubles.registerTool.mock.calls.length. However, the counting proxy also increments its internal counter for every tool-proxy interception, not only for registerTool calls. The mock tool implementation calls this.registerTool(), which appends to testDoubles.registerTool.mock.calls — but those delegated calls are made on target (bypassing the proxy) and are counted by the proxy's tool handler, not its registerTool handler. If any register function uses server.tool() directly and the SDK mock is changed to NOT delegate, the two counts diverge and the assertion would silently accept an incorrect mcp.tools.count value. The assertion should instead check the exact expected count.
File: src/index.test.ts (165-170)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
| return (...args: Parameters<McpServer["tool"]>) => { | ||
| const registeredTool = target.tool(...args); | ||
| count += 1; | ||
| return registeredTool; | ||
| }; |
There was a problem hiding this comment.
🐞 Bug - Count on Throw: Use Reflect.apply to forward all arguments unconditionally, keeping the count increment only on success:
if (property === "tool") {
return (...args: unknown[]) => {
const result = Reflect.apply(target.tool as (...a: unknown[]) => unknown, target, args);
count += 1;
return result;
};
}| return (...args: Parameters<McpServer["tool"]>) => { | |
| const registeredTool = target.tool(...args); | |
| count += 1; | |
| return registeredTool; | |
| }; | |
| return (...args: unknown[]) => { | |
| const result = Reflect.apply(target.tool as (...a: unknown[]) => unknown, target, args); | |
| count += 1; | |
| return result; | |
| }; |
Is this review accurate? Use 👍 or 👎 to rate it
If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over
2 participants
Summary
toolandregisterToolregistrations dynamicallymcp.tools.registerspantooldelegation without double-countingTesting
npx vitest run src/index.test.ts src/tools/annotations.test.tsnpm run checknpm run check:typesnpx vitest run --exclude tests/integration/**npm run buildnpm run check:changesetResolves #502
✨ PR Description
Purpose: Replace hardcoded tool count with dynamic tracking to accurately report registered MCP tools during server initialization.
Main changes:
createToolCountingServer()proxy wrapper to track actual tool registrations viatool()andregisterTool()callsGenerated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how