Skip to content

A refused plan-tool dry run says the server's switch state; custom alert rule scope documented (part 1 of #6455) - #6617

Merged
erikdarlingdata merged 6 commits into
devfrom
fix/6455-mcp-write-wording
Oct 24, 2026
Merged

erikdarlingdata merged 6 commits into
devfrom
fix/6455-mcp-write-wording

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 23, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Refs #6455 (all seven items were fixed on dev by #6509).

Follow-up to #6509 on items 3 and 4: the dry-run refusal now also states the server's action switch, and update_custom_alert_rule plus the docs state the rule scope. Item 7 needed nothing more (#6509 added RefuseMissingRequired). No second part.

Why

Dev already carries most of this (#6509 merged earlier): the plan tools' dry-run refusal explains why a dry run needs the claim, create_custom_alert_rule states its scope, and the shared McpUnknownArgumentGuard names a missing required argument. What was still open against the issue's "Expected" lines is listed below.

What changes, per item

Item 7 (missing required argument): nothing to change. PerformanceMonitor.Common/Mcp/McpUnknownArgumentGuard.cs already has RefuseMissingRequired, called from the same call filter the unknown-argument guard is registered as, for every tool, in Darling and in Lite (both register McpUnknownArgumentGuard.Instance). It reads the tool's advertised required list and answers Missing required argument 'version' for tool '...'. The brief asked for a sibling filter; the existing arm in the one shared filter does the same job, so adding a second filter would only run the same check twice. Its tests are in McpUnknownArgumentGuardTests (around the RefuseMissingRequired calls: refused with the name, passes when present). I made no item 7 change, so there is no revert-proof to run.

Item 3 (dry run refused as agent_action_required). Gate order is unchanged. The refusal text already said why a dry run needs the claim; what was missing was the per-server switch state. For a dry run refused as agent_action_required, the four plan tools (force/unforce, evict, kill, kill on PostgreSQL) now read the switch with the existing ServerActionSwitches.ListAsync (no claim needed) and append one sentence: For reference, <action> on <server> is currently on. or ... currently off, so the real call would be refused as server_action_off until it is turned on. (the code is built from AgentActionCodes.SwitchOff; actions_off is the service-wide switch). A real call, and any other refusal reason, keeps its old text. If the switch read throws, the note is left out and the refusal still goes out (logged as a warning).

Item 4 (custom alert rule scope). The code does offer a way: the rule definition's scope (mode all, servers with a non-empty servers list, or tag with a fleet-tag tagId); with no scope the rule applies to every monitored server. create_custom_alert_rule's description already said so (#6509). Added: the same statement in update_custom_alert_rule's description (and that a new definition replaces the old one whole, so the scope must be included to keep it), a row in docs/mcp-server.md, and a paragraph in Darling/README.md under "Level 2: agents change mute rules and custom alert rules". Only description strings were touched in DarlingMcpCustomAlertTools.cs.

Bot review rounds 1-2 (changes requested)

  • Item 1, wrong code (blocking). The off-state sentence said the real call would be refused as actions_off; the per-server gate answers server_action_off (AgentActionCodes.SwitchOff). The sentence now takes the code from that constant, so it cannot drift. The other dry-run sentences this PR added name no code.
  • Item 2, the test pinning the wrong word. ServerActionSwitchWriteLiveTests now asserts server_action_off (and that actions_off is absent). TheGate_ReadsTheNewState... also asserts the code the dry-run note predicts equals the Reason the real gate call returned for the same server. The wording test's sample note uses the constant too.
  • Item 3, failed switch read. The code already fell back to the bare refusal. New behaviour test ADryRunWhoseSwitchReadFails_FallsBackToTheBareRefusal_NotAnError points the read at a data source nothing listens on (port 1): the note is null and the refusal is still refused / agent_action_required with no "For reference". It exercises the read helper and the answer builder, not the full tool path.
  • Item 4, read width. Left as is. The store has no read of one server's switch rows; ServerActionSwitches.ListAsync is the only public read (the gate's single-switch SQL is internal and needs an open transaction), and the brief allowed no new store method. The registry is small and the read runs only for a dry run refused for want of a claim.

Test plan

  • dotnet build Darling/Darling.Tests : 0 errors; the 6 warnings are in files this PR does not touch.
  • Classes McpWriteToolWordingTests, ServerActionSwitchWriteLiveTests, McpUnknownArgumentGuardTests, PlanForceTool*, McpInstructions*, PayloadContract*, every *Census* and *Ratchet*: Total 654, Failed 0, Skipped 68 (the PlanForceToolLiveTests set skips without DARLING_TEST_SQL). Run against my own throwaway PostgreSQL (stopped after).
  • New: McpWriteToolWordingTests.ADryRunRefusedForWantOfTheClaim_AddsTheSwitchState_AndAWriteOrAnotherReasonDoesNot (the answer for a dry run vs a write vs another reason); ServerActionSwitchWriteLiveTests.ADryRunRefusedForWantOfTheClaim_ReadsTheSwitchWithoutOne_AndSaysOnOrOff (live: off, then on after SetAsync, and no note for an unknown server). Both ran and passed.
  • New through-the-tool tests PlanForceToolLiveTests.ADryRunWithNoAgentClaim_IsRefusedAsRequired_AndSaysWhyAndThatTheSwitchIsOff / ...IsOn: need a test SQL Server (DARLING_TEST_SQL), which I did not set, so they skipped here. CI decides.
  • Full suite and pre-push.sh not run (brief: CI decides).

Considered, not added

  • Lite has no plan-force tools and no custom alert rules, so there is no parity change.
  • No new filter for item 7 (see above).

CHANGELOG entry

SECTION: Fixed
ENTRY:

Dev merge

Merged origin/dev. One conflict, the UpdateCustomAlertRule tool description in DarlingMcpCustomAlertTools.cs: dev added the previous/undo sentence to the same line this PR extends with the server-scope wording. Kept both, in that order. Dev's CustomAlertRuleStore construction is unchanged (no deadline argument on dev's form) and nothing in the file needed to change for it.

