Skip to content

MCP tools refuse a value the parameter's type cannot hold, by name, in Lite and the Darling service (#4956) - #4960

Merged
erikdarlingdata merged 12 commits into
devfrom
fix/mcp-argument-types
Oct 2, 2026
Merged

erikdarlingdata merged 12 commits into
devfrom
fix/mcp-argument-types

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 2, 2026 •

Copy link
Copy Markdown
Owner

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 CanBind lets 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

  • Lite and Darling MCP tools refuse a fraction for a whole-number parameter, such as hours_back: 0.5, with a message that names it #4945 made the guard refuse a fraction for a whole-number parameter, but it judged every integer parameter by the served schema, which says "integer" for an int, a long, a short and a byte alike, has no format, and does not say which parameters take null. So these still reached the SDK and got its generic error: an int past int range, null for a parameter that is not nullable, a short or byte past 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).
  • The same schema-only reading refused an in-range ulong such as 18446744073709551615 (it only accepted what fits a long), although the binder reads it. seen (RED lines below). No shipped tool declares a ulong (seen: the table's tool lookup fell through to the test-only tool), so no shipped tool was affected; inferred.
  • The second pass. A refusal for a whole number past an int said takes a whole number of hours from -2147483648 to 2147483647 for hours_back, while McpHelpers.ValidateHoursBack refuses 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: a CancellationToken was 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 said of ProbeColor values, a CLR type name, where the refusal for the enum itself names its members. seen (RED line, item 2).

What changes

  • New PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs. McpToolParameterTypes holds, per tool and per parameter, the declared CLR type and whether it takes null. McpSchemaCompat.WithGeminiCompatibleTools fills it as each tool is created, from the parameters the created tool's served schema lists (the second pass; it was the SchemaOptionsFor rule, which let a CancellationToken through), and registers the one instance in the same service collection (found-or-added, as GuideCatalogOf does for the guide catalog).
  • McpUnknownArgumentGuard.Refuse takes the record as an optional third argument; the call-tool filter reads it from request.Services. For each argument it reads the value into the declared type with McpJsonUtilities.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 when System.Text.Json throws a JsonException. 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.
  • The refusal keeps today's whole-number style: it names the argument and the tool, says what the parameter takes, quotes the value (cut at 40 units as before), and lists the accepted parameters. What it says per type: a whole number ("too large" or "too small" for a whole number outside the type's range, with no range quoted: see the second pass, item 1), true or false, text, a number, a list (a JSON array) of those, and "cannot be null" for a null sent to a value type. Example, inferred from the format strings: Argument 'hours_back' for tool 'get_wait_stats' takes a whole number of hours, and the call sent 3000000000, which is too large.
  • A tool with no record, or a call without the services, keeps today's behaviour: the schema-only whole-number check, which passes null. The existing EveryIntegerParameter_... doc comment now says so.
  • Tests and comments: four small fixes left from #4945 and #4946 #4956 item c: the comment on the inert data source in McpWholeNumberArgumentTests says 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.
  • Tests and comments: four small fixes left from #4945 and #4946 #4956 item d: the McpHelpers.TextElementCutLength comment 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.
  • The guard's type doc, and the docs of the schema-only helpers, now describe the typed contract and the fallback.
  • Tests and comments: four small fixes left from #4945 and #4946 #4956 item a: WebServerPageRangeTests.EveryOfferedRange_IsOneEveryRangedReadOnEveryTabTakes now also checks the whole list of reads the run made (the harness's fetches, read after its final settle()) against McpHelpers.MaxHoursBack. Before, each tab's reads were checked right after that tab's own settleFast(), so a read made later (behind a timer, say) reached fetches but never beyond, 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.
  • Tests and comments: four small fixes left from #4945 and #4946 #4956 item b: the offeredRanges scenario returned no nodes, so nothing was mounted and its notices and errors were 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 the census scenario already fails on errors). The harness's strips puts that name in front of the strips beneath it; the other scenarios mark no node, so their output is unchanged.
  • Guard hardening: McpArgumentValueCheck.CanBind returned false on JsonException, true on NotSupportedException and InvalidOperationException, and let every other exception out of McpUnknownArgumentGuard.Refuse, and so out of the call-tool filter. A converter that throws an ArgumentException is a case the guard cannot judge. It now returns false only for JsonException and true for any other exception, so the guard never fails a call the binder would answer itself.
  • Enum parameters (first pass): McpArgumentValueCheck.Bounds used Type.GetTypeCode, which answers an enum's underlying integer type, so it matched every enum. Takes and Plural answered "a whole number" ("whole numbers" for a list) before reaching their enum arms, and Problem took the range branch, so a number past the underlying int was "too large". Bounds now returns null for an enum (a block body, so the member scan in TsqlConventionGuardTests reads 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".
  • The digit parse (first pass): OutOfRange handed the argument's whole text to BigInteger.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.
  • Binder options pin: already covered, so no new test. McpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt (in both Darling.Tests and Lite.Tests) calls every row of McpArgumentTypeRows through 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 if McpSchemaCompat ever creates a tool with its own SerializerOptions. seen: with the JsonNumberHandling.Strict plant listed under "Binder-options pin" below, 4 rows of McpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt failed (Darling.Tests only).
  • The second pass, item 1: the refusal states no range. McpArgumentValueCheck.Problem said what the CLR type holds ("takes a whole number of hours from -2147483648 to 2147483647"), which is not the range a tool takes: hours_back is an int, and McpHelpers.ValidateHoursBack refuses 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. RangeOf and its doc are deleted.
  • The second pass, item 2: Plural has 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.
  • The second pass, item 3: the comment on InertInstanceFor in Darling.Tests' McpWholeNumberArgumentTests named the wrong tests. Of the two tests in McpUnknownArgumentGuardTests that start the host, one reaches a tool body: WithoutTheGuard_TheBinderDropsAKeyDifferingOnlyByCase, through its HOURS_BACK call (the binder drops that key, so get_wait_stats runs at its default window). And McpArgumentTypeTests runs 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' McpWholeNumberArgumentTests has no such sentence, so nothing changes there.
  • The second pass, item 4: the record holds only what a caller may send. WithGeminiCompatibleTools now creates the tool first, reads the property names of its served InputSchema, and registers with parameter => parameter.Name is { } name && advertised.Contains(name). Before, the predicate was the schema options' IncludeParameter, which leaves out DI services but not a CancellationToken (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.
  • The census of advertised parameters (RecordedTypeProblems) counted an enum as a whole number, because Type.GetTypeCode answers an enum's underlying integer type. It now leaves enums out, as McpArgumentValueCheck.Bounds does. No tool the census sees declares an enum parameter, so no result changes today. seen

Test plan

New table McpArgumentTypeRows (one file, linked into both test projects), run by McpArgumentTypeTests in 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 a JsonException on 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 same McpSchemaCompat path, supplies it: short, byte, ulong, int[] and bool? for both products, plus long and long? for Lite (Lite declares no long?, and its one long, plan_id on analyze_query_store_plan, comes with a second required argument, so the runner passes that tool over). It carries no McpServerToolType attribute, so no census over the product's tool types sees it. seen

The pick passes over a tool whose name starts with delete_, create_ or update_. 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 (InertInstanceFor in Darling.Tests' McpWholeNumberArgumentTests), and Lite's services are uninitialized instances with every field null. seen

Three 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 double and string[], 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):

Darling.Tests  Total: 59, Errors: 0, Failed: 33, Skipped: 0, Not Run: 0
Lite.Tests     Total: 59, Errors: 0, Failed: 33, Skipped: 0, Not Run: 0

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.Add lines in McpArgumentTypeRows and, beside the one [Theory], one [Fact] (EveryClrTypeAShippedToolDeclares_IsOneTheTableCovers). The class had two facts after the guard change. EveryAdvertisedParameter_HasARecordedType_ThatAgreesWithTheSchema came with the guard change, because it reads McpToolParameterTypes, 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"):

