Skip to content

feat: per-server annotation overrides for misbehaving upstreams - #1323

Open
DScoNOIZ wants to merge 15 commits into
smart-mcp-proxy:mainfrom
DScoNOIZ:🚀-FEAT-ANNOTATION-OVERRIDES-🛡️-PER-SERVER-TOOL-EXCEPTIONS-✨

Hidden character warning

The head ref may contain hidden characters: "\ud83d\ude80-FEAT-ANNOTATION-OVERRIDES-\ud83d\udee1\ufe0f-PER-SERVER-TOOL-EXCEPTIONS-\u2728"
Open

DScoNOIZ wants to merge 15 commits into
smart-mcp-proxy:mainfrom
DScoNOIZ:🚀-FEAT-ANNOTATION-OVERRIDES-🛡️-PER-SERVER-TOOL-EXCEPTIONS-✨

Conversation

@DScoNOIZ

Copy link
Copy Markdown

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.

@codecov-commenter

codecov-commenter commented Sep 21, 2026 •

Copy link
Copy Markdown

@DScoNOIZ DScoNOIZ changed the title per-server annotation overrides for misbehaving upstreams feat: per-server annotation overrides for misbehaving upstreams Sep 22, 2026
AGI Developer and others added 3 commits September 23, 2026 02:20
- 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)
@Dumbris

Dumbris commented Oct 6, 2026

Copy link
Copy Markdown
Member

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):

  • Merged current main and resolved the 13 conflicts. They were mostly keep-both merges with the new Spec 112 forward_headers and Spec 108 profiles code. I also regenerated oas/ with make swagger.
  • Removed the root PLAN_IDEAL_ANNOTATION_OVERRIDES.md. The design notes are better kept in the PR description.
  • Both editions build, go test -race -tags server passes on all touched packages, the editor vitest passes (18/18), and vue-tsc is clean.

Please git pull before continuing.

Changes requested

  1. Overrides should have one source of truth (P1). Right now the override is applied in two places. It is baked into tool metadata in core.Client.ListTools, and it is also re-applied in some readers (lookupExactToolAnnotations, lookupToolGate, preflight, direct catalog). The main call_tool_* gate reads gate.identity.Annotations directly (internal/server/mcp.go, in handleCallToolVariant right after evaluateExactToolGate), and so does the code_execution profile gate (profile.IntrinsicTier(identity.Annotations, …) in mcp_code_execution.go). When the snapshot lags the config, the listing, the token tier and the profile tier can disagree. That happens if RefreshServerTools fails, or after a hot reload of mcp_config.json.

  2. Removing an override can fail open (P1). The snapshot already holds the overridden hints, and the overlay treats them as the "upstream" base. So removing a permissive override (for example readOnlyHint:true on a tool that is destructive upstream) keeps the tool at read tier until a re-list succeeds. A refresh failure is only logged.

    • Suggested fix for 1 and 2: keep the raw upstream annotations in the snapshot and index, and resolve the effective annotations in one place, the shared identity resolver/gate (resolveExactToolIdentity / evaluateExactToolGate). Every tier decision then reads that single result: listing, call_with, token scope, profile max_tier, preflight and the direct catalog.
  3. Transport edits no longer reconnect (P1, affects every server). Server.UpdateServer now calls client.SetConfig(existing) unconditionally before reconcile. upstream.Manager.AddServerConfig then compares the incoming config against the config that was just swapped in. It sees no change and keeps the old core client. As a result, a PATCH that changes url, command, args, env, headers, enabled or quarantined keeps the old transport running. Please push only the hot field, for example a copy of client.GetConfig() with just AnnotationOverrides replaced, or leave it to the reconcile path. A regression test for a URL change via UpdateServer would be great.

  4. Ambiguous null markers (P2). The flattened remove markers annotation_overrides.<tool>.<hint> can't tell a per-hint null apart from a whole-tool null when the tool name itself is foo.readOnlyHint, which is a valid key under the current name rule. Either keep the two marker kinds structurally distinct, or disallow tool keys that end in .<hintName>.

One smaller note: Spec 108 profiles now have tools.classify, which gives unannotated tools a tier in a single profile. Your overrides complement this nicely because they fix mislabelled tools globally. It would be worth one sentence in docs/configuration/config-file.md explaining when to use which.

Thanks again. Happy to answer questions on the identity/gate code; it's one of the denser parts of the codebase.

@Dumbris Dumbris left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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!

This branch has not been deployed

No deployments
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.

3 participants