Repository navigation
Darling MCP write tools: honest statuses, hash validation, named missing arguments (#6455) - #6509
Conversation
…why the password is needed (part of #6455)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| /* #6455: a NEW assignment must name something a query hash can be; unassign skips this so a key stored before the check | ||
| can still be removed. */ | ||
| if (requireHashShape && !IsHashText(hash)) |
There was a problem hiding this comment.
Parity: Lite's assign_query_tag is the same tool and does not get this check.
Lite/Mcp/McpQueryTagTools.cs:788-790 is the same BuildKey hash branch with no shape test — it still does QueryHashIdentity.CanonicalKey(hash) ?? hash and stores whatever arrived. So after this PR:
assign_query_tag {tag_id: 1, kind: "hash", query_hash: "hello"}
Darling → {"status":"invalid","hints":{...},"code":"bad_key"} (this hunk)
Lite → {"status":"assigned", ...} and a row the resolver can never match
That is exactly the silent-inert-assignment the check exists to stop, and Lite's own class doc promises the opposite: "Lite's twin of Darling's query-tag and query-tag-rule tools (#5981, #5982), under the same names, parameters, refusal codes and result shapes" (Lite/Mcp/McpQueryTagTools.cs:27). .github/REVIEW-TRAPS.md calls this shape out: "a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit".
Nothing in Lite's existing tests blocks the fix — every hash in Lite.Tests/McpQueryTagToolTests is already valid hex (0xABCD, 0x01, 0xAB, 0xabcdef).
Please also avoid a second copy of the predicate when you do it (#3684's "Alias, do not copy"): IsHashText reads only text, both SKUs already reference PerformanceMonitor.Common, and it sits naturally next to QueryHashIdentity.CanonicalKey in PerformanceMonitor.Common/QueryHashIdentity.cs — one definition called from both BuildKeys, with the refusal sentence shared too so the two tools cannot drift into refusing differently.
| can still be removed. */ | ||
| if (requireHashShape && !IsHashText(hash)) | ||
| { | ||
| refusal = Refusal("invalid", "bad_key", "query_hash must be hexadecimal: 0x followed by 1 to 16 hex digits (for example 0x1A2B3C4D5E6F7081), or the decimal number the long-query trace stores."); |
There was a problem hiding this comment.
Non-blocking, but the check stops one case short of its own doc ("a NEW assignment must name something a query hash can be"): an all-zero hash passes. IsHashText("0") → ulong.TryParse succeeds, IsHashText("0x0000000000000000") → all hex digits, so both are accepted, and CanonicalKey returns NULL for them (QueryHashIdentity.IsUnknown) so the key is stored as typed.
QueryHashIdentity is explicit that this is not a hash: "0 here means 'the engine did not say', never 'query number zero'" — and the comment immediately below this hunk still names it as such: "A hash that is not a hash at all (all zeros, or neither hex nor decimal) is kept as typed." An assignment to the did-not-say bucket labels nothing, the same outcome as "hello".
refusal = ... when requireHashShape && (!IsHashText(hash) || QueryHashIdentity.IsUnknown(hash)) would close it. While you are here, that comment on 524-525 is now half-stale for the assign path: "neither hex nor decimal" is no longer kept as typed, it is refused two lines above.
| public static McpRequestFilter<CallToolRequestParams, CallToolResult> Instance => | ||
| next => async (request, cancellationToken) => | ||
| Refuse(request.Params, FindTool(request), RecordOf(request)) | ||
| ?? RefuseMissingRequired(request.Params, FindTool(request)) |
There was a problem hiding this comment.
Non-blocking: this arm contradicts the type's own contract paragraph, which the diff leaves standing.
Line 46-49, unchanged: "What it refuses, and what it deliberately does not. Only a key that is NOT a property of the tool's advertised schema. An OMITTED optional parameter is not an unknown key and never was — the guard reads the keys that arrived, never the ones that did not, so optionality and null handling are untouched."
As of this line the guard does read the keys that did not arrive. The optional half is still true, so it is a sentence to amend rather than delete — something like "an omitted OPTIONAL parameter is not refused; an omitted REQUIRED one is, by name, because the binder would throw on it anyway." Worth doing for a file whose doc is the only place this surface's rules are written down. The same sentence is repeated in both SKUs' test-class docs (Darling/Darling.Tests/McpUnknownArgumentGuardTests.cs:44, and Lite's twin: "An omitted optional parameter (the guard reads arrived keys only)").
| writable column name appears on both tables, so a bare name is still unambiguous -- | ||
| asserted, not assumed, by WritableColumnNames_DoNotCollideAcrossTheTwoTables. */ | ||
| updated_fields = updates.Select(u => u.Column).ToArray(), | ||
| updated_fields = updates.Select(u => UpdatedFieldName(u.Column)).ToArray(), |
There was a problem hiding this comment.
Non-blocking: the comment directly above this line is the argument against the line.
Unchanged at 1297-1300: "Bare column names, unqualified, even though two tables are now in play: this array is a consumer API and qualifying the existing entries would redefine every one of them." This change qualifies one of them, deliberately — and it is the right call, because DeliveryCooldownColumn's own doc already claimed it ("delivery.cooldown_minutes is what the tool reports", 2273-2275) and Darling/README.md documents the knob as "reported and accepted as delivery.cooldown_minutes". So the code was the thing that was wrong, which the comment should now say: bare column names except the delivery cooldown, which is reported under its wire name because it is the only writable column whose table is not config_alert_settings.
Left as is, the next reader of #3314's reasoning finds a rule and a counter-example on adjacent lines with nothing saying which won.
|
Three measurements and one observation from the read, none of them blocking — the blocking one is the Lite parity gap, inline on The two lengthened heads still fit their caps. Both The password sentence is consistent in all six places it lives — The three added Unrelated to this PR, but adjacent to finding 7 and now newly visible: |
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Claims
- VERIFIED — the new move-refusal sentence is in every surface that answers it, and the rule does cover "any other connection setting" —
ServerConnectionRule.cs:21,admin.js:481,DarlingManagedRoles.cs:3548,provision-roles.sql:1846;v_movedatprovision-roles.sql:1817-1829is exactly the field list the body names. - VERIFIED — no numbered migration is needed — the function is created by the provisioning batch (
provision-roles.sql:1754) andDarlingManagedRolesTests.cs:125asserts the name is in noPgMigrationsscript. - VERIFIED — dismiss/restore answer a success word only when a row changed,
not_foundfor keys naming no row,unchangedotherwise, counts and undo untouched —DarlingMcpAlertTools.RouteAdmin.cs:532-538overKnown= "rows that exist, dismissed or not" (AlertDismissStore.cs:20-23,CountKnownKeysSql); descriptions at 451 and 491. - VERIFIED — a dry run is gated like a write and the refusal now says why, at all four paths —
DarlingMcpPlanForceTools.cs:229(force/unforce shareRunAsync),:404,:528,:684; a dry run really does sign in with the remediation login (:234-257). - VERIFIED — finding 4 was partly wrong and the new text matches the parser —
CustomAlertRuleDefinition.cs:461-512: defaultAll,modeofall/servers/tag, non-emptyservers, positivetagId. - VERIFIED —
updated_fieldsnames the cooldowndelivery.cooldown_minuteswhichever spelling arrived, other fields unchanged —DarlingMcpAlertTools.cs:1301+:2283, keyed on the column; two spellings in one body are already refused at:3005-3015. Updated assertion atDarlingMcpAlertToolsTests.cs:2996. - VERIFIED — assign refuses a non-hex hash, unassign is unchanged —
requireHashShapedefaults false, passed only atDarlingMcpQueryTagTools.cs:287; the unassign path at:373omits it. - VERIFIED —
RefuseMissingRequiredis in.Common, chained afterRefuse, covers a no-argument call, and reaches both SKUs —McpUnknownArgumentGuard.cs:106,:267-283,:157; registered atLite/Mcp/McpHostService.cs:349andDarlingMcpHostService.cs:1724.Refuseis untouched, so Lite's probes (which call it directly,Lite.Tests/McpUnknownArgumentGuardTests.cs:185) keep their meaning. - VERIFIED —
McpWriteToolWordingTestsis 21 cases (4+3+1+7+5+1), the hash refusals precede any store call (BuildKeyis the first statement ofAssignQueryTagCore), and the rig assertions are atMcpAlertAdminLiveTests.cs:218-223with the Blocking row visible there (dismissed :203, restored :212), sounchangedis right. - UNVERIFIED — every reported run (659/0, 1079/0, Lite 49/0, the PostgreSQL-rig classes) — no
dotnethere, andDarling.Testsneeds Windows plusDARLING_TEST_PG. - UNVERIFIED — "a first version put the check inside
Refuse; four tests failed" — a discarded revision, nothing in the tree to check. - UNVERIFIED — that an upgraded store gets the new PW002 text — the trigger body changes only when provisioning re-runs there.
- REFUTED — "Left to others — Nothing: all seven findings are covered." Finding 6 is Darling-only. Lite ships the same
assign_query_tag, documented as the twin "under the same names, parameters, refusal codes and result shapes" (Lite/Mcp/McpQueryTagTools.cs:27), and its hash branch (:788-790) still stores any text as a key. The body reasons about Lite for finding 7 and not for 6.
Riskiest lines
PerformanceMonitor.Common/Mcp/McpUnknownArgumentGuard.cs:106— one arm refuses calls across every tool of both SKUs off the generatedrequiredlist; a list that names a parameter a caller cannot send (a DI service, a host-bound value) refuses every call to that tool — covered byACallWithoutARequiredArgument_IsRefusedNamingItfor its two tools only (thedismiss_alert_historyrow fails on a leaked name, since the message takes the plural branch); UNCOVERED for every other registered tool.Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryTagTools.cs:518— the new refusal, and the half missing on the other SKU — covered on Darling byMcpWriteToolWordingTests.AssignQueryTag_RefusesAHashThatIsNotHexadecimal; UNCOVERED on Lite, where neither check nor test exists.Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.RouteAdmin.cs:532— a status word callers branch on changed meaning — covered byDismissAlertHistory_OnlyAnswersDismissedWhenItDismissedARow, its restore twin, andADismissedAlert_LeavesTheHistoryRead_AndTheUndoBringsItBack.
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" — applies —
DarlingMcpQueryTagTools.cs:518refuses a non-hash hash;Lite/Mcp/McpQueryTagTools.cs:788-790does not. Every other changed surface is Darling-only (grep custom_alert Lite/Mcpis empty; the alert-admin and plan tools have no Lite twin), and finding 7 sits in.Commonwith both hosts registering it. - "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies, prospectively — all six copies of the sentence moved together and
DarlingManagedRolesTests.cs:157pins the SQL againstServerConnectionRule;IsHashTextis the definition that must be shared, not copied, when Lite is fixed. - "Ruling (#5833): tests are load-bearing … never answer a trap with a test that reads source text or asserts how code is spelled" — applies, minor — the three
Assert.ContainsatServerEditCoreTests.cs:785-789sit beside anAssert.Equalon the same string and can only fail on a wording edit; everything else added is behaviour. - "
hours_back,limit,top,days_backare refused when out of range, never clamped" — does not apply — the guard refuses without filling the argument in (McpUnknownArgumentGuard.cs:280-282) and the hash check refuses rather than canonicalising (DarlingMcpQueryTagTools.cs:518-521).
Divergence (blind read vs PR body)
The one that matters: the seven-for-seven completeness claim does not hold for finding 6 — Lite's identically-named assign_query_tag still accepts "hello" as a query hash, so two tools documented as twins now refuse differently. Two smaller ones, both documentation the diff left standing and the body does not mention: McpUnknownArgumentGuard's contract paragraph still says the guard "reads the keys that arrived, never the ones that did not" (lines 46-49) directly above the arm that reads the ones that did not, and DarlingMcpAlertTools.cs:1297-1300 still argues that qualifying an updated_fields entry "would redefine every one of them" directly above the line that qualifies one. In the other direction the body undersells finding 5: the change makes the code agree with DeliveryCooldownColumn's own doc and with Darling/README.md, which both already claimed the reported name.
Reviewer checklist
- Build and run both suites on Windows (no
dotnetin this environment): the two new classes,McpToolsListBudgetTestsandMcpToolGuideTests(the dismiss head now measures 585 of the 620 target), and Lite's guard tests. - Call a tool through a real MCP host with a required argument omitted, and confirm from a live
tools/listthat no tool'srequiredarray names a DI-injected parameter — the body states no live MCP call was made for finding 7, and that array now decides whether a call runs at all. - Upgrade an existing store and confirm the provisioning run replaces the PW002 trigger body: with no migration rung, until it re-runs that store raises the old sentence while the app ships the new one.
…n the database (#6455) The test counted every backend with an xid in the scratch database, so a TimescaleDB job worker or autovacuum holding an xid made it read 2. It now asks about its own writer.
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — "Lite's assign_query_tag still stored any text as a hash key... the shape check moved from DarlingMcpQueryTagTools.IsHashText to one shared QueryHashIdentity.IsAssignableHash... both products' assign paths ask it (requireHashShape, passed only by assign; unassign skips it on both)" — Lite/Mcp/McpQueryTagTools.cs:303 and Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryTagTools.cs:287 both pass
requireHashShape: trueon assign; the unassign call sites (Lite:376, Darling:373) omit it; the old privateIsHashTextis deleted and no reference to it remains in the tree. - VERIFIED — "an all-zero hash (0, 0x0000000000000000) passed the check... now refused on both" —
IsAssignableHashroutes throughKnownHash/IsUnknown(PerformanceMonitor.Common/QueryHashIdentity.cs:91-95), which treats all-zero as unknown and returns false; new[InlineData("0")]/[InlineData("0x0000000000000000")]cases were added to both Darling.Tests/McpWriteToolWordingTests.cs and Lite.Tests/McpQueryTagToolTests.cs. - VERIFIED — "Lite.Tests McpQueryTagToolTests gained a nine-case behaviour test that assign refuses each bad hash and stores nothing, and one that unassign does not apply the refusal" —
AssignQueryTag_RefusesAHashThatIsNotHexadecimal_AndStoresNothinghas 9[InlineData]entries and assertsstatus == "invalid"plus an empty-assignment follow-up read;UnassignQueryTagCore_DoesNotApplyTheHashShapeRefusal_SoAnOlderKeyCanStillBeRemovedis the separate fact. - VERIFIED — "the two stale comments the bot found are updated" —
McpUnknownArgumentGuard.cs's contract paragraph now separates the unknown-key check fromRefuseMissingRequired, andDarlingMcpAlertTools.cs'supdated_fieldscomment now names the cooldown exception. - VERIFIED — CI-fix commit's mechanism:
QueryStoreComposeStampWriterGuardLiveTests.cs's xid count now filterspid = {writer.ProcessID}instead of counting every backend in the database, matching the stated TimescaleDB-worker/autovacuum false-positive explanation. - UNVERIFIED — reported test run totals (e.g. "Darling.Tests... 1189 passed, 0 failed", "Lite.Tests... 205 passed, 0 failed", "3 of 3 pass at the PR head... 3 of 3 pass at origin/dev") — these are live/CI execution results that cannot be reproduced from a read-only tree review.
Riskiest lines
- PerformanceMonitor.Common/QueryHashIdentity.cs:88-122 (
IsAssignableHash) — now the single shared validator both SKUs' assign paths call; a regression here breaks hash-key validation identically on both products — covered by Darling'sMcpWriteToolWordingTests.AssignQueryTag_RefusesAHashThatIsNotHexadecimal(9 cases through the realAssignQueryTag) and Lite'sMcpQueryTagToolTests.AssignQueryTag_RefusesAHashThatIsNotHexadecimal_AndStoresNothing(9 cases through the realAssignQueryTagCore), both of which fail if the shape check regresses. - Lite/Mcp/McpQueryTagTools.cs:303 (
requireHashShape: trueon assign'sBuildKeycall) — this one argument is the entire Lite/Darling parity fix; dropping it silently reopens the original bot-reported gap — covered byMcpQueryTagToolTests.AssignQueryTag_RefusesAHashThatIsNotHexadecimal_AndStoresNothing. - Darling/Darling.Tests/QueryStoreComposeStampWriterGuardLiveTests.cs:67 (
pid = {writer.ProcessID}) — this is itself the fix for a flaky live-Postgres assertion; a regression (e.g. back to counting every backend) would reintroduce the CI flake under concurrent TimescaleDB workers — UNCOVERED by any other automated test; validated only by the author's reported live-rig runs (noted UNVERIFIED above), since it is a live-rig assertion rather than product code with its own test.
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" — applies as the subject of this diff, not as a new violation: the diff is exactly closing that gap for
assign_query_tag(Lite/Mcp/McpQueryTagTools.cs:303 now mirrors Darling/.../DarlingMcpQueryTagTools.cs:287). - "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 here: the diff removes the second copy (Darling's private
IsHashText) in favor of one sharedQueryHashIdentity.IsAssignableHash(PerformanceMonitor.Common/QueryHashIdentity.cs:88) that both products now call, which is the trap's own prescribed fix rather than a new instance of the trap.
Divergence (blind read vs PR body)
None — the body's account (the shared-method extraction, the requireHashShape wiring on both assign paths only, the all-zero-hash fix, the stale-comment fixes, and the xid-count scoping) matches the diff on every claim graded above.
Reviewer checklist
- Confirm the reported test totals (Darling.Tests 1189 passed / Lite.Tests 205 passed, and the 3/3 local reruns of the xid test at both the PR head and
origin/dev) by running the suites, since a read-only tree review cannot execute them.
1 participant
Claude posting for Erik Darling.
Fixes #6455.
What changed
Seven findings, in the issue's order:
edit_serverhost change wording: the refusal for a move that keeps the stored password now reads "Changing the host, port or any other connection setting of this server needs its password sent with the change, because a saved password is only ever sent to the server it was saved for. Send the password together with the new settings." The rule (ServerConnectionRule, the store trigger) refuses a change of host, port, engine, database, read-only intent, auth, username, encryption mode, trust-certificate, multi-subnet failover or the Query Store replica, so the text names the host and port and says "any other connection setting". Same text inServerConnectionRule.PasswordNeededOnMoveText(MCP, Viewer), the webadmin.js, the store trigger inDarlingManagedRoles.csandDarling/tools/provision-roles.sql. No numbered migration: the trigger is redefined by the role provisioning, whichCREATE OR REPLACEs it on each run.dismiss_alert_history/restore_alert_history: status is nowdismissed/restoredonly when a row changed. Keys that name no row answernot_found; rows already in the asked state answerunchanged. Counts and the undo are unchanged. Tool descriptions say so.force_query_store_plan,unforce_query_store_plan,evict_cached_plan,kill_session, SQL Server and PostgreSQL paths): a dry run with noagent_actionstill refuses (it signs in with the remediation login and reads the per-server switch, so it is gated like a write), but the refusal text now says why a dry run needs the claim. I took the issue's "or the refusal text says why" option; a dry run that reports the switch state without a claim would change the gate's order and is a design call.create_custom_alert_rule: the finding is partly wrong. A server scope already exists inside the definition (scope.modeall, servers, tag). The tool text now says so, with the JSON to use, and that no scope means every monitored server.update_alert_settings:updated_fieldsnames the cooldowndelivery.cooldown_minutes(the name used in the call and the read), whether the caller sent that or theemail_cooldown_minutesalias. Other fields keep their names. The existing live test that asserted the old name is updated.assign_query_tag: aquery_hashthat is not0xplus 1 to 16 hex digits, or a decimal that fits an unsigned 64-bit number (the long-query trace form), is refused asinvalid/bad_key.unassign_query_tagis not changed, so a key stored before this check can still be removed.McpUnknownArgumentGuard.RefuseMissingRequired, chained afterRefusein the shared call-tool filter. It reads the tool's advertisedrequiredlist and answersinvalidnaming the argument, with the accepted parameters. It also covers a call with no arguments at all (the old guard skipped those). It is inPerformanceMonitor.Common, so Lite gets it too. I leftRefuseunchanged so its single-argument probe tests keep their meaning.Proof
McpWriteToolWordingTests(21 cases): the dismiss/restore status rule, the cooldown field name, hash refusals through the realAssignQueryTag(refused before any store call), hash acceptance, and the dry-run refusal text for the four code paths' shared helper.McpUnknownArgumentGuardTests:ACallWithoutARequiredArgument_IsRefusedNamingItagainst the real registeredupdate_custom_alert_rule(noversion) anddismiss_alert_history(no arguments);ACallWithNoArguments_IsNotRefusedWhenNothingIsRequired.McpAlertAdminLiveTests(rig): dismiss and restore of an unknown key answernot_found; restore of a not-dismissed row answersunchanged.DarlingMcpAlertToolsLivePostgresTestsassertsdelivery.cooldown_minutesinupdated_fields.McpWriteToolWordingTests,McpUnknownArgumentGuardTests,McpWholeNumberArgumentTests,McpArgumentTypeTests, every*Census*,*Ratchet*,*WritePolicy*class: Total 659, Failed 0 (1 skipped). On a throwaway PostgreSQL rig:McpAlertAdminLiveTests(9),DarlingMcpQueryTagToolsLiveTests,DarlingMcpAlertToolsLivePostgresTests,DarlingMcpAlertToolsSurfaceAndSqlTests,PlanForceToolLiveTests,ServerActionGateTests: 0 failed. Lite: guard, whole-number, Census and Ratchet classes, Total 49, Failed 0.Refuse; four existing tests that probe one argument at a time failed, which is why it is a separate method.Finding 1 proof (added)
ServerEditCoreTests.AMoveOfHostOrPort_OnASecretRow_AsksForThePasswordBeforeTheTest_WithNoProbeAndNoWrite(callsedit_serverwith a new host or port and no password) now also checks the refusal names the host and port and says why.ServerEditPasswordRuleViewerTests(MCP, Viewer and the core agree) andAdminServerEditBehaviourTests(web) pass with the new text.pg-runtime.zip:StoreServerPasswordRulesLiveTestsgets the new text from the provisioned store trigger (SQLSTATE PW002), as the admin, the MCP role and the Viewer.DarlingManagedRolesTests,ServerPasswordFromRequestTests, every*Census*and*Ratchet*class: Total 1079, Failed 0, Skipped 0. Not run: the full suite and the pre-push gate (laptop override; CI decides).Left to others
assign_query_tagis now covered too, see the round 1 line below.)Instanceonly, covered throughRefuseMissingRequiredon the real tool schemas.CHANGELOG entry
SECTION: Fixed
dismiss_alert_historyandrestore_alert_historyanswernot_foundorunchangedinstead ofdismissed/restoredwhen no row changed;update_alert_settingsnames the cooldowndelivery.cooldown_minutes;assign_query_tagrefuses a non-hexadecimalquery_hash; a call that leaves out a required argument is refused naming it (also in Lite) instead of "An error occurred invoking";edit_servernames the host, port and connection settings when it asks for the password again; the plan tools' refusal says why a dry run needsagent_action;create_custom_alert_ruledocuments the scope field in its definition.CI fix
CI fix: test.
QueryStoreComposeStampWriterGuardLiveTests.AnUnrelatedLongWriter_...asserted that exactly one backend in the scratch database holds an xid (SELECT count(*) FROM pg_stat_activity WHERE datname = current_database() AND backend_xid IS NOT NULL AND pid <> pg_backend_pid()), so any other backend that holds an xid at that moment (a TimescaleDB job worker or autovacuum in the throwaway database) made it read 2, as CI did. The PR's diff (MCP write-tool wording, unknown-argument guard, role provisioning comments) touches nothing this test reads: no compose-stamp, scratch-database or pg_stat_activity code. The assertion now scopes the count to the test's own writer (pid = writer.ProcessID), so it proves the writer holds an xid whatever else runs in the database. Every other assertion is unchanged. Mergedorigin/devinto the branch.Local runs of the test on a throwaway rig (UTC): 3 of 3 pass at the PR head (with dev merged), 3 of 3 pass at origin/dev, so it does not fail locally either way; the cause comes from reading the test, not from reproducing it. Ran the CI-selected Darling classes,
*Census*,*Ratchet*,*QueryStoreComposeStamp*, McpWriteToolWordingTests, McpUnknownArgumentGuardTests: Totals 946 / 553 / 591, Failed 0 (88 skipped: tests that need a SQL Server).Bot review round 1 fixes: the bot was right that finding 6 reached Darling only. Lite's
assign_query_tagstill stored any text as a hash key (Lite/Mcp/McpQueryTagTools.cs, "hello" went in as typed). Fixed in about 30 lines of product code: the shape check moved fromDarlingMcpQueryTagTools.IsHashTextto one sharedQueryHashIdentity.IsAssignableHashinPerformanceMonitor.Common(0xplus 1 to 16 hex digits, or up to 20 decimal digits that fit a ulong, and not the all-zero did-not-say hash), both products' assign paths ask it (requireHashShape, passed only by assign; unassign skips it on both), and Lite's refusal text and code (invalid/bad_key) are the Darling ones. The bot's non-blocking note that an all-zero hash (0,0x0000000000000000) passed the check is taken: it is now refused on both. Lite.TestsMcpQueryTagToolTestsgained a nine-case behaviour test that assign refuses each bad hash and stores nothing, and one that unassign does not apply the refusal; the Darling refusal theory gained the two all-zero cases. The two stale comments the bot found are updated (McpUnknownArgumentGuard's contract paragraph now says the unknown-key check reads the keys that arrived and a missing required one is its own arm; theupdated_fieldscomment inDarlingMcpAlertToolsnames the cooldown as the one qualified entry) and the stale "kept as typed" comment on the assign path is reworded. Ran: Darling.Tests AdminServerEditBehaviourTests, DarlingMcpAlertToolsTests, McpAlertAdminLiveTests (own PostgreSQL), McpUnknownArgumentGuardTests, McpWriteToolWordingTests, ServerEditCoreTests, QueryHashIdentity*, QueryTag*, McpToolsListBudgetTests, McpToolGuideTests and all Census and Ratchet classes (1189 passed, 0 failed, 0 skipped); Lite.Tests McpQueryTagToolTests, LiteQueryTag*, QueryHashIdentityTests, McpUnknownArgumentGuardTests, McpToolsListBudgetTests and all Census and Ratchet classes (205 passed, 0 failed).