int 3000000000; int -3000000000; int "3000000000"; int? 3000000000; int null; long null; bool null
short 32768; short -32769; byte 256; byte -1; ulong -1
ulong 18446744073709551615 (read by the binder, refused by the guard); ulong 9223372036854775808 (same)
int[] [1,2.5]; int[] [3000000000]; int[] [null]; int[] "1,2,3"; int[] 5
string 5; string true; string ["a"]; string {}; string[] ["a",5]; string[] "a"
bool "yes"; bool "true"; bool 1; bool? "yes"
double? "abc"; double? true; double "abc"; double null

Two verbatim failure lines:

get_ag_health.server_name (string) <- 5: the guard passed a value the binder cannot read, so the SDK answered -> An error occurred invoking 'get_ag_health'.
probe_argument_types.huge (ulong) <- 18446744073709551615: the guard refused a value the binder reads -> {"status":"invalid","message":"Argument 'huge' for tool 'probe_argument_types' takes a whole number, and the call sent 18446744073709551615, which is too large. ...

The four items added on top (#4956 items a and b, the CanBind exceptions, the binder-options pin). Each was watched RED before it was GREEN (seen), with a temporary plant that is not in the diff:

  • Item a. Plant: in the harness, a read for 720 hours deferred by a 50 ms timer on the last tab, so it lands after that tab's settleFast(). With the old test the run passed: the harness output held the 720-hour read in fetches and an empty beyond. With the new check:
WebServerPageRangeTests.EveryOfferedRange_IsOneEveryRangedReadOnEveryTabTakes [FAIL]
  1 reads the page made asked for more than 168 hours, the widest window the capped reads take (this list is every read the run made, so a read that cannot be tied to a tab is in it):
  /api/read/get_planted_slow?...&hours=720
  • Item b. Plant: in the harness's scripted answers, get_cpu_utilization refuses 168 hours and keeps 96. With the old scenario notices stayed empty (nothing was mounted). With the mount:
WebServerPageRangeTests.EveryOfferedRange_IsOneEveryRangedReadOnEveryTabTakes [FAIL]
  2 notices were shown at an offered range, where no read is refused:
  last 7 days: SQL Server overview tab: This view reads at most 96 hours (4 days) at a time, so it shows the last 4 days.
  last 7 days: SQL Server cpu tab: This view reads at most 96 hours (4 days) at a time, so it shows the last 4 days.

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.

  • Both offeredRanges checks (items a and b) were seen failing on those local plants, with Node present. Without Node the test ends early as skipped (WebRangeKeptHistoryBehaviourTests.TryRun calls Assert.Skip when it cannot start node), so a machine without Node runs none of these checks.
  • CanBind. New test McpUnknownArgumentGuardTests.AValueWhoseConverterThrowsAnythingButAJsonError_PassesToTheBinder registers a tool method by hand (through McpToolParameterTypes.Register) whose parameter type carries a [JsonConverter] that throws an ArgumentException, and calls McpUnknownArgumentGuard.Refuse. Before the change the exception came out of Refuse:
System.ArgumentException : This converter reads nothing.
   at ConverterThatThrowsOnRead.Read(...)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(...)

After it, Refuse returns 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.

  • Binder-options pin. Plant: SerializerOptions = new JsonSerializerOptions(McpJsonUtilities.DefaultOptions) { NumberHandling = JsonNumberHandling.Strict } on the McpServerToolCreateOptions in McpSchemaCompat (Darling.Tests only). Four rows of the table failed on the binder half, which is the half that catches it:
McpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt(type: "int", raw: "\"5\"", refused: False, ...) [FAIL]
  get_ag_health.limit (int) <- "5": the row says the binder reads this, but it threw: ...JsonException <- FormatException ...

The other three rows were short "7", int[] ["1","2"] and double? "2.5". With the plant removed the class passes, and git diff on McpSchemaCompat.cs is empty.

After the first commit (seen):

  • The new classes plus McpWholeNumberArgumentTests, McpUnknownArgumentGuardTests and McpPayloadContractCensusTests: Darling 144 passed, Lite 76 passed, 0 failed.
  • Full Darling suite: Total 19952, Failed 0, Skipped 1320, Not Run 1. The skips are the live PostgreSQL classes, which skip with no connection configured; they were not run.
  • Full Lite suite: Total 7235, Failed 0.
  • deprecated/Dashboard/Dashboard.csproj builds: 0 warnings, 0 errors. Darling.Tests and Lite.Tests build with 0 warnings.
  • The first full Darling run had one failure, mine: TsqlConventionGuardTests.TheMemberScan_ReadsEveryDeclarationWhole reported the new file's RangeOf (an expression body with an is not { } x pattern) 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.
  • Not run: the live PostgreSQL classes (the connection settings were unset on purpose).

After the second commit (seen):

  • Darling.Tests, WebServerPageRangeTests, McpArgumentTypeTests, McpUnknownArgumentGuardTests, McpWholeNumberArgumentTests and WebRangeKeptHistoryBehaviourTests (which shares the harness): 89 passed, 0 failed.
  • Darling.Tests, wider runs by class-name pattern, all 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.
  • Lite.Tests, McpArgumentTypeTests and McpWholeNumberArgumentTests: 68 passed, 0 failed.
  • Darling.Tests and Lite.Tests build with 0 warnings; deprecated/Dashboard/Dashboard.csproj builds with 0 warnings and 0 errors.
  • Not run locally on that commit: the whole Darling.Tests suite in one run. The wider class-pattern runs above stood in for it, and CI later ran every suite in full. The first commit's full runs (Darling 19952, Lite 7235, 0 failed) did not include the changes in that commit.

After the third commit, the first pass (seen):

  • Enum parameters. New test McpUnknownArgumentGuardTests.AnEnumParameter_IsRefusedAsOneOfItsNames_NotAsAWholeNumber. It registers a tool method by hand, the way the CanBind test does, with an enum parameter, a list of that enum and an int. 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):
Darling.Tests  Total: 24, Errors: 0, Failed: 1, Skipped: 0, Not Run: 0

McpUnknownArgumentGuardTests.AnEnumParameter_IsRefusedAsOneOfItsNames_NotAsAWholeNumber [FAIL]
  Assert.Contains() Failure: Sub-string not found
  String:    "Argument 'color' for tool 'pick' takes a whole num"...
  Not found: "takes one of Crimson, Teal, Amber, and the call se"...

The whole refusal text on that round's commit, from a one-off run (not in the diff):

before  color  <- 0.5:   Argument 'color' for tool 'pick' takes a whole number with no decimal point, such as 1, and the call sent 0.5. Accepted parameters: color, colors, count. ...
before  colors <- [0.5]: Argument 'colors' for tool 'pick' takes a list (a JSON array) of whole numbers, and the call sent [0.5]. Accepted parameters: color, colors, count. ...
after   color  <- 0.5:   Argument 'color' for tool 'pick' takes one of Crimson, Teal, Amber, and the call sent 0.5. Accepted parameters: color, colors, count. ...
after   colors <- [0.5]: Argument 'colors' for tool 'pick' takes a list (a JSON array) of ProbeColor values, and the call sent [0.5]. Accepted parameters: color, colors, count. ...
  • The digit parse. The new tests pass before and after, as they should, because no verdict changed: a 100,000-digit value for an 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 a ulong, 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).
  • The digit parse, timed instead of a RED (a one-off, not a test; no clock assertion is in the diff). A stopwatch around McpUnknownArgumentGuard.Refuse for an int parameter 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:
value sent before (BigInteger.TryParse) after
a JSON number 936, 632 and 590 ms 2.9, 3.8 and 6.1 ms
a JSON string 575, 576 and 547 ms 1.2, 1.2 and 1.2 ms
a JSON number with a leading minus 539, 566 and 544 ms 2.7, 2.5 and 2.7 ms
  • Targeted runs on the round's commit (01c9b6dfc). Darling.Tests McpUnknownArgumentGuardTests, McpArgumentTypeTests, McpWholeNumberArgumentTests and TsqlConventionGuardTests (the convention guard whose member scan reads the changed file): Total 108, Failed 0. Lite.Tests McpArgumentTypeTests and McpWholeNumberArgumentTests: 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.
  • The new tests sit in Darling.Tests only. The code under test is the shared PerformanceMonitor.Common class, which Lite runs too, and Lite's two classes above pass with it. (The second pass's new tests are in both products.)
  • CI on the previous round's head (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 for hours_back, limit and top. 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  Total: 110, Errors: 0, Failed: 14, Skipped: 0, Not Run: 0
