Repository navigation
Darling MCP: an operator token sets route agent fields, part 2 of #6542 - #6593
Conversation
…#6542 part 2) create_notification_route and update_notification_route take agent_level, the four limits and fallback_route_id for a named operator token only, checked with NotificationRouteWriter.Validate and written by config.set_notification_route_agent, which journals route_agent_changed as operator:<name>. Level 3 is refused over MCP, and a route at level 3 is not changed over MCP. Early agent refusals journal under the agent token's name. A named token's resolve error names its own setting.
…il-closed start (#6542 part 2)
…6542 part 2) MCP is full admin: the level 3 refusals are removed from the tool and the store function, and the test now asserts level 3 is set, changed and lowered by an operator token, each change journaled.
…ields (#6542 part 2) Agreed with the coordinator: under R2 of #5632 that token never holds the investigation tools, so it sets agent levels 0 to 3 and edits agent routes like a named operator token. Its journal actor is operator: plus the token identity the route write logs already carry. A token served the investigations sets no agent field.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| if (AgentRouteRefusal(existing, "update_notification_route") is { } agentRefusal) | ||
| agent ??= RouteAgentRequest.None; | ||
| if (AgentRouteRefusal(existing, "update_notification_route", operatorActor) is { } agentRefusal) |
There was a problem hiding this comment.
An operator token's update of an agent route is still refused by the store function — including the one this PR's own test asserts succeeds.
Passing operatorActor here makes AgentRouteRefusal return null for every operator-class caller, so the call proceeds. But config.update_notification_route is unchanged and refuses the whole row, not just match and scope:
IF v_row.agent_level IS NOT NULL AND v_row.agent_level >= 1 THEN
RAISE EXCEPTION 'That route is an agent route: it is changed in the Viewer, not over MCP.' USING ERRCODE = 'DL403';(DarlingManagedRoles.cs:2703-2705, tools/provision-roles.sql:1477-1479)
So whenever columns.Count > 0 on a route at agent_level >= 1, the transaction aborts at line 374 before SetRouteAgentAsync ever runs, RouteFunctionOutcome maps DL403 to refused, and nothing is written — not the non-agent column, not the agent fields.
Failing scenario, which is exactly McpRouteAgentFieldsLiveTests.cs:151-153:
- operator token creates "Blocking" with
agent_level: 1→created - operator token sets
agent_level: 3→updated(columns empty, so only the agent function runs) - operator token sends
send_resolutions: "always"→columns = ['send_resolutions'], route is at level 3 → DL403 →{"status":"refused","…agent route: it is changed in the Viewer, not over MCP"}
The test asserts "updated". It will fail in Darling PostgreSQL tests (the PR body says no test has run yet). The same happens for enabled: true, and for any call that mixes an agent field with enabled/send_resolutions/metric_match/scope on a route already at level ≥ 1 — the mixed call loses the agent write too.
Either relax the store function for the columns part 2 means to open (it holds the row's lock, so it is the right place to decide), or keep the C# guard for the non-agent columns and refuse the mixed call in words before the transaction opens. As written, the documented capability ("Only an operator token sets the agent fields") silently fails the moment a second field rides along.
| /// including agent_level 0: that is the route's explicit Off (null inherits, 0 is off, 1 investigates), so it opens | ||
| /// no investigation. It can be turned on over MCP; turning it off or deleting it is <see cref="OffRouteRefusal"/>'s (#6281). | ||
| /// <para>#6542 part 2: an operator-class caller (<paramref name="operatorActor"/>: a named operator token, or the legacy token with writes) passes, so update_notification_route can set the agent | ||
| /// fields of an agent route. The store functions still refuse its match, scope and destinations, and level 3 stays person-only.</para> |
There was a problem hiding this comment.
Two statements in this new paragraph are contradicted by the code it documents:
- "level 3 stays person-only" —
config.set_notification_route_agentaccepts0, 1, 2, 3(DarlingManagedRoles.cs:2808),AgentFieldRefusaladds no level ceiling, and the PR body's own "level-3 decision" section says an operator token sets 0 to 3. The sentence that is true is the one in the function's header comment: a level 3 server action still needsconfig.config_server_actions. Worth saying that instead, since this comment sits on the gate a reader checks first. - "The store functions still refuse its match, scope and destinations" — they refuse considerably more than that:
config.update_notification_routerefuses every column on a route atagent_level >= 1. That is the defect noted onDarlingMcpAlertTools.RouteAdmin.cs:245.
| { | ||
| internal static readonly RouteAgentRequest None = new(null, null, null, null, null); | ||
|
|
||
| internal int? Fallback { get; init; } |
There was a problem hiding this comment.
Non-blocking: Fallback is written (line 334) but never read — SetRouteAgentAsync takes the fallback as its own fallback parameter (line 500) and binds that at line 513, so this property is dead. Two carriers for one value, one of them unused, is the kind of pair that drifts later; either drop the property and keep the parameter, or drop the parameter and bind agent.Fallback.
| s.StartsWith("GRANT EXECUTE ON FUNCTION config.create_notification_route", StringComparison.Ordinal) | ||
| || s.StartsWith("GRANT EXECUTE ON FUNCTION config.update_notification_route", StringComparison.Ordinal) | ||
| || s.StartsWith("GRANT EXECUTE ON FUNCTION config.set_notification_route_agent", StringComparison.Ordinal)).ToList(); | ||
| Assert.Equal(3, executes.Count); |
There was a problem hiding this comment.
Non-blocking, but this is the shape REVIEW-TRAPS.md rules against: "never answer a trap with a test that reads source text or asserts how code is spelled (a signature at a line, a SQL fragment's wording, …)". executes is a prefix match over ViewerGrantReplay.BatchStatements(), so pinning it at 3 asserts how the provisioning batch is spelled, not what the role can do. A fourth GRANT EXECUTE ON FUNCTION config.update_notification_route… added for an unrelated reason reds this test with a message about a count.
The setup needs the three grants replayed; it does not need the count. Dropping the assert (or asserting each of the three prefixes was found, which is what the loop actually depends on) keeps the fixture honest without the pin.
|
Two notes that are not on a line this diff touches.
|
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Claims
- VERIFIED — both route tools take the five agent fields and a
DarlingMcpServedPolicyservice parameter, so the client cannot say who it is —RouteAdmin.cs:69-74,214-219; DI-bound atDarlingMcpHostService.cs:1316-1318. - VERIFIED — only operator-class tokens may use the fields, via new
IsOperator/OperatorActor—DarlingMcpWritePolicy.cs:240-249;PolicyForgives a named operatorEffective(true, admin, false, name)(DarlingMcpTokenRoles.cs:68-70). - VERIFIED — the actor is
operator:<name>oroperator:plus the legacy fingerprint, never a token value —DarlingMcpWritePolicy.cs:247-249overReadCaller.cs:67-75(8 hex of SHA-256, or the operator-chosen name). - VERIFIED — the function is SECURITY DEFINER, pinned search_path, no dynamic SQL, EXECUTE to
mcponly —DarlingManagedRoles.cs:2766-2767,2853, grant1785;provision-roles.sql:1155,1242,1250. - VERIFIED — the two copies have the same text — the hunks differ only by
{config}→configand the doubled{{1,64}}. - VERIFIED — the journal detail matches the Viewer's save — the nine keys and order at
DarlingManagedRoles.cs:2843-2850matchNotificationRouteWriter.cs:775-786;beforeis JSONnullon a create. - VERIFIED — a create journals only on a level, an edit only on a level or limit change —
DarlingManagedRoles.cs:2837;v_changedexcludesfallback_route_id, as commented. - VERIFIED — item 7 is
agentTokenName ?? agentIdat each pre-gate site —DarlingAgentActionGate.cs:70,DarlingMcpAgentActions.cs:162/202,DarlingMcpPlanForceTools.cs:220/887. The post-gate sites keepingaction.AgentId(250, 428, 540, 558, 573) are not a gap:ServerActionGate.cs:74-80refuses a mismatch first. - VERIFIED — item 10c names the entry's own setting —
DarlingConfig.cs:514-530, message built atDarlingSecretSource.cs:125-127and returned byDarlingMcpTokenRoles.cs:238-253. - VERIFIED — no rung needed; the CHECK already allows 0 to 3 —
PgMigrations.csV196 widensck_config_notification_routes_agentwith the same bounds the function re-checks. - VERIFIED — the journal trigger does not block it:
route_agent_changedis in noInvestigationEvents.Kindsroster, noonce_keyis written, and the trigger returns early for the owner (DarlingManagedRoles.cs:3463-3466), which a definer body is. - VERIFIED — a level 3 server action still needs
config.config_server_actions, which no MCP tool writes — its only mention underMcp/is the comment atServerActionGate.cs:26. - VERIFIED — an investigations-only policy sets no agent field —
IsOperatorfalse, soAgentFieldRefusalreturnsAgentFieldsNeedOperator(RouteAdmin.cs:482-487). - UNVERIFIED — "builds with 0 errors on macOS" and "scrubcheck reports 0 hits" — both need the build and the tool run. (The body also says plainly no test here has run anywhere; I take that at face value.)
- REFUTED — "The existing store functions still refuse a match, scope or destination change on an agent route for every caller. Part 2 opens only the agent fields." —
config.update_notification_routerefuses every column on a route atagent_level >= 1:DarlingManagedRoles.cs:2703-2705/provision-roles.sql:1477-1479raise DL403 on the row before any per-column check. WithAgentRouteRefusalbypassed, a mixed update reaches that RAISE and is refused whole, agent fields included. - REFUTED — "
TheLegacyWritesToken_…: … and it edits the agent route." —McpRouteAgentFieldsLiveTests.cs:151-153sendssend_resolutions: "always"to a route it just put at level 3; that adds a column (RouteAdmin.cs:307-311), so the function runs atRouteAdmin.cs:374, raises DL403, andRouteFunctionOutcomemaps it torefused(RouteAdmin.cs:530-535). The assertion expectsupdated.
Riskiest lines
Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.RouteAdmin.cs:245— dropping the C# agent-route guard for an operator token while the store function's own guard is unchanged, so a mixed update is refused whole instead of writing the agent fields — covered byMcpRouteAgentFieldsLiveTests.TheLegacyWritesToken_SetsLevels1And3_AndEachIsJournaled_ButAnInvestigationsTokenSetsNone, which fails on the defect at itssend_resolutionsassertion and passes on the fixDarling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.RouteAdmin.cs:326—fallbackWithAgentdecides, from a read outside the transaction, whether the fallback goes throughupdate_notification_routeor the new agent function; wrong either way it hits the DL403 above or is dropped fromcolumns— UNCOVERED (no test moves an agent route's fallback over MCP;NotificationRouteFallbackLiveTestsis the Viewer's path)Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs:2808—v_level NOT IN (0, 1, 2, 3)with the actor regex at2790is the whole guard on the one statement by which themcplogin writes the agent channel; the C# checks are advisory, since this function holds the row's lock — covered byMcpRouteAgentFieldsLiveTests.AnOperatorToken_SetsLevel3_AndChangesARouteAtLevel3_EachJournaled
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 as a standing cost, not a defect here — the function is copied verbatim into
DarlingManagedRoles.cs:2766andprovision-roles.sql:1142, and its journal JSON is a third copy ofNotificationRouteWriter.Describe; the diff keeps all three in step, as the three existing route functions already do. - "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 — Lite ships no route-authoring MCP tool to match (
grep create_notification_route Lite/Mcp/*.csis empty), andLite/Mcp/McpAlertTools.cs:606-611keepsAgentRouteRefusalfor the only three route tools Lite serves, none touched here. - "Ruling (#5833): tests are load-bearing. … never answer a trap with a test that reads source text or asserts how code is spelled" — applies —
McpRouteAgentFieldsLiveTests.cs:197pinsAssert.Equal(3, executes.Count)over a prefix match onViewerGrantReplay.BatchStatements(); commented inline as non-blocking, since the fixture does not need the count. - "A test that encodes the lie … Ask what defect a test would fail ON" — applies in the author's favour —
McpRouteAgentFieldsLiveTests.cs:153is what caught the DL403 defect: it asserts the claimed capability rather than the code's behaviour. - "
hours_back,limit,top,days_backare refused when out of range, never clamped" — does not apply, and is honoured — every agent limit is refused, never clamped (RouteAdmin.cs:494, and again under the lock atDarlingManagedRoles.cs:2811-2822);McpRouteAgentFieldsLiveTests.cs:118-123assertsagent_max_per_hour: 500answersinvalidand leaves the stored 5.
Divergence (blind read vs PR body)
The two accounts agree everywhere except on how far the store functions refuse, and that gap is the finding.
The body says the functions "still refuse a match, scope or destination change on an agent route" and that part 2 "opens only the agent fields" — a reading under which an operator's non-agent columns keep working as today. The functions do not say that: config.update_notification_route refuses the row outright at agent_level >= 1 before it reads a column name. So the bypass at RouteAdmin.cs:245 does not open the agent fields beside the old behaviour; on a route at level 1 or more it turns every mixed update into a DL403 that writes nothing, with a message ("it is changed in the Viewer, not over MCP") that contradicts the tool's own new description. The body then cites TheLegacyWritesToken_… as proof those other fields "are no longer refused" — the diff refuses them.
Two smaller divergences, posted as comments: the new paragraph at DarlingMcpAlertTools.cs:969-970 asserts "level 3 stays person-only", contradicted by the body's own level-3 decision and by the function's (0, 1, 2, 3); and Darling/README.md:867 still tells operators "an agent route is changed only in the Viewer" and names three store functions where there are now four, which the body does not mention.
I looked for a widening the body might have understated and found none: delete_notification_route, set_notification_route_enabled and set_notification_route_fallback still call AgentRouteRefusal with no actor, match/scope/destinations stay refused on any route with an agent setting, and level 3 buys no server action without config.config_server_actions.
Reviewer checklist
- Run
Darling PostgreSQL testswithDARLING_TEST_PGset and readTheLegacyWritesToken_SetsLevels1And3_AndEachIsJournaled_ButAnInvestigationsTokenSetsNone's third assertion — it decides whether the fix belongs in the function's DL403 or in the C# refusal. - Record revert proofs 1 to 5 from the body; none has run, so no claim about what each test fails on is confirmed yet.
- On a self-managed store, re-run
tools/provision-roles.sqland confirmconfig.set_notification_route_agentexists with EXECUTE tomcpbefore an operator token calls the tool — without it the call lands inMcpHelpers.FormatError(42883 is not a DL code) rather than in a sentence an operator can act on. - With a real MCP client, confirm the journal actor is
operator:<name>for a named operator token andoperator:token <8 hex>for the legacy one —ReadCaller.AmbientMcpTokenis set only by the tool filter.
…ames a match change (#6542 part 2, review round 1) M2: config.update_notification_route and config.set_notification_route_destinations take the actor the service passes. They change a route with an agent setting only for an operator: actor and refuse every other caller with DL403, as set_notification_route_agent does. The old signatures are dropped in the managed batch and in tools/provision-roles.sql, so neither stays callable. set_notification_route_enabled and set_notification_route_fallback hand an operator token's change to an agent route to the same path. L2: update_notification_route returns the row before the change, and set_notification_route_agent journals it as the before, so one call that changes metric_match and agent_level journals both, as the Viewer does. L3: a theory over the kill and PostgreSQL kill refusals made before the gate, and one tools/call through the host with a named agent token, each asserting the journal row names the token. L1 and M1: the test comment and the ServerActionGate comment now say what holds.
…used (#6542 part 2, review round 1)
… review round 1) "v_changed || 'agent_level'" reads the literal as a text array and fails with "malformed array literal", so every journaled agent change rolled back. Checked against a throwaway PostgreSQL 16 container.
|
Follow-up review of the 3 commits in The M2 ( L2 ( L3 (new identity-journaling tests in No defects found in these three commits. The PR body's claims about this round (M2, L2, L3, and the array_append discovery) all match the diff. |
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — "set_notification_route_agent appends with array_append... fails with 'malformed array literal'" — confirmed the prior
v_changed || 'agent_level'pattern in the base commit and the fix toarray_append(v_changed, 'agent_level')in bothDarlingManagedRoles.cs:2854-2865andtools/provision-roles.sql(same text, parity held) - VERIFIED — "M2: config.update_notification_route and config.set_notification_route_destinations take the actor the service passes... The old signatures are dropped" —
DROP FUNCTION IF EXISTSlines match the exact prior signatures from commit5a20f5b, and every GRANT/REVOKE arity was updated to match (9/11/8 params respectively) - VERIFIED — "set_notification_route_enabled and set_notification_route_fallback hand an operator token's change to such a route to the same path" — both call
UpdateNotificationRouteAsyncdirectly whenexisting.AgentLevelis set andserved?.Policy.OperatorActoris present (DarlingMcpAlertTools.cs:792,:859), argument counts matchUpdateNotificationRouteAsync's 16-parameter signature - VERIFIED — "L2: update_notification_route returns the row before the change, and set_notification_route_agent journals it as the before, so one call that changes metric_match and agent_level journals both" — traced the
before/p_beforeplumbing end to end (RouteAdmin.cs:367throughset_notification_route_agent'sp_beforehandling) and the new testOneCall_ChangingTheMatchAndTheLevel_JournalsBoth_WithTheOldMatchInBeforeasserts exactly this - VERIFIED — "L3: a theory over the kill and PostgreSQL kill refusals made before the gate... each asserting the journal row names the token" — new tests in
AgentTokenIdentityGateLiveTests.cscall existing, unchangedRunKillAsync/RefuseBeforeGateAsyncpaths that already threadagentTokenName(pre-dating this range), so these are regression tests for already-correct behavior, consistent with "no production file inDarlingMcpPlanForceTools.cschanged in this diff" - UNVERIFIED — "Checked against a throwaway PostgreSQL 16 container" and "CI's Windows build and Darling PostgreSQL tests are the only full runs" — a live DB/CI run cannot be confirmed from a read-only tree
Riskiest lines
Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs:2854-2865— thearray_appendfix itself; a regression back to||silently breaks every agent-route journal write — covered byOneCall_ChangingTheMatchAndTheLevel_JournalsBoth_WithTheOldMatchInBeforeandAnOperatorToken_ChangesSendResolutionsAndEnabled_OnALevel3Route_AndTheStoreRefusesAnyOtherActor(both exercise thev_changedappend paths and assert the journal row exists)Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.RouteAdmin.cs:333,398—operatorActor!null-forgiving dereference insideif (agent.Any || journalsMatchOrFlag); a future change that letsjournalsMatchOrFlaggo true with a nulloperatorActorwould NRE — covered byAnOperatorToken_ChangesSendResolutionsAndEnabled_OnALevel3Route_...(exercises the enabled-only branch withagent.Anyfalse)Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs:2712,2718,2723— theNOT v_operatoradditions on the three agent-route DL403 guards inupdate_notification_route; a droppedNOT v_operatoreither direction either locks operators out or opens agent routes to any actor — covered byAnOperatorToken_ChangesSendResolutionsAndEnabled_OnALevel3Route_AndTheStoreRefusesAnyOtherActor(asserts both the operator success and the DL403 refusal forNULL/agent:actors)
Traps near this diff
- "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 follow-up only touches Darling-only files (Postgres SECURITY DEFINER functions, MCP route-admin tools, operator-token identity tests); Lite's
McpAlertTools.cs:606AgentRouteRefusalhas nooperatorActorparameter at all, confirming Lite never had the operator-exception feature this PR extends, so there is no counterpart to drift from in this range - Array-literal
||ambiguity (not a named trap inREVIEW-TRAPS.md, but the exact defect this round's third commit fixes) — applies to the two files it was fixed in and nowhere else was found using the samevar || 'literal'shape in this diff (checked bothDarlingManagedRoles.csandprovision-roles.sqlfor the pattern post-fix: none remain)
Divergence (blind read vs PR body)
None — the body's account of this round (M2, L2, L3, and the array_append discovery) matches the diff on every claim graded above.
Reviewer checklist
- Run the live-PostgreSQL
Darling.Testssuite (McpRouteAgentFieldsLiveTests,AgentTokenIdentityGateLiveTests) in CI to confirm thearray_appendfix and the new L2/L3 tests actually pass against a real server, since this tree is read-only
One conflict, in DarlingMcpHostGateLiveTests.cs: this branch's new NamedTokensThatCannotBeServed_WithNoLegacyToken_StopTheStart test is kept, and the source-text test TryStartServerAsync_ResolvesTheLoopbackOnlyToken, which dev removed in #6507, stays removed.
ScalarAsync turns a boolean into "True", so the enabled and send_resolutions asserts compared "true" with "True" and failed (Darling PG tests shard 1). Both reads now cast to text, as the test's other reads do. The send_resolutions read was after the failing line, so it had not run yet.
Moved out of this round: the create-path set_notification_route_agent call, the level-3 refusal and the legacy OperatorActor. They change tests this round's other reverts target, so they run in rounds 3 and 4. - DarlingManagedRoles.cs and provision-roles.sql: AND NOT v_operator dropped from the agent-route DL403 guard - DarlingMcpPlanForceTools.cs: RefuseBeforeGateAsync passes parsed.AgentId - DarlingMcpHostService.cs: return false after the named-token Critical line deleted - DarlingMcpAlertTools.RouteAdmin.cs: the update-path SetRouteAgentAsync passes no before - DarlingMcpPlanForceTools.cs: agentTokenName dropped at the bad-session, bad-login-time, Azure-no-database, PG-bad-backend-id and PG-pid-mismatch refusals
… (do not merge) - DarlingMcpWritePolicy.cs: the legacy-token branch of OperatorActor returns TokenName alone
…ields # Conflicts: # Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.cs
- DarlingMcpAlertTools.RouteAdmin.cs: CreateNotificationRouteAsync no longer calls SetRouteAgentAsync - DarlingMcpAlertTools.RouteAdmin.cs: AgentFieldRefusal refuses level 3 again - DarlingMcpWritePolicy.cs: OperatorActor returns TokenName alone for the legacy token - DarlingMcpPlanForceTools.cs: RefuseBeforeGateAsync journals parsed.AgentId, not agentTokenName - DarlingMcpHostService.cs: return false removed after the named-token Critical line - DarlingManagedRoles.cs and provision-roles.sql: AND NOT v_operator dropped from the agent-route DL403 guard - DarlingMcpAlertTools.RouteAdmin.cs: SetRouteAgentAsync call in the update path no longer passes before - DarlingMcpPlanForceTools.cs: agentTokenName dropped at the kill bad-session, bad-login-time, Azure database, PG bad backend id and PG pid mismatch refusal sites (cherry picked from commit 4e8b98c)
Moved out of this round: the create-path set_notification_route_agent call, the level-3 refusal and the legacy OperatorActor. They change tests this round's other reverts target, so they run in rounds 3 and 4. - DarlingManagedRoles.cs and provision-roles.sql: AND NOT v_operator dropped from the agent-route DL403 guard - DarlingMcpPlanForceTools.cs: RefuseBeforeGateAsync passes parsed.AgentId - DarlingMcpHostService.cs: return false after the named-token Critical line deleted - DarlingMcpAlertTools.RouteAdmin.cs: the update-path SetRouteAgentAsync passes no before - DarlingMcpPlanForceTools.cs: agentTokenName dropped at the bad-session, bad-login-time, Azure-no-database, PG-bad-backend-id and PG-pid-mismatch refusals (cherry picked from commit 8452948)
- DarlingManagedRoles.cs and provision-roles.sql: the agent-route DL403 block in update_notification_route deleted (cherry picked from commit e003cda)
… (do not merge) - DarlingMcpWritePolicy.cs: the legacy-token branch of OperatorActor returns TokenName alone (cherry picked from commit e246365)
- DarlingMcpAlertTools.RouteAdmin.cs: the create-path set_notification_route_agent call removed - DarlingMcpAlertTools.RouteAdmin.cs: AgentFieldRefusal refuses level 3 again (cherry picked from commit e692aa1)
Claude posting for Erik Darling. Stale review. It was written at 5a20f5b. Its two refuted claims (the store refused every change on an agent route, and the legacy-token test did not edit one) were fixed in review round 1 (M2). The bot's re-review 5625028501 at 5d6788b is LGTM. CI is green at 8b60a0b.
…ature #6593 added the actor to config.set_notification_route_destinations too, so the owner list still asked for its old 7-argument signature. Every entry in the list is now checked against the script's own definitions: each names a function the script creates and does not drop before step 8.
…ed (#6644) * Fix the store script's owner list for the two route functions #6593 changed Step 8 of provision-roles.sql (#6613) hands each definer-rights function to the owner of config.config_command. It named update_notification_route by its old 8-argument signature. #6593 dropped that signature and created a 9-argument one with the actor. The two PRs merged in that order without a CI run on both, so the script now stops at step 8 with "function ... does not exist". The list now names the 9-argument signature, and set_notification_route_agent, the definer-rights function #6593 added. * Step 8 also names set_notification_route_destinations by its new signature #6593 added the actor to config.set_notification_route_destinations too, so the owner list still asked for its old 7-argument signature. Every entry in the list is now checked against the script's own definitions: each names a function the script creates and does not drop before step 8.
…stores the agent fields (L5 of #6593) (#6639) update_notification_route takes clear_agent_level true for an operator token. It sets agent_level to NULL (Inherit) and keeps the four limits, as the Viewer's route window saves an Inherit. agent_level and clear_agent_level together are invalid. The store function already accepts a NULL level, so it is unchanged. The undo of an operator token's update_notification_route call now carries agent_level and the four limits, or clear_agent_level true when the level was Inherit. Agent fields that already hold the stored value are not written, so an undo changes only what differs. set_notification_route_enabled and set_notification_route_fallback hand an operator token's change on an agent route to update_notification_route, so their answers carry the same undo.
Claude posting for Erik Darling.
Part 2 of #6542. Closes #6542. Closes #6395. (The base is
dev, so close both issues by hand at merge.)What was wrong
A route's agent fields (agent level, the four limits, the fallback of an agent route) could be set only in the Viewer or on the web dashboard. An operator agent that needed to set them had no MCP path. On one store it wrote the route rows with store-owner SQL instead. That path skipped
NotificationRouteWriter.Validateand wrote noroute_agent_changedjournal row.What changed
All paths are under
Darling/.create_notification_routeandupdate_notification_routetakeagent_level(0 to 3),agent_max_per_hour,agent_max_per_day,agent_lease_minutesandagent_expiry_minutes. They take aDarlingMcpServedPolicyservice parameter, so the client cannot supply who it is. Only an operator-class token may use the agent fields: a namedoperatortoken, or the legacy single token servedmcp.allowWrites(Effective.IsOperatorandEffective.OperatorActor, new inDarlingMcpWritePolicy.cs). The fields are checked withNotificationRouteWriter.Validateover the route as it would be stored.fallback_route_idwas already a parameter. On an agent route, or on a route the same call gives agent fields, an operator token's fallback is written together with the agent fields.config.set_notification_route_agent(SECURITY DEFINER, pinned search_path, no dynamic SQL, EXECUTE tomcponly). It is inDarlingManagedRoles.BuildNotificationRouteFunctionsSqland intools/provision-roles.sql, with the same text. It reads the rowFOR UPDATE, checks the level, the bounds and the fallback again, writes the five agent columns and the fallback, and writes theroute_agent_changedjournal row in the same transaction. The row's actor isoperator:<token name>for a named operator token, andoperator:plus the token identity the route write log lines already carry (the fingerprint,token 1a2b3c4d) for the legacy token. It is never a token value. The function refuses any actor that is notoperator:plus a name of that shape. The journal detail has the same before and after shape as the Viewer's save. A new route is journaled only when it sets a level, and an edit only when a level or a limit changed, asNotificationRouteWriter.AgentChangedoes.config.update_notification_routeandconfig.set_notification_route_destinationstake the actor the service passes and refuse a route with an agent setting for any actor that is notoperator:. The old signatures are dropped.set_notification_route_enabledandset_notification_route_fallbackhand an operator token's change to such a route to the same path.delete_notification_routestill refuses an agent route for every caller.mcp.allowInvestigationswithmcp.allowWritesoff keeps today's behaviour: it is served no route write tool. An agent token still never sees or calls these tools (part 1, unchanged).McpTokenEntry.ResolveTokennow namesmcp.tokens[<name>].token(or.encryptedToken) in its error, notmcp.network.token.The level-3 decision
Erik, 10-23: MCP is full admin, so an operator token sets 0 to 3.
route_agent_changedunderoperator:<name>.AnAgentToken_ListsNoRouteAdminTool_AndACallToOneIsRefusedAsUnknown, unchanged).mcp.allowWritesis operator-class and sets the agent fields too (see the role table).config.config_server_actionsand a remediation login, whatever the route says.ServerActionGate.cschecks both. The switches are NOT person-only:set_server_action(DarlingMcpServerActionTools.cs) writes them for an operator-class token whenmcp.allowWritesandmcp.allowServerAdminare both on (allowServerAdminis off by default). So with both on, an operator token can set a route to level 3 and turn a server's switch on. No agent token reaches either tool (both are in NeverAgentTools).env:orfile:reference. Checked:edit_serverrefuses everyremediation_*key (DarlingMcpServerAdminTools.Edit.cs), no other MCP tool or web route writes those columns, and the store trigger onconfig_monitored_serversrefuses a reference (PW001) from every role but the owner, so only the configuration file sets one.Roles (agreed with the coordinator, 10-23)
Under R2 of #5632 the legacy token never holds both arms, so no investigation agent holds a token that can set route agent fields.
Overlap
fix/6403-previous-value-undo) editsDarlingMcpAlertTools.RouteAdmin.csandDarlingMcpAlertTools.cs. Expect a rebase on whichever merges second.previousandundo. When this PR is rebased onto it, the new fields (agent_level,agent_max_per_hour,agent_max_per_day,agent_lease_minutes,agent_expiry_minutesandfallback_route_id) must go intoupdate_notification_route'spreviousandundoas well, so that an undo restores them. TodayUndoArgumentsinDarlingMcpAlertTools.RouteAdmin.cscarries the fallback but not the agent fields. An undo by an operator token would then restore them through the agent function, and the ceiling would hold.The rung: none
No migration rung, so the schema stays at 214. The reasons:
create_notification_routeandupdate_notification_routebefore them (Darling MCP: notification routes can't be created or edited, alert history can't be dismissed, and there is no quiet window #6231). A self-managed store gets them by re-runningtools/provision-roles.sql, as it already does for those two functions.ck_config_notification_routes_agentCHECK already allows levels 0 to 3 and the limit bounds.update_notification_routeandset_notification_route_destinationstake a newp_actorargument, andset_notification_route_agenttakesp_before. Each old signature is dropped withDROP FUNCTION IF EXISTSin the same batch and intools/provision-roles.sql, so none stays callable. The only callers are the MCP route tools (DarlingMcpAlertTools.RouteAdmin.cs, throughNotificationRouteWriter.SetRouteDestinationsAsyncfor destinations). The Viewer and the web write routes withNotificationRouteWriter.SaveAsyncand plain table writes, and call none of these functions, so they are unchanged.tools/provision-roles.sql, the MCP route writes fail with undefined-function (42883) and roll back. They fail closed. The CHANGELOG entry tells them to re-run it.investigation_journal_owner_rows) refuses only the request event kinds and once keys other than its list.route_agent_changedis not a request event, and the function writes no once key. The trigger skips the owner, and the function runs as the owner. Themcprole's column INSERT grant on the journal is not used, because the definer writes the row. The live test runs with the trigger installed and the grants replayed from the batch.Items 7 to 10 of #6556's security review
agent_actionwith another agent'sagent_id, plus an argument the tool refuses early. It is easy to trigger, but it writes only audit noise, because the action is still refused. The fix is oneagentTokenName ?? agentIdat each journal site (RefuseEarlyAsync, the PostgreSQL target refusal in force and unforce,RefuseBeforeGateAsyncwith its seven callers, andLooseAgentIdin the gate). Test:AgentTokenIdentityGateLiveTests.APostgresTargetRefusal_*andEveryRefusalBeforeTheGate_*assert one row asagent:triage-botand none asagent:someone-else.AgentTokenIdentityGateLiveTests.TheLevel2Gate_*(DarlingAgentActionGate) andTheServerActionGate_*, a theory over force, unforce, evict and kill. Each asserts theagent_id_not_tokenrefusal, onerefusedrow under the token's name, and noaction_calledrow. No SQL Server is needed, because every refusal comes before a remediation login opens.DarlingMcpHostGateLiveTests.NamedTokensThatCannotBeServed_WithNoLegacyToken_StopTheStart_AndNothingAnswersdrives the realTryStartServerAsyncwith two named tokens under the length floor and no legacy token. It asserts that the start returns false, that nothing answers on the port, that the Critical line was logged, and that no token value was logged.mcp.network.token. Fixed, with the testDarlingMcpNamedTokenTests.AnUnreadableNamedToken_NamesItsOwnSetting_NotTheLegacyOne.claim_idonly. Considered, not added. Trigger: agent token B must present theclaim_idof agent A's live claim. Aclaim_idis a random GUID that is returned only to the claimer, and no read tool lists it for another agent. Likelihood: very low, because it needs a leaked claim id. The fix is a name check in two more tool paths and their tests.agent_id. Considered, not added. That is a supported setup the operator chooses, and the start already logs a Critical line for it. Refusing it would change the legacy token's behaviour, which the brief keeps exact.Effective.Offersfor everyallowWritesandallowInvestigationscombination. This PR does not change the legacy policy, and the legacy route test below shows that.Tests, with revert proofs
McpRouteAgentFieldsLiveTests(live PostgreSQL, runs as themcprole with the batch's own grants):AnOperatorToken_CreatesARouteAtLevel1_AndTheJournalNamesTheTokenAnOperatorToken_SetsLevel3_AndChangesARouteAtLevel3_EachJournaledTheLegacyWritesToken_SetsLevels1And3_AndEachIsJournaled_ButAnInvestigationsTokenSetsNone, which includes the legacy token'ssend_resolutions: "always"step on a level 3 route.AnOperatorToken_ChangesSendResolutionsAndEnabled_OnALevel3Route_AndTheStoreRefusesAnyOtherActorOneCall_ChangingTheMatchAndTheLevel_JournalsBoth_WithTheOldMatchInBeforeAgentTokenIdentityGateLiveTests(live PostgreSQL): items 7 and 8, plus newEachKillRefusalBeforeTheGate_IsJournaledUnderTheTokensName(theory, 5 cases) andAKillCallThroughTheHost_WithANamedAgentToken_JournalsTheTokensName.DarlingMcpHostGateLiveTestsandDarlingMcpNamedTokenTests: items 9 and 10c.McpRouteRoleLiveTests,McpRouteDestinationsLiveTestsandMcpRouteDestinationAnswersLiveTestscall the functions directly with a NULL actor and keep their refusals, so the non-operator path keeps its pins.Revert proofs. They ran in CI after the merge, on four throwaway PRs that were then closed (results below). Each guarded change and the test that must fail without it:
set_notification_route_agentcall inCreateNotificationRouteAsyncAnOperatorToken_CreatesARouteAtLevel1_*(stored level, journal count)AgentFieldRefusalAnOperatorToken_SetsLevel3_*(created)OperatorActorfor the legacy tokenTokenNamealoneTheLegacyWritesToken_*(created)agentTokenNameinRefuseBeforeGateAsyncparsed.AgentIdEveryRefusalBeforeTheGate_*(0|1)return falseafter the named-token Critical lineNamedTokensThatCannotBeServed_*(started)NOT v_operatoron the agent-route DL403 inupdate_notification_routeAND NOT v_operatorAnOperatorToken_ChangesSendResolutionsAndEnabled_*andTheLegacyWritesToken_*(refused, notupdated)AnOperatorToken_ChangesSendResolutionsAndEnabled_*(no DL403 for NULL andagent:) andMcpRouteRoleLiveTestsagent-route refusalupdate_notification_routeUpdateNotificationRouteAsyncAnOperatorToken_ChangesSendResolutionsAndEnabled_*(refused)p_beforefrom the update functionSetRouteAgentAsyncOneCall_ChangingTheMatchAndTheLevel_*(reason and before match)array_appendin the agent functionv_changed || 'agent_level'McpRouteAgentFieldsLiveTests(malformed array literal)agentTokenNameat the kill bad-session siteEachKillRefusalBeforeTheGate_*(bad_session)andAKillCallThroughTheHost_*EachKillRefusalBeforeTheGate_*caseserved?.Policy.AgentTokenNamein theKillSessionwrapperAKillCallThroughTheHost_*(0|1)agentTokenNamepassed toRunKillPostgresAsyncEachKillRefusalBeforeTheGate_*(pg_bad_backend_id)and(pg_pid_mismatch)Revert proof results. Four throwaway PRs ran in CI, on this PR's final head with some changes reverted. Each PR was closed without merge.
agentTokenNameinRefuseBeforeGateAsync, thereturn false,AND NOT v_operator,p_before, and the five kill sitesEveryRefusalBeforeTheGate_*,NamedTokensThatCannotBeServed_*,AnOperatorToken_ChangesSendResolutionsAndEnabled_*,TheLegacyWritesToken_*,OneCall_ChangingTheMatchAndTheLevel_*,AKillCallThroughTheHost_*and all fiveEachKillRefusalBeforeTheGate_*cases.UpdateNotificationRouteAsync, theKillSessionwrapper, andRunKillPostgresAsync's argumentAnOperatorToken_ChangesSendResolutionsAndEnabled_*,AKillCallThroughTheHost_*, andEachKillRefusalBeforeTheGate_*(pg_bad_backend_id)and(pg_pid_mismatch).OperatorActorAnOperatorToken_ChangesSendResolutionsAndEnabled_*andTheLegacyWritesToken_*failed.McpRouteRoleLiveTestsdid not fail. Its agent-route refusal comes from the MCP layer before the store, so that row's second test does not depend on the DL403 block. The first test proves the block.set_notification_route_agentcall, and a level 3 refusal put backAnOperatorToken_CreatesARouteAtLevel1_*andAnOperatorToken_SetsLevel3_*.The
array_appendrow was proven earlier.Every failure above is an assertion failure. Rounds 1, 3 and 4 also failed the bring-your-own-script grant tests. That came from dev, not from the reverts: this PR and #6613 merged without a CI run on both. #6644 fixes it.
What ran where:
Darling.Testsbuilds with 0 errors (-p:EnableWindowsTargeting=true).Darling.Teststargetsnet10.0-windowsand cannot run on macOS.tools/provision-roles.sqlover a minimal table: the old signatures are gone, a match plus level call journals "changed metric_match, agent_level" with the old match in before, an operator actor changessend_resolutionsandenabledon a level 3 route, NULL andagent:actors get DL403, and a non-operator agent actor gets DL400. That run also found that the head commit'sv_changed || 'agent_level'fails with "malformed array literal", so every journaled agent change rolled back. Fixed witharray_append.buildandDarling PostgreSQL testsare the only full runs of these tests. See the CI state below.Review round 1
The Opus security review found no path for an agent-class token. Its findings and the fixes:
IsOperator) is the real gate. In the store the DL403 guard is now an actor check, the same shapeset_notification_route_agentuses.set_server_actionwrites the switches for an operator token whenmcp.allowWritesandmcp.allowServerAdminare both on. The "Next:" line is gone, and the comment inServerActionGate.csis corrected. The remediation login is the remaining person-only barrier, and that claim was checked (see The level-3 decision).mcprole, so anymcpsession can passoperator:anyone. The SQL actor check is a label, and the C# gate is the real one. The test comment now says so. No code: no MCP path to raw SQL was found. It does mean the database no longer stops themcprole from making an agent route, so a future SQL-execution bug on the MCP surface would reach level 3.update_notification_routereturns the row before the change, and the agent function journals it as before and namesmetric_match,scope_server_id,scope_tag_idandenabledas the Viewer does. An operator's match, scope or enabled change to a route with an agent setting is journaled even with no agent field. Test:OneCall_ChangingTheMatchAndTheLevel_*.tools/callthrough the host.v_changed || 'agent_level'failed at runtime. Fixed (see What ran where).Considered, not added:
OperatorActorreadingoperator:token unknownifTokenIdentityOfthrows (no trigger named), andp_createdorp_beforepassed by raw SQL asmcpto fake or hide a journal row (needs raw SQL asmcp, which no MCP path gives).Checked after the review, by a code read at this head (no test added, because each is a fact about the current code, not a change):
claim_id. Onlyclaim_investigation_requestreturns one. It goes to the caller that claims, or back to the same named token on a reclaim. A claim by the agent holding the live lease returns its claim_id (#6576) #6586 allows a reclaim only on a named token. Thelease_expiredjournal row keeps the claim id only inonce_key, and no read selects that column.ParseAgentActionrefuses that JSON at the evidence check ("missing"). The request id, claim id, agent id and reason in it all parse. So the case reaches the gate's parse-refusal branch, which journalspolicy.AgentTokenNamebefore it falls back to the loose agent id.Handoff
devcompiles again since A request's brief names the earlier report with the same root fact under another story hash (#6565) #6582.8ad056afbmergesdevinto this branch. Its CI round failed 1 test of 5,171, and the fault was in the test:AnOperatorToken_ChangesSendResolutionsAndEnabled_*read a boolean column back as "True" and compared it with "true".f022441fcreads both boolean columns as text, as the test's other reads do. CI runs on that head.McpRouteAgentFieldsLiveTestsfacts answerederroron create (the agent function'smalformed array literal, nowarray_append), andEveryConvertedHead_StaysAtOrUnder620Charactersreadupdate_notification_route: 679. That head is 584 characters of head text here (648 at 5a20f5b), plus the 31-character guide pointer, so 615.create_notification_routeis 545 plus 31. Lite has no route admin tools, so no Lite head shares these names.CHANGELOG entry
Darling MCP: an operator token (a named operator token, or the single token with
mcp.allowWrites) can set a notification route's agent level (0 to 3), its limits and its fallback throughcreate_notification_routeandupdate_notification_route. The Viewer's rules check each change, and aroute_agent_changedjournal row names the token. An operator token also changes the match, scope, enabled flag, resolution override, fallback and destinations of an agent route. Refusal rows from an agent token are journaled under the token's name (#6542). Re-run tools/provision-roles.sql after upgrading if you provision the store's roles yourself.