feat(webui): per-tool enable/disable + bulk Enable All/Disable All#463
Merged
Dumbris merged 12 commits intoMay 14, 2026
Merged
Conversation
Closes the gap between the per-server enable/disable that already exists and
the per-tool admin toggle UI that was wired only halfway. Three issues
prevented users from actually using the existing storage-backed
ToolApprovalRecord.Disabled mechanism:
1. Runtime.SetToolEnabled returned an error when no approval record existed
for the tool — i.e. for every tool when QuarantineEnabled was false or
SkipQuarantine was true on the server. Now it synthesizes a baseline
"approved" record on demand so the toggle works in every config.
2. The Web UI per-tool button was gated on approval_status === 'approved',
so non-quarantined tools (the common case) never saw it.
3. There was no bulk path. Operators with chatty servers had to click each
tool individually.
What this adds:
- internal/runtime/tool_quarantine.go: SetAllToolsEnabled(serverName,
enabled, updatedBy) (changed int, err error). Walks the StateView/index/
storage union of known tools and toggles only those not already in the
target state — so the audit trail and SSE traffic stay clean on repeated
calls.
- internal/server/server.go: SetAllToolsEnabled controller wrapper.
- internal/httpapi/server.go: POST /api/v1/servers/{id}/tools/enable_all
and .../disable_all. Existing GET /api/v1/servers/{id}/tools now also
surfaces the per-tool disabled flag (contracts.Tool.Disabled) so the
frontend can render the toggle without a second round-trip.
- internal/cliclient/client.go + cmd/mcpproxy/upstream_cmd.go: new
`mcpproxy upstream tools enable|disable|enable-all|disable-all`
subcommands, mirroring the verbs of `mcpproxy upstream
enable|disable --all`.
- frontend/src/views/ServerDetail.vue: bulk "Enable All" / "Disable All"
buttons in the Tools tab header — each visible only when there is
actually something for it to do (avoids labels that promise a no-op).
Per-tool toggle now also renders for tools without an approval record.
- Unit + handler tests for the no-record SetToolEnabled path, the bulk
runtime method, and the two new HTTP handlers.
- oas/swagger.yaml + oas/docs.go regenerated.
7 tasks
golangci-lint staticcheck SA1019 flags strings.Title as deprecated since Go 1.18 — the rule it uses for word boundaries doesn't handle Unicode punctuation. The string here is always one of the ASCII verbs "enable" / "disable", so an inline ASCII-only title-case is fine and avoids adding a golang.org/x/text/cases dependency just to format a debug log.
…ding
Three issues reported on the per-tool enable/disable UI:
1. Per-tool toggle stuck: disabling a tool made the "1 disabled" pill
appear, but enabling it again left the pill stuck. Same root cause
for quarantine staleness users had been seeing intermittently.
2. Server list view occasionally showed outdated tool-quarantine /
disabled-tool counts after navigation.
3. Wording "blocked" was confusing — unclear who's doing the blocking.
Root cause for (1) and (2): the runtime's emitServersChanged embeds the
current server list in the SSE payload (Spec 047), but does so via the
management service's ListServers — which doesn't populate the
Quarantine{Pending,Changed,Blocked}Count fields. Only the REST handler
in httpapi/server.go enriches them. The Web UI's mergeServers treats
the incoming list as authoritative and deletes any absent field
(intentional, to handle "count just dropped to zero"), so on every SSE
delivery after a tool toggle the previously-fetched Quarantine struct
got wiped and the pills disappeared until the user reloaded.
Fixes:
- internal/runtime/event_bus.go: mirror the REST enrichment in
emitServersChanged. New helper enrichServersWithQuarantineStats walks
ToolApprovalBucket the same way the HTTP path does and attaches a
contracts.QuarantineStats with pending/changed/blocked counts. Same
"omit when all-zero" rule as the REST handler so the merge correctly
drops the field on the all-zero transition.
- frontend/src/views/ServerDetail.vue: new syncAfterToolToggle helper
refreshes the store-backed servers list, re-syncs the local server
ref, and reloads tools + approvals. Called from both single-tool and
bulk-toggle paths so the in-page "N disabled" pill loses any staleness
immediately instead of waiting for the next SSE.
- frontend/src/views/ServerDetail.vue + components/ServerCard.vue:
replace user-visible "blocked" with "disabled" so the wording
matches the server-level Enabled/Disabled toggle and doesn't imply
external blocking.
- frontend/src/views/ServerDetail.vue: per-tool Enable/Disable colored
button replaced with a daisyUI `toggle toggle-sm` switch, the same
widget used for the server-level Enabled toggle on the Config tab —
same affordance applied at different scopes.
Three follow-up fixes from user testing of the per-tool enable/disable UI:
1. "1 disabled" pill didn't clear when re-enabling a tool. The fetch +
merge after the toggle was correct, but it raced with the user's
perception — the round-trip is long enough that the pill looks
stuck. Optimistic bump of server.value.quarantine.blocked_count
on click + syncAfterToolToggle() snap-to-truth on response, with
rollback on failure.
2. Toggle widget was visually dimmed when the tool was disabled
because the whole card had `opacity-60`. The dim now applies only
to title row + description + annotations + View Schema button —
the toggle stays in the bright base-content layer and uses
`toggle-primary` so its on-state has a saturated color (so the
off-state can't be misread as a visually-disabled control).
Toggle also moved from the bottom card-actions row to the top-right
header corner, next to the title — same affordance pattern as the
per-server Enabled toggle, applied at tool scope.
3. Toggling the server itself (Disable / Enable from the dropdown)
left the big "Tools" counter stuck at the previous value until
manual reload. The counter reads serverTools.length, which is the
local ref populated by GET /api/v1/servers/{id}/tools — a
server-level toggle never touches it. Added a watch that reacts to
server.connected + server.enabled flips (which the store-level SSE
handler updates) and re-fetches tools on (re)connect, or clears
them when disabled. Same pattern handles the symmetric case:
disabling shows "Disconnected" with 0 tools immediately; enabling
waits for connect then refreshes once tools are discovered.
Systematic fix for the whole "stale local UI" class of bugs that kept surfacing as users exercised the per-tool enable/disable feature. Symptom history: - "1 disabled" pill stuck on re-enable - "1 disabled" pill stuck after Enable All - Big "Tools" counter freezing across server-level Disable / Enable - Tools-tab quarantine badges going stale on navigation Root cause: ServerDetail's `server` was a plain `ref<Server | null>(null)` populated by a manual snapshot of `serversStore.servers.find(...)`. The store gets updated correctly on every SSE delivery and every explicit fetch, but the local ref is its own copy of the data. Every action handler had to remember to reassign `server.value = ...find(...)` after its async work, and any handler that forgot it (or that the SSE beat to the punch) left the user looking at stale fields. Refactor: `server` is now a `computed<Server | null>` that derives from `serversStore.servers` keyed by `props.serverName`. Property accesses flow through Pinia's reactive proxy, so every store mutation — SSE embed (spec 047), notify-only fallback + re-fetch, direct mutation — automatically propagates to template bindings and dependent computeds. All ~10 `server.value = serversStore.servers.find(...)` reassignment sites are deleted (the computed re-derives automatically). Optimistic updates that previously wrote to `server.value.X` now go through `mutateStoreServer(fn)`, which mutates the store object directly — so the optimistic change is visible to the Server List view and the tray too, not just this page. bulkToggleAllTools picks up the same optimistic-update path as the single-tool handler: blocked_count is driven to 0 (Enable All) or to the count of togglable tools (Disable All) on click, before the round-trip — the "N disabled" pill now animates in real time on bulk actions, not after a 200–500ms gap.
electrolobzik
commented
May 13, 2026
Contributor
Author
electrolobzik
left a comment
electrolobzik
left a comment
There was a problem hiding this comment.
Code review
Found 2 issues (focused on regressions + security per the request).
-
SSE
servers.changedembed only enrichesQuarantine, notSecurityScan— every SSE delivery wipessecurity_scanfrom the Web UI store. The new enrichment atinternal/runtime/event_bus.gomirrors the REST handler'senrichServersWithQuarantineStats, but the REST handler also enriches each server withSecurityScan(Spec 039). The Web UI'smergeServerswas specifically designed (issue #438 / PR #443) to treat incoming as authoritative and delete absent keys — the explicit warning comment calls out exactly this pattern. Result: everyservers.changedevent (enable/disable, restart, quarantine change, tool toggle — many per session) silently stripssecurity_scanfrom each store server, blanking the risk-score badge / scan summary until the next manual REST refresh. Same bug class the PR itself fixes forQuarantine. Fix: also enrichSecurityScaninemitServersChanged, or drop the embed for fields the SSE path can't mirror. -
Bulk
SetAllToolsEnabledemitsservers.changedper tool, each emit re-scans every server's BBolt approvals — partially reverses Spec 047's CPU win.SetAllToolsEnabledcallsSetToolEnabledin a loop, andSetToolEnabledemitsservers.changedper invocation. Each emit runs the newenrichServersWithQuarantineStatswhich does oneListToolApprovalsper server. For K=50 tools × N=30 servers a single Disable-All produces ~1500 BBoltViewcalls. The Spec 047 coalescer collapses the publish but not the per-submit work — exactly the pattern eae45ef (#450, "cut idle CPU 92%") and #444 ("emit once per bulk call", seeApproveAllTools's single trailing emit) were designed against. Fix: emitservers.changedonce at the end of the bulk loop (suppress per-tool emits when invoked from the bulk path), or cache approval counts inStateViewand read from memory instead of BBolt.
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
Spec 047 §B2 coalesced servers.changed publishes to ≤1 per 50ms window, last-write-wins. The build, however, still ran eagerly inside emitServersChanged before the coalescer saw the event: a ListServers call plus an N-row BBolt scan per server (enrichServersWithQuarantineStats). Bulk operations that fired K rapid emits paid K×(1+N) BBolt ops even though the coalescer dropped K-1 publishes — Enable-All on a 30-server, 50-tool setup was ≈1,500 wasted BBolt reads per click. Refactor: the coalescer now holds a lightweight (reason, extra) marker instead of a fully-realised Event. The drainer materialises the full SSE payload at flush time via buildServersChangedPayload. Every emit is essentially free; the build runs once per publish window regardless of how many submits land. Extends Spec 047's amortisation promise from publish to build. The no-coalescer fallback (some tests, shutdown) still builds inline so prior semantics are preserved. Regression test: TestCoalescer_AmortisesBuildAcrossBurst asserts that 100 rapid emits → exactly 1 ListServers call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related fixes to the per-tool toggle path: 1. Extract setToolEnabledNoEmit(...) (changed bool, err error). The bulk path SetAllToolsEnabled now calls this directly in its loop and emits exactly one trailing servers.changed event (reason: tools_enabled / tools_disabled), mirroring the ApproveAllTools pattern in the same file. Before, the loop delegated to SetToolEnabled per item, so K tools fired K SSE emits — each kicking off a full server-list build (now amortised by the lazy-build drainer in the previous commit, but still N BBolt writes worth of churn that this commit eliminates). The split also lifts the "already in desired state" pre-check into the noEmit core, so single-tool toggles no longer pay a no-op BBolt write + SSE emit when the user re-clicks the current state. 2. Synthesis-on-not-found now requires errors.Is(err, storage.ErrToolApprovalNotFound), not a generic err != nil check. The GetToolApproval helper returns the new sentinel (wrapped) when the record is missing; any other error (decode failure, closed DB, mmap remap during compaction) propagates to the caller. Without this gate a transient I/O error could silently demote a pending/changed record to approved — the exact rug-pull bypass Spec 032 was designed to prevent. Tests: - TestSetToolEnabled_PreservesExistingPendingStatus — pending record's Status stays pending across a Disable click; only Disabled flips. - TestSetAllToolsEnabled_EmitsOncePerBulk — N tools → exactly one servers.changed with reason=tools_disabled. - TestSetAllToolsEnabled_NoEventOnNoOp — no event when every tool is already in the desired state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After the lazy-build refactor (10527bd), serversChangedCoalescer.flushNow was calling buildServersChangedPayload synchronously, which in turn issued ListServers with a context.WithTimeout(context.Background(), 2s). The 2-second timeout was rooted in Background — independent of the runtime's appCtx — so when Runtime.Close() fired appCancel the drainer goroutine could sit on a detached timer for up to 2 seconds before exiting. Runtime.Close itself doesn't wait on the drainer, so the practical impact is a leaked goroutine doing wasted work after shutdown begins, not user-visible latency; but the regression silently broke the original Spec 047 promise that flushNow was O(1). Thread the parent ctx: - serversChangedCoalescer gains a parentCtx field, set by start(). - flushNow uses c.parentCtx (or Background if not started) as the parent for buildServersChangedPayload's WithTimeout. - buildServersChangedPayload takes ctx as an explicit parameter; the no-coalescer fallback in emitServersChanged passes r.appCtx. Regression test: TestBuildServersChangedPayload_HonoursCancelledParentCtx constructs a ctx-aware lister that blocks until ctx fires, then calls buildServersChangedPayload with an already-cancelled ctx and asserts the call returns in < 200 ms (down from the 2 s a Background-rooted timeout would have taken). Asserts the notify-only fallback fires (no servers key when ListServers errors). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… share enrichment The servers.changed SSE embed (added in Spec 047 §B1, extended by PR smart-mcp-proxy#463 to also carry Quarantine stats) was missing SecurityScan. REST's GET /api/v1/servers enriched both Quarantine and SecurityScan via inline calls in internal/httpapi/server.go; the SSE path (runtime → buildServersChangedPayload → enrichServersWithQuarantineStats) only mirrored the Quarantine half. mergeServers on the Web UI treats incoming server data as authoritative and deletes absent fields (intentional — it's how a count dropping to zero clears its badge). So every tool toggle → SSE delivery silently stripped security_scan from every server in the Pinia store. Users with security scans configured saw risk-score badges go blank after any toggle, only returning on the next manual REST refresh. Same bug class as the pre-existing quarantine-stats staleness PR smart-mcp-proxy#463 fixes for Quarantine — and now closed structurally so the next per-server field can't reintroduce it. Approach: plumb scan summaries through management.ListServers — the single ListServers call is the meeting point of both REST (which calls it from the handler) and SSE (runtime.buildServersChangedPayload calls it too). Avoids a parallel parity-mirror pattern that the next field would have to re-implement in both places. - internal/management/service.go: new SecurityScanEnricher interface and SetScanSummaryEnricher setter on the Service interface. ListServers calls the enricher per server before returning. - internal/server/server.go: scanSummaryEnricherAdapter bridges scanner.Service.GetScanSummary (scanner-internal type) to management.SecurityScanEnricher (contracts.SecurityScanSummary wire shape). Wired alongside SetSecurityController. - internal/httpapi/server.go: drop the inline securityController loop that enriched the REST response — ListServers now does it. Tests: - internal/management/service_test.go: subtest "populates SecurityScan via enricher" verifies ListServers calls the enricher per server and preserves nil for servers the enricher doesn't know about; subtest "nil enricher is a no-op" pins the optional-wiring path. - internal/runtime/event_bus_payload_test.go: TestEmitServersChanged_PayloadPreservesSecurityScan asserts the SSE embed carries SecurityScan through to subscribers unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Detail.vue) Vue Router 4 reuses the ServerDetail.vue component instance across /servers/foo → /servers/bar — same route, different param. The `server` computed correctly retargets via the Pinia store, but the local data refs (serverTools, toolApprovals, serverLogs, scan*) stayed populated with the previous server's data until something refetched them. Navigating between server detail pages briefly showed server B's name + stats combined with server A's tool list, which looked like a data-corruption bug. Adds a `watch(() => props.serverName)` that resets every per-server local ref and re-runs loadServerDetails. The reset enumerates exactly the refs that carry per-server data and leaves UI-state refs (activeTab, logTail, toolSearch) alone so the user's tab/scope choice persists across navigation. Race protection: introduces a `loadGeneration` counter bumped on every loadServerDetails entry. The three parallel fetches now run via internal `_loadToolsWithGen / _loadToolApprovalsWithGen / _loadLogsWithGen` helpers that capture the generation at entry and only commit results if it hasn't advanced — protects the foo→bar→foo case where foo's response arrives AFTER bar's load already started. The public no-arg `loadTools / loadToolApprovals / loadLogs` wrappers preserve existing call sites (template @click handlers, the connected/enabled watch, post-action refreshes) unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rors
The new isToolCallable check returned true (callable) for any non-nil
GetToolApproval error other than the new ErrToolApprovalNotFound
sentinel. A transient BBolt mmap-remap during compaction, a decode
failure, or any other read error would silently re-enable a tool the
operator had previously persisted as Disabled=true — the inverse of
the Spec 032 rug-pull bypass that setToolEnabledNoEmit already guards
against on the write side.
Mirror that pattern on the read side:
switch {
case err == nil:
if approval != nil && approval.Disabled { return false }
case errors.Is(err, storage.ErrToolApprovalNotFound):
// no record → default callable (matches prior fallthrough)
default:
// real storage error → fail closed and log
return false
}
The "no record → callable" branch is preserved so the synthesis path
for never-toggled tools still works (writes happen lazily on first
toggle; reads before any toggle should still expose the tool by
default). A WARN log fires on the fail-closed path so operators can
correlate storage health with sudden tool unavailability.
Regression test: TestIsToolCallable_FailsClosedOnStorageError writes
corrupt non-JSON bytes directly into the tool_approvals bucket via the
storage manager's GetDB hook, then asserts isToolCallable returns
false. Exercises the real json.Unmarshal error path, not a mocked
storage layer.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The MCP-layer quarantine_security tool gates enable_tool / disable_tool
behind authCtx.IsAdmin() (mcp.go:2326). The three new REST endpoints
that landed in this PR did NOT mirror that check:
POST /api/v1/servers/{id}/tools/{tool}/enabled
POST /api/v1/servers/{id}/tools/enable_all
POST /api/v1/servers/{id}/tools/disable_all
Result: an agent token (mcp_agt_ prefix) — scoped to a subset of
servers and read-only permissions via the agent-token system — could
hit any of these REST endpoints and flip per-tool visibility on any
server, bypassing the admin gate the MCP layer enforces.
Add the guard at the top of handleSetToolEnabled and the
handleSetAllToolsEnabled closure (covers both enable_all and
disable_all routes). Non-admin or unauthenticated callers receive 403
"operation requires admin access".
Scope limited to the new endpoints. The pre-existing per-server REST
mutation endpoints (e.g. POST /api/v1/servers/{name}/enable,
.../quarantine, .../tools/approve) also lack admin checks — that's a
broader hardening pattern worth a separate PR rather than entangling
it with the per-tool feature.
Regression tests in tool_quarantine_test.go:
- TestHandleSetToolEnabled_AgentTokenForbidden
- TestHandleSetAllToolsEnabled_EnableAll_AgentTokenForbidden
- TestHandleSetAllToolsEnabled_DisableAll_AgentTokenForbidden
- TestHandleSetToolEnabled_AdminKeyAllowed (positive control)
Helpers: mockToolToggleController wraps the existing mock with a real
*config.Config so the auth middleware activates; agentTokenServer
mints a live agent token via the token store.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
electrolobzik
added a commit
to HaloCollar/mcpproxy-go
that referenced
this pull request
May 14, 2026
Fast-forward-compatible merge that brings halo-main up to the PR branch tip. Cherry-picks already on halo-main (f7694c8, f548de1, c3e99ac, ba2dcad) are byte-identical to their PR-branch originals (6032ed8, d584b16, b5f0a0c, 7bc7c73), so the 3-way merge resolves cleanly and the resulting tree matches the PR tip. Brings in: - Spec 047 (CPU hotpath fix + coalescer + SSE payload embed) - Spec 048 (tray refetch elimination) - Spec 046 (local launcher for HTTP/SSE upstreams + onboarding wizard v2) - Spec 044 phase H diagnostics counters - Spec 042 telemetry tier 2 finalization - The 3 PR-smart-mcp-proxy#463 commits that depend on the Spec 047 coalescer: - perf(runtime): build servers.changed payload lazily in coalescer drainer - fix(runtime): honour app-shutdown cancellation in coalescer drainer - fix(sse): plumb SecurityScan via management.ListServers - All other upstream-merged fixes between 2b9b5f9 and 7bc7c73 Authorized force-push equivalent: the user explicitly requested halo-main be set to the PR branch tip; force-push was blocked by the git MCP and the sandbox lacks raw-git auth, so we take the merge-commit path which achieves tree-equality without rewriting halo-main history. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> # Conflicts: # .gitignore # oas/docs.go
Dumbris
approved these changes
May 14, 2026
Dumbris
pushed a commit
that referenced
this pull request
May 14, 2026
User feedback on the previous round: REST PATCH had two ways to delete
a key (`headers_remove: ["X"]` array vs MCP tool's `{"X": null}`). The
MCP tool's syntax is cleaner and is a documented standard (RFC 7396).
Unify on it and add a CLI command so all three interfaces — MCP, REST,
CLI — behave the same way.
Backend (internal/httpapi/server.go):
- AddServerRequest.Headers and .Env switch from `map[string]string` to
`map[string]*string`. Go's encoding/json then maps a missing key to
"no entry", a present non-null value to a non-nil `*string`, and a
present `null` to a nil pointer. The merge loop reads each entry: nil
pointer = delete, non-nil = upsert.
- Drop the `headers_remove` / `env_remove` array fields. A single `null`
in the same map carries the same intent and aligns with the MCP tool.
- POST (add) ignores nil entries via the new flattenNullableMap helper;
`null` on create has no meaning.
- redactServerHeaders / SSE redaction unchanged.
Tests (internal/httpapi/patch_server_test.go):
- Rewrite the previous `*_remove`-style tests to use literal JSON null
payloads via `json.RawMessage`. The raw-byte approach is independent
of any Go marshaling quirks that could collapse `null` values.
- New TestHandlePatchServer_HeadersEmptyStringSetsNotDeletes pins the
distinction between `""` (set to empty) and `null` (delete) — JSON
Merge Patch is explicit about it and a future refactor that
"helpfully" collapses one to the other would silently break.
- Total: 7 tests, all green.
Web UI (frontend/src/views/ServerDetail.vue):
- deleteKv now sends `{headers: {key: null}}` instead of the array
form. JSON.stringify emits `null` literally, no special handling.
- Drop the `scopeRemoveKey` helper (no longer needed).
macOS Swift (native/macos/MCPProxy/MCPProxy/Views/ServerDetailView.swift):
- diffKVMap returns a single `[String: Any]` patch dict where deleted
keys map to `NSNull()` instead of returning the previous
`(set, remove)` tuple.
- saveEdits writes the patch as `updates["headers"] = patch` directly;
no `headers_remove` companion field anymore.
- performConvertToSecret sends a single-key patch
`{field: {key: ref}}` instead of building the full map — minimal
wire payload, never round-trips the redacted Authorization.
- The trap was real and surprising: Swift's default `JSONEncoder` on
`[String: String?]` SILENTLY DROPS nil entries from the JSON output.
Using `[String: Any]` with `NSNull()` + `JSONSerialization` (the
encoder our APIClient already uses) renders `null` correctly.
Swift unit test (native/macos/MCPProxy/MCPProxyTests/MergePatchEncodingTests.swift):
- 4 tests pinning the encoding contract:
1. NSNull encodes as literal `null` via JSONSerialization.
2. A delete-only patch round-trips through JSON and the value
parses back as NSNull (not "", not absent).
3. The wrong path — `[String: String?]` + default `JSONEncoder` —
does silently drop nils. Documented as a poison-pill test so a
future refactor that "simplifies" to it has to explicitly delete
this test and read the comment first.
4. Empty string still encodes as `""` and explicit null as `null`
— the JSON Merge Patch set-vs-delete distinction is preserved.
CLI (cmd/mcpproxy/upstream_cmd.go + internal/cliclient/client.go):
- New `mcpproxy upstream patch <name>` subcommand with flags:
--header K=V upsert (repeatable)
--header-remove K delete (repeatable)
--env K=V upsert (repeatable)
--env-remove K delete (repeatable)
- New cliclient.PatchServer(name, body) sends raw JSON to PATCH
/api/v1/servers/{name}. Body shape is the same JSON Merge Patch the
Web UI and macOS tray send.
- Closes the CLI gap I called out in the boundaries-matrix summary —
REST, MCP, and CLI now all support both write and delete on headers
/ env with the same semantics.
Live end-to-end verification:
$ mcpproxy upstream patch synapbus --header "X-Cli-Test: hello-from-cli"
✅ Patched synapbus: 1 header(s) set
→ on disk: { Authorization (real Bearer), X-Cli-Test } (Auth preserved)
$ mcpproxy upstream patch synapbus --header-remove X-Cli-Test
✅ Patched synapbus: 1 header(s) removed
→ on disk: { Authorization (real Bearer) } (Auth preserved)
$ mcpproxy upstream patch synapbus --header "X-Foo: v" --header-remove X-Foo
Error: --header and --header-remove for "X-Foo" conflict; pick one
Web UI: "+ Add header" → X-WebUI-Test=from-browser → Save
→ on disk: { Authorization (real Bearer), X-WebUI-Test }
macOS tray: Edit → textarea pre-populated with
"Authorization=***REDACTED***" → user appends "X-Mac-Test=hello-from-mac"
→ Save
→ on disk: { Authorization (REAL Bearer, preserved!), X-Mac-Test }
PR #463 subagent review confirmed the unrelated "disable tool" pattern
is a different domain (reversible state in BBolt vs destructive
mutation in mcp_config.json) and should not be unified with this work.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
|
Thank you for this — really well-scoped and thoroughly tested work. Shipping per-tool enable/disable plus the bulk Enable All / Disable All actions closes a gap that's been bugging operators with chatty servers for a while, and the SSE quarantine-stats parity fix was a nice catch along the way. Merging now. Thanks again, Roman. |
Dumbris
pushed a commit
that referenced
this pull request
May 14, 2026
User feedback on the previous round: REST PATCH had two ways to delete
a key (`headers_remove: ["X"]` array vs MCP tool's `{"X": null}`). The
MCP tool's syntax is cleaner and is a documented standard (RFC 7396).
Unify on it and add a CLI command so all three interfaces — MCP, REST,
CLI — behave the same way.
Backend (internal/httpapi/server.go):
- AddServerRequest.Headers and .Env switch from `map[string]string` to
`map[string]*string`. Go's encoding/json then maps a missing key to
"no entry", a present non-null value to a non-nil `*string`, and a
present `null` to a nil pointer. The merge loop reads each entry: nil
pointer = delete, non-nil = upsert.
- Drop the `headers_remove` / `env_remove` array fields. A single `null`
in the same map carries the same intent and aligns with the MCP tool.
- POST (add) ignores nil entries via the new flattenNullableMap helper;
`null` on create has no meaning.
- redactServerHeaders / SSE redaction unchanged.
Tests (internal/httpapi/patch_server_test.go):
- Rewrite the previous `*_remove`-style tests to use literal JSON null
payloads via `json.RawMessage`. The raw-byte approach is independent
of any Go marshaling quirks that could collapse `null` values.
- New TestHandlePatchServer_HeadersEmptyStringSetsNotDeletes pins the
distinction between `""` (set to empty) and `null` (delete) — JSON
Merge Patch is explicit about it and a future refactor that
"helpfully" collapses one to the other would silently break.
- Total: 7 tests, all green.
Web UI (frontend/src/views/ServerDetail.vue):
- deleteKv now sends `{headers: {key: null}}` instead of the array
form. JSON.stringify emits `null` literally, no special handling.
- Drop the `scopeRemoveKey` helper (no longer needed).
macOS Swift (native/macos/MCPProxy/MCPProxy/Views/ServerDetailView.swift):
- diffKVMap returns a single `[String: Any]` patch dict where deleted
keys map to `NSNull()` instead of returning the previous
`(set, remove)` tuple.
- saveEdits writes the patch as `updates["headers"] = patch` directly;
no `headers_remove` companion field anymore.
- performConvertToSecret sends a single-key patch
`{field: {key: ref}}` instead of building the full map — minimal
wire payload, never round-trips the redacted Authorization.
- The trap was real and surprising: Swift's default `JSONEncoder` on
`[String: String?]` SILENTLY DROPS nil entries from the JSON output.
Using `[String: Any]` with `NSNull()` + `JSONSerialization` (the
encoder our APIClient already uses) renders `null` correctly.
Swift unit test (native/macos/MCPProxy/MCPProxyTests/MergePatchEncodingTests.swift):
- 4 tests pinning the encoding contract:
1. NSNull encodes as literal `null` via JSONSerialization.
2. A delete-only patch round-trips through JSON and the value
parses back as NSNull (not "", not absent).
3. The wrong path — `[String: String?]` + default `JSONEncoder` —
does silently drop nils. Documented as a poison-pill test so a
future refactor that "simplifies" to it has to explicitly delete
this test and read the comment first.
4. Empty string still encodes as `""` and explicit null as `null`
— the JSON Merge Patch set-vs-delete distinction is preserved.
CLI (cmd/mcpproxy/upstream_cmd.go + internal/cliclient/client.go):
- New `mcpproxy upstream patch <name>` subcommand with flags:
--header K=V upsert (repeatable)
--header-remove K delete (repeatable)
--env K=V upsert (repeatable)
--env-remove K delete (repeatable)
- New cliclient.PatchServer(name, body) sends raw JSON to PATCH
/api/v1/servers/{name}. Body shape is the same JSON Merge Patch the
Web UI and macOS tray send.
- Closes the CLI gap I called out in the boundaries-matrix summary —
REST, MCP, and CLI now all support both write and delete on headers
/ env with the same semantics.
Live end-to-end verification:
$ mcpproxy upstream patch synapbus --header "X-Cli-Test: hello-from-cli"
✅ Patched synapbus: 1 header(s) set
→ on disk: { Authorization (real Bearer), X-Cli-Test } (Auth preserved)
$ mcpproxy upstream patch synapbus --header-remove X-Cli-Test
✅ Patched synapbus: 1 header(s) removed
→ on disk: { Authorization (real Bearer) } (Auth preserved)
$ mcpproxy upstream patch synapbus --header "X-Foo: v" --header-remove X-Foo
Error: --header and --header-remove for "X-Foo" conflict; pick one
Web UI: "+ Add header" → X-WebUI-Test=from-browser → Save
→ on disk: { Authorization (real Bearer), X-WebUI-Test }
macOS tray: Edit → textarea pre-populated with
"Authorization=***REDACTED***" → user appends "X-Mac-Test=hello-from-mac"
→ Save
→ on disk: { Authorization (REAL Bearer, preserved!), X-Mac-Test }
PR #463 subagent review confirmed the unrelated "disable tool" pattern
is a different domain (reversible state in BBolt vs destructive
mutation in mcp_config.json) and should not be unified with this work.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
May 14, 2026
…t-to-secret (#466) * feat(server-detail): expose and edit headers + env in Web UI Headers and env were dropped at the runtime -> management -> contract boundary, so neither the Web UI nor the macOS tray could render or round-trip them. A server configured with a static Authorization header (e.g. synapbus with a Bearer token) appeared with no headers section at all on the Web UI Config tab. Backend wiring (this commit, all surfaces): - internal/runtime/runtime.go: GetAllServers emits headers and env from serverStatus.Config in serverMap. - internal/management/service.go: ListServers extracts headers and env in both typed (map[string]string) and generic (map[string]interface{}) shapes. Existing redaction at the HTTP layer continues to apply. Web UI (Config tab): - New Headers card with redact-by-default + click-to-reveal, per-row inline edit/delete, an Add row, and a "Convert to secret" button that stores the literal value in the OS keyring and rewrites the field as `${keyring:<name>}`. - Same affordances applied to the Environment Variables card. - New reusable KVValueCell component encapsulates the per-row UX so Headers and Env share the same display/edit/reveal/convert logic. - api.ts: new patchServer() and storeSecret() helpers wrapping PATCH /api/v1/servers/{id} and POST /api/v1/secrets respectively. - types/api.ts: add `headers` to Server interface (the contracts.ts twin already had it; api.ts had drifted). Note on reveal: backend redaction replaces sensitive header values with `***REDACTED***` unless `reveal_secret_headers: true` is set in config. The KVValueCell detects that sentinel and disables both reveal and the "Convert to secret" button (since neither has the real value in hand), surfacing a tooltip that points the user at the config flag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(server-detail): render \${keyring:...} preview literally in convert modal Vue templates do NOT interpret \${...} as JS template-literal syntax — the dollar-and-braces inside the modal body were rendering verbatim as '${'{'}'... text. Replace the awkward \${'{'} escape with a plain string interpolation through {{ '...' }} mustache so the user sees the actual reference syntax they're about to substitute into config. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(macos): display + edit headers and env on server detail Adds full round-trip support for headers and env on the macOS tray's server detail screen, matching the new Web UI experience. API model: - ServerStatus gains `headers` and `env` (both [String: String]?). CodingKeys updated to map the JSON `headers` and `env` fields the Go backend now emits (companion to the runtime + management wiring in the parent commit). View: - Headers section under Connection for HTTP / streamable-http servers, visible in both view mode (sorted key list with masked values) and edit mode (KEY=VALUE textarea, parallel to the existing env textarea). - editEnvVars now pre-populates from `server.env` instead of starting empty — fixes the long-standing stub at L939 that explicitly noted the missing config API. - editHeaders works the same way for headers. - saveEdits() sends both maps unconditionally so deletes round-trip; refuses to save if any header value is still `***REDACTED***` (the backend sentinel emitted when `reveal_secret_headers: false`) so we don't silently overwrite a real secret with the placeholder. - New helpers: parseKVTextarea (shared between env and headers) and maskedHeaderValue (recognises `${keyring:...}` and `${env:...}` references and renders them as-is, masks literal values, surfaces the redaction sentinel verbatim so users know to flip reveal_secret_headers in their config). Convert-to-secret in Swift: deferred. The Web UI surfaces this per row through KVValueCell; the equivalent SwiftUI experience would need a new modal + state machine that doesn't fit the existing textarea-based edit form. Tracked as a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(macos): Convert-to-secret + sidebar cap + redacted-save guard hardening User feedback on the previous macOS commit triggered three follow-up fixes that all land naturally together: Sidebar cap (MainWindow.swift): - NavigationSplitView only set `min: 180, ideal: 220` for the sidebar column. SwiftUI was free to expand it unbounded after some layout transitions, squeezing the detail pane to a sliver on the right. - Add `max: 280` so the sidebar stays at a sensible width while the detail pane gets the rest. Verified visually on the server detail Config tab. Redacted-save guard (ServerDetailView.swift::saveEdits): - Previous version only refused to save when the textarea still contained `***REDACTED***` literally. A user could still delete the redacted line, add a new header, and silently wipe out the real Authorization behind the redaction. - Now we diff the new headers map against the original `server.headers` and refuse the save when either (a) a `***REDACTED***` literal is still present OR (b) any key whose original value was redacted is missing entirely from the new map. The error message lists the offending keys and points the user at `reveal_secret_headers: true`. Convert-to-secret on macOS (the previously-deferred work): - New SwiftUI sheet binding through `@State convertSheet: ConvertToSecretContext?`. The sheet body mirrors the Web UI flow — pre-suggests a secret name (`<server-name>-<key>`, lowercased and hyphen-sanitised), shows a live `${keyring:NAME}` preview, has Cancel + Convert with proper keyboard shortcuts. - Headers view-mode rendering switched from the static `configRow(label:value:)` to a new `kvRow(scope:key:value:)` that renders `${keyring:…}` / `${env:…}` references as a capsule chip with no actions, surfaces the `***REDACTED***` sentinel verbatim (still no convert button — we don't have the real value), and shows a 🔒 button for genuine literal values that opens the convert sheet. - Environment Variables now also renders in view mode (previously it was edit-only) with the same kvRow affordances. Visible whenever the server has any env vars, regardless of protocol — stdio servers can finally inspect their pre-populated env without entering edit mode. - APIClient gains `storeSecret(name:value:)` wrapping POST /api/v1/secrets which returns the `${keyring:<name>}` reference string to substitute back into the server config via the existing `updateServer` PATCH. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(headers): stop redacting REST + SSE responses; scope reveal_secret_headers to MCP tool only User feedback on the previous round: the Server Detail edit form refused to save when any header value still contained the `***REDACTED***` sentinel, and pushed users at the `reveal_secret_headers: true` config flag to unblock themselves. That trade-off was wrong — it punished the human UI to protect an agent code path. The actual threat model: - REST API (`/api/v1/servers`, SSE `/events`): gated by the local per-install API key. Same trust boundary as access to `~/.mcpproxy/mcp_config.json` on disk where these values are already stored in plaintext. Redacting in the response bought no real security, only broke the Web UI + macOS tray edit-and-save round-trip. - MCP `upstream_servers` tool: invoked by AI agents, output gets read back to the LLM context. THIS is the agent-context exposure `reveal_secret_headers` was created to protect — and that redaction was already implemented separately in `internal/server/mcp.go:~2545`, unaffected by this change. Backend changes: - internal/httpapi/server.go: drop `redactServerHeaders` calls in `handleGetServers` (both code paths) and remove the now-unused method. - internal/runtime/event_bus.go: drop the `redactServerHeaders` call on SSE `servers.changed` payloads and remove the now-unused method. - internal/runtime/event_bus_payload_test.go: rewrite the redaction test to assert the new policy — SSE must now carry plaintext, the test name changes from `…_RedactsSensitiveHeaders` to `…_SendsPlaintextHeaders`. - internal/config/config.go: update the `RevealSecretHeaders` doc to explicitly scope the flag to the MCP tool. REST + SSE always send plaintext from now on. UI cleanup (the redaction sentinel is no longer expected on the wire): - macOS Swift `saveEdits()`: drop the elaborate redacted-save guard (refused save when ***REDACTED*** literal still in textarea OR a redacted key was deleted). Both cases became impossible once the REST API serves plaintext. - macOS Swift `kvRow()`: drop the `value != "***REDACTED***"` check that disabled the Convert-to-secret button. - macOS Swift `performConvertToSecret()`: drop the matching guard. - Vue `KVValueCell.vue`: drop the `isBackendRedacted` computed flag and the "Backend redacted this value, set reveal_secret_headers" tooltip. Reveal and Convert are always available on literal values. - Vue `ServerDetail.vue::commitConvert()`: drop the same guard. Verified end-to-end on macOS and Web UI: - REST: `curl /api/v1/servers` → `Authorization: Bearer 1d386f...` (the real 71-char token). - MCP tool: `upstream_servers list` → `"Authorization":"***REDACTED***"` (still hidden from agents). - macOS Server Detail → synapbus → Edit → Headers textarea pre-populated with the real Bearer token; Save no longer blocked. - Web UI synapbus Config → Headers row shows `••••59 (71 chars)` with reveal eye, Convert-to-secret 🔒, edit, delete — all functional. OAS spec regenerated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(headers): restore REST/SSE redaction and add deep-merge PATCH User feedback on the previous commit: dropping REST/SSE redaction unblocked the UI but narrowed the threat model from PR #425 (a local process that holds the API key but not filesystem access could read other upstreams' Bearer tokens off the wire). The better trade-off is to keep both halves of PR #425 intact AND let the UI edit without round-tripping the redacted sentinel. Solution: deep-merge PATCH semantics so the client sends only the keys that changed. Redacted-but-untouched values stay out of the patch entirely, the backend keeps the real string on disk. Backend: - internal/httpapi/server.go::handlePatchServer — when the request body contains `headers` or `env`, MERGE into the existing stored map instead of replacing. New `headers_remove` / `env_remove` fields carry explicit deletes. Sending both is allowed (deletes apply after upserts). - internal/httpapi/server.go::AddServerRequest — adds the two `*_remove` fields with a comment documenting the new semantics. The add path ignores them. - internal/httpapi/server.go::handleGetServers — re-enable `redactServerHeaders` on both code paths. - internal/runtime/event_bus.go::emitServersChanged — re-enable redaction on SSE `servers.changed` payloads. SSE rides the same trust boundary as the REST GET. - internal/config/config.go::RevealSecretHeaders — restore the original PR #425 doc (REST + MCP both gated) with a new paragraph pointing at the deep-merge mechanism that makes the UI work anyway. Tests: - internal/httpapi/patch_server_test.go — 4 new tests pinning the merge semantics: - TestHandlePatchServer_HeadersDeepMerge — `headers: {X-New: v}` against existing `{Authorization, X-Trace}` preserves both original keys and adds X-New. - TestHandlePatchServer_HeadersRemove — `headers_remove: [...]` deletes the listed keys. - TestHandlePatchServer_HeadersSetAndRemove — both fields in one PATCH; deletes apply after upserts. - TestHandlePatchServer_EnvDeepMerge — same pattern for env. - internal/runtime/event_bus_payload_test.go — restored the original PR #425 assertion (`...RedactsSensitiveHeaders`). Frontend (Web UI): - ServerDetail.vue — `patchServerDiff(patch, action)` replaces the old `patchKVMap`. Each per-row UI action now sends a minimal targeted patch: - Edit one row: `{headers: {key: newValue}}` - Delete one row: `{headers_remove: [key]}` - Add one row: `{headers: {newKey: newValue}}` - Convert to secret: `{headers: {key: "${keyring:NAME}"}}` The redacted sentinel for unchanged keys never round-trips, by construction. - KVValueCell.vue — restore the `isBackendRedacted` branch. When the cell renders `***REDACTED***`, the reveal / Convert-to-secret buttons disappear (we don't hold the real value) and the cell shows the sentinel verbatim with a tooltip explaining that editing still works through the inline edit button. macOS Swift: - ServerDetailView.swift::saveEdits — switch from "send the full parsed map" to "diff against `server.headers` / `server.env` and send the diff". New private helper `diffKVMap(original:next:)` returns a `(set, remove)` tuple suitable for the deep-merge PATCH body. The same invariant holds: leaving a redacted line untouched in the textarea produces `next[k] == "***REDACTED***" == original[k]`, so the key stays out of both sides of the diff and the backend preserves the real value. - ServerDetailView.swift::kvRow — restore the `value != "***REDACTED***"` gate on the Convert-to-secret button (the sentinel isn't useful as a keyring payload). - ServerDetailView.swift::editHeaders doc — explain the new flow. End-to-end verification against the live local instance: $ curl -s -H "X-API-Key: ..." /api/v1/servers | jq '...synapbus.headers' → {"Authorization": "***REDACTED***"} # redacted ✓ $ jq '...synapbus.headers' ~/.mcpproxy/mcp_config.json → {"Authorization": "Bearer 1d386..."} # real on disk $ curl -X PATCH .../servers/synapbus -d '{"headers":{"X-Trace-Test":"merge-works"}}' $ jq '...synapbus.headers' ~/.mcpproxy/mcp_config.json → {"Authorization": "Bearer 1d386...", "X-Trace-Test": "merge-works"} # real token preserved ✓ $ curl -X PATCH .../servers/synapbus -d '{"headers_remove":["X-Trace-Test"]}' $ jq '...synapbus.headers' ~/.mcpproxy/mcp_config.json → {"Authorization": "Bearer 1d386..."} # X-Trace-Test deleted ✓ PR #425's E2E tests still pass (TestE2E_PatchDeepMergesEnvAndHeaders, TestE2E_MultipleEnableDisablePreservesConfig — both exercise the MCP tool which still redacts). PR #425's intent is fully preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(headers): unify delete syntax to JSON Merge Patch + add CLI patch User feedback on the previous round: REST PATCH had two ways to delete a key (`headers_remove: ["X"]` array vs MCP tool's `{"X": null}`). The MCP tool's syntax is cleaner and is a documented standard (RFC 7396). Unify on it and add a CLI command so all three interfaces — MCP, REST, CLI — behave the same way. Backend (internal/httpapi/server.go): - AddServerRequest.Headers and .Env switch from `map[string]string` to `map[string]*string`. Go's encoding/json then maps a missing key to "no entry", a present non-null value to a non-nil `*string`, and a present `null` to a nil pointer. The merge loop reads each entry: nil pointer = delete, non-nil = upsert. - Drop the `headers_remove` / `env_remove` array fields. A single `null` in the same map carries the same intent and aligns with the MCP tool. - POST (add) ignores nil entries via the new flattenNullableMap helper; `null` on create has no meaning. - redactServerHeaders / SSE redaction unchanged. Tests (internal/httpapi/patch_server_test.go): - Rewrite the previous `*_remove`-style tests to use literal JSON null payloads via `json.RawMessage`. The raw-byte approach is independent of any Go marshaling quirks that could collapse `null` values. - New TestHandlePatchServer_HeadersEmptyStringSetsNotDeletes pins the distinction between `""` (set to empty) and `null` (delete) — JSON Merge Patch is explicit about it and a future refactor that "helpfully" collapses one to the other would silently break. - Total: 7 tests, all green. Web UI (frontend/src/views/ServerDetail.vue): - deleteKv now sends `{headers: {key: null}}` instead of the array form. JSON.stringify emits `null` literally, no special handling. - Drop the `scopeRemoveKey` helper (no longer needed). macOS Swift (native/macos/MCPProxy/MCPProxy/Views/ServerDetailView.swift): - diffKVMap returns a single `[String: Any]` patch dict where deleted keys map to `NSNull()` instead of returning the previous `(set, remove)` tuple. - saveEdits writes the patch as `updates["headers"] = patch` directly; no `headers_remove` companion field anymore. - performConvertToSecret sends a single-key patch `{field: {key: ref}}` instead of building the full map — minimal wire payload, never round-trips the redacted Authorization. - The trap was real and surprising: Swift's default `JSONEncoder` on `[String: String?]` SILENTLY DROPS nil entries from the JSON output. Using `[String: Any]` with `NSNull()` + `JSONSerialization` (the encoder our APIClient already uses) renders `null` correctly. Swift unit test (native/macos/MCPProxy/MCPProxyTests/MergePatchEncodingTests.swift): - 4 tests pinning the encoding contract: 1. NSNull encodes as literal `null` via JSONSerialization. 2. A delete-only patch round-trips through JSON and the value parses back as NSNull (not "", not absent). 3. The wrong path — `[String: String?]` + default `JSONEncoder` — does silently drop nils. Documented as a poison-pill test so a future refactor that "simplifies" to it has to explicitly delete this test and read the comment first. 4. Empty string still encodes as `""` and explicit null as `null` — the JSON Merge Patch set-vs-delete distinction is preserved. CLI (cmd/mcpproxy/upstream_cmd.go + internal/cliclient/client.go): - New `mcpproxy upstream patch <name>` subcommand with flags: --header K=V upsert (repeatable) --header-remove K delete (repeatable) --env K=V upsert (repeatable) --env-remove K delete (repeatable) - New cliclient.PatchServer(name, body) sends raw JSON to PATCH /api/v1/servers/{name}. Body shape is the same JSON Merge Patch the Web UI and macOS tray send. - Closes the CLI gap I called out in the boundaries-matrix summary — REST, MCP, and CLI now all support both write and delete on headers / env with the same semantics. Live end-to-end verification: $ mcpproxy upstream patch synapbus --header "X-Cli-Test: hello-from-cli" ✅ Patched synapbus: 1 header(s) set → on disk: { Authorization (real Bearer), X-Cli-Test } (Auth preserved) $ mcpproxy upstream patch synapbus --header-remove X-Cli-Test ✅ Patched synapbus: 1 header(s) removed → on disk: { Authorization (real Bearer) } (Auth preserved) $ mcpproxy upstream patch synapbus --header "X-Foo: v" --header-remove X-Foo Error: --header and --header-remove for "X-Foo" conflict; pick one Web UI: "+ Add header" → X-WebUI-Test=from-browser → Save → on disk: { Authorization (real Bearer), X-WebUI-Test } macOS tray: Edit → textarea pre-populated with "Authorization=***REDACTED***" → user appends "X-Mac-Test=hello-from-mac" → Save → on disk: { Authorization (REAL Bearer, preserved!), X-Mac-Test } PR #463 subagent review confirmed the unrelated "disable tool" pattern is a different domain (reversible state in BBolt vs destructive mutation in mcp_config.json) and should not be unified with this work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(headers): unify display + server-side convert-to-secret for redacted values User feedback: the Headers card looked split-brained. A non-sensitive header showed `••••XX (NN chars)` with a Convert-to-secret button. The Authorization header showed `***REDACTED***` with no button — exactly the most-likely candidate for "move me to keyring" was the one the UI refused to convert, because the client side couldn't hand the real value to the existing two-step (POST /secrets, then PATCH server) flow. This change unifies both display and conversion: 1. **Single mask format on the wire** — internal/oauth/logging.go replaces the `***REDACTED***` sentinel in RedactStringHeaders with MaskValue(v), producing `••••<last2> (<N> chars)` for literal secrets. The same format the Web UI / macOS tray have been computing client-side. Bare ${keyring:NAME} / ${env:VAR} references pass through unchanged (they're labels, not secrets; the UI needs to recognise them to render the keyring chip). 2. **Server-side atomic convert** — new endpoint: POST /api/v1/servers/{name}/config-to-secret body: {"scope": "header"|"env", "key": "<k>", "secret_name": "<n>"} The backend reads the real value from the loaded config, stores it in the OS keyring under secret_name, and rewrites the config field with ${keyring:<n>}. The client never has to possess the plaintext, so Convert-to-secret now works on the redacted-on-read path too. 3. **Reveal button removed from KVValueCell.vue** — with all literals displayed identically and Convert-to-secret available everywhere, the reveal toggle was a security-shaped speed bump with no real use case. The two paths to peek at a value (open the config file, or edit-cancel) remain. revealedKeys reactive state and the reveal/hide events disappear from the parent too. 4. **Symmetric macOS Swift cleanup** — ServerDetailView.swift::kvRow drops the `value != "***REDACTED***"` gate on the Convert-to-secret button. maskedHeaderValue() drops the sentinel special case. performConvertToSecret() calls the new atomic endpoint via the new APIClient.convertConfigToSecret helper instead of two-stepping through storeSecret + PATCH. Backend tests updated for the new format: - internal/oauth/logging_test.go::TestRedactStringHeaders — asserts the ••••et (40 chars) mask shape with length + last-2 suffix. New sub-tests pin the keyring/env-ref pass-through, short-value (<=4 chars -> bare ••••), and empty-value ((empty)) edge cases. - internal/runtime/event_bus_payload_test.go — SSE redaction test asserts the new format on the wire. - internal/server/e2e_test.go — both PR #425 round-trip tests now assert mask shape instead of literal sentinel. New backend test coverage: - internal/httpapi/patch_server_test.go — 9 new sub-tests for the /config-to-secret endpoint validation paths: missing scope / key / secret_name, invalid scope, key not on server, value already a reference (keyring + env separately), empty value, server not found. Happy-path lives in the live verification because secret.Resolver is a concrete struct without a mock. End-to-end verification on live local mcpproxy: GET /api/v1/servers -> synapbus.headers.Authorization = ••••59 (71 chars) # new format -> kaggle.headers.Authorization = ••••N} (30 chars) # Bearer\${k...} also masked POST /api/v1/servers/synapbus/config-to-secret {"scope":"header","key":"Authorization","secret_name":"synapbus-authorization"} -> 200 {"reference":"\${keyring:synapbus-authorization}"} -> on disk: {"Authorization": "\${keyring:synapbus-authorization}"} -> GET response: passes the bare reference through unchanged Web UI: Headers row transformed from `Authorization ••••59 (71 chars) [lock] Convert to secret` to `Authorization [key-chip] stored in keyring: synapbus-authorization` via the Convert-to-secret modal — no intermediate steps for the user, no plaintext on the client. CLI: `upstream patch synapbus --header X-Cli-Verify=hello` / `upstream patch synapbus --header-remove X-Cli-Verify` still work; the keyring reference on Authorization survives both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: cover PR #466 headers/env editing + Convert-to-secret + new endpoints REST API (docs/api/rest-api.md): - New section on Header redaction and the ••••<last2> (<N> chars) mask format, when reveal_secret_headers applies, and why operators rarely need to flip it. - New PATCH /api/v1/servers/{name} endpoint documented with full JSON Merge Patch (RFC 7396) semantics: non-null upserts, JSON null deletes, absent keys preserved. Includes worked curl examples and the empty-string-is-not-delete gotcha. - New POST /api/v1/servers/{name}/config-to-secret endpoint — atomically moves a header/env value out of mcp_config.json into the OS keyring without the client ever holding the plaintext. CLI (docs/cli/management-commands.md): - New `upstream patch` subcommand with --header / --header-remove / --env / --env-remove flags and the deep-merge guarantee that no un-named key gets disturbed. Configuration guide (docs/configuration/upstream-servers.md): - New "Headers, Environment Variables, and Secrets" section explaining the wire mask, the three editing surfaces (Web UI / macOS tray / CLI / REST), and the ${keyring:NAME} / ${env:VAR} reference shapes. - Cross-links to the REST PATCH reference, the CLI subcommand, and the keyring integration page. Web UI (docs/web-ui/server-detail.md, new): - Dedicated page for the Server Detail Configuration tab focused on the Headers and Environment Variables cards. Documents the value formats (masked literal, keyring chip, env chip, plain), the per-row actions (add / edit / delete / convert), and the convert-to-secret flow including the atomic backend swap. - Two embedded screenshots showing the Headers card and the Convert-to-secret modal. Screenshots (docs/screenshots/server-detail/): - web-headers-card.png — the Headers card with masked Authorization + Convert to secret button. Captured against the live local instance with synapbus configured with a real Bearer token. - web-convert-modal.png — the modal preview with auto-suggested secret name and the ${keyring:NAME} live preview. Cross-references between the four pages so a reader can land anywhere and find the relevant detail in two clicks. * lint: drop trailing period from upstream patch daemon-required error staticcheck ST1005 — error strings must not end with punctuation. Flatten the multi-line message into a single sentence so the no-trailing-period rule is easy to keep over time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Code <noreply@anthropic.com>
7 tasks
4 tasks
electrolobzik
added a commit
to HaloCollar/mcpproxy-go
that referenced
this pull request
May 16, 2026
Adds TestContractsInSync, which runs the generator's content function and asserts byte-equality with the committed frontend/src/types/contracts.ts. The next time a contributor hand-edits contracts.ts (or hand-edits the hardcoded TS string in main.go) without updating both sides, CI fails with a clear message pointing at the fix: Either run \`go run ./cmd/generate-types\` from the module root (if the generator is the source of truth) or update the string literals in main.go (if contracts.ts is the source of truth). The drift this test guards against is what allowed PRs smart-mcp-proxy#424 and smart-mcp-proxy#463 to silently leave the generator out of sync. Refactors main.go to factor out generateFileContent() so the test can compare without re-implementing the header concat.
electrolobzik
added a commit
to HaloCollar/mcpproxy-go
that referenced
this pull request
May 16, 2026
Brings in upstream's 9 commits since 2b9b5f9: - 0597762 fix(upstream): stop misclassifying transport errors as auth failures (smart-mcp-proxy#464) - aaec117 fix(diagnostics): correct bug-report URL (smart-mcp-proxy#465) - d27fa38 feat(server-detail): display + edit headers/env, with reveal & convert-to-secret (smart-mcp-proxy#466) - c086770 feat(webui): per-tool enable/disable + bulk Enable All/Disable All (smart-mcp-proxy#463) - 0ef75a8 chore(docs): remove stale root-level docs - 4b4b62a fix(ui): respect engaged flag in sidebar Setup pulse (smart-mcp-proxy#462) - 24aab3d docs(installation): add migrating-from-manual-install section (smart-mcp-proxy#459) - 9b79254 feat(doctor): surface snap-docker override hint when host needs it (smart-mcp-proxy#460) - be927b6 packaging(deb): ship unattended-upgrades whitelist so installs auto-update (smart-mcp-proxy#458) Conflicts resolved: 1. internal/management/service_test.go — purely additive on upstream's side (3 new headers/env t.Run blocks). Halo-main had no overlapping test work. Took upstream's additions verbatim. 2. oas/docs.go + oas/swagger.yaml — auto-generated by swaggo from Go annotations in source. Took --theirs on the conflict then regenerated with `make swagger` against the merged source so both halo-main- and upstream-introduced REST endpoints are reflected. Note on upstream's c086770 (per-tool enable/disable, upstream's version of smart-mcp-proxy#463): halo-main already has its own per-tool enable/disable work (via feat/per-tool-enable-disable + the security-hardening stack on top — admin gating, isToolCallable fail-closed, sentinel-error quarantine synthesis, etc.). The upstream version produced no merge conflicts because the file-level diffs aligned cleanly — the fork's hardening sits atop the same surface upstream landed. Sanity-checked: - `go build ./...` succeeds. - `go test -short ./internal/management/ ./internal/runtime/ ./internal/httpapi/ ./internal/storage/ ./cmd/generate-types/` passes. - internal/server pre-existing sandbox-environment flake (TestBinaryAPIEndpoints/GET_/servers) was verified to fail on pre-merge halo-main (ad31fde) too — not a regression.
Dumbris
added a commit
that referenced
this pull request
May 17, 2026
…edes #472) (#475) * fix(generate-types): re-sync hardcoded TS output with contracts.ts The TypeScript-as-Go-string literal in cmd/generate-types/main.go drifted from frontend/src/types/contracts.ts when PR #424 (Server Config tab parity) and PR #463 (per-tool enable/disable) edited contracts.ts directly without updating the generator. Running `go run ./cmd/generate-types` (invoked by Makefile's `frontend-build` target) silently reverts those fields, producing a dirty working tree on every `make build`: - Server.isolation_defaults - IsolationConfig.network_mode, IsolationConfig.extra_args - IsolationDefaults (entire interface) - Tool.disabled, Tool.approval_status The reverted contracts.ts also feeds back into Vite's bundle hashes, which is the likely reason web/frontend/dist/* also churns on rebuilds. This commit catches the generator up to the actual contracts.ts content. After this, `go run ./cmd/generate-types` is idempotent against HEAD. Verified: generator output is byte-identical to contracts.ts. * test(generate-types): catch future contracts.ts drift in CI Adds TestContractsInSync, which runs the generator's content function and asserts byte-equality with the committed frontend/src/types/contracts.ts. The next time a contributor hand-edits contracts.ts (or hand-edits the hardcoded TS string in main.go) without updating both sides, CI fails with a clear message pointing at the fix: Either run \`go run ./cmd/generate-types\` from the module root (if the generator is the source of truth) or update the string literals in main.go (if contracts.ts is the source of truth). The drift this test guards against is what allowed PRs #424 and #463 to silently leave the generator out of sync. Refactors main.go to factor out generateFileContent() so the test can compare without re-implementing the header concat. * fix(generate-types): make TestContractsInSync CRLF-safe on Windows The new TestContractsInSync did a raw byte comparison of the committed contracts.ts against generator output. On Windows CI (core.autocrlf=true) git checks out contracts.ts with CRLF endings while the generator emits LF, so the test failed on windows-amd64 even though the contract was in sync (observed on PR #472). Two-layer fix: - .gitattributes pins frontend/src/types/contracts.ts to `text eol=lf` so it is checked out identically on every platform (the real fix). - The test now normalizes CRLF->LF before comparing, keeping it green regardless of a contributor's local git config (defense in depth). Verified: test passes with both LF and CRLF checkouts of contracts.ts; `go run ./cmd/generate-types` remains idempotent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Roman Chernyak <electrolobzik@gmail.com> Co-authored-by: Claude Code <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the gap between the per-server enable/disable that already exists and the per-tool admin toggle UI that was wired only halfway. The storage-backed
ToolApprovalRecord.Disabledmechanism + the runtime filtering inisToolCallable/retrieve_tools/handleCallToolVariantwere all already in place — but three issues prevented users from actually reaching the toggle:Runtime.SetToolEnabledreturned an error when no approval record existed for the tool — i.e. for every tool whenquarantine_enabledwasfalse(the default) orskip_quarantinewas set on the server. Now it synthesizes a baselineapprovedrecord on demand so the toggle works in every configuration.approval_status === 'approved', so non-quarantined tools (the common case) never saw it.What this adds
Backend
internal/runtime/tool_quarantine.go:SetAllToolsEnabled(serverName, enabled, updatedBy) (changed int, err error). Walks the StateView + index + storage union of known tools and toggles only those not already in the target state — so the audit trail and SSE traffic stay clean on repeated calls.internal/server/server.go:SetAllToolsEnabledcontroller wrapper.internal/httpapi/server.go:POST /api/v1/servers/{id}/tools/enable_alland.../disable_all. The existingGET /api/v1/servers/{id}/toolsnow also surfaces the per-tooldisabledflag (newcontracts.Tool.Disabledfield) so the frontend can render the toggle without a second round-trip to the approvals endpoint.internal/runtime/event_bus.go: SSEservers.changedpayload now enriches each embedded server withQuarantine{Pending,Changed,Blocked}Count(mirroringhttpapi.enrichServersWithQuarantineStats). This also fixes a pre-existing tool-quarantine staleness bug: the Web UI'smergeServerstreats incoming server data as authoritative and deletes absent fields, so on every SSE delivery the previously-fetchedQuarantineblock got wiped — manifesting as quarantine badges and the new "N disabled" pill going stale until manual page refresh.oas/swagger.yaml+oas/docs.goregenerated viamake swagger.make swagger-verifyclean.CLI
internal/cliclient/client.go:SetToolEnabled+SetAllToolsEnabledHTTP clients.cmd/mcpproxy/upstream_cmd.go: newmcpproxy upstream toolssubcommand group withenable <server> <tool>,disable <server> <tool>,enable-all <server>,disable-all <server>. Mirrors the verbs ofmcpproxy upstream enable|disable --allso the surface is consistent.Web UI (
frontend/src/views/ServerDetail.vue,frontend/src/components/ServerCard.vue)toggle toggle-smswitch, the same widget used for the server-level Enabled toggle on the Config tab — same affordance applied at different scopes.syncAfterToolTogglehelper refreshes the store-backed servers list, re-syncs the local server ref, and reloads tools + approvals after any per-tool or bulk toggle. Eliminates the lag where the in-page "N disabled" pill (and quarantine badges) stayed stale until the next SSE.frontend/src/types/contracts.ts:Tool.disabled+Tool.approval_statusare now typed so consumers don't fall back toany.Tests
TestSetToolEnabled_CreatesRecordWhenMissing— the no-record path that was the root cause of (1).TestSetAllToolsEnabled_DisablesAllKnownTools— bulk runtime method, including the idempotent re-call.TestHandleSetAllToolsEnabled_DisableAll/_EnableAll/_ControllerError— handler-level coverage for the new routes.Commits
feat(webui): per-tool enable/disable + bulk Enable All/Disable All— the main feature.fix(cli): replace deprecated strings.Title in upstream tools commands— golangci-lint staticcheck SA1019 (caught by CI).fix(ux): refresh server quarantine stats on SSE + per-tool sync + wording— the SSE quarantine-stats parity, sync-after-toggle, wording change, and toggle-switch widget.fix(ux): more per-tool toggle polish — corner toggle, server-state sync— minor UX follow-ups.refactor(ux): derive server from store (computed), drop snapshot ref— systematic fix for the stale local-UI bug class onServerDetail.vue.perf(runtime): build servers.changed payload lazily in coalescer drainer(post-review follow-up — see Update below).fix(quarantine): split SetToolEnabled, gate synthesis on sentinel error(post-review follow-up).fix(runtime): honour app-shutdown cancellation in coalescer drainer(post-review follow-up).fix(sse): plumb SecurityScan via management.ListServers so REST + SSE share enrichment(post-review follow-up).fix(ux): reload tools/approvals/logs on serverName navigation (ServerDetail.vue)(post-review follow-up).Update (2026-05-13): post-review follow-up commits
Five additional commits land the architectural cleanup recommended in the project's
PR463-FOLLOWUP-PLAN.md. They extend Spec 047 §B2's amortisation guarantee from publish-side to build-side, fix the bulk-emit storm a different way than the original sketch (no global suppression flag), close one Spec 032 rug-pull bypass, close the SecurityScan SSE-parity gap, and fix the deep-link tool/approvals/logs staleness on/servers/foo→/servers/barnavigation.6.
perf(runtime): build servers.changed payload lazily in coalescer drainer(10527bd)Spec 047 §B2 coalesced
servers.changedpublishes to ≤1 per 50ms window, but the build still ran eagerly insideemitServersChanged: aListServerscall plus an N-rowListToolApprovalsper server. Bulk operations that fired K rapid emits paid K×(1+N) BBolt ops despite the coalescer dropping K-1 publishes — Enable-All on a 30-server / 50-tool setup was ≈1,500 wasted BBolt reads per click.The coalescer now stores a lightweight
(reason, extra)marker instead of a fully-realisedEvent. The drainer materialises the SSE payload at flush time viabuildServersChangedPayload. Every emit is essentially free; the build runs once per publish window regardless of how many submits land. The no-coalescer fallback inemitServersChangedstill builds inline.Regression test:
TestCoalescer_AmortisesBuildAcrossBurstasserts 100 rapid emits → exactly 1ListServerscall.7.
fix(quarantine): split SetToolEnabled, gate synthesis on sentinel error(6032ed8)Two related fixes:
setToolEnabledNoEmit(...) (changed bool, err error).SetAllToolsEnablednow calls the no-emit core directly in its loop and emits exactly one trailingservers.changedevent (reason:tools_enabled/tools_disabled), mirroring the existingApproveAllToolspattern in the same file. Lifts the "already in desired state" pre-check into the no-emit core so single-toggle calls also avoid no-op BBolt writes + SSE emits.errors.Is(err, storage.ErrToolApprovalNotFound).GetToolApprovalreturns the new sentinel (wrapped). Any other error (decode failure, closed DB, mmap remap during compaction) is propagated to the caller. Without this gate a transient I/O error could silently demote apending/changedrecord toapproved— the Spec 032 rug-pull bypass risk.New tests:
TestSetToolEnabled_PreservesExistingPendingStatus,TestSetAllToolsEnabled_EmitsOncePerBulk,TestSetAllToolsEnabled_NoEventOnNoOp.8.
fix(runtime): honour app-shutdown cancellation in coalescer drainer(5572611)Regression caught by code-review while landing #6.
flushNownow does the build, which callsListServerswith acontext.WithTimeout(context.Background(), 2s)— rooted inBackground, notappCtx. AfterRuntime.Close()firesappCancel, the drainer goroutine could sit on the detached 2-second timer before exiting (Closedoesn't wait on the drainer, so the practical impact is a leaked goroutine doing wasted work, not user-visible shutdown latency — but it silently broke the original Spec 047 promise thatflushNowwas O(1)).Threads the parent ctx through: the coalescer stores it in
start(),flushNowuses it as the parent forbuildServersChangedPayload'sWithTimeout. Regression test:TestBuildServersChangedPayload_HonoursCancelledParentCtxasserts the call returns in < 200ms (vs. 2s) when the parent ctx is already cancelled.9.
fix(sse): plumb SecurityScan via management.ListServers so REST + SSE share enrichment(818a06e)The servers.changed SSE embed enriched
Quarantine(post-PR-463) but notSecurityScan. The RESTGET /api/v1/servershandler enriched both via inlinesecurityController.GetScanSummarycalls. mergeServers on the Web UI deletes absent keys on incoming payloads — so every tool toggle silently strippedsecurity_scanfrom every server in the store, blanking risk-score badges until the next REST refresh. Same bug class as the pre-existing quarantine-stats staleness this PR already fixes for Quarantine.Fix: plumb scan summaries through
management.ListServers(which BOTH the REST handler andruntime.buildServersChangedPayloadcall) so the two paths can't drift out of parity. Newmanagement.SecurityScanEnricherinterface +SetScanSummaryEnrichersetter;internal/serverwires ascanSummaryEnricherAdapterthat bridgesscanner.Service.GetScanSummary(scanner-internal type) tocontracts.SecurityScanSummary(the wire shape REST and SSE consumers already expect). The duplicate inline enrichment ininternal/httpapi/server.gois dropped.New tests:
TestListServers_populates_SecurityScan_via_enricher+TestListServers_nil_enricher_is_a_no-op(management/service_test.go),TestEmitServersChanged_PayloadPreservesSecurityScan(runtime/event_bus_payload_test.go).10.
fix(ux): reload tools/approvals/logs on serverName navigation (ServerDetail.vue)(d584b16)Vue Router 4 reuses the
ServerDetail.vuecomponent instance across/servers/foo→/servers/bar(same route, different param). Theservercomputed correctly retargets via the Pinia store, but the local data refs (serverTools,toolApprovals,serverLogs, scan*) stayed populated with the previous server's data. Navigating between server detail pages showed server B's name + stats with server A's tool list until a manual refresh — looked like a data-corruption bug.Adds
watch(() => props.serverName)that resets every per-server local ref and re-runsloadServerDetails. UI-state refs (activeTab,logTail,toolSearch) are intentionally not reset so the user's tab/scope choice persists across navigation.Race protection: introduces a
loadGenerationcounter bumped on everyloadServerDetailsentry. The three parallel fetches now go through internal_loadToolsWithGen/_loadToolApprovalsWithGen/_loadLogsWithGenhelpers that capture the generation at entry and only commit results if it hasn't advanced — protects the foo→bar→foo case where foo's response arrives AFTER bar's load already started. Public no-argloadTools/loadToolApprovals/loadLogswrappers preserve existing call sites (template@clickhandlers, the connected/enabled watch, post-action refreshes) unchanged.Test plan
go vet ./...cleango build ./...cleango test ./internal/runtime/ ./internal/httpapi/ ./internal/management/ ./internal/storage/ -count=1— all passnpx vue-tsc --noEmit— cleanmake swagger-verify— OAS is committed and matches generated outputTestCoalescer_AmortisesBuildAcrossBurst— bulk amortisation regression testTestSetAllToolsEnabled_EmitsOncePerBulk/_NoEventOnNoOp— bulk emit-once regression testsTestSetToolEnabled_PreservesExistingPendingStatus— sentinel-error rug-pull regression testTestBuildServersChangedPayload_HonoursCancelledParentCtx— shutdown-drain regression testTestEmitServersChanged_PayloadPreservesSecurityScan— SSE SecurityScan parity testTestListServers_populates_SecurityScan_via_enricher— management enricher wiring test/servers/foo→/servers/barand confirm bar's tool list / approvals / logs replace foo's (no stale data, no flash of A-then-B)Disable All→ confirm every tool flips, refresh page, confirm state persistsmcpproxy upstream tools disable-all <server>from the CLI then re-list to confirm🤖 Generated with Claude Code