feat(doctor): static tool-hygiene lints as advisory warnings - #3753
feat(doctor): static tool-hygiene lints as advisory warnings#3753chelojimenez wants to merge 2 commits into
Conversation
Add a zero-traffic lint pass over the tools/list catalog to the shared doctor core, porting the agent-facing failure modes PostHog surfaced from production MCP telemetry into connect-time checks: - inconsistent-param-naming: same param spelled differently across tools (insightId vs insight_id) - unknowable-required-id: required ID-shaped param whose prose never says where to get a value - undocumented-constraint: numeric schema bounds (maxLength etc.) the description never mentions - unbounded-list-tool: list/query-shaped tool with no limit or pagination param - missing-description: empty or trivial tool description Findings ride on ServerDoctorResult.toolLints plus a new toolHygiene check with a new "warn" status. Warnings are advisory: deriveDoctorStatus ignores them, so readiness and CLI exit codes are unaffected. Both doctor variants (node manager + browser/http) get the pass via buildConnectedServerDoctorState; the CLI human formatter renders a capped "Tool hygiene" section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
dosubot
Bot
added
size:L
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c2f91935-3e24-4987-b5e0-c846a49a7a5e) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_8c7218bd-f7e1-41f6-a441-12c654b8a9d9) |
WalkthroughThe SDK now lints tool catalogs for five advisory hygiene rules and stores findings in server-doctor results. Tool hygiene checks report skipped, passing, or warning states without changing readiness. The linter and its types are exported from SDK entrypoints. CLI output displays warning details, limits displayed findings to 20, and references the full JSON artifact for omitted findings. Tests cover lint rules, doctor integration, formatting, and updated result fixtures. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/src/lib/server-doctor.ts`:
- Around line 103-107: Sanitize server-controlled catalog text before rendering
it in the loop over toolLints: escape terminal control characters, including
ANSI escapes, carriage returns, and newlines, in both the constructed where
value and finding.message before pushing the formatted line. Add a formatting
test covering an ANSI escape sequence and newline embedded in a tool name, while
preserving the existing output structure for safe text.
In `@sdk/src/tool-lints.ts`:
- Around line 98-103: Update the findings sort comparator to replace
localeCompare calls for a.tools[0] and a.param with deterministic
locale-independent code-unit comparisons. Preserve the existing RULE_ORDER
priority and empty-string fallbacks while ensuring identical ordering across
hosts.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 77920ae0-dd88-4afd-a55e-e9f6df256fc3
📒 Files selected for processing (10)
cli/src/lib/server-doctor.tscli/tests/debug-artifact.test.tscli/tests/server-doctor.test.tssdk/src/browser.tssdk/src/index.tssdk/src/platform/show-servers.tssdk/src/server-doctor-core.tssdk/src/tool-lints.tssdk/tests/server-doctor.test.tssdk/tests/tool-lints.test.ts
| for (const finding of toolLints.slice(0, MAX_TOOL_LINT_LINES)) { | ||
| const where = finding.param | ||
| ? `${finding.tools.join(", ")} · ${finding.param}` | ||
| : finding.tools.join(", "); | ||
| lines.push(`- [${finding.rule}] ${where}: ${finding.message}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sanitize catalog text before terminal rendering.
finding.tools, finding.param, and finding.message contain server-controlled catalog text. A tool name or parameter name can include escape, carriage-return, or newline controls. These controls can forge CLI output or trigger terminal escape sequences.
Escape terminal control characters in both where and finding.message before adding them to lines. Add a formatting test with an ANSI escape sequence and a newline in a tool name.
Proposed fix
+function escapeTerminalText(value: string): string {
+ return value.replace(/[\u0000-\u001F\u007F-\u009F]/g, (character) =>
+ `\\u${character.codePointAt(0)!.toString(16).padStart(4, "0")}`,
+ );
+}
+
for (const finding of toolLints.slice(0, MAX_TOOL_LINT_LINES)) {
const where = finding.param
- ? `${finding.tools.join(", ")} · ${finding.param}`
- : finding.tools.join(", ");
- lines.push(`- [${finding.rule}] ${where}: ${finding.message}`);
+ ? `${finding.tools.map(escapeTerminalText).join(", ")} · ${escapeTerminalText(finding.param)}`
+ : finding.tools.map(escapeTerminalText).join(", ");
+ lines.push(
+ `- [${finding.rule}] ${where}: ${escapeTerminalText(finding.message)}`,
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const finding of toolLints.slice(0, MAX_TOOL_LINT_LINES)) { | |
| const where = finding.param | |
| ? `${finding.tools.join(", ")} · ${finding.param}` | |
| : finding.tools.join(", "); | |
| lines.push(`- [${finding.rule}] ${where}: ${finding.message}`); | |
| function escapeTerminalText(value: string): string { | |
| return value.replace(/[\u0000-\u001F\u007F-\u009F]/g, (character) => | |
| `\\u${character.codePointAt(0)!.toString(16).padStart(4, "0")}`, | |
| ); | |
| } | |
| for (const finding of toolLints.slice(0, MAX_TOOL_LINT_LINES)) { | |
| const where = finding.param | |
| ? `${finding.tools.map(escapeTerminalText).join(", ")} · ${escapeTerminalText(finding.param)}` | |
| : finding.tools.map(escapeTerminalText).join(", "); | |
| lines.push( | |
| `- [${finding.rule}] ${where}: ${escapeTerminalText(finding.message)}`, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/src/lib/server-doctor.ts` around lines 103 - 107, Sanitize
server-controlled catalog text before rendering it in the loop over toolLints:
escape terminal control characters, including ANSI escapes, carriage returns,
and newlines, in both the constructed where value and finding.message before
pushing the formatted line. Add a formatting test covering an ANSI escape
sequence and newline embedded in a tool name, while preserving the existing
output structure for safe text.
| return findings.sort( | ||
| (a, b) => | ||
| RULE_ORDER[a.rule] - RULE_ORDER[b.rule] || | ||
| (a.tools[0] ?? "").localeCompare(b.tools[0] ?? "") || | ||
| (a.param ?? "").localeCompare(b.param ?? "") | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use locale-independent finding ordering.
localeCompare() without an explicit locale uses the host default locale. The same catalog can produce different finding orders in JSON artifacts on different hosts. Use a fixed code-unit comparison for the tool and parameter sort keys.
Proposed fix
+function compareStableStrings(left: string, right: string): number {
+ return left === right ? 0 : left < right ? -1 : 1;
+}
+
return findings.sort(
(a, b) =>
RULE_ORDER[a.rule] - RULE_ORDER[b.rule] ||
- (a.tools[0] ?? "").localeCompare(b.tools[0] ?? "") ||
- (a.param ?? "").localeCompare(b.param ?? "")
+ compareStableStrings(a.tools[0] ?? "", b.tools[0] ?? "") ||
+ compareStableStrings(a.param ?? "", b.param ?? "")
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return findings.sort( | |
| (a, b) => | |
| RULE_ORDER[a.rule] - RULE_ORDER[b.rule] || | |
| (a.tools[0] ?? "").localeCompare(b.tools[0] ?? "") || | |
| (a.param ?? "").localeCompare(b.param ?? "") | |
| ); | |
| function compareStableStrings(left: string, right: string): number { | |
| return left === right ? 0 : left < right ? -1 : 1; | |
| } | |
| return findings.sort( | |
| (a, b) => | |
| RULE_ORDER[a.rule] - RULE_ORDER[b.rule] || | |
| compareStableStrings(a.tools[0] ?? "", b.tools[0] ?? "") || | |
| compareStableStrings(a.param ?? "", b.param ?? "") | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/src/tool-lints.ts` around lines 98 - 103, Update the findings sort
comparator to replace localeCompare calls for a.tools[0] and a.param with
deterministic locale-independent code-unit comparisons. Preserve the existing
RULE_ORDER priority and empty-string fallbacks while ensuring identical ordering
across hosts.
Internal previewPreview URL: https://mcp-inspector-pr-3753.up.railway.app |
1 participant
What
Adds a zero-traffic lint pass over the
tools/listcatalog to the shared doctor core. It ports the agent-facing failure modes PostHog surfaced from production MCP telemetry (their thread) into static connect-time checks — you see them inmcpjam server doctorbefore any agent ever hits the server.Rules (all advisory)
inconsistent-param-naminginsightIdvsinsight_id)unknowable-required-idprojectIdagents never knew)undocumented-constraintmaxLengthetc.) the description never mentionsunbounded-list-toolmissing-descriptionHow
sdk/src/tool-lints.ts—lintToolCatalog(tools) → ToolLintFinding[], dependency-free, defensive against malformed catalogs.buildConnectedServerDoctorState(server-doctor-core.ts), so the node-manager doctor, the browser/http doctor, and the hosted/servers/doctorroute all get it.toolHygienecheck +toolLintsarray onServerDoctorResult; new"warn"check status.deriveDoctorStatusignoreswarn, CLI exit codes unchanged (covered by a dedicated test).Tool hygienesection (capped at 20 lines; full list in the--outJSON artifact).Example output
Status stays
ready, exit code 0.Testing
sdk/tests/tool-lints.test.ts(10 cases incl. false-positive guards:gridisn't an ID,minLength: 1ignored, malformed entries skipped).servers-doctorroute test passed.mcpworkspace typecheck passed.Deliberately out of scope: the dynamic analyzers (bulk-gap loop detection, response-size p95) — those belong in swarm trace analysis, not a static connect-time pass.
🤖 Generated with Claude Code
Summary by cubic
Adds a zero-traffic, static hygiene lint pass over
tools/listand surfaces advisorytoolHygienewarnings in the doctor. Improves agent usability without affecting readiness or CLI exit codes.sdk/src/tool-lints.tswithlintToolCatalog; exported fromsdkandbrowser(ToolLintFinding,ToolLintRule).buildConnectedServerDoctorStateand reported viachecks.toolHygiene;warnis advisory and does not change readiness or exit codes. Platformshow-serversnow acceptswarnon primitive checks.ServerDoctorResult.toolLints. CLI prints a “Tool hygiene” section (capped at 20 lines), with full details in--outJSON.missing-description,unknowable-required-id,inconsistent-param-naming,undocumented-constraint,unbounded-list-tool.Written for commit df63414. Summary will update on new commits.
Note
Low Risk
Additive doctor diagnostics and CLI display only; warnings are explicitly excluded from readiness derivation and existing failure paths are unchanged.
Overview
Adds static, zero-traffic linting of the
tools/listcatalog during server doctor runs. A newlintToolCatalogmodule applies five advisory rules (missing descriptions, unknowable required IDs, inconsistent param naming, undocumented schema constraints, unbounded list/query tools) and returns structuredtoolLintsfindings.Doctor results gain a
toolHygienecheck (with a newwarncheck status), atoolLintsarray on the report, and wiring inbuildConnectedServerDoctorStateso CLI, SDK, browser, and hosted doctor paths share the same behavior. Readiness and exit codes are unchanged —deriveDoctorStatusstill only treatserrorchecks as failures.The CLI human formatter prints a capped Tool hygiene section when warnings exist; full findings remain in JSON artifacts.
lintToolCatalogand types are exported from the main SDK and browser entrypoints.Reviewed by Cursor Bugbot for commit df63414. Bugbot is set up for automated code reviews on this repo. Configure here.