Skip to content

Commit 58512d8

Browse files
Tarek Mahmoud Sayedhalter73
authored andcommitted
Address review feedback on cacheScope converter and conformance gate
- CacheScopeConverter.Read now consumes non-string tokens with reader.Skip() before returning null. Previously an object or array value for cacheScope left the reader mispositioned and threw "read too much or not enough", breaking deserialization of the whole result. Added object and array cases to the tolerant-deserialization test. - GetInstalledConformanceVersion no longer calls EnsureNpmDependenciesInstalled. The version check backs Theory skip gates and must be side-effect-free; it now returns null when the conformance package is absent. The actual scenario run path still restores npm dependencies via ConformanceTestStartInfo.
1 parent 51571c6 commit 58512d8

3 files changed

Lines changed: 13 additions & 9 deletions

File tree

src/ModelContextProtocol.Core/Protocol/CacheScopeConverter.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,15 @@ internal sealed class CacheScopeConverter : JsonConverter<CacheScope?>
4040
{
4141
return CacheScope.Private;
4242
}
43+
44+
return null;
4345
}
4446

47+
// Any non-string token (number, bool, object, array) is an unrecognized hint. Consume the whole
48+
// value, including the contents of an object or array, so the reader is left correctly positioned
49+
// before mapping to null. Skipping is required for container tokens: returning without consuming
50+
// them would leave the reader mispositioned and break deserialization of the enclosing result.
51+
reader.Skip();
4552
return null;
4653
}
4754

tests/Common/Utils/NodeHelpers.cs

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -224,14 +224,10 @@ private static bool HasInstalledConformanceVersionAtLeast(Version minimumVersion
224224
var packageJsonPath = Path.Combine(
225225
repoRoot, "node_modules", "@modelcontextprotocol", "conformance", "package.json");
226226

227-
// Only trigger an install if the package isn't already present. This avoids a
228-
// gate check inadvertently running 'npm ci' (which would restore the pinned
229-
// version and clobber a locally installed private build).
230-
if (!File.Exists(packageJsonPath))
231-
{
232-
EnsureNpmDependenciesInstalled();
233-
}
234-
227+
// This is a skip gate for version-conditional conformance scenarios, so it must stay
228+
// side-effect-free. If the conformance package isn't installed, report no version (the
229+
// scenario is simply gated off); the actual scenario run path restores npm dependencies
230+
// separately via ConformanceTestStartInfo.
235231
if (!File.Exists(packageJsonPath))
236232
{
237233
return null;

tests/ModelContextProtocol.Tests/Protocol/CacheableResultTests.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,8 @@ public static void CacheableResult_DeserializesUnknownCacheScope_AsNull(Type typ
172172
{
173173
// A future/unknown cacheScope string must not break deserialization of the entire result; it is
174174
// tolerated and surfaced as null (equivalent to an absent field, which clients treat as public).
175-
foreach (string scope in new[] { "\"shared\"", "\"\"", "123", "true", "null" })
175+
// Non-string tokens, including objects and arrays, must likewise be tolerated and fully consumed.
176+
foreach (string scope in new[] { "\"shared\"", "\"\"", "123", "true", "null", "{}", "[]", "{\"a\":1}", "[1,2]" })
176177
{
177178
string json = $"{{\"cacheScope\":{scope}}}";
178179

0 commit comments

Comments
 (0)