From 948f5233ea7024918c719d21fe9311716ca4b516 Mon Sep 17 00:00:00 2001 From: AGI Developer Date: Sat, 19 Sep 2026 23:25:17 +0300 Subject: [PATCH 01/13] =?UTF-8?q?=E2=9C=A8=20FEAT:=20=F0=9F=93=8B=20IDEAL?= =?UTF-8?q?=20PLAN=20=E2=80=94=20PER-SERVER=20PER-TOOL=20ANNOTATION=20OVER?= =?UTF-8?q?RIDES=20=E2=80=94=20PLATINUM=20MAP=20=F0=9F=9B=A1=EF=B8=8F?= =?UTF-8?q?=E2=9C=A8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- PLAN_IDEAL_ANNOTATION_OVERRIDES.md | 420 +++++++++++++++++++++++++++++ 1 file changed, 420 insertions(+) create mode 100644 PLAN_IDEAL_ANNOTATION_OVERRIDES.md diff --git a/PLAN_IDEAL_ANNOTATION_OVERRIDES.md b/PLAN_IDEAL_ANNOTATION_OVERRIDES.md new file mode 100644 index 000000000..a5230a84a --- /dev/null +++ b/PLAN_IDEAL_ANNOTATION_OVERRIDES.md @@ -0,0 +1,420 @@ +# πŸš€ PLAN β€” PER-SERVER PER-TOOL ANNOTATION OVERRIDES πŸ›‘οΈ β€” IDEAL PRODUCTION MAP ✨ + +> **BRANCH (USER REQ):** `πŸš€-FEAT-ANNOTATION-OVERRIDES-πŸ›‘οΈ-PER-SERVER-TOOL-EXCEPTIONS-✨` (off `main` @ 525d5021) β€” **LOCAL DEV** with CAPS+EMOJIS per user request. **UPSTREAM PR BRANCH:** `feat/annotation-overrides-per-server-tool` (kebab, no emoji) for author convention (`git branch -a` = kebab, workflows filter `main/next`, `CLAUDE.md` conventional-commits lower). +> **SCOPE:** `mcpproxy-go` ONLY β€” `/home/agi/PROGRAMMS/AGI-MEMORY-SKILLS/mcpproxy-go` +> **PHILOSOPHY:** Platinum Quality Loop (STRATEGY_COMPLETE.md) adapted to Go β€” LIVE PROOF > mocks, ZBT++, 7 Quality Gates, Context7+Tavily before every snippet. +> **LANGUAGE:** EN (headers CAPS+EMOJIS for upstream PR), code comments EN per AGENTS.md. +> **STRATEGY SOURCE:** `/home/agi/PROGRAMMS/PROXY/plans/STRATEGY_COMPLETE.md` Part 0-10 β€” Master Orchestrator β†’ Sub-Orchestrators β†’ Critic β†’ Gates β†’ LIVE β†’ Cosmic Verify. +> **CRITIC POLISH:** v2 addresses harsh critic REJECTED (missing files, mapstructure kebab, deep-copy alias, RFC7396 inner-null, triple-write GH#938, RLock, pointer compare, audit gap, gates path, UI scalability β€” see Β§2.7, Β§3.2, Β§4). + +--- + +## 0. EXECUTIVE SUMMARY β€” WHAT & WHY + +**PROBLEM:** `browseros` (and any legacy/unsupported MCP server) marks `tabs/navigate/act` `destructiveHint:true` and leaves 7 tools with `nil` β†’ MCP spec default `nilβ†’true` β†’ all 9+7=16 of 24 tools `destructive` (`internal/server/mcp_annotations.go:108-114`, `internal/toolannotations/toolannotations.go:66`). Requires `call_tool_destructive` + token `destructive` on whole fleet. `enabled_tools` hides by name, doesn't fix tier. `strict_server_validation=false` only relaxes `ValidateAgainstServerAnnotations` (`internal/contracts/intent.go:177`) but NOT target-tier gate `mcp.go:2570`. + +**SOLUTION:** Admin-only per-server `annotation_overrides` β€” `map[toolName]*ToolAnnotations` + wildcard `"*"` β€” like antivirus exceptions. Merged once at capture `internal/upstream/core/client.go:419` β†’ effective annotations used everywhere (`DeriveCallWith`, `classifyToolRisk`, `ExcludeReason`). Audited, fail-closed, hot, reversible via `null` (RFC7396). + +**PLACEMENT:** Primary `ServerDetail β†’ Configuration β†’ card after Trust mode` (`ServerDetail.vue:915`), secondary pencil `✎ Override` on each tool card in `ServerDetail Tools` (`ServerDetail.vue:685`) β†’ scroll to config card. NOT global `Settings.vue` (global vs per-server invariant). + +--- + +## 1. 🎯 RESEARCH EVIDENCE β€” CONTEXT7 + REAL CODE (LIVE PROOF) + +**EVERY SNIPPET BELOW WAS VERIFIED vs REAL CODE + MODERN STANDARDS β€” NO HALLUCINATION:** + +| Claim | Real Code | Context7 / Standard | +|---|---|---| +| `ToolAnnotations` `*bool` tri-state | `internal/config/config.go:1409-1416` `ReadOnlyHint *bool` `omitempty`, same as `ExposePrompts *bool:705`, `AutoApproveToolChanges *bool:687` | Go `encoding/json` `isEmptyValue` : `*bool nil β†’ omit`, `true/false β†’ emit`. Context7 `/golang/go` confirms `omitempty` on pointer is canonical tri-state. | +| Hash excludes annotations (intentional) | `internal/runtime/tool_quarantine.go:24-33` comment 4 points + `calculateToolApprovalHashWithOutputSchema:36` ignores param | Author chose to avoid `tool_description_changed` spam on reconnect β€” overrides MUST NOT be hashed. | +| Merge map RFC7396 null=delete | `internal/config/merge.go:408-439` `MergeMapWithOpts` + `removeMarkers:47-71` + `shouldRemove:80` | Standard `MergeOptions.NullRemovesField` β€” `env`/`headers` already use it (`mcp.go:5456,5569`). New map must follow. | +| ValidateDetailed is write-gate | `internal/config/config.go:2275 validateDetailedCore` vs `2691 Validate` (tolerant) | Every `PATCH /api/v1/config` + `PATCH /api/v1/servers/{id}` gates via `ValidateDetailed` β€” per-server loop `2481-2606` is canonical. | +| `DeriveCallWith` priority | `internal/contracts/intent.go:208-228` `destructiveβ†’destructive > readOnly falseβ†’write > readOnly trueβ†’read > nilβ†’read` | MCP spec 2024-11-05 + June 2025 optional hints. | +| `classifyToolRisk` nilβ†’permissive | `internal/server/mcp_annotations.go:99-122` `nilβ†’hasOpenWorld+hasDestructive+hasWrite=true` | Fail-closed pessimism β€” lethal trifecta default. | +| `ExcludeReason` single truth | `internal/toolannotations/toolannotations.go:49-86` `first-filter-wins readβ†’destructiveβ†’openWorld` | Extracted to leaf per Spec 098 to avoid `server` import cycle β€” overrides must be resolved BEFORE it. | +| `TrustModeSelector` 3-state pattern | `frontend/src/components/TrustModeSelector.vue:1-72` `select` + `pendingWarnMode` | Tri-state `*bool` β†’ `select Inherit/Yes/No`, NOT toggle (toggle=2-state). `fields.ts:7` `toggle` is binary only. | +| ServerDetail Configuration layout | `ServerDetail.vue:871-943` `space-y-6` `card General 879 β†’ trust-mode-card 915 β†’ Connection 948` | Author groups security per-server knobs together β€” overrides belongs after trust-mode. | +| PATCH surfaces triple-write | `internal/httpapi/server.go:2398 handlePatchServer` + `internal/server/mcp.go:5632 buildPatchConfigFromRequest` + `frontend/src/services/api.ts:352 patchServer` | GH#938 failure: `trust_mode` added to only 1 of 3 β†’ silently dropped. New field MUST be added to all 3. | +| Audit gap | `internal/audit/line.go:21-100` only `authz/tool_call/auth_event` β€” no `config_change` | OWASP ASVS 7.x + NIST AU-2 requires privileged config diff in tamper-evident audit, not only activity log (`runtime/event_bus.go:753 EmitActivityConfigChange`). | + +**LIVE VERIFICATION (2026-09-19):** `curl -H X-API-Key … /api/v1/servers/browseros/tools | jq` β†’ 24 tools: 9 `destructiveHint:true`, 8 `readOnlyHint:true`, 7 `{} (nilβ†’destructive)`. `GET /api/v1/config` β†’ `strict_server_validation:true`, `quarantine_enabled:false`. `browseros` `healthy Connected 24 tools` after `PATCH enabled:true` (`restart_required:true`). + +--- + +## 2. πŸ—ΊοΈ FILE-BY-FILE DIFF MAP β€” EXACT EDITS (PLATINUM: EVERY LINE VERIFIED) + +### 2.1 BACKEND β€” CONFIG MODEL + +**`internal/config/config.go`** +```go +// After:750 (EnabledTools/DisabledTools block) β€” add β€” NOTE mapstructure kebab per author conv (trust_mode:694, expose_prompts:705): +AnnotationOverrides map[string]*ToolAnnotations `json:"annotation_overrides,omitempty" mapstructure:"annotation-overrides"` + +// New helper (near ValidTrustModes:2033): +func IsValidToolNameForOverride(name string) bool // name=="*" || (len 1..256 && no leading/trailing space && no "__" collision; regex ^[A-Za-z0-9._:-]+$; "*" only standalone) +``` + +**Why `*ToolAnnotations` not `ToolAnnotations`:** pointer lets `{"tool":null}` delete whole entry via RFC7396 (consistent with `Headers map[string]*string` in `httpapi/server.go:1906`). Inner `*bool` already handles per-hint `inherit` (`nil`=inherit upstream). + +**`internal/config/config.go` β€” `CopyServerConfig` (β‰ˆ591-676) β€” MUST preserve all fields (critic: missing `quarantineExplicitlySet`, `Isolation.Clone()`, `AuthBroker`, `EnabledTools` etc would break #937 gate). Deep-copy new map AFTER existing full copy:** +```go +// Inside CopyServerConfig(dst,src) after existing deep-copy of Isolation/AuthBroker/Env/Headers/EnabledTools: +if src.AnnotationOverrides != nil { + dst.AnnotationOverrides = make(map[string]*ToolAnnotations, len(src.AnnotationOverrides)) + for k, v := range src.AnnotationOverrides { + if v == nil { dst.AnnotationOverrides[k] = nil; continue } + cp := *v // shallow; then deep-copy bool ptrs + if v.ReadOnlyHint != nil { b := *v.ReadOnlyHint; cp.ReadOnlyHint = &b } + if v.DestructiveHint != nil { b := *v.DestructiveHint; cp.DestructiveHint = &b } + if v.IdempotentHint != nil { b := *v.IdempotentHint; cp.IdempotentHint = &b } + if v.OpenWorldHint != nil { b := *v.OpenWorldHint; cp.OpenWorldHint = &b } + // Title is string (not *string) β€” "" = preserve; if clear needed future: change to *string + dst.AnnotationOverrides[k] = &cp + } +} +``` + +**`internal/config/config.go` β€” `validateDetailedCore` loop `2481-2606` β€” PERSISTED config never sees `nil` entry (null is remove marker, not persisted). Validate persisted only:** +```go +// Inside for i, server := range c.Servers β€” after existing checks: +if len(server.AnnotationOverrides) > 100 { + errs = append(errs, FieldError{Field: fmt.Sprintf("mcpServers[%d].annotation_overrides", i), Message: "too many overrides (max 100)"}) +} +for k, v := range server.AnnotationOverrides { + if k != "*" && !IsValidToolNameForOverride(k) { errs = append(errs, FieldError{Field: fmt.Sprintf("mcpServers[%d].annotation_overrides[%q]", i, k), Message: "invalid tool name (use \"*\" or alphanumeric._:-)"}) ; continue } + if v == nil { // should never persist, but guard + errs = append(errs, FieldError{Field: fmt.Sprintf("mcpServers[%d].annotation_overrides[%q]", i, k), Message: "nil override"}) ; continue + } + if v.Title == "" && v.ReadOnlyHint==nil && v.DestructiveHint==nil && v.IdempotentHint==nil && v.OpenWorldHint==nil { + errs = append(errs, FieldError{Field: fmt.Sprintf("mcpServers[%d].annotation_overrides[%q]", i, k), Message: "at least one hint must be set"}) + } +} +``` + +**`internal/config/merge.go` β€” new merger β€” handles RFC7396 whole-key + inner-hint null via RawMessage probe (critic: cannot ignore inner `readOnlyHint:null`):** +```go +func MergeAnnotationOverrides(base, patch map[string]*ToolAnnotations, opts MergeOptions) map[string]*ToolAnnotations +// Patch semantics: +// - patch==nil β†’ preserve base (omitted field) +// - patch non-nil empty β†’ no-op (author conv: deletion only via removeMarkers) +// Implementation: +// 1. Deep copy base (as above). +// 2. For k, v := range patch { +// if v == nil { continue } // null handled via removeMarkers, not here (defense) +// if base[k]==nil { base[k]=&ToolAnnotations{} } +// // per-hint merge: nil in patch = preserve base (inherit), non-nil = override +// if v.ReadOnlyHint != nil { b:=*v.ReadOnlyHint; base[k].ReadOnlyHint=&b } +// // similarly Destructive/Idempotent/OpenWorld; if v.Title!="" { base[k].Title=v.Title } +// } +// 3. For k := range opts.GetRemoveMarkersForMap("annotation_overrides") { +// // covers {"annotation_overrides":null} and {"annotation_overrides":{"tool":null}} +// // For nested "annotation_overrides.tool" marker, delete that tool key +// delete(base, k) +// } +// 4. For inner hint null: caller (httpapi/mcp) must populate opts removeMarkers for "annotation_overrides.tool.readOnlyHint" etc +// via raw JSON scan (see quarantineExplicitlySet:671 pattern + MergeMapWithOpts:412 inner loop). If present, set that hint nil in base[k]. +// Document: v1 supports whole-tool delete; per-hint revert via re-PATCH tool with hint omitted OR via inner-null marker if implemented. +``` + +**`internal/config/merge.go` β€” `MergeServerConfig` (β‰ˆ267-295)** +- After `MergeMapWithOpts` for `env/headers`: `dst.AnnotationOverrides = MergeAnnotationOverrides(dst.AnnotationOverrides, patch.AnnotationOverrides, opts)` + +**`internal/config/config.go` β€” `NormalizeAnnotationOverrides` (new, called from `internal/config/loader.go:1284` alongside `normalizeServerQuarantineFlags`, `migrateDeepScanConfig:3153`)** +- Prune empty map entries where all hints nil (legacy `* {}`), enforce 100 cap, no-op if nil. + +**`internal/config/merge.go` β€” imports:** `fmt` already, no new dep. + +### 2.2 BACKEND β€” EFFECTIVE ANNOTATIONS RESOLVER (CRITIC FIX: LOCATION + CONCURRENCY + PTR COMPARE) + +**NEW β€” `internal/config/annotation_overrides.go` (leaf, no cycle)** +- House `EffectiveAnnotationsForTool(overrides map[string]*ToolAnnotations, toolName string, upstream *ToolAnnotations) *ToolAnnotations` here (NOT in `core/client.go` β€” `internal/server/mcp.go:7114 lookupToolAnnotations` + `internal/server/preflight_glue.go:447` need it and cannot import `core` (import cycle)). +- Same logic as snippet below, but as `config` package helper so both `core` and `server` import `config`. +- Use value equality, NOT pointer `!=` (critic: `eff != toolMeta.Annotations` always true when override present). Instead: `if eff != nil && !annotationsEqual(eff, toolMeta.Annotations) { toolMeta.Annotations = eff }` or simply `toolMeta.Annotations = eff` when `eff` returned (eff already nil when no override). + +**`internal/upstream/core/client.go:402-433` β€” `ListTools` β€” ADD RLock + call config helper** +```go +// After hasAnnotations block (425), before toolMeta.Hash (430): +// Apply per-server operator overrides (admin-only, persisted in ServerConfig) β€” read under RLock (critic: race with hot-reload) +c.mu.RLock() +overrides := c.config.AnnotationOverrides +// Deep-copy not needed β€” Effective helper copies +c.mu.RUnlock() // keep lock short; or hold across effective call if config pointer stable +if eff := config.EffectiveAnnotationsForTool(overrides, tool.Name, toolMeta.Annotations); eff != nil { + // eff is merged copy; log at debug for audit trace + // c.logger.Debug("Tool annotations overridden", zap.String("tool", tool.Name), zap.Any("upstream", toolMeta.Annotations), zap.Any("effective", eff)) + toolMeta.Annotations = eff +} else if overrides != nil && (overrides["*"] != nil || overrides[tool.Name] != nil) { + // overrides existed but resulted in empty β†’ treat as nil (no hint) to preserve nil-default semantics + toolMeta.Annotations = nil +} +``` + +**Helper signature in `internal/config/annotation_overrides.go`:** +```go +func EffectiveAnnotationsForTool(overrides map[string]*ToolAnnotations, toolName string, upstream *ToolAnnotations) *ToolAnnotations { + wild := overrides["*"]; exact := overrides[toolName] + if wild==nil && exact==nil { return upstream } + base := &ToolAnnotations{} + if upstream!=nil { + *base = *upstream + if upstream.ReadOnlyHint!=nil {b:=*upstream.ReadOnlyHint; base.ReadOnlyHint=&b} + if upstream.DestructiveHint!=nil {b:=*upstream.DestructiveHint; base.DestructiveHint=&b} + if upstream.IdempotentHint!=nil {b:=*upstream.IdempotentHint; base.IdempotentHint=&b} + if upstream.OpenWorldHint!=nil {b:=*upstream.OpenWorldHint; base.OpenWorldHint=&b} + } + for _, ov := range []*ToolAnnotations{wild, exact} { // wildcard first, exact wins per-hint + if ov==nil { continue } + if ov.Title != "" { base.Title = ov.Title } + if ov.ReadOnlyHint != nil { b:=*ov.ReadOnlyHint; base.ReadOnlyHint=&b } + if ov.DestructiveHint != nil { b:=*ov.DestructiveHint; base.DestructiveHint=&b } + if ov.IdempotentHint != nil { b:=*ov.IdempotentHint; base.IdempotentHint=&b } + if ov.OpenWorldHint != nil { b:=*ov.OpenWorldHint; base.OpenWorldHint=&b } + } + if base.Title=="" && base.ReadOnlyHint==nil && base.DestructiveHint==nil && base.IdempotentHint==nil && base.OpenWorldHint==nil { return nil } + return base +} +func annotationsEqual(a,b *ToolAnnotations) bool { /* compare Title + *bool deref */ } +``` + +**Also patch:** `internal/server/preflight_glue.go:301,447,471` `preflightSnapshot()` + `internal/server/mcp_direct_catalog.go:91` + `internal/server/mcp.go:7114 lookupToolAnnotations` β€” wrap their `ToolAnnotations` return through `config.EffectiveAnnotationsForTool(server.AnnotationOverrides, tool, upstream)` so preflight + direct catalog lethal-trifecta also see effective. + +**NOTE:** Do NOT hash overrides (`internal/runtime/tool_quarantine.go:54` + `internal/storage/models.go:ToolApprovalRecord` stays). Merge is read-path only β€” `calculateToolApprovalHashWithOutputSchema` still ignores annotations. + +### 2.3 BACKEND β€” API SURFACES (TRIPLE WRITE β€” CRITIC FIX: MANUAL MERGE + NULL MARKERS) + +**`internal/httpapi/server.go` β€” `AddServerRequest:1901` + `oas/swagger.yaml:Server`** +- Add `AnnotationOverrides map[string]*config.ToolAnnotations `json:"annotation_overrides,omitempty"` +- Regenerate swagger via `make gen` (or `go generate ./...`) β€” required for `oas/swagger.yaml` contract (`api.ts` is generated from it). + +**`internal/httpapi/server.go` β€” `handlePatchServer:2398` β€” MANUAL DEEP-MERGE (critic: cannot `patchSC.AnnotationOverrides = req.AnnotationOverrides` β€” would overwrite 23 other tools)** +```go +// Inside handlePatchServer after existing req.Env/req.Headers handling (2499-2646 pattern): +if req.AnnotationOverrides != nil || hasRemoveMarker("annotation_overrides") { + // Merge, not replace: reuse config.MergeAnnotationOverrides with opts from parsePatchWithRemoveMarkers + // Detect per-tool null via raw map scan: + // rawPatch := map[string]json.RawMessage; json.Unmarshal(body, &rawPatch); if rawPatch["annotation_overrides"] != nil { + // var inner map[string]json.RawMessage; json.Unmarshal(rawPatch["annotation_overrides"], &inner) + // for k, v := range inner { if string(bytes.TrimSpace(v))=="null" { opts.WithRemoveMarker("annotation_overrides."+k) } } + // } + merged := config.MergeAnnotationOverrides(existing.AnnotationOverrides, req.AnnotationOverrides, opts) + patchSC.AnnotationOverrides = merged +} +// Validate via ValidateDetailed already covers. Hot-reload: no restart_required (like tool enable), return restart_required:false (unlike isolation which is true:2681). +``` + +**`internal/server/mcp.go` β€” `buildPatchConfigFromRequest:5632` β€” PARSE WITH RAW MARKERS (same as httpapi)** +```go +if raw, ok := patchRaw["annotation_overrides"]; ok { + if string(bytes.TrimSpace(raw))=="null" { + opts.WithRemoveMarker("annotation_overrides") + } else { + var m map[string]*config.ToolAnnotations + if err:=json.Unmarshal(raw, &m); err!=nil { return nil, FieldError{Field:"annotation_overrides", Message:"invalid JSON"} } + // Per-tool null markers + var inner map[string]json.RawMessage + if err:=json.Unmarshal(raw, &inner); err==nil { + for k, v := range inner { if string(bytes.TrimSpace(v))=="null" { opts.WithRemoveMarker("annotation_overrides."+k) } } + } + // Per-hint inner null: {"tool":{"readOnlyHint":null}} β†’ need marker "annotation_overrides.tool.readOnlyHint" + // Scan inner objects similarly and WithRemoveMarker that path; MergeAnnotationOverrides will set that hint nil. + patchSC.AnnotationOverrides = m + } +} +``` +- Gate via `AuthorizeServerOp` β€” reuse `patch` (already admin-only) β€” but also ensure `Add` path (`ServerOpAdd`) checks `annotation_overrides` requires admin (critic: agent could POST with `destructive:false` to lower tier without audit). Add `annotation_overrides` to denylist check in `ServerOpAdd` or require admin for any `annotation_overrides` present. + +**`internal/auth/server_ops.go:59` β€” explicit denylist** +- Add `ServerOpAnnotationOverride = "annotation_override"` and include in `agentDeniedServerOps` (or ensure `patch`/`add` already covers β€” but be explicit per critic: fail-closed on new mutation). + +**`frontend/src/services/api.ts:352` β€” `PatchServerRequest` type** +- Must match `httpapi` snake_case `annotation_overrides` (not camel). Generated from `oas/swagger.yaml` β€” run `make gen`. + +**`internal/config/loader.go` β€” add `normalizeAnnotationOverrides` alongside `normalizeServerQuarantineFlags:1284` if legacy key exists (no legacy, but keep hook).** + +### 2.4 BACKEND β€” AUDIT (CRITIC FIX: TAMPER-EVIDENT) + +**`internal/audit/line.go:21-100` β€” ADD `config_change` audit kind (OWASP ASVS 7, NIST AU-2/9)** +- Existing only `authz/tool_call/auth_event`. Privileged `annotation_override` MUST be in `audit.Sink` (append-only, `request_id` correlated via `reqcontext.GetRequestID`), not only activity BBolt. +- New `func NewConfigChangeAuditLine(actor Actor, target string, before, after map[string]*ToolAnnotations, requestID string) *Line` with `Action:"annotation_override"` + `Before/After` (bool hints, no secret mask needed but keep `CheckServerWriteMasks` pass). + +**`internal/runtime/event_bus.go:753` `EmitActivityConfigChange` + `internal/server/mcp.go:3780-3807` `redactedConfigDiff`** +- Keep activity diff for ops; ALSO call `audit.Sink.Emit(NewConfigChange...)` on same path. Ensure `request_id` propagated (`X-Request-Id` header already at `mcp.go:3780`). +- Document if audit sink disabled (personal edition) β†’ activity log suffices for AU-9(4) with retention. + +**`internal/storage/models.go:ToolApprovalRecord` β€” no hash change β€” document in `tool_quarantine_test.go:54` that effective annotations do NOT affect `Hash`.** + +### 2.5 FRONTEND β€” CONFIG CARD (CRITIC FIX: SCALABILITY + VISIBILITY) + +**`frontend/src/views/ServerDetail.vue:915` β€” after `trust-mode-card` β€” card with INLINE POPOVER SCALABILITY (critic: 24Γ—4=96 selects in narrow card not scalable)** +```vue +
+
+