Merging dev conflicted in docs/mcp-server.md only: this PR's new Custom alert rules row and dev's rewritten Alert tuning row (adds update_notification_channels) sat on adjacent lines. Kept both, dev's row unchanged. This PR touched no tool count, so none were recounted.

@erikdarlingdata
erikdarlingdata enabled auto-merge (squash) October 23, 2026 16:45
@erikdarlingdata
erikdarlingdata marked this pull request as draft October 23, 2026 16:59
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 23, 2026 22:35
@erikdarlingdata
erikdarlingdata enabled auto-merge (squash) October 23, 2026 22:35
Comment on lines +190 to +191
return $"For reference, {ServerActionNames.Label(serverAction)} on {serverName} is currently {(state.Enabled ? "on" : "off")}"
+ (state.Enabled ? "." : ", so the real call would be refused as actions_off until it is turned on.");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The note names the wrong refusal code. A per-server switch that is off is refused as server_action_off, not actions_off:

  • AgentActionCodes.SwitchOff = "server_action_off" — "The server-action gate: that server's switch for that action is off, or has no row (no row means off)" (PerformanceMonitor.Common/Investigations/InvestigationContracts.cs:403).
  • AgentActionCodes.ActionsOff = "actions_off" is a different condition: "config_service.agent_actions_enabled is off" (same file, line 382) — the service-wide knob, checked first.
  • The gate returns AgentActionCodes.SwitchOff for exactly this case (InvestigationRequestStore.ServerAction.cs:193), and the tree's own behaviour test pins it: PlanForceToolLiveTests.WithoutTheServersSwitch_TheCallIsRefused_Journaled_AndWritesNothing() => AssertRefusedAsync(Without.Switch, "server_action_off") (PlanForceToolLiveTests.cs:403), with the same word for evict (PlanForceToolLiveTests.Evict.cs:288) and kill (PlanForceToolLiveTests.Kill.cs:467, PostgresKillLiveTests.cs:362).

The whole point of #6455 item 3 is that the refusal should not mislead the agent about what the real call would do. As written it does: the agent is told the call would come back actions_off, and the two codes have different remedies — actions_off is cleared by the service's agent_actions_enabled, while this one is cleared by a person setting enabled for that (server, action) row in config.config_server_actions. An agent that acts on the sentence looks at the wrong knob, and an agent that later sees the real server_action_off sees a code the dry run never mentioned.

This is also the #3684 shape from .github/REVIEW-TRAPS.md — "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted. Alias, do not copy." The code word is spelled as a literal here instead of coming from AgentActionCodes, and the copy has drifted on the first write.