Lite.Tests     Total: 70, Errors: 0, Failed: 2, Skipped: 0, Not Run: 0

Darling.Tests ran McpUnknownArgumentGuardTests, McpArgumentTypeTests, McpWholeNumberArgumentTests and TsqlConventionGuardTests; Lite.Tests ran McpArgumentTypeTests and McpWholeNumberArgumentTests. The 14 Darling failures are the new HoursBackPastInt_IsRefusedAsTooLarge_AndStatesNoRange, the list assertion in AnEnumParameter_IsRefusedAsOneOfItsNames_NotAsAWholeNumber, the new EveryRecordedParameter_IsOneTheSchemaAdvertises, and 11 theory rows that already existed (4 of AHundredThousandDigitValueForAnInt_IsRefusedAsTooLargeOrTooSmall and 7 of ASignAndLeadingZeros_DoNotChangeWhichWayAWholeNumberIsOutOfRange) whose assertion now expects no range. The 2 Lite failures are the new range test and the new reverse check.

  • Item 1, both products: hours_back: 99999999999 sent to get_wait_stats through 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".
McpWholeNumberArgumentTests.HoursBackPastInt_IsRefusedAsTooLarge_AndStatesNoRange [FAIL]
  Assert.DoesNotContain() Failure: Sub-string found
  String: ···"ours from -2147483648 to 2147483647, and the call "···
  Found:  "2147483647"

The 11 reworded rows assert takes a whole number, and the call sent and the absence of both bounds and of " from ". The refusal for hours_back now 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.

  • Item 2, Darling.Tests: the list assertion now expects a list (a JSON array) of names from Crimson, Teal, Amber and the absence of ProbeColor.
McpUnknownArgumentGuardTests.AnEnumParameter_IsRefusedAsOneOfItsNames_NotAsAWholeNumber [FAIL]
  Assert.Contains() Failure: Sub-string not found
  String:    "Argument 'colors' for tool 'pick' takes a list (a "···
  Not found: "a list (a JSON array) of names from Crimson, Teal,"···
  • Item 3: a comment only, so no RED. Checked by reading: two tests in McpUnknownArgumentGuardTests start the host. AKeyDifferingOnlyByCase_IsRefusedBeforeTheBinder_ThroughARealServer is refused before the binder. WithoutTheGuard_TheBinderDropsAKeyDifferingOnlyByCase runs with the guard left out: its HOURS_BACK call is dropped by the binder, so the tool body runs at the default window, and its hours_back: "not-a-number" call fails binding before the body. McpArgumentTypeTests sends 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' McpWholeNumberArgumentTests does not carry the sentence, so nothing changes there.
  • Item 4, both products: the new check EveryRecordedParameter_IsOneTheSchemaAdvertises is the reverse of EveryAdvertisedParameter_HasARecordedType_ThatAgreesWithTheSchema, and fails on cancellationToken. The test-only probe_argument_types tool now declares a CancellationToken too (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.
Darling.Tests  McpArgumentTypeTests.EveryRecordedParameter_IsOneTheSchemaAdvertises [FAIL]
  134 problems:
  get_ag_health.cancellationToken: recorded, but the schema does not advertise it
  get_alert_history.cancellationToken: recorded, but the schema does not advertise it
  ...
Lite.Tests     McpArgumentTypeTests.EveryRecordedParameter_IsOneTheSchemaAdvertises [FAIL]
  8 problems:
  get_analysis_facts.cancellationToken: recorded, but the schema does not advertise it
  ...
  probe_argument_types.cancellationToken: recorded, but the schema does not advertise it

After the product change (seen):

  • Darling.Tests, the four classes above: Total 110, Failed 0. Lite.Tests, the two classes above: Total 70, Failed 0.
  • Darling.Tests and Lite.Tests build with 0 warnings and 0 errors (they build the Darling service, Lite and PerformanceMonitor.Common); deprecated/Dashboard/Dashboard.csproj builds with 0 warnings and 0 errors.
  • TsqlConventionGuardTests (its member scan reads McpToolParameterTypes.cs, from which RangeOf is deleted, and McpSchemaCompat.cs) passes with the rest of the Darling run.
  • CI on this head (d392f2ec4, every suite in full): every test job passed: build, Darling Linux build, Darling whole-tree guards, Darling PG tests shards 0, 1 and 2, and Lite tests shards 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 09d9ae5ce answers three first-pass items left open, and four small ones (seen).

  1. A string is 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.
  2. A tools/call row over the shipped stateless HTTP pipeline, in DarlingMcpHostGateLiveTests. It sends get_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 reaches tools/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.
  3. AllowsNull: its doc and the guard's class doc say only the tests read it, as a cross-check.
  4. Two messages. (a) A null refusal reads "takes a whole number of hours, and cannot be null, and the call sent null." The HTTP row pins it. (b) "-0" for an unsigned type is "too small". RED, a new ulong row: "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.
  5. The guard's class doc now names a cost. It reads every argument once before the binder reads it again, so a caller-sized list is read twice.
  6. McpArgumentTypeRows.Resolve passes over delete_, create_ and update_ tools. Every row still finds a target. Darling's int? row moved from create_mute_rule to get_lock_wait_trend (bucket_minutes), and its long row from delete_custom_alert_rule to get_custom_alert_rule (rule_id). Lite serves no such tool.
  7. WebRangeKeptHistoryBehaviourTests.TryRun already calls Assert.Skip when Node is missing, so nothing changed. With Node off the path, it and WebServerPageRangeTests report 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 and deprecated/Dashboard/Dashboard.csproj build with 0 warnings. seen

For a reader to check:

  • The second pass changes the wording of a refusal for a whole number past the type: it carries no range, on the typed path or the schema-only path, and says only "too large" or "too small". The range a tool takes is stated by the tool's own validator (McpHelpers.ValidateHoursBack for hours_back).
  • The guard's verdict rests on McpJsonUtilities.DefaultOptions being the options the SDK binds with when a tool is created without its own SerializerOptions (McpSchemaCompat sets none). The table pins agreement on every row (seen: the JsonNumberHandling.Strict plant above, which gives the tools their own options, failed 4 rows of McpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt), but a tool created with its own options later would need the guard to learn them.
  • A refusal for a list of an enum names its members ("names from Crimson, Teal, Amber"), as the refusal for the enum itself does (the second pass, item 2).
  • A null for a reference type annotated as non-nullable (string without ?) is not refused, because the binder reads it. Only a null for a value type is refused. No row pins this. string and string? are the same Type, so the string null row cannot aim at a non-nullable parameter. The guard never reads the annotation. inferred

CHANGELOG

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.

…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).
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs Outdated
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs Outdated
Comment thread PerformanceMonitor.Common/Mcp/McpUnknownArgumentGuard.cs Outdated
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs
Comment thread Darling/Darling.Tests/McpArgumentTypeRows.cs Outdated
@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

Blind read, before the body

What 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 McpToolParameterTypes (found-or-added exactly as GuideCatalogOf does, McpSchemaCompat.cs:216-231), and the pre-dispatch guard decides each argument by trial-deserializing the value into that type with McpJsonUtilities.DefaultOptions and refusing on JsonException — a check with no per-type table of its own. Everything else follows from that: the wider refusal vocabulary, the schema-only path demoted to a fallback for a tool with no record, and 58 shared rows run twice per SKU (guard in, guard out).

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 Type.GetTypeCode reading that makes the enum arm unreachable, the "anything else passes" contract the catch list does not keep, and the fact that the switch deciding whether any of this runs is pinned only on a transport neither product ships.