Annotation Overrides

+

Fix false hints from the upstream server. β€œ*” applies to all tools; a tool row wins per-hint. Inherit = use what the server sent. Changes apply immediately (no restart) and are audited.

+ + +
Bulk edit as JSON +
+
+
+
+``` + +**New component `frontend/src/components/AnnotationOverridesEditor.vue` β€” INLINE POPOVER, NOT 96 SELECTS** +- Props: `serverName, tools[], overrides, upstream` +- Layout: compact table `Tool | Effective | Actions` (Effective badge via `AnnotationBadges` colors `badge-info/error/neutral/secondary`). Clicking row opens popover with 4Γ— `select Inherit/true/false` (like `TrustModeSelector` 3-radio). +- Row for `"*"` pinned at top with `badge badge-outline` wildcard label. +- Per-hint `select` options `Inherit / true / false`; `effectiveAnnotationsForTool` computed for preview badge. +- Save: `api.patchServer(serverName, {annotation_overrides: overrides})` β€” mirrors `saveTrustMode:3490` (`trust_mode`). Use snake_case `annotation_overrides` (critic: `api.ts:352` generated from `oas/swagger.yaml` snake). +- `data-test`: `annotation-overrides-card`, `annotation-override-select-${tool}-${hint}`, `annotation-overrides-save`, `annotation-overrides-raw`. + +**`frontend/src/services/api.ts:352` β€” `PatchServerRequest` now includes `annotation_overrides?: Record` β€” run `make gen` from `oas/swagger.yaml`.** + +**`ServerDetail.vue:685` β€” Tools tab per-tool card header β€” FIX VISIBILITY (critic: `isToolToggleAvailable` false for pending/changed hides pencil exactly when needed)** +```vue + + + +``` +- `focusAnnotationOverride(toolName)` β†’ `activeTab='config'` + `nextTick` + `querySelector('[data-test="annotation-overrides-card"]')?.scrollIntoView({behavior:'smooth'})` + `highlight` (`ring-2 ring-primary` as `Settings.vue:475`) + pre-open popover for that tool. + +### 2.6 DOCS + GENERATION + +- `docs/configuration/config-file.md` β€” add `annotation_overrides` section with browseros 24-tool example (wildcard `*` + per-tool `act`). +- `docs/configuration/upstream-servers.md` β€” table row for `annotation_overrides` (type `map[string]object`, admin-only, hot, audited). +- `oas/swagger.yaml` β€” add `annotation_overrides` to `Server` schema (snake_case). Run `make gen` to regenerate `frontend/src/services/api.ts` + Go server stubs. +- `roadmap.yaml` is SOURCE, `ROADMAP.md` GENERATED via `scripts/gen-roadmap.py` (critic: not reverse). No epic needed for additive feature, but add feature note if required. + +### 2.7 MISSING FILES β€” CRITIC FIX (ADDED) + +- `internal/config/loader.go` β€” `normalizeAnnotationOverrides` hook (alongside `normalizeServerQuarantineFlags:1284`, `migrateDeepScanConfig:3153`) for legacy pruning. +- `internal/storage/models.go` / `internal/runtime/tool_quarantine.go` β€” ensure effective not persisted as `ToolApprovalRecord.Hash` (document hash stability). +- `internal/server/preflight_glue.go:301,447,471` + `internal/server/mcp_direct_catalog.go:91` + `internal/server/mcp.go:7114` β€” all wrap through `config.EffectiveAnnotationsForTool` (also fixes preflight + direct lethal-trifecta). +- `frontend/src/services/api.ts` + `oas/swagger.yaml` β€” codegen sync (critic: without `make gen` frontend won't build). +- `internal/auth/server_ops.go:59` β€” add `ServerOpAnnotationOverride` denylist or verify `patch` covers `Add`. +- `internal/audit/line.go` + `internal/server/audit_funnel.go` + `internal/runtime/event_bus.go:753` β€” audit sink for config_change. + +### 2.8 LINE CITATION CORRECTIONS (CRITIC) + +- `ValidateAgainstServerAnnotations` at `internal/contracts/intent.go:162` (not :177), `classifyToolRisk` at `99-122` (not 108-114), target-tier gate at `mcp.go:2555` (not 2570). +- Spec numbering: next spec after `107-server-edition-sso-hardening` is `108-annotation-overrides` β€” reserve, not hallucinate existing. + +--- + +## 3. πŸ§ͺ TEST PLAN β€” PLATINUM COVERAGE (EVERY BRANCH) + +### 3.1 BACKEND GO β€” `go test -race ./... -count=1` + +| File | Test | Given/When/Then | Mutation Kill | +|---|---|---|---| +| `internal/config/config_test.go` | `TestAnnotationOverrides_ValidateDetailed_RejectsEmptyHint` | Given server with `{"a":{}}` β†’ ValidateDetailed error `at least one hint` | Boundary empty | +| | `TestAnnotationOverrides_ValidateDetailed_RejectsTooMany` | 101 entries β†’ error max 100 | Boundary 100/101 | +| | `TestAnnotationOverrides_ValidateDetailed_WildcardAllowed` | `{"*":{destructiveHint:false}}` β†’ pass | Wildcard vs tool name | +| | `TestCopyServerConfig_DeepCopyOverrides` | Mutate copy doesn't mutate src pointer | Pointer alias | +| | `TestMergeAnnotationOverrides_NullDeletes` | PATCH `{"tool":null}` with marker β†’ delete | RFC7396 | +| | `TestMergeAnnotationOverrides_PreserveOnNilPatch` | nil patch β†’ preserve | Merge semantics | +| | `TestMergeAnnotationOverrides_PerHintWins` | base `destructive:true` + patch `readOnly:true` β†’ both, exact wins over `*` | Merge priority | +| `internal/upstream/core/client_test.go` (new) | `TestEffectiveAnnotations_WildcardThenExact` | upstream `destructive:true`, overrides `{"*":{destructive:false}, "act":{destructive:true}}` β†’ `act:true`, `navigate:false` | Priority | +| | `TestEffectiveAnnotations_NilUpstream` | upstream nil + `* {readOnly:true}` β†’ readOnly true | Nilβ†’inherit | +| `internal/contracts/intent_test.go` | `TestDeriveCallWith_Overridden` | overridden `readOnly:true,destructive:false` β†’ `read` not `destructive` | Derive priority | +| `internal/server/mcp_annotations_test.go` | `TestClassifyToolRisk_Overridden` | overridden `openWorld:false,destructive:false,readOnly:true` β†’ low not lethal | Classify | +| `internal/toolannotations/toolannotations_test.go` | `TestExcludeReason_Overridden` | `excludeDestructive` with overridden `destructive:false` β†’ not excluded | Filter | +| `internal/httpapi/server_test.go` | `TestPatchServer_AnnotationOverrides_AdminOnly` | agent token PATCH β†’ 403, admin β†’ 200 | Auth gate | +| `internal/server/mcp_test.go` | `TestBuildPatchConfig_AnnotationOverrides_Marker` | MCP patch with null marker β†’ delete | Parse | +| `internal/runtime/tool_quarantine_test.go` | `TestHash_UnchangedByOverrides` | same tool, different override β†’ same hash | Hash stability | +| `frontend` | `annotation-overrides-editor.spec.ts` | 3-state select Inherit/Yes/No β†’ correct PATCH payload | UI | + +**Frontend unit:** `frontend/tests/unit/annotation-overrides-editor.spec.ts` β€” mount editor, assert wildcard row, per-hint selects, save emits `annotation_overrides`. + +**E2E Playwright:** `e2e/playwright/annotation-overrides.spec.ts` β€” add `browseros` mock server, set `* {destructive:false}`, verify Tools page badge changes from `destructive` red β†’ `read` green, `call_tool_read` succeeds where before `SERVER_MISMATCH`. + +**LIVE:** `tests/live_annotation_overrides_test.go` (Go `httptest` + real Docker `browseros` at `127.0.0.1:9001`) β€” start core with override, list tools, assert effective `readOnly:true` for `snapshot`, `destructive:false` for `navigate`. + +### 3.2 QUALITY GATES (7 β€” adapted to Go β€” CRITIC FIX: REAL PATHS + SKIP) + +``` +Gate 1: SYNTAX+LINT `golangci-lint run --config .golangci.yml ./...` + `golangci-lint run --config .golangci.yml --build-tags server ./...` β†’ 0 (critic: path is .golangci.yml not .github/.golangci.yml; need 2 runs per CLAUDE.md:103) +Gate 2: TYPE SAFETY `go vet ./...` + `go test -race -tags server -timeout 20m -skip "E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/server/... ./internal/httpapi/...` builds β†’ 0 (critic: bare `go test ./internal/server` hangs) +Gate 3: UNIT TESTS `go test -race ./internal/... -v -count=1 -timeout 20m` (+ frontend `npm run test` if exists) β†’ 0 failed, coverage >=85% (target 93%); include persistence round-trip `config.Load β†’ Save β†’ Copy` and hot-reload no-restart test for overrides (restart_required:false vs trust_mode true:2681) +Gate 4: MUTATION `go test -run TestMutation` boundary (100/101, Title "" vs omit, wildcard * vs tool *), bool (true/false/nil) β†’ >=80% (document known 0) +Gate 5: LIVE TESTS Real `modelcontextprotocol/server-everything` (scripts/run-e2e-tests.sh) OR docker `browseros` at 127.0.0.1:9001 if available β†’ skip if 9001 down (critic: browseros not in CI fixture). Verify effective no restart β†’ badge change without restart. +Gate 6: METRICS `golangci-lint` cyclomatic ≀15, `frontend npm run lint` 0, `oas/swagger.yaml` valid +Gate 7: SECURITY `govulncheck ./...` 0 HIGH/MED, `npm audit` 0 HIGH, `CheckServerWriteMasks` no `β€’β€’β€’β€’` leak +``` +**Per STRATEGY_COMPLETE Part 5:** live tests mandatory for merge priority bug (mocks hide priority). Use `internal/*_test.go` not `tests/` (critic: Go live tests live in `internal/`, not `tests/`). + +**Additional required tests (critic):** `loader_test.go` for `annotation_overrides` persistence, hot-reload without `restart_required`, `oas` generation, `Copy` deep-copy alias test, `Title ""` preserve vs clear with `*string` future. + +--- + +## 4. πŸ” CRITIC CHECKLIST β€” HARD REVIEW (MUST PASS BEFORE MERGE) + +- [ ] No `mcp.ToolAnnotation` (wrong type) vs `config.ToolAnnotations` confusion (`internal/server/mcp_direct_catalog.go:95` vs `Upstream`). +- [ ] `CopyServerConfig` deep copies `*bool` β€” pointer alias not shared. +- [ ] `ValidateDetailed` not `Validate` β€” write-gate correct, 101-entry limit, wildcard `*` not validated as tool name. +- [ ] `MergeAnnotationOverrides` respects `removeMarkers` for both whole-map null and per-tool null. +- [ ] `effectiveAnnotationsForTool` precedence `exact > * > upstream` per-hint, not whole-object replace. +- [ ] Hash NOT affected β€” `tool_quarantine.go:54` still ignores annotations. +- [ ] PATCH triple-write: `AddServerRequest` + `handlePatchServer` + `buildPatchConfigFromRequest` + `api.patchServer` all updated β€” GH#938 not repeated. +- [ ] Audit: activity diff + (optional) audit sink line for `annotation_override`, `request_id` correlated. +- [ ] Frontend tri-state is `select` not `toggle`, `data-test` kebab, badges reuse `AnnotationBadges` colors, no `restart_required` false positive (hot). +- [ ] No secret leak: `CheckServerWriteMasks` not needed for bool hints but new field not break masking for `env/headers`. +- [ ] No new dep: Go 1.26, Vue 3.5, existing `mcp-go`, `zap`, `BBolt` only β€” `CLAUDE.md` rule. +- [ ] BDD specs in `specs/108-annotation-overrides/spec.md` (if spec required) or inline β€” but per author, small feature may skip spec if tests+docs exist. Document decision. + +--- + +## 5. πŸš€ IMPLEMENTATION ORDER β€” AUTONOMOUS SUB-ORCHESTRATORS + +``` +MASTER (this plan) β†’ SUB-ORCHESTRATOR per phase: +Phase A: Backend config+merge (Code Agent + Critic) +Phase B: Upstream effective resolver (Code + Test + Critic) +Phase C: API triple-write + validation (Code + Critic) +Phase D: Frontend card+editor (Code + Critic) +Phase E: Full test suite + gates + live browseros verify (Test + Debug) +Phase F: Docs + final cosmic verify (Critic) +FAIL at any gate β†’ back to [3] (max 10 review rounds per PR per CLAUDE.md) +``` + +**DELEGATION RULE:** Each agent gets: (1) Goal, (2) Full context JSON (this file + STRATEGY_COMPLETE.md Part 0.5-5), (3) MSI part, (4) MCP tools (Context7 for Vue/Go docs, Tavily for patterns, browseros for live), (5) Report path `OTCHETY/report_phase_X.md`. + +**LIVE VERIFY (Phase E):** Use `browseros:navigate` (destructive) + `snapshot` (readOnly) via mcpproxy at `127.0.0.1:12754` β†’ open `http://127.0.0.1:12754/ui/#/servers/browseros` β†’ Configuration tab β†’ verify card renders, save `* {destructive:false}`, reload Tools tab β†’ badges change, `call_tool_read browseros:snapshot` now succeeds without `SERVER_MISMATCH`. + +--- + +## 6. πŸ“¦ COMMIT & PR CONVENTIONS (CRITIC FIX: DUAL BRANCH) + +- **Local dev branch (user req):** `πŸš€-FEAT-ANNOTATION-OVERRIDES-πŸ›‘οΈ-PER-SERVER-TOOL-EXCEPTIONS-✨` β€” CAPS+EMOJIS caps+emojis commits per user (`✨ FEAT: ...` etc) β€” stays local, never pushed to upstream workflows. +- **Upstream PR branch (author conv):** `feat/annotation-overrides-per-server-tool` β€” conventional commits lower (`feat: per-server per-tool annotation overrides`, `fix: merge priority exact>*`, `feat: ui card + editor`, `test: platinum coverage 93%`, `docs: config reference`) per `git log --oneline` `fix(scope):` `chore(deps):` and `CLAUDE.md` `gofmt`. +- For submission: `git checkout -b feat/annotation-overrides-per-server-tool && git cherry-pick --strategy=recursive` or `git format-patch` and re-commit with conventional messages. +- Hot-reload: no `roadmap.yaml` epic sweep β€” additive per-server feature. + +--- + +## 7. ⚠️ RISKS & MITIGATIONS + +- Wildcard `*` collides with real tool named `*` β†’ `IsValidToolName` rejects `*` as tool name except as wildcard; tool name `*` invalid per MCP (no such tool). +- Inner `readOnlyHint:null` revert needs tool object replace β€” v1 limitation documented, not blocking (operator can delete whole tool override and re-add). +- Per-server map grows β†’ 100 limit + truncate in UI. +- Browseros snapshot refs invalid after navigate β€” re-snapshot before `act` (browseros pattern `snapshotβ†’actβ†’diff`). + +--- + +## 8. βœ… DONE CRITERIA (PLATINUM) + +- [ ] `annotation_overrides` persisted, validated, merged, effective, hot, audited, no hash change. +- [ ] UI card after Trust mode, pencil in Tools, no restart badge, `data-test` green. +- [ ] `go test -race ./...` + `golangci-lint` + `frontend npm run build` + `e2e` + live browseros snapshot PASS. +- [ ] Critic APPROVED (no REJECTED issues). +- [ ] User arrives and only ACCEPTS β€” no debug needed. + +--- + +*END OF PLAN β€” EVERY LINE VERIFIED vs REAL CODE AT HEAD 525d5021 β€” READ BY EVERY AGENT BEFORE WORK.* From 562d82f58919111503dd92519f8507e3b2a1e50d Mon Sep 17 00:00:00 2001 From: AGI Developer Date: Sat, 19 Sep 2026 23:31:32 +0300 Subject: [PATCH 02/13] =?UTF-8?q?=E2=9C=A8=20FEAT:=20=F0=9F=94=A7=20BACKEN?= =?UTF-8?q?D=20CONFIG=20MODEL=20=E2=80=94=20ANNOTATION=20OVERRIDES=20MAP?= =?UTF-8?q?=20+=20MERGE=20+=20VALIDATION=20=F0=9F=9B=A1=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/config/annotation_overrides.go | 126 ++++++++++++++++ internal/config/config.go | 61 ++++++++ internal/config/loader.go | 2 + internal/config/merge.go | 183 ++++++++++++++++++++++++ 4 files changed, 372 insertions(+) create mode 100644 internal/config/annotation_overrides.go diff --git a/internal/config/annotation_overrides.go b/internal/config/annotation_overrides.go new file mode 100644 index 000000000..b545492e3 --- /dev/null +++ b/internal/config/annotation_overrides.go @@ -0,0 +1,126 @@ +package config + +// EffectiveAnnotationsForTool resolves the effective MCP tool annotations for a +// tool by merging upstream hints with per-server operator overrides. +// Precedence per hint: exact tool name > wildcard "*" > upstream. +// A nil upstream is treated as empty; an empty result (no hints) returns nil +// to preserve the MCP spec nil-default semantics. +func EffectiveAnnotationsForTool(overrides map[string]*ToolAnnotations, toolName string, upstream *ToolAnnotations) *ToolAnnotations { + wild := overrides["*"] + exact := overrides[toolName] + if wild == nil && exact == nil { + return upstream + } + base := &ToolAnnotations{} + if upstream != nil { + *base = *upstream + if upstream.ReadOnlyHint != nil { + b := *upstream.ReadOnlyHint + base.ReadOnlyHint = &b + } + if upstream.DestructiveHint != nil { + b := *upstream.DestructiveHint + base.DestructiveHint = &b + } + if upstream.IdempotentHint != nil { + b := *upstream.IdempotentHint + base.IdempotentHint = &b + } + if upstream.OpenWorldHint != nil { + b := *upstream.OpenWorldHint + base.OpenWorldHint = &b + } + } + for _, ov := range []*ToolAnnotations{wild, exact} { + if ov == nil { + continue + } + if ov.Title != "" { + base.Title = ov.Title + } + if ov.ReadOnlyHint != nil { + b := *ov.ReadOnlyHint + base.ReadOnlyHint = &b + } + if ov.DestructiveHint != nil { + b := *ov.DestructiveHint + base.DestructiveHint = &b + } + if ov.IdempotentHint != nil { + b := *ov.IdempotentHint + base.IdempotentHint = &b + } + if ov.OpenWorldHint != nil { + b := *ov.OpenWorldHint + base.OpenWorldHint = &b + } + } + if base.Title == "" && base.ReadOnlyHint == nil && base.DestructiveHint == nil && base.IdempotentHint == nil && base.OpenWorldHint == nil { + return nil + } + return base +} + +// annotationsEqual reports value equality for two ToolAnnotations, comparing +// Title and dereferenced *bool hints (nil == nil). Pointer identity is ignored. +func annotationsEqual(a, b *ToolAnnotations) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + if a.Title != b.Title { + return false + } + if !boolPtrEqual(a.ReadOnlyHint, b.ReadOnlyHint) { + return false + } + if !boolPtrEqual(a.DestructiveHint, b.DestructiveHint) { + return false + } + if !boolPtrEqual(a.IdempotentHint, b.IdempotentHint) { + return false + } + if !boolPtrEqual(a.OpenWorldHint, b.OpenWorldHint) { + return false + } + return true +} + +func boolPtrEqual(a, b *bool) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return *a == *b +} + +// NormalizeAnnotationOverrides prunes empty entries where all hints are nil +// and Title is empty (legacy "* {}"), and enforces the 100-entry cap by +// truncating (validation will still reject >100 on write, but load-time +// convergence must not panic). No-op if cfg or Servers is nil. +func NormalizeAnnotationOverrides(cfg *Config) { + if cfg == nil { + return + } + for _, s := range cfg.Servers { + if s == nil || s.AnnotationOverrides == nil { + continue + } + for k, v := range s.AnnotationOverrides { + if v == nil { + delete(s.AnnotationOverrides, k) + continue + } + if v.Title == "" && v.ReadOnlyHint == nil && v.DestructiveHint == nil && v.IdempotentHint == nil && v.OpenWorldHint == nil { + delete(s.AnnotationOverrides, k) + } + } + if len(s.AnnotationOverrides) == 0 { + s.AnnotationOverrides = nil + } + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 0953064ef..edb96ecb8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -750,6 +750,8 @@ type ServerConfig struct { EnabledTools []string `json:"enabled_tools,omitempty" mapstructure:"enabled_tools"` // Allowlist: only these tools are exposed; mutually exclusive with disabled_tools DisabledTools []string `json:"disabled_tools,omitempty" mapstructure:"disabled_tools"` // Denylist: these tools are hidden; mutually exclusive with enabled_tools + AnnotationOverrides map[string]*ToolAnnotations `json:"annotation_overrides,omitempty" mapstructure:"annotation-overrides"` + // SourceRegistryID records which registry this server was added from (empty // for manually-configured servers). MCP-866: surfaced in the approval / // quarantine view so a reviewer can see a server's origin. @@ -2051,6 +2053,36 @@ func IsValidTrustMode(s string) bool { } } +// IsValidToolNameForOverride reports whether name is a valid key for +// annotation_overrides. The wildcard "*" is allowed as a standalone key; +// otherwise the name must be 1..256 chars, must not have leading/trailing +// whitespace, must match ^[A-Za-z0-9._:-]+$ and must not contain "__" +// (reserved for direct-mode server__tool separator). +func IsValidToolNameForOverride(name string) bool { + if name == "*" { + return true + } + if len(name) == 0 || len(name) > 256 { + return false + } + if strings.TrimSpace(name) != name { + return false + } + if strings.Contains(name, "__") { + return false + } + if strings.Contains(name, "*") { + return false + } + for _, r := range name { + if (r >= 'A' && r <= 'Z') || (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '.' || r == '_' || r == ':' || r == '-' { + continue + } + return false + } + return true +} + // EnvTPABundlePath is the environment override for the offline TPA // signature-bundle location (spec 086 FR-019). It outranks // security.tpa_bundle_path on EVERY path that resolves the corpus β€” the loader, @@ -2603,6 +2635,35 @@ func (c *Config) validateDetailedCore() []ValidationError { if e := validateIntervalBound(fieldPrefix+".init_timeout", server.InitTimeout, time.Second, 30*time.Minute); e != nil { errors = append(errors, *e) } + + if len(server.AnnotationOverrides) > 100 { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("%s.annotation_overrides", fieldPrefix), + Message: "too many overrides (max 100)", + }) + } + for k, v := range server.AnnotationOverrides { + if k != "*" && !IsValidToolNameForOverride(k) { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("%s.annotation_overrides[%q]", fieldPrefix, k), + Message: "invalid tool name (use \"*\" or alphanumeric._:-)", + }) + continue + } + if v == nil { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("%s.annotation_overrides[%q]", fieldPrefix, k), + Message: "nil override", + }) + continue + } + if v.Title == "" && v.ReadOnlyHint == nil && v.DestructiveHint == nil && v.IdempotentHint == nil && v.OpenWorldHint == nil { + errors = append(errors, ValidationError{ + Field: fmt.Sprintf("%s.annotation_overrides[%q]", fieldPrefix, k), + Message: "at least one hint must be set", + }) + } + } } // Validate DataDir exists (if specified and not empty). diff --git a/internal/config/loader.go b/internal/config/loader.go index b85f52381..c73adf6ad 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -694,6 +694,8 @@ func initializeRegistries(cfg *Config) { // field. Runs on initial load and every hot-reload (LoadFromFile path). normalizeServerQuarantineFlags(cfg) + NormalizeAnnotationOverrides(cfg) + // One-time migration (Spec 077 US3): fold the deprecated top-level // scanner_fetch_package_source / scanner_disable_no_new_privileges keys into // the unified security.deep_scan block, and drop the removed diff --git a/internal/config/merge.go b/internal/config/merge.go index 787c53ba6..46c52714d 100644 --- a/internal/config/merge.go +++ b/internal/config/merge.go @@ -294,6 +294,23 @@ func MergeServerConfig(base, patch *ServerConfig, opts MergeOptions) (*ServerCon merged.Headers = newHeaders } + annotationRemovalKeys := opts.GetRemoveMarkersForMap("annotation_overrides") + if patch.AnnotationOverrides != nil || len(annotationRemovalKeys) > 0 || opts.ShouldRemove("annotation_overrides") { + newOverrides := MergeAnnotationOverrides(base.AnnotationOverrides, patch.AnnotationOverrides, opts) + if diff != nil && !reflect.DeepEqual(base.AnnotationOverrides, newOverrides) { + diff.Modified["annotation_overrides"] = FieldChange{Path: "annotation_overrides", From: base.AnnotationOverrides, To: newOverrides} + for _, key := range annotationRemovalKeys { + if _, existed := base.AnnotationOverrides[key]; existed { + diff.Removed = append(diff.Removed, "annotation_overrides."+key) + } + } + if opts.ShouldRemove("annotation_overrides") && base.AnnotationOverrides != nil { + diff.Removed = append(diff.Removed, "annotation_overrides") + } + } + merged.AnnotationOverrides = newOverrides + } + // Nested struct fields - deep merge or remove // Handle Isolation if opts.ShouldRemove("isolation") { @@ -586,6 +603,144 @@ func MergeOAuthConfig(base, patch *OAuthConfig, removeIfNil bool) *OAuthConfig { return result } +// MergeAnnotationOverrides deep-merges annotation_overrides with RFC 7396 +// null-means-remove semantics for whole-tool deletes and per-hint +// nulls (readOnlyHint:null, etc.). Patch semantics: nil patch preserves +// base (omitted field); non-nil empty is a no-op (deletion only via +// removeMarkers); whole-map null and per-tool null are signaled via +// opts.removeMarkers ("annotation_overrides" / "annotation_overrides."). +func MergeAnnotationOverrides(base, patch map[string]*ToolAnnotations, opts MergeOptions) map[string]*ToolAnnotations { + if patch == nil && len(opts.GetRemoveMarkersForMap("annotation_overrides")) == 0 && !opts.ShouldRemove("annotation_overrides") { + if base == nil { + return nil + } + dst := make(map[string]*ToolAnnotations, len(base)) + for k, v := range base { + if v == nil { + dst[k] = nil + continue + } + cp := *v + if v.ReadOnlyHint != nil { + b := *v.ReadOnlyHint + cp.ReadOnlyHint = &b + } + if v.DestructiveHint != nil { + b := *v.DestructiveHint + cp.DestructiveHint = &b + } + if v.IdempotentHint != nil { + b := *v.IdempotentHint + cp.IdempotentHint = &b + } + if v.OpenWorldHint != nil { + b := *v.OpenWorldHint + cp.OpenWorldHint = &b + } + dst[k] = &cp + } + return dst + } + if opts.ShouldRemove("annotation_overrides") { + return nil + } + result := make(map[string]*ToolAnnotations, len(base)) + for k, v := range base { + if v == nil { + result[k] = nil + continue + } + cp := *v + if v.ReadOnlyHint != nil { + b := *v.ReadOnlyHint + cp.ReadOnlyHint = &b + } + if v.DestructiveHint != nil { + b := *v.DestructiveHint + cp.DestructiveHint = &b + } + if v.IdempotentHint != nil { + b := *v.IdempotentHint + cp.IdempotentHint = &b + } + if v.OpenWorldHint != nil { + b := *v.OpenWorldHint + cp.OpenWorldHint = &b + } + result[k] = &cp + } + for k, v := range patch { + if v == nil { + continue + } + if result[k] == nil { + result[k] = &ToolAnnotations{} + } + if v.Title != "" { + result[k].Title = v.Title + } + if v.ReadOnlyHint != nil { + b := *v.ReadOnlyHint + result[k].ReadOnlyHint = &b + } + if v.DestructiveHint != nil { + b := *v.DestructiveHint + result[k].DestructiveHint = &b + } + if v.IdempotentHint != nil { + b := *v.IdempotentHint + result[k].IdempotentHint = &b + } + if v.OpenWorldHint != nil { + b := *v.OpenWorldHint + result[k].OpenWorldHint = &b + } + } + for _, key := range opts.GetRemoveMarkersForMap("annotation_overrides") { + if idx := indexDot(key); idx >= 0 { + toolName := key[:idx] + hint := key[idx+1:] + if entry, ok := result[toolName]; ok && entry != nil { + switch hint { + case "readOnlyHint": + entry.ReadOnlyHint = nil + case "destructiveHint": + entry.DestructiveHint = nil + case "idempotentHint": + entry.IdempotentHint = nil + case "openWorldHint": + entry.OpenWorldHint = nil + case "title": + entry.Title = "" + default: + delete(result, key) + continue + } + if entry.Title == "" && entry.ReadOnlyHint == nil && entry.DestructiveHint == nil && entry.IdempotentHint == nil && entry.OpenWorldHint == nil { + delete(result, toolName) + } + } else { + delete(result, key) + } + } else { + delete(result, key) + } + } + if len(result) == 0 { + return nil + } + return result +} + +func indexDot(s string) int { + for i, c := range s { + if c == '.' { + return i + } + } + return -1 +} + // Helper functions to copy configs (avoiding pointer aliasing) func CopyServerConfig(src *ServerConfig) *ServerConfig { @@ -687,6 +842,34 @@ func CopyServerConfig(src *ServerConfig) *ServerConfig { dst.ExposePrompts = &exposePrompts } + if src.AnnotationOverrides != nil { + dst.AnnotationOverrides = make(map[string]*ToolAnnotations, len(src.AnnotationOverrides)) + for k, v := range src.AnnotationOverrides { + if v == nil { + dst.AnnotationOverrides[k] = nil + continue + } + cp := *v + if v.ReadOnlyHint != nil { + b := *v.ReadOnlyHint + cp.ReadOnlyHint = &b + } + if v.DestructiveHint != nil { + b := *v.DestructiveHint + cp.DestructiveHint = &b + } + if v.IdempotentHint != nil { + b := *v.IdempotentHint + cp.IdempotentHint = &b + } + if v.OpenWorldHint != nil { + b := *v.OpenWorldHint + cp.OpenWorldHint = &b + } + dst.AnnotationOverrides[k] = &cp + } + } + // Copy nested structs dst.Isolation = copyIsolationConfig(src.Isolation) dst.OAuth = copyOAuthConfig(src.OAuth) From c3e331ded42d0d449e2f4a0100f05cc2cae0b959 Mon Sep 17 00:00:00 2001 From: AGI Developer Date: Sat, 19 Sep 2026 23:49:34 +0300 Subject: [PATCH 03/13] =?UTF-8?q?=E2=9C=A8=20FEAT:=20=F0=9F=94=97=20EFFECT?= =?UTF-8?q?IVE=20ANNOTATIONS=20+=20=F0=9F=8C=90=20API=20TRIPLE-WRITE=20+?= =?UTF-8?q?=20=F0=9F=8E=A8=20UI=20CARD=20&=20EDITOR=20=F0=9F=9B=A1?= =?UTF-8?q?=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../components/AnnotationOverridesEditor.vue | 312 ++++++++++++++++++ frontend/src/services/api.ts | 14 +- frontend/src/types/api.ts | 5 + frontend/src/views/ServerDetail.vue | 165 +++++++-- internal/httpapi/server.go | 107 +++++- internal/server/mcp.go | 120 ++++++- internal/server/preflight_glue.go | 10 +- internal/server/server.go | 43 +++ internal/upstream/core/client.go | 10 + oas/swagger.yaml | 22 ++ 10 files changed, 778 insertions(+), 30 deletions(-) create mode 100644 frontend/src/components/AnnotationOverridesEditor.vue diff --git a/frontend/src/components/AnnotationOverridesEditor.vue b/frontend/src/components/AnnotationOverridesEditor.vue new file mode 100644 index 000000000..b8ecebd02 --- /dev/null +++ b/frontend/src/components/AnnotationOverridesEditor.vue @@ -0,0 +1,312 @@ + + + diff --git a/frontend/src/services/api.ts b/frontend/src/services/api.ts index cc2fc618c..457cf22cd 100644 --- a/frontend/src/services/api.ts +++ b/frontend/src/services/api.ts @@ -1,7 +1,17 @@ -import type { APIResponse, Server, Tool, ToolApproval, SearchResult, StatusUpdate, SecretRef, MigrationAnalysis, ConfigSecretsResponse, GetToolCallsResponse, GetToolCallDetailResponse, GetServerToolCallsResponse, GetConfigResponse, ValidateConfigResponse, ConfigApplyResult, ServerTokenMetrics, GetRegistriesResponse, SearchRegistryServersResponse, RegistrySummary, GetSessionsResponse, GetSessionDetailResponse, InfoResponse, ActivityListResponse, ActivityDetailResponse, ActivityRecord, ActivitySummaryResponse, ImportResponse, AgentTokenInfo, CreateAgentTokenRequest, CreateAgentTokenResponse, RoutingInfo, ConnectStatusResponse, ClientStatus, ConnectResult, ConnectPreview, OnboardingStateResponse, OnboardingMarkRequest, DiagnosticFixResponse, GlobalToolsResponse, UsageAggregateResponse, UsageWindow, UsageSort, UsageStatus, ListProfilesResponse, ActiveProfileResponse } from '@/types' +import type { APIResponse, Server, Tool, ToolApproval, SearchResult, StatusUpdate, SecretRef, MigrationAnalysis, ConfigSecretsResponse, GetToolCallsResponse, GetToolCallDetailResponse, GetServerToolCallsResponse, GetConfigResponse, ValidateConfigResponse, ConfigApplyResult, ServerTokenMetrics, GetRegistriesResponse, SearchRegistryServersResponse, RegistrySummary, GetSessionsResponse, GetSessionDetailResponse, InfoResponse, ActivityListResponse, ActivityDetailResponse, ActivityRecord, ActivitySummaryResponse, ImportResponse, AgentTokenInfo, CreateAgentTokenRequest, CreateAgentTokenResponse, RoutingInfo, ConnectStatusResponse, ClientStatus, ConnectResult, ConnectPreview, OnboardingStateResponse, OnboardingMarkRequest, DiagnosticFixResponse, GlobalToolsResponse, UsageAggregateResponse, UsageWindow, UsageSort, UsageStatus, ListProfilesResponse, ActiveProfileResponse, ToolAnnotation } from '@/types' import { joinHoldEvidence, type HoldEvidenceSource } from '@/utils/holdEvidence' +// PatchServerRequest mirrors the PATCH /api/v1/servers/{id} body (snake_case, per oas). +// Generated from oas/swagger.yaml but also manually extended for annotation_overrides. +export interface PatchServerRequest extends Record { + trust_mode?: string + url?: string + headers?: Record + env?: Record + annotation_overrides?: Record +} + // Event types for API service export interface APIAuthEvent { type: 'auth-error' @@ -349,7 +359,7 @@ class APIService { // request field as optional and preserves anything not supplied, so callers // can send only what they want to change. Passing `headers: {}` clears // headers; omitting the field keeps the existing value. - async patchServer(serverName: string, patch: Record): Promise { + async patchServer(serverName: string, patch: PatchServerRequest): Promise { return this.request(`/api/v1/servers/${encodeURIComponent(serverName)}`, { method: 'PATCH', body: JSON.stringify(patch), diff --git a/frontend/src/types/api.ts b/frontend/src/types/api.ts index 110199555..191b02a19 100644 --- a/frontend/src/types/api.ts +++ b/frontend/src/types/api.ts @@ -190,6 +190,9 @@ export interface SecurityScanReportSummary { info_level: number } +// Tool annotation overrides (per-server per-tool, admin-only, RFC7396 null=delete) +export type AnnotationOverrides = Record + // Server types export interface ServerIsolationConfig { // EFFECTIVE isolation state, after global + per-server + structural @@ -290,6 +293,8 @@ export interface Server { health?: HealthStatus // Unified health status calculated by the backend quarantine?: QuarantineStats // Tool-level quarantine stats (Spec 032) security_scan?: SecurityScanSummary // Security scan summary (Spec 039) + // Per-server per-tool annotation overrides (spec 108): map[toolName]*ToolAnnotations + wildcard "*" + annotation_overrides?: AnnotationOverrides // Spec 044: structured diagnostic error + stable error code error_code?: string diagnostic?: Diagnostic | null diff --git a/frontend/src/views/ServerDetail.vue b/frontend/src/views/ServerDetail.vue index 08e0a9490..1dde6505d 100644 --- a/frontend/src/views/ServerDetail.vue +++ b/frontend/src/views/ServerDetail.vue @@ -741,27 +741,36 @@ :count="findingGroupForTool(tool.name)!.findings.length" /> - - πŸ”’ locked by config +
+ + + πŸ”’ locked by config +
+ +
+
+

Annotation Overrides

+

Fix false hints from the upstream server. β€œ*” applies to all tools; a tool row wins per-hint. Inherit = use what the server sent. Changes apply immediately (no restart) and are audited.

+ + +
Bulk edit as JSON +

{{ rawJsonError }}

+
+
+
+ + so the "Edit URL" remedy offered on an unresolvable host lands on + the control that fixes it, not on a read-only echo of it. -->
>(() => { + const m: Record = {} + for (const t of serverTools.value) { + if (t.annotations) m[t.name] = t.annotations as ToolAnnotation + } + return m +}) + +// Raw JSON bulk fallback (100 entries) +const rawJson = ref('') +const rawJsonError = ref('') +const annotationOverridesHighlighted = ref(false) +const annotationEditorRef = ref | null>(null) + +watch( + () => (server.value as unknown as { annotation_overrides?: Record })?.annotation_overrides, + (v) => { + rawJson.value = JSON.stringify(v || {}, null, 2) + rawJsonError.value = '' + }, + { immediate: true, deep: true }, +) + +async function saveAnnotationOverrides(overrides: Record) { + if (!server.value) return + try { + const resp = await api.patchServer(server.value.name, { annotation_overrides: overrides } as unknown as Record) + if (!resp.success) { + systemStore.addToast({ type: 'error', title: 'Annotation overrides save failed', message: resp.error || 'Unknown error' }) + return + } + systemStore.addToast({ type: 'success', title: 'Annotation overrides saved', message: 'Changes apply immediately (no restart) and are audited.' }) + await serversStore.fetchServers(true) + await loadTools() + } catch (e: unknown) { + systemStore.addToast({ type: 'error', title: 'Annotation overrides save failed', message: e instanceof Error ? e.message : String(e) }) + } +} + +async function applyRawJson() { + if (!server.value) return + rawJsonError.value = '' + let parsed: Record + try { + parsed = JSON.parse(rawJson.value || '{}') + } catch (e: unknown) { + rawJsonError.value = e instanceof Error ? e.message : 'Invalid JSON' + return + } + try { + const resp = await api.patchServer(server.value.name, { annotation_overrides: parsed } as unknown as Record) + if (!resp.success) { + rawJsonError.value = resp.error || 'Save failed' + systemStore.addToast({ type: 'error', title: 'Annotation overrides save failed', message: resp.error || 'Unknown error' }) + return + } + systemStore.addToast({ type: 'success', title: 'Annotation overrides saved', message: '' }) + await serversStore.fetchServers(true) + await loadTools() + } catch (e: unknown) { + rawJsonError.value = e instanceof Error ? e.message : String(e) + } +} + +function focusAnnotationOverride(toolName: string) { + activeTab.value = 'config' + void nextTick(() => { + const card = document.querySelector('[data-test="annotation-overrides-card"]') + if (card) { + card.scrollIntoView({ behavior: 'smooth', block: 'center' }) + annotationOverridesHighlighted.value = true + setTimeout(() => (annotationOverridesHighlighted.value = false), 2000) + } + if (toolName && annotationEditorRef.value) { + // pre-open popover for that tool if editor exposes openEdit + const maybe = annotationEditorRef.value as unknown as { openEdit?: (name: string) => void } + if (maybe.openEdit) maybe.openEdit(toolName) + } + }) +} + async function saveEdit(scope: 'header' | 'env', k: string, val: string) { const ok = await patchServerDiff({ [scopeKey(scope)]: { [k]: val } }, `Updated ${k}`) if (ok) editingKey.value = null diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 894093ae9..3e38229e6 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -1952,6 +1952,12 @@ type AddServerRequest struct { // field leaves it alone; clear an individual override by sending it // explicitly (`"enabled": null`, `"image": ""`). Isolation *IsolationRequest `json:"isolation,omitempty"` + // AnnotationOverrides carries per-server per-tool annotation fixes + // (map[toolName]*ToolAnnotations, wildcard "*" allowed). Nil means + // "leave unchanged" on PATCH; a present map is deep-merged via + // config.MergeAnnotationOverrides with RFC7396 null-means-delete for + // whole-tool entries and per-hint nulls. See config.ToolAnnotations. + AnnotationOverrides map[string]*config.ToolAnnotations `json:"annotation_overrides,omitempty"` } // IsolationRequest is the request-body representation of @@ -2303,6 +2309,20 @@ func (s *Server) handleAddServer(w http.ResponseWriter, r *http.Request) { if req.Isolation != nil { serverConfig.Isolation = req.Isolation.resolve(nil) } + // Annotation overrides: POST ignores nil entries (JSON Merge Patch null has no + // meaning on create) β€” drop nils, keep the rest as persisted. + if req.AnnotationOverrides != nil { + filtered := make(map[string]*config.ToolAnnotations, len(req.AnnotationOverrides)) + for k, v := range req.AnnotationOverrides { + if v == nil { + continue + } + filtered[k] = v + } + if len(filtered) > 0 { + serverConfig.AnnotationOverrides = filtered + } + } // #1148 round 6: on CREATE there is no stored value to bind a mask back to, // so ANY mask this proxy rendered can only be a placeholder copied out of @@ -2402,11 +2422,47 @@ func (s *Server) handlePatchServer(w http.ResponseWriter, r *http.Request) { return } + body, err := io.ReadAll(r.Body) + if err != nil { + s.writeError(w, r, http.StatusBadRequest, "Invalid request body") + return + } var req AddServerRequest - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + if err := json.Unmarshal(body, &req); err != nil { s.writeError(w, r, http.StatusBadRequest, "Invalid request body") return } + // Build remove markers for annotation_overrides via raw JSON scan + // (RFC7396 whole-map null, per-tool null, per-hint null). + opts := config.DefaultMergeOptions() + var rawPatch map[string]json.RawMessage + if err := json.Unmarshal(body, &rawPatch); err == nil { + if raw, ok := rawPatch["annotation_overrides"]; ok { + trimmed := bytes.TrimSpace(raw) + if string(trimmed) == "null" { + opts = opts.WithRemoveMarker("annotation_overrides") + } else { + var inner map[string]json.RawMessage + if err := json.Unmarshal(raw, &inner); err == nil { + for k, v := range inner { + tv := bytes.TrimSpace(v) + if string(tv) == "null" { + opts = opts.WithRemoveMarker("annotation_overrides." + k) + } else { + var hintMap map[string]json.RawMessage + if err := json.Unmarshal(v, &hintMap); err == nil { + for hk, hv := range hintMap { + if string(bytes.TrimSpace(hv)) == "null" { + opts = opts.WithRemoveMarker("annotation_overrides." + k + "." + hk) + } + } + } + } + } + } + } + } + } // GH #938: reject an unrecognized trust_mode before anything is persisted. // A PATCH that omits trust_mode sends "" and is unaffected ("" = leave @@ -2644,12 +2700,55 @@ func (s *Server) handlePatchServer(w http.ResponseWriter, r *http.Request) { updates.Isolation = req.Isolation.resolve(existingIso) hasUpdates = true } + // Annotation overrides: manual deep-merge via config.MergeAnnotationOverrides + // with RFC7396 null-means-delete for whole-map, per-tool, and per-hint. + hadOtherUpdates := hasUpdates + hasAnnotationOverrideUpdate := req.AnnotationOverrides != nil || opts.ShouldRemove("annotation_overrides") || len(opts.GetRemoveMarkersForMap("annotation_overrides")) > 0 + if hasAnnotationOverrideUpdate { + var baseAO map[string]*config.ToolAnnotations + if existingSrv != nil { + baseAO = existingSrv.AnnotationOverrides + } + merged := config.MergeAnnotationOverrides(baseAO, req.AnnotationOverrides, opts) + updates.AnnotationOverrides = merged + hasUpdates = true + } else if existingSrv != nil { + updates.AnnotationOverrides = existingSrv.AnnotationOverrides + } + onlyAnnotationOverrideHot := hasAnnotationOverrideUpdate && !hadOtherUpdates if !hasUpdates { s.writeError(w, r, http.StatusBadRequest, "No fields to update") return } + // Validate annotation overrides via ValidateDetailed (write gate) before + // persisting. Build a temporary config with the merged server to reuse the + // canonical per-server validation (max 100, tool name, at least one hint). + if hasAnnotationOverrideUpdate { + if cfg, err := s.controller.GetConfig(); err == nil && cfg != nil && existingSrv != nil { + tmpCfg := &config.Config{Servers: make([]*config.ServerConfig, len(cfg.Servers))} + for i, sc := range cfg.Servers { + if sc != nil && sc.Name == serverName { + // Use the merged server for validation + mergedSrv := config.CopyServerConfig(sc) + mergedSrv.AnnotationOverrides = updates.AnnotationOverrides + tmpCfg.Servers[i] = mergedSrv + } else { + tmpCfg.Servers[i] = sc + } + } + if errs := tmpCfg.ValidateDetailed(); len(errs) > 0 { + for _, e := range errs { + if strings.Contains(e.Field, "annotation_overrides") { + s.writeError(w, r, http.StatusBadRequest, e.Error()) + return + } + } + } + } + } + // #1148 round 6: the fail-closed net. The key-bound reverts above restored // every mask this proxy can bind back to the value it was read from; // anything still carrying one is refused rather than persisted over a live @@ -2676,9 +2775,13 @@ func (s *Server) handlePatchServer(w http.ResponseWriter, r *http.Request) { } logger.Infow("Server updated successfully", "server", serverName) + restartRequired := true + if onlyAnnotationOverrideHot { + restartRequired = false + } s.writeSuccess(w, map[string]interface{}{ "message": fmt.Sprintf("Server '%s' updated successfully", serverName), - "restart_required": true, + "restart_required": restartRequired, }) } diff --git a/internal/server/mcp.go b/internal/server/mcp.go index 85b7708a5..f6679625e 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -1,6 +1,7 @@ package server import ( + "bytes" "context" "encoding/json" "fmt" @@ -5222,6 +5223,44 @@ func (p *MCPProxyServer) handleAddUpstream(ctx context.Context, request mcp.Call } } + // Annotation overrides on add: accept annotation_overrides_json (JSON string) + // or direct annotation_overrides object. POST ignores nil entries. + if aoJSON := request.GetString("annotation_overrides_json", ""); aoJSON != "" { + var m map[string]*config.ToolAnnotations + if err := json.Unmarshal([]byte(aoJSON), &m); err != nil { + return mcp.NewToolResultError(fmt.Sprintf("Invalid annotation_overrides_json format: %v", err)), nil + } + filtered := make(map[string]*config.ToolAnnotations, len(m)) + for k, v := range m { + if v == nil { + continue + } + filtered[k] = v + } + if len(filtered) > 0 { + serverConfig.AnnotationOverrides = filtered + } + } else if rawArgs := request.GetArguments(); rawArgs != nil { + if raw, ok := rawArgs["annotation_overrides"]; ok { + data, err := json.Marshal(raw) + if err == nil { + var m map[string]*config.ToolAnnotations + if err := json.Unmarshal(data, &m); err == nil { + filtered := make(map[string]*config.ToolAnnotations, len(m)) + for k, v := range m { + if v == nil { + continue + } + filtered[k] = v + } + if len(filtered) > 0 { + serverConfig.AnnotationOverrides = filtered + } + } + } + } + } + // #1148 round 6 (finding 4): on CREATE there is no stored value to bind a // mask back to, so ANY mask this proxy rendered can only be a placeholder // an agent copied out of another server's read payload β€” never a value @@ -5834,6 +5873,77 @@ func (p *MCPProxyServer) buildPatchConfigFromRequest(request mcp.CallToolRequest } } + // Annotation overrides: per-server per-tool hint fixes (wildcard "*" + // allowed). Supports RFC7396 whole-map null, per-tool null, and per-hint + // null via remove markers (e.g. annotation_overrides.tool.readOnlyHint). + // The MCP surface accepts both annotation_overrides_json (JSON string like + // env_json) and direct annotation_overrides object for flexibility. + if aoJSON := request.GetString("annotation_overrides_json", ""); aoJSON != "" { + trimmed := bytes.TrimSpace([]byte(aoJSON)) + if string(trimmed) == "null" { + opts = opts.WithRemoveMarker("annotation_overrides") + } else { + var m map[string]*config.ToolAnnotations + if err := json.Unmarshal([]byte(aoJSON), &m); err != nil { + return nil, opts, fmt.Errorf("invalid annotation_overrides_json format: %v", err) + } + var inner map[string]json.RawMessage + if err := json.Unmarshal([]byte(aoJSON), &inner); err == nil { + for k, v := range inner { + tv := bytes.TrimSpace(v) + if string(tv) == "null" { + opts = opts.WithRemoveMarker("annotation_overrides." + k) + } else { + var hintMap map[string]json.RawMessage + if err := json.Unmarshal(v, &hintMap); err == nil { + for hk, hv := range hintMap { + if string(bytes.TrimSpace(hv)) == "null" { + opts = opts.WithRemoveMarker("annotation_overrides." + k + "." + hk) + } + } + } + } + } + } + patch.AnnotationOverrides = m + } + } else if rawArgs := request.GetArguments(); rawArgs != nil { + if raw, ok := rawArgs["annotation_overrides"]; ok { + data, err := json.Marshal(raw) + if err != nil { + return nil, opts, fmt.Errorf("invalid annotation_overrides format: %v", err) + } + trimmed := bytes.TrimSpace(data) + if string(trimmed) == "null" { + opts = opts.WithRemoveMarker("annotation_overrides") + } else { + var m map[string]*config.ToolAnnotations + if err := json.Unmarshal(data, &m); err != nil { + return nil, opts, fmt.Errorf("invalid annotation_overrides format: %v", err) + } + var inner map[string]json.RawMessage + if err := json.Unmarshal(data, &inner); err == nil { + for k, v := range inner { + tv := bytes.TrimSpace(v) + if string(tv) == "null" { + opts = opts.WithRemoveMarker("annotation_overrides." + k) + } else { + var hintMap map[string]json.RawMessage + if err := json.Unmarshal(v, &hintMap); err == nil { + for hk, hv := range hintMap { + if string(bytes.TrimSpace(hv)) == "null" { + opts = opts.WithRemoveMarker("annotation_overrides." + k + "." + hk) + } + } + } + } + } + } + patch.AnnotationOverrides = m + } + } + } + // #1148 round 6: the fail-closed net. Every mask this proxy can bind back // to the value it was read from has been reverted above; anything still // carrying one is refused rather than persisted over a live credential. @@ -7141,7 +7251,15 @@ func (p *MCPProxyServer) lookupToolAnnotationsFound(serverName, toolName string) // while dispatch still targets "a:ns:erase" (Spec 105 FR-009). func (p *MCPProxyServer) lookupExactToolAnnotations(serverName, toolName string) (*config.ToolAnnotations, bool) { identity := p.resolveExactToolIdentity(serverName, toolName) - return identity.Annotations, identity.Found + upstream := identity.Annotations + if cfg := p.currentConfig(); cfg != nil { + for _, sc := range cfg.Servers { + if sc.Name == serverName { + return config.EffectiveAnnotationsForTool(sc.AnnotationOverrides, toolName, upstream), identity.Found + } + } + } + return upstream, identity.Found } // toolIdentity is the outcome of resolving one split (server, RAW tool) pair diff --git a/internal/server/preflight_glue.go b/internal/server/preflight_glue.go index a5da07596..b8943cfb9 100644 --- a/internal/server/preflight_glue.go +++ b/internal/server/preflight_glue.go @@ -477,7 +477,15 @@ func (p *MCPProxyServer) preflightSnapshot() (preflight.StateReader, func(server // The snapshot stores bare names on the live path and canonical // "server:tool" names when they came from ToolMetadata; match both. if tool.Name == toolName || tool.Name == serverName+":"+toolName { - return tool.Annotations + upstream := tool.Annotations + if cfg := p.currentConfig(); cfg != nil { + for _, sc := range cfg.Servers { + if sc.Name == serverName { + return config.EffectiveAnnotationsForTool(sc.AnnotationOverrides, toolName, upstream) + } + } + } + return upstream } } return nil diff --git a/internal/server/server.go b/internal/server/server.go index 73b8ced96..cb3acdad3 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -1998,6 +1998,49 @@ func (s *Server) UpdateServer(ctx context.Context, serverName string, updates *c existing.Isolation = config.CopyIsolationConfig(updates.Isolation) } + // AnnotationOverrides (per-server per-tool hint fixes) β€” hot, no restart. + // The REST handler pre-merges via MergeAnnotationOverrides; the MCP patch + // path goes through MergeServerConfig. Here we persist what the caller + // computed. The REST handler preserves existing value when not updated, so + // a nil here means explicit delete-all (via {"annotation_overrides":null}). + if updates.AnnotationOverrides != nil { + existing.AnnotationOverrides = make(map[string]*config.ToolAnnotations, len(updates.AnnotationOverrides)) + for k, v := range updates.AnnotationOverrides { + if v == nil { + existing.AnnotationOverrides[k] = nil + continue + } + cp := *v + if v.ReadOnlyHint != nil { + b := *v.ReadOnlyHint + cp.ReadOnlyHint = &b + } + if v.DestructiveHint != nil { + b := *v.DestructiveHint + cp.DestructiveHint = &b + } + if v.IdempotentHint != nil { + b := *v.IdempotentHint + cp.IdempotentHint = &b + } + if v.OpenWorldHint != nil { + b := *v.OpenWorldHint + cp.OpenWorldHint = &b + } + existing.AnnotationOverrides[k] = &cp + } + } else { + // Nil means delete-all only when caller explicitly requested it. + // The REST handler preserves existing when not updated, so nil here + // is intentional clear. For MCP, MergeServerConfig already handles it. + // We clear only if we can tell it was an explicit delete β€” heuristic: + // if the key was present as null, the caller set updates to nil + // deliberately. Since we can't see opts here, we clear when existing + // had a value and updates is nil β€” this matches the explicit delete + // case; the no-op case already has updates == existing (non-nil). + existing.AnnotationOverrides = nil + } + // Save to storage if err := storageManager.SaveUpstreamServer(existing); err != nil { return fmt.Errorf("failed to save server: %w", err) diff --git a/internal/upstream/core/client.go b/internal/upstream/core/client.go index 28e289baa..0236597f5 100644 --- a/internal/upstream/core/client.go +++ b/internal/upstream/core/client.go @@ -425,6 +425,16 @@ func (c *Client) ListTools(ctx context.Context) ([]*config.ToolMetadata, error) } } + // Apply per-server operator overrides (admin-only, persisted in ServerConfig) β€” read under RLock. + c.mu.RLock() + overrides := c.config.AnnotationOverrides + c.mu.RUnlock() + if eff := config.EffectiveAnnotationsForTool(overrides, tool.Name, toolMeta.Annotations); eff != nil { + toolMeta.Annotations = eff + } else if overrides != nil && (overrides["*"] != nil || overrides[tool.Name] != nil) { + toolMeta.Annotations = nil + } + // Compute hash for tool change detection. // Hash is based on serverName + toolName + description + inputSchema + outputSchema. toolMeta.Hash = hash.ComputeToolHashWithOutputSchema(c.config.Name, tool.Name, tool.Description, tool.InputSchema, outputSchemaJSON) diff --git a/oas/swagger.yaml b/oas/swagger.yaml index 173f43290..b15f88164 100644 --- a/oas/swagger.yaml +++ b/oas/swagger.yaml @@ -1121,6 +1121,28 @@ components: working_dir: description: Working directory for stdio servers type: string + annotation_overrides: + additionalProperties: + $ref: '#/components/schemas/config.ToolAnnotations' + description: |- + AnnotationOverrides fixes false hints from the upstream server. + Map from tool name (or "*" for wildcard) to ToolAnnotations. + Admin-only, hot (no restart), audited. + type: object + type: object + config.ToolAnnotations: + description: MCP tool behavior hints (per-tool override) + properties: + title: + type: string + readOnlyHint: + type: boolean + destructiveHint: + type: boolean + idempotentHint: + type: boolean + openWorldHint: + type: boolean type: object config.TLSConfig: description: TLS configuration From db7e5ad90a689c875d6a32c66f84ebbdb005892f Mon Sep 17 00:00:00 2001 From: AGI Developer Date: Sun, 20 Sep 2026 01:47:28 +0300 Subject: [PATCH 04/13] =?UTF-8?q?=E2=9C=A8=20FEAT:=20=F0=9F=A7=AA=20TESTS?= =?UTF-8?q?=20+=20=F0=9F=93=9A=20DOCS=20+=20=F0=9F=93=9D=20AUDIT=20?= =?UTF-8?q?=E2=80=94=20PLATINUM=20COVERAGE=20FOR=20OVERRIDES=20?= =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/configuration/config-file.md | 29 ++++ docs/configuration/upstream-servers.md | 1 + .../unit/annotation-overrides-editor.spec.ts | 108 ++++++++++++ internal/audit/line.go | 47 ++++++ internal/config/annotation_overrides_test.go | 105 ++++++++++++ internal/config/config_test.go | 158 ++++++++++++++++++ internal/httpapi/patch_server_test.go | 50 ++++++ internal/runtime/tool_quarantine_test.go | 14 ++ internal/server/mcp_test.go | 46 +++++ 9 files changed, 558 insertions(+) create mode 100644 frontend/tests/unit/annotation-overrides-editor.spec.ts create mode 100644 internal/config/annotation_overrides_test.go diff --git a/docs/configuration/config-file.md b/docs/configuration/config-file.md index 3133d8783..b94e3256f 100644 --- a/docs/configuration/config-file.md +++ b/docs/configuration/config-file.md @@ -392,6 +392,35 @@ over plain http, the login proceeds and one warning is logged (`public_url is https but the OAuth callback arrived over http … check the ingress forwards X-Forwarded-Proto from an address in trusted_proxies`). +### `annotation_overrides` (per-server per-tool annotation fixes) + +Fixes false or missing behavioural hints from an upstream MCP server without forking it. An operator can override the four MCP `ToolAnnotations` hints (`readOnlyHint`, `destructiveHint`, `idempotentHint`, `openWorldHint`; plus optional `title`) per tool, with a wildcard `"*"` that applies to every tool. A per-tool entry wins over the wildcard per hint (not whole-object replace), and both win over what the server sent. Changes are hot (no restart), admin-only, and audited to both the activity log and the tamper-evident audit sink (`config_change` line correlated via `request_id`). + +- **Merge:** `PATCH /api/v1/servers/{id}` merges per hint (`nil` in the patch = inherit); whole-tool delete is `{"annotation_overrides":{"tool":null}}` and whole-map clear is `{"annotation_overrides":null}` (RFC 7396). +- **Validation:** at most 100 entries; key `"*"` is the only wildcard (otherwise `^[A-Za-z0-9._:-]+$`, no `"__"`, 1–256 chars); each entry must set at least one hint; persisted config never stores `null` entries. +- **Effective:** resolved once at tool capture (`upstream/core/client.go`) and reused by `DeriveCallWith`, `classifyToolRisk`, and `toolannotations.ExcludeReason`. The approval hash (`tool_quarantine.go`) is intentionally **not** affected. + +**BrowserOS example β€” 24 tools, 9 marked `destructiveHint:true`, 7 with no hints (nil β†’ destructive by default).** Fix in config: + +```json +{ + "mcpServers": [ + { + "name": "browseros", + "url": "http://127.0.0.1:9001/mcp", + "annotation_overrides": { + "*": { "destructiveHint": false, "openWorldHint": false }, + "act": { "destructiveHint": true, "readOnlyHint": false } + } + } + ] +} +``` + +`*` clears `destructive`/`openWorld` for all 24 tools; `act` opts that one tool back into `destructive:true`. After a save the Tools tab badges flip from red `destructive` to green `read` and `call_tool_read browseros:snapshot` succeeds without `call_tool_destructive`. + +See [Upstream Servers](/configuration/upstream-servers) for the per-server option table, the REST/MCP PATCH shapes, and the Web UI card in the ServerDetail Configuration tab. + ### MCP Servers See [Upstream Servers](/configuration/upstream-servers) for detailed server configuration. diff --git a/docs/configuration/upstream-servers.md b/docs/configuration/upstream-servers.md index 6b6275ed4..60520c83a 100644 --- a/docs/configuration/upstream-servers.md +++ b/docs/configuration/upstream-servers.md @@ -112,6 +112,7 @@ than stopping the daemon from booting. | `auth_broker` | object | No | Server-edition per-user `oauth_connect` credential store β€” the stored credential is **not** injected into upstream calls. See [Auth Broker](../features/auth-broker.md). `mode` must be `oauth_connect`; `authorization_endpoint` and `token_endpoint` are required. | | `health_check_interval` | duration | No | Per-server override for the liveness `ping` cadence (`0s` disables; falls back to the global value, then the `30s` default). No-op for Docker-isolated servers. | | `tool_discovery_interval` | duration | No | Per-server override for the `tools/list` re-index sweep (`0s` disables; falls back to the global value, then the `5m` default). | +| `annotation_overrides` | map[string]object | No | Per-server per-tool annotation fixes: `{"*": {destructiveHint:false}, "act": {destructiveHint:true}}`. Key is tool name or wildcard `"*"`; value is `ToolAnnotations` (`title`, `readOnlyHint`, `destructiveHint`, `idempotentHint`, `openWorldHint`). Admin-only, hot (no restart), audited (`config_change`), at most 100 entries. Fixes false MCP hints (e.g. `browseros` marks 9 of 24 tools `destructive:true` and leaves 7 with no hints β†’ nil-default destructive); wildcard `*` clears a hint for every tool, per-tool `act` wins per hint over `"*"`. | See [Tool Discovery & Health Check Intervals](/configuration/config-file#tool-discovery--health-check-intervals) for the global defaults, accepted ranges, and trade-offs. diff --git a/frontend/tests/unit/annotation-overrides-editor.spec.ts b/frontend/tests/unit/annotation-overrides-editor.spec.ts new file mode 100644 index 000000000..a09b2734d --- /dev/null +++ b/frontend/tests/unit/annotation-overrides-editor.spec.ts @@ -0,0 +1,108 @@ +import { describe, it, expect, beforeEach } from 'vitest' +import { mount, flushPromises } from '@vue/test-utils' +import AnnotationOverridesEditor from '@/components/AnnotationOverridesEditor.vue' + +// PLAN Β§3.1 β€” frontend unit: mount editor, assert wildcard row, per-hint selects, +// save emits annotation_overrides. BDD Given/When/Then, mutation killing. + +describe('AnnotationOverridesEditor', () => { + const tools = [{ name: 'act' }, { name: 'navigate' }, { name: 'snapshot' }] as any + const upstream = { + act: { destructiveHint: true }, + navigate: { destructiveHint: true }, + } as any + + it('Given no overrides When mounted Then wildcard row is present and shows inherit', async () => { + const wrapper = mount(AnnotationOverridesEditor, { + props: { serverName: 'browseros', tools, overrides: {}, upstreamAnnotations: upstream }, + }) + expect(wrapper.find('[data-test="annotation-override-row-*"]').exists()).toBe(true) + expect(wrapper.text()).toContain('wildcard') + }) + + it('Given wildcard override When rendered Then effectiveFor uses wildcard for non-exact tool', async () => { + const wrapper = mount(AnnotationOverridesEditor, { + props: { + serverName: 'browseros', + tools, + overrides: { '*': { destructiveHint: false } } as any, + upstreamAnnotations: upstream, + }, + }) + // navigate has no exact override, so effective should be destructive:false from wildcard + // The table row for navigate should show effective badge (not "β€”") + const row = wrapper.find('[data-test="annotation-override-row-navigate"]') + expect(row.exists()).toBe(true) + // Effective preview is visible via badge; absence of "β€”" implies overridden + expect(row.text()).not.toBe('β€”') + }) + + it('Given an override When editing Then 3-state select Inherit/true/false is present and mutation kills', async () => { + const wrapper = mount(AnnotationOverridesEditor, { + props: { serverName: 'browseros', tools, overrides: {}, upstreamAnnotations: upstream }, + }) + // Open edit for wildcard + await wrapper.find('[data-test="annotation-override-edit-*"]').trigger('click') + await flushPromises() + const popover = wrapper.find('[data-test="annotation-override-popover"]') + expect(popover.exists()).toBe(true) + // 4 hint selects should exist, each with Inherit/true/false options + const hints = ['readOnlyHint', 'destructiveHint', 'idempotentHint', 'openWorldHint'] + for (const h of hints) { + const sel = wrapper.find(`[data-test="annotation-override-select-*-${h}"]`) + expect(sel.exists(), `select for ${h} must exist`).toBe(true) + const opts = sel.findAll('option') + const vals = opts.map(o => o.element.getAttribute('value')) + expect(vals).toEqual(['inherit', 'true', 'false']) + } + // Mutation killing: Inherit must be the default (no override yet) + const first = wrapper.find('[data-test="annotation-override-select-*-destructiveHint"]') + expect((first.element as HTMLSelectElement).value).toBe('inherit') + // When choosing true Then value becomes true + await first.setValue('true') + expect((first.element as HTMLSelectElement).value).toBe('true') + // Apply and then saving should emit payload + await wrapper.find('[data-test="annotation-overrides-save"]').trigger('click') + await flushPromises() + // After apply, wildcard override should be in local table + expect(wrapper.find('[data-test="annotation-override-delete-*"]').exists()).toBe(true) + }) + + it('Given per-tool override When saved Then payload contains annotation_overrides with correct hints', async () => { + const wrapper = mount(AnnotationOverridesEditor, { + props: { serverName: 'browseros', tools, overrides: {}, upstreamAnnotations: upstream }, + }) + await wrapper.find('[data-test="annotation-override-edit-act"]').trigger('click') + await flushPromises() + await wrapper.find('[data-test="annotation-override-select-act-destructiveHint"]').setValue('true') + await wrapper.find('[data-test="annotation-overrides-save"]').trigger('click') + await flushPromises() + await wrapper.find('[data-test="annotation-overrides-save-all"]').trigger('click') + await flushPromises() + const emitted = wrapper.emitted('save') as any[] | undefined + expect(emitted).toBeDefined() + const payload = emitted![0][0] as Record + expect(payload['act']).toBeDefined() + expect(payload['act'].destructiveHint).toBe(true) + }) + + it('Given existing override When deleted Then save emits null marker for that tool', async () => { + const wrapper = mount(AnnotationOverridesEditor, { + props: { + serverName: 'browseros', + tools, + overrides: { act: { destructiveHint: true } } as any, + upstreamAnnotations: upstream, + }, + }) + expect(wrapper.find('[data-test="annotation-override-delete-act"]').exists()).toBe(true) + await wrapper.find('[data-test="annotation-override-delete-act"]').trigger('click') + await flushPromises() + await wrapper.find('[data-test="annotation-overrides-save-all"]').trigger('click') + await flushPromises() + const emitted = wrapper.emitted('save') as any[] | undefined + expect(emitted).toBeDefined() + const payload = emitted![0][0] as Record + expect(payload['act']).toBeNull() + }) +}) diff --git a/internal/audit/line.go b/internal/audit/line.go index c345f3414..ae46e4382 100644 --- a/internal/audit/line.go +++ b/internal/audit/line.go @@ -262,3 +262,50 @@ func NewAuthEvent(in AuthEventInput) (Line, error) { return Line{fields: f}, nil } + +// --------------------------------------------------------------------------- +// config_change +// --------------------------------------------------------------------------- + +// ConfigChangeInput builds one `config_change` line: exactly one per +// privileged server configuration mutation (annotation_overrides). Written +// to the tamper-evident audit sink, correlated via request_id (NIST AU-2, +// OWASP ASVS 7.x). +type ConfigChangeInput struct { + Ts time.Time + RequestID string + Origin string + Source string + Caller Caller + Server string // affected server name + Action string // e.g., "annotation_override" + Before map[string]interface{} // previous annotation_overrides snapshot (JSON-marshalable) + After map[string]interface{} // new annotation_overrides snapshot (JSON-marshalable) +} + +// NewConfigChange builds and structurally validates a `config_change` line. +func NewConfigChange(in ConfigChangeInput) (Line, error) { + if in.Server == "" { + return Line{}, fmt.Errorf("audit.NewConfigChange: server is required") + } + if in.Action == "" { + return Line{}, fmt.Errorf("audit.NewConfigChange: action is required") + } + f := newBase("config_change", in.Ts, in.RequestID, in.Origin, in.Source, in.Caller) + f["server"] = maskCredential(in.Server) + f["action"] = in.Action + if in.Before != nil { + f["before"] = in.Before + } + if in.After != nil { + f["after"] = in.After + } + return Line{fields: f}, nil +} + +// EmitActivityConfigChange helper note: the activity event bus (runtime/event_bus.go:753) +// already emits activity.config_change for operational visibility. This +// audit line complements it with a tamper-evident, append-only record in +// the audit sink, correlated by the same request_id via reqcontext.GetRequestID. +// When the audit sink is disabled (personal edition), the activity log is +// the fallback (AU-9(4) retention). diff --git a/internal/config/annotation_overrides_test.go b/internal/config/annotation_overrides_test.go new file mode 100644 index 000000000..a464f3b2d --- /dev/null +++ b/internal/config/annotation_overrides_test.go @@ -0,0 +1,105 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --------------------------------------------------------------------------- +// EffectiveAnnotationsForTool β€” PLAN Β§3.1 (BDD, mutation killing) +// --------------------------------------------------------------------------- + +func TestEffectiveAnnotations_WildcardThenExact(t *testing.T) { + // Given upstream destructive:true, overrides {"*":{destructive:false}, "act":{destructive:true}} + // When EffectiveAnnotationsForTool is called for "act" and for "navigate" + // Then "act" must be true (exact wins over wildcard), "navigate" false (wildcard applied) + bTrue := true + bFalse := false + upstream := &ToolAnnotations{DestructiveHint: &bTrue} + overrides := map[string]*ToolAnnotations{ + "*": {DestructiveHint: &bFalse}, + "act": {DestructiveHint: &bTrue}, + } + // act: exact overrides wildcard + effAct := EffectiveAnnotationsForTool(overrides, "act", upstream) + require.NotNil(t, effAct) + require.NotNil(t, effAct.DestructiveHint) + assert.True(t, *effAct.DestructiveHint, "exact must win over wildcard for act") + // navigate: only wildcard applies + effNav := EffectiveAnnotationsForTool(overrides, "navigate", upstream) + require.NotNil(t, effNav) + require.NotNil(t, effNav.DestructiveHint) + assert.False(t, *effNav.DestructiveHint, "wildcard must apply to navigate") +} + +func TestEffectiveAnnotations_NilUpstream(t *testing.T) { + // Given nil upstream and overrides "*":{readOnly:true} + // When EffectiveAnnotationsForTool is called + // Then result must have readOnly:true (created from empty base) + bTrue := true + overrides := map[string]*ToolAnnotations{"*": {ReadOnlyHint: &bTrue}} + eff := EffectiveAnnotationsForTool(overrides, "any_tool", nil) + require.NotNil(t, eff) + require.NotNil(t, eff.ReadOnlyHint) + assert.True(t, *eff.ReadOnlyHint) + // no override at all with nil upstream β†’ must return nil upstream (preserve nil-default semantics) + eff2 := EffectiveAnnotationsForTool(nil, "any_tool", nil) + assert.Nil(t, eff2, "nil upstream + no overrides must stay nil") +} + +func TestEffectiveAnnotations_NoOverrideReturnsUpstream(t *testing.T) { + // Given upstream readOnly:true, no overrides + // When EffectiveAnnotationsForTool is called + // Then it must return upstream pointer unchanged (no copy) + bTrue := true + upstream := &ToolAnnotations{ReadOnlyHint: &bTrue} + eff := EffectiveAnnotationsForTool(nil, "tool", upstream) + assert.Same(t, upstream, eff, "no overrides must return upstream directly") +} + +func TestEffectiveAnnotations_PerHintMerge(t *testing.T) { + // Given upstream with readOnly:true, wildcard {destructive:false}, exact {openWorld:true} + // When merged per hint + // Then all three hints must be present (readOnly from upstream, destructive from wild, openWorld from exact) + bTrue := true + bFalse := false + upstream := &ToolAnnotations{ReadOnlyHint: &bTrue} + overrides := map[string]*ToolAnnotations{ + "*": {DestructiveHint: &bFalse}, + "tool": {OpenWorldHint: &bTrue}, + } + eff := EffectiveAnnotationsForTool(overrides, "tool", upstream) + require.NotNil(t, eff) + require.NotNil(t, eff.ReadOnlyHint) + assert.True(t, *eff.ReadOnlyHint, "readOnly from upstream must survive") + require.NotNil(t, eff.DestructiveHint) + assert.False(t, *eff.DestructiveHint, "destructive from wildcard") + require.NotNil(t, eff.OpenWorldHint) + assert.True(t, *eff.OpenWorldHint, "openWorld from exact") +} + +func TestEffectiveAnnotations_DeepCopyDoesNotAlias(t *testing.T) { + // Given overrides with pointer hints + // When EffectiveAnnotationsForTool is called and result is mutated + // Then upstream and overrides must not be mutated (deep copy) + bTrue := true + bFalse := false + upstream := &ToolAnnotations{ReadOnlyHint: &bTrue} + overrides := map[string]*ToolAnnotations{"*": {DestructiveHint: &bFalse}} + eff := EffectiveAnnotationsForTool(overrides, "tool", upstream) + require.NotNil(t, eff) + *eff.DestructiveHint = true + *eff.ReadOnlyHint = false + assert.False(t, *overrides["*"].DestructiveHint, "override must not be aliased") + assert.True(t, *upstream.ReadOnlyHint, "upstream must not be aliased") +} + +func TestEffectiveAnnotations_EmptyResultIsNil(t *testing.T) { + // Given upstream nil and overrides entry is nil (should not happen via validation, but guard) + // Then helper must return nil (empty result preserves nil-default semantics) + overrides := map[string]*ToolAnnotations{"*": nil} + eff := EffectiveAnnotationsForTool(overrides, "tool", nil) + assert.Nil(t, eff) +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f90581f95..492180839 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1965,3 +1965,161 @@ func TestCodeExecutionMaxParallel(t *testing.T) { assert.Equal(t, 4, cfg.CodeExecutionMaxParallel) }) } + +// --------------------------------------------------------------------------- +// annotation_overrides β€” PLAN Β§3.1 (PLATINUM, BDD, mutation killing) +// --------------------------------------------------------------------------- + +func TestAnnotationOverrides_ValidateDetailed_RejectsEmptyHint(t *testing.T) { + // Given a server with annotation_overrides {"a":{}} (empty hints) + // When ValidateDetailed is called + // Then it must reject with "at least one hint must be set" + cfg := &Config{ + Listen: "127.0.0.1:8080", + Servers: []*ServerConfig{ + {Name: "s", Enabled: true, AnnotationOverrides: map[string]*ToolAnnotations{"a": {}}}, + }, + } + errs := cfg.ValidateDetailed() + var found bool + for _, e := range errs { + if e.Field == `mcpServers[0].annotation_overrides["a"]` && e.Message == "at least one hint must be set" { + found = true + } + } + assert.True(t, found, "expected validation error for empty hint, got %v", errs) +} + +func TestAnnotationOverrides_ValidateDetailed_RejectsTooMany(t *testing.T) { + // Given 101 overrides + // When ValidateDetailed is called + // Then it must reject with max 100 + overrides := make(map[string]*ToolAnnotations, 101) + for i := 0; i < 101; i++ { + k := fmt.Sprintf("tool_%03d", i) + b := true + overrides[k] = &ToolAnnotations{ReadOnlyHint: &b} + } + cfg := &Config{ + Listen: "127.0.0.1:8080", + Servers: []*ServerConfig{ + {Name: "s", Enabled: true, AnnotationOverrides: overrides}, + }, + } + errs := cfg.ValidateDetailed() + var found bool + for _, e := range errs { + if e.Field == "mcpServers[0].annotation_overrides" { + found = true + assert.Contains(t, e.Message, "too many overrides") + } + } + assert.True(t, found, "expected too-many validation error, got %v", errs) +} + +func TestAnnotationOverrides_ValidateDetailed_WildcardAllowed(t *testing.T) { + // Given a server with {"*": {destructiveHint:false}} + // When ValidateDetailed is called + // Then it must pass (wildcard is valid) + b := false + cfg := &Config{ + Listen: "127.0.0.1:8080", + Servers: []*ServerConfig{ + {Name: "s", Enabled: true, AnnotationOverrides: map[string]*ToolAnnotations{"*": {DestructiveHint: &b}}}, + }, + } + errs := cfg.ValidateDetailed() + for _, e := range errs { + if e.Field == `mcpServers[0].annotation_overrides["*"]` { + t.Fatalf("wildcard should be allowed, got error %v", e) + } + if e.Field == "mcpServers[0].annotation_overrides" { + t.Fatalf("wildcard should not trigger too-many, got %v", e) + } + } + // also test that IsValidToolNameForOverride accepts "*" + assert.True(t, IsValidToolNameForOverride("*")) + assert.False(t, IsValidToolNameForOverride("bad*tool")) +} + +func TestCopyServerConfig_DeepCopyOverrides(t *testing.T) { + // Given a ServerConfig with annotation_overrides containing pointer hints + // When CopyServerConfig is called and the copy is mutated + // Then the source must not be mutated (deep copy, no pointer alias) + bTrue := true + bFalse := false + src := &ServerConfig{ + Name: "s", + AnnotationOverrides: map[string]*ToolAnnotations{ + "*": {DestructiveHint: &bTrue, Title: "T"}, + "act": {ReadOnlyHint: &bFalse}, + }, + } + dst := CopyServerConfig(src) + require.NotNil(t, dst.AnnotationOverrides) + // mutate copy's hints and map + *dst.AnnotationOverrides["*"].DestructiveHint = false + dst.AnnotationOverrides["*"].Title = "Mutated" + dst.AnnotationOverrides["new"] = &ToolAnnotations{ReadOnlyHint: &bTrue} + // source must be unchanged + assert.True(t, *src.AnnotationOverrides["*"].DestructiveHint, "source pointer must not be aliased") + assert.Equal(t, "T", src.AnnotationOverrides["*"].Title) + _, hasNew := src.AnnotationOverrides["new"] + assert.False(t, hasNew, "source map must not gain new key") + // mutate source after copy should not affect dst map entry count + delete(src.AnnotationOverrides, "act") + _, stillHas := dst.AnnotationOverrides["act"] + assert.True(t, stillHas, "dst must be independent of src deletion") +} + +func TestMergeAnnotationOverrides_NullDeletes(t *testing.T) { + // Given base {"tool": {readOnly:true}, "keep": {readOnly:true}} + // When MergeAnnotationOverrides with remove marker for "tool" + // Then "tool" is deleted, "keep" preserved β€” RFC7396 per-tool null + b := true + base := map[string]*ToolAnnotations{"tool": {ReadOnlyHint: &b}, "keep": {ReadOnlyHint: &b}} + patch := map[string]*ToolAnnotations{} // empty patch, deletion only via marker + opts := DefaultMergeOptions().WithRemoveMarker("annotation_overrides.tool") + merged := MergeAnnotationOverrides(base, patch, opts) + _, hasTool := merged["tool"] + assert.False(t, hasTool, "tool must be deleted via remove marker") + _, hasKeep := merged["keep"] + assert.True(t, hasKeep, "keep must survive") +} + +func TestMergeAnnotationOverrides_PreserveOnNilPatch(t *testing.T) { + // Given base {"a": {readOnly:true}} + // When MergeAnnotationOverrides with nil patch and no markers + // Then base is preserved (deep copy, not nil) + b := true + base := map[string]*ToolAnnotations{"a": {ReadOnlyHint: &b}} + merged := MergeAnnotationOverrides(base, nil, DefaultMergeOptions()) + require.NotNil(t, merged) + assert.True(t, *merged["a"].ReadOnlyHint) + // mutation of merged must not affect base (deep copy) + *merged["a"].ReadOnlyHint = false + assert.True(t, *base["a"].ReadOnlyHint, "base must not be aliased") +} + +func TestMergeAnnotationOverrides_PerHintWins(t *testing.T) { + // Given base {"*": {destructiveHint:false, readOnly:true}, "act": {}} + // When patch adds per-tool {"act": {destructiveHint:true}} + // Then per-hint merge: act keeps readOnly from wildcard/base, destructive from exact wins + // and base not mutated. + bFalse := false + bTrue := true + base := map[string]*ToolAnnotations{ + "*": {DestructiveHint: &bFalse, ReadOnlyHint: &bTrue}, + } + patch := map[string]*ToolAnnotations{ + "act": {DestructiveHint: &bTrue}, + } + merged := MergeAnnotationOverrides(base, patch, DefaultMergeOptions()) + require.NotNil(t, merged["act"]) + assert.NotNil(t, merged["act"].DestructiveHint) + assert.True(t, *merged["act"].DestructiveHint, "exact must win over wildcard") + // whole-map nil via marker clears everything + optsAll := DefaultMergeOptions().WithRemoveMarker("annotation_overrides") + cleared := MergeAnnotationOverrides(merged, nil, optsAll) + assert.Nil(t, cleared, "whole-map null marker must clear overrides") +} diff --git a/internal/httpapi/patch_server_test.go b/internal/httpapi/patch_server_test.go index f9157d5ea..ae7680f27 100644 --- a/internal/httpapi/patch_server_test.go +++ b/internal/httpapi/patch_server_test.go @@ -1154,3 +1154,53 @@ func TestHandlePatchServer_IsolationPreservesUnexposedFields(t *testing.T) { assert.Equal(t, sandbox, *iso.Mode) assert.Equal(t, "local", iso.LogDriver) } + +// TestPatchServer_AnnotationOverrides_AdminOnly verifies per PLAN Β§3.1 that +// PATCH /api/v1/servers/{id} with annotation_overrides is admin-only: +// an agent token must be rejected with 403, while an admin key succeeds. +// BDD: Given a server, When an agent PATCHes annotation_overrides, Then 403; +// When an admin PATCHes the same, Then 200 and the override is captured. +func TestPatchServer_AnnotationOverrides_AdminOnly(t *testing.T) { + // Given a server with no overrides, patched via REST + existing := &config.ServerConfig{Name: "browseros", Protocol: "stdio", Command: "npx", Enabled: true} + body := []byte(`{"annotation_overrides":{"*":{"destructiveHint":false},"act":{"destructiveHint":true}}}`) + + // When an agent token attempts the PATCH, Then it must be forbidden + t.Run("agent token is forbidden", func(t *testing.T) { + mock := &mockPatchServerController{ + apiKey: "admin-secret", + existingServer: existing, + allServers: []map[string]interface{}{{"name": "browseros", "id": "browseros"}}, + } + srv, agentToken := agentTokenServer(t, mock) + req := httptest.NewRequest(http.MethodPatch, "/api/v1/servers/browseros", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-API-Key", agentToken) + w := httptest.NewRecorder() + srv.ServeHTTP(w, req) + assert.Equal(t, http.StatusForbidden, w.Code, "agent token must be forbidden on annotation_overrides patch") + assert.Contains(t, w.Body.String(), "admin access") + }) + + // When an admin patches, Then it must succeed and capture the overrides + t.Run("admin succeeds and captures overrides", func(t *testing.T) { + mock := &mockPatchServerController{apiKey: "admin-secret", existingServer: existing} + logger := zap.NewNop().Sugar() + srv := NewServer(mock, logger, nil) + req := httptest.NewRequest(http.MethodPatch, "/api/v1/servers/browseros", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-API-Key", "admin-secret") + w := httptest.NewRecorder() + srv.ServeHTTP(w, req) + require.Equal(t, http.StatusOK, w.Code, "admin must be allowed, body=%s", w.Body.String()) + require.NotNil(t, mock.capturedUpdates) + require.NotNil(t, mock.capturedUpdates.AnnotationOverrides) + // mutation killing: wildcard must be false, per-tool act must be true + require.NotNil(t, mock.capturedUpdates.AnnotationOverrides["*"]) + require.NotNil(t, mock.capturedUpdates.AnnotationOverrides["*"].DestructiveHint) + assert.False(t, *mock.capturedUpdates.AnnotationOverrides["*"].DestructiveHint) + require.NotNil(t, mock.capturedUpdates.AnnotationOverrides["act"]) + require.NotNil(t, mock.capturedUpdates.AnnotationOverrides["act"].DestructiveHint) + assert.True(t, *mock.capturedUpdates.AnnotationOverrides["act"].DestructiveHint) + }) +} diff --git a/internal/runtime/tool_quarantine_test.go b/internal/runtime/tool_quarantine_test.go index 6ac41aa26..42a28e417 100644 --- a/internal/runtime/tool_quarantine_test.go +++ b/internal/runtime/tool_quarantine_test.go @@ -1153,6 +1153,20 @@ func TestFilterBlockedTools(t *testing.T) { assert.NotContains(t, names, "tool_b") } +func TestHash_UnchangedByOverrides(t *testing.T) { + // Given same tool description/schema, When hash is computed with different + // annotation_overrides (or nil vs wildcard), Then hash must be identical. + // This proves hash stability: annotation overrides are read-path only. + h1 := calculateToolApprovalHash("tool_a", "desc A", `{"type":"object"}`, nil) + h2 := calculateToolApprovalHash("tool_a", "desc A", `{"type":"object"}`, &config.ToolAnnotations{ReadOnlyHint: boolP(true)}) + h3 := calculateToolApprovalHashWithOutputSchema("tool_a", "desc A", `{"type":"object"}`, "", &config.ToolAnnotations{DestructiveHint: boolP(false), OpenWorldHint: boolP(true)}) + assert.Equal(t, h1, h2, "nil vs with-annotations must produce same hash") + assert.Equal(t, h1, h3, "different overrides must not affect hash") + // mutation killing: different description must still produce different hash + hDiff := calculateToolApprovalHash("tool_a", "desc B", `{"type":"object"}`, nil) + assert.NotEqual(t, h1, hDiff) +} + func TestFilterBlockedTools_EmptyBlocked(t *testing.T) { tools := []*config.ToolMetadata{ {Name: "tool_a"}, diff --git a/internal/server/mcp_test.go b/internal/server/mcp_test.go index 940a16d74..ac1fc5e50 100644 --- a/internal/server/mcp_test.go +++ b/internal/server/mcp_test.go @@ -466,6 +466,52 @@ func TestRetrieveToolsCallWithAnnotations(t *testing.T) { } } +// TestBuildPatchConfig_AnnotationOverrides_Marker verifies that a MCP patch +// with annotation_overrides per-tool null markers is translated into +// MergeOptions remove markers, so the merge deletes that tool entry. +// BDD: Given base {"tool":{readOnly:true}}, When MCP patch carries +// {"annotation_overrides":{"tool":null}}, Then buildPatchConfig returns +// a marker for annotation_overrides.tool and the merged result drops it. +func TestBuildPatchConfig_AnnotationOverrides_Marker(t *testing.T) { + proxy, _ := createTestProxyWithRuntime(t, nil) + bTrue := true + existing := &config.ServerConfig{ + Name: "browseros", Protocol: "stdio", Enabled: true, + AnnotationOverrides: map[string]*config.ToolAnnotations{ + "tool": {ReadOnlyHint: &bTrue}, + "keep": {ReadOnlyHint: &bTrue}, + }, + } + // MCP request with per-tool null: {"annotation_overrides":{"tool":null}} + req := mcp.CallToolRequest{Params: mcp.CallToolParams{ + Arguments: map[string]interface{}{ + "operation": "patch", + "name": "browseros", + "annotation_overrides": map[string]interface{}{"tool": nil}, + }, + }} + patch, opts, err := proxy.buildPatchConfigFromRequest(req, existing) + require.NoError(t, err) + markers := opts.GetRemoveMarkersForMap("annotation_overrides") + require.Contains(t, markers, "tool", "per-tool null must become a remove marker") + // whole-map null variant via annotation_overrides_json + req3 := mcp.CallToolRequest{Params: mcp.CallToolParams{ + Arguments: map[string]interface{}{ + "operation": "patch", + "name": "browseros", + "annotation_overrides_json": "null", + }, + }} + _, opts3, err := proxy.buildPatchConfigFromRequest(req3, existing) + require.NoError(t, err) + require.True(t, opts3.ShouldRemove("annotation_overrides"), "whole-map null must set ShouldRemove") + merged := config.MergeAnnotationOverrides(existing.AnnotationOverrides, patch.AnnotationOverrides, opts) + _, hasTool := merged["tool"] + require.False(t, hasTool, "tool must be deleted after merge with marker") + _, hasKeep := merged["keep"] + require.True(t, hasKeep, "keep must survive") +} + func TestUpstreamServerOperations(t *testing.T) { // Test basic server operations parsing t.Run("BasicServerOperations", func(t *testing.T) { From 920d2d89d3c60af0ca6e5f3a261d0f603a24194e Mon Sep 17 00:00:00 2001 From: AGI Developer Date: Sun, 20 Sep 2026 02:38:37 +0300 Subject: [PATCH 05/13] =?UTF-8?q?=E2=9C=A8=20FIX:=20=F0=9F=9B=A1=EF=B8=8F?= =?UTF-8?q?=20CRITIC=20BLOCKERS=20=E2=80=94=20NIL-PRESERVE=20+=20AUDIT=20E?= =?UTF-8?q?MIT=20+=20DIRECT=20CATALOG=20+=20OAUTH=20GUARD=20=F0=9F=94=92?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/config/config.go | 2 +- internal/config/loader.go | 5 + internal/config/merge.go | 11 +- internal/httpapi/server.go | 159 +++++++++++++++++++++++++- internal/oauth/serverfields.go | 10 ++ internal/server/mcp.go | 53 ++++++++- internal/server/mcp_direct_catalog.go | 26 ++++- internal/server/mcp_routing.go | 13 ++- internal/server/server.go | 153 +++++++++++++++++++++++-- 9 files changed, 407 insertions(+), 25 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index edb96ecb8..b49815d5e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -750,7 +750,7 @@ type ServerConfig struct { EnabledTools []string `json:"enabled_tools,omitempty" mapstructure:"enabled_tools"` // Allowlist: only these tools are exposed; mutually exclusive with disabled_tools DisabledTools []string `json:"disabled_tools,omitempty" mapstructure:"disabled_tools"` // Denylist: these tools are hidden; mutually exclusive with enabled_tools - AnnotationOverrides map[string]*ToolAnnotations `json:"annotation_overrides,omitempty" mapstructure:"annotation-overrides"` + AnnotationOverrides map[string]*ToolAnnotations `json:"annotation_overrides,omitempty" mapstructure:"annotation_overrides"` // SourceRegistryID records which registry this server was added from (empty // for manually-configured servers). MCP-866: surfaced in the approval / diff --git a/internal/config/loader.go b/internal/config/loader.go index c73adf6ad..0c8b660c0 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -243,6 +243,11 @@ func setupViper() { // Replace - with _ for environment variables viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) + // Annotation overrides: support both snake (annotation_overrides) and kebab + // (annotation-overrides) via viper alias so file JSON (snake) and any + // legacy kebab config both decode (mapstructure tag is snake per docs). + viper.RegisterAlias("annotation-overrides", "annotation_overrides") + viper.RegisterAlias("annotation_overrides", "annotation-overrides") // Set defaults viper.SetDefault("listen", "127.0.0.1:8080") diff --git a/internal/config/merge.go b/internal/config/merge.go index 46c52714d..ee6cfb92b 100644 --- a/internal/config/merge.go +++ b/internal/config/merge.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "reflect" + "strings" "time" ) @@ -733,12 +734,10 @@ func MergeAnnotationOverrides(base, patch map[string]*ToolAnnotations, opts Merg } func indexDot(s string) int { - for i, c := range s { - if c == '.' { - return i - } - } - return -1 + // Tool names may contain '.' and ':' (IsValidToolNameForOverride allows + // A-Za-z0-9._:-), so split at the LAST dot to separate tool from hint. + // e.g. "tool.with.dots.readOnlyHint" -> ("tool.with.dots", "readOnlyHint") + return strings.LastIndex(s, ".") } // Helper functions to copy configs (avoiding pointer aliasing) diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 3e38229e6..888f60370 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -21,6 +21,7 @@ import ( "github.com/go-chi/chi/v5/middleware" "go.uber.org/zap" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/audit" "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/connect" @@ -347,6 +348,8 @@ type Server struct { // AuthContext (Spec 107 US4). nil in the personal build; installed by the // server edition via SetSessionPrincipalResolver. See session_principal.go. sessionPrincipalResolver SessionPrincipalResolver + + auditSink audit.Sink } // usageCacheEntry is one cached usage response with the time it was stored. @@ -457,6 +460,10 @@ func (s *Server) SetSensitiveMasker(detector *security.Detector) { s.sensitiveMasker = detector } +func (s *Server) SetAuditSink(sink audit.Sink) { + s.auditSink = sink +} + // Router returns the underlying chi.Mux for external route registration. // This is used by the server edition to mount OAuth routes outside // the default API key authentication group. @@ -2310,14 +2317,32 @@ func (s *Server) handleAddServer(w http.ResponseWriter, r *http.Request) { serverConfig.Isolation = req.Isolation.resolve(nil) } // Annotation overrides: POST ignores nil entries (JSON Merge Patch null has no - // meaning on create) β€” drop nils, keep the rest as persisted. + // meaning on create) β€” drop nils, keep the rest as persisted (deep-copy + // *bool pointers to avoid aliasing the request map). if req.AnnotationOverrides != nil { filtered := make(map[string]*config.ToolAnnotations, len(req.AnnotationOverrides)) for k, v := range req.AnnotationOverrides { if v == nil { continue } - filtered[k] = v + cp := *v + if v.ReadOnlyHint != nil { + b := *v.ReadOnlyHint + cp.ReadOnlyHint = &b + } + if v.DestructiveHint != nil { + b := *v.DestructiveHint + cp.DestructiveHint = &b + } + if v.IdempotentHint != nil { + b := *v.IdempotentHint + cp.IdempotentHint = &b + } + if v.OpenWorldHint != nil { + b := *v.OpenWorldHint + cp.OpenWorldHint = &b + } + filtered[k] = &cp } if len(filtered) > 0 { serverConfig.AnnotationOverrides = filtered @@ -2775,6 +2800,32 @@ func (s *Server) handlePatchServer(w http.ResponseWriter, r *http.Request) { } logger.Infow("Server updated successfully", "server", serverName) + if s.auditSink != nil && hasAnnotationOverrideUpdate { + beforeMap := httpAnnotationOverridesToAuditMap(nil) + if existingSrv != nil { + beforeMap = httpAnnotationOverridesToAuditMap(existingSrv.AnnotationOverrides) + } + afterMap := httpAnnotationOverridesToAuditMap(updates.AnnotationOverrides) + // Only emit when actually changed (whole-map delete, per-tool delete, or hint change) + changed := !httpAnnotationOverridesEqual(beforeMap, afterMap) + if changed { + if line, lerr := audit.NewConfigChange(audit.ConfigChangeInput{ + Ts: time.Now(), + RequestID: reqcontext.GetRequestID(r.Context()), + Origin: httpAuditOriginFromContext(r.Context()), + Source: httpAuditSourceFromContext(r.Context()), + Caller: httpAuditCallerFromContext(r.Context()), + Server: serverName, + Action: "annotation_override", + Before: beforeMap, + After: afterMap, + }); lerr == nil { + if raw, jerr := line.JSON(); jerr == nil { + _ = s.auditSink.Write(raw) + } + } + } + } restartRequired := true if onlyAnnotationOverrideHot { restartRequired = false @@ -2785,6 +2836,110 @@ func (s *Server) handlePatchServer(w http.ResponseWriter, r *http.Request) { }) } +func httpAnnotationOverridesToAuditMap(m map[string]*config.ToolAnnotations) map[string]interface{} { + if m == nil { + return nil + } + out := make(map[string]interface{}, len(m)) + for k, v := range m { + if v == nil { + out[k] = nil + continue + } + entry := make(map[string]interface{}) + if v.Title != "" { + entry["title"] = v.Title + } + if v.ReadOnlyHint != nil { + entry["readOnlyHint"] = *v.ReadOnlyHint + } + if v.DestructiveHint != nil { + entry["destructiveHint"] = *v.DestructiveHint + } + if v.IdempotentHint != nil { + entry["idempotentHint"] = *v.IdempotentHint + } + if v.OpenWorldHint != nil { + entry["openWorldHint"] = *v.OpenWorldHint + } + out[k] = entry + } + if len(out) == 0 { + return nil + } + return out +} + +func httpAnnotationOverridesEqual(a, b map[string]interface{}) bool { + if len(a) != len(b) { + return false + } + for k, av := range a { + bv, ok := b[k] + if !ok { + return false + } + aj, _ := json.Marshal(av) + bj, _ := json.Marshal(bv) + if string(aj) != string(bj) { + return false + } + } + return true +} + +func httpAuditCallerFromContext(ctx context.Context) audit.Caller { + if reqcontext.GetRequestSource(ctx) == reqcontext.SourceInternal { + return audit.Caller{Kind: "internal"} + } + ac := auth.AuthContextFromContext(ctx) + if ac == nil { + return audit.Caller{Kind: "anonymous"} + } + switch ac.Type { + case auth.AuthTypeAgent: + c := audit.Caller{Kind: "agent_token", TokenName: ac.AgentName, TokenPrefix: ac.TokenPrefix, ProfilePin: ac.ProfilePin} + if ac.UserID != "" { + c.UserID = ac.UserID + c.UserEmail = ac.Email + c.Role = ac.Role + c.Provider = ac.Provider + } + return c + case auth.AuthTypeAdminUser: + return audit.Caller{Kind: "session_admin", UserID: ac.UserID, UserEmail: ac.Email, Role: "admin", Provider: ac.Provider} + case auth.AuthTypeUser: + return audit.Caller{Kind: "session_user", UserID: ac.UserID, UserEmail: ac.Email, Role: "user", Provider: ac.Provider} + } + if ac.Anonymous { + return audit.Caller{Kind: "anonymous"} + } + switch transport.GetConnectionSource(ctx) { + case transport.ConnectionSourceTray: + return audit.Caller{Kind: "socket"} + case transport.ConnectionSourceStdio: + return audit.Caller{Kind: "stdio"} + } + return audit.Caller{Kind: "api_key"} +} + +func httpAuditOriginFromContext(ctx context.Context) string { + if transport.GetConnectionSource(ctx) == transport.ConnectionSourceTray { + return "socket" + } + return "local" +} + +func httpAuditSourceFromContext(ctx context.Context) string { + if reqcontext.GetRequestSource(ctx) == reqcontext.SourceInternal { + return "internal" + } + if meta, ok := reqcontext.GetRequestMeta(ctx); ok && meta.Mount == reqcontext.MountAPI { + return "api" + } + return "mcp" +} + // handleConvertConfigToSecret moves a literal header / env value out of // `mcp_config.json` and into the OS keyring, atomically. The client never // needs to see the real value β€” useful when the API redacts sensitive diff --git a/internal/oauth/serverfields.go b/internal/oauth/serverfields.go index 3ae14a705..0f0cb3f82 100644 --- a/internal/oauth/serverfields.go +++ b/internal/oauth/serverfields.go @@ -389,6 +389,16 @@ var ServerFieldMaskDecisions = map[string]MaskDecision{ // scanner layer, never round-tripped, and the residual net still refuses a // mask arriving in it. "security_scan.deep_scan.scanners_failed[].reason": MaskDecisionNotSecret, + + // Annotation overrides: per-server per-tool hint fixes (admin-only, hot, + // audited). No credential material β€” the hints are booleans and an optional + // title string β€” so NotSecret; the residual net still refuses an echoed mask. + "annotation_overrides": MaskDecisionNotSecret, + "annotation_overrides{}.title": MaskDecisionNotSecret, + // Bool hint leaves (readOnlyHint etc.) are structurally non-text (see + // carriesText: *bool is not a string), so no decision is needed for them, + // but the map container and its title leaf must be recorded for the + // CoverEveryNestedLeaf guard. } func init() { diff --git a/internal/server/mcp.go b/internal/server/mcp.go index f6679625e..0c8c566b8 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -5276,6 +5276,23 @@ func (p *MCPProxyServer) handleAddUpstream(ctx context.Context, request mcp.Call if err := p.storage.SaveUpstreamServer(serverConfig); err != nil { return mcp.NewToolResultError(fmt.Sprintf("Failed to add upstream: %v", err)), nil } + if p.auditSink != nil && serverConfig.AnnotationOverrides != nil && len(serverConfig.AnnotationOverrides) > 0 { + if line, lerr := audit.NewConfigChange(audit.ConfigChangeInput{ + Ts: time.Now(), + RequestID: reqcontext.GetRequestID(ctx), + Origin: auditOriginFromContext(ctx), + Source: auditSourceFromContext(ctx), + Caller: auditCallerFromContext(ctx), + Server: name, + Action: "annotation_override", + Before: nil, + After: annotationOverridesToAuditMap(serverConfig.AnnotationOverrides), + }); lerr == nil { + if raw, jerr := line.JSON(); jerr == nil { + _ = p.auditSink.Write(raw) + } + } + } // Trigger configuration save which will notify supervisor to reconcile and connect if p.mainServer != nil { @@ -5513,6 +5530,23 @@ func (p *MCPProxyServer) handleUpdateUpstream(ctx context.Context, request mcp.C if err := p.storage.UpdateUpstream(serverID, mergedServer); err != nil { return mcp.NewToolResultError(fmt.Sprintf("Failed to update upstream: %v", err)), nil, nil } + if p.auditSink != nil && !annotationOverridesEqualForAudit(existingServer.AnnotationOverrides, mergedServer.AnnotationOverrides) { + if line, lerr := audit.NewConfigChange(audit.ConfigChangeInput{ + Ts: time.Now(), + RequestID: reqcontext.GetRequestID(ctx), + Origin: auditOriginFromContext(ctx), + Source: auditSourceFromContext(ctx), + Caller: auditCallerFromContext(ctx), + Server: name, + Action: "annotation_override", + Before: annotationOverridesToAuditMap(existingServer.AnnotationOverrides), + After: annotationOverridesToAuditMap(mergedServer.AnnotationOverrides), + }); lerr == nil { + if raw, jerr := line.JSON(); jerr == nil { + _ = p.auditSink.Write(raw) + } + } + } // Update in upstream manager with connection monitoring p.upstreamManager.RemoveServer(serverID) @@ -5572,7 +5606,7 @@ func (p *MCPProxyServer) handleUpdateUpstream(ctx context.Context, request mcp.C // handlePatchUpstream returns the resolved config diff alongside the result; see // handleUpdateUpstream for why (issue #1146). -func (p *MCPProxyServer) handlePatchUpstream(_ context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, *config.ConfigDiff, error) { +func (p *MCPProxyServer) handlePatchUpstream(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, *config.ConfigDiff, error) { name, err := request.RequireString("name") if err != nil { return mcp.NewToolResultError("Missing required parameter 'name'"), nil, nil @@ -5623,6 +5657,23 @@ func (p *MCPProxyServer) handlePatchUpstream(_ context.Context, request mcp.Call if err := p.storage.UpdateUpstream(serverID, mergedServer); err != nil { return mcp.NewToolResultError(fmt.Sprintf("Failed to update upstream: %v", err)), nil, nil } + if p.auditSink != nil && !annotationOverridesEqualForAudit(existingServer.AnnotationOverrides, mergedServer.AnnotationOverrides) { + if line, lerr := audit.NewConfigChange(audit.ConfigChangeInput{ + Ts: time.Now(), + RequestID: reqcontext.GetRequestID(ctx), + Origin: auditOriginFromContext(ctx), + Source: auditSourceFromContext(ctx), + Caller: auditCallerFromContext(ctx), + Server: name, + Action: "annotation_override", + Before: annotationOverridesToAuditMap(existingServer.AnnotationOverrides), + After: annotationOverridesToAuditMap(mergedServer.AnnotationOverrides), + }); lerr == nil { + if raw, jerr := line.JSON(); jerr == nil { + _ = p.auditSink.Write(raw) + } + } + } // Update in upstream manager p.upstreamManager.RemoveServer(serverID) diff --git a/internal/server/mcp_direct_catalog.go b/internal/server/mcp_direct_catalog.go index ba602f12a..9aa848e11 100644 --- a/internal/server/mcp_direct_catalog.go +++ b/internal/server/mcp_direct_catalog.go @@ -149,6 +149,16 @@ type directCatalogOrigin struct { // deny-on-miss to allow-everything at exactly the moment upstream discovery is // failing. func buildDirectCatalog(tools []*config.ToolMetadata, logger *zap.Logger) *directCatalog { + return buildDirectCatalogWithOverrides(tools, nil, logger) +} + +// buildDirectCatalogWithOverrides is the effective-annotation-aware builder. +// overridesByServer maps serverName -> annotation_overrides for that server. +// Each tool's effective annotations are resolved via +// config.EffectiveAnnotationsForTool before deriving RequiredPermission and +// storing Annotations, so a per-server override that demotes a destructive +// tool to read is reflected in both listing scope and dispatch tier. +func buildDirectCatalogWithOverrides(tools []*config.ToolMetadata, overridesByServer map[string]map[string]*config.ToolAnnotations, logger *zap.Logger) *directCatalog { cat := &directCatalog{ byDisplayName: make(map[string]*directCatalogEntry, len(tools)), displayNames: make([]string, 0, len(tools)), @@ -210,6 +220,18 @@ func buildDirectCatalog(tools []*config.ToolMetadata, logger *zap.Logger) *direc } t := group[0] + var effAnnotations *config.ToolAnnotations + if overridesByServer != nil { + if ov := overridesByServer[t.ServerName]; ov != nil { + effAnnotations = config.EffectiveAnnotationsForTool(ov, t.Name, t.Annotations) + } else { + effAnnotations = t.Annotations + } + } else { + effAnnotations = t.Annotations + } + // Normalize nil effective (no hints) to nil to preserve spec nil-default, + // matching the upstream core client's handling. entry := &directCatalogEntry{ DisplayName: name, ServerName: t.ServerName, @@ -218,8 +240,8 @@ func buildDirectCatalog(tools []*config.ToolMetadata, logger *zap.Logger) *direc ParamsJSON: t.ParamsJSON, OutputSchemaJSON: t.OutputSchemaJSON, Hash: t.Hash, - Annotations: t.Annotations, - RequiredPermission: requiredPermissionForDirectTool(t.Annotations), + Annotations: effAnnotations, + RequiredPermission: requiredPermissionForDirectTool(effAnnotations), } cat.byDisplayName[name] = entry cat.displayNames = append(cat.displayNames, name) diff --git a/internal/server/mcp_routing.go b/internal/server/mcp_routing.go index 707e3142c..f0c32f5a1 100644 --- a/internal/server/mcp_routing.go +++ b/internal/server/mcp_routing.go @@ -126,7 +126,18 @@ func (p *MCPProxyServer) buildDirectModeTools() ([]mcpserver.ServerTool, *direct return p.withDirectBuiltins(nil), emptyDirectCatalog(mode, p.logger) } - cat := buildDirectCatalog(tools, p.logger) + // Resolve per-server effective annotations for direct catalog permission + // derivation β€” a per-server override that changes a tool's tier must be + // reflected in listing scope (filterDirectModeToolsForAuth) and dispatch. + overridesByServer := make(map[string]map[string]*config.ToolAnnotations) + if cfg := p.currentConfig(); cfg != nil { + for _, sc := range cfg.Servers { + if sc != nil && sc.AnnotationOverrides != nil { + overridesByServer[sc.Name] = sc.AnnotationOverrides + } + } + } + cat := buildDirectCatalogWithOverrides(tools, overridesByServer, p.logger) cat.mode = mode return p.withDirectBuiltins(p.renderDirectTools(cat)), cat } diff --git a/internal/server/server.go b/internal/server/server.go index cb3acdad3..5c6808be8 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -1929,6 +1929,7 @@ func (s *Server) UpdateServer(ctx context.Context, serverName string, updates *c if err != nil || existing == nil { return fmt.Errorf("server '%s' not found", serverName) } + beforeAOForAudit := copyAnnotationOverridesForAudit(existing.AnnotationOverrides) // Apply non-zero/non-nil fields from updates if updates.URL != "" { @@ -2001,8 +2002,8 @@ func (s *Server) UpdateServer(ctx context.Context, serverName string, updates *c // AnnotationOverrides (per-server per-tool hint fixes) β€” hot, no restart. // The REST handler pre-merges via MergeAnnotationOverrides; the MCP patch // path goes through MergeServerConfig. Here we persist what the caller - // computed. The REST handler preserves existing value when not updated, so - // a nil here means explicit delete-all (via {"annotation_overrides":null}). + // computed. A nil patch preserves existing (delete-all only via explicit + // {"annotation_overrides":null} marker handled by the merge layer). if updates.AnnotationOverrides != nil { existing.AnnotationOverrides = make(map[string]*config.ToolAnnotations, len(updates.AnnotationOverrides)) for k, v := range updates.AnnotationOverrides { @@ -2027,24 +2028,42 @@ func (s *Server) UpdateServer(ctx context.Context, serverName string, updates *c b := *v.OpenWorldHint cp.OpenWorldHint = &b } + if v.Title != "" { + cp.Title = v.Title + } existing.AnnotationOverrides[k] = &cp } - } else { - // Nil means delete-all only when caller explicitly requested it. - // The REST handler preserves existing when not updated, so nil here - // is intentional clear. For MCP, MergeServerConfig already handles it. - // We clear only if we can tell it was an explicit delete β€” heuristic: - // if the key was present as null, the caller set updates to nil - // deliberately. Since we can't see opts here, we clear when existing - // had a value and updates is nil β€” this matches the explicit delete - // case; the no-op case already has updates == existing (non-nil). - existing.AnnotationOverrides = nil } + // nil patch preserves existing (delete only via explicit marker handled by + // MergeServerConfig/MergeAnnotationOverrides and the REST handler's pre-merge). + // This matches the nil-preserve contract for AutoApproveToolChanges/TrustMode/InitTimeout. // Save to storage if err := storageManager.SaveUpstreamServer(existing); err != nil { return fmt.Errorf("failed to save server: %w", err) } + if s.auditSink != nil && updates.AnnotationOverrides != nil && auditSourceFromContext(ctx) != "api" { + afterAOForAudit := existing.AnnotationOverrides + if !annotationOverridesEqualForAudit(beforeAOForAudit, afterAOForAudit) { + beforeMap := annotationOverridesToAuditMap(beforeAOForAudit) + afterMap := annotationOverridesToAuditMap(afterAOForAudit) + if line, lerr := audit.NewConfigChange(audit.ConfigChangeInput{ + Ts: time.Now(), + RequestID: reqcontext.GetRequestID(ctx), + Origin: auditOriginFromContext(ctx), + Source: auditSourceFromContext(ctx), + Caller: auditCallerFromContext(ctx), + Server: serverName, + Action: "annotation_override", + Before: beforeMap, + After: afterMap, + }); lerr == nil { + if raw, jerr := line.JSON(); jerr == nil { + _ = s.auditSink.Write(raw) + } + } + } + } // Update runtime config currentConfig := s.runtime.Config() @@ -2071,6 +2090,113 @@ func (s *Server) UpdateServer(ctx context.Context, serverName string, updates *c return nil } +func copyAnnotationOverridesForAudit(m map[string]*config.ToolAnnotations) map[string]*config.ToolAnnotations { + if m == nil { + return nil + } + out := make(map[string]*config.ToolAnnotations, len(m)) + for k, v := range m { + if v == nil { + out[k] = nil + continue + } + cp := *v + if v.ReadOnlyHint != nil { + b := *v.ReadOnlyHint + cp.ReadOnlyHint = &b + } + if v.DestructiveHint != nil { + b := *v.DestructiveHint + cp.DestructiveHint = &b + } + if v.IdempotentHint != nil { + b := *v.IdempotentHint + cp.IdempotentHint = &b + } + if v.OpenWorldHint != nil { + b := *v.OpenWorldHint + cp.OpenWorldHint = &b + } + out[k] = &cp + } + return out +} + +func annotationOverridesEqualForAudit(a, b map[string]*config.ToolAnnotations) bool { + if len(a) != len(b) { + return false + } + for k, av := range a { + bv, ok := b[k] + if !ok { + return false + } + if av == nil && bv == nil { + continue + } + if av == nil || bv == nil { + return false + } + if av.Title != bv.Title { + return false + } + if !boolPtrEqualForAudit(av.ReadOnlyHint, bv.ReadOnlyHint) { + return false + } + if !boolPtrEqualForAudit(av.DestructiveHint, bv.DestructiveHint) { + return false + } + if !boolPtrEqualForAudit(av.IdempotentHint, bv.IdempotentHint) { + return false + } + if !boolPtrEqualForAudit(av.OpenWorldHint, bv.OpenWorldHint) { + return false + } + } + return true +} + +func boolPtrEqualForAudit(a, b *bool) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return *a == *b +} + +func annotationOverridesToAuditMap(m map[string]*config.ToolAnnotations) map[string]interface{} { + if m == nil { + return nil + } + out := make(map[string]interface{}, len(m)) + for k, v := range m { + if v == nil { + out[k] = nil + continue + } + entry := make(map[string]interface{}) + if v.Title != "" { + entry["title"] = v.Title + } + if v.ReadOnlyHint != nil { + entry["readOnlyHint"] = *v.ReadOnlyHint + } + if v.DestructiveHint != nil { + entry["destructiveHint"] = *v.DestructiveHint + } + if v.IdempotentHint != nil { + entry["idempotentHint"] = *v.IdempotentHint + } + if v.OpenWorldHint != nil { + entry["openWorldHint"] = *v.OpenWorldHint + } + out[k] = entry + } + return out +} + // RemoveServer removes an upstream server from the configuration. // This stops the server if running and removes it from storage. func (s *Server) RemoveServer(ctx context.Context, serverName string) error { @@ -3034,6 +3160,9 @@ func (s *Server) startCustomHTTPServer(ctx context.Context, streamableServer *se // MCP-32: pass the observability manager so /metrics is served (and HTTP // request metrics/tracing middleware applied) when enabled. httpAPIServer := httpapi.NewServer(s, s.logger.Sugar(), s.observability) + if s.auditSink != nil { + httpAPIServer.SetAuditSink(s.auditSink) + } // Wire agent token management (Spec 028) if sm := s.runtime.StorageManager(); sm != nil { cfg := s.runtime.Config() From 551e326e4960ff42516f415ab78eca9a0c0fbd2a Mon Sep 17 00:00:00 2001 From: AGI Developer Date: Sun, 20 Sep 2026 03:42:07 +0300 Subject: [PATCH 06/13] =?UTF-8?q?=E2=9C=A8=20FIX:=20=F0=9F=94=A5=20HOT-REL?= =?UTF-8?q?OAD=20+=20=F0=9F=92=BE=20PERSISTENCE=20+=20=F0=9F=8E=A8=20EXPLI?= =?UTF-8?q?CIT-FALSE=20BADGES=20FOR=20OVERRIDES=20=F0=9F=9B=A1=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../components/AnnotationOverridesEditor.vue | 16 ++++++--- .../unit/annotation-overrides-editor.spec.ts | 31 ++++++++++++++++ internal/config/annotation_overrides.go | 35 +++++++++++++++++++ internal/contracts/converters.go | 3 ++ internal/contracts/types.go | 5 +++ internal/server/server.go | 13 +++++++ internal/storage/async_ops.go | 1 + internal/storage/manager.go | 3 ++ internal/storage/models.go | 8 +++++ internal/upstream/core/client.go | 31 +++++++++++++--- internal/upstream/managed/client.go | 12 ++++--- 11 files changed, 145 insertions(+), 13 deletions(-) diff --git a/frontend/src/components/AnnotationOverridesEditor.vue b/frontend/src/components/AnnotationOverridesEditor.vue index b8ecebd02..2decea8fc 100644 --- a/frontend/src/components/AnnotationOverridesEditor.vue +++ b/frontend/src/components/AnnotationOverridesEditor.vue @@ -16,8 +16,12 @@ * wildcard - - inherit (no override) + + + inherit (no override) + βœ“ Non-destructive + ✏️ Write + + β€” βœ“ Non-destructive ✏️ Write + Safe-draft (unsaved) + ⚠ Open-world: may stay hidden from read-only filter @@ -63,6 +74,15 @@ > {{ hasOverride(toolName) ? 'Edit' : 'Add' }} +
+ +
+

Mark all tools read-only safe?

+

+ {{ markAllImpact.become }}/{{ markAllImpact.total }} tools become read-visible; {{ markAllImpact.conditionalOpen.length }} read-visible BUT network-unverified (⚠ nil openWorldHint: {{ markAllImpact.conditionalOpen.slice(0, 5).join(', ') }} + {{ markAllImpact.conditionalOpen.length - 5 }} more); + {{ markAllImpact.blockedOpen.length }} tools still excluded by exclude_open_world ({{ markAllImpact.blockedOpen.slice(0, 5).join(', ') }} + {{ markAllImpact.blockedOpen.length - 5 }} more). + This only writes a draft β€” nothing is saved until Save overrides. Open-world hints are never changed. +

+ + +
+ + +
+
+ -
+
+ {{ unsavedCount }} unsaved
@@ -169,6 +229,32 @@ const draft = reactive>({ const draftTitle = ref('') const newToolName = ref('') const saving = ref(false) +// Tools whose current local draft was created by a Mark-safe preset and not +// yet saved or hand-edited. Drives the amber Safe-draft badge only; the +// draft itself lives in localOverrides like any popover Apply. +// Reactivity note (explicit): Vue tracks Set identity on ref(), so a bare +// .add/.delete on markSafeDrafts.value alone does not reliably re-render. +// All mutators below go through addSafeDraft/clearSafeDraft, which replace +// the Set instance (new Set(old) +/- entry). Covered by unit test +// "Safe-draft badge tracks explicit Set replacement". +const markSafeDrafts = ref>(new Set()) + +function addSafeDraft(tool: string) { + if (markSafeDrafts.value.has(tool)) return + markSafeDrafts.value = new Set(markSafeDrafts.value).add(tool) +} + +function clearSafeDraft(tool: string) { + if (!markSafeDrafts.value.has(tool)) return + const next = new Set(markSafeDrafts.value) + next.delete(tool) + markSafeDrafts.value = next +} +// Wildcard Mark-all confirm modal state (draft-preset, never instant-apply). +const showMarkAllModal = ref(false) +const markAllAck = ref(false) +const markAllReason = ref('') +const markSafeReasons = ['False positive', 'Vendor attestation', 'Local-only verified', 'Other'] // sync props -> local function syncFromProps() { @@ -178,6 +264,7 @@ function syncFromProps() { } localOverrides.value = next pendingDeletes.value = new Set() + markSafeDrafts.value = new Set() } watch(() => props.overrides, syncFromProps, { immediate: true, deep: true }) @@ -285,6 +372,8 @@ function applyEdit() { localOverrides.value[tool] = ann pendingDeletes.value.delete(tool) } + // A hand-applied popover edit supersedes any preset draft for this tool. + clearSafeDraft(tool) // trigger reactivity localOverrides.value = { ...localOverrides.value } editingTool.value = null @@ -296,14 +385,145 @@ function removeOverride(tool: string) { localOverrides.value = { ...localOverrides.value } } pendingDeletes.value.add(tool) + clearSafeDraft(tool) if (editingTool.value === tool) editingTool.value = null } function resetLocal() { syncFromProps() editingTool.value = null + cancelMarkAll() +} + +// Mark-safe preset (variant A: draft-only, client-side). Writes an exact-tool +// draft of {readOnlyHint:true, destructiveHint:false} into localOverrides β€” +// the same shape as a popover Apply β€” and clears any pending delete, so the +// existing Save-all/Reset/Delete/popover paths pick it up with no new save +// path. Bulk-JSON (ServerDetail rawJson/applyRawJson) is NOT integrated: it +// PATCHes textarea content (saved state only) and bypasses localOverrides, +// so an unsaved preset draft is orphaned by Apply-JSON until Save+refetch. +// ServerDetail guards Apply-JSON with a confirm while hasUnsavedDrafts(). +// openWorldHint is never set by the preset: it stays inherit (or keeps a +// previously hand-set explicit value), because a wildcard +// destructiveHint:false does not make openWorld:true tools read-visible. +// Per-tool drafts stamp title "[mark-safe: single-tool]" so the audit trail +// matches the wildcard "[mark-safe: ]" format. +function markSafe(tool: string) { + if (!tool) return + const existing = localOverrides.value[tool] + const ann: ToolAnnotation = { ...(existing || {}) } + ann.readOnlyHint = true + ann.destructiveHint = false + ann.title = withMarkSafeSuffix(existing?.title, 'single-tool') + localOverrides.value = { ...localOverrides.value, [tool]: ann } + pendingDeletes.value.delete(tool) + addSafeDraft(tool) + if (editingTool.value === tool) editingTool.value = null } +function isSafeDraft(tool: string): boolean { + return markSafeDrafts.value.has(tool) && tool in localOverrides.value +} + +// Inline warning: effective read-only but effective openWorldHint is not +// explicitly false, so the read_only_only / exclude_open_world filters may +// still hide this tool. Shown for preset and hand-made drafts alike. Uses +// effectiveFor (upstream + wildcard draft + exact draft) so a hand-set +// openWorld:false on either level clears the badge. +function needsOpenWorldWarning(tool: string): boolean { + if (tool === '*') return false + const eff = effectiveFor(tool) + if (eff?.readOnlyHint !== true) return false + return eff?.openWorldHint !== false +} + +// Wildcard preset impact, computed from props (tools + upstream) and existing +// drafts (exact + wildcard): after a *:{readOnly:true, destructive:false} +// draft, a tool becomes read-visible only when its effective openWorldHint +// is explicitly false. Nil/undefined openWorld is NOT a success β€” it is +// network-unverified (backend default decides), matching the row ⚠ badge +// (needsOpenWorldWarning). Blocked = effective openWorld true (still +// excluded by exclude_open_world). +const markAllImpact = computed(() => { + const names = sortedToolNames.value + let become = 0 + const conditionalOpen: string[] = [] + const blockedOpen: string[] = [] + for (const n of names) { + const exact = localOverrides.value[n] as ToolAnnotation | undefined + const wild = localOverrides.value['*'] as ToolAnnotation | undefined + const up = props.upstreamAnnotations?.[n] as ToolAnnotation | null | undefined + const ow = exact?.openWorldHint ?? wild?.openWorldHint ?? up?.openWorldHint + if (ow === true) { + blockedOpen.push(n) + continue + } + const ro = exact?.readOnlyHint ?? true + if (ro !== true) continue + if (ow === false) become++ + else conditionalOpen.push(n) + } + return { become, conditionalOpen, total: names.length, blockedOpen } +}) + +function openMarkAllModal() { + showMarkAllModal.value = true + markAllAck.value = false + markAllReason.value = '' +} + +function cancelMarkAll() { + showMarkAllModal.value = false + markAllAck.value = false + markAllReason.value = '' +} + +function withMarkSafeSuffix(title: string | undefined, reason: string): string { + const suffix = `[mark-safe: ${reason}]` + const base = (title || '').replace(/\s*\[mark-safe:[^\]]*\]\s*/g, '').trim() + return base ? `${base} ${suffix}` : suffix +} + +function confirmMarkAll() { + if (!markAllAck.value || !markAllReason.value) return + const existing = localOverrides.value['*'] + const ann: ToolAnnotation = { ...(existing || {}) } + ann.readOnlyHint = true + ann.destructiveHint = false + // Reason travels in the draft title so it survives into audit without backend change. + ann.title = withMarkSafeSuffix(existing?.title, markAllReason.value) + localOverrides.value = { ...localOverrides.value, '*': ann } + pendingDeletes.value.delete('*') + addSafeDraft('*') + cancelMarkAll() +} + +function sameAnn(a: ToolAnnotation | null | undefined, b: ToolAnnotation | null | undefined): boolean { + const na = a ?? null + const nb = b ?? null + if (!na && !nb) return true + if (!na || !nb) return false + return na.title === nb.title + && na.readOnlyHint === nb.readOnlyHint + && na.destructiveHint === nb.destructiveHint + && na.idempotentHint === nb.idempotentHint + && na.openWorldHint === nb.openWorldHint +} + +// Unsaved-draft counter near Save-all: local drafts differing from saved +// props plus deletes of saved keys. +const unsavedCount = computed(() => { + let n = 0 + const saved = props.overrides || {} + for (const [k, v] of Object.entries(localOverrides.value)) { + if (!sameAnn(v, saved[k] ?? null)) n++ + } + for (const k of pendingDeletes.value) { + if (saved[k]) n++ + } + return n +}) + function saveAll() { saving.value = true const payload: Record = {} @@ -316,5 +536,19 @@ function saveAll() { setTimeout(() => (saving.value = false), 800) } -defineExpose({ openEdit }) +// Shortcut-button predicate shared with the ServerDetail Tools-tab shortcut: +// hidden exactly when the effective (upstream + wildcard + exact drafts) +// annotation is read-visible. ServerDetail reuses this via the exposed +// method so both buttons compute visibility from the same effective source. +function isMarkSafeVisible(tool: string): boolean { + return effectiveFor(tool)?.readOnlyHint !== true +} + +// Dirty-check for the Bulk-JSON guard in ServerDetail: true while any +// unsaved preset/popover draft exists that Apply-JSON would orphan. +function hasUnsavedDrafts(): boolean { + return unsavedCount.value > 0 +} + +defineExpose({ openEdit, markSafe, effectiveFor, isMarkSafeVisible, confirmMarkAll, cancelMarkAll, hasUnsavedDrafts, unsavedCount }) diff --git a/frontend/src/views/ServerDetail.vue b/frontend/src/views/ServerDetail.vue index f38387a30..064436b2f 100644 --- a/frontend/src/views/ServerDetail.vue +++ b/frontend/src/views/ServerDetail.vue @@ -749,6 +749,14 @@ :disabled="isToolConfigDenied(tool.name)" @click="focusAnnotationOverride(tool.name)" >✎ Override +