Skip to content

Commit 770b58e

Browse files
Dumbrisclaude
andauthored
fix(tray): slim the menu + represent code_execution by its sub-calls (#1023)
The status-bar menu had grown long enough to scroll, which is the opposite of a glance, and the glance itself spent a row on an internal primitive. Menu length (3 rows back): - Drop the grey, disabled "N quarantined server(s)" line — Needs Attention already carries that fact, and actionably. - Drop "Report an Issue…" — the tracker is still reachable through the GitHub link in the About panel's credits. - Move "Add Server..." into the Servers submenu as its first item (Cmd+N still matches; performKeyEquivalent recurses into submenus). It stays at the top level only when there are no servers and therefore no submenu to hold it, so adding the first server is never unreachable. code_execution is an internal primitive, not a call the user made. Since its sub-calls became tool_call records of their own (#1016), they show the REAL work — "jira:get_issue ×3" — where the wrapper row said only "code_execution", naming no server and no tool. A SUCCESSFUL wrapper now rows in neither the tray glance nor the shared usage timeline; a FAILED one still does, through rule 3's failure branch, because a script that died before dispatch has no children to speak for it. Kept in lockstep: GlanceSelection.glanceInternalTools (Swift), AppState.countsTowardUsageTimeline (follows the same set), and runtime.glanceInternalTools (Go). usageAdmissionVersion 3 → 4 so snapshots counted under the old rule are rebuilt instead of carried forward. The Web UI activity list still shows code_execution parents — it is the drill-down home for a script — so the parity test now models the glance as the list filter plus the glance's own narrowing rather than equating the two. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 3ecbf4e commit 770b58e

10 files changed

Lines changed: 361 additions & 75 deletions

File tree

‎internal/runtime/usage_aggregate.go‎

Lines changed: 31 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,11 @@ type UsageAggregate struct {
150150
// usageAdmissionVersion identifies the CURRENT admission rule for the
151151
// aggregate. Bump it whenever Apply changes which records are counted (2:
152152
// internal tool calls and blocked policy decisions joined the timeline; 3:
153-
// management built-ins excluded again per GlanceSelection rule 1), so
154-
// pre-change snapshots are rebuilt instead of carried forward with hours
155-
// counted under the old rule.
156-
const usageAdmissionVersion = 3
153+
// management built-ins excluded again per GlanceSelection rule 1; 4: successful
154+
// code_execution wrappers excluded — their sub-calls are tool_call records that
155+
// count on their own), so pre-change snapshots are rebuilt instead of carried
156+
// forward with hours counted under the old rule.
157+
const usageAdmissionVersion = 4
157158

158159
func newUsageAggregate() *UsageAggregate {
159160
return &UsageAggregate{
@@ -197,12 +198,14 @@ func (a *UsageAggregate) tool(server, toolName string) *ToolUsage {
197198
// glance row-for-row, or the bars under the list disagree with the list
198199
// above them — the single most confusing thing a dashboard can do.
199200
//
200-
// The glance shows tool_calls, the internal tool calls that are mcpproxy's own
201-
// work (retrieve_tools, describe_tool, code_execution, upstream_servers …) and
202-
// blocked policy decisions, so all three enter the timeline. Internal
203-
// call_tool_* records are the one exclusion: they mirror a direct dispatch that
204-
// ALREADY emitted its own tool_call record with the same request id, so
205-
// counting them too would double every direct call.
201+
// The glance shows tool_calls, some of the internal tool calls that are
202+
// mcpproxy's own work (retrieve_tools, describe_tool — plus any internal call
203+
// that FAILED) and blocked policy decisions, so all three enter the timeline.
204+
// Internal call_tool_* records are excluded: they mirror a direct dispatch that
205+
// ALREADY emitted its own tool_call record with the same request id, so counting
206+
// them too would double every direct call. Successful code_execution records are
207+
// excluded for the same double-counting reason one level up — see
208+
// applyInternalToolCall.
206209
func (a *UsageAggregate) Apply(rec *storage.ActivityRecord) {
207210
if rec == nil || rec.ToolName == "" {
208211
return
@@ -295,11 +298,14 @@ func (a *UsageAggregate) applyBlocked(rec *storage.ActivityRecord) {
295298
// handleCallToolVariant, which emits both) — counting both would double it.
296299
//
297300
// SUCCESSFUL internal calls count only for the built-ins the glance actually
298-
// rows (GlanceSelection rule 3: retrieve_tools, describe_tool,
299-
// code_execution): a successful upstream_servers or quarantine_security call
300-
// is management chatter the glance hides, and counting it would put bars over
301-
// hours with no rows. Failures count regardless — any internal failure is a
302-
// glance row.
301+
// rows (GlanceSelection rule 3: retrieve_tools, describe_tool): a successful
302+
// upstream_servers or quarantine_security call is management chatter the glance
303+
// hides, and counting it would put bars over hours with no rows. A successful
304+
// code_execution is out for a different reason — the sub-calls its script made
305+
// are tool_call records that already count, so the wrapper would double them
306+
// under a name no upstream owns. Failures count regardless — any internal
307+
// failure is a glance row, and a script that died before dispatching anything
308+
// has nothing else to represent it.
303309
func (a *UsageAggregate) applyInternalToolCall(rec *storage.ActivityRecord) {
304310
if strings.HasPrefix(rec.ToolName, storage.InternalCallToolPrefix) {
305311
return
@@ -319,13 +325,19 @@ func (a *UsageAggregate) applyInternalToolCall(rec *storage.ActivityRecord) {
319325

320326
// glanceInternalTools are the built-ins whose SUCCESSFUL calls appear as rows
321327
// in the tray glance (GlanceSelection.swift rule 3) and therefore in the
322-
// timeline; glanceManagementBuiltins are excluded from the glance entirely
323-
// (rule 1), success or failure. Keep both in lockstep with the Swift sets and
324-
// with AppState.countsTowardUsageTimeline.
328+
// timeline: retrieve_tools and describe_tool. code_execution is NOT one of them
329+
// — it is an internal primitive, and the upstream calls its scripts make are
330+
// tool_call records that count on their own, so counting the wrapper too would
331+
// bar a script twice while naming no tool. A FAILED code_execution still counts,
332+
// through the failure branch in applyInternalToolCall: it has no sub-calls to
333+
// speak for it.
334+
//
335+
// glanceManagementBuiltins are excluded from the glance entirely (rule 1),
336+
// success or failure. Keep both in lockstep with the Swift sets and with
337+
// AppState.countsTowardUsageTimeline.
325338
var glanceInternalTools = map[string]bool{
326339
"retrieve_tools": true,
327340
"describe_tool": true,
328-
"code_execution": true,
329341
}
330342

331343
var glanceManagementBuiltins = map[string]bool{

‎internal/runtime/usage_catchup_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ func TestActivityService_SnapshotLoad_RebuildsWhenTheAdmissionRuleChanged(t *tes
132132

133133
// The store holds an internal call the OLD rule never counted.
134134
require.NoError(t, mgr.SaveActivity(&storage.ActivityRecord{
135-
Type: storage.ActivityTypeInternalToolCall, ToolName: "code_execution",
135+
Type: storage.ActivityTypeInternalToolCall, ToolName: "retrieve_tools",
136136
Status: storage.ActivityStatusSuccess, Timestamp: base,
137137
}))
138138

‎internal/runtime/usage_glance_parity_test.go‎

Lines changed: 74 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,24 +10,52 @@ import (
1010
"github.com/smart-mcp-proxy/mcpproxy-go/internal/storage"
1111
)
1212

13-
// glanceRows applies the SAME admission rule the activity list uses by default
14-
// (storage.DefaultActivityFilter) so the two populations can be compared
15-
// directly rather than by eye.
13+
// glanceRows counts the rows the TRAY GLANCE renders for these records, so the
14+
// timeline can be compared against them directly rather than by eye.
15+
//
16+
// It is two stages, because the glance is NOT the activity list:
17+
//
18+
// stage 1 — storage.DefaultActivityFilter, the list's own default admission.
19+
// It is what the Web UI shows, and it deliberately keeps the code_execution
20+
// parent row: the list is that script's drill-down home.
21+
// stage 2 — the glance's extra narrowing (GlanceSelection rules 1 and 3). The
22+
// tray has five rows, so it drops what something else already represents: a
23+
// successful code_execution (its sub-calls are tool_call records of their
24+
// own), successful management chatter, and sheds that never ran.
25+
//
26+
// Stage 2 is spelled out literally instead of reading glanceInternalTools /
27+
// glanceManagementBuiltins, so this stays an independent oracle rather than a
28+
// restatement of the code under test.
1629
func glanceRows(records []*storage.ActivityRecord) int {
1730
filter := storage.DefaultActivityFilter()
31+
rowsOnSuccess := map[string]bool{"retrieve_tools": true, "describe_tool": true}
32+
management := map[string]bool{"upstream_servers": true, "quarantine_security": true}
33+
1834
n := 0
1935
for _, rec := range records {
20-
if filter.Matches(rec) {
21-
n++
36+
if !filter.Matches(rec) {
37+
continue
38+
}
39+
if rec.Type == storage.ActivityTypeInternalToolCall {
40+
if management[rec.ToolName] {
41+
continue // rule 1: never a row, whatever the status
42+
}
43+
if rec.Status == storage.ActivityStatusSuccess && !rowsOnSuccess[rec.ToolName] {
44+
continue // rule 3: on success, only the discovery built-ins row
45+
}
2246
}
47+
if rec.Type == storage.ActivityTypeToolCall && rec.Status == storage.ActivityStatusRejected {
48+
continue // spec 093: a shed call never executed
49+
}
50+
n++
2351
}
2452
return n
2553
}
2654

2755
// The bars under the Recent list have to count the rows in it. They used to
28-
// count tool_calls only, so an agent whose traffic was mostly retrieve_tools
29-
// and code_execution — which is most agents — looked at a list of work under an
30-
// almost empty histogram.
56+
// count tool_calls only, so an agent whose traffic was mostly retrieve_tools —
57+
// which is most agents — looked at a list of work under an almost empty
58+
// histogram.
3159
func TestUsageAggregate_TimelineMatchesTheGlancePopulation(t *testing.T) {
3260
ts := time.Date(2026, 6, 1, 10, 0, 0, 0, time.UTC)
3361
records := []*storage.ActivityRecord{
@@ -37,7 +65,10 @@ func TestUsageAggregate_TimelineMatchesTheGlancePopulation(t *testing.T) {
3765
// mcpproxy's own built-ins: visible in the glance, and until now
3866
// invisible in the histogram.
3967
{Type: storage.ActivityTypeInternalToolCall, ToolName: "retrieve_tools", Status: storage.ActivityStatusSuccess, DurationMs: 5, Timestamp: ts},
68+
// A script that ran: the glance shows the calls it MADE, not the
69+
// wrapper, so this record is a row in neither list nor histogram.
4070
{Type: storage.ActivityTypeInternalToolCall, ToolName: "code_execution", Status: storage.ActivityStatusSuccess, DurationMs: 400, Timestamp: ts},
71+
{Type: storage.ActivityTypeToolCall, ServerName: "jira", ToolName: "get_issue", Status: storage.ActivityStatusSuccess, DurationMs: 30, Timestamp: ts, ParentID: "exec-1"},
4172
{Type: storage.ActivityTypeInternalToolCall, ToolName: "describe_tool", Status: storage.ActivityStatusError, DurationMs: 2, Timestamp: ts},
4273
// A policy block: a red row in the glance.
4374
{Type: storage.ActivityTypePolicyDecision, ServerName: "evil", ToolName: "exfil", Status: storage.ActivityStatusBlocked, Timestamp: ts},
@@ -52,10 +83,42 @@ func TestUsageAggregate_TimelineMatchesTheGlancePopulation(t *testing.T) {
5283
require.Len(t, timeline, 1)
5384
assert.EqualValues(t, glanceRows(records), timeline[0].Calls,
5485
"every row the user sees must be a call in the histogram")
86+
assert.EqualValues(t, 6, timeline[0].Calls,
87+
"two dispatches, the script's sub-call, retrieve_tools, the failed "+
88+
"describe_tool and the block — but not the code_execution wrapper")
5589
assert.EqualValues(t, 3, timeline[0].Errors,
5690
"the failed dispatch, the failed built-in and the block are all failures")
5791
}
5892

93+
// code_execution is an internal PRIMITIVE, not a call the user made: a script
94+
// that ran is represented by the upstream calls it issued, which are tool_call
95+
// records that count on their own. Counting the wrapper too would put a bar
96+
// under a name no upstream owns and double the script.
97+
//
98+
// A FAILED one is the exception, and the reason is that it has no children: a
99+
// script that died of a syntax error dispatched nothing, so its own record is
100+
// the only trace of the attempt anywhere.
101+
func TestUsageAggregate_CodeExecutionWrapperCountsOnlyWhenItFailed(t *testing.T) {
102+
ts := time.Date(2026, 6, 1, 10, 0, 0, 0, time.UTC)
103+
104+
agg := newUsageAggregate()
105+
agg.Apply(&storage.ActivityRecord{
106+
Type: storage.ActivityTypeInternalToolCall, ToolName: "code_execution",
107+
Status: storage.ActivityStatusSuccess, DurationMs: 400, Timestamp: ts,
108+
})
109+
assert.Empty(t, agg.Timeline(), "a successful script bars through its sub-calls, not through itself")
110+
assert.Empty(t, agg.Tools, "and it is not an upstream tool either")
111+
112+
agg.Apply(&storage.ActivityRecord{
113+
Type: storage.ActivityTypeInternalToolCall, ToolName: "code_execution",
114+
Status: storage.ActivityStatusError, DurationMs: 3, Timestamp: ts,
115+
})
116+
timeline := agg.Timeline()
117+
require.Len(t, timeline, 1)
118+
assert.EqualValues(t, 1, timeline[0].Calls, "a script that never dispatched has only its own row")
119+
assert.EqualValues(t, 1, timeline[0].Errors)
120+
}
121+
59122
// The direct dispatch path emits BOTH a tool_call record and a call_tool_*
60123
// internal record for one call. The glance hides the internal one; so must the
61124
// histogram, or every direct call is counted twice.
@@ -97,9 +160,9 @@ func TestUsageAggregate_BuiltinsStayOutOfThePerToolRollup(t *testing.T) {
97160
}
98161

99162
// Successful management built-ins (upstream_servers, quarantine_security, …)
100-
// are not glance rows — GlanceSelection rule 3 admits only retrieve_tools,
101-
// describe_tool and code_execution plus any internal failure — so a config
102-
// sweep must not paint bars over an hour with no visible rows.
163+
// are not glance rows — GlanceSelection rule 3 admits only retrieve_tools and
164+
// describe_tool plus any internal failure — so a config sweep must not paint
165+
// bars over an hour with no visible rows.
103166
func TestUsageAggregate_SuccessfulManagementBuiltinsStayOutOfTheTimeline(t *testing.T) {
104167
ts := time.Date(2026, 6, 1, 10, 0, 0, 0, time.UTC)
105168
agg := newUsageAggregate()

‎native/macos/MCPProxy/MCPProxy/MCPProxyApp.swift‎

Lines changed: 38 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -659,6 +659,19 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS
659659
connectClientForm.dismiss()
660660
}
661661

662+
/// The "Add Server..." item, built in one place because it is now rendered
663+
/// from two: normally as the first entry of the Servers submenu, and at the
664+
/// top level only when there are no servers and therefore no submenu.
665+
///
666+
/// `target` is set explicitly — an item in a submenu with a nil target would
667+
/// fall back to the responder chain, and the tray has none while the menu is
668+
/// open, so the click would silently do nothing.
669+
func makeAddServerItem() -> NSMenuItem {
670+
let item = NSMenuItem(title: "Add Server...", action: #selector(showAddServer), keyEquivalent: "n")
671+
item.target = self
672+
return item
673+
}
674+
662675
@objc private func showAddServer() {
663676
showMainWindow()
664677
// First switch to the Servers tab so ServersView is mounted and
@@ -1070,23 +1083,26 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS
10701083
menu.addItem(.separator())
10711084
}
10721085

1073-
// Quarantine
1074-
if appState.quarantinedToolsCount > 0 {
1075-
let quarantineItem = NSMenuItem(
1076-
title: "\(appState.quarantinedToolsCount) quarantined server(s)",
1077-
action: nil, keyEquivalent: "")
1078-
quarantineItem.isEnabled = false
1079-
quarantineItem.image = NSImage(systemSymbolName: "shield.lefthalf.filled",
1080-
accessibilityDescription: "quarantine")
1081-
menu.addItem(quarantineItem)
1082-
menu.addItem(.separator())
1083-
}
1086+
// No standalone quarantine line: "N quarantined server(s)" was a
1087+
// disabled, grey, non-actionable row, and the same fact is already
1088+
// carried — actionably — by the Needs Attention submenu above. The menu
1089+
// had grown long enough to scroll, and an informational row that cannot
1090+
// be clicked is the first thing to spend.
10841091

10851092
// Servers — as a SUBMENU (not flat list)
10861093
if !appState.servers.isEmpty {
10871094
let serversMenuItem = NSMenuItem(title: "Servers (\(appState.servers.count))", action: nil, keyEquivalent: "")
10881095
let serversSubmenu = NSMenu()
10891096

1097+
// "Add Server..." lives HERE, not at the top level: adding a server
1098+
// is a servers-menu action, and hoisting it out cost a permanent
1099+
// top-level row in a menu that had started to scroll. The Cmd+N key
1100+
// equivalent still works from a submenu — AppKit walks the whole menu
1101+
// tree when matching key equivalents — and still displays beside the
1102+
// title once the submenu is open.
1103+
serversSubmenu.addItem(makeAddServerItem())
1104+
serversSubmenu.addItem(.separator())
1105+
10901106
for server in appState.servers {
10911107
let item = NSMenuItem(title: server.name, action: nil, keyEquivalent: "")
10921108

@@ -1200,9 +1216,14 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS
12001216
}
12011217

12021218
// Actions
1203-
let addServer = NSMenuItem(title: "Add Server...", action: #selector(showAddServer), keyEquivalent: "n")
1204-
addServer.target = self
1205-
menu.addItem(addServer)
1219+
//
1220+
// "Add Server..." normally lives inside the Servers submenu. The one
1221+
// exception is a proxy with no servers at all: there is no Servers
1222+
// submenu to hold it then, and the very user who most needs to add one
1223+
// would be left without a way to do it from the tray.
1224+
if appState.servers.isEmpty {
1225+
menu.addItem(makeAddServerItem())
1226+
}
12061227

12071228
// Spec 091 FR-001: the native connect journey, beside Add Server. The
12081229
// item is built by its router so the item and its routing are tested
@@ -1334,9 +1355,9 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS
13341355
docsItem.target = self
13351356
menu.addItem(docsItem)
13361357

1337-
let issueItem = NSMenuItem(title: "Report an Issue…", action: #selector(reportIssue), keyEquivalent: "")
1338-
issueItem.target = self
1339-
menu.addItem(issueItem)
1358+
// No "Report an Issue…" item: the menu had grown long enough to scroll
1359+
// on a normal display, and the tracker is still reachable through the
1360+
// GitHub link in the About panel's credits block.
13401361

13411362
let aboutItem = NSMenuItem(title: "About MCPProxy", action: #selector(showAboutPanel), keyEquivalent: "")
13421363
aboutItem.target = self
@@ -1643,10 +1664,6 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS
16431664
NSWorkspace.shared.open(ProjectLinks.docs)
16441665
}
16451666

1646-
@objc private func reportIssue() {
1647-
NSWorkspace.shared.open(ProjectLinks.issues)
1648-
}
1649-
16501667
/// Shows the standard About panel (app name + version) with a credits block
16511668
/// that links back to the homepage, source, and docs — so "About MCPProxy"
16521669
/// carries the way back to the project, not just a version string.

‎native/macos/MCPProxy/MCPProxy/Menu/Glance/GlanceSelection.swift‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,17 @@ enum GlanceSelection {
118118
/// Proxy administration built-ins. Never shown, whatever their status.
119119
static let managementBuiltIns: Set<String> = ["upstream_servers", "quarantine_security"]
120120

121-
/// Discovery/execution built-ins that are worth a row even on success.
122-
static let glanceInternalTools: Set<String> = ["retrieve_tools", "code_execution", "describe_tool"]
121+
/// Discovery built-ins that are worth a row even on success.
122+
///
123+
/// `code_execution` is deliberately NOT here. It is an internal primitive —
124+
/// a wrapper around the upstream calls a script makes — and since those
125+
/// sub-calls became `tool_call` records of their own they speak for it far
126+
/// better than it did: the glance now shows the REAL work ("jira_get_issue
127+
/// ×3") instead of one opaque "code_execution" row that named no server and
128+
/// no tool. A FAILED code_execution still rows, through rule 3's failure
129+
/// branch below — a script that died of a syntax error has no children to
130+
/// speak for it, so its own record is the only trace there is.
131+
static let glanceInternalTools: Set<String> = ["retrieve_tools", "describe_tool"]
123132

124133
/// How many rows each list shows.
125134
static let rowLimit = 5
@@ -148,8 +157,10 @@ enum GlanceSelection {
148157
return entry.outcomeClass == .blocked
149158
}
150159

151-
// Rule 3 — discovery/execution built-ins, plus any internal failure
152-
// (a wrapper that died before dispatch has no upstream record).
160+
// Rule 3 — discovery built-ins, plus any internal failure (a wrapper
161+
// that died before dispatch has no upstream record). A SUCCESSFUL
162+
// code_execution falls through both clauses on purpose: its sub-calls
163+
// are the rows now.
153164
if entry.type == "internal_tool_call" {
154165
return glanceInternalTools.contains(tool) || entry.status != "success"
155166
}
@@ -178,9 +189,10 @@ enum GlanceSelection {
178189
/// per sub-call — the counter suffix in `mintCorrelationIDAt` is what makes
179190
/// it unique — and names the script through `parent_id`, which carries the
180191
/// parent's request id rather than a shared one. No child shares an id with
181-
/// another child or with the parent, so a multi-tool script renders as the
182-
/// script's own row plus one row per sub-call (the transparency the feature
183-
/// exists for) instead of collapsing to one.
192+
/// another child or with the parent, so a multi-tool script renders as one
193+
/// row per sub-call (the transparency the feature exists for) instead of
194+
/// collapsing to one. The successful wrapper itself never reaches this step
195+
/// — rule 3 dropped it — so those rows are the script's whole story.
184196
///
185197
/// Note the legacy `ToolCallRecord` written beside it still carries
186198
/// `RequestID: u.executionID` ("Use execution ID as request ID to link

0 commit comments

Comments
 (0)