Skip to content

Commit ca00790

Browse files
committed
[strategos] docs: Add comprehensive explanation of schema fix and three-layer defense strategy
WHY: - Complete fix requires understanding of BOTH schema update AND input sanitization - Schema oneOf prevents overly strict client-side validation from blocking LLM mistakes - Input sanitization provides server-side defense and type coercion - Together they ensure tool availability while maintaining correctness EXPECTED: - Clear documentation of root cause (client-side schema validation + missing sanitization) - Explanation of three-layer defense: permissive schema, sanitization, strict validation - Real-world scenarios showing why both fixes are required - Design philosophy: liberal in acceptance, strict in production Refs: MCP specification - clients SHOULD validate, servers MUST validate
1 parent 58a9988 commit ca00790

File tree

1 file changed

+221
-0
lines changed

1 file changed

+221
-0
lines changed
Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,221 @@
1+
# Sequential Thinking MCP Server - Schema Fix Explanation
2+
3+
## Problem Statement
4+
5+
The sequential thinking MCP server was experiencing errors when LLMs passed numeric parameters as strings instead of numbers:
6+
7+
```
8+
Error: Invalid thoughtNumber: must be a number
9+
```
10+
11+
This occurred even though the string values were semantically valid (e.g., `"1"`, `"2"`, `"3"`).
12+
13+
## Root Cause Analysis
14+
15+
The issue had **two distinct layers**:
16+
17+
### Layer 1: Overly Strict Schema (Client-Side)
18+
The original schema defined numeric parameters as:
19+
```typescript
20+
thoughtNumber: {
21+
type: "integer",
22+
minimum: 1
23+
}
24+
```
25+
26+
**Problem**: Some MCP clients perform schema validation **before** sending requests to the server. When an LLM mistakenly sent `thoughtNumber: "1"` (string) instead of `thoughtNumber: 1` (number), these clients would reject the request entirely, making the tool unusable.
27+
28+
### Layer 2: Missing Input Sanitization (Server-Side)
29+
Even when strings reached the server (clients without strict validation), the validation code immediately rejected them:
30+
```typescript
31+
if (!data.thoughtNumber || typeof data.thoughtNumber !== 'number') {
32+
throw new Error('Invalid thoughtNumber: must be a number');
33+
}
34+
```
35+
36+
**Problem**: No attempt was made to coerce valid string numbers to actual numbers before validation.
37+
38+
## The Complete Solution: Three-Layer Defense
39+
40+
### Layer 1: Permissive Schema (Client-Side Flexibility)
41+
```typescript
42+
thoughtNumber: {
43+
oneOf: [
44+
{ type: "integer", minimum: 1 },
45+
{ type: "string", pattern: "^[1-9]\\d*$" }
46+
],
47+
description: "Current thought number (numeric value, e.g., 1, 2, 3)"
48+
}
49+
```
50+
51+
**Purpose**:
52+
- ✅ Allows LLM mistakes - If the LLM sends `"1"` instead of `1`, don't reject it
53+
- ✅ Passes client validation - Smart clients won't block the request
54+
- ✅ Documents flexibility - Shows both types are acceptable
55+
- ✅ Prevents tool unavailability - Tool remains usable even with imperfect LLM output
56+
57+
### Layer 2: Input Sanitization (Server-Side Coercion)
58+
```typescript
59+
private sanitizeNumericParam(value: unknown): unknown {
60+
// INPUT SANITIZATION: Coerce string numbers to actual numbers
61+
// WHY: Some MCP clients may pass numeric parameters as strings
62+
// EXPECTED: Convert valid numeric strings to numbers before validation
63+
if (typeof value === 'number') {
64+
return value; // Already a number
65+
}
66+
// Regex matches positive integers starting from 1 (excludes 0)
67+
if (typeof value === 'string' && /^[1-9]\d*$/.test(value)) {
68+
const parsed = parseInt(value, 10);
69+
if (!isNaN(parsed) && parsed > 0) {
70+
return parsed; // Coerced to number
71+
}
72+
}
73+
return value; // Return as-is, let validation fail with clear error
74+
}
75+
```
76+
77+
**Purpose**:
78+
- ✅ Coerces valid strings to numbers - `"1"``1`
79+
- ✅ Handles client variations - Works regardless of client behavior
80+
- ✅ Defense in depth - Never trust client input
81+
- ✅ Graceful handling - Converts when possible, fails clearly when not
82+
83+
### Layer 3: Strict Validation (Server-Side Enforcement)
84+
```typescript
85+
// Sanitize numeric parameters before validation
86+
if (data.thoughtNumber !== undefined) {
87+
data.thoughtNumber = this.sanitizeNumericParam(data.thoughtNumber);
88+
}
89+
90+
// Now validate - after sanitization, we require actual numbers
91+
if (!data.thoughtNumber || typeof data.thoughtNumber !== 'number') {
92+
throw new Error('Invalid thoughtNumber: must be a number');
93+
}
94+
```
95+
96+
**Purpose**:
97+
- ✅ Final enforcement - After sanitization, require actual number
98+
- ✅ Clear error messages - If sanitization couldn't convert, fail explicitly
99+
- ✅ Type safety - Guarantees downstream code gets correct type
100+
101+
## Why Both Schema AND Sanitization Are Required
102+
103+
### Without `oneOf` (Old Schema)
104+
```
105+
LLM sends "1" → Client schema validation (type: integer) → ❌ REJECTED
106+
Tool becomes unusable due to LLM imperfection
107+
```
108+
109+
### With `oneOf` But No Sanitization
110+
```
111+
LLM sends "1" → Client schema validation (oneOf) → ✅ PASSES
112+
→ Server validation (typeof !== 'number') → ❌ REJECTED
113+
Tool fails at server level
114+
```
115+
116+
### With Both `oneOf` AND Sanitization (Complete Fix)
117+
```
118+
LLM sends "1" → Client schema validation (oneOf) → ✅ PASSES
119+
→ Server sanitization → "1" becomes 1
120+
→ Server validation → ✅ PASSES
121+
Tool works correctly!
122+
```
123+
124+
## Real-World Scenarios
125+
126+
| Input | Old Schema | New Schema + Sanitization |
127+
|-------|-----------|---------------------------|
128+
| `1` (correct type) | ✅ Works | ✅ Works |
129+
| `"1"` (wrong type, valid value) | ❌ Client blocks | ✅ Client passes → Server fixes |
130+
| `"0"` (invalid value) | ❌ Client blocks | ❌ Server rejects (sanitization returns as-is) |
131+
| `"abc"` (garbage) | ❌ Client blocks | ❌ Server rejects (sanitization returns as-is) |
132+
| `-1` (negative) | ❌ Schema rejects | ❌ Schema rejects |
133+
134+
## Key Insight: Schema as Client-Side Contract
135+
136+
**The schema isn't just about what the server accepts - it's about preventing overly strict client-side validation from making the tool unusable when the LLM makes minor type mistakes.**
137+
138+
According to the MCP specification:
139+
- **Servers** are responsible for validating tool inputs
140+
- **Clients SHOULD** validate (but it's optional, not required)
141+
- **The schema is primarily for documentation and LLM guidance**
142+
143+
The MCP SDK does **not** enforce server-side schema validation. The schema tells clients what to send, but the server must still validate because clients might not respect it.
144+
145+
## Implementation Details
146+
147+
### Files Modified
148+
1. **index.ts** (lines 34-50): Added `sanitizeNumericParam()` method
149+
2. **index.ts** (lines 52-67): Updated `validateThoughtData()` to sanitize before validating
150+
3. **index.ts** (lines 241-272): Updated schema to use `oneOf` for all numeric parameters
151+
152+
### Parameters Updated
153+
All numeric parameters now accept both integers and strings:
154+
- `thoughtNumber`
155+
- `totalThoughts`
156+
- `revisesThought`
157+
- `branchFromThought`
158+
159+
### Pattern Used
160+
```typescript
161+
oneOf: [
162+
{ type: "integer", minimum: 1 },
163+
{ type: "string", pattern: "^[1-9]\\d*$" }
164+
]
165+
```
166+
167+
**Why this pattern?**
168+
- `minimum: 1` - Thought numbers are 1-indexed (1, 2, 3, ...)
169+
- `pattern: "^[1-9]\\d*$"` - Matches positive integers starting from 1, excludes 0 and negative numbers
170+
- Semantic consistency - Both schema and sanitization enforce the same rules
171+
172+
## Design Philosophy
173+
174+
This fix exemplifies robust API design:
175+
176+
1. **Be liberal in what you accept** (schema: `oneOf`)
177+
2. **Be strict in what you produce** (sanitization + validation)
178+
3. **Prioritize availability** (don't let minor LLM mistakes break tools)
179+
4. **Maintain correctness** (still reject truly invalid input)
180+
181+
The schema says "we're flexible," the sanitization says "we'll help you," and the validation says "but we still have standards."
182+
183+
## Testing
184+
185+
### Manual Testing
186+
```typescript
187+
// Test 1: Number (always worked)
188+
thoughtNumber: 1 // ✅ PASS
189+
190+
// Test 2: String (now works)
191+
thoughtNumber: "2" // ✅ PASS (sanitized to 2)
192+
193+
// Test 3: Invalid string (correctly rejected)
194+
thoughtNumber: "0" // ❌ FAIL (sanitization returns as-is, validation rejects)
195+
196+
// Test 4: Garbage (correctly rejected)
197+
thoughtNumber: "abc" // ❌ FAIL (sanitization returns as-is, validation rejects)
198+
```
199+
200+
### Build Verification
201+
```bash
202+
npm run build # Compiles TypeScript to JavaScript
203+
grep -A 10 "oneOf" dist/index.js # Verify schema in built file
204+
```
205+
206+
## Commits
207+
208+
1. **240221c**: Initial regex fix for thought number validation
209+
2. **5de32a7**: Added input sanitization for numeric parameters
210+
3. **58a9988**: Updated schema to accept string or number using `oneOf`
211+
212+
## References
213+
214+
- [MCP Tools Specification](https://modelcontextprotocol.io/docs/concepts/tools)
215+
- [JSON Schema oneOf](https://json-schema.org/understanding-json-schema/reference/combining#oneof)
216+
- Original issue: LLMs passing numeric parameters as strings
217+
218+
## Conclusion
219+
220+
This fix ensures the sequential thinking MCP server is robust, user-friendly, and resilient to common LLM output variations. By accepting both types in the schema and sanitizing on the server, we maintain tool availability while ensuring correctness.
221+

0 commit comments

Comments
 (0)