add_servers reports every entry after a save fault, refuses a literal password on Linux, and never answers added for a save that wrote nothing (#4734)

[erikdarlingdata]

Part of #4734 (items 3 and 4: add_servers results)

Why

Item 3. add_servers saves its entries one at a time, and one catch wrapped the whole loop. When entry N's save threw, the call answered a single error and dropped the list that recorded entries 1 to N-1 as added. Those servers were saved and being collected, but the caller was told the call failed, and a retry then reported them as duplicates. One trigger always reproduced it on Linux: a literal SQL password is encrypted with Windows DPAPI, ParseRequest did not check the platform, and the throw came after the probe, in the middle of the batch.

Item 4. server_id is a 32-bit hash of the storage key, and the insert ends in ON CONFLICT (server_id) DO NOTHING. Two different identities that hash to one id, or two concurrent adds of the same server, write nothing, and the tool still answered added for a server nothing would collect.

What changes

All in Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs, except the docs in the last item.

• Catch per entry. The password encryption, the insert and the row-count follow-up sit in a per-entry try. On a fault that entry is reported as not_saved with the fault's message, and every remaining entry is reported as not_saved without a probe (a probe waits out a connect timeout). Entries saved before the fault stay added. Every entry gets a result, so the four counters still sum to requested. The outer catch stays for faults before any entry is attempted (reading the existing servers).
• New status not_saved, counted under failed. The existing statuses did not fit: connection_failed is a probe failure, invalid is a structural one. Added to AddStatus, CounterOfStatus and both places in the tool description that list the statuses.
• Literal secret off Windows. New pure LiteralSecretRefusal(secret, isWindows, isServicePrincipal). ParseRequest calls it for SQL passwords and service-principal client secrets, so the entry is invalid before any probe or write, with a message that names the env:NAME or file:/run/secrets/<name> reference form (the form the README gives for every secret slot). References pass on every platform. ProtectPasswordForStorage keeps its throw as the backstop for other callers.
• A write that changed no rows. InsertServerAsync now returns the rows changed. On 0 the row holding the server_id is read back (ServerByIdSql, the same five identity columns the duplicate gate reads) and the pure ClassifyUnwrittenInsert answers: a different storage key gives collides, the same key (compared case-folded, like the gate) gives duplicate, and no row gives not_saved. It is never added.
• Seams for the tests. The three table operations sit behind an internal IServerDefinitions (the real one wraps the existing SQL), AddServersAsync has an overload that takes it, and ParseRequest has an overload that names the platform. The public tool method and the existing AddServersAsync(NpgsqlDataSource, ...) signature are unchanged.
• Tool description and docs updated for the new status, the id collision case and the Linux rule. The tool description is in the file above. The docs are the add_servers paragraph in Darling/README.md: it now names the not_saved and collides statuses and the off-Windows rule, and it no longer says Entra auth is invalid (ServicePrincipal and ManagedIdentity are supported, Darling: non-interactive Entra auth (Service Principal + Managed Identity) for headless targets #3484; only the interactive Entra modes are invalid).

Test plan

• Darling.Tests builds with 0 warnings, 0 errors.
• The first commit adds the tests and the seams with the old behavior in place: 11 of the new or updated tests fail there, each for the intended reason (one error instead of a result per entry, a literal password accepted off Windows, a write that changed no rows answered added, the census pin and the description pin). The second commit makes them pass.
• DarlingMcpServerAdminToolsSurfaceTests, DarlingMcpServerAdminToolsLivePostgresTests, AddServerVerbTests, DarlingConfigTests, PostgresTargetConfigTests, RegistrationCollisionTests, McpPayloadContractCensusTests: 215 tests, 0 failed, 1 skipped (the gated live test).
• McpToolsListBudgetTests, DocCommentHygieneTests, DarlingMcpInstructionsBudgetTests, LiveCleanupConversionRatchetTests, LivePostgresCollectionHygieneTests: 0 failed.
• Full Darling.Tests suite once, without DARLING_TEST_PG, after merging origin/dev: 17256 total, 0 failed, 1134 skipped (gated live tests), 1 not run.
• New tests: a save fault on entry 1, 2 or 3 of 3 (1 added and 2 failed for entry 2; the entries after the fault are not probed, the faulting entry was); the literal-secret check on both platforms' answers; ParseRequest with the platform named, and with the real platform (asserts the Windows or the non-Windows arm to match the machine); a write that changes nothing because a different identity holds the id (two real server names whose keys hash to one id, asserted in the test) answers collides and the rest of the batch continues; the same identity added concurrently answers duplicate; no row holding the id answers not_saved.
• The two existing tests that pass a literal password through ParseRequest now name the platform (isWindows: true), since they test what the probe config carries.
• Gated live class against a PostgreSQL (DARLING_TEST_PG): not run here. It gains one test that adds an identity, adds a second identity with the same server_id against the real table, and checks collides and that the first row is untouched. CI's PostgreSQL job runs it.
• A run on Linux: only the Windows arm of the platform-conditional test ran here. The isWindows: false tests cover the Linux answer on any machine.

Worth a second look

• not_saved is a new word on the wire. Callers that switch on status see it only in the fault and no-row cases; the counters and the sum rule are unchanged.
• The entry whose save faulted was probed (that is how it reached the save). Only the entries after it skip the probe.
• OperationCanceledException is not treated as a save fault; it still reaches the outer catch as before.
• The --add-server summary line still counts added, skipped and failed only, so a collided entry is in the per-server lines but not in the totals line. That predates this change and is left alone.
• Seeding the definitions on first start uses the same DO NOTHING insert and is not changed here.

CHANGELOG

SECTION: Fixed
ENTRY:

• add_servers reports every server in the batch when a later entry fails to save, and no longer says "added" for a save that wrote nothing ([Darling MCP: mute_analysis_finding picks the first partial match, create_mute_rule duplicates on retry, add_servers misreports, store growth hides gaps #4734]) - add_servers now reports every server in the batch when a later entry fails to save, instead of reporting the whole call as failed after it had already added the earlier ones: the failed entry and the ones after it come back as the new status not_saved (counted as failed) and the earlier ones stay added. It rejects a literal SQL password or service principal client secret on Linux, where it cannot be encrypted, and points at the env: or file: reference form. When the save wrote nothing because another server holds the same id, it answers collides (or duplicate for a concurrent add of the same server) instead of "added".
  REF:
  [Darling MCP: mute_analysis_finding picks the first partial match, create_mute_rule duplicates on retry, add_servers misreports, store growth hides gaps #4734]: Darling MCP: mute_analysis_finding picks the first partial match, create_mute_rule duplicates on retry, add_servers misreports, store growth hides gaps #4734