[erikdarlingdata]

Claude posting for Erik Darling.

Follow-up to #6593 (L5 of its review) and #6514. An operator token can now put a route's agent level back to Inherit over MCP. The undo of an operator token's route write now puts the agent fields back.

What changed

All code changes are in Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.RouteAdmin.cs.

1. clear_agent_level: true on update_notification_route. It sets agent_level to NULL (Inherit), so the next level decides. It has the same shape as clear_fallback: true.
   • The parameter is for operator tokens only. Any other token is refused by the existing agent-field check.
   • agent_level and clear_agent_level together answer invalid, and nothing is written.
   • The journal row is route_agent_changed, reason changed agent_level, with before.agent_level 2 and after.agent_level null.
2. previous and undo carry the agent fields. previous already held them, because it is the full route projection. For an operator token, undo.arguments now carries agent_level and the four limits. When the level was Inherit, it carries clear_agent_level: true and no agent_level.
   • A token that is not operator-class gets the old undo, with no agent fields. That token can't set them, so an undo that carries them would be refused.
   • The agent fields that already hold the stored value are dropped before the write. So an undo that carries all five writes only the fields that differ. A call that changes nothing still answers unchanged, and the journal gets no empty row.
3. The single-field tools. set_notification_route_enabled (any route with an agent setting) and set_notification_route_fallback (level 1 or more) hand an operator token's call to update_notification_route. So their answers already use its previous and undo, and they now carry the agent fields too. The live test runs that path for set_notification_route_enabled. Their own undo, for a route with no agent setting or a token that is not operator-class, is unchanged. Those calls can't touch an agent field.
4. Tools that can't touch agent fields are unchanged. create_notification_route (its undo is a delete), delete_notification_route (it refuses an agent route and an Off route for every caller), and the mute and alert-settings tools.

Decisions

• Clearing the level keeps the four limits. The Viewer's route window (NotificationRouteEditDialog.ReadAgent, saved by NotificationRouteWriter.AddAgentColumns) saves an Inherit as agent_level NULL, and it writes the four limit boxes as they stand. The boxes are disabled, not reset. MCP does the same. The limits apply only at level 1 or more, so a kept limit has no effect while the route inherits.
• No store change. config.set_notification_route_agent takes the level from p_agent_level when agent_level is in p_columns, so a NULL parameter stores NULL. Its check is v_level IS NOT NULL AND v_level NOT IN (0,1,2,3), so NULL passes. Its journal compares with IS DISTINCT FROM, so 2 to NULL is a change. DarlingManagedRoles.cs and provision-roles.sql are not touched. No migration rung.
• Tool descriptions. The update_notification_route head is unchanged. The new guidance is in the new parameter's description (136 characters, under the 230 cap) and after <<GUIDE>>.
• Lite. Lite has no route authoring over MCP. A grep of Lite/ for update_notification_route finds nothing, so there is no Lite twin.

Tests

The new facts are in Darling/Darling.Tests/McpRouteAgentFieldsLiveTests.cs. They use the class's own live-PostgreSQL pattern. Booleans are read with ::text.

• AnOperatorToken_ClearsALevel2Route_BackToInherit_AndTheJournalShowsBefore2AfterNull: the stored level is NULL and the limit is kept. There is one route_agent_changed row with before 2 and after null. previous.agent_level is 2, and the undo carries agent_level 2.
• TheUndo_OfAnAgentFieldChange_RestoresTheLevelAndTheLimits: it changes the level and all four limits, runs the answer's undo, and the five stored fields equal the originals. Then it turns the route off with set_notification_route_enabled. That answer's previous and undo carry the level, and running the undo puts enabled back with the agent fields unchanged.
• TheUndo_OfAChangeMadeFromInherit_PutsInheritBack: the undo carries clear_agent_level: true and no agent_level, and running it stores NULL.
• ClearAgentLevel_WithAgentLevel_IsInvalid_AndANonOperatorTokenCannotClearIt: both together answer invalid. A named agent-class token and an investigations-only token are each refused, on a level 2 route and on an Off route. No level changes and no journal row is written.

