[erikdarlingdata]

Claude posting for Erik Darling.

Fixes #6403.

What changed

Every write tool on #6403's list now answers with the previous value and an undo call, in the shape the newer tools use (previous plus undo = {tool, arguments}, or undo: null with an undo_note where no single call restores the state). The work landed in groups, each described in its own section below (A, B, C), and the server-admin group (add_servers, edit_server, remove_server) is the last one, described first here.

Server-admin group

• edit_server (updated answer): previous holds the fields the call changed, as they were; undo is an edit_server call that sets them back, carrying the new expected_modified_at, so a change made after the edit makes the replay a conflict instead of an overwrite. The stored password and the AWS external ID cannot be read back, so they are never in previous or undo. When the edit changed only secrets (a password-only edit), undo is null. undo_note says so in one sentence, and says the undo needs the password (and the external ID, for an AWS role) added to its changes_json when it puts a SQL or service-principal server's connection or role back.
• add_servers: each result row has undo. An added row carries the remove_server call for its storage name; skipped, refused and failed rows carry null.
• remove_server: previous is the removed definition in the keys add_servers takes (no AWS external ID; a SQL or service-principal row carries the placeholder <password> in previous as well as in the undo, since the stored password cannot be read back); undo is the add_servers call that re-adds it, with the password as the placeholder <password> (a SQL or service-principal server only). undo_note names what a re-add does not bring back, read from the code: the removal also deletes the server's tag assignments, query-tag assignments and rules, and collector schedule overrides and run times (Darling server admin: list_servers current at once, remove_server clears schedule rows, cost can be cleared (#6454) #6515); the new row has the default monthly cost, excluded databases, plan-capture and alert-delivery settings and comes back enabled; an AWS external ID is not restored. Collected history and the connected-servers registry row are never deleted and reattach by server id.
• Tool descriptions name the new fields. edit_server's core is shared with the web edit route, which now also receives the three extra keys in its answer.
• Merged origin/dev twice (the second time after A SQL Server port is refused on new entries, warned about on existing ones, and no longer named in a failed connect (#6583) #6584 landed; no overlap). One conflict in DarlingMcpAlertTools.cs (Darling MCP write tools: unclear wording and missing validation #6455's updated_fields name and this PR's previous/undo): kept both. update_alert_settings reports delivery.cooldown_minutes in updated_fields and in previous.

Proof (server-admin group)

Darling on a throwaway PostgreSQL rig (stopped and deleted after), new facts in ServerEditLiveTests.Undo.cs:

• EditServer_AnswersWithPreviousAndAnUndo_...: edits name, cost, host and a TLS flag, runs the undo, and the stored definition equals the before state; a later edit makes the replay conflict and the row keeps the later state. Reverted the expected_modified_at in the undo: the test failed (updated instead of conflict).
• EditServer_APasswordChange_...: a password-only edit has undo: null and a note; a name-plus-password edit's undo carries only the name, and the typed password appears nowhere in the answer.
• AddServers_EachAddedEntry...: the added row's undo (remove_server) removes it; the duplicate row has no undo.
• RemoveServer_AnswersWithThePreviousDefinition_...: the removed definition is in previous with no stored password; the undo with the placeholder replaced re-adds the same name, host, auth, username, encryption and intent; the tag assignment the removal deleted is not back, as the note says.
• Run: ServerEditLiveTests, ServerAdminStaleReadsLiveTests, DarlingMcpServerAdminToolsTests, ServerEditCoreTests, ServerAddRouteTests, ServerAddViewerRoleLiveTests, AddServerVerbTests, AwsRoleEditTests, DarlingMcpAddServersPolicyTests, DarlingMcpAgentRouteTagGuardLiveTests, QueryTagStoreLiveTests, McpMachineIdentityTests, QueryStoreReplicaConnectionTestTests, McpWriteToolUndoLiveTests, ToolUndoAnswersLiveTests, every *Census* and *Ratchet* class: Total 962, Failed 0, Skipped 2. After the merge of A SQL Server port is refused on new entries, warned about on existing ones, and no longer named in a failed connect (#6583) #6584: SqlServerPortIgnoredTests, ServerEditCoreTests, DarlingMcpServerAdminToolsTests, ServerAddRouteTests: 140, Failed 0. The alert-tool classes the first merge touched (DarlingMcpAlertToolsTests, ExcludedVolumesStoreTests, ClientTimeoutsSettingsLiveTests, AlertResolutionSettingsRungTests, McpToolGuideHeads*, McpWriteToolWordingTests): 195, Failed 0. Darling.Tests builds with 0 errors; the 4 warnings in the full build are in DarlingMcpNamedTokenTests.cs, a file this PR does not touch (the service project alone builds with 0 warnings).
• cpucap.sh and testslot.sh were refused by the harness, so I built with plain dotnet build ... -m:1 and ran Darling.Tests.exe directly. Not run: Lite (no Lite file changed in this lane), the full suite and the pre-push gate (CI decides).

Considered, not added

• A re-add that restores tags, query tags, schedules and cost: no single call does it, and the note says what is lost.
• Reading previous inside the delete transaction for remove_server: the row is read just before the transaction. A concurrent edit in between would make previous one edit old; no concrete trigger beyond two operators editing and removing the same server in the same moment.
• A password placeholder check on the undo: add_servers already refuses a placeholder that fails the connection probe.

CHANGELOG entry

SECTION: Fixed
ENTRY:

• Darling MCP write tools return the previous value and an undo call ([Darling MCP: mute-rule and route toggles return the previous value and an undo call (part of #6403) #6514]) - the alert, mute, route, custom alert rule, custom view, server-tag and query-tag tools (and the Lite route and query-tag tools) answered a change without the old value or a call to put it back. Each now answers with previous and an undo call in the {tool, arguments} shape, or undo: null with an undo_note where no single call restores it. This includes add_servers (each added server carries the remove_server call), edit_server (the changed fields as they were, as an edit_server call guarded by the new modified_at; passwords and AWS external IDs are never returned, and the note says what to add) and remove_server (the removed definition and an add_servers call with a password placeholder; the note says the server's tags, query-tag assignments, schedule overrides, cost and excluded databases are not brought back).
  REF:
  [Darling MCP: mute-rule and route toggles return the previous value and an undo call (part of #6403) #6514]: Darling MCP: mute-rule and route toggles return the previous value and an undo call (part of #6403) #6514

Group B

Each tool now answers with previous and undo as {tool, arguments} (same shape as enable_server/disable_server), built through one helper, DarlingMcpUndoShape.Call. Each proof below calls the tool, replays the returned undo through the real tool, and checks the store is back.

Darling (McpWriteToolUndoLiveTests, throwaway PostgreSQL, one test per tool pair):

• create_custom_alert_rule (undo = delete_custom_alert_rule), update_custom_alert_rule (undo = update_custom_alert_rule at the new version, naming only the fields the call changed), delete_custom_alert_rule (undo = create_custom_alert_rule with the deleted definition; undo_note says the rule gets a new id and its per-server alert state is not restored). The older string undo on the expiry path is now the same object (CustomAlertRuleExpiryLiveTests updated). There is no separate "enable" tool for custom alert rules: enabling goes through update_custom_alert_rule, covered by the update test (it flips enabled).
• create_custom_view, update_custom_view, delete_custom_view (a personal view is recreated shared, and the note says so).
• create_query_tag, update_query_tag (also on unchanged), delete_query_tag (a leaf tag undoes with create_query_tag; a tag with children, assignments or rules returns previous and an undo_note, because no single call restores the subtree), assign_query_tag (new: undo is unassign_query_tag; move: undo is assign_query_tag with the earlier tag), unassign_query_tag.
• create_query_tag_rule, update_query_tag_rule (also on unchanged), delete_query_tag_rule.
• merge_deadlock_signatures: previous (counted as itself), undo: null and an undo_note pointing at Agent changes (a merge has no tool-call undo). Asserted in SignatureMergeLiveTests.MergedSignatures_ShareCounts.

Lite (McpQueryTagWriteUndoTests, temp DuckDB): the same eight query-tag and query-tag-rule tools, same shape.

Tool descriptions name the new fields. Left to group A: the Lite route tools (set_notification_route_enabled, delete_notification_route) and Lite mute_analysis_finding/unmute_analysis_finding; I did not touch them.

An update that cleared an expiry which had already passed now gets an undo that restores no expiry (a past instant is refused by the tool), with an undo_note saying so (round 1).

Run on a throwaway PostgreSQL rig (stopped): McpWriteToolUndoLiveTests, SignatureMergeLiveTests, CustomAlertRuleExpiryLiveTests (22 tests, 0 failed); then DarlingMcpCustomAlertToolsTests, DarlingMcpCustomViewToolsTests, DarlingMcpQueryTagToolsLiveTests, DarlingMcpQueryTagRuleToolsLiveTests, DarlingMcpWritePolicyTests, AgentChangeToolUndoLiveTests, AgentLevel2*, every *Census* and *Ratchet*: Total 663, Failed 0. Lite: McpQueryTagWriteUndoTests, McpQueryTagToolTests, LiteQueryTagMcpToolTests, every Census and Ratchet class: Total 68, Failed 0. Not run: the full suite and the pre-push gate (laptop override; CI decides).

Whether #6403 is fully closed depends on group A and on #6402 (server-admin tools): "Part of #6403" stays until the last group confirms nothing is left.

Group A

Every remaining alerting write tool on Darling now answers with previous and undo as {tool, arguments} (same shape as above; helper UndoCall). Each is proved by ToolUndoAnswersLiveTests (own scratch store): the test makes the change, takes the undo out of the answer, runs it through the same tool, and compares the stored state. Where a tool already answered partly, only the gap was filled.

Tool	previous	undo	Test
update_alert_settings	the sent fields as they were, in the nested read shape (the cooldown alias maps to delivery.cooldown_minutes; a route the file or default decided comes back null, which clears the store column again)	update_alert_settings with that object	UpdateAlertSettings_AnswersWithThePreviousSettings_AndAnUndoThatRestoresThem
set_alert_threshold_override	previous_override was already there	same tool with the old value, or clear: true when there was none	SetAlertThresholdOverride_AnswersWithAnUndo_...
create_mute_rule	null	delete_mute_rule	CreateMuteRule_AnswersWithAnUndoThatDeletesTheRule
update_mute_rule (updated and unchanged)	the sent fields as they were (empty = null, which clears)	update_mute_rule with them	UpdateMuteRule_AnswersWithThePreviousFields_... (2 cases)
delete_mute_rule	the deleted rule	create_mute_rule carrying its fields (new id and creation date, enabled)	DeleteMuteRule_AnswersWithTheDeletedRule_...
create_notification_route	null (its undo existed)	delete_notification_route	DeleteNotificationRoute_...
update_notification_route	already there; the unchanged answer now has it too	update_notification_route	same test
delete_notification_route	the route as it was	create_notification_route with match, scope, enabled, resolutions, fallback, recipients; sealed webhook URLs and keys cannot be read back, so the undo says to pass them again	same test
mute_analysis_finding	null for a new mute	unmute_analysis_finding for the same scope; "None needed" when it already existed (unmute_analysis_finding already had both)	MuteAnalysisFinding_AnswersWithAnUndoThatUnmutesIt
set_mute_rule_enabled, Level 2 (agent_action)	previous.enabled	set_mute_rule_enabled	AgentChangeToolUndoLiveTests.TheLevel2SetMuteRuleEnabledAnswer_...

The Level 2 create/update/delete mute tools run the same cores, so their answers carry the same fields.

Also fixed on this branch: set_notification_route_enabled's tool head had drifted from Lite's (the first commit put the new sentence in the head), failing McpToolGuideTests.EverySharedToolName_...; the sentence now sits in the guide. DarlingMcpAlertToolsSurfaceAndSqlTests.TheTwoConfigTables_AreReadUnderOneRepeatableReadSnapshot counts three combined-entry-point reads now (the new pre-write read uses the same snapshot entry point); its rule holds, the count moved.

Run on a throwaway PostgreSQL rig (stopped after): *Mute*, *McpRoute*, *Census*, *Ratchet*, *McpToolGuide*, *McpToolsListBudget*, *DarlingMcpAlertTools*, *AgentChange*, *ToolUndo*, *NotificationRoute*, *ThresholdOverride*, *McpAlertAdmin*: Total 1347, Failed 0. An earlier wider run (instructions budget, DarlingMcpTools, ClientTimeouts, AlertResolution, ExcludedVolumes, payload contract) found only the two failures fixed above. Not run: the full suite and the pre-push gate (laptop override; CI decides).

Limits: an undo of a delete cannot restore the id, the creation date, or a disabled flag (create makes a new enabled rule or route); the tool text says so. update_alert_settings previous reads the effective settings, so a stored pair that was crossed comes back clamped.

For the coordinator: group B's text says the Lite route tools and Lite mute_analysis_finding/unmute_analysis_finding are "left to group A"; my brief assigned the Lite tools to group B, so I did not touch them. They are unclaimed. Server-admin tools stay with #6402. Keep "Part of #6403" until that is settled.

Group C

Same previous / undo shape (DarlingMcpUndoShape.Call on Darling; the same {tool, arguments} object on Lite). Each proof calls the tool, runs the returned undo through the real tool, and checks the stored state is back.

Product	Tool	previous	undo	Test
Darling	set_notification_route_fallback	{fallback_route_id}	same tool with the old fallback, or with none	already done in the first commit of this PR (NotificationRouteFallbackLiveTests); confirmed in the code, nothing left
Darling	create_server_tag	null	delete_server_tag	McpServerTagUndoLiveTests.CreateServerTag_...
Darling	update_server_tag (updated and unchanged)	name, colour, parent_id as they were	update_server_tag with them	UpdateServerTag_AnswersWithThePreviousFields_... (2 cases)
Darling	update_server_tag move	the sibling order before	the opposite move; an edge move that writes nothing says "None needed"	AMoveOfAServerTag_...
Darling	delete_server_tag	the tag	a lone tag: create_server_tag (new id). With children, assignments or a rule scoped to it: undo null with an undo_note (the subtree note, as for query tags)	DeleteServerTag_OfALeaf_..., DeleteServerTag_WithChildrenOrAssignments_...
Darling	assign_server_tag	the requested servers that already held the tag	unassign_server_tag for the servers added; "None needed" when nothing was added	AssignServerTag_...
Darling	unassign_server_tag	the servers that held the tag	assign_server_tag for the servers removed; "None needed" when nothing was removed	UnassignServerTag_...
Lite	set_notification_route_enabled (updated and unchanged)	{enabled}	same tool with the old value	NotificationRoutesMcpTests.SetEnabled_AnswersWithThePreviousValue_AndAnUndoThatRestoresIt
Lite	delete_notification_route	the route as it was (no destinations are in it)	null with an undo_note: Lite has no tool that creates a route (destinations are secrets), so Settings > Manage Notification Routes is the only way back	Delete_AnswersWithTheRouteAsItWas_AndNoUndoCall
Lite	mute_analysis_finding	null for a new mute	unmute_analysis_finding for the same scope; "None needed" when the mute already existed	McpMuteReportsWhatItMatchedTests.TheMuteAndTheUnmute_AnswerWithAnUndoThatPutsTheRegistryBack
Lite	unmute_analysis_finding	the removed rows (mute_id, story_path, reason, muted_utc)	mute_analysis_finding with the same scope and the reason a row carried; "None needed" when nothing was removed	same test

Lite's route tools are only these two (Lite has no route create/update tool). Lite FindingStore.UnmuteStoryAsync now returns the removed rows (RETURNING mute_id, story_path, muted_date, reason) instead of a count, as Darling's twin already did; its one caller is the tool. FindingStoreTests anchors on that method's signature to pin which call holds the write lock; the anchor strings follow the new signature and the rule it pins (one write-lock site, with its written reason) is unchanged.

Tool text: the previous/undo sentences are in the guide part after the marker for the Lite route and analysis-mute tools (so the shared heads stay byte-identical, McpToolGuide* passes); the five server-tag descriptions were kept short by hand; 600 is a target, not a gate (the budget test enforces 1,000 on tools carrying the guide marker, and none of the five has one).

Level 2 (agent action) mute rule answers

AgentChangeToolUndoLiveTests gains three cases that run the Level 2 create_mute_rule, update_mute_rule and delete_mute_rule with an agent seat, take the undo out of the answer, run it through the Level 0 tool, and check the store: create is deleted again, update puts the reason and database pattern back, delete makes the rule again. One thing the tests showed: at Level 2 the stored reason carries the agent suffix (known batch [agent:...]), so previous.reason and the recreated rule carry it too. That is the stored value, which is what an undo should restore.

Check of group A's update_alert_settings read (item 5): it was NOT in the write's snapshot; fixed in the code

The pre-write read ran on its own connection and transaction before the write transaction began, so a concurrent update_alert_settings could commit between the two and previous would name a value the write never replaced (the count of 3 was a count of that separate snapshot). Fix: the tool now opens the write transaction first, locks the singleton rows it is about to update (SELECT id FROM <table> WHERE id = 1 FOR UPDATE, in the same fixed table order the writes use), and reads both tables through a new DarlingAlertReader.GetAlertConfigurationAsync(connection, transaction) overload inside that transaction (the single-table reads stay private). A second writer waits on the lock instead of landing between the read and the UPDATE, and nothing can fail that did not before. TheTwoConfigTables_AreReadUnderOneRepeatableReadSnapshot goes back to counting 2 GetAlertConfigurationAsync(postgres) entry-point calls (get_alert_settings and the post-write re-read); its rule (every tool path reads both tables through the combined entry point, under one snapshot) holds, and the pre-write read is now in the write's own transaction instead of a snapshot of its own. Proof: ToolUndoAnswersLiveTests.UpdateAlertSettings_PreviousIsReadUnderTheRowLock_SoAConcurrentWriteIsNotMissed holds the settings row in another transaction, starts the tool (it waits), commits the other write, and checks previous names the other writer's value and the undo restores it. I did not revert the fix to watch that test fail.

Run

Darling on a throwaway PostgreSQL rig (stopped after): *McpServerTagUndoLive*, *ServerTag*, *AgentChangeToolUndoLive*, *ToolUndoAnswersLive*, *DarlingMcpAlertTools*, *McpToolGuide*, *McpToolsListBudget*, *Census*, *Ratchet*, *MuteRule*, *NotificationRoute*, *McpWriteToolUndo*, *DarlingMcpWritePolicy*: Total 1277, Failed 0. Lite: *McpMuteReportsWhatItMatched*, *NotificationRoutes*, *FindingStore*, *McpToolGuide*, *McpToolsListBudget*, *Census*, *Ratchet*, *LiteMuteAnalysisFinding*, *McpWritePolicy*, *CrossAppMcp*, *McpReadToolBudget*, *QueryTag*, *Route*, *AnalysisTools*: Total 463, Failed 0. Both test projects build with 0 warnings. Not run: the full suite and the pre-push gate (laptop override; CI decides).

Status of #6403

With groups A, B, C and the server-admin group, every tool in the issue's list answers with previous and undo (or undo: null with an undo_note where no single call restores the state). Lite's route and query-tag tools and Lite's analysis mutes were answered by group C.

Round 1 review

• remove_server description and body said previous has no password: Fixed (aaedf50). The description now says a SQL or service-principal row carries the placeholder <password> in previous and the undo, and the AWS external ID is left out. Kept the placeholder in previous (same projection as the undo, one source) rather than splitting them.
• No copyright header on DarlingMcpUndoShape.cs: Fixed (aaedf50).
• Custom alert rule update: the undo of a revive carried a past expires_at_utc, which the tool refuses, dropping every other restored field too: Fixed (aaedf50). A passed expiry is restored as "" with an undo_note. CustomAlertRuleExpiryLiveTests now replays the undo (fails on the old code, revert-proved) instead of asserting its spelling.
• edit_server undo refused for legacy rows (port saved before A SQL Server entry's port is ignored without a warning, and a failed connect reports that port #6583, host saved before Server host field: refuse named-pipe and admin protocol prefixes and UNC paths #6352): Fixed (aaedf50). undo_note now says the undo will be refused with nothing written, naming the value. Test EditServer_ClearingAPortSavedBeforeTheRule_... seeds a row with port 1444, clears it, checks the note and that the replay answers invalid and writes nothing (revert-proved). The host case shares the same code path and is not separately tested.
• RouteRecreateUndo landed inside RoutePayload's doc comment: Fixed (aaedf50), the summary is back on RoutePayload.
• DarlingAlertReader overload is READ COMMITTED, not one snapshot: Fixed as a comment (aaedf50). The remark says why it is correct (the lock loop covers the tables the write targets, and previous only projects fields from updates) and when it stops being true.
• undo as a bare "None needed: ..." string beside the {tool, arguments} object: Not changed. AlertTools, CollectionStateTools and McpTools already answer this way on dev and existing tests assert it; moving it to null plus undo_note is a cross-tool contract change for its own PR.
• Route undo carries a third key, destinations: Not changed, documented in its summary; the webhook secrets cannot be placed in arguments.
• UNVERIFIED test totals, "5 tag descriptions under 600", "list is not in the tree": the totals are below; the 600 line in the body is corrected above; Darling MCP: older write tools return no previous value and no undo call #6403's list is on the issue.
• Reviewer checklist: the live facts ran against a throwaway PostgreSQL 55614 (not skipped). Not run: FOR UPDATE as the mcp login on config_notification is exercised by ToolUndoAnswersLiveTests through the managed role (passed); the web PATCH browser check and the excluded_databases/excluded_volumes replay were not done.

Test run (rig on a throwaway PostgreSQL): CustomAlertRuleExpiryLiveTests, ServerEditLiveTests, ToolUndoAnswersLiveTests, McpServerTagUndoLiveTests: 55 run, 0 failed, 2 skipped (real SQL Server). Plus every *Census* and *Ratchet* class, McpToolsListBudget, DarlingMcpAlertToolsTests and McpServerAdminTools classes: 615 run, 0 failed, 2 skipped. Build 0 Error(s). Full suite not run; CI decides.

Round 2 review

Claude posting for Erik Darling.

1. Expiry undo on an unchanged resend. The passed branch in DarlingMcpCustomAlertTools.UpdateAnswer now applies only when this call actually changed the expiry (after.ExpiresAtUtc != before.ExpiresAtUtc) and the old one had passed. An update that resends the rule's own passed expiry unchanged (while changing another field) now gets an undo that sends the same expiry back, which the tool accepts because it equals the current value, and carries no "already passed" note. New live case CustomAlertRuleExpiryLiveTests.UpdateUndo_OnAnUnchangedResendOfAPassedExpiry_RestoresThatSameExpiry_WithNoNote: a rule aged to a past expiry, an update resending it and changing the description, then the undo replays without "invalid" and the expiry is unchanged. Revert-proved: with the old line the test fails on Assert.Null(undo_note); with the fix it passes.
2. CI failure. FleetIdentifierScrubTests.NoTrackedSourceFileNamesAServerByShortNameAndOrdinal: in ServerEditLiveTests.Undo.cs the seeded names undo-01/02/03/04/05/09/10 are now zeta-NN (zeta is on the synthetic list) and blob-02/blob-04 are rem-blob-b2/rem-blob-b4.

Ran on a throwaway PostgreSQL (stopped after): CustomAlertRuleExpiryLiveTests, FleetIdentifierScrubTests, every *ServerEdit* class, DarlingMcpCustomAlertToolsTests: Total 580, Failed 0, Skipped 2 (the two ServerEditLiveTests cases that need a real SQL Server). Not run: the full suite (CI decides).

[claude]

previous does carry a password key, so "no password" is wrong here (and in the PR body).

previous and the undo's one entry are the same projection — RemovedPreviousOf(row) is called twice (lines 868 and 869–872) — and that projection sets entry["password"] = PasswordPlaceholder for a secret-auth row (line 775). The new test pins it: ServerEditLiveTests.Undo.cs asserts Assert.Equal(Edit.PasswordPlaceholder, (string)previous["password"]!).

No secret leaks (the stored blob is unreadable and Assert.DoesNotContain("blob-04", …) proves it), but an agent reading this description does not expect a password key under previous at all, and undo_note explains the placeholder only as something in the undo. Either say so here, or drop password from previous and keep it in the undo's servers_json alone.

Suggested change

"display_name, matched_by:\"exact\"|\"partial\", matched_in:\"config_monitored_servers\", ever_connected, previous (the removed definition, no password, no AWS external ID), undo (the add_servers call that re-adds it, with a password placeholder), undo_note (what a re-add does not bring back)} on " +

"display_name, matched_by:\"exact\"|\"partial\", matched_in:\"config_monitored_servers\", ever_connected, previous (the removed definition in the keys add_servers takes; the stored password cannot be read back, so a SQL or service-principal row carries the placeholder <password> in its place, and the AWS external ID is left out), undo (the add_servers call that re-adds it, with that same password placeholder), undo_note (what a re-add does not bring back)} on " +

[claude]

New file with no copyright header. CONTRIBUTING.md:450 has it on the PR checklist ("New files have copyright headers"), and the four new test files in this PR all carry the block. 189 of the 193 files in this folder have it; this is one of the four that do not.

Suggested change

	using System.Text.Json.Nodes;
	/*
	* Copyright (c) 2026 Erik Darling, Darling Data LLC
	*
	* This file is part of the SQL Server Performance Monitor.
	*
	* Licensed under the MIT License. See LICENSE file in the project root for full license information.
	*/
	using System.Text.Json.Nodes;

[claude]

Non-blocking, and the body already names it ("Known limit: undoing an update that cleared an expiry which had already passed is refused by the tool"). Two things the limit's phrasing understates, worth one line of code or one line of note:

1. The refusal is all-or-nothing. ResolveUpdateExpiry refuses any past value that is not equal to the rule's current expiry (line 905), and the tool returns {status:"invalid"} with nothing written — so the undo of a revive loses name, description and enabled too, not just the expiry. CustomAlertRuleExpiryLiveTests proves the refusal four lines above the assertion it adds: movedBack with UtcNow.AddHours(-2) → "invalid", then revived["undo"] carries exactly such an instant. Nothing replays that undo, so it stays green.

2. DeleteAnswer already solved this shape (lines 2040–2049): a past expiry is left out of the create arguments and undo_note says so. The same two lines here would make undo a call that works:

Suggested change

	arguments["expires_at_utc"] = CustomAlertExpiry.Format(before.ExpiresAtUtc) ?? string.Empty;
	arguments["expires_at_utc"] = before.ExpiresAtUtc is { } was && was <= DateTime.UtcNow
	? string.Empty
	: CustomAlertExpiry.Format(before.ExpiresAtUtc) ?? string.Empty;

(with a sentence in undo_note saying the expiry had already passed, so the undo leaves the rule with none — otherwise the undo silently differs from previous.)

[claude]

Non-blocking — the same "the undo is a call the tool refuses" shape as the custom-alert-rule expiry, but two cases here that the body does not list, both legacy-row only:

• port. PlanEdit refuses c.Port is > 0 on a SQL Server row (line 1100, A SQL Server entry's port is ignored without a warning, and a failed connect reports that port #6583). A row saved before that rule with a stored port, edited to port: 0 to clear it, puts port in ChangedFields; the undo then sends the old non-zero port and is refused with nothing written.
• host. ServerHostSetting.Refusal is applied to a changed host (line 1087, Server host field: refuse named-pipe and admin protocol prefixes and UNC paths #6352) but deliberately not to a host sent unchanged. A row saved before that rule on a named-pipe/admin-style host, edited onto a normal host, gets an undo carrying the old illegal host — also refused.

In both the undo is refused cleanly (invalid, nothing written), so no data is at risk; the cost is that undo reads as a call that works and undo_note is silent. EditServer_AnswersWithPreviousAndAnUndo_… replays the undo but only over display_name/monthly_cost_usd/host/trust_server_certificate on a legal host, so neither case is covered. A sentence in undo_note when plan.ChangedFields holds port or host and the previous value would not pass the add rule would close it.

[claude]

Non-blocking: the new method landed inside RoutePayload's doc comment. RouteRecreateUndo now carries two <summary> elements — the one directly above this line ("The ONE Darling route projection (#5750): get_notification_routes lists it and set_notification_route_enabled returns it…") documents RoutePayload, which now sits at line 978 with no doc at all. Moving that first block down to line 978 restores both.

Also here, for the record: this undo is {tool, arguments, destinations}, not the {tool, arguments} shape DarlingMcpUndoShape.Call exists to make uniform (the update_notification_route "updated" answer does the same at RouteAdmin.cs:347–354, while its new unchanged answer uses the bare two-key form). Deliberate and documented in the summary — but a client that reads undo by shape now sees three variants across this PR: the {tool, arguments} object, null beside an undo_note, and a bare "None needed: …" string (assign_server_tag, unassign_server_tag, unassign_query_tag, mute_analysis_finding, the edge move). The null + undo_note form already covers "no call undoes this"; the string in the same slot is the odd one out, and answer.undo.tool throws on it.

[claude]

Non-blocking, but worth a sentence in this summary so the next reader does not have to re-derive it: unlike the entry point above, this overload runs on the caller's transaction, which update_alert_settings opens as plain READ COMMITTED — so the two single-table reads are not one snapshot, which is the property TheTwoConfigTables_AreReadUnderOneRepeatableReadSnapshot names. The PR body's "its rule … holds" is true of the rule the test now enforces (every path goes through the combined entry point), not of the snapshot half.

It is still correct, for a reason that is not obvious from here: the lock loop only takes FOR UPDATE on the tables the write targets, and AlertSettingsPreviousFields only ever projects fields drawn from updates. So a field that could be read stale (the other table's, when the write does not touch it) is never one that reaches previous. If a later change ever lets previous carry a field from a table this write did not lock, that stops being true silently.

[claude]

Verdict: CHANGES REQUESTED

Claims

• VERIFIED — edit_server's previous holds the changed fields and undo carries the new expected_modified_at, so a later change makes the replay a conflict — Edit.cs:541, :568-624, key parsed at :968; replayed in ServerEditLiveTests.Undo.cs. A password-only edit answers undo: null (:607-610).
• VERIFIED — add_servers: only an added row carries the remove_server undo — DarlingMcpServerAdminTools.cs:360 is the one site passing entry.StorageKey; :1940-1947. The edit core is shared with the web route, which now gets the three keys (DarlingWebEndpoints.cs:2139).
• VERIFIED — update_alert_settings' previous is the sent fields in the nested read shape, the cooldown alias maps to delivery.cooldown_minutes, a file/default route comes back null — DarlingMcpAlertTools.cs:2141, :2147-2151; every UpdateTarget.Field is the dotted wire path and matches BuildAlertSettingsPayload's keys.
• VERIFIED — the pre-write read now runs in the write transaction, after FOR UPDATE in the writes' own table order, through a new (connection, transaction) overload whose single-table reads stay private — :1275-1300; DarlingAlertReader.cs:736-743.
• VERIFIED — "nothing can fail that did not before" for that lock under the mcp role — FOR UPDATE needs SELECT on the projected column plus UPDATE on one column: DarlingManagedRoles.cs:1665/:1668 grant column-level UPDATE on config_notification, and id is in its non-secret read carve (:230).
• VERIFIED — Lite/Darling parity holds: Lite's write-tool roster is exactly the twelve tools changed here, and Darling's unmute_analysis_finding already carried both fields (DarlingMcpTools.cs:1508-1529), so Lite is catching up, not diverging. Lite's UnmuteStoryAsync returns the removed rows and its one caller is the tool (FindingStore.cs:673, McpAnalysisTools.cs:1173).
• VERIFIED — merge_deadlock_signatures answers previous, undo: null, undo_note, asserted in SignatureMergeLiveTests.MergedSignatures_ShareCounts (:107); delete_mute_rule's undo uses create's own parameter spellings while previous uses get_mute_rules' (:2194-2218 vs AgentChangeImages.cs:61-76).
• UNVERIFIED — every test-run total and the build/warning counts — needs a build and a live rig; each new fact is Assert.SkipWhen(DARLING_TEST_PG is empty).
• UNVERIFIED — "the five server-tag descriptions stay under the 600-character cap" — 600 is a target, not a gate: McpToolsListBudgetTests.cs:44 enforces 1,000 and only on tools carrying the <<GUIDE>> marker (:56-68); none of the five has one, so nothing measures them.
• UNVERIFIED — "every tool in #6403's list answers with previous and undo" (the list is not in the tree), and the two negative controls ("reverted the expected_modified_at … the test failed").
• REFUTED — "remove_server: previous is the removed definition … (no password, no external ID)" — previous and the undo entry are the same projection (:868 and :869-872 both call RemovedPreviousOf), which sets entry["password"] = PasswordPlaceholder for a secret-auth row (:775); the PR's own test asserts previous["password"] == "<password>", and the shipped description repeats the wording at :429. No secret leaks, but a password key an agent is told is absent is one it can misread. Suggestion inline.

Riskiest lines

1. DarlingMcpCustomAlertTools.cs:991 — the undo's expires_at_utc is the previous expiry verbatim, and ResolveUpdateExpiry refuses a past value that is not the rule's current one (:905), so the undo of a revive is refused with nothing written, losing the name/description/enabled restoration too; DeleteAnswer already handles this (:2040-2049). — UNCOVERED (the replay test only goes the clear-to-"" direction)
2. DarlingMcpAlertTools.cs:1289 — the FOR UPDATE that makes previous what the write replaced; without it, or out of WritableTables order, a concurrent write lands between read and UPDATE again — covered by ToolUndoAnswersLiveTests.UpdateAlertSettings_PreviousIsReadUnderTheRowLock_SoAConcurrentWriteIsNotMissed
3. DarlingMcpServerAdminTools.Edit.cs:618 — expected_modified_at on the undo's changes_json; without it the replay overwrites a later edit — covered by EditServer_AnswersWithPreviousAndAnUndo_ThatPutsTheChangedFieldsBack_AndASecondReplayIsAConflict

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 — every Lite write tool with a Darling twin changes here (Lite/Mcp/McpQueryTagTools.cs, McpAlertTools.cs:663-751, McpAnalysisTools.cs:1122-1200). The residual text_tag pattern asymmetry is pre-existing in each SKU's RuleNode (McpQueryTagTools.cs:1394 vs DarlingMcpQueryTagRuleTools.cs:656) and each new previous mirrors its own SKU.
• "A test that encodes the lie … Ask what defect a test would fail ON." — applies — CustomAlertRuleExpiryLiveTests.cs:104 now asserts the undo's spelling (undo["tool"]); four lines above, the same test proves a past expires_at_utc answers "invalid", which is what that undo carries. Replaying it is the assertion that fails on the defect.
• "Ruling (#5833): tests are load-bearing … never answer a trap with a test that reads source text" — does not apply as a finding — no new source-reading pin; every new fact replays the undo through the real tool. The two pins amended (FindingStoreTests.cs:730-731, DarlingMcpAlertToolsTests.cs:122-123) pre-date this PR and only follow a renamed signature and a moved count.
• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies weakly — {tool, arguments} is written four times (DarlingMcpUndoShape.Call, DarlingMcpAlertTools.cs:2571, Lite/Mcp/McpAlertTools.cs:816, Lite/Mcp/McpQueryTagTools.cs:1159); two are a real project boundary, the payload is two keys, and the replay tests pin the shape.
• "A JSON number above 2^53 is rounded by every JavaScript client (#3487)" — does not apply — the undo arguments carry low-range route/tag/server/rule ids, and a mute rule's id is already a string on the wire (DarlingMcpAlertTools.cs:1123).
• "hours_back, limit, top, days_back are refused when out of range, never clamped (#3699)" — does not apply — no window or page argument is touched.
• "A default interface member compiles a parity gap away (#3640)" — does not apply — the one new type is a non-virtual static helper (DarlingMcpUndoShape.cs:11); no store interface gained a member.

Divergence (blind read vs PR body)

Three places, none in the write paths:

1. previous carries a password key — REFUTED above; the body and the shipped remove_server description both say "no password". One-word fix, suggestion inline.
2. A new file with no copyright header — Mcp/DarlingMcpUndoShape.cs starts at its using. CONTRIBUTING.md:450 puts "New files have copyright headers" on the PR checklist, and the four new test files here all carry the block.
3. The "one snapshot" half of the invariant — the body says the test's rule "holds". The rule it now enforces does; the new overload runs on the caller's plain READ COMMITTED transaction, so its two reads are not one snapshot. Still correct, for a reason the code does not state: the lock loop only locks the tables the write targets and AlertSettingsPreviousFields only projects fields drawn from updates. Noted on DarlingAlertReader.cs:736, because that stops being true silently if previous widens.
   The rest lines up, including the two limits the body volunteers against itself. Two more "the undo is a call the tool refuses" cases the body does not list are inline on Edit.cs:618 (port and host on rows saved before #6583 / #6352): legacy-row only, refused cleanly with nothing written.

Reviewer checklist

• Run the new live facts against a real store — all are Assert.SkipWhen on DARLING_TEST_PG, so a leg without a rig reports skipped, not green.
• Confirm SELECT id FROM config_notification WHERE id = 1 FOR UPDATE succeeds as the mcp login. The grants read correctly, and DarlingManagedRoles.cs:1657 records that this table's privileges were measured rather than assumed last time for exactly this reason (a missing SELECT and a missing UPDATE raise the same 42501).
• Replay update_alert_settings' undo for the shapes no test covers: excluded_databases / excluded_volumes (back through VolumeExclusions.Refusal, which a list stored before #6098's bounds could fail) and analysis.uncorroborated_route where the file is deciding.
• Look at the web PATCH /api/servers/{id} response in a browser: it now carries previous, undo and undo_note from the shared core.

[claude]

passed only looks at before.ExpiresAtUtc, not at whether this call actually changed it. ResolveUpdateExpiry (line ~900) exempts a resend that equals the row's current expiry from the past-refusal (if (parsed == current) return (parsed, null);), specifically so "re-saving an already-expired rule still works."

So: if a caller resends the rule's own already-passed expires_at_utc unchanged (e.g. while also changing description), before.ExpiresAtUtc == after.ExpiresAtUtc, both past, and the real update_custom_alert_rule call would accept that exact resend (no refusal, no-op on that field). But this code still sets passed = true and builds an undo that clears the expiry to "" with undo_note claiming the undo "will be refused" — which is wrong here: replaying the real previous value would have succeeded.

The fix needs before.ExpiresAtUtc != after.ExpiresAtUtc (or equivalently, that this call actually moved/cleared the expiry) in the passed condition, not just "was it in the past." CustomAlertRuleExpiryLiveTests's new assertions only exercise the case where the forward call clears the expiry (before=past, after=null), so this unchanged-resend path isn't covered.

[claude]

Verdict: CHANGES REQUESTED

Claims

• VERIFIED — "Custom alert rule update: ... Fixed (aaedf50). A passed expiry is restored as "" with an undo_note." — DarlingMcpCustomAlertTools.cs:989-998 adds the passed branch; CustomAlertRuleExpiryLiveTests.cs:258-266 replays the undo and checks undo_note contains "already passed" and expires_at_utc is "", which fails against the pre-fix unconditional CustomAlertExpiry.Format(before.ExpiresAtUtc) line.
• VERIFIED — "remove_server description and body said previous has no password: Fixed." — DarlingMcpServerAdminTools.cs:429 now says the placeholder is carried; confirmed against RemovedPreviousOf (DarlingMcpServerAdminTools.cs:754-776), which does set entry["password"] = PasswordPlaceholder for secret auth.
• VERIFIED — "No copyright header on DarlingMcpUndoShape.cs: Fixed." — header added matches the standard block used by sibling files in the same directory (e.g. DarlingMcpAlertTools.cs:1-7).
• VERIFIED — "RouteRecreateUndo landed inside RoutePayload's doc comment: Fixed... the summary is back on RoutePayload." — diff moves the <summary>The ONE Darling route projection...</summary> block from above DeleteNotificationRouteUndo to above RoutePayload (DarlingMcpAlertTools.cs).
• VERIFIED — "DarlingAlertReader overload is READ COMMITTED, not one snapshot: Fixed as a comment... the lock loop covers the tables the write targets." — the caller (DarlingMcpAlertTools.cs:1289 FOR UPDATE loop, line 1295 calling the transaction overload) does lock both tables before the read the new comment describes.
• VERIFIED — "edit_server undo refused for legacy rows (port..., host...): Fixed... The host case shares the same code path and is not separately tested." — confirmed in DarlingMcpServerAdminTools.Edit.cs:607-627: the host branch (line 617-620) is structurally identical to the tested port branch (line 612-615) and reuses the well-tested ServerHostSetting.Refusal; only the port path has a new live test.
• UNVERIFIED — the various test-run totals ("Total 55 run, 0 failed, 2 skipped", etc.) — these are live-rig results from the author's machine, not reproducible from a read-only tree.

Riskiest lines

1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpCustomAlertTools.cs:992-994 — the passed check tests only "is before.ExpiresAtUtc in the past," not whether this call actually changed it, so a caller resending the rule's own already-passed expires_at_utc unchanged (alongside another field change) gets an undo that wrongly clears the expiry and an undo_note claiming a refusal that would not actually happen (ResolveUpdateExpiry exempts a resend equal to the current value from the past check). See inline comment. UNCOVERED — CustomAlertRuleExpiryLiveTests only exercises the "forward call clears the expiry" case, not the "forward call resends the unchanged past expiry" case.
2. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.Edit.cs:617-620 — the host-refusal branch of the new undo-note logic. UNCOVERED by a behaviour test (author discloses this in the PR body: "not separately tested"), but low risk since it mirrors the tested port branch and calls the already-well-tested ServerHostSetting.Refusal.
3. Darling/Darling.Tests/CustomAlertRuleExpiryLiveTests.cs:263-266 — the replay assertion (Assert.NotEqual("invalid", ...)) is the one line that actually revert-proves the main fix; it is the test that fails on the old unconditional-restore code. Covered by itself (it's the test) — not a gap, listed to show the fix's one exercised path.

Traps near this diff

• "Ruling (#5833): tests are load-bearing... A trap is answered with a behaviour test that fails on the defect and passes on the fix, or with one sentence in the PR body saying why none is needed." — applies, and is satisfied: both new live tests (CustomAlertRuleExpiryLiveTests, ServerEditLiveTests.Undo.cs) replay the real undo through the real tool and assert on outcome/state, not on source spelling.
• "Lite and Darling ship in parity... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding." — does not apply: this commit's fixes are to Darling-only features (multi-server edit_server/remove_server, custom alert rule expiry) that have no Lite equivalent (Lite has no server-admin tools and no custom alert rules; grep confirms no CustomAlertRule or UndoShape under Lite/).
• "A test that reads the clock twice measures the CI scheduler" — does not apply: the new tests anchor their past timestamps off a single DB-side now() call (CustomAlertRuleExpiryLiveTests.cs:227) or a fixed literal (ServerEditLiveTests.Undo.cs port 1444), not two independent clock reads.

Divergence (blind read vs PR body)

The PR body's "Round 1 review" section describes the expiry fix only as handling "the undo of a revive carried a past expires_at_utc" (i.e., the forward call that clears an expired rule's expiry). The actual code change is broader: it treats ANY update call that resends an expires_at_utc where before.ExpiresAtUtc is in the past as needing the clear-and-note treatment, regardless of whether the field actually changed. That broader condition introduces the unchanged-resend false positive described above and in the inline comment — a case the body doesn't mention and the test doesn't cover.

Reviewer checklist

• Confirm with a live rig whether resending a rule's own already-passed expires_at_utc unchanged (with another field also changed) produces an undo that incorrectly clears the expiry, per the inline comment on DarlingMcpCustomAlertTools.cs:994.

Claude posting for Erik Darling. Stale and capped at 2 review rounds: both round-2 findings (the expiry undo on an unchanged resend, and the scrub names) are fixed in cd775d1, with a live test for the expiry case.