feat(web,macos): one server-card status line + primary action (Spec 109-e) - #1381
Merged
Merged
Conversation
Spec 109 FR-010-FR-012: contracts.HealthStatus gains `status` (one cross-surface vocabulary: ready, connecting, sign_in_required, needs_review, needs_secret, needs_config, error, disabled), `usable` (true only when status == ready) and `actions` (every applicable next step in priority order, with the invariant action == actions[0]). The quarantined branch of the calculator now checks the same OAuth-login-required signals the later OAuth branches use before it short-circuits on the admin state alone, so a quarantined server that also needs sign-in reports actions ["login","approve"] and action "login" (was "approve" — the one declared value change, FR-010). level/admin_state/summary/detail/action keep their existing values for every existing branch; this is purely additive at the Go layer.
cmd/generate-types/main.go: add the HealthStatusValue enum plus HEALTH_STATUS_LABELS / HEALTH_ACTION_LABELS (mirroring internal/health/constants.go's StatusLabels / ActionLabels) and the new HealthStatus.status/usable/actions fields, then regenerate frontend/src/types/contracts.ts and oas/swagger.yaml (go run ./cmd/generate-types; make swagger).
Spec 109 FR-011: no surface may render `level` as text. Adds healthStatusLabel()/healthActionLabel() to utils/health.ts (backed by the generated HEALTH_STATUS_LABELS/HEALTH_ACTION_LABELS tables) and uses them for: - ServerCard's status chip fallback (was `health.summary || health.level`) - ServerDetail's top status line (same fallback) and its Configuration → Health card, which now shows a Status row (label) and every Actions[] entry instead of the bare Level/Action strings - The server-edition Teams views (UserServers.vue, AdminServers.vue), which rendered the raw `level` value directly `level` keeps driving badge/tray color only, per FR-011's severity vs. text distinction.
…us filter Spec 109 FR-015: `mcpproxy upstream list` STATUS column now renders the cross-surface status label (health.StatusLabel) instead of the free-text summary — a declared table-output change; the summary stays available via `-o json`'s health.summary. ACTION is keyed on health.actions[0] instead of the legacy `action` field (the two are invariant-equal today), with a new `edit_url` -> "Edit config" hint. Adds a repeatable `--status <value>` flag (a comma-separated value is equivalent to repeating the flag; several values select the union of matching rows), applied to every output format. The GH #938 held-tools suffix and ACTION fallback are unchanged in shape, now appended to the label instead of the summary. Verified live against a real running instance (`upstream list`, `--status needs_review`, repeated/comma-separated `--status`, `-o json`).
… text Spec 109 FR-010-FR-011/FR-014 labels: HealthStatus decodes the new status/usable/actions fields (optional, tolerant of an older core) and gains statusLabel/isUsable plus the statusLabels/actionLabels tables mirroring contracts/health-vocabulary.md. ServerDetailView's Configuration -> Health section now shows a Status row (label) and every Actions[] entry instead of the bare Level/Action strings. Three accessibility labels (ServerDetailView, DashboardView, ServersView) that spoke the raw `level` value to VoiceOver now use statusLabel instead.
GET /api/v1/servers's Health Object Fields table and example payload were still FR-010-era only. Adds status/usable/actions, and calls out that level is a severity signal only — no surface may render it as text (FR-011).
All 7 findings verified against the code and confirmed genuine; none
rejected as false positives.
- web: ServerDetail's main Health tile still rendered `level` as text
("Healthy"/"Degraded"/"Unhealthy") for enabled servers, violating
FR-011. A connecting server (level=healthy/status=connecting/
usable=false) showed a green "Healthy" right above "Connecting...".
Now renders `health.status` through the shared label table; `level`
still drives the tile's color only. Updated the pre-existing spec
that pinned the old wording.
- macos: HealthStatus.isUsable's backward-compat fallback (for a core
payload with no `usable` field) read a "connecting" server as usable
because it only checked level+adminState. Added a check for the
exact pre-Spec-109 "connecting" summary shape.
- macos: the Dashboard's AttentionRow primary button read its label
from the old HealthAction enum ("Approve", "Set Secret") instead of
the new HealthStatus.actionLabels table ("Review", "Add secret"),
diverging from the Web UI/CLI wording. HealthAction also had no
edit_url case, so a server whose action is edit_url showed no button
at all. Added the case and switched the button/accessibility label
to the shared table.
- health: strengthened status_test.go's derivation table — added rows
for the plain "disconnected" state and for a quarantined server with
an OAuth-related error in "error" state (previously untested), and
asserted full Actions slices (not just Actions[0]) for every branch
that returns more than one action.
- cli: `mcpproxy upstream list --status <value>` never validated
against the closed status vocabulary, so a typo or wrong case
silently returned an empty table (exit 0), indistinguishable from
"no servers in that state". Added validateStatusFlag, mirroring
validateTrustModeFlag. Also added a test that exercises --status
through actual Cobra/pflag registration instead of only calling
filterServersByStatus directly.
- docs: corrected the CLI emoji legend, which claimed needs_secret/
needs_config are amber (they are level=unhealthy, i.e. red) and
needs_review is amber (it only ever appears while quarantined, i.e.
the lock emoji) — contradicted by the doc's own example three lines
below. Also fixed a stale quarantine example showing a STATUS value
("Pending approval") the CLI never emits.
All 14 findings verified against the code; none rejected as false positives, though two are documented as intentional rather than changed. Fixed: - macos: performAction's switch had no case for editURL/setSecret/ configure/viewLogs (round 1 added the editURL button but not its handler), so those buttons silently no-op'd. All four now navigate to the server's detail view (Config tab, or Logs for view_logs) via the existing .showServerDetail notification route, extended with an optional target tab. - generate-types: added TestHealthVocabularyMatchesConstants, which reads internal/health/constants.go directly (mirroring the existing internal/preflight/contracts_drift_test.go pattern) so a status/action/ label added there without a matching hand-edit in main.go's string literals now fails a test instead of leaving contracts.ts silently stale. - health: added 6 missing derivation-table rows (disconnected+endpoint- address-error, disconnected+OAuth-error, UserLoggedOut, and the three OAuthStatus branches) asserting full Status/Usable/Actions, not just the actions[0]==action invariant. Also pinned the full Actions slice on every remaining single-action row (previously nil = unchecked) and added TestConnectionErrorStatus documenting its default branch. - cli: added a test that calls runUpstreamList itself with an invalid --status, so a refactor that drops/moves the validateStatusFlag call reintroduces a visible failure instead of silently reaching the daemon. - cli: an invalid --status value exited 4 (config error) instead of 1, because its own error text enumerates the real status "needs_config" and tripped classifyError's generic string heuristic. Added a typed flagValidationError so both --status and --trust-mode validation errors classify as ExitCodeGeneralError directly, bypassing the heuristic (same pattern already used for preflight/StartupError). - docs+code: removed all 15 dangling references to contracts/health-vocabulary.md, which exists only on the separate 109-ux-navigation-consistency spec branch and is absent from this branch's tree; replaced with Spec 109 FR references and pointers to internal/health/constants.go, which does exist here. - httpapi: added a REST-path test (GET /api/v1/servers) proving the handler's enrichment chain (quarantine/security-scan/secret-redaction/ scope) passes health.status/usable/actions through unmodified; only the MCP surface had this coverage before. - web: AdminServers/UserServers status fallback was `summary || level`, which would print the banned raw level word when a payload lacks both status and summary (version skew). Falls back to connected/disconnected instead. - web+macos: ServerDetail.vue's Config tab Health card (Status field) and the Suggested Action row, and the macOS Config tab's Suggested Action row, dropped/blanked for an old-core payload carrying only the legacy singular `action` field, inconsistent with this same PR's own isUsable old-core fallback. Added matching fallbacks (macOS: HealthStatus. actionsOrLegacyFallback; web: configHealthStatusLabel/ configHealthActions). - docs: CLI emoji legend now documents the GH #938 tool-hold overlay (⚠️ + "· N held" suffix on an otherwise-healthy server). - docs: REST API example showed `"action": ""` for a ready server, but the `omitempty` tag means it is never serialized as an empty string; removed the misleading line. Documented, not changed (verified genuine but not worth the behavior change): - macos: ServersView's row context menu intentionally does not read HealthStatus.actionLabels — it lists every applicable command as its own imperative verb ("Approve All Tools", "View Logs"), several with no HealthAction counterpart at all, so there is no single shared table for it to bind to. The Status column's visible text vs. its accessibility label is a deliberate sighted/VoiceOver wording choice (summary is richer prose, never the banned `level` word), not an FR-011 violation. Added comments scoping both out of the one-table mandate. - cli: `--status ""` (or a trailing comma) is silently accepted as "no filter" — this already mirrors --trust-mode's own "" = inherit-default contract, and the existing TestValidateStatusFlag pins it as accepted behavior. Rejecting it would reverse a pinned test and risk breaking a script that interpolates `--status=$VAR`. Added a comment documenting the equivalence instead of changing behavior. Verified locally: go build ./...; go test -race on internal/health, cmd/mcpproxy, cmd/generate-types, internal/httpapi (bare and -tags server); golangci-lint v2 bare and --build-tags server (pre-existing issues only, none in touched files); frontend npm run build + npx vitest run (1361 tests, all green); swift build + swift test (1 pre-existing, environment-caused failure in AppLifecycleTests, unrelated to any file this round touches — this dev machine's real ~/.mcpproxy/tray-lifecycle.jsonl already exists from prior real tray usage, which the test's fileExists check cannot distinguish from a write by the test itself).
All 7 round-3 findings verified against the code; all confirmed genuine,
none rejected.
- internal/health/calculator.go: quarantinedOAuthLoginState (FR-010) only
checked pending-auth, an OAuth-shaped error string, and call-time OAuth
requirement. It never checked UserLoggedOut, OAuthStatus=="expired",
OAuthStatus=="error", or OAuthRequired&&OAuthStatus in {"none",""} — the
same inputs the non-quarantined branch checks and that are populated for
quarantined servers via the same runtime plumbing (internal/runtime.go).
A quarantined server with an expired/revoked/never-obtained OAuth token
reported needs_review/[approve] instead of sign_in_required/[login,
approve]; approving it would immediately flip back to "Sign-in required".
Added the four missing signals, mirroring the non-quarantined branch's
level/summary. New test rows in status_test.go for each signal, verified
failing before the fix.
- native/macos/.../ServersView.swift: the manual double-click open path
never reset `selectedServerInitialTab`, which is otherwise sticky from
whatever tab the last `.showServerDetail` notification requested (e.g. a
Dashboard "Add secret" action opening on Config). A later manual
double-click on an unrelated server would silently reopen on that same
stale tab instead of Tools. Reset the tab to `.tools` before assigning
`selectedServer` in the double-click closure. Added
ServersViewRoutingTests.swift as a source-level regression guard (no
ViewInspector/snapshot harness in this package to drive @State directly);
confirmed it fails without the fix and passes with it.
- native/macos/.../DashboardRoutingTests.swift:
testAttentionRowPerformActionHandlesEveryHealthAction only checked that
each action name appears somewhere inside a `case ...:` label, never what
that case's body does — it would still pass if `.viewLogs` navigated to
`.config`, or if a case reverted to `default: break`. Added
testAttentionRowPerformActionRoutesToTheCorrectTab, which extracts each
case's actual body and asserts the tab it passes to
navigateToServerDetail, plus that navigateToServerDetail posts a typed
ServerDetailTarget rather than a bare String (which would make
ServersView's observer silently default every route to .tools). Confirmed
it fails when the tab argument is swapped.
- docs/api/rest-api.md: the oauth-server example fabricated a
`"detail": "OAuth access token has expired"` value; the expired-token
branch in calculator.go sets no Detail, and `detail,omitempty` drops the
key from the wire. Removed the invented line.
- docs/cli-management-commands.md: pointed operators at
`mcpproxy tools --server=<name>`, a group with no such flag; the real
command named correctly one line above is `mcpproxy tools list
--server=<name>`. Fixed.
- cmd/mcpproxy/upstream_cmd.go: the config-mode (daemon-less) path builds
`health["actions"]` directly from CalculateHealth(...).Actions, a native
[]string with no JSON round-trip, but the ACTION-column parser only
type-asserted []interface{} (the shape a JSON-decoded client-mode
response has). The assertion silently failed in config mode, falling back
to the legacy `action` field. Latent today only because action ==
actions[0] holds for every branch. Added a []string case to the type
switch. New regression test (upstream_actions_type_test.go) with a
deliberately mismatched action/actions[0] pins the real invariant against
both slice shapes; confirmed failing before the fix.
- frontend/tests/unit/{admin,user}-servers-status-fallback.spec.ts: both
only asserted the banned `level` word was absent, never that the actual
'connected' fallback text renders, and checked the whole row instead of
the status badge cell. Strengthened to target the Status column's badge
specifically and assert its exact text.
Verified: go build ./... and -tags server both clean; go test -race on
internal/health and cmd/mcpproxy pass; golangci-lint v2 (bare and
--build-tags server) show only pre-existing issues in files this round
did not touch; frontend build + full vitest suite (1361 tests) pass,
package-lock.json untouched; swift test passes except one pre-existing,
unrelated failure (AppLifecycleTests, confirmed present on the
pre-round-3 tree too, tied to this machine's real ~/.mcpproxy instance
root).
Verified each round-4 finding against the code before fixing.
- internal/oauth/serverfields.go (high, confirmed): contracts.Server's new
health.status/health.actions leaves had no entry in
ServerFieldMaskDecisions, so TestServerFieldMaskDecisions_CoverEveryNestedLeaf
failed (reproduced: FAIL before the fix). Added both as
MaskDecisionNotSecret.
- internal/health/calculator.go (moderate, confirmed): quarantinedOAuthLoginState
mirrored the non-quarantined "error" state's isOAuthRelatedError override but
not "disconnected", even though the non-quarantined switch applies it to
both. A quarantined server whose OAuth-related failure settles into
state=disconnected with a stale OAuthStatus (e.g. still "authenticated")
fell through to the generic "Quarantined for review" response instead of
surfacing sign-in-required. Added "disconnected" to that switch case
(test-first: TestCalculateHealth_QuarantinedDisconnectedOAuthError failed
before the fix).
- internal/health/calculator.go (low, confirmed): quarantinedOAuthLoginState
had no connecting/idle early-return, so a quarantined server mid-dial could
get a login CTA pre-empting the "genuine connecting states always take
priority" invariant section 4 documents. Added the early-return and
corrected the function's doc comment, which overclaimed it mirrored "all
seven" OAuth-login signals "regardless of admin state" (test-first:
TestCalculateHealth_QuarantinedConnectingSkipsOAuthLoginCTA).
- internal/tray/managers.go, cmd/mcpproxy-tray/internal/api/{client,adapter}.go
(moderate, confirmed): the Go cross-platform tray never carried
health.status/usable/actions and its empty-summary fallback rendered the
raw severity level ("healthy") as status text. Added the three fields to
the wire structs/adapter and switched the fallback to the shared
health.StatusLabel table (test-first:
TestGetServerStatusDisplay_SummaryEmptyFallsBackToStatusLabel,
TestServerAdapter_GetAllServers_ForwardsHealthVocabulary). The menu's
action-selection logic already keys off the legacy single `action` field,
which still satisfies action==actions[0]; left as-is rather than
reworking it to consume the ordered Actions slice, since that would be a
new capability, not a fix.
- cmd/mcpproxy/upstream_actions_type_test.go (moderate, confirmed
coverage gap): the round-3 test only pinned the actions[0] invariant for
the []string (config-mode) shape, not the []interface{} (client/daemon
JSON) shape the production switch also handles. Added
TestUpstreamRowsReadActionsAsJSONInterfaceSlice with a deliberate
action/actions[0] mismatch; it passes against the existing code, closing
the coverage gap without needing a production fix.
- internal/httpapi/get_servers_health_vocabulary_test.go (low, confirmed
coverage gaps): added a Detail assertion (the one health field the REST
redaction chain can rewrite) and a new
TestHandleGetServers_HealthCarriesMultiActionSlice pinning a genuine
2-element actions slice through the REST handler's JSON round-trip. Left
internal/server's MCP-side wire test as-is: it already documents this as
a wire-layer-only residual risk with existing unit coverage in
internal/health/status_test.go, and wiring a multi-action fixture through
the MCP upstream_servers add flow needs live OAuth-signal plumbing not
worth the added integration-test weight for a low-severity gap.
- frontend/src/utils/health.ts, ServerCard.vue, ServerDetail.vue (low,
confirmed): AdminServers.vue/UserServers.vue already fell back to
connected/disconnected when both health.summary and health.status are
empty; ServerCard.vue and ServerDetail.vue did not, rendering an empty
status chip on that version-skew payload. Extracted the shared
healthStatusText() helper and used it in both (test-first: new case in
health-status-labels.spec.ts).
Verification: go build ./... (default and GOOS=linux/windows), go vet
(default and -tags server), golangci-lint v2 with .github/.golangci.yml
(default and --build-tags server, no new issues), go test -race on every
touched package, plus the CLAUDE.md CI skip-regex race run under -tags
server for internal/serveredition/config/oauth/server/httpapi/storage (all
ok, internal/server 499s). Frontend: npm run build (vue-tsc + vite) and
npx vitest run (1362 tests, 139 files) both green; package-lock.json
unchanged.
Fixes 4 verified findings from the round-5 cross-review of Spec 109's
health-vocabulary work. All four were confirmed against the code and
fixed test-first; none were rejected.
1. (critical) Go tray Client.GetServers() decoded health.level/
admin_state/summary/detail/action from the raw JSON map but never
extracted status/usable/actions into HealthStatus, even though the
struct declares json tags for all three (Spec 109 FR-010-012).
ServerAdapter.GetAllServers() already forwarded Health.Status/Usable/
Actions correctly, and managers.go already had a `status`-driven
render branch — but both silently no-op'd because the real HTTP
decode path never populated the fields. Every server reported
Status="", Usable=false, Actions=nil on the real Windows/macOS-archive
tray binary. Added a getStringSlice helper and wired the three fields
into the decode; added TestClientGetServers_DecodesHealthVocabulary,
which exercises the real HTTP JSON decode path (the existing adapter
test injected a pre-built Server{} via MockClient and never caught
this).
2. (high) macOS DashboardView's AttentionRow relabels the `.approve`
action's button "Review" via the shared cross-surface action-label
table, but performAction still routed `.approve` into the direct-
API-call branch, calling client.approveTools(_:) with no confirmation
— the exact one-click approve FR-014/FR-005 forbid, and a regression
from this same PR's own sibling fix for setSecret/configure/editURL
(which navigate instead of acting directly). Routed `.approve` to
navigateToServerDetail(server, tab: .tools), the existing review UI
(quarantine banner + per-tool approve rows), matching the Web UI's
and tray's identically-labeled "Review" button. Added
testAttentionRowApproveNavigatesInsteadOfOneClickApprove.
3. (high) ServerDetail.vue's healthLevelLabel (Spec 109 FR-011's status
tile) fell back to `connected ? 'Online' : 'Unknown'` when
health.status is absent (an old-core/version-skew payload), without
checking sign-in state first. For a still-connected, OAuth-expired
server (summary="Token expired", action="login", no `status` field),
this rendered "Online" directly above the sub-line's "Sign-in
required" text — one of SC-003's forbidden words for a usable=false
server. Every sibling surface (ServerCard.vue, statusBadgeText in
this same file) already checks signInState before falling back to
connected; healthLevelLabel was the one holdout. Added a regression
test to server-detail-health-admin-state.spec.ts and fixed the
fallback order to match.
4. (medium) health/calculator.go's quarantinedOAuthLoginState fell
through to the generic OAuthRequired/OAuthStatus check whenever a
quarantined server's "error"/"disconnected" state carried a LastError
that was NOT OAuth-shaped (e.g. "dial tcp: ... no route to host"),
reporting sign_in_required off a stale OAuthStatus instead of the
genuine transport fault. The non-quarantined twin of the same input
always returns before any OAuthStatus is consulted — a connection
error outranks any OAuth signal. Added
TestCalculateHealth_QuarantinedTransportFaultOutranksStaleOAuthStatus
and made the quarantined branch return early (needsLogin=false) on a
genuine non-OAuth error, letting CalculateHealth's own transport-fault
fallback (state=="error" && LastError != "") take over, same as the
non-quarantined path.
Verification: go build ./...; go test -race (internal/health,
cmd/mcpproxy-tray/..., internal/tray) all green; go test -race -tags
server -skip <CI regex> across internal/serveredition, config, oauth,
server, httpapi, storage all green (one internal/server run hit a
transient port-collision flake from other concurrent test processes on
this shared host — reran in isolation and it passed); golangci-lint v2
bare and --build-tags server both clean on ./... (pre-existing,
unrelated issues only); frontend npm run build + npx vitest run
(1363 tests) green, no package-lock.json changes; swift test on
native/macos/MCPProxy green for the touched suite (one pre-existing,
unrelated AppLifecycleTests failure tied to this dev machine's real
instance-root state, reproduces identically on files this PR does not
touch).
No REST contract fields changed (status/usable/actions already existed
in contracts.HealthStatus/oas/swagger.yaml from earlier rounds), so no
swagger/contracts regeneration was needed. No built-in MCP tool schema
changed, so no toolsurface goldens needed updating.
Fixes four verified findings from round-6 cross-model review on this
branch; the fifth is closed out with documentation plus a regression
test pinning current behavior, since its full fix needs a separate,
larger runtime-wiring change (see below).
- cmd/mcpproxy/upstream_cmd.go: `upstream list` STATUS regressed from
"Daemon not running" to the generic "Error" for every enabled server
when no daemon is reachable. runUpstreamListFromConfig overrode only
health.summary; health.status stayed "error" (CalculateHealth's
synthetic disconnected input always resolves to StatusError), and
upstreamServerRows prefers health.status over the free-text summary
whenever it is set. Now clears health.status too when overriding the
summary for an enabled server in daemon-less mode. Note: this also
means `--status <value>` no longer matches these rows at all in
daemon-less mode (previously they matched `--status error`, which
was itself a synthetic lie about a state the CLI cannot observe).
- internal/health/calculator.go: the quarantined transport-fault
upgrade only checked state=="error", never "disconnected", even
though "disconnected" is the documented, designed state a
quarantined server settles into and can carry a genuine LastError
(already proven by the round-4 regression test). Broadened the
condition to cover both states.
- internal/health/calculator.go: quarantinedOAuthLoginState's
"error"/"disconnected" case only outranked a stale OAuthStatus when
LastError was non-empty; with an empty LastError it still fell
through to the generic OAuthStatus check and returned a misleading
sign-in-required. It now always defers once in those connection
states (after the OAuth-related-error check), matching the
non-quarantined twin, which never consults OAuthStatus while in an
error/disconnected state. Narrows FR-010 for the specific case of a
quarantined server resting in its designed disconnected state with a
stale/expired OAuthStatus and no error text: it now shows
"Quarantined for review" instead of a login CTA, matching the
mirroring principle the earlier rounds established.
- internal/tray/managers.go: getServerStatusDisplay's status-text
fallback printed the raw health.level severity word ("degraded")
when both summary and status were empty, contradicting its own
adjacent comment and diverging from the Web UI's guard. Falls back
to Connected/Disconnected instead, matching
frontend/src/utils/health.ts healthStatusText.
- frontend/src/views/teams/UserServers.vue: documented that
GET /api/v1/user/servers never actually sends `connected`/`health`
(ServerResponse embeds only *config.ServerConfig plus
Ownership/UserEnabled), so the health-aware fallback logic in
healthLabel/healthBadgeClass is presently unreachable in production;
every enabled server renders "disconnected" today. Wiring real
per-user connection/health status into the server edition's
multi-user door needs its own runtime-status provider plumbed
through UserHandlers — a separate, larger change, out of scope here.
Made `connected` optional on the UserServer type (it was a type lie)
and added user-servers-real-payload-shape.spec.ts, which mounts the
component against a payload shaped exactly like the real endpoint
response and pins today's actual output, so a future wiring fix is a
deliberate, visible diff instead of silently-passing dead coverage.
Rejected: none of the five findings — all were genuine. Finding 5's
full runtime fix is deliberately deferred, as noted above.
Reviewed with zcode (GLM), full trace against every existing test in
calculator_quarantine_fault_test.go, status_test.go, calculator_test.go,
upstream_list_status_test.go, managers_test.go, and both frontend
specs: all four code fixes confirmed correct, no regressions, no new
issues. Applied two of its cosmetic suggestions (tray comment now
mentions the Connected/Disconnected fallback; UserServer.connected is
now optional).
Verified locally: go build ./...; go build -tags server;
go test -race ./internal/health/... ./internal/tray/... ./cmd/mcpproxy/...;
golangci-lint v2 (bare + --build-tags server) on the touched packages;
frontend: npm ci && npm run build && npx vitest run (140 files, 1365
tests), package-lock.json unchanged.
GET /activity/summary gains an additive per_server array covering every server with a call in the period, computed in the same counting pass as the existing totals (Spec 109 FR-013). Feeds the server-card stats line and the macOS Servers rows with one request per page load instead of a per-server query each.
Spec 109 FR-013/FR-014 (109-e): ServerCard.vue now renders one status line (label + detail), a stats line linking to Activity (server + 24h, plus status=error when there were any), and at most one primary button driven by health.actions[0] — none for the normal ready case. Every secondary action (Enable/Disable, Scan, Restart, Logs, Edit, Trust mode, Delete) moves into a single ⋯ menu; Delete is reachable only from there, with a confirmation naming the server. Trust mode renders as a shield icon with a tooltip instead of a text badge. The approve action is now a plain "Review" link to /review/<name> — it never approves from the card. This removes ServerCard's own force-approve confirmation dialog; ServerDetail.vue's Security tab already has an equivalent flow and keeps it (server-detail-approve-dialog.spec.ts). Servers.vue fetches GET /activity/summary?period=24h once per page load and passes each server's slice down as a prop, rather than the card issuing its own request. Updates several pre-Spec-109 tests whose assertions targeted UI this redesign folds away or relabels (the quarantine banner, the inline error alert, the tray-style trust text badge, the card's own approve dialog); each keeps its original regression intent against the new structure.
Spec 109 FR-013/FR-014 (109-e): TrayPrimaryPresentation.primaryItem(for:) is the one pure function that resolves a server's actions[0] (or the legacy singular action, or a synthesized "approve" for a quarantined server on an old core with no health object) to a primary item shared verbatim by the Servers-table row and the tray server submenu. login/ restart/enable run in place; every other value opens the screen that performs it (approve -> Tools tab review, set_secret/configure/edit_url -> Config tab, view_logs -> Logs) — never a one-click approve. ServersView.swift: the row's leading action-cell icon is now this primary item; the right-click context menu is built from the new pure ServerRowPresentation.contextMenuActions(for:), whose review action opens the Tools tab instead of calling apiClient.approveTools directly (there is no case in ServerRowMenuAction that means "approve directly"). MCPProxyApp.swift: the per-server tray submenu's ad hoc "needsAuth"/ "quarantined" special cases (which showed Sign-in/Review but nothing for a missing secret, a bad config or a bad URL) are replaced by the same primary-item block; showServerDetailFromMenu now forwards its representedObject (String or ServerDetailTarget) instead of only handling a bare server name, so the primary item can open a specific tab. TrayAuditMenuTests: the quarantine review row's label moves from the tray's private "Review quarantine…" wording to the one cross-surface "Review" label (HealthStatus.actionLabels).
Spec 109 T068: server cards must render at the same height whatever state they are in (FR-013), and a ready server's empty primary-action row must still reserve its height. Registered in run-web-smoke.sh's Playwright file list so the smoke gate runs it from this PR onward, and later additions to the same file (109-i) are gated too.
mergeServers() replaces existingServer.health with a fresh object on every poll (Object.assign), so the health-derived status line and primary button (now the card's central interaction, Spec 109 FR-013) could go stale behind v-memo: a health change carrying none of the already-memoized fields (e.g. actions gaining a token-expiring "login" nudge on an otherwise unchanged connected/enabled/quarantined server) would not trigger a re-render. Adds health's scalar fields to the memo key, comparing them by value rather than by the object's identity.
All six findings verified against the code before fixing; all six were
genuine.
HIGH — the card's primary "Review" action linked to `/review/<name>`,
which is not a registered route anywhere on this branch (109-a's interim
`/review` -> `?tab=tools` redirect it depended on has not landed, and
109-a is not merged to main). Clicking it 404'd. Now links straight to
`/servers/<name>?tab=tools`, needing no redirect to exist.
(ServerCard.vue primaryHref; test fixtures/assertions across
server-card-next-action.spec.ts, server-card-review-and-error.spec.ts,
server-card-quarantine-scan-headline.spec.ts updated to match.)
HIGH — a server that is both quarantined and needs OAuth sign-in reports
health.actions = ["login", "approve"] (FR-010). Every "one primary
button/item" surface reads only actions[0] ("Sign in"), silently
dropping "approve" with no path to review left:
- Web UI: the ⋯ menu had no Review entry at all in this state (only
the indirect "Details" link as a fallback). Added a Review entry
gated directly on `server.quarantined`, independent of whichever
action is primary — the same way macOS ServersView.swift's
contextMenuActions already gates independently on `quarantined`.
- macOS tray: the per-server submenu's old unconditional
`if server.quarantined { show Review }` was replaced wholesale by
the new primary-item block, reopening the exact gap the deleted
F8a fix's own comment describes. Restored an independent Review row
for `server.quarantined && !primaryOpensReview`.
Covered by a new Vue test and a new
testAQuarantinedServerThatAlsoNeedsLoginOffersBothSignInAndReview in
TrayAuditMenuTests (run via `swift test`, 16/16 pass).
MEDIUM — the trust-mode shield's FR-013 icon-only rewrite left it with
no accessible name for a screen reader in the normal case (aria-hidden
SVG, tooltip conveyed via CSS-only `data-tip`); only the invalid-value
edge case got an sr-only span. Added `role="img"` + `aria-label`
unconditionally from the same `trustBadgeTitle` text (which already
covers the invalid case), and dropped the now-redundant sr-only span.
MEDIUM — TestActivitySummaryPerServerExcludesNonCallOnlyServers's own
fixture had no server whose only activity was a non-call event, so it
could not actually catch a regression that hoisted per-server
accumulator creation out of the `if counted` gate. Added a genuine
scan-only server to the shared parity fixture and asserted its absence
from PerServer by name (not just the general "no zero-call entry"
invariant against servers that all have real calls anyway).
MEDIUM — frontend unit-test coverage gaps in
server-card-next-action.spec.ts: no fixture had actions.length > 1
(a regrown second button would have passed unnoticed); the
`actions=[], action` truthy fallback branch was untested; the
Delete-only-in-⋯-menu check used a fragile adjacent-sibling CSS
selector + exact-string match; ⋯-menu assertions checked `.exists()`
only, never rendered labels. Added fixtures/assertions for all four.
MEDIUM — the new "equal card heights" Playwright test (this PR's own
FR-013/D15 headline test) never actually ran under the release gate:
scripts/run-web-smoke.sh registers at most one fixture server, and the
test skips below two. Registered a second stdio entry (same fixture
binary, different name) so the gate exercises it from this PR onward.
Its sibling "keeps its height with no button" test also passed
vacuously — the unconditional "Details" link guarantees a non-zero
bounding box on its own — so it now also asserts the row's actual
`min-height` CSS reservation via getComputedStyle. Verified against a
real `make build`-equivalent instance: built cmd/mcpproxy +
cmd/mcpfixture, ran scripts/run-web-smoke.sh end to end with two
fixture servers registered — both navigation-consistency.spec.ts tests
pass for real (462ms / 294ms), not skipped.
Local verification: go build ./...; go test -race ./internal/httpapi/...
and -tags server with the CI -skip regex; golangci-lint v2 bare and
--build-tags server (pre-existing unrelated issues only, none in touched
files); frontend npm ci + npm run build + vitest run (140 files / 1384
tests green); swift test (1204 tests, 1 unrelated pre-existing failure —
AppLifecycleTests.testTheSharedJournalNeverWritesToTheRealInstanceRootUnderTests
fails in isolation on this machine because a real, separately-running
mcpproxy.app instance's own tray-lifecycle.jsonl already exists at
InstancePaths.root; unrelated to AppLifecycle/InstancePaths, neither of
which this change touches); real end-to-end web-ui-sweep run described
above.
All five findings verified against the code before fixing; all five were
genuine.
MEDIUM — the FR-014 primary item (tray submenu + Servers row) was laid
above the always-present Enable/Disable, Restart and View Logs rows
without ever suppressing whichever one it duplicates: a disabled server
(primary = Enable), a restart-needing one (primary = Restart) and a
token-refresh-pending one (primary = View logs) each showed the identical
command twice in the same menu/row. Added `TraySecondaryPresentation`
(Menu/TrayPresentation.swift), the one pure function both
MCPProxyApp.swift's tray submenu and ServersView.swift's row icon strip
now read to drop whichever tail row the primary already covers. Covered
by TraySecondaryActionTests (pure function) and 3 new TrayAuditMenuTests
cases asserting the real rebuilt menu shows each command exactly once.
MEDIUM — ServerCard.vue's `lastCallText` read `Date.now()` but its only
reactive Vue dependency was `activityStats.last_call_at`, so the "Xm ago"
label froze at whatever it read on first render and never advanced while
a server stayed idle. Added a `nowMs` ref ticked every 30s via
onMounted/onUnmounted, and made the computed depend on it instead of
calling Date.now() directly. New tests in server-card-next-action.spec.ts
use fake timers to prove the label updates with no prop change, and that
the interval is cleared on unmount.
MEDIUM — the equal-card-heights Playwright test (navigation-consistency
.spec.ts) could never fail: at 1440px the `lg:grid-cols-3` grid puts both
(config-identical) fixture servers in the same row, where CSS grid's
`align-items:stretch` equalizes every card in a row regardless of content
or the `.server-card{min-height}` rule the test is meant to be pinning.
scripts/run-web-smoke.sh now seeds 4 fixture servers (one quarantined) so
the fleet spans a second grid row and includes genuinely different card
content; the test now clusters cards into rows by their `y` offset and
skips loudly (naming the reason) instead of silently passing when
everything still lands in one row.
MEDIUM — scanner-gate-wording.spec.ts's no-scan-mode dialog-wording test
was deleted when ServerCard's own copy of the force-approve dialog was
removed, on the claim it is "covered (both modes)" by
server-detail-approve-dialog.spec.ts — verified false: that file had only
the findings-mode fixture. Added the no-scan-mode case there (toggling a
new `withScan` flag in the mocked server payload), asserting the shared
gate sentence appears and never mentions "dangerous finding"/"these
findings". Verified the new test actually fails if that regression is
reintroduced (temporarily reworded the shared sentence, confirmed both
assertions failed, then reverted).
MEDIUM — every "review never approves directly" guarantee was pinned only
against the pure ServerRowPresentation/TrayPrimaryPresentation data
functions, never the real `ctxOpenReview`/`primaryActionClicked` dispatch
handlers in ServersView.swift or MCPProxyApp.swift's selector wiring,
and the tray-vs-row label-parity test compared two call sites that share
one implementation, so it could not catch a rendered-string divergence.
Also, the quarantined+login dual-action fixture (actions=["login",
"approve"]) was tested only on the tray side.
- ServerRowDispatchTests: builds the real `ServerTableView.Coordinator`,
drives its real `menuNeedsUpdate`/`primaryActionClicked`, and
dispatches through `NSApplication.sendAction` exactly as a real click
would, asserting the Tools tab opens and nothing executes directly.
- ReviewNeverApprovesDirectlySourceGuardTests: a source-level regression
guard (matching the existing DashboardRoutingTests/
ServersViewRoutingTests pattern for AppKit paths this test target
can't safely drive, e.g. `showMainWindow()`'s window creation) pinning
that `showServerDetailFromMenu` and the independent quarantine-review
row never call `approveTools(`/`unquarantine(` directly.
- CrossSurfaceRenderedLabelParityTests: builds the real tray submenu
(`AppController.rebuildMenu`) and the real row actions cell
(`Coordinator.tableView(_:viewFor:row:)`) independently for all 8
`HealthStatus.actionLabels` values and compares the actual rendered
strings — not the shared pure function both call. Verified this test
fails (all 8 actions) when the row's rendered tooltip is perturbed,
then reverted.
- Added `testQuarantinedRowThatAlsoNeedsLoginOffersBothSignInAndReview`
to ServerRowActionTests, mirroring the existing tray-only fixture
against `ServerRowPresentation.contextMenuActions`.
Verification: `swift build` and `swift test` (1223 tests, 1 pre-existing
unrelated failure — AppLifecycleTests.testTheSharedJournalNeverWrites
ToTheRealInstanceRootUnderTests fails identically on a clean checkout of
this branch with none of this commit's changes, confirmed by stashing);
`go build ./...` (no Go files touched by this round); frontend
`npm run build` (vue-tsc + vite) and `npx vitest run` (140 files, 1387
tests, all passing); `frontend/package-lock.json` untouched by `npm ci`
so nothing to revert. golangci-lint and `go test -race` were not re-run
since no Go package was touched this round. The Playwright
navigation-consistency.spec.ts change was verified by code review only —
running it needs a built mcpproxy binary + Playwright browsers, outside
this round's local verification.
Deploying mcpproxy-docs with
|
| Latest commit: |
cda1e22
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6f347b93.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://109-e-server-card-next-actio.mcpproxy-docs.pages.dev |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 36388710079 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Base PRs #1376 (109-c health vocabulary) and #1366 (login on quarantined OAuth servers) are squash-merged into main; this branch's base is retargeted from 109-c-health-vocabulary to main and main is merged in. Conflict resolution preserves both sides' intent: - internal/health/calculator.go: kept main's quarantinedAwaitingSignIn/ oauthActionApplies generalization (#1366) over this branch's narrower OAuthRequired-only check, so a quarantined server needing OAuth sign-in reports actions=[login, approve] with login as health.action. - ServerCard.vue / macOS ServersView.swift, MCPProxyApp.swift, DashboardView.swift: kept this branch's Spec 109-e single-primary-action redesign, which already reads health.actions[0] and independently gates the quarantined Review path in the ⋯ menu / tray submenu — the #1366 login behaviour surfaces correctly as the ONE primary action for that state without reintroducing separate Approve/Login buttons. - frontend/src/utils/health.ts, AdminServers.vue, UserServers.vue: kept main's healthStatusTextOrEmpty() consolidation and the fixed quarantined/disabled fallback ordering (FR-011 round finding). - Rewrote server-card-quarantined-oauth-login.spec.ts and server-card-approve-review-navigation.spec.ts (both added by main, testing the pre-109-e per-action button design) against the merged single-primary-action design; fixtures now match calculator.go's actual actions=[login, approve] output. - Added a race guard to ServerCard's primaryAction: disableServer()'s optimistic update flips `enabled` immediately but `health` only refreshes on the next SSE payload, so a stale 'login' action is forced to 'enable' when `enabled` is false — restores a guarantee the pre-merge showLogin computed had that the unified primaryAction had dropped. - Regenerated oas/swagger.yaml, oas/docs.go, and frontend/src/types/ contracts.ts (no net diff — contracts unchanged by this merge). Verification: go build ./... (bare + -tags server); go test -race on internal/health, cmd/mcpproxy; go test -race -tags server (CI -skip regex) on internal/serveredition, config, oauth, server, httpapi, storage; golangci-lint v2 (bare + --build-tags server, pre-existing canaries only); frontend npm run build (vue-tsc + vite) and npx vitest run (168 files, 1487 tests); macOS swift test (1259 tests, 1 pre-existing unrelated failure, confirmed on a clean checkout without this branch's changes).
Three medium findings from the zcode/GLM review round: F-FR014-focus: FR-014 requires configure/edit_url's primary action to open the Configuration tab "with the field focused". Neither the tray submenu nor the Servers row did this for edit_url, unlike the Web UI's existing `&focus=endpoint` behavior (ServerDetail.vue). Adds TrayConfigFocusField/TrayPrimaryItem.focusField (nil for every action except edit_url, matching the Web UI, which also has no single field for `configure`'s heterogeneous causes), threads it through ServerDetailTarget and the Servers-row/tray-submenu dispatch paths into ServerDetailView, which now enters edit mode and focuses the URL field via @focusstate. Also fixes the same gap in DashboardView's AttentionRow, which has an independent edit_url->Config route. F4.1: ReviewNeverApprovesDirectlySourceGuardTests' forbidden-substring list only checked "approveTools(" / "unquarantine(", which are not substrings of the actual APIClient methods (approveSpecificTools(_:tools:), unquarantineServer(_:)) — a rewiring to either real method would have passed silently. Both real spellings are now checked alongside the old ones. F4.2: ServerRowDispatchTests built its coordinator with apiClient left nil, so a regression that called apiClient?.approveTools/ unquarantineServer directly would no-op on the nil optional and every assertion would still pass. The two "never approves directly" tests now wire a real APIClient backed by GlanceStubURLProtocol and assert no approve/unquarantine request was recorded (with a short async wait so a Task-wrapped regression call has time to fire before the check). Verified: swift build clean; swift test — 1258/1259 pass, the one failure (AppLifecycleTests.testTheSharedJournalNeverWritesToTheRealInstanceRootUnderTests) is a pre-existing environment-dependent failure unrelated to this diff (fails in isolation on a clean checkout of this branch, touches no file changed here). go build ./... unaffected (no .go files touched).
…t-action # Conflicts: # frontend/src/views/Servers.vue # oas/docs.go
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This was referenced Sep 28, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Sep 28, 2026
…e search (Spec 108-b) (#1390) ## Summary Implements profile-aware discovery for MCP and REST surfaces. Search now filters excluded tools before applying result limits and reports accurate hidden-by-profile counts; describe_tool and direct list routes hide excluded tools uniformly. Cache reuse is keyed by the profile policy so a policy change cannot reuse an admitted result from the prior policy. The fail-closed rollout gate remains active until execution enforcement lands in Spec 108-d. ## Spec and findings - Spec: `specs/108-profiles-v3/` — FR-009a, FR-010–012, FR-015 and FR-015a. - UX/security finding: P3 profile discovery enforcement. - Frozen golden: `internal/server/testdata/retrieve_tools_profile_v3.golden.json`. ## Verification - `go build ./...` - Race tests passed for `internal/cache`, `internal/index`, `internal/runtime/supervisor`, and `internal/server` with the server build tag and repository skip regex. - Both golangci-lint full passes were run; the local Go 1.26 tool reports existing diagnostics outside this PR. Both editions report 0 new diagnostics with `--new-from-rev=origin/main`. GitHub CI is running on the re-synced head. - Live isolated config check on port 18791 with scratch HOME/config/data and telemetry disabled: a real binary rejects a profile with `max_tier: read`, exits 4, and prints `max_tier is not supported by this build (Profiles v3 enforcement incomplete)`, as required by FR-009a. Discovery-specific enforcement is covered by the PR's profile test suite; a live profile-policy session is enabled in 108-d when every execution path is enforced. - ZCode GLM-5.3: implementation review found and verified fixes for the discovery gaps; merge-resolution incremental review returned NO FINDINGS. ## Re-sync Clean automatic merge of current main after PR #1381; no manual conflict resolution.
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
Aligns Web server cards, macOS server rows and tray server actions around one health label and one primary action, while keeping secondary controls in menus. Activity links carry the server and time filters. Home attention actions for endpoint configuration open the Configuration tab with the endpoint field focused.
The live sweep verifies the catalog-first Add Server route and keeps browser-level focus, Tab-trap and Escape coverage on the active Add Secret dialog.
Spec and findings
oas/swagger.yamlandoas/docs.goupdated for the additive per-server activity summary.Verification
go build ./...env HOME=/private/tmp/ux-1381-home.nRBGml MCPPROXY_TELEMETRY=false MCPPROXY_BINARY_PATH=/private/tmp/ux-handoff/1381-build/mcpproxy MCPPROXY_FIXTURE_PATH=/private/tmp/ux-handoff/1381-build/mcpfixture MCPPROXY_BASE_URL=http://127.0.0.1:18763 ARTIFACT_DIR=/private/tmp/ux-handoff/1381-qa-modal ./scripts/run-web-smoke.sh. Result: 32 passed, 2 skipped; Home, Servers, Tools, Activity, Settings, card heights/actions, catalog-first Add Server and Add Secret modal accessibility passed.