What ran where

Darling.Tests builds on macOS but can't run there. I ran the class in a throwaway net10.0 console that compiles the same test files against the real Service project, on a local timescale/timescaledb:latest-pg17 container with the CI superuser name. That rig and the container are gone, and neither is committed. In that rig the whole class passed: 9 of 9 facts, the 5 that were there before and the 4 new ones. McpToolsListBudgetTests (2), McpToolGuideTests (10) and McpToolGuideHeadsAlertingTests (13) passed in the same rig. Darling.Tests.csproj builds with 0 errors. The full suite waits for CI.

Revert proofs

Each revert was applied to the code, built, and run against the live class. Then the file was restored.

Revert	Facts that failed	Where
Drop the clear path (ClearLevel not counted as an agent field, not in the columns, not applied to the draft)	ClearsALevel2Route, PutsInheritBack, ClearAgentLevel_WithAgentLevel (3 of 9)	ran locally
Only ClearLevel taken out of HasAgentField	the same 3 of 9	ran locally
Drop the agent fields from undo	RestoresTheLevelAndTheLimits, ClearsALevel2Route, PutsInheritBack (3 of 9)	ran locally
undo uses agent_level: 0 instead of the clear	PutsInheritBack (1 of 9)	ran locally
undo leaves the level out instead of the clear	PutsInheritBack (1 of 9)	ran locally
Drop the refusal of agent_level and clear_agent_level together	ClearAgentLevel_WithAgentLevel (1 of 9)	ran locally
Drop the operator-token check for agent fields (both checks)	ClearAgentLevel_WithAgentLevel, TheLegacyWritesToken (an existing fact) (2 of 9)	ran locally

The same facts on Windows CI wait for CI.

Considered, not added

• Undo for create_notification_route stays the delete. A delete removes the agent fields with the route, so there is nothing to restore.
• No clear_agent_level on create_notification_route. A new route's level is NULL unless agent_level is given, so the flag would do nothing.

CHANGELOG entry