…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.
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs Outdated
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs Outdated
Comment thread Darling/Darling.Tests/WebServerPageRangeTests.cs
Comment thread PerformanceMonitor.Common/Mcp/McpSchemaCompat.cs Outdated
@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

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. McpArgumentTypeRows.CheckAsync checking both halves — the unguarded host to establish what the binder actually does with a row, then the guarded host to establish that the guard agrees — is what makes CanBind's "the decision is the binder's own" claim auditable instead of aspirational. It is the direct answer to REVIEW-TRAPS' "A pin that encodes the lie… Ask what a pin would fail ON": a row fails on a real disagreement between the guard and the SDK, in either direction, and bound != refused (McpArgumentTypeRows.cs:298) is symmetric so neither direction can rot quietly. The two typed rows the body lists as RED in the "guard refused a value the binder reads" direction (ulong 18446744073709551615, ulong 9223372036854775808) are exactly the failures that shape can catch and a one-sided table cannot.

Counts I checked by hand. Table() has 58 rows (6 int + 2 int? + 2 long + 1 long? + 3 non-nullable-null + 4 short + 3 byte + 4 ulong + 9 int[] + 6 string + 5 bool + 2 bool? + 4 double? + 3 double + 4 string[]), matching the body. One thing does not reconcile: the RED block reports Total: 59 per product, and the file declares 58 theory cases plus two [Fact]s (EveryAdvertisedParameter_HasARecordedType_ThatAgreesWithTheSchema, EveryClrTypeAShippedToolDeclares_IsOneTheTableCovers) = 60. Probably the RED tree had one fewer case before double/string[] were added, but the body attributes that discovery to the census fact, which implies it already existed. Worth a line either way.

Type coverage, independently. Grepping both SKUs' [Description(...)]-annotated tool parameters gives string? (433), int (254), bool (40), string (38), int? (25), long (8), long? (2), double/double? (2 each), bool? (1), plus string[]? on get_tool_guide's tools/topics in both. So: the body's "no shipped tool declares a ulong" holds, no shipped tool declares short, byte or int[] (the probe tool earns its place), no enum/DateTime/Guid/char/decimal parameter exists anywhere, and string[] is covered by a shipped tool on both SKUs rather than by the probe. Minor: the probe's doc (McpArgumentTypeRows.cs:33) says long is a type "Darling's tools declare and Lite's do not" — Lite declares one, long plan_id at Lite/Mcp/McpPlanTools.cs:129. It is required alongside other arguments so Resolve skips it and the probe supplies the row anyway; the sentence is just off.

Parity. Not a finding. The change is entirely in PerformanceMonitor.Common/Mcp, both hosts register the same McpUnknownArgumentGuard.Instance (Lite/Mcp/McpHostService.cs:165, Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs:1053), every tool type on both SKUs goes through WithGeminiCompatibleTools so no tool silently falls to the schema-only path, and the table plus its probe tool run on both (Lite.Tests/Lite.Tests.csproj link + Lite.Tests/McpArgumentTypeTests.cs:42 resolving against LiteToolTypes()). WebServerPageRangeTests has no Lite twin to owe, because Lite ships no web viewer — RANGE_OPTIONS exists only under Darling/PerformanceMonitor.Darling.Service/wwwroot/js. The new AValueWhoseConverterThrowsAnythingButAJsonError_PassesToTheBinder landing only in Darling.Tests is likewise fine: it exercises shared Common code through a hand-built tool, and the two guard-test files are already not strict twins (Darling has ProtocolMetadata_TravelsOutsideTheArgumentsObject, Lite has BothHosts_RegisterTheSameSharedGuard_…).

No behaviour the old guard allowed is newly refused by accident. Every existing caller of Refuse that I could find passes two arguments and therefore keeps the schema-only path verbatim (McpWholeNumberArgumentTests.cs:184, :197, :225, :247 and the Lite twins), the two host-level tests that call through a guarded in-process server send an unknown key rather than a bad value (McpUnknownArgumentGuardTests.cs:330), and the one that pins the binder's own behaviour runs on installGuard: false (:349). The new optional third parameter is source- and binary-compatible for the public Refuse.

The McpHelpers doc correction is right, not just softer. TextElementCutLength walks start down from limit while IsAsciiBoundary is false (McpHelpers.cs:107-110) and, on text that is non-ASCII at the cut, can reach 0 and then segment forward across the whole window — so the old unconditional "a 512,000-unit limit costs no more than a 40-unit one" was wrong for exactly the inputs the #3625 cut exists for. The new wording states the ASCII precondition and names the fallback cost.

Findings are inline; verdict follows.

claude[bot]
claude Bot previously requested changes Oct 2, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SchemaOptionsFor rule, one instance found-or-added as GuideCatalogOf does — McpToolParameterTypes.cs:24,:49-73; McpSchemaCompat.cs:125,:166-172,:216-231.
  • VERIFIED — Refuse takes it as an optional third argument; the filter reads it from request.Services — McpUnknownArgumentGuard.cs:112-113,:100.
  • VERIFIED — refuses exactly on JsonException, anything else passes — McpToolParameterTypes.cs:138-153, pinned by AValueWhoseConverterThrowsAnythingButAJsonError_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 worked hours_back sentence 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 — McpSchemaCompat sets no SerializerOptions — 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 only string?/string/int/int?/long/long?/bool/bool?/double/double?/string[]?.
  • VERIFIED — RangeOf is 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: Problem never reads AllowsNull and CanBind(null, typeof(string)) succeeds — McpToolParameterTypes.cs:110, row at McpArgumentTypeRows.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.DefaultOptions are the options the SDK binds with (the body marks it inferred): needs the SDK path or a run.
  • UNVERIFIED — "deprecated/Dashboard builds, 0 warnings": needs a build.
  • UNVERIFIED — that the four new offeredRanges assertions hold: needs Node; TryRun returns 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-72 plus web-kept-history-harness.mjs:274-277,:300 and the where-threading in strips at :313-323.

Riskiest lines

  1. PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:204 — Takes tests Bounds(type) before type.IsEnum, and Type.GetTypeCode of an enum returns its underlying code, so Bounds matches every enum: the IsEnum arm at :229 can never run, Plural:263 shadows 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_IsOneTheTableCovers fails on the first enum parameter added, but on the type being uncovered, not the wording).
  2. PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:193 — BigInteger.TryParse over the argument's full raw text, caller-controlled and unbounded, superlinear; CanBind above bails cheaply and the predecessor (McpUnknownArgumentGuard.cs:294) only scanned digits. — UNPINNED (longest value in the table is 20 digits).
  3. 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 by McpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt on both SKUs and by EveryAdvertisedParameter_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-316 takes the binder's own verdict on an unguarded host first and bound != refused fails both ways. It does describe the offeredRanges half, 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. WebServerPageRangeTests owes no twin — RANGE_OPTIONS exists only under the Darling service's wwwroot/js and 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) and McpUnknownArgumentGuard.OutOfRangeWord (:279) are near-twins in shape but answer different questions and are not pinned equal.
  • "hours_back, limit, top, days_back are refused when out of range, never clamped" (#3699, #3641) — does not apply as a defect; honoured. Problem returns 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: RangeOf ships as a block body (:171-179); Bounds:156 and Plural:262 hold 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, and McpToolTypeRegistrationTests pins 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_IsOneEveryRangedReadOnEveryTabTakes where Node is installed, with the two plants the body describes: a deferred read on the last tab (must trip fetches at line 52) and one read refusing 168 hours (must trip notices at line 65). Confirm loading == 0 is genuinely reached now that every tab at every range is mounted.
  • Confirm the quoted suite totals were taken after the harness and WebServerPageRangeTests hunks landed, and reconcile "Total: 59" against 58 rows + 2 facts.
  • Confirm from the SDK source that a tool created without SerializerOptions binds with McpJsonUtilities.DefaultOptions, and that nothing before JsonSerializer.Deserialize (schema coercion, missing-versus-null for a parameter with a .NET default) lets the binder accept a value CanBind rejects.
  • Send a multi-megabyte run of digits to an integer parameter on a running host and time the refusal, to size the OutOfRange parse.
  • Verify on a live host that request.Services resolves McpToolParameterTypes in HTTP mode as well as in-process: a real call with hours_back: null must come back naming hours_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).
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs Outdated
Comment thread Darling/Darling.Tests/McpWholeNumberArgumentTests.cs Outdated
Comment thread Darling/Darling.Tests/McpUnknownArgumentGuardTests.cs
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs Outdated
@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

