Repository navigation
MCP tools refuse a value the parameter's type cannot hold, by name, in Lite and the Darling service (#4956) - #4960
Conversation
…eal binder with and without the guard (RED before the guard reads types) Each row pins both halves: the SDK binder alone reads or rejects the value as the row says, and the guard refuses exactly what the binder rejects, by name. 33 rows fail on each product before the guard change: 31 where the guard passes a value the binder cannot read, 2 where it refuses an in-range ulong the binder reads.
… type, and refuses an in-range ulong no longer McpSchemaCompat records each tool's parameter types as it creates the tool; the guard reads them through the request's services and refuses exactly the values System.Text.Json throws on when binding to that type (an int past int range, null for a non-nullable value, short and byte past range, integer arrays, a number for a string, a string for a boolean). A tool with no record keeps the schema-only check. Also scopes the McpHelpers cost claim to ASCII text at the cut, and corrects the inert data source comment (#4956 items c and d).
…in TsqlConventionGuardTests reads it whole
Blind read, before the bodyWhat I took the change to be, reading the diff first: the host now records each tool's declared parameter CLR types at tool-creation time into a new per-service-collection Three things I want to put on the record as right, since they are the shapes this repo's traps are about:
Where my read and the body part company is in the five inline comments: the per-call cost the typed check adds on the one tool built for bulk input, the |
…aws, and the argument guard passes a value whose converter throws something other than a JSON error (#4956) WebServerPageRangeTests now checks every read the run made against the widest window the capped reads take, so a read that comes after its tab has settled (the last tab's has nothing after it to be counted in) still fails. The harness mounts what each tab builds under a node that names its range and tab, and the test fails on any notice, error or panel left loading at an offered range, naming where. McpArgumentValueCheck.CanBind now answers true for any exception except JsonException, so a converter that throws an ArgumentException is left to the binder instead of escaping the call-tool filter; a test registers such a tool and calls Refuse.
|
Blind read first, body second. Notes on the parts I could settle from the tree, so they are not relitigated: The table is the right shape. Counts I checked by hand. Type coverage, independently. Grepping both SKUs' Parity. Not a finding. The change is entirely in No behaviour the old guard allowed is newly refused by accident. Every existing caller of The Findings are inline; verdict follows. |
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Detail is in the inline comments and the top-level note; this is the ledger.
Claims
- VERIFIED — the record holds each tool parameter's declared CLR type and nullability, filled per tool by the same
SchemaOptionsForrule, one instance found-or-added asGuideCatalogOfdoes —McpToolParameterTypes.cs:24,:49-73;McpSchemaCompat.cs:125,:166-172,:216-231. - VERIFIED —
Refusetakes it as an optional third argument; the filter reads it fromrequest.Services—McpUnknownArgumentGuard.cs:112-113,:100. - VERIFIED — refuses exactly on
JsonException, anything else passes —McpToolParameterTypes.cs:138-153, pinned byAValueWhoseConverterThrowsAnythingButAJsonError_PassesToTheBinder(Darling/Darling.Tests/McpUnknownArgumentGuardTests.cs:517). - VERIFIED — the per-type wording, range only below
long, "cannot be null" for a value type; the body's workedhours_backsentence reproduces exactly —McpToolParameterTypes.cs:121-133,:169-179. - VERIFIED — a tool with no record keeps the schema-only check, which passes null —
McpUnknownArgumentGuard.cs:152-159,:171-184,:265-271;McpToolParameterTypes.cs:84. - VERIFIED —
McpSchemaCompatsets noSerializerOptions—McpSchemaCompat.cs:176-183. - VERIFIED — 58 rows — counted in
McpArgumentTypeRows.Table():199-280. - VERIFIED — the probe tool carries no
McpServerToolType, so no census sees it —McpArgumentTypeRows.cs:38-50. - VERIFIED — no shipped tool declares a
ulong— grep of both SKUs'[Description(...)]tool parameters yields onlystring?/string/int/int?/long/long?/bool/bool?/double/double?/string[]?. - VERIFIED —
RangeOfis a block body, so the #3667 member scan reads it whole —McpToolParameterTypes.cs:171-179. - VERIFIED — items c and d are comment-only —
McpHelpers.cs:87-92,McpWholeNumberArgumentTests.cs:48-51. - VERIFIED — a null for a non-nullable reference type is not refused:
Problemnever readsAllowsNullandCanBind(null, typeof(string))succeeds —McpToolParameterTypes.cs:110, row atMcpArgumentTypeRows.cs:253. - VERIFIED — one table linked into both test projects, resolved against each product's own tools —
Lite.Tests/Lite.Tests.csproj,Lite.Tests/McpArgumentTypeTests.cs:42,Darling/Darling.Tests/McpArgumentTypeTests.cs:44. - VERIFIED — both hosts register the same shared
Instance, so neither SKU is left behind —Lite/Mcp/McpHostService.cs:165,Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs:1053. - UNVERIFIED — the RED run (59 total, 33 failed per product, the 31/2 split, the verbatim lines, "the binder half passed for every row before the change"): needs a build and two runs at the pre-change guard. The arithmetic also does not close here — 58 rows plus two
[Fact]s is 60, not 59. - UNVERIFIED — "Full Darling 19952, Failed 0" / "Full Lite 7235, Failed 0": needs the runs, and see REFUTED for why they may not cover
WebServerPageRangeTests. - UNVERIFIED — that
McpJsonUtilities.DefaultOptionsare the options the SDK binds with (the body marks it inferred): needs the SDK path or a run. - UNVERIFIED — "
deprecated/Dashboardbuilds, 0 warnings": needs a build. - UNVERIFIED — that the four new
offeredRangesassertions hold: needs Node;TryRunreturns false and the test passes silently without it, so a green leg is not evidence. - REFUTED — "Not done (parked): #4956 item a … and item b … this branch ran out of room for them." Both are in the diff: item a is
WebServerPageRangeTests.cs:52-57; item b is:61-72plusweb-kept-history-harness.mjs:274-277,:300and thewhere-threading instripsat:313-323.
Riskiest lines
PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:204—TakestestsBounds(type)beforetype.IsEnum, andType.GetTypeCodeof an enum returns its underlying code, soBoundsmatches every enum: theIsEnumarm at:229can never run,Plural:263shadows the same way, and an enum argument is told it takes "a whole number with no decimal point, such as 1" when the binder reads the member name. — UNPINNED (EveryClrTypeAShippedToolDeclares_IsOneTheTableCoversfails on the first enum parameter added, but on the type being uncovered, not the wording).PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:193—BigInteger.TryParseover the argument's full raw text, caller-controlled and unbounded, superlinear;CanBindabove bails cheaply and the predecessor (McpUnknownArgumentGuard.cs:294) only scanned digits. — UNPINNED (longest value in the table is 20 digits).PerformanceMonitor.Common/Mcp/McpUnknownArgumentGuard.cs:100— if the request scope cannot see the singleton, every argument silently falls back to schema-only and the feature is a no-op with no tell. — pinned byMcpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIton both SKUs and byEveryAdvertisedParameter_HasARecordedType_ThatAgreesWithTheSchema.
Traps near this diff
- "A pin that encodes the lie … Ask what a pin would fail ON." (#3699) — does not apply to the MCP table:
CheckAsync:294-316takes the binder's own verdict on an unguarded host first andbound != refusedfails both ways. It does describe theofferedRangeshalf, clean only because the scenario returned[]before this diff — see REFUTED. - "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply: the change is wholly in
PerformanceMonitor.Common/Mcp, both hosts register it, and the table runs on both.WebServerPageRangeTestsowes no twin —RANGE_OPTIONSexists only under the Darling service'swwwroot/jsand Lite ships no viewer. - "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply, narrowly:
McpArgumentValueCheck.OutOfRange(:183) andMcpUnknownArgumentGuard.OutOfRangeWord(:279) are near-twins in shape but answer different questions and are not pinned equal. - "
hours_back,limit,top,days_backare refused when out of range, never clamped" (#3699, #3641) — does not apply as a defect; honoured.Problemreturns a sentence or null (:108-136) and never coerces. - "An expression-bodied member holding a
{ }property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — applies, already handled:RangeOfships as a block body (:171-179);Bounds:156andPlural:262hold a switch and a conditional chain, not a property pattern. - "A roster-based census does not see the file it does not roster" (#3699) — does not apply:
ShippedParameterTypeKeys(McpArgumentTypeRows.cs:100-123) walks the types the host source registers, andMcpToolTypeRegistrationTestspins that list against[McpServerToolType]. - "A test that reads the clock twice measures the CI scheduler (#3676)" — does not apply: nothing new reads a clock;
BindingFailureSince(McpInProcessHost.cs:301) anchors on an entry count.
Divergence (blind read vs PR body)
The body parks #4956 items a and b under "Not done"; the diff implements both. Those are the only changes here the body does not vouch for, and the only assertions that were vacuously true before the diff — offeredRanges returned [], so notices, errors and loading reported on an empty tree. The body's own reason for parking them was that each needs a plant-run-revert cycle to prove it would go RED, so four new assertions arrive with neither a RED pin nor coverage from the quoted suite totals, and a read-only tree cannot substitute because TryRun skips silently without Node.
Everything else lines up, including where the body understates: the typed path is strictly narrower than the schema path it replaces, no existing caller of Refuse moves onto it (all pass two arguments), and the signature change is source- and binary-compatible.
My two findings are independent of the body — the enum shadowing in Takes/Plural, and the unbounded BigInteger parse. Neither is reachable through a shipped tool today, but both sit in new shared code on the pre-dispatch path of every tool call on both SKUs, and the enum arm is dead the moment anyone adds the parameter it was written for.
Reviewer checklist
- Run
EveryOfferedRange_IsOneEveryRangedReadOnEveryTabTakeswhere Node is installed, with the two plants the body describes: a deferred read on the last tab (must tripfetchesat line 52) and one read refusing 168 hours (must tripnoticesat line 65). Confirmloading == 0is genuinely reached now that every tab at every range is mounted. - Confirm the quoted suite totals were taken after the harness and
WebServerPageRangeTestshunks landed, and reconcile "Total: 59" against 58 rows + 2 facts. - Confirm from the SDK source that a tool created without
SerializerOptionsbinds withMcpJsonUtilities.DefaultOptions, and that nothing beforeJsonSerializer.Deserialize(schema coercion, missing-versus-null for a parameter with a .NET default) lets the binder accept a valueCanBindrejects. - Send a multi-megabyte run of digits to an integer parameter on a running host and time the refusal, to size the
OutOfRangeparse. - Verify on a live host that
request.ServicesresolvesMcpToolParameterTypesin HTTP mode as well as in-process: a real call withhours_back: nullmust come back naminghours_back.
…un of digits too long for any integer type by its length (#4956) An enum parameter was worded as a whole number. Type.GetTypeCode answers an enum's underlying integer type, so Bounds matched every enum: a value the binder could not read for one (0.5, a name no member has) was refused as "a whole number ... with no decimal point", and a list of an enum as "a list of whole numbers", with no word of the names the parameter takes. Bounds now returns null for an enum, so the refusal says "one of Crimson, Teal, Amber" and "a list (a JSON array) of ProbeColor values". The whole-number check handed the argument's full text to BigInteger.TryParse, and the caller sets the text's length; the parse costs more than the length grows (about 590 ms for 1,000,000 digits). The widest integer type holds 20 digits, so after the leading zeros a longer run is out of every range, and is now answered by its length and sign without being parsed (about 3 ms for the same value). Every shorter value gets the verdict it got before. Tests, in McpUnknownArgumentGuardTests: an enum parameter, and a list of one, are refused by their member names (it failed before the change with the whole-number wording); a 100,000-digit value for an int is refused as too large, and with a leading minus as too small, as a JSON number and as text; a plus sign and 30 leading zeros keep their verdicts, and a signed run of zeros is not worded as out of range (it fails if the leading zeros are not trimmed).
Blind-read notes (detail behind the verdict)Trap that applies. Traps checked and not applying.
Wiring I checked rather than assumed. No MCP tool-creation site sets Shipped-type coverage. Performance, non-blocking. Every argument of every call is now deserialized once by the guard and again by the binder. Cheap for the integer and bool parameters this started with; a large |
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
One substantive finding: the refusal now states the CLR type's range as what the parameter "takes", contradicting the range this same server's validator states for hours_back/limit/top. The rest holds up. Fuller trap-by-trap notes are in a separate comment on this PR.
Claims
- VERIFIED —
McpToolParameterTypesis filled byWithGeminiCompatibleToolsas each tool is created, one instance found-or-added the wayGuideCatalogOfis —McpSchemaCompat.cs:125,:166-172,:216-231 - VERIFIED —
Refusetakes the record as an optional third argument; the filter reads it fromrequest.Services—McpUnknownArgumentGuard.cs:100,:112-113 - VERIFIED — Values are read into the declared type with
McpJsonUtilities.DefaultOptions, refused only onJsonException, everything else passed —McpToolParameterTypes.cs:138-153, pinned byAValueWhoseConverterThrowsAnythingButAJsonError_PassesToTheBinder - VERIFIED — A tool with no record keeps the schema-only check, which passes null —
McpUnknownArgumentGuard.cs:152-162,:171-184,:265-271 - VERIFIED —
Boundsreturns null for an enum, in a block body the convention guard reads whole —McpToolParameterTypes.cs:158-163 - VERIFIED —
OutOfRangeanswers by length and sign past 20 digits, after trimming leading zeros —:211-230 - VERIFIED — The example refusal is exactly what the format strings produce —
:116-131,:181-189. This is the finding, not a defence of it. - VERIFIED — 58 rows, one file linked into both test projects —
McpArgumentTypeRows.Table(),Lite.Tests/Lite.Tests.csproj:46-48; the probe tool has noMcpServerToolTypeattribute so no census sees it - VERIFIED —
doubleandstring[]really are declared by shipped tools on both SKUs —double min_severity,string[]? tools/topicsonget_tool_guide - VERIFIED — Items a and b: the test checks the harness's whole
fetcheslist, andofferedRangesnow mounts each tab under awhere-labelled holder and fails on any notice, error or loading panel —WebServerPageRangeTests.cs:52,:64-72; harness:275-278,:300,:318 - VERIFIED —
TextElementCutLength's comment now limits its cost claim to text ASCII at the cut —McpHelpers.cs:90-92vs:106-131 - VERIFIED — No tool-creation site sets
SerializerOptions, and both hosts install the one shared filter —McpSchemaCompat.cs:174-183,DarlingMcpHostService.cs:1053,Lite/Mcp/McpHostService.cs:165 - UNVERIFIED — Every RED/GREEN run, test count and the
BigIntegertiming table: local runs with plants deliberately absent from the diff, and no .NET or Node run is available here - UNVERIFIED — "the binder half passed for every row before the change": needs the unguarded host run against ModelContextProtocol 2.2.0, the premise the typed check rests on
- UNVERIFIED — "CI on the round's head: pending", and the unchecked whole-Darling-suite box
- REFUTED — Item c, "two tests in
McpUnknownArgumentGuardTests... do reach the tool body": one does.:325is refused on the unknown key before the body; only theHOURS_BACKcall at:346binds. Those are the only two tests there that start the host (:328,:349). - REFUTED —
McpUnknownArgumentGuardTests.cs:422, "The refusal for an enum, or a list of one, names its members":Pluralhas no enum arm, so a list readsa list (a JSON array) of ProbeColor values(McpToolParameterTypes.cs:450-455) — as the test's own assertion at:559and the PR body both say - REFUTED —
McpToolParameterTypes.cs:36, "only what a caller may send is here":IsService(typeof(CancellationToken))is false, socancellationTokenis recorded for every Darling tool declaring one (DarlingMcpDataTools.cs:64) though the schema omits it. Harmless; the unknown-key pass fires first.
Riskiest lines
PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:142— the deserialize IS the correctness premise; refuse one notch more than the binder and working calls die on every tool, both SKUs — pinned byMcpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt(guarded and unguarded host per row)PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:188—RangeOf's sentence, the only new line claiming something about the tool's contract rather than the binder's behaviour — wording pinned byAHundredThousandDigitValueForAnInt_IsRefusedAsTooLargeOrTooSmall, truthfulness against the parameter's real bounds UNPINNEDPerformanceMonitor.Common/Mcp/McpSchemaCompat.cs:172—Registermoved inside the tool factory, so the record exists only once the tool singletons resolve — pinned indirectly byEveryAdvertisedParameter_HasARecordedType_ThatAgreesWithTheSchemaand theint/long/boolnull rows
Traps near this diff
hours_back, limit, top, days_back are refused when out of range, never clamped — a clamp is a silent lie about the window— APPLIES: nothing is clamped, but the refusal now sayshours_backtakes-2147483648 to 2147483647whileMcpHelpers.ValidateHoursBack(McpHelpers.cs:158-162) refuses everything outside 1-168Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit— does not apply: sharedPerformanceMonitor.Commononly, one filter on both hosts, one linked table (Lite.Tests/Lite.Tests.csproj:46-48)Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy.— does not apply:McpArgumentTypeRows.csis aCompile Include ... Linkentry, one file compiled twiceAn expression-bodied member holding a { } property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)— applies and is handled:McpToolParameterTypes.cs:108,:158,:181,:197are block bodiesA pin that encodes the lie ... Ask what a pin would fail ON (#3699)— does not apply: each row asserts both halves, so the unguarded host must independently throw on exactly the refused rows (McpArgumentTypeRows.cs:294-303)A roster-based census does not see the file it does not roster (#3699)— does not apply:EveryClrTypeAShippedToolDeclares_IsOneTheTableCoversreflects over the host's registered tool typesInternalsVisibleTo ... a helper Darling.Tests must call has to be public (#3688)— does not apply:McpToolParameterTypes.cs:12,:26are public;McpArgumentValueCheckstays internal behind the publicRefuse
Divergence (blind read vs PR body)
1. The range sentence. The body prints the new hours_back refusal verbatim as an illustration of format and never asks whether the range is true. It is not: get_wait_stats refuses hours_back outside 1-168, so the server emits two refusals with contradictory ranges for one parameter, and the guard's invites a retry at a negative or 20,000-hour value. My blind read flagged this as the only new line making a claim about the tool's contract; the body treats it as settled formatting. Before this PR the message made no range claim. Fix suggestion inline on McpToolParameterTypes.cs:188.
2. Item c names the wrong tests. One test in McpUnknownArgumentGuardTests reaches the tool body, not two — and neither the body nor the comment names McpArgumentTypeTests, the class this PR adds, whose Unguarded fixture host has no guard and so runs a real Darling tool body for every row the binder reads. That is the consumer that makes the closed-port behaviour matter.
3. The enum-list wording is described two ways in one PR. The body states the limitation correctly; the test doc at :422 claims both an enum and a list of one name their members. The code names only the type for a list.
Elsewhere the two accounts agree, checked against the tree rather than taken: the typed check really is derived from the binder, CanBind really lets every non-JsonException through, OutOfRange really trims zeros before the 20-digit cutoff, and the harness change really leaves the other scenarios' output untouched.
Reviewer checklist
- Run the whole Darling.Tests suite on this head — the body's own unchecked box; the quoted full runs predate the second and third commits
- Run
McpArgumentTypeTestswhere the real SDK binder is exercised, and confirm the UNGUARDED half still throwsJsonExceptionon all 33 refused rows: if the SDK ever treats a null argument as "use the parameter default", theint null/long null/bool nullrows become refusals of calls that used to work - Run
WebServerPageRangeTestswith Node installed — without itTryRunskips and both new checks are silently uncovered - Call a real Lite and Darling host with
hours_back: 3000000000and read the refusal a client receives: confirms the record resolves fromrequest.Servicesoutside the in-process host, and shows the range sentence as a model would see it - Sanity-check the added per-call cost: every argument is now deserialized once by the guard and again by the binder
…names the members of an enum list, and records only the parameters a tool advertises (#4956) A refusal for a whole number past an int no longer quotes the CLR type's range: hours_back is an int, but its own validator takes 1-168, so the two refusals gave contradictory ranges. A list of an enum now says 'names from Crimson, Teal, Amber' like the enum itself, not the CLR type's name. The record of declared parameter types is filled from the schema each created tool serves, so a cancellation token the SDK supplies is no longer in it. The comment on the inert data source names the tests that reach a tool body.
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — record of CLR type + nullability per tool parameter, filled as each tool is created, one instance found-or-added as
GuideCatalogOfdoes —McpToolParameterTypes.cs:24,50-74;McpSchemaCompat.cs:126,185-187,243-258. - VERIFIED —
Refusetakes the record as an optional 3rd argument; the filter reads it fromrequest.Services—McpUnknownArgumentGuard.cs:100,112-113. - VERIFIED — the verdict is
Deserialize(value, declaredType, McpJsonUtilities.DefaultOptions), refusing only onJsonException—McpToolParameterTypes.cs:142-157; noSerializerOptionsis set —McpSchemaCompat.cs:170-176. - VERIFIED — no record / no services keeps the schema-only check, which passes null —
McpUnknownArgumentGuard.cs:157-159,171-184,269.EveryIntegerParameter_...'s body is unchanged, so no pin was weakened. - VERIFIED — no range on either path,
RangeOfdeleted —grep -rn RangeOfempty;McpToolParameterTypes.cs:134-136,McpUnknownArgumentGuard.cs:179-183. - VERIFIED —
Boundsnull for enums, block body; enum refusals name members,Plural's enum arm precedes the type-name arm —McpToolParameterTypes.cs:162-168,258-261,291-297. - VERIFIED —
OutOfRangeanswers >20 digits by length and sign —:203-227;TrimStart('0')at:215precedes the length check (what makesASignedRunOfZeros_...hold), andulong.MaxValueis exactly 20 digits, so the boundary is right. - VERIFIED — round 2 item 4: the predicate is the created tool's advertised property names, so
CancellationTokenis excluded —McpSchemaCompat.cs:167-187,221-237. - VERIFIED — 58 rows, one file linked into both projects, probe tool has no
McpServerToolType—grep -c "rows.Add("= 58;Lite.Tests.csproj:46-48;McpArgumentTypeRows.cs:41-54. - VERIFIED — Darling declares
long/long?, Lite neither —DarlingMcpCustomViewTools.cs:74,DarlingMcpPlanTools.cs:137; nothing underLite/Mcp(my first grep missed the required, default-less ones). - VERIFIED —
doubleandstring[]needed rows —McpAnalysisTools.cs:277,McpHealthTools.cs:812,McpToolGuideTools.cs:29; no parameter type in either tool tree falls outside the table's 14 keys. - VERIFIED — items a, b, d —
WebServerPageRangeTests.cs:50-56,58-69; harness:270-300,311-323;McpHelpers.cs:90-92. Other scenarios set nowhere, so theirstripsoutput is unchanged. - UNVERIFIED — "
McpJsonUtilities.DefaultOptionsis what the SDK binds with absent its own" (body says inferred) — MCP 2.2.0 is not in this box's NuGet cache and no .NET SDK is installed, so the binder's option resolution could not be read. The typed verdict rests entirely on it. - UNVERIFIED — every RED/GREEN count (110/0, 70/0, the 59-61 totals, "33 failed", 19952 / 7235) — rig results; needs a build.
- UNVERIFIED — the
JsonNumberHandling.Strictplant failing exactly 4 binder-half rows — a plant not in the diff. - UNVERIFIED — the 1,000,000-digit timing table — a one-off Debug measurement; no clock assertion in the diff, correctly.
- UNVERIFIED — "615 reads, 0 notices, 0 errors, 0 loading" and the two plant failures — needs the Node harness run, which was not available to me. The harness does emit every key the new assertions read (
:325-338). - UNVERIFIED — "CI on this head: running when this was written" — still an open checkbox; the PR is a draft.
Riskiest lines
PerformanceMonitor.Common/Mcp/McpUnknownArgumentGuard.cs:100— the only read ofRequestContext.Servicesin the repo; a null or non-resolving scope reverts every argument to the schema-only path silently. Pinned byMcpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadItin both SKUs, but only overWithStreamServerTransport(McpInProcessHost.cs:114) — UNPINNED onWithHttpTransport(Stateless = true), which both products ship.PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:146— a false refusal breaks a working call, and only "same options as the binder" prevents it. Pinned by both halves ofMcpArgumentTypeRows.CheckAsync(:329-351): guard-less host says what the binder does, guarded host what the guard does, same tool and value.PerformanceMonitor.Common/Mcp/McpSchemaCompat.cs:186—Registeras a side effect in theAddSingletontool factory, so the record fills lazily while the catalog fills eagerly; safe only becauseFindToolreads aToolCollectionthat exists once every factory has run. Pinned byEveryAdvertisedParameter_HasARecordedType_...andEveryRecordedParameter_IsOneTheSchemaAdvertises, which sweephost.RegisteredToolsin both SKUs.
Traps near this diff
- "Lite and Darling ship in parity ... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply: the change is all in shared
PerformanceMonitor.Common, both hosts register the one filter (Lite/Mcp/McpHostService.cs:165,DarlingMcpHostService.cs:1053), and the table plus all three facts land in both test projects. - "An expression-bodied member holding a
{ }property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — does not apply:RangeOfis gone,Problem'sis { } boundsis in a block body (McpToolParameterTypes.cs:109-140), and the one new instance (WebServerPageRangeTests.HoursAsked:93-94) is in a directory the scan skips (TsqlConventionGuardTests.cs:2329,2347). - "A pin that encodes the lie ... Ask what a pin would fail ON" (#3699) — does not apply to the new table (every row is graded against the binder first). It did describe
offeredRanges, which returned[]sonotices/errorscould not fail; the diff fixes that (harness:300). - "A roster-based census does not see the file it does not roster (#3699)" — does not apply: both new facts iterate
host.RegisteredTools(McpArgumentTypeRows.cs:140,209), not a list. - "A census fires on any NEW string constant in
PgTargetFactKeys" (#3688) — does not apply: no fact key is touched. - "
InternalsVisibleToon PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)" — applies, honoured: the record type and its two methods are public (:24,39,50,77),McpArgumentValueCheckinternal (:103). - "
hours_back,limit,top,days_backare refused when out of range, never clamped (#3699)" — applies, honoured: nothing coerces, and round 2 dropped the CLR range because it contradictedValidateHoursBack(:129-136). - "A JSON number above 2^53 is rounded by every JavaScript client (#3487)" — does not apply: nothing emits an id; large numbers are echoed inside a refusal string via
Shorten(McpUnknownArgumentGuard.cs:344). - "A test that reads the clock twice measures the CI scheduler (#3676)" — does not apply: the timings are in the PR body only;
AHundredThousandDigitValueForAnInt_...asserts wording, never a duration. - "A test that constructs a 'four maintenance anomalies in one run' fixture is testing a shape no live run produces (#3709)" — partly applies, handled:
short/byte/ulong/int[]/bool?are shapes no shipped tool has, the probe doc says so (:30-40), andEveryClrTypeAShippedToolDeclares_IsOneTheTableCoverskeeps the real population covered.
Divergence (blind read vs PR body)
No contradiction — every checkable claim came back as the body states it, including two I first read the other way (Darling's required long parameters, which carry no default; and #3667, which HoursAsked matches but the scan excludes). Two places where the body is more confident than the tree:
- "A null for a reference type annotated as non-nullable is not refused, because the binder reads it" is asserted, not pinned.
Resolvematches onParameterType, andstringandstring?are the sameType, so thestring nullrow (McpArgumentTypeRows.cs:288) cannot be aimed at a non-nullable parameter — andAllowsNull, recorded for that distinction, is read only by the test (:179,183), never by the guard. See the inline note. - The plan calls the row targets "first tool in name order that declares the CLR type and needs no other argument" without saying what that resolves to: on Darling the
longrows land ondelete_custom_view, so the theory dispatches a destructive tool on both hosts for everylongthe binder reads — harmless only becauseInertInstanceForpoints the data source at a closed port. See the inline note.
Neither changes my read of the product code, which is sound: the guard borrows the binder's own decision rather than tabulating types, the table grades both halves on one host, the fallback is per-argument, and parity holds through one shared implementation.
Reviewer checklist
- Run
Darling.TestsandLite.Testson Windows with the .NET 10 SDK — the 58-row theory is the entire pin for "cannot refuse a value the binder reads", and neither the SDK nor the MCP package is on this box, so none of it ran here. - Send one
tools/callwith a bad value over each product's shipped stateless-HTTP pipeline and confirm the typed refusal returns (hours_back: null-> "cannot be null").DarlingMcpHostGateLiveTests.SendJsonRpcAsyncis the seam. - Run
node web-kept-history-harness.mjs <wwwroot/js> offeredRanges:168and confirm ~615 reads, 0 notices, 0 errors, 0 loading. - Confirm nothing the theory now reaches works outside the dead data source: bodies run for ~28 rows x 2 hosts x 2 SKUs, including
delete_custom_view. - Eyeball two strings the format allows: "...2026-10-02T14:30:00Z and cannot be null, and the call sent null." (no comma before "and cannot"), and
"-0"for an unsigned parameter landing on "with no decimal point, such as 1".
…ned zero too small for an unsigned type, and says once when its type record does not resolve (#4956) String check. A string takes a String token or a Null one and nothing else, so CanBind decides it from the token and no longer reads the value into a string. Reading it copied the whole value only to throw the copy away, and the binder reads it again right after, so a 10 MB plan_xml was held a second time for nothing. A new pin measures the allocation of a 5,000,000-character string: 10,000,024 bytes before, under 100,000 now. Every existing row is unchanged. Transport pin. The typed path takes its record from the request's services, and only the in-process stream server pinned that, while both products ship stateless HTTP. A tools/call row over the shipped Darling HTTP pipeline sends null for hours_back (only the typed path refuses it) and asserts the typed wording. With the record lookup planted to return nothing the row fails with "expected the argument guard's refusal, got: An error occurred invoking 'get_alert_history'." Lite builds its server inline in its host service and has no HTTP harness that reaches tools/call, so that half is pinned for Darling only. Because both products share the guard, it now logs one warning (per process, through the request's logger factory) when the record does not resolve, instead of degrading to the schema alone without a word. Messages. A null for a parameter that cannot take one now reads "takes a whole number of hours, and cannot be null, and the call sent null." A "-0" sent for an unsigned type is "too small" instead of "with no decimal point, such as 1" (the call had no decimal point). Design call: the unsigned type reads no minus sign at all, so any minus-signed text for it is too small, including a run of zeros; the older pin that a signed run of zeros is never worded as out of range now holds for a plus sign and expects "too small" for a minus. The new row (ulong, -0) failed before the change with "the refusal does not say 'too small' -> ... with no decimal point, such as 1, and the call sent -0." Docs. AllowsNull says only the tests read it, as a cross-check, because the refusal asks the binder whether it can read the value. The guard's class doc says the same, and that it reads every argument once before the binder reads it again, so a caller-sized list is read twice. Test rows. McpArgumentTypeRows.Resolve passes over a served tool whose name starts with delete_, create_ or update_ (ordinal), since a row that sends a value the binder reads runs the tool. Every row still finds a target. Targets that changed in Darling: int? create_mute_rule.server_id is now get_lock_wait_trend.bucket_minutes, and long delete_custom_alert_rule.rule_id is now get_custom_alert_rule.rule_id. Lite serves no such tool, so none of its targets changed. Node-backed page tests: WebRangeKeptHistoryBehaviourTests.TryRun already calls Assert.Skip when Node is missing, so nothing changed there. With Node removed from the path, the class and WebServerPageRangeTests report 10 skipped, 2 passed.
Blind-read notes, before the bodyI read the diff and the tree first, then the PR body. Two things in the body do not match the tree, and one roster in the new shared test helper does not keep the contract its own comment states. Everything else I could check from a read-only tree held up — and the typed path is a genuinely good design: because Where the body and the tree disagree1. "On Darling the This cuts in the PR's favour — no delete body runs at all — but it is the sentence the test plan uses to carry the "it reaches no store" reassurance, so the reassurance currently rests on a tool the code refuses to call. The body also never mentions 2. "58 rows" / "it totals 61 per product now (58 rows, 3 facts)." Coverage gap worth a human's eye: Lite's shipped HTTP path
I am not calling this a parity finding: the decision lives once in Traps I checked and ruled out
Verified from the tree, for the record
|
Its findings are fixed at 8f65d09, and each thread has a reply.
The table has 59 rows, not 58 — and the derived totals move with it
The test plan says 58 in three places, and builds arithmetic on it:
With 59 rows that last figure is 62 per product, and the "59 = 58 rows plus one fact" reconciliation for the RED commit no longer reconciles. Counting the table by group ( The likely cause is that the count predates commit What a human still has to checkEverything load-bearing in this change is pinned by a test, and nothing in it can be read off a static tree:
Read-only notes that came out clean, recorded so they are not re-litigated: |
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Detail is in the top-level note and three inline comments on this PR; this is the ledger.
Claims
- VERIFIED — the record is filled as each tool is created from the created tool's served schema, and the one instance is found-or-added on the same service collection as
GuideCatalogOf—McpSchemaCompat.cs:126,:185-186,:243-258. - VERIFIED —
Refusetakes the record as an optional third argument; the filter reads it fromrequest.Services—McpUnknownArgumentGuard.cs:153-154,:103-123. - VERIFIED — "refuses exactly when
System.Text.Jsonthrows aJsonException; anything else passes" —McpToolParameterTypes.cs:157,166, pinned byAValueWhoseConverterThrowsAnythingButAJsonError_PassesToTheBinder. - VERIFIED — a
stringis decided from its token with no deserialize (McpToolParameterTypes.cs:145-151, pinned byALargeString_IsPassedWithoutBeingCopied); the refusal states no range and an enum is excluded fromBoundsin a block body (:137-147,:173-180,:315, pinned byAssertStatesNoRangeandHoursBackPastInt_IsRefusedAsTooLarge_AndStatesNoRangeon both SKUs); the digit walk trims zeros, short-circuits past 20 digits and otherwise parses as before (:201-255). - VERIFIED — the schema-only fallback is reached on both the null-record and missing-key routes and still passes null —
McpUnknownArgumentGuard.cs:196-203,:212-225,:306-312. - VERIFIED — the
TextElementCutLengthcomment now matches the walk (McpHelpers.cs:87-92vs:106-131); the test checks the harness's wholefetcheslist printed after its finalsettle()(web-kept-history-harness.mjs:309,326); onlyofferedRangesmarks nodes, so other scenarios' output is byte-identical (:275-278,300,313-323); "Lite has no HTTP harness that reachestools/call" (Lite.Tests/HostHeaderGuardTests.cs:69,82-83read source text only) and "Lite's host clears its logging providers" (Lite/Mcp/McpHostService.cs:87). - UNVERIFIED — every RED/GREEN block, the suite totals, the four plants, the allocation figures and the
BigIntegertiming table. Needed: a build anddotnet teston this head; no SDK is runnable here. - UNVERIFIED — "agreed with the real binder on all … rows on both products". That lives in the
installGuard: falsehalf ofEachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt; only a run answers it. - UNVERIFIED — "Every row still finds a target", and the named
int?/longrow moves.Resolvewalks reflection order over the live tool collection, and a miss throws rather than asserts. - REFUTED — "58 rows" (three times) and "it totals 61 per product now (58 rows, 3 facts)".
grep -c "rows.Add(" Darling/Darling.Tests/McpArgumentTypeRows.csreturns 59; by group at:287-365it is 59, so the total is 62. One row stale: commit09d9ae5ce's own item 4(b) added "a newulongrow" (-0, line 324). - REFUTED — "the pick passes over
delete_,create_orupdate_, so no row runs a tool that changes data". The doc directly above the check says the opposite (McpArgumentTypeRows.cs:425-427): it "covers those three prefixes only, so a write tool named otherwise (add_servers,remove_server,set_mute_rule_enabled) is not passed over". Lite addsmute_analysis_finding, describedWrite:(Lite/Mcp/McpAnalysisTools.cs:996). Arefused: falserow runs the resolved body on both hosts. - REFUTED — "the data source the rows get also points at a closed local port … so a row reaches no store", said of both products. That is Darling's. Lite passes
_ => nullfor inert instances (Lite.Tests/McpWholeNumberArgumentTests.cs:49), so every Lite tool service is aRuntimeHelpers.GetUninitializedObject(McpInProcessHost.cs:107).
Riskiest lines
PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:157— this deserialize IS the refusal verdict; wrong options mean refusing calls the server would have answered — pinned byMcpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt(both SKUs)PerformanceMonitor.Common/Mcp/McpSchemaCompat.cs:186—Registerruns inside theAddSingleton<McpServerTool>factory; a record that never fills degrades the whole surface behind one process-wide warning — pinned byDarlingMcpHostGateLiveTests.ToolsCall_ANullForAWholeNumber_IsRefusedByName_OverTheShippedHttpPipeline; Lite's HTTP transport (Lite/Mcp/McpHostService.cs:112) is UNPINNEDPerformanceMonitor.Common/Mcp/McpUnknownArgumentGuard.cs:196-203— the value pass widens to every argument of every tool, and every non-string argument is deserialised twice — pinned by the 59-row table on both SKUs
Traps near this diff
- "A pin that encodes the lie … Ask what a pin would fail ON." (#3699) — applies —
EveryRecordedParameter_IsOneTheSchemaAdvertisescomparesrecorded.Keys, built fromInputSchemaatMcpSchemaCompat.cs:185-186, against that sameInputSchemaatMcpArgumentTypeRows.cs:216-222, so the subset holds by construction. - "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply as a defect — the record is derived from the served schema (
McpSchemaCompat.cs:185-186), the right side of this trap; it is only why the pin above is near-tautological. - "An expression-bodied member holding a
{ }property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — does not apply —Problem(McpToolParameterTypes.cs:112),Bounds(:173),OutOfRange(:201),CanBind(:145),Takes(:264) andWarnOnceThatTheRecordDidNotResolve(McpUnknownArgumentGuard.cs:129) are block bodies. - "
InternalsVisibleToon PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)." — does not apply —PerformanceMonitor.Common.csproj:41,46name both test projects, which is what lets them call theinternalMcpArgumentValueCheck.Problem. - "
hours_back,limit,top,days_backare refused when out of range, never clamped (#3699)." — applies and is honoured —McpUnknownArgumentGuard.cs:205refuses and never substitutes a default, and dropping the CLR range exists so the guard cannot contradictValidateHoursBack's 1-168 (McpToolParameterTypes.cs:137-147). - "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit." — does not apply — the change is entirely in shared
PerformanceMonitor.Common, registered identically by both hosts (Lite/Mcp/McpHostService.cs:165,DarlingMcpHostService.cs:1053), and the row table is linked into both test projects viaLite.Tests/Lite.Tests.csproj.
Divergence (blind read vs PR body)
Three places, all in the account rather than the shipped guard; the body is consistently stronger than the code it describes. (1) The row count is 58 in the body and 59 in the tree, so the "61 per product" figure and the RED commit's "59 = 58 rows plus one fact" reconciliation each come out one off. (2) The body concludes "so no row runs a tool that changes data"; the comment directly above the check declines to support that and names three tool shapes the prefix list misses, to which Lite adds mute_analysis_finding — on today's reflection order no write appears to be selected, but that is luck, not the mechanism the body credits. (3) The body credits a closed-port data source for both products, where Lite's protection is in fact an uninitialized object. Two further statements are accurate for Darling and silently generalised: "the order the host registers them" (Lite orders by Type.FullName) and the HTTP tools/call coverage, which the body does disclose.
Nothing in the diff goes unmentioned by the body, and I found no shipped-code defect: the fallback routing, the enum exclusion, the bounded digit walk, the string token short-circuit, the non-JsonException escape hatch and the removal of the CLR range all read correctly and are each pinned by a named test.
Reviewer checklist
- Build and run
McpArgumentTypeTestson both SKUs; theinstallGuard: falsehalf is the only thing between this change and refusing traffic the server used to answer, and a read cannot evaluate it. - Confirm the
int nullrow (McpArgumentTypeRows.cs:305) is green — it decides whether an explicitnullfor a defaultedintpreviously bound to the default, in which case this widens refusals into working calls. - Confirm
Resolvefinds a target for all 14 type keys on both SKUs; a miss is anInvalidOperationException, anddouble/double?/string[]resolve through different tools on each. - Issue a real
tools/callwith{"hours_back": null}against a running Lite host (Lite/Mcp/McpHostService.cs:112) and confirm the typed refusal; Lite'srequest.Servicespath has no pin and the fallback warning is swallowed byClearProviders()at:87. - Watch
Assert.Equal(0, loading)inWebServerPageRangeTestsover a few CI runs, and note it skips entirely on an agent without Node. - Correct the body's row count, write-tool sentence and Lite inert-service sentence before this leaves draft.
…ds do, and the row resolver's doc names the type-name order and the stand-ins that keep a row off a store (#4956)
Two findings are fixed in 8afedba, and the third is answered in its thread.
|
|
||
| var type = Nullable.GetUnderlyingType(declared.Type) ?? declared.Type; | ||
| var takes = Takes(parameter, type); | ||
| var sent = McpUnknownArgumentGuard.Shorten(value.GetRawText()); |
There was a problem hiding this comment.
Non-blocking, and pre-existing in shape: the string fast path in CanBind (line 150) avoids copying a caller-sized value on the pass path, and ALargeString_IsPassedWithoutBeingCopied pins that. The refusal path still materialises the whole value. For get_wait_stats {"hours_back": "<5 MB of digits>"} the guard now copies it up to three times before the message is built: JsonSerializer.Deserialize (line 157) writes the element out before reading it, Shorten(value.GetRawText()) here copies it again, and OutOfRange calls GetRawText() twice more for a Number token (line 205) or GetString() for a String one (line 206).
Nothing here is a regression — #4945's WholeNumberEnvelope already did Shorten(argument.Value.GetRawText()) and BinderReadsAsInteger already did value.GetString(), and the request body was that size already, so there is no amplification. Raising it only because the digit-run bound at line 235 and the allocation pin both exist to keep a caller from setting the guard's cost, and the copies are the one part of that cost the two measures do not cover. A value.GetRawText().Length > 40 short-circuit (or reading the first 40 units off the writer) would close it if it is worth closing.
|
|
||
| var problems = arguments | ||
| .OrderBy(argument => argument.Key, StringComparer.Ordinal) | ||
| .Select(argument => (argument.Key, Sentence: declared is not null && declared.TryGetValue(argument.Key, out var type) |
There was a problem hiding this comment.
Non-blocking, a coverage shape worth naming: this line widens the value check from "integer parameters, judged by the schema" to "every argument, judged by its declared CLR type" — so from here on the guard can refuse a value for a string, a bool, a double or a string[] parameter, which it never could before.
The fleet-wide census that pins the inverse (the guard must not refuse a value a tool takes) does not see this path. Darling/Darling.Tests/McpUnknownArgumentGuardTests.cs:289 and Lite.Tests/McpUnknownArgumentGuardTests.cs:212 both call the two-argument Refuse(Call(name, everything), tool), so with types defaulting to null every one of the ~400 shipped tool parameters is still graded by SchemaProblem alone. The typed path's no-false-refusal property rests instead on (a) the structural argument that CanBind is the binder's own deserialize, (b) McpArgumentTypeRows — one tool per CLR type, not one row per tool, and (c) RecordedTypeProblems, which checks the recorded type agrees with the schema's word but never sends a value through it.
That is a defensible split, and (c) covers the risk I would actually worry about (a mis-recorded type). But the census that would fail ON a false refusal across the whole surface is the one census that was not extended, and extending it is cheap: pass _hosts.Guarded.ParameterTypes as the third argument and pick the sent value off the recorded McpParameterType rather than off the schema's "integer" word.
Blind read, then the body: the row count in the body is stale by oneThe table in the tree has 59 rows, not 58: The body's own account says why: the fourth round ( Three claims read on the current tree rather than on the commit they were measured against, and two of them are now wrong:
Nothing in the code is wrong and no pin changes meaning — the Everything else I checked heldThe three riskiest lines and what pins each, plus the traps I walked, are in the formal review. The parts I went looking for and did not find:
Two non-blocking notes are inline: the refusal path still materialises a caller-sized value (pre-existing, no amplification), and the fleet-wide "never refuses a value a tool takes" census is the one census that still runs only the schema-only fallback. |
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Detail for every item below is in the top-level comment on this PR; this is the ledger.
Claims
- VERIFIED — the record is filled as each tool is created, from the parameters the created tool's served schema lists, one instance found-or-added as
GuideCatalogOfdoes —McpSchemaCompat.cs:126,:167-186,:243-258against:200-215 - VERIFIED —
Refusetakes the record as an optional third argument; the filter reads it fromrequest.Services—McpUnknownArgumentGuard.cs:153-154,:105,:116 - VERIFIED — the value is read into the declared type with
McpJsonUtilities.DefaultOptions, refused only onJsonException, so the guard cannot refuse a value the binder reads nor throw out of the filter —McpToolParameterTypes.cs:157,:160-167;McpSchemaCompat.cs:170-176sets noSerializerOptions - VERIFIED — no record, or no services, keeps the schema-only check, which passes null —
McpUnknownArgumentGuard.cs:193-203,:212-225,:310 - VERIFIED — the refusal quotes no range,
RangeOfis gone, andBoundsexcludes enums in a block body —McpToolParameterTypes.cs:137-139,:173-179;Plural's enum arm at:315precedes:316 - VERIFIED — a
stringis decided from its token, with no deserialize —McpToolParameterTypes.cs:150-153, ahead of thetry - VERIFIED — the 20 digits are counted after the leading zeros, and 20-or-fewer keeps the old verdict —
McpToolParameterTypes.cs:234-246;ulong.MaxValueis exactly 20 digits - VERIFIED — one warning per process, invisible in Lite because its host clears logging providers, read by no test —
McpUnknownArgumentGuard.cs:129-142,Lite/Mcp/McpHostService.cs:87 - VERIFIED — the HTTP row's null is a real proof:
get_alert_history.hours_backis a non-nullableint—DarlingMcpAlertTools.cs:83 - VERIFIED — the
TextElementCutLengthcost claim is now limited to text ASCII at the cut —McpHelpers.cs:90-92against the unbounded backward scan at:107-110 - VERIFIED — the whole-run read check uses the harness's own convention —
HoursAsked's[?&]hours=matchesaskedatweb-kept-history-harness.mjs:155; both treat an absenthoursas 0, so it is a strict superset ofbeyond - VERIFIED — the
longstory holds in both SKUs, and every CLR type a shipped tool declares has a row — Darling declares both (DarlingMcpCustomAlertTools.cs:209,:324); Lite's onlylongisMcpPlanTools.cs:129, required besidedatabase_name; the sweep yields only typess_typeskeys - UNVERIFIED — "agreed with the real binder on all table rows", and the RED/GREEN ledger (Darling 19952 / Lite 7235 / 0 failed; CI green on
d392f2ec4) — run results; the edge rows turn on SDK/runtime behaviour that is measured, not read - UNVERIFIED — the 10,000,024-to-under-100,000 allocation figures and the
BigIntegertiming table — the budget and the mechanism are in the tree; the numbers need a run - UNVERIFIED — "615 reads ... 0 notices, 0 errors and 0 panels left loading" — needs Node; the class skips without it, which the body states
- REFUTED — "58 rows" and "it totals 61 per product now (58 rows, 3 facts)" —
grep -c "rows.Add("onDarling/Darling.Tests/McpArgumentTypeRows.csis 59, with 3[Fact]s, so 62 per product. The body's own item 4b added "a newulongrow" (McpArgumentTypeRows.cs:230) in09d9ae5ceafter the counts were written. The RED paragraph's scoped "58" is correct as written
Riskiest lines
- PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:157 — the deserialize probe carries the whole correctness claim and now runs on every argument of every call — pinned by
McpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIton both SKUs, whose guard-less half fails if the guard and the binder ever disagree - PerformanceMonitor.Common/Mcp/McpSchemaCompat.cs:186 —
Registerinside the DI factory: the record fills lazily and is keyed by the served name, so a provider mismatch degrades a tool to the schema-only path behind one warning — pinned byEveryAdvertisedParameter_HasARecordedType_ThatAgreesWithTheSchema,EveryRecordedParameter_IsOneTheSchemaAdvertises, andDarlingMcpHostGateLiveTests.ToolsCall_ANullForAWholeNumber_IsRefusedByName_OverTheShippedHttpPipeline - PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:229-238 — the sign/zero/length arithmetic words "too large" versus "too small" without parsing — pinned by
AHundredThousandDigitValueForAnInt_IsRefusedAsTooLargeOrTooSmall,ASignAndLeadingZeros_DoNotChangeWhichWayAWholeNumberIsOutOfRange,ASignAndLeadingZeros_OnAWholeNumberThatFits_AreNotRefused,ASignedRunOfZeros_IsWordedAsTooSmallOnlyWhenItCarriesAMinus
Traps near this diff
- "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — one shared implementation, the same filter object in both hosts (
Lite/Mcp/McpHostService.cs:165,DarlingMcpHostService.cs:1053), pins on both sides (Lite.Tests/Lite.Tests.csproj:46-48,Lite.Tests/McpWholeNumberArgumentTests.cs:137) - "
InternalsVisibleToon PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)" — does not apply —PerformanceMonitor.Common.csproj:41,46names both, and the cross-linkedMcpArgumentTypeRows.cscalls only public members - "An expression-bodied member holding a
{ }property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — applies, and is handled — every member holding such a pattern is a block body (McpToolParameterTypes.cs:173-192,McpUnknownArgumentGuard.cs:129-142,:153-206); the body records the RED on the since-deletedRangeOf - "A pin that encodes the lie ... Ask what a pin would fail ON (#3699)" — does not apply — each row is graded against the binder on a guard-less host before the guard sees it (
CheckAsync, "Half one") - "
hours_back,limit,top,days_backare refused when out of range, never clamped (#3699; #3641)" — applies, and holds —McpToolParameterTypes.cs:137-139substitutes nothing, and dropping the CLR range leavesMcpHelpers.ValidateHoursBackas the only range a caller is told - "A JSON number above 2^53 is rounded by every JavaScript client (#3487)" — does not apply — the quoted value is spliced into the message string (
McpToolParameterTypes.cs:121,:138), never emitted as a number - "A text cut at a UTF-16 index can land between the halves of a surrogate pair (#3625)" — applies, and holds —
Shortenstill routes throughMcpHelpers.TruncateandTextElementCutLength(McpUnknownArgumentGuard.cs:385) - "A roster-based census does not see the file it does not roster (#3699)" — does not apply — the new censuses sweep by reflection over the attribute scan, not a hand roster
Divergence (blind read vs PR body)
One, bookkeeping rather than mechanism: the present-tense row count is stale by one. The table holds 59 rows, so "58 rows" is wrong and "61 per product now" should read 62; the cause is the body's own item 4b, which added the ulong -0 row after the counts were written. No pin changes meaning and no code is affected — it matters only because this repository's bodies are read as the census they describe.
In the other direction I found no mechanism the body names that the diff does not implement, and no change the diff makes that the body does not mention. Two things it could have overclaimed and did not: AllowsNull is recorded but read only by the tests, and a null for a non-nullable reference type is not refused because the binder reads it — stated, with the admission that no row pins it.
The one coverage shape I would add is inline: the fleet-wide "never refuses a value a tool takes" census (Darling/Darling.Tests/McpUnknownArgumentGuardTests.cs:289, Lite.Tests/McpUnknownArgumentGuardTests.cs:212) still calls the two-argument Refuse, so it grades every shipped parameter on the schema-only fallback and never on the path this PR introduces.
Reviewer checklist
- Re-foot the body's counts (59 rows, 3 facts, 62 per product), or scope each to the commit it was measured on as the RED paragraph does.
- Run
McpArgumentTypeTestson both SKUs:Assert.Null(ProbeRefusal("color", "\"Amber\""))andASignAndLeadingZeros_OnAWholeNumberThatFits_AreNotRefusedrest on SDK and runtime behaviour a read-only tree cannot see, and an SDK bump would move them. - On a live host, confirm the warning at
McpUnknownArgumentGuard.cs:137does NOT appear in Darling's log — it is the only signal that the record resolved in the shipped process, and the degraded path is silent by design. - Watch peak memory for one
get_tool_guidecall with a largetoolsarray, now that the guard deserialises every argument before the binder does (McpToolParameterTypes.cs:157). - Run
WebServerPageRangeTestswith Node present and confirm the 615 reads / 0 notices / 0 errors / 0 loading figures — without Node the class skips, so CI can report green on a scenario that never mounted.
Closes #4956: items a, b, c and d are all done here, plus the checks on the new guard that came up in the first pass: the exceptions
CanBindlets through, the binder-options assumption, an enum parameter that read as a whole number, and a digit parse whose cost the caller could set. The second pass adds four more, each below: the refusal states no range, a list of an enum names its members, the record holds only the parameters a tool advertises, and a comment that named the wrong tests is corrected.Follows #4945. It also makes the MCP argument guard check each value against the parameter's real type, so a value the SDK cannot read is refused by name instead of coming back as "An error occurred invoking ...".
Why
int, along, ashortand abytealike, has noformat, and does not say which parameters take null. So these still reached the SDK and got its generic error: anintpast int range, null for a parameter that is not nullable, ashortorbytepast range, an integer array with a bad element, a number sent for a string, a string sent for a boolean. seen (the rows below fail on both products before the change).ulongsuch as 18446744073709551615 (it only accepted what fits along), although the binder reads it. seen (RED lines below). No shipped tool declares aulong(seen: the table's tool lookup fell through to the test-only tool), so no shipped tool was affected; inferred.intsaidtakes a whole number of hours from -2147483648 to 2147483647forhours_back, whileMcpHelpers.ValidateHoursBackrefuses anything outside 1-168, so the server gave two refusals with contradictory ranges and invited a retry the validator refuses. seen (RED line, item 1). The record of declared types held parameters the served schema leaves out: aCancellationTokenwas recorded for every tool that declares one, although the class doc says only what a caller may send is there. seen (RED line, item 4). A refusal for a list of an enum saidof ProbeColor values, a CLR type name, where the refusal for the enum itself names its members. seen (RED line, item 2).What changes
PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs.McpToolParameterTypesholds, per tool and per parameter, the declared CLR type and whether it takes null.McpSchemaCompat.WithGeminiCompatibleToolsfills it as each tool is created, from the parameters the created tool's served schema lists (the second pass; it was theSchemaOptionsForrule, which let aCancellationTokenthrough), and registers the one instance in the same service collection (found-or-added, asGuideCatalogOfdoes for the guide catalog).McpUnknownArgumentGuard.Refusetakes the record as an optional third argument; the call-tool filter reads it fromrequest.Services. For each argument it reads the value into the declared type withMcpJsonUtilities.DefaultOptions(the options the SDK binds tool arguments with; inferred from the SDK, and agreed with the real binder on all 58 table rows on both products, seen) and refuses exactly whenSystem.Text.Jsonthrows aJsonException. So the guard has no per-type rules of its own and cannot refuse a value the binder reads. Anything else the serializer does (no converter for the type) passes.Argument 'hours_back' for tool 'get_wait_stats' takes a whole number of hours, and the call sent 3000000000, which is too large.EveryIntegerParameter_...doc comment now says so.McpWholeNumberArgumentTestssays the source is not opened by that class, and names the places that share the host and do reach a tool body, where the one-second connect timeout keeps them fast. Its first wording named the wrong tests; the second pass, item 3, corrects it.McpHelpers.TextElementCutLengthcomment now limits the "512,000-unit limit costs no more than a 40-unit one" claim to text that is ASCII at the cut; otherwise the walk goes back toward the start and the cost grows with the limit.WebServerPageRangeTests.EveryOfferedRange_IsOneEveryRangedReadOnEveryTabTakesnow also checks the whole list of reads the run made (the harness'sfetches, read after its finalsettle()) againstMcpHelpers.MaxHoursBack. Before, each tab's reads were checked right after that tab's ownsettleFast(), so a read made later (behind a timer, say) reachedfetchesbut neverbeyond, and the last tab had nothing after it to be counted in. A read that cannot be tied to a tab now fails the test too.offeredRangesscenario returned no nodes, so nothing was mounted and itsnoticesanderrorswere always empty. It now mounts what each tab builds, under a node that names the range and tab, and the test fails on any notice, error, or panel still loading at an offered range, naming where (the way thecensusscenario already fails on errors). The harness'sstripsputs that name in front of the strips beneath it; the other scenarios mark no node, so their output is unchanged.McpArgumentValueCheck.CanBindreturned false onJsonException, true onNotSupportedExceptionandInvalidOperationException, and let every other exception out ofMcpUnknownArgumentGuard.Refuse, and so out of the call-tool filter. A converter that throws anArgumentExceptionis a case the guard cannot judge. It now returns false only forJsonExceptionand true for any other exception, so the guard never fails a call the binder would answer itself.McpArgumentValueCheck.BoundsusedType.GetTypeCode, which answers an enum's underlying integer type, so it matched every enum.TakesandPluralanswered "a whole number" ("whole numbers" for a list) before reaching their enum arms, andProblemtook the range branch, so a number past the underlyingintwas "too large".Boundsnow returns null for an enum (a block body, so the member scan inTsqlConventionGuardTestsreads it whole). A refusal for an enum now says "one of Crimson, Teal, Amber" (its member names), and for a list of an enum "a list (a JSON array) of ProbeColor values" (its type name, the wording every element type without its own gets). The second pass, item 2, changes the list to "a list (a JSON array) of names from Crimson, Teal, Amber".OutOfRangehanded the argument's whole text toBigInteger.TryParse, and the caller sets the length of that text. The widest integer type (ulong) holds 20 digits, so after the leading zeros a longer run is out of every range. It is now answered "too large" or "too small" from its length and sign, without a parse; a run of 20 digits or fewer is parsed as before (NumberStyles.None, after the sign is taken off). Every value of 20 digits or fewer gets the verdict it got before.McpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt(in both Darling.Tests and Lite.Tests) calls every row ofMcpArgumentTypeRowsthrough the in-process host with the guard left out (installGuard: false) and asserts the SDK binder fails exactly on the rows the guard refuses. It is what notices ifMcpSchemaCompatever creates a tool with its ownSerializerOptions. seen: with theJsonNumberHandling.Strictplant listed under "Binder-options pin" below, 4 rows ofMcpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadItfailed (Darling.Tests only).McpArgumentValueCheck.Problemsaid what the CLR type holds ("takes a whole number of hours from -2147483648 to 2147483647"), which is not the range a tool takes:hours_backis anint, andMcpHelpers.ValidateHoursBackrefuses anything outside 1-168. It now says "takes a whole number of hours, and the call sent 99999999999, which is too large.", the shape the schema-only fallback already used.RangeOfand its doc are deleted.Pluralhas an enum arm, "names from Crimson, Teal, Amber", ahead of the type-name arm, so a list of an enum names its members as the enum itself does.InertInstanceForin Darling.Tests'McpWholeNumberArgumentTestsnamed the wrong tests. Of the two tests inMcpUnknownArgumentGuardTeststhat start the host, one reaches a tool body:WithoutTheGuard_TheBinderDropsAKeyDifferingOnlyByCase, through itsHOURS_BACKcall (the binder drops that key, soget_wait_statsruns at its default window). AndMcpArgumentTypeTestsruns a real tool body for every row the binder reads, on its guard-less host and on its guarded host, which passes those rows on. The comment now says that and keeps the point that the one-second connect timeout on the closed port keeps them fast. Lite.Tests'McpWholeNumberArgumentTestshas no such sentence, so nothing changes there.WithGeminiCompatibleToolsnow creates the tool first, reads the property names of its servedInputSchema, and registers withparameter => parameter.Name is { } name && advertised.Contains(name). Before, the predicate was the schema options'IncludeParameter, which leaves out DI services but not aCancellationToken(the SDK leaves that out of the schema itself), so every tool that declares one recorded it. The comments that said the record follows the schema options' rule now say it follows the served schema.RecordedTypeProblems) counted an enum as a whole number, becauseType.GetTypeCodeanswers an enum's underlying integer type. It now leaves enums out, asMcpArgumentValueCheck.Boundsdoes. No tool the census sees declares an enum parameter, so no result changes today. seenTest plan
New table
McpArgumentTypeRows(one file, linked into both test projects), run byMcpArgumentTypeTestsin Darling.Tests and Lite.Tests: 58 rows, one per (CLR type, value), through a real in-process MCP server and client. Each row pins both halves: with the guard left out (installGuard: false) the binder throws aJsonExceptionon exactly the rows marked refused, and with the guard in those rows are refused by a message that names the argument, says the phrase the row lists and quotes the value, while every other row is not refused by the guard. The binder half passed for every row on both products before the change, so the table states what the binder does (seen).Tool and parameter for each row come from reflection over the product's own tool types (the first tool that declares the CLR type and needs no other argument, taking the tool types by full type name, as both products' test lists sort them, and each type's methods by method name). Where a product declares no tool with the type, a test-only tool
probe_argument_types, registered through the sameMcpSchemaCompatpath, supplies it:short,byte,ulong,int[]andbool?for both products, pluslongandlong?for Lite (Lite declares nolong?, and its onelong,plan_idonanalyze_query_store_plan, comes with a second required argument, so the runner passes that tool over). It carries noMcpServerToolTypeattribute, so no census over the product's tool types sees it. seenThe pick passes over a tool whose name starts with
delete_,create_orupdate_. That check covers those three prefixes only, so a write tool named otherwise (add_servers,remove_server,set_mute_rule_enabled,mute_analysis_finding) is not passed over. What keeps a row from reaching a store is the test host's stand-in services, not the name check: Darling's data source points at a closed local port (InertInstanceForin Darling.Tests'McpWholeNumberArgumentTests), and Lite's services are uninitialized instances with every field null. seenThree more checks in each product: every parameter a shipped tool advertises has a recorded type that agrees with the schema's word for it, and an integer parameter refuses null exactly when its type is not nullable; every CLR type a shipped tool declares has a row in the table (this found
doubleandstring[], which now have rows); and (the second pass) every parameter the host recorded is one the tool's schema advertises.RED, before the guard change (seen; each failure is the guard half, none is the binder half):
The 59 is the 58 table rows plus one fact, per product, counted from the commit that was run (the first commit, which holds tests only): 58
rows.Addlines inMcpArgumentTypeRowsand, beside the one[Theory], one[Fact](EveryClrTypeAShippedToolDeclares_IsOneTheTableCovers). The class had two facts after the guard change.EveryAdvertisedParameter_HasARecordedType_ThatAgreesWithTheSchemacame with the guard change, because it readsMcpToolParameterTypes, which the RED commit did not have. So the class then totaled 60 per product (58 rows, 2 facts), and the 33 failures above are all table rows. The second pass added a third fact,EveryRecordedParameter_IsOneTheSchemaAdvertises, so it totals 61 per product now (58 rows, 3 facts).Failing rows, identical on both products (31 "the guard passed a value the binder cannot read", 2 "the guard refused a value the binder reads"):
Two verbatim failure lines:
The four items added on top (#4956 items a and b, the
CanBindexceptions, the binder-options pin). Each was watched RED before it was GREEN (seen), with a temporary plant that is not in the diff:settleFast(). With the old test the run passed: the harness output held the 720-hour read infetchesand an emptybeyond. With the new check:get_cpu_utilizationrefuses 168 hours and keeps 96. With the old scenarionoticesstayed empty (nothing was mounted). With the mount:With no plant the run draws 5 ranges (1, 4, 12, 24 and 168 hours) through both registries: 615 reads, none over 168 hours, and 0 notices, 0 errors and 0 panels left loading.
offeredRangeschecks (items a and b) were seen failing on those local plants, with Node present. Without Node the test ends early as skipped (WebRangeKeptHistoryBehaviourTests.TryRuncallsAssert.Skipwhen it cannot startnode), so a machine without Node runs none of these checks.CanBind. New testMcpUnknownArgumentGuardTests.AValueWhoseConverterThrowsAnythingButAJsonError_PassesToTheBinderregisters a tool method by hand (throughMcpToolParameterTypes.Register) whose parameter type carries a[JsonConverter]that throws anArgumentException, and callsMcpUnknownArgumentGuard.Refuse. Before the change the exception came out ofRefuse:After it,
Refusereturns null. The test first sends a bad value for the tool's other parameter (a whole number) and expects a refusal that names it, so the null for the throwing value is a verdict and not a guard that never reached the value.SerializerOptions = new JsonSerializerOptions(McpJsonUtilities.DefaultOptions) { NumberHandling = JsonNumberHandling.Strict }on theMcpServerToolCreateOptionsinMcpSchemaCompat(Darling.Tests only). Four rows of the table failed on the binder half, which is the half that catches it:The other three rows were
short "7",int[] ["1","2"]anddouble? "2.5". With the plant removed the class passes, andgit diffonMcpSchemaCompat.csis empty.After the first commit (seen):
McpWholeNumberArgumentTests,McpUnknownArgumentGuardTestsandMcpPayloadContractCensusTests: Darling 144 passed, Lite 76 passed, 0 failed.deprecated/Dashboard/Dashboard.csprojbuilds: 0 warnings, 0 errors. Darling.Tests and Lite.Tests build with 0 warnings.TsqlConventionGuardTests.TheMemberScan_ReadsEveryDeclarationWholereported the new file'sRangeOf(an expression body with anis not { } xpattern) as a member whose range stops short. It is now a block body; the class passes (14 of 14) and the second full run has 0 failures.After the second commit (seen):
WebServerPageRangeTests,McpArgumentTypeTests,McpUnknownArgumentGuardTests,McpWholeNumberArgumentTestsandWebRangeKeptHistoryBehaviourTests(which shares the harness): 89 passed, 0 failed.Mcp*498 tests (27 skipped),Web*272 (1 skipped), the census, pin, hygiene, convention, contract, inventory and guard classes 1101 (34 skipped), and the style, source, encoding, line-ending, header, namespace, architecture, layer, catch, exception, swallow, timeout and comment classes 852 (27 skipped). The skips are live-store classes with no connection set.McpArgumentTypeTestsandMcpWholeNumberArgumentTests: 68 passed, 0 failed.deprecated/Dashboard/Dashboard.csprojbuilds with 0 warnings and 0 errors.After the third commit, the first pass (seen):
McpUnknownArgumentGuardTests.AnEnumParameter_IsRefusedAsOneOfItsNames_NotAsAWholeNumber. It registers a tool method by hand, the way theCanBindtest does, with an enum parameter, a list of that enum and anint. It sends 0.5, a name no member has, and 3000000000 for the enum, and[0.5]for the list; each must be refused with "one of Crimson, Teal, Amber" (or, as that round worded the list, "a list (a JSON array) of ProbeColor values"; the second pass, item 2, changes it to "names from Crimson, Teal, Amber") and without "whole number" or "too large". A member name and 1 must not be refused. RED, with the fix not yet made (the class then held 15 new tests, and only this one failed):The whole refusal text on that round's commit, from a one-off run (not in the diff):
int, as a JSON number and as text, positive and negative (4 rows); a plus sign and 30 leading zeros before an out-of-range number, both signs (7 rows); and a plus sign or 30 leading zeros before a 5, which is not refused (3 rows). All 14 were green with the old parse (in the RED run above, where only the enum test failed). Two more rows, a signed run of 30 zeros sent for aulong, fail if the leading zeros are not trimmed before the length check: both failed when I took the trim out for one run (that change is not in the diff).McpUnknownArgumentGuard.Refusefor anintparameter sent a 1,000,000-digit value, Debug build, three runs each. The verdict was "too large" (or "too small") on every run, before and after:BigInteger.TryParse)01c9b6dfc). Darling.TestsMcpUnknownArgumentGuardTests,McpArgumentTypeTests,McpWholeNumberArgumentTestsandTsqlConventionGuardTests(the convention guard whose member scan reads the changed file): Total 108, Failed 0. Lite.TestsMcpArgumentTypeTestsandMcpWholeNumberArgumentTests: Total 68, Failed 0. Darling.Tests and Lite.Tests build with 0 warnings and 0 errors.deprecated/Dashboard/Dashboard.csproj: builds with 0 warnings and 0 errors.PerformanceMonitor.Commonclass, which Lite runs too, and Lite's two classes above pass with it. (The second pass's new tests are in both products.)01c9b6dfc, every suite in full): every test job passed. One finding stayed open on it: the refusal stated the CLR type's range (such as from -2147483648 to 2147483647), which differs from the range the server's own validator states forhours_back,limitandtop. The second pass, item 1, changes that wording.After the second pass (commit
d392f2ec4), RED first: each new or changed test was watched failing before the product change (seen).Darling.Tests ran
McpUnknownArgumentGuardTests,McpArgumentTypeTests,McpWholeNumberArgumentTestsandTsqlConventionGuardTests; Lite.Tests ranMcpArgumentTypeTestsandMcpWholeNumberArgumentTests. The 14 Darling failures are the newHoursBackPastInt_IsRefusedAsTooLarge_AndStatesNoRange, the list assertion inAnEnumParameter_IsRefusedAsOneOfItsNames_NotAsAWholeNumber, the newEveryRecordedParameter_IsOneTheSchemaAdvertises, and 11 theory rows that already existed (4 ofAHundredThousandDigitValueForAnInt_IsRefusedAsTooLargeOrTooSmalland 7 ofASignAndLeadingZeros_DoNotChangeWhichWayAWholeNumberIsOutOfRange) whose assertion now expects no range. The 2 Lite failures are the new range test and the new reverse check.hours_back: 99999999999sent toget_wait_statsthrough the in-process server and client, with the guard and the recorded types on the path. The test asserts "which is too large" and neither "2147483647" nor "-2147483648".The 11 reworded rows assert
takes a whole number, and the call sentand the absence of both bounds and of " from ". The refusal forhours_backnow reads, inferred from the format string and pinned by the new test's assertions:Argument 'hours_back' for tool 'get_wait_stats' takes a whole number of hours, and the call sent 99999999999, which is too large. Accepted parameters: .... That is the shape the schema-only fallback already had, so the two paths now word it the same way.a list (a JSON array) of names from Crimson, Teal, Amberand the absence ofProbeColor.McpUnknownArgumentGuardTestsstart the host.AKeyDifferingOnlyByCase_IsRefusedBeforeTheBinder_ThroughARealServeris refused before the binder.WithoutTheGuard_TheBinderDropsAKeyDifferingOnlyByCaseruns with the guard left out: itsHOURS_BACKcall is dropped by the binder, so the tool body runs at the default window, and itshours_back: "not-a-number"call fails binding before the body.McpArgumentTypeTestssends every row to a guard-less host and to a guarded one, and a row the binder reads runs a tool body on both. Lite.Tests'McpWholeNumberArgumentTestsdoes not carry the sentence, so nothing changes there.EveryRecordedParameter_IsOneTheSchemaAdvertisesis the reverse ofEveryAdvertisedParameter_HasARecordedType_ThatAgreesWithTheSchema, and fails oncancellationToken. The test-onlyprobe_argument_typestool now declares aCancellationTokentoo (the SDK leaves it out of the schema), so the check always has a parameter the schema leaves out to look at, whatever the shipped tools declare.After the product change (seen):
PerformanceMonitor.Common);deprecated/Dashboard/Dashboard.csprojbuilds with 0 warnings and 0 errors.TsqlConventionGuardTests(its member scan readsMcpToolParameterTypes.cs, from whichRangeOfis deleted, andMcpSchemaCompat.cs) passes with the rest of the Darling run.d392f2ec4, every suite in full): every test job passed:build,Darling Linux build,Darling whole-tree guards,Darling PG testsshards 0, 1 and 2, andLite testsshards 0 to 3.Nothing is parked: #4956 items a and b, which the first commit listed as not done, are done above, and the issue closes with this PR.
Commit
09d9ae5ceanswers three first-pass items left open, and four small ones (seen).stringis decided from its token. A String or Null token passes and anything else is refused, with no deserialize. A new pin sends a 5,000,000-character string: the old shape allocated 10,000,024 bytes, and this one under 100,000.tools/callrow over the shipped stateless HTTP pipeline, inDarlingMcpHostGateLiveTests. It sendsget_alert_history {"hours_back":null}and asserts the typed wording. RED, with the record lookup planted to return nothing: "expected the argument guard's refusal, got: An error occurred invoking 'get_alert_history'." Lite has no HTTP harness that reachestools/call, so the row is Darling's only. The guard now logs one warning per process when the record does not resolve. Lite's host clears its logging providers, so that warning shows only in Darling, and no test reads it.AllowsNull: its doc and the guard's class doc say only the tests read it, as a cross-check.ulongrow: "the refusal does not say 'too small' -> ... with no decimal point, such as 1, and the call sent -0." An older test said a signed run of zeros is never out of range. It now says a plus sign is never "too" anything, and a minus sign is "too small" for an unsigned type.McpArgumentTypeRows.Resolvepasses overdelete_,create_andupdate_tools. Every row still finds a target. Darling'sint?row moved fromcreate_mute_ruletoget_lock_wait_trend(bucket_minutes), and itslongrow fromdelete_custom_alert_ruletoget_custom_alert_rule(rule_id). Lite serves no such tool.WebRangeKeptHistoryBehaviourTests.TryRunalready callsAssert.Skipwhen Node is missing, so nothing changed. With Node off the path, it andWebServerPageRangeTestsreport 10 skipped and 2 passed.After merging dev (
ca71a1aa2): Darling.Tests, 280 run and 0 failed (McpArgumentTypeTests,McpUnknownArgumentGuardTests,McpWholeNumberArgumentTests,DarlingMcpHostGateLiveTests,WebRangeKeptHistoryBehaviourTests,WebServerPageRangeTests,DocCommentHygieneTests,TsqlConventionGuardTests,McpPayloadContractCensusTests). Lite.Tests, 79 run and 0 failed (McpArgumentTypeTests,McpWholeNumberArgumentTests,McpUnknownArgumentGuardTests,DocCommentHygiene,TsqlConventionGuardTests). Darling.Tests, Lite.Tests anddeprecated/Dashboard/Dashboard.csprojbuild with 0 warnings. seenFor a reader to check:
McpHelpers.ValidateHoursBackforhours_back).McpJsonUtilities.DefaultOptionsbeing the options the SDK binds with when a tool is created without its ownSerializerOptions(McpSchemaCompatsets none). The table pins agreement on every row (seen: theJsonNumberHandling.Strictplant above, which gives the tools their own options, failed 4 rows ofMcpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt), but a tool created with its own options later would need the guard to learn them.stringwithout?) is not refused, because the binder reads it. Only a null for a value type is refused. No row pins this.stringandstring?are the sameType, so thestringnull row cannot aim at a non-nullable parameter. The guard never reads the annotation. inferredCHANGELOG
No entry in this PR; the release owner adds it under [Unreleased].
Proposed entry: MCP tool calls whose arguments the server cannot read, such as a whole number past the parameter's range, null for a number, a number sent for text or text sent for true or false, are refused with a message that names the argument instead of the generic invocation error.