Repository navigation
feat: per-server annotation overrides for misbehaving upstreams - #1323
Hidden character warning
DScoNOIZ wants to merge 15 commits into
Conversation
…G + OAUTH GUARD 🔒
…T + DISPATCH FRESH 🔒
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- httpapi: remove redundant nil check before len (S1009) and simplify restartRequired assignment (QF1007) - config: remove unused helpers annotationsEqual/boolPtrEqual - server: remove unused test helpers servedHints/boolVal - server: fix UpdateServer data race by copy-on-write for runtime Config snapshot (mirrors configWithAppendedServer pattern)
|
Thanks a lot for this, @DScoNOIZ. It's a careful and thorough piece of work. The browseros case is a real pain point, and the admin-only gating, audit lines, hot-reload path and test coverage are all well done. We'd like to land it. A cross-model review plus my own reading of the code turned up a few issues on the security-tier path that need fixing first. What I did on your branch (pushed as the owner; your commits are untouched, nothing was force-pushed):
Please Changes requested
One smaller note: Spec 108 profiles now have Thanks again. Happy to answer questions on the identity/gate code; it's one of the denser parts of the codebase. |
Dumbris
left a comment
There was a problem hiding this comment.
Requesting changes per the comment above: one source of truth for effective annotations (P1 x2) and the UpdateServer transport-reconnect regression (P1). The feature is otherwise solid. Thank you!
Hi, ran into this with a couple of old mcp servers, browseros in my case. They dont send any annotations at all, so all their tools fall into destructive tier by default. Readonly agents cant even see plain readers like navigate or snapshot, even though theres nothing dangerous in them. Patching every upstream by hand is obviously not an option, so I made operator side overrides.
How it works, theres a new optional map annotation_overrides in the server config. Key is the tool name or a star for all of them at once, value is annotations. Merges per hint on top of what the server sent, priority is straightforward, exact then wildcard then upstream. Whatever is not set is inherited. You can delete with null by key, same as everywhere else in the config. Only admin can change it, both rest and mcp. No restart needed, applies right away on the next get. Written to audit as config_change. Left the tool approval hash alone, its got no business there.
On the ui side I put a card into the server configuration right after trust mode. Theres a compact table plus a mark safe button per tool, it just presets readonly true destructive false and thats it. It never touches openWorldHint by itself, thats manual only if youre really sure. Left the selects three state inherit true false, cause a toggle just cant express inherit.
Compat wise all quiet. Field is omitempty everywhere, old configs load like they used to, without overrides behavior is exactly like before. Covered merge priority validation and admin only with tests. Docs added in config-file and upstream-servers, swagger regenerated.
Tested live on browseros, 24 tools there and 5 with no annotations at all. Readonly token used to see zero browseros tools, after the override navigate is found and called through the read door. Deleted the override and everything flipped right back. If something doesnt fit or needs splitting up, let me know, will rework.