Blind-read notes (detail behind the verdict)

Trap that applies. .github/REVIEW-TRAPS.md: "hours_back, limit, top, days_back are refused when out of range, never clamped — a clamp is a silent lie about the window". Nothing is clamped here, but the new refusal states a range the tool does not accept. McpToolParameterTypes.cs:188 (RangeOf) splices the CLR type's bounds into the sentence after WholeNumberOf(parameter), so the body's own example is Argument 'hours_back' for tool 'get_wait_stats' takes a whole number of hours from -2147483648 to 2147483647 … while McpHelpers.ValidateHoursBack (McpHelpers.cs:158-162) refuses everything outside 1–168 with Must be a positive integer (1-168). One server, one parameter, two contradictory ranges — and the guard's whole purpose is one-turn self-correction by a model. Detail in the inline comment.

Traps checked and not applying.

Wiring I checked rather than assumed. No MCP tool-creation site sets SerializerOptions (McpSchemaCompat.cs:174-183 sets Services/SchemaCreateOptions/Description/Meta only; a repo-wide grep for SerializerOptions finds nothing else), so McpJsonUtilities.DefaultOptions really is the binder's. Register moved inside the tool factory (:172), so the record exists only once the McpServerTool singletons are resolved — fine today because ServerOptions.ToolCollection is filled from those singletons before a call can arrive, and FindTool reads that same collection, but it is the one place a future change could silently drop every tool to the schema-only path.

Shipped-type coverage. Resolve needs a shipped tool for each table type on each SKU; I confirmed the four that were not obvious: bool (full_text), double (min_severity), double? (min_duration_ms on get_collection_log / GetCollectionLog, every parameter defaulted so required is empty), string[] (tools/topics on get_tool_guide). All present on both SKUs, so no row falls through to InvalidOperationException.

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 string or string[] is copied twice on the hot path. Worth a sentence in the type doc if it is a deliberate price.

claude[bot]
claude Bot previously requested changes Oct 2, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 — McpToolParameterTypes is filled by WithGeminiCompatibleTools as each tool is created, one instance found-or-added the way GuideCatalogOf is — McpSchemaCompat.cs:125, :166-172, :216-231
  • VERIFIED — Refuse takes the record as an optional third argument; the filter reads it from request.Services — McpUnknownArgumentGuard.cs:100, :112-113
  • VERIFIED — Values are read into the declared type with McpJsonUtilities.DefaultOptions, refused only on JsonException, everything else passed — McpToolParameterTypes.cs:138-153, pinned by AValueWhoseConverterThrowsAnythingButAJsonError_PassesToTheBinder
  • VERIFIED — A tool with no record keeps the schema-only check, which passes null — McpUnknownArgumentGuard.cs:152-162, :171-184, :265-271
  • VERIFIED — Bounds returns null for an enum, in a block body the convention guard reads whole — McpToolParameterTypes.cs:158-163
  • VERIFIED — OutOfRange answers 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 no McpServerToolType attribute so no census sees it
  • VERIFIED — double and string[] really are declared by shipped tools on both SKUs — double min_severity, string[]? tools/topics on get_tool_guide
  • VERIFIED — Items a and b: the test checks the harness's whole fetches list, and offeredRanges now mounts each tab under a where-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-92 vs :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 BigInteger timing 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. :325 is refused on the unknown key before the body; only the HOURS_BACK call at :346 binds. 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": Plural has no enum arm, so a list reads a list (a JSON array) of ProbeColor values (McpToolParameterTypes.cs:450-455) — as the test's own assertion at :559 and the PR body both say
  • REFUTED — McpToolParameterTypes.cs:36, "only what a caller may send is here": IsService(typeof(CancellationToken)) is false, so cancellationToken is recorded for every Darling tool declaring one (DarlingMcpDataTools.cs:64) though the schema omits it. Harmless; the unknown-key pass fires first.

