[add] MCP tools for the alerting engine - #113
Merged
Merged
Conversation
list_alerts, active_alert_conditions and acknowledge_alert. The first two are separate deliberately: now that an alert is a condition with a lifetime, 'what happened' and 'what is wrong now' are different questions, and an assistant that asks the first when it meant the second reports a problem that resolved an hour ago. Host scoping goes INTO the query. An aggregate read cannot be authorised by checking one host_id — with no host argument, list_alerts would otherwise return every host's alerts to a token scoped to one. Two findings from writing the tests: - scopedHostIDs missed admins. They bypass roles, so they have no grants and ReachableHosts reports no hosts for them — which would have hidden every remote alert from the one principal allowed to see all of them. The REST side checks IsAdmin first for exactly this reason. - The first pentest asserted on the helper and the store query, so deleting the scoping from listAlerts itself changed nothing and the test still passed. It now calls the tool.
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MCP knew nothing about alerting. Three tools close that:
list_alerts— the history, with the same filters the UI has (severity,lifecycle kind, container, rule, message text).
active_alert_conditions— what is over threshold right now, and for howlong each has been going.
acknowledge_alert— record that a human has seen one.The first two are separate on purpose. Now that an alert is a condition with a
lifetime rather than a line reprinted every cycle, "what happened" and "what is
wrong now" are different questions. An assistant that asks the first when it
meant the second will confidently report a problem that resolved an hour ago, so
the tool descriptions say which is which and point diagnosis at the live one.
Type of change
Checklist
go test -short ./...andgo vet ./...passgofmtgate is clean (gofmt -l $(git ls-files '*.go')after staging)web/dist— N/Adocs/and added aCHANGELOG.mdentryNotes for reviewers
The security shape here is one the existing systemic tests could not catch.
TestEveryToolConsultsTheAccessGateandTestEveryToolRespectsTokenScopecalleach tool with an explicit
host_id.list_alertsis an aggregate: called withno host argument, the gate is consulted once, passes (the local daemon is always
in reach), and the result set would span every host. So host scoping goes into
the query via
AlertQuery.HostIDs, the same way the REST feed does it, and thisPR adds its own pentests.
Two things the tests found, both worth stating:
scopedHostIDsmissed admins. They bypass roles, so they have no grants,and
ReachableHostsreports "no hosts" for them — which would have hidden everyremote host's alerts from the one principal allowed to see all of them. The REST
side checks
IsAdmin()beforeReachableHostsfor exactly this reason; I hadnot. Caught because a test failed for a reason I first assumed was the test's
fault — it was half the test's fault and half a real bug.
The first version of the pentest was vacuous. It asserted on
scopedHostIDsand on a store query built by the test itself, so deleting thescoping from
listAlertschanged nothing and it still passed. It now calls thetool, and fails under that mutation with
SECURITY: list_alerts returned an alert from an unreachable host. That is thefifth vacuous test caught by mutation testing in this line of work — the pattern
is always the same: the test restates the logic instead of exercising the caller.
acknowledge_alertis a write even though it changes nothing about acontainer. Acknowledging is an attributed claim that somebody looked, so a
read-only user or token is refused, and it is audited as
mcp.alert.ack.Not included: anything that would widen the MCP surface — no rule editing, no
silencing. Both are roadmap items and both deserve their own decision.