Suggested change
return $"For reference, {ServerActionNames.Label(serverAction)} on {serverName} is currently {(state.Enabled ? "on" : "off")}"
+ (state.Enabled ? "." : ", so the real call would be refused as actions_off until it is turned on.");
return $"For reference, {ServerActionNames.Label(serverAction)} on {serverName} is currently {(state.Enabled ? "on" : "off")}"
+ (state.Enabled ? "." : $", so the real call would be refused as {AgentActionCodes.SwitchOff} until it is turned on.");

Two smaller things in the same sentence, both non-blocking:

  • ServerActionNames.Label returns a verb phrase, so the note reads "For reference, Evict plans from the plan cache on srv-1 is currently off" / "Force or unforce plans on srv-1 is currently off". The fixture string in the new McpWriteToolWordingTests case ("Force plan on srv") reads like what was intended. Either the raw serverAction word (force_plan, which is also what the gate's own refusal message uses at InvestigationRequestStore.ServerAction.cs:195) or a re-worded sentence ("the force_plan switch on srv-1 is currently off") would be both grammatical and consistent with the code the agent sees elsewhere.
  • Even with the switch on, the real call still has four conditions ahead of it (actions_off, claim_not_live, not_level_3, no_remediation_credential). The "on." arm doesn't promise success, so this is fine as written — just worth not letting the "off" arm claim more precision than the gate order supports.

/* #6455: no claim, no agent_action. The plan tools read the switch for a refused dry run through ServerActionSwitches.ListAsync. */
var off = await DarlingMcpPlanForceTools.ReadSwitchStateAsync(owner, null, SqlServerId, "srv-1", "force_plan", ct);
Assert.Contains("is currently off", off, StringComparison.Ordinal);
Assert.Contains("actions_off", off, StringComparison.Ordinal);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This assertion pins the defect rather than failing on it: it requires the note to contain actions_off, which is the service-wide agent_actions_enabled code, not the per-server switch code the real call would answer (server_action_off — AgentActionCodes.SwitchOff, InvestigationRequestStore.ServerAction.cs:193). See the comment on DarlingMcpPlanForceTools.cs:191.

.github/REVIEW-TRAPS.md names this shape: "A test that encodes the lie: ACappedResult_WithholdsTheUnsatisfiedSummary_… handed the builder two facets at limit = 2 and asserted truncated — the >= limit inference as its own proof (#3699). Ask what defect a test would fail ON." This assertion is green on the wrong code and would go red on the right one.

If the message is fixed, this becomes the test that proves it:

Suggested change
Assert.Contains("actions_off", off, StringComparison.Ordinal);
Assert.Contains(AgentActionCodes.SwitchOff, off, StringComparison.Ordinal);

(AgentActionCodes is already in scope here via using PerformanceMonitor.Common; at line 25 — it is used at line 154 of this same file.) Asserting the constant rather than the literal also keeps the test from drifting the next time the code word moves.

{
try
{
var rows = await ServerActionSwitches.ListAsync(owner, ct);

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, two observations about which pool and how much is read here.

The owner pool for a read the mcp role can do. ServerActionGate's own doc explains why owner exists on this path: "the remediation secret column is not readable by the mcp role" (ServerActionGate.cs:34-36). The switch state is not in that category — get_server_actions reads it through the mcp data source (DarlingMcpServerActionTools.cs:120), and so does the web GET (DarlingWebEndpoints.cs:4149). RunAsync has postgres in hand at every call site, so passing that instead would keep the owner pool for the reads that actually need owner rights.

A whole-fleet read for one sentence. ServerActionSwitches.ListAsync has no predicate and no LIMIT (ServerActionSwitches.cs:84-88): it returns every monitored server joined to every switch row, then materialises three ServerActionSwitchState records per server, and this code keeps one of them. On a large fleet that is the full registry crossed with the switch table on a path that any MCP token can hit repeatedly with no claim and no gate behind it (the refusal is returned before any of the gate's conditions are evaluated). A single-row read keyed on (server_id, action) — the shape ServerActionSwitchSql already uses at InvestigationRequestStore.ServerAction.cs:187 — would answer the same question. Worth it only if the fleet sizes you care about make it measurable; the correctness of the note doesn't depend on it.

claude[bot]
claude Bot previously requested changes Oct 23, 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

Claims

  • VERIFIED — item 3 touches all four plan tools (force/unforce, evict, kill, kill-on-PostgreSQL) and reads the switch with ServerActionSwitches.ListAsync, which needs no claim — DarlingMcpPlanForceTools.cs:271, :446, :570, :726 (the only four ServerActionGate.AuthorizeAsync sites in the tree) and the read at :186.
  • VERIFIED — "Gate order is unchanged" — ServerActionGate.cs and InvestigationRequestStore.ServerAction.cs are untouched; the switch is read only after the gate has returned its refusal (:177-179).
  • VERIFIED — "A real call, and any other refusal reason, keeps its old text" — the dryRun && Reason == AgentActionCodes.Required guard at :161 gates sentence and read alike; asserted at McpWriteToolWordingTests.cs:107-108.
  • VERIFIED — a throwing switch read drops the note and still answers the refusal, logging a warning — :197-201, with the cancellation rethrow at :193-196.
  • REFUTED — "... currently off, so the real call would be refused as actions_off until it is turned on" — the tree answers server_action_off for a per-server switch that is off (AgentActionCodes.SwitchOff, InvestigationContracts.cs:403; returned at InvestigationRequestStore.ServerAction.cs:193). actions_off is a different condition, the service-wide config_service.agent_actions_enabled (InvestigationContracts.cs:382). Four behaviour tests already pin server_action_off for this exact state: PlanForceToolLiveTests.cs:403, PlanForceToolLiveTests.Evict.cs:288, PlanForceToolLiveTests.Kill.cs:467, PostgresKillLiveTests.cs:362. Inline at DarlingMcpPlanForceTools.cs:191.
  • VERIFIED — no scope means every monitored server, and the servers / tag shapes are as documented — CustomAlertRuleDefinition.cs:462 (defaults to CustomAlertScopeMode.All), :473-490, :491-510, :105.
  • VERIFIED — "a new definition replaces the old one whole" — definition ?? row.DefinitionJson at DarlingMcpCustomAlertTools.cs:293, so a scope omitted from new JSON is gone.
  • VERIFIED — create_custom_alert_rule already carried the sentence from #6509 — DarlingMcpCustomAlertTools.cs:159-160, outside this diff.
  • VERIFIED — the README paragraph sits under "Level 2: agents change mute rules and custom alert rules" (Darling/README.md:2291, paragraph at :2307) and its agent-scope claim matches DarlingMcpAgentActions.RuleScopeRefusalAsync:370-377.
  • VERIFIED — "Only description strings were touched in DarlingMcpCustomAlertTools.cs" — the file's one hunk is inside the [Description(...)] literal of UpdateCustomAlertRule.
  • VERIFIED — item 7 needed nothing: RefuseMissingRequired exists (McpUnknownArgumentGuard.cs:268), is called from the one filter (:107) registered in both SKUs (DarlingMcpHostService.cs:1849, Lite/Mcp/McpHostService.cs:487), and the diff does not touch it.
  • UNVERIFIED — "0 errors; the 6 warnings are in files this PR does not touch" — needs a build. Nullable flow on the four new GateRefusalAnswerAsync(..., owner, ...) sites is what it would settle, and reads sound: owner is null-checked at :262, :436, :549, :715.
  • UNVERIFIED — "Total 654, Failed 0, Skipped 68 ... my own throwaway PostgreSQL" — a rig result; no runner or DARLING_TEST_PG here.
  • UNVERIFIED — the two new PlanForceToolLiveTests cases skipped for want of DARLING_TEST_SQL. Note neither asserts the refusal code in the note (only "is currently off" / "is currently on.", :415-418), so CI passing them would not catch the REFUTED claim.

Riskiest lines

  1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpPlanForceTools.cs:191 — the sentence this change exists to produce names actions_off where the gate answers server_action_off, pointing the agent at the service-wide knob instead of the per-server switch row — UNCOVERED, and worse: ServerActionSwitchWriteLiveTests.cs:94 asserts the wrong word, so the defect is pinned green and the fix turns that test red.
  2. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpPlanForceTools.cs:197-201 — the catch that must let a failed switch read fall back to the bare refusal rather than answer an error — UNCOVERED; no behaviour test drives a failing ListAsync through this path.
  3. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpPlanForceTools.cs:186 — ListAsync(owner, ...) reads the whole registry crossed with the switch table, on the owner pool, on a path reachable with no claim and no gate condition met — happy path covered by ServerActionSwitchWriteLiveTests.ADryRunRefusedForWantOfTheClaim_ReadsTheSwitchWithoutOne_AndSaysOnOrOff; pool choice and read width UNCOVERED and non-blocking.

Traps near this diff

  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies — the code word is a literal actions_off in a message string at :191 instead of AgentActionCodes, and the copy is already wrong against AgentActionCodes.SwitchOff.
  • "A test that encodes the lie ... Ask what defect a test would fail ON. (#3699)" — applies — ServerActionSwitchWriteLiveTests.cs:94 asserts Assert.Contains("actions_off", off): green on the wrong code, red on the right one.
  • "Ruling (#5833): tests are load-bearing ... never answer a trap with a test that reads source text or asserts how code is spelled" — does not apply — all three new tests drive behaviour (McpWriteToolWordingTests.cs:100-108, ServerActionSwitchWriteLiveTests.cs:92-101, PlanForceToolLiveTests.cs:425).
  • "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's ... description without the counterpart's is a finding, not a nit" — does not apply — Lite ships no counterpart; the plan and custom-rule tools exist only under Darling/ plus the shared roster at PerformanceMonitor.Common/Investigations/InvestigationRules.cs:68.
  • "hours_back, limit, top, days_back are refused when out of range, never clamped (#3699)" — does not apply — no window or page argument is added; ReadSwitchStateAsync:182 takes a server id and an action name.
  • "Truncation inferred from a full page: truncated = rows.Count >= limit ... Fetch limit + 1" — does not apply — no page and no truncated; ServerActionSwitches.ListSql has no LIMIT (ServerActionSwitches.cs:84-88) and the code keeps one state object (:187).
  • "A total_* assigned from a .Count is a count of the page unless the set is provably whole (#3699)" — does not apply — nothing in the diff emits a count; the payload gains one sentence on message.

Divergence (blind read vs PR body)

One, and it is the finding. The body quotes the off-arm sentence verbatim and presents it as the mechanism item 3 needed; my blind read of the gate found that mechanism answers server_action_off, not actions_off — two codes separated by condition and by checking order (InvestigationContracts.cs:382 vs :403), with four live tests already pinning the right word. Body and diff agree with each other and both disagree with the gate: the body reproduces the literal instead of checking it against AgentActionCodes, and the new test at ServerActionSwitchWriteLiveTests.cs:94 asserts the same literal, closing the loop on itself.

Two smaller gaps. The body writes For reference, <action> on <server> is currently on., but <action> renders through ServerActionNames.Label (InvestigationContracts.cs:334-340), so the real sentences read "Force or unforce plans on srv-1 is currently off" and "Evict plans from the plan cache on srv-1 is currently off" — and the author's own fixture string ("Force plan on srv", McpWriteToolWordingTests.cs:98) does not match what the code emits. The body also does not mention that the note reads the whole fleet's switch table on the store owner's pool on a pre-gate path; non-blocking, inline at :186.

Reviewer checklist

  • Run the two new PlanForceToolLiveTests dry-run cases with DARLING_TEST_SQL and DARLING_TEST_PG set and read the whole refusal message, not just the "is currently off" substring.
  • Against a live store, send force_query_store_plan with dry_run=true and no agent_action on a server whose force_plan switch is off, then the same call with a valid Level 3 agent_action, and check the second answer's code is the one the first predicted.
  • Confirm the build is clean, including nullable flow on the four new GateRefusalAnswerAsync(..., owner, ...) sites (:271, :446, :570, :726).
  • On a fleet-sized registry, time one refused dry run to see whether the whole-fleet ListAsync on the owner pool is measurable.

claude[bot]
claude Bot previously requested changes Oct 23, 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

Claims

  • VERIFIED — "Merged origin/dev. One conflict, the UpdateCustomAlertRule tool description in DarlingMcpCustomAlertTools.cs: dev added the previous/undo sentence to the same line this PR extends with the server-scope wording. Kept both, in that order." — Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpCustomAlertTools.cs:234-235: the previous/undo sentence (dev) is immediately followed by the "Server scope is part of the definition…" sentence (this PR), in that order.
  • VERIFIED — "Merging dev conflicted in docs/mcp-server.md only… Kept both, dev's row unchanged." — docs/mcp-server.md:142-143: the "Custom alert rules" row (this PR) and the rewritten "Alert tuning" row with update_notification_channels (dev) both sit intact, adjacent.
  • REFUTED (carried forward, unfixed by this commit) — the PR's own summary line, "the dry-run refusal now also states the server's switch state," including the off-arm wording quoted in the body — Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpPlanForceTools.cs:191 still emits "...so the real call would be refused as actions_off until it is turned on.", but the gate for an off switch answers AgentActionCodes.SwitchOff (server_action_off), not actions_off (InvestigationContracts.cs:403 vs the service-wide actions_off at :382). This is the exact defect the prior review on this PR (head=1bc880f8) flagged as CHANGES REQUESTED. The only commit in this follow-up range is the dev merge, and it does not touch DarlingMcpPlanForceTools.cs or ServerActionSwitchWriteLiveTests.cs at all (git diff 1bc880f8..3051d1b22 is empty for both files), so the defect — and the self-confirming wrong assertion at ServerActionSwitchWriteLiveTests.cs:94 (Assert.Contains("actions_off", off, ...)) — stands exactly as before.

Riskiest lines

  1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpPlanForceTools.cs:191 — still unfixed from the prior review: an agent refused a dry run for a switch that is off is told the real call "would be refused as actions_off," pointing it at the wrong knob (config_service.agent_actions_enabled) instead of the per-server switch it just read — UNCOVERED, and worse, ServerActionSwitchWriteLiveTests.cs:94 pins the wrong word green, so a correct fix turns that test red.
  2. docs/mcp-server.md:142-143 — this follow-up's only real content: a two-way merge conflict between this PR's new row and dev's rewritten row — correctly resolved (both verified present) — UNCOVERED (docs, no behaviour test expected).
  3. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpCustomAlertTools.cs:234-235 — the other merge conflict, splicing dev's previous/undo sentence with this PR's scope sentence into one [Description] literal — correctly resolved, both sentences intact — UNCOVERED (description text, no behaviour test expected).

Traps near this diff

  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies (carried forward, unfixed) — the literal actions_off at DarlingMcpPlanForceTools.cs:191 is a hand copy of a code, not a reference to AgentActionCodes, and it is already wrong against AgentActionCodes.SwitchOff; the merge in this range left it untouched.
  • "A test that encodes the lie … Ask what defect a test would fail ON. (#3699)" — applies (carried forward, unfixed) — ServerActionSwitchWriteLiveTests.cs:94 still asserts Assert.Contains("actions_off", off, ...), green on the wrong code.
  • "Ruling (#5833): tests are load-bearing … never answer a trap with a test that reads source text or asserts how code is spelled." — does not apply to this follow-up's own content — neither merge conflict (DarlingMcpCustomAlertTools.cs:234-235, docs/mcp-server.md:142-143) added or touched a test.

Divergence (blind read vs PR body)

The body's "Dev merge" note accurately describes both conflicts and how they were resolved — verified on both counts, no divergence there. The divergence is with the PR's main account, carried into this commit unchanged: the body still presents the off-arm sentence as delivering "the server's switch state," and the merge commit in this follow-up range had the opportunity to touch DarlingMcpPlanForceTools.cs (dev's refactor passed directly through several nearby files, e.g. DarlingMcpCustomAlertTools.cs's CustomAlertRuleStore construction) but did not reconcile or even revisit the actions_off wording the prior review rejected. Nothing in this commit range addresses the earlier CHANGES REQUESTED finding.

Reviewer checklist

  • Fix DarlingMcpPlanForceTools.cs:191 to emit the real refusal code (AgentActionCodes.SwitchOff / server_action_off, not the literal actions_off), then fix ServerActionSwitchWriteLiveTests.cs:94 to assert the corrected word so it fails on the old defect and passes on the fix.
  • Run the two new PlanForceToolLiveTests dry-run cases with DARLING_TEST_SQL/DARLING_TEST_PG set and read the whole refusal message, not just the "is currently off" substring, to confirm the corrected code reads the way a live agent would parse it.
  • Confirm the build is clean after the merge (dotnet build Darling/Darling.Tests).

…he constant (#6455)

The note said the real call would be refused as actions_off, the service-wide
switch. The per-server switch answers server_action_off. Build it from
AgentActionCodes.SwitchOff, assert the predicted code equals the gate's, and
cover the failed-read fallback.
@erikdarlingdata
erikdarlingdata dismissed stale reviews from claude[bot] and claude[bot] October 24, 2026 01:49

Claude posting for Erik Darling. Addressed in 20bf2a5: the dry-run note now names the per-server switch code from AgentActionCodes.SwitchOff, and the live test asserts that code. CI green on that head.

@erikdarlingdata
erikdarlingdata merged commit 91ec691 into dev Oct 24, 2026
23 of 24 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/6455-mcp-write-wording branch October 24, 2026 01:49
@github-actions github-actions Bot mentioned this pull request Oct 24, 2026
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