Darling MCP: an operator token can put a notification route's agent level back to Inherit with update_notification_route's clear_agent_level: true. The four limits are kept, as the Viewer's Inherit keeps them, and the change is journaled as route_agent_changed. The undo of an operator token's route write now puts the agent level and limits back too, including Inherit. (#6593, #6514)

[claude]

Non-blocking, but this is the one mechanism in the PR with no proof behind it.

Narrowed changes an already-shipped answer, not just the new undo path. Before this commit, an operator token calling update_notification_route(route_id, agent_max_per_hour: 20) on a route already at 20 got status: "updated", changed: ["agent_max_per_hour"], a config.set_notification_route_agent call and a modified_at / config_version bump (which makes the service reload its channel configuration on the next sweep). After it, the same call answers unchanged and writes nothing. That is the better answer — but the body presents it only as a consequence of the undo carrying all five fields, and the revert table has seven rows and no row for this one.

Nothing in the class fails if Narrowed is deleted. The four new facts don't: TheUndo_OfAnAgentFieldChange_RestoresTheLevelAndTheLimits asserts updated and the restored values, which hold whether or not the equal fields are dropped. The five existing facts don't either, only because their chosen values miss the defaults — agent_max_per_day: 50 against DefaultAgentMaxPerDay = 100 and agent_lease_minutes: 30 against DefaultAgentLeaseMinutes = 15 (PerformanceMonitor.Notifications/NotificationRoute.cs:136,139). Pass agent_lease_minutes: 15 instead and AnOperatorToken_CreatesARouteAtLevel1_…'s journal assertion (reason = 'changed agent_level, agent_lease_minutes') would start depending on this line. So the "9 of 9" rig result is consistent with the narrowing being untested in both directions.

A fact that would fail on the defect: an operator token re-passing a stored limit answers unchanged, writes no route_agent_changed row, and leaves modified_at where it was.

Second, smaller point on the same line: existing was read at line 242 on its own connection, outside the transaction that opens at 389. Narrowed therefore decides "this field already holds that value" from a snapshot, where config.set_notification_route_agent decided it under SELECT … FOR UPDATE (Darling/tools/provision-roles.sql:1659). A Viewer save landing in that window turns a real change into status: "unchanged" with route projected from the stale existing. This is the same shape the method already has for metric_match, enabled, scope, send_resolutions and fallback_route_id (all compared against the same snapshot at 274-342), so it's consistent rather than new — flagging it only because the agent columns are the ones with a journal attached.

[claude]

Non-blocking, and I think the behaviour is the right one — but worth naming what the gate is.

agentFields is operatorActor is not null, not "this call touched an agent field", so every operator-token answer now carries the five agent arguments, including the unchanged answer at line 378 and the set_notification_route_fallback / set_notification_route_enabled hand-offs. On a route that never had an agent setting that means the undo of a plain metric_match change reads clear_agent_level: true.

Two consequences:

1. If a person sets the level in the Viewer between the change and the undo, running that undo clears it as a side effect of undoing the match change. That is the same "previous is previous" semantics the undo already applies to enabled and fallback_route_id, so I'd leave it — but the guide sentence at line 198 says undo "puts the agent fields back too", which reads as "the ones this call changed". One clause saying it restores them whether or not the call touched them would close the gap between the sentence and line 617.
2. An LLM reading clear_agent_level: true in the undo of a plain route edit has no way to tell "this route has no agent setting" from "this call cleared one". The changed array answers it, but only if the reader consults it.

Both are fine as designed; neither needs a code change if the guide sentence says which it is.

[claude]

Non-blocking. These two policies are the same caller class, and neither is the one that matters in production.

IsOperator => Writes && !Investigations (DarlingMcpWritePolicy.cs:248) doesn't read TokenName, so agentToken and investigations differ only in a field the gate ignores: both are Writes: false, Investigations: true, both non-operator, and the loop runs the same arm twice. The docstring's "an agent-class token, an investigations-only token" names two classes the code has one of.

More to the point, with Writes: false neither policy would ever be offered update_notification_route — Effective.Offers falls through to IsWrite(toolName) → return Writes (DarlingMcpWritePolicy.cs:272), so the host filter refuses both before the tool body runs. The non-operator caller that genuinely reaches this code is a token served both arms, Effective(Writes: true, ServerAdmin: false, Investigations: true): Offers returns Writes (the IsAgentAction arm is dead here because update_notification_route is in NeverAgentTools), while IsOperator is false. That token is the only production caller for which the new agentFields: false branch at RouteAdmin.cs:617 is reachable — i.e. the one the PR body's claim "a token that is not operator-class gets the old undo, with no agent fields" is actually about — and nothing covers it.

Swapping one of the two for Effective(true, false, Investigations: true) would both cover the refusal through the path a real token takes and give that undo branch its only fact.

[claude]

One note that falls just outside the diff, since this PR is the second commit to widen the thing it describes.

docs/darling-agent-dispatch.md:91 still reads:

Route agent fields over MCP (the agent level and its limits on create_notification_route and update_notification_route, for the operator role only) come in a later change. Until then you set agent routes in the Viewer.

That sentence was true before #6542 part 2. It is now wrong twice over — the level and the four limits ship on both tools, and with this PR update_notification_route also puts a level back to Inherit. The staleness predates this branch, so it isn't a finding against it, but this is the PR whose CHANGELOG entry a reader would arrive from, and the doc tells them the opposite. One paragraph there (the operator-token rule, clear_agent_level, and that the change is journaled as route_agent_changed under operator:<name>) would retire it.

[claude]

Verdict: LGTM

The clear path is right in the place that decides it: agent_level goes into p_columns while p_agent_level binds SQL NULL, the shape config.set_notification_route_agent already honored, so no rung and no function change. The operator gate runs before the narrowing, so clear_agent_level is no hole for a non-operator token. Three inline comments, all non-blocking.

Claims

• VERIFIED — all code changes in DarlingMcpAlertTools.RouteAdmin.cs — gh pr diff --name-only returns that file and the test file, nothing else
• VERIFIED — clear_agent_level: true stores NULL and keeps the four limits — RouteAdmin.cs:485 names the column, :516 nulls the draft, :537 binds null, provision-roles.sql:1663 takes p_agent_level for a named column
• VERIFIED — operator only; any other token refused by the existing check — ClearLevel joins HasAgentField at :466 and the gate at :254 runs before the narrowing at :266
• VERIFIED — both flags together answer invalid, nothing written — :259-262 returns before the transaction at :388
• VERIFIED — journal route_agent_changed, reason changed agent_level, before 2 after null — provision-roles.sql:1697 (IS DISTINCT FROM), :1722-1729
• VERIFIED — previous already held them, being the full projection — RouteWritePayload to RoutePayload, DarlingMcpAlertTools.cs:1022-1026
• VERIFIED — operator undo.arguments carries the level and four limits, or clear_agent_level: true and no level — :617-632
• VERIFIED — a non-operator-class token gets the old undo — agentFields is operatorActor is not null at :378/:446, and IsOperator => Writes && !Investigations (DarlingMcpWritePolicy.cs:248) leaves a writes-plus-investigations token non-operator, so the branch is live
• VERIFIED — equal fields dropped before the write, a no-op call answers unchanged with no journal row — :266, :371, provision-roles.sql:1717
• VERIFIED — the two single-field tools hand an operator call to update and carry the fields — DarlingMcpAlertTools.cs:790-794, :869-874; test at McpRouteAgentFieldsLiveTests.cs:272-283
• VERIFIED — create and delete untouched; delete refuses an agent route and an Off route for every caller — DarlingMcpAlertTools.cs:927-935
• VERIFIED — the Viewer saves Inherit as NULL and writes the limit boxes as they stand, disabled not reset — NotificationRouteEditDialog.xaml.cs:424, :429-430, ReadAgent at :440-453
• VERIFIED — a kept limit is inert while the route inherits — NotificationRouter.cs:261-263 skips a null level and returns the route that set one, whose limits InvestigationDispatchCore.cs:181 reads
• VERIFIED — no store change, no rung: NULL passes the level check, IS DISTINCT FROM makes 2 to NULL a change — provision-roles.sql:1669; neither SQL file is in the diff
• VERIFIED — head unchanged, new text after <<GUIDE>> (:191, :198, marker :185), parameter description 136 chars under the 230 cap at McpToolsListBudgetTests.cs:45
• VERIFIED — Lite has no route authoring over MCP — Lite/Mcp/McpAlertTools.cs:451, :613, :696 are its only route tools, no update_notification_route
• UNVERIFIED — the rig results (9 of 9 facts, budget/guide tests, 0 build errors) — needs a build and a live PostgreSQL; the rig and container are stated gone and uncommitted
• UNVERIFIED — the seven-row revert table — each row needs its revert applied, built and re-run live; a read-only tree reads which fact would fail, not that it did

Riskiest lines

1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.RouteAdmin.cs:266 — agent.Narrowed(existing) is the only line changing an answer this tool already shipped (an operator re-passing a stored limit flips updated plus a config_version bump to unchanged), decided from the snapshot read at :242 outside the transaction rather than under the function's FOR UPDATE — UNCOVERED (all four new facts and the five existing ones pass with the line deleted; the existing ones miss it only because their values differ from the 20/100/15/240 defaults at NotificationRoute.cs:133-142)
2. RouteAdmin.cs:485 — drop the || ClearLevel and provision-roles.sql:1663 keeps the stored level while the tool still answers updated — covered by AnOperatorToken_ClearsALevel2Route_BackToInherit_AndTheJournalShowsBefore2AfterNull (asserts the row reads null|7)
3. RouteAdmin.cs:619 — the arm picking agent_level against clear_agent_level: true; the wrong one makes an Inherit route un-restorable by its own undo — covered by TheUndo_OfAChangeMadeFromInherit_PutsInheritBack

All three sit behind Assert.SkipWhen on DARLING_TEST_PG (McpRouteAgentFieldsLiveTests.cs:377-379): behaviour tests, but silently skipped where that variable is unset, which is what makes line 1's gap matter more than it looks.

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 — the census in McpPageContractTests compares the two SKUs' payload keys by tool name for exactly this (#3594)." — does not apply — Lite registers no route-authoring tool (Lite/Mcp/McpAlertTools.cs:451, :613, :696), so there is no counterpart to grow; its agent_level is read-only payload at :592-596
• "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 — Narrowed (:469-475) re-computes in C# the "which agent fields differ" question the function computes as v_changed (provision-roles.sql:1697-1701), but the two are not pinned and need not agree: C# decides only the status word and the column list, SQL still owns the journal under the lock
• "hours_back, limit, top, days_back are refused when out of range, never clamped — a clamp is a silent lie about the window." — applies and holds — the contradictory pair is refused at :259-262 rather than one winning, and an out-of-range limit is refused twice, through :521 and again at provision-roles.sql:1672-1683
• "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, holds for three of four mechanisms — the new facts assert stored columns and journal rows, not source text; the narrowing at :266 has neither a fact nor a sentence
• "A test that encodes the lie … Ask what defect a test would fail ON." — applies and holds — each new fact names a defect it fails on, and the invalid/refusal fact closes with negative assertions proving nothing was written (levels still 2 and 0, zero changed agent_level rows)

Divergence (blind read vs PR body)

1. The narrowing has no proof row. The revert table is otherwise exhaustive, seven rows, one per mechanism. Narrowed has no row and no fact. It is also the only part of the change touching an answer the tool already gave: the body frames it as a consequence of the undo carrying all five fields, which undersells it — the same line turns agent_max_per_hour: 20 on a route already at 20 from updated into unchanged for every operator token, undo or not. One fact, or one sentence saying why none is needed, squares the two accounts.
2. The fourth test's cross product. "each refused, on a level 2 route and on an Off route" reads true (both policies refused, both shapes exercised), but the Off route is tried with agentToken only (McpRouteAgentFieldsLiveTests.cs:346-349) — three of four combinations. Immaterial, since IsOperator ignores TokenName and the two policies are the same caller class, which is the inline comment's larger point.

Reviewer checklist

• Run the class on Windows CI with DARLING_TEST_PG set and check the log says nine facts ran, not nine skipped — without it Assert.SkipWhen retires this PR's whole proof and the job still goes green
• Re-run McpToolsListBudgetTests and McpToolGuideTests in CI: only a real tools/list conversion exercises the 230-char cap and the <<GUIDE>> split
• On a live store, clear the level on a route the dispatcher is resolving against and watch one resolution fall through to the next level that sets one, with the kept limits inert
• After an MCP clear, confirm the web dashboard's routes page and the Viewer's route window both show Inherit — the config_version reload path is the one surface a read-only tree cannot exercise

[claude]

Verdict: LGTM

Claims

• VERIFIED — PR body describes commit 2ac409e (clear_agent_level / undo restoring agent fields) — that commit precedes this follow-up's range and was covered by the prior review; nothing in it changed in this range.

Riskiest lines

1. N/A — the only commit in range (2ac409e..12014cf) is 12014cf29, a merge of origin/dev bringing in two unrelated, already-merged PRs (#6644 "Fix the store script's owner list for the route functions #6593 changed" and #6608/#6429 "MCP setup page" docs). Per the follow-up rules, a merge of the base branch is not the author's change and is left alone unless a new commit interacts with it — there is no such commit here. — covered by N/A | UNCOVERED (nothing authored to cover)

Traps near this diff

• None evaluated — no authored change in range to hold against the traps file; the merge's content (a grant-list signature fix in provision-roles.sql and MCP setup doc rewrites) came from separately-reviewed PRs.

Divergence (blind read vs PR body)

None — the PR body's account describes commit 2ac409e, which is outside this follow-up's range (2ac409ecf6c55000b1e06486bab5062104049012..12014cf290a72770457665d72cf60cb09db19da9) and was already reviewed. The only commit actually in range is the origin/dev merge, which the body does not and need not mention.

Reviewer checklist

• None — confirm on the next push that the follow-up range for the next review starts after 12014cf290a72770457665d72cf60cb09db19da9.