Riskiest lines

  1. 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 by McpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt (guarded and unguarded host per row)
  2. 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 by AHundredThousandDigitValueForAnInt_IsRefusedAsTooLargeOrTooSmall, truthfulness against the parameter's real bounds UNPINNED
  3. PerformanceMonitor.Common/Mcp/McpSchemaCompat.cs:172 — Register moved inside the tool factory, so the record exists only once the tool singletons resolve — pinned indirectly by EveryAdvertisedParameter_HasARecordedType_ThatAgreesWithTheSchema and the int/long/bool null 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 says hours_back takes -2147483648 to 2147483647 while McpHelpers.ValidateHoursBack (McpHelpers.cs:158-162) refuses everything outside 1-168
  • 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: shared PerformanceMonitor.Common only, 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.cs is a Compile Include ... Link entry, one file compiled twice
  • 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: McpToolParameterTypes.cs:108, :158, :181, :197 are block bodies
  • A 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_IsOneTheTableCovers reflects over the host's registered tool types
  • InternalsVisibleTo ... a helper Darling.Tests must call has to be public (#3688) — does not apply: McpToolParameterTypes.cs:12, :26 are public; McpArgumentValueCheck stays internal behind the public Refuse

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 McpArgumentTypeTests where the real SDK binder is exercised, and confirm the UNGUARDED half still throws JsonException on all 33 refused rows: if the SDK ever treats a null argument as "use the parameter default", the int null / long null / bool null rows become refusals of calls that used to work
  • Run WebServerPageRangeTests with Node installed — without it TryRun skips and both new checks are silently uncovered
  • Call a real Lite and Darling host with hours_back: 3000000000 and read the refusal a client receives: confirms the record resolves from request.Services outside 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.
Comment thread Darling/Darling.Tests/McpArgumentTypeRows.cs
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs
Comment thread PerformanceMonitor.Common/Mcp/McpUnknownArgumentGuard.cs Outdated
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM

Claims

  • VERIFIED — record of CLR type + nullability per tool parameter, filled as each tool is created, one instance found-or-added as GuideCatalogOf does — McpToolParameterTypes.cs:24,50-74; McpSchemaCompat.cs:126,185-187,243-258.
  • VERIFIED — Refuse takes the record as an optional 3rd argument; the filter reads it from request.Services — McpUnknownArgumentGuard.cs:100,112-113.
  • VERIFIED — the verdict is Deserialize(value, declaredType, McpJsonUtilities.DefaultOptions), refusing only on JsonException — McpToolParameterTypes.cs:142-157; no SerializerOptions is 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, RangeOf deleted — grep -rn RangeOf empty; McpToolParameterTypes.cs:134-136, McpUnknownArgumentGuard.cs:179-183.
  • VERIFIED — Bounds null 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 — OutOfRange answers >20 digits by length and sign — :203-227; TrimStart('0') at :215 precedes the length check (what makes ASignedRunOfZeros_... hold), and ulong.MaxValue is exactly 20 digits, so the boundary is right.
  • VERIFIED — round 2 item 4: the predicate is the created tool's advertised property names, so CancellationToken is 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 under Lite/Mcp (my first grep missed the required, default-less ones).
  • VERIFIED — double and string[] 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 no where, so their strips output is unchanged.
  • UNVERIFIED — "McpJsonUtilities.DefaultOptions is 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.Strict plant 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

  1. PerformanceMonitor.Common/Mcp/McpUnknownArgumentGuard.cs:100 — the only read of RequestContext.Services in the repo; a null or non-resolving scope reverts every argument to the schema-only path silently. Pinned by McpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt in both SKUs, but only over WithStreamServerTransport (McpInProcessHost.cs:114) — UNPINNED on WithHttpTransport(Stateless = true), which both products ship.
  2. 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 of McpArgumentTypeRows.CheckAsync (:329-351): guard-less host says what the binder does, guarded host what the guard does, same tool and value.
  3. PerformanceMonitor.Common/Mcp/McpSchemaCompat.cs:186 — Register as a side effect in the AddSingleton tool factory, so the record fills lazily while the catalog fills eagerly; safe only because FindTool reads a ToolCollection that exists once every factory has run. Pinned by EveryAdvertisedParameter_HasARecordedType_... and EveryRecordedParameter_IsOneTheSchemaAdvertises, which sweep host.RegisteredTools in 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: RangeOf is gone, Problem's is { } bounds is 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 [] so notices/errors could 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.
  • "InternalsVisibleTo on 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), McpArgumentValueCheck internal (:103).
  • "hours_back, limit, top, days_back are refused when out of range, never clamped (#3699)" — applies, honoured: nothing coerces, and round 2 dropped the CLR range because it contradicted ValidateHoursBack (: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), and EveryClrTypeAShippedToolDeclares_IsOneTheTableCovers keeps 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:

  1. "A null for a reference type annotated as non-nullable is not refused, because the binder reads it" is asserted, not pinned. Resolve matches on ParameterType, and string and string? are the same Type, so the string null row (McpArgumentTypeRows.cs:288) cannot be aimed at a non-nullable parameter — and AllowsNull, recorded for that distinction, is read only by the test (:179,183), never by the guard. See the inline note.
  2. 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 long rows land on delete_custom_view, so the theory dispatches a destructive tool on both hosts for every long the binder reads — harmless only because InertInstanceFor points 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.Tests and Lite.Tests on 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/call with a bad value over each product's shipped stateless-HTTP pipeline and confirm the typed refusal returns (hours_back: null -> "cannot be null"). DarlingMcpHostGateLiveTests.SendJsonRpcAsync is the seam.
  • Run node web-kept-history-harness.mjs <wwwroot/js> offeredRanges:168 and 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.
Comment thread Darling/Darling.Tests/McpArgumentTypeRows.cs
Comment thread Darling/Darling.Tests/McpArgumentTypeRows.cs Outdated
Comment thread Darling/Darling.Tests/McpArgumentTypeRows.cs Outdated
Comment thread PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs
@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

Blind-read notes, before the body

I 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 McpArgumentValueCheck.CanBind asks System.Text.Json instead of carrying a table, and because every row of McpArgumentTypeRows is run twice (once against a host with the guard left out, McpArgumentTypeRows.cs:381-390), the table's Refused column states what the binder does rather than what the guard hopes. That is the opposite of the >= limit-as-its-own-proof shape in .github/REVIEW-TRAPS.md, and it is what makes the "cannot refuse a value the binder reads" claim checkable at all.

Where the body and the tree disagree

1. "On Darling the long rows land on delete_custom_view, so its body runs on both hosts." The tree says they cannot. McpArgumentTypeRows.Resolve passes over any tool whose name starts with delete_ (:405-408) before it ever looks at a parameter, and three read tools declare a non-nullable long with no other required argument: get_query_store_plans-side query_id (DarlingMcpPlanTools.cs:135), get_custom_view's view_id (DarlingMcpCustomViewTools.cs:74), get_custom_alert_rule's rule_id (DarlingMcpCustomAlertTools.cs:86). So the long rows run a read, and delete_custom_view (DarlingMcpCustomViewTools.cs:227, the only delete_ tool with a long) is excluded twice over.

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 ChangesData existing, which is the mechanism that makes the sentence false.

2. "58 rows" / "it totals 61 per product now (58 rows, 3 facts)." McpArgumentTypeRows.cs holds 59 rows.Add lines (grep -c 'rows.Add(' = 59), so the current total is 62 per product, not 61. The RED-run reconciliation ("Total: 59 … the 58 table rows plus one fact") is plausibly accurate for the commit it was run on — the ulong "-0" row carries a #4956 comment and is absent from the 33-row failing list, so it looks like a later addition. The present-tense count is the stale one.

Coverage gap worth a human's eye: Lite's shipped HTTP path

DarlingMcpHostGateLiveTests.ToolsCall_ANullForAWholeNumber_IsRefusedByName_OverTheShippedHttpPipeline argues its own necessity well: "the guard takes its record of each tool's parameter types from the request's services … so only a call made like this one shows that a real client's request resolves it too." By that argument, Lite is unpinned. Lite ships the same shape — .WithHttpTransport(options => options.Stateless = true) at Lite/Mcp/McpHostService.cs:112, the same McpUnknownArgumentGuard.Instance filter at :165 — and gets only the in-process stream-transport pins (McpInProcessHost uses WithStreamServerTransport, McpInProcessHost.cs:114). If request.Services ever failed to resolve McpToolParameterTypes on Lite's pipeline, the guard would fall back to schema-only and pass a null for every non-nullable integer parameter, with nothing but a one-time log line saying so (McpUnknownArgumentGuard.cs:129-142).

I am not calling this a parity finding: the decision lives once in PerformanceMonitor.Common, both SKUs run the whole table and all three censuses (Lite.Tests/McpArgumentTypeTests.cs, Lite.Tests/Lite.Tests.csproj links the table), and Lite.Tests has no live-host harness to extend. It belongs on the reviewer checklist rather than in the diff.

Traps I checked and ruled out

  • "hours_back, limit, top, days_back are refused when out of range, never clamped — a clamp is a silent lie about the window." Honoured, and round 2 tightened it: the refusal no longer quotes the CLR range, so it cannot invite a retry at a bound McpHelpers.ValidateHoursBack (McpHelpers.cs:157-164) refuses. Nothing here coerces a value.
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted. Alias, do not copy." Aliased: one table file linked into Lite.Tests, one decision in McpToolParameterTypes.cs.
  • "An expression-bodied member holding a { } property pattern is read SHORT by the T-SQL convention guard's member walk." The new WebServerPageRangeTests.HoursAsked (:98-99) is exactly that shape, but TsqlConventionGuardTests.SourceFiles skips Darling/Darling.Tests/ (:2347), so the walk never opens it. In the shared libraries the diff adds no such member — Bounds is already a block body, as the body says.
  • "InternalsVisibleTo … names Lite.Tests only." Not here: PerformanceMonitor.Common.csproj:41,46 names both test projects, so the internal McpArgumentValueCheck and McpHelpers are reachable from each.

Verified from the tree, for the record

RANGE_OPTIONS is exactly the five the body claims — 1, 4, 12, 24 and 168 hours (wwwroot/js/pages/server.js:58-64), so "no notice at an offered range" is a claim the 168-hour cap can actually satisfy. The where prefix format in the body's plant output ("last 7 days: SQL Server cpu tab: …") matches harness:276 and harness:318 character for character. RangeOf is gone from PerformanceMonitor.Common. The harness emits fetches after its final await settle() (harness:309, 326), which is what makes the new whole-list check at WebServerPageRangeTests.cs:52 see a late read.

…d argument, so Lite's long rows land on the probe tool (#4956)
…ethod names, and that its write check matches three name prefixes only (#4956)
@erikdarlingdata
erikdarlingdata dismissed stale reviews from claude[bot] and claude[bot] October 2, 2026 21:20

Its findings are fixed at 8f65d09, and each thread has a reply.

Comment thread Darling/Darling.Tests/McpArgumentTypeRows.cs
Comment thread Darling/Darling.Tests/McpArgumentTypeRows.cs Outdated
Comment thread Darling/Darling.Tests/McpArgumentTypeRows.cs
@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

The table has 59 rows, not 58 — and the derived totals move with it

grep -c "rows.Add(" Darling/Darling.Tests/McpArgumentTypeRows.cs → 59.

The test plan says 58 in three places, and builds arithmetic on it:

  • "58 rows, one per (CLR type, value)"
  • "58 rows.Add lines in McpArgumentTypeRows"
  • "The second pass added a third fact … so it totals 61 per product now (58 rows, 3 facts)"

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 (McpArgumentTypeRows.cs:287-365): int 6, int? 2, long 2, long? 1, the three nulls 3, short 4, byte 3, ulong 5, int[] 9, string 6, bool 5, bool? 2, double? 4, double 3, string[] 4 = 59.

The likely cause is that the count predates commit 09d9ae5ce, whose item 4(b) says "RED, a new ulong row" — the -0 row at line 324. The 59 figure appears to be the RED commit's count carried forward, one row stale. Worth fixing because the body uses these numbers as the audit trail for "33 of 58 failed", and a reader reconciling the RED block against the tree lands one off.

What a human still has to check

Everything load-bearing in this change is pinned by a test, and nothing in it can be read off a static tree:

  1. The binder-agreement half. The entire refusal decision is JsonSerializer.Deserialize(value, type, McpJsonUtilities.DefaultOptions) plus ex is not JsonException (PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:157,166). Whether that matches what McpServerTool.Create actually binds with — and therefore whether the guard can refuse a call that used to work — is pinned only by the installGuard: false half of EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt. That half is the one real safety net here; the JsonNumberHandling.Strict plant the body reports is good evidence it bites. It needs a green CI run on this head, not a read.
  2. rows.Add("int", "null", true, …) (line 305) is the single row that decides whether this change widens refusals into previously-working territory. If AIFunctionFactory ever treats an explicit null for a defaulted int as "use the default" rather than throwing, that row flips and the guard starts refusing a call the server used to answer. The row asserts the opposite, so CI is the arbiter.
  3. Resolve finding a target for every one of the 14 type keys on both SKUs (double, double?, string[] in particular). A miss is an InvalidOperationException("No tool declares a parameter of type …"), not a readable failure, and the resolution depends on reflection order that differs between the two SKUs' type lists.
  4. Assert.Equal(0, r.GetProperty("loading").GetInt32()) in WebServerPageRangeTests — now that offeredRanges mounts 5 ranges × both registries × every tab, this asserts no panel anywhere is still showing a loading strip after one settleFast() per tab plus the final settle(). The body reports 615 reads and 0 loading on a local run with Node present; on a slower CI agent this is the assertion most likely to be the first flake, and WebRangeKeptHistoryBehaviourTests.TryRun skips entirely where Node is absent, so the check is invisible on those agents rather than failing.
  5. The record resolving through Lite's HTTP transport. Lite ships the same stateless HTTP MCP pipeline (Lite/Mcp/McpHostService.cs:112) and the same filter (:165), but the live tools/call pin for request.Services resolving McpToolParameterTypes is Darling's only (DarlingMcpHostGateLiveTests). The body discloses this and the reason is sound — Lite.Tests has no harness that issues a real tools/call (Lite.Tests/HostHeaderGuardTests.cs:69 reads the host's source text, it does not run it). So on Lite, a record that failed to resolve would degrade to the schema-only path and surface only as the one-per-process warning, which Lite/Mcp/McpHostService.cs:87 clears the providers for. Worth one manual tools/call with {"hours_back": null} against a running Lite host before this ships.

Read-only notes that came out clean, recorded so they are not re-litigated: InternalsVisibleTo covers both test projects for the internal McpArgumentValueCheck (PerformanceMonitor.Common.csproj:41,46) — the #3688 trap does not bite; the #3667 expression-body/{ }-pattern trap does not bite on the current tree (Problem, Bounds, OutOfRange, Takes, CanBind and WarnOnceThatTheRecordDidNotResolve are all block bodies); declared is correctly left null when types is null and set to an empty dictionary when TryGet misses, so both routes fall to SchemaProblem rather than skipping the value pass (McpUnknownArgumentGuard.cs:193-200); and the refusal quotes no CLR range, so it cannot contradict ValidateHoursBack — which is the #3699/#3641 "refused, never clamped" line held correctly.

claude[bot]
claude Bot previously requested changes Oct 2, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 — Refuse takes the record as an optional third argument; the filter reads it from request.Services — McpUnknownArgumentGuard.cs:153-154, :103-123.
  • VERIFIED — "refuses exactly when System.Text.Json throws a JsonException; anything else passes" — McpToolParameterTypes.cs:157,166, pinned by AValueWhoseConverterThrowsAnythingButAJsonError_PassesToTheBinder.
  • VERIFIED — a string is decided from its token with no deserialize (McpToolParameterTypes.cs:145-151, pinned by ALargeString_IsPassedWithoutBeingCopied); the refusal states no range and an enum is excluded from Bounds in a block body (:137-147, :173-180, :315, pinned by AssertStatesNoRange and HoursBackPastInt_IsRefusedAsTooLarge_AndStatesNoRange on 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 TextElementCutLength comment now matches the walk (McpHelpers.cs:87-92 vs :106-131); the test checks the harness's whole fetches list printed after its final settle() (web-kept-history-harness.mjs:309,326); only offeredRanges marks nodes, so other scenarios' output is byte-identical (:275-278,300,313-323); "Lite has no HTTP harness that reaches tools/call" (Lite.Tests/HostHeaderGuardTests.cs:69,82-83 read 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 BigInteger timing table. Needed: a build and dotnet test on this head; no SDK is runnable here.
  • UNVERIFIED — "agreed with the real binder on all … rows on both products". That lives in the installGuard: false half of EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt; only a run answers it.
  • UNVERIFIED — "Every row still finds a target", and the named int?/long row moves. Resolve walks 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.cs returns 59; by group at :287-365 it is 59, so the total is 62. One row stale: commit 09d9ae5ce's own item 4(b) added "a new ulong row" (-0, line 324).
  • REFUTED — "the pick passes over delete_, create_ or update_, 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 adds mute_analysis_finding, described Write: (Lite/Mcp/McpAnalysisTools.cs:996). A refused: false row 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 _ => null for inert instances (Lite.Tests/McpWholeNumberArgumentTests.cs:49), so every Lite tool service is a RuntimeHelpers.GetUninitializedObject (McpInProcessHost.cs:107).

Riskiest lines

  1. PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs:157 — this deserialize IS the refusal verdict; wrong options mean refusing calls the server would have answered — pinned by McpArgumentTypeTests.EachValue_IsRefusedByName_ExactlyWhenTheBinderCannotReadIt (both SKUs)
  2. PerformanceMonitor.Common/Mcp/McpSchemaCompat.cs:186 — Register runs inside the AddSingleton<McpServerTool> factory; a record that never fills degrades the whole surface behind one process-wide warning — pinned by DarlingMcpHostGateLiveTests.ToolsCall_ANullForAWholeNumber_IsRefusedByName_OverTheShippedHttpPipeline; Lite's HTTP transport (Lite/Mcp/McpHostService.cs:112) is UNPINNED
  3. PerformanceMonitor.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_IsOneTheSchemaAdvertises compares recorded.Keys, built from InputSchema at McpSchemaCompat.cs:185-186, against that same InputSchema at McpArgumentTypeRows.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) and WarnOnceThatTheRecordDidNotResolve (McpUnknownArgumentGuard.cs:129) are block bodies.
  • "InternalsVisibleTo on PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)." — does not apply — PerformanceMonitor.Common.csproj:41,46 name both test projects, which is what lets them call the internal McpArgumentValueCheck.Problem.
  • "hours_back, limit, top, days_back are refused when out of range, never clamped (#3699)." — applies and is honoured — McpUnknownArgumentGuard.cs:205 refuses and never substitutes a default, and dropping the CLR range exists so the guard cannot contradict ValidateHoursBack'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 via Lite.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 McpArgumentTypeTests on both SKUs; the installGuard: false half is the only thing between this change and refusing traffic the server used to answer, and a read cannot evaluate it.
  • Confirm the int null row (McpArgumentTypeRows.cs:305) is green — it decides whether an explicit null for a defaulted int previously bound to the default, in which case this widens refusals into working calls.
  • Confirm Resolve finds a target for all 14 type keys on both SKUs; a miss is an InvalidOperationException, and double/double?/string[] resolve through different tools on each.
  • Issue a real tools/call with {"hours_back": null} against a running Lite host (Lite/Mcp/McpHostService.cs:112) and confirm the typed refusal; Lite's request.Services path has no pin and the fallback warning is swallowed by ClearProviders() at :87.
  • Watch Assert.Equal(0, loading) in WebServerPageRangeTests over 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)
@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 2, 2026 23:14

Two findings are fixed in 8afedba, and the third is answered in its thread.

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 2, 2026 23:25
@erikdarlingdata
erikdarlingdata merged commit 8c6a299 into dev Oct 2, 2026
19 of 20 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/mcp-argument-types branch October 2, 2026 23:25

var type = Nullable.GetUnderlyingType(declared.Type) ?? declared.Type;
var takes = Takes(parameter, type);
var sent = McpUnknownArgumentGuard.Shorten(value.GetRawText());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

Blind read, then the body: the row count in the body is stale by one

The table in the tree has 59 rows, not 58:

$ grep -c "rows.Add(" Darling/Darling.Tests/McpArgumentTypeRows.cs
59
$ grep -c "\[Fact\]" Darling/Darling.Tests/McpArgumentTypeTests.cs Lite.Tests/McpArgumentTypeTests.cs
Darling/Darling.Tests/McpArgumentTypeTests.cs:3
Lite.Tests/McpArgumentTypeTests.cs:3

The body's own account says why: the fourth round (09d9ae5ce, item 4b) added "a new ulong row" for -0, and McpArgumentTypeRows.cs:230 is that row. The earlier counts were never re-footed after it.

Three claims read on the current tree rather than on the commit they were measured against, and two of them are now wrong:

  • "agreed with the real binder on all 58 table rows on both products" and "58 rows, one per (CLR type, value)" — present tense, so 59.
  • "so it totals 61 per product now (58 rows, 3 facts)" — "now" is the current tree, and 59 + 3 = 62 per product.
  • "58 rows.Add lines in McpArgumentTypeRows" in the RED paragraph is correctly scoped ("counted from the commit that was run (the first commit, which holds tests only)") and stands as written.

Nothing in the code is wrong and no pin changes meaning — the -0 row's own RED is quoted at item 4b, so it was watched. This is bookkeeping in the body, and it matters only because the counts in this repository's PR bodies are read as the census they describe: a reader reconciling 62 against 61 on the next change to the table has to re-derive which round each number was taken in.

Everything else I checked held

The 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:

  • Parity. The change is in shared PerformanceMonitor.Common; both hosts register the same filter object (Lite/Mcp/McpHostService.cs:165, Darling/.../DarlingMcpHostService.cs:1053), and Lite got the table (Lite.Tests/Lite.Tests.csproj:46-48, Lite.Tests/McpArgumentTypeTests.cs) and the range pin (Lite.Tests/McpWholeNumberArgumentTests.cs:137) alongside Darling's. The only Darling-only additions are the HTTP-pipeline row and the viewer harness, and the body says so and says why (Lite has no HTTP harness reaching tools/call; Lite has no web viewer).
  • No false refusal is structurally possible where a type is on record: CanBind (McpToolParameterTypes.cs:155-167) is System.Text.Json reading the value into the declared type, and only a JsonException refuses. The NotSupportedException / ArgumentException / InvalidOperationException / Undefined-element cases all fall out of ex is not JsonException as "pass", so Refuse cannot throw out of the call-tool filter.
  • The 20-digit shortcut is exact. ulong.MaxValue is 20 digits, the zeros are trimmed first (:234), so a run of 21+ is out of every integer range and the length-and-sign verdict agrees with what BigInteger.Parse would have said. A 20-digit run is still parsed.
  • The census of CLR types will hold today. Across both SKUs the tool methods declare only string, string?, string[]?, int, int?, long, long?, bool, bool?, double and double? — every one has a table key, so EveryClrTypeAShippedToolDeclares_IsOneTheTableCovers has no gap to fall through. The long claim checks out in both directions: Lite's only non-nullable long is McpPlanTools.cs:129 (plan_id, required beside database_name, so Resolve passes the tool over and falls to the probe), and Darling declares both (DarlingMcpCustomAlertTools.cs:209, :324).
  • InternalsVisibleTo does not bite here. PerformanceMonitor.Common.csproj:41,46 names both test projects, so internal McpArgumentValueCheck and the newly-internal WholeNumberOf / Shorten are reachable from both; and the one file linked across projects, McpArgumentTypeRows.cs, touches only public members.
  • The fetches check uses the harness's own convention. WebServerPageRangeTests.HoursAsked's [?&]hours= matches asked = (url) => url.searchParams.get("hours") (web-kept-history-harness.mjs:155), and both treat an absent hours as 0, so the new whole-run check is a strict superset of the per-tab beyond list rather than a second, differently-spelled rule.

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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 GuideCatalogOf does — McpSchemaCompat.cs:126, :167-186, :243-258 against :200-215
  • VERIFIED — Refuse takes the record as an optional third argument; the filter reads it from request.Services — McpUnknownArgumentGuard.cs:153-154, :105, :116
  • VERIFIED — the value is read into the declared type with McpJsonUtilities.DefaultOptions, refused only on JsonException, so the guard cannot refuse a value the binder reads nor throw out of the filter — McpToolParameterTypes.cs:157, :160-167; McpSchemaCompat.cs:170-176 sets no SerializerOptions
  • 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, RangeOf is gone, and Bounds excludes enums in a block body — McpToolParameterTypes.cs:137-139, :173-179; Plural's enum arm at :315 precedes :316
  • VERIFIED — a string is decided from its token, with no deserialize — McpToolParameterTypes.cs:150-153, ahead of the try
  • VERIFIED — the 20 digits are counted after the leading zeros, and 20-or-fewer keeps the old verdict — McpToolParameterTypes.cs:234-246; ulong.MaxValue is 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_back is a non-nullable int — DarlingMcpAlertTools.cs:83
  • VERIFIED — the TextElementCutLength cost claim is now limited to text ASCII at the cut — McpHelpers.cs:90-92 against the unbounded backward scan at :107-110
  • VERIFIED — the whole-run read check uses the harness's own convention — HoursAsked's [?&]hours= matches asked at web-kept-history-harness.mjs:155; both treat an absent hours as 0, so it is a strict superset of beyond
  • VERIFIED — the long story holds in both SKUs, and every CLR type a shipped tool declares has a row — Darling declares both (DarlingMcpCustomAlertTools.cs:209, :324); Lite's only long is McpPlanTools.cs:129, required beside database_name; the sweep yields only types s_types keys
  • 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 BigInteger timing 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(" on Darling/Darling.Tests/McpArgumentTypeRows.cs is 59, with 3 [Fact]s, so 62 per product. The body's own item 4b added "a new ulong row" (McpArgumentTypeRows.cs:230) in 09d9ae5ce after the counts were written. The RED paragraph's scoped "58" is correct as written

Riskiest lines

  1. 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_ExactlyWhenTheBinderCannotReadIt on both SKUs, whose guard-less half fails if the guard and the binder ever disagree
  2. PerformanceMonitor.Common/Mcp/McpSchemaCompat.cs:186 — Register inside 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 by EveryAdvertisedParameter_HasARecordedType_ThatAgreesWithTheSchema, EveryRecordedParameter_IsOneTheSchemaAdvertises, and DarlingMcpHostGateLiveTests.ToolsCall_ANullForAWholeNumber_IsRefusedByName_OverTheShippedHttpPipeline
  3. 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)
  • "InternalsVisibleTo on PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)" — does not apply — PerformanceMonitor.Common.csproj:41,46 names both, and the cross-linked McpArgumentTypeRows.cs calls 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-deleted RangeOf
  • "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_back are refused when out of range, never clamped (#3699; #3641)" — applies, and holds — McpToolParameterTypes.cs:137-139 substitutes nothing, and dropping the CLR range leaves McpHelpers.ValidateHoursBack as 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 — Shorten still routes through McpHelpers.Truncate and TextElementCutLength (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 McpArgumentTypeTests on both SKUs: Assert.Null(ProbeRefusal("color", "\"Amber\"")) and ASignAndLeadingZeros_OnAWholeNumberThatFits_AreNotRefused rest 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:137 does 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_guide call with a large tools array, now that the guard deserialises every argument before the binder does (McpToolParameterTypes.cs:157).
  • Run WebServerPageRangeTests with 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant