feat(health): status vocabulary across surfaces (Spec 109 FR-010-012) - #1376
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.
Deploying mcpproxy-docs with
|
| Latest commit: |
ca7d308
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://76e66629.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://109-c-health-vocabulary.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 36249838359 --repo smart-mcp-proxy/mcpproxy-go
|
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.
This was referenced Sep 26, 2026
# Conflicts: # native/macos/MCPProxy/MCPProxy/Views/DashboardView.swift # oas/docs.go
…d panic TestMutatingServerRoutes_AdminAllowed called these through mockManagementService's embedded nil management.Service, panicking on every run. chi's Recoverer turned that into a 500 the test's assertions don't check for, so it always passed -- but on windows/amd64 the recovered hardware fault corrupts the Go heap under Go 1.26 (golang/go#81238), crashing an unrelated package's tests later in the same CI run (seen here as internal/upstream/managed failing with no failing subtest of its own). Same fix pattern as the existing handleAddFromRegistry nil-guard: give the mock real, non-panicking implementations.
# Conflicts: # internal/health/calculator.go
# Conflicts: # oas/docs.go
AdminServers.vue and UserServers.vue each carried their own inline statusLabel()/healthLabel() that resolved health.status BEFORE health.summary, the reverse of the order ServerCard.vue/ServerDetail.vue use via healthStatusText() (summary first, then the shared status label). The same health payload therefore rendered different text depending on which surface showed it. Add healthStatusTextOrEmpty() to @/utils/health as the one shared precedence resolver (summary || label(status) || ''), and have all four surfaces resolve through it; each surface keeps its own final connected/disconnected/admin-state fallback wording, which is a per-surface convention (e.g. lowercase 'connected' on the tables vs capitalized 'Connected' on the card), not part of the shared precedence. Test-first: frontend/tests/unit/status-text-precedence-consistency.spec.ts reproduces the mismatch with a payload where status and summary disagree, failing against the old per-surface implementations before this change.
zcode review findings (medium severity, all fixed): 1. cmd/mcpproxy/upstream_cmd.go: the daemon-less `upstream list` path (runUpstreamListFromConfig) clears health.status to "" for every enabled, non-quarantined server so the STATUS column keeps the informative "Daemon not running" summary instead of a bogus "Error" (round-6 fix). But filterServersByStatus matches health.status exactly, so `--status ready|connecting|sign_in_required|needs_secret|needs_config|error` silently returned an empty (headers-only, exit 0) table whenever the daemon was down — the GH #938 "silent empty result" failure mode validateStatusFlag's own doc comment exists to prevent. Only "disabled" and "needs_review" are classifiable without a daemon (they short-circuit CalculateHealth before the synthetic "disconnected" state matters), so the fix is a stderr notice — not a fabricated status — that fires only when the filter contains a value this path cannot resolve on its own. Test: TestRunUpstreamListFromConfig_UnresolvableStatusFilterWarnsInsteadOfSilentlyEmpty. 2. frontend/src/views/teams/AdminServers.vue: statusLabel()'s quarantined/disabled fallback checks were chained as `else if` onto `if (server.health)`, so they only ran when `server.health` was absent entirely. A server with a health object but empty summary/status (an old-core / version-skew payload) fell straight through to the bare connected/disconnected fallback, skipping quarantined -> "Needs review" and disabled -> "Disabled" — diverging from UserServers.vue's healthLabel(), which re-checks `enabled` unconditionally. Both checks now run unconditionally, matching statusBadge() above and UserServers.vue. Tests added to admin-servers-status-fallback.spec.ts. 3. native/macos/MCPProxy/.../ServersView.swift: `.onReceive(.showServerDetail)` and `.onReceive(.showAddServer)` were attached to `serverListView`, a computed property `body` only includes while no server detail is shown. Opening a server's detail (e.g. via DashboardView's AttentionRow, which posts `.switchToServers` then `.showServerDetail` 0.3s later) swaps `serverListView` out of the tree and detaches the observer, so a second `.showServerDetail` notification arriving while a detail view is already open (two rapid attention-row clicks, or any navigation to a second server without dismissing the first) is silently dropped. Both handlers now live on `body`'s own always-mounted VStack. Existing tests in this file are source-level regression guards (no ViewInspector harness in this package); added testShowServerDetailObserverStaysLiveWhileADetailViewIsOpen in the same style. Verification: go build ./...; go test -race ./cmd/mcpproxy/...; golangci-lint v2 (bare and --build-tags server) clean on changed packages, pre-existing unrelated issues elsewhere; frontend npm run build + npx vitest run (1459 tests) all green; swift test — 1217/1218 pass, the one failure (AppLifecycleTests.testTheSharedJournalNeverWritesToTheRealInstanceRootUnderTests) is pre-existing environment state (this machine's real ~/.mcpproxy/tray-lifecycle.jsonl already exists from prior real usage) unrelated to this change or this PR.
Live QA on a quarantined server with a completed clean security scan: the
card's primary action button, labeled "Approve", called
doSecurityApprove(false) straight from handleApproveClick with no
confirmation or navigation, unquarantining and enabling the server on a
single click. specs/109-ux-navigation-consistency/contracts/health-vocabulary.md
requires the 'approve' action to open the review screen and never approve
directly (FR-005), and the shared HEALTH_ACTION_LABELS table already renders
it as "Review" everywhere else (frontend/tests/unit/health-status-labels.spec.ts) —
ServerCard's button template just never called it. The macOS dashboard
already gets this right by navigating to the server's review tab instead of
approving.
Turn the button into a router-link to the server's Security tab (labeled via
healthActionLabel('approve')) and drop the approve confirmation modal and its
handlers from ServerCard — that flow already exists, unchanged, on the
review screen in ServerDetail.vue.
Add server-card-approve-review-navigation.spec.ts, which fails against the
prior code (reproducing the live-QA repro: a direct securityApproveServer
call with no confirmation) and passes against the fix. Retarget
scanner-gate-wording.spec.ts's two dialog-wording assertions, which tested a
dialog that no longer renders on the card; that wording is already covered
on the review screen by server-detail-approve-dialog.spec.ts.
# Conflicts: # oas/docs.go
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Dumbris
added a commit
that referenced
this pull request
Sep 26, 2026
109-c (health vocabulary, #1376) is squash-merged into main, so this PR now targets main directly. Resolves conflicts across the health calculator, tray/Home Swift views (DashboardView.swift retired in favor of HomeView.swift), and the frontend health-status helpers so Spec 109-d's one-list attention design (AttentionItem, not ServerStatus-derived predicates) is what survives the merge. Regenerates oas/swagger.yaml, oas/docs.go and frontend/src/types/contracts.ts for the merged REST contracts.
Dumbris
added a commit
that referenced
this pull request
Sep 26, 2026
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).
5 tasks
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
Adds a single status vocabulary for server health (
status,usable,actions) alongside the existing severitylevel, and moves every surfaceoff rendering
levelas text.internal/health:CalculateHealthnow populatesstatus(one ofready, connecting, sign_in_required, needs_review, needs_secret, needs_config, error, disabled),usable(true only whenstatus == "ready"), andactions(ordered next-step list, withaction == actions[0]). Quarantined + OAuth-needed servers now reportaction: "login"instead of"approve", matching the existing Sign-in CTAbehavior.
frontend/src/utils/health.ts,ServerCard.vue,ServerDetail.vue,AdminServers.vue,UserServers.vue): renders the newstatuslabel through one shared table;levelis used only forbadge/tray severity coloring, never as text.
cmd/mcpproxy upstream list): STATUS/ACTION columns now come from thestatus vocabulary, plus a new
--statusfilter.native/macos/MCPProxy): decodes the new fields, stopsrendering
levelas text inDashboardView/ServersView/ServerDetailView.docs/api/rest-api.md,oas/swagger.yaml,frontend/src/types/contracts.ts,internal/contracts/types.go):document/regenerate the new
status/usable/actionsfields on thehealth object.
internal/oauth/serverfields.go: adds mask decisions for the two newhealth fields so the field-coverage tests stay exhaustive.
Audit findings fixed
Implements Spec 109 FR-010, FR-011 and FR-012 from
specs/109-ux-navigation-consistency/spec.md:status/usable/actionstocontracts.HealthStatus, withaction == actions[0](or"").levelas text; every surface rendersstatusthrough one label table.actionslists every applicable next step in priority order(
login > set_secret > configure > edit_url > approve > restart > view_logs > enable).This also resolves the audit's X5 finding (a quarantined server reporting
level: healthywhile unusable) via theusableflag and the FR-010/011display rule.
Spec
specs/109-ux-navigation-consistency/spec.md(FR-010, FR-011, FR-012).Tests added
internal/health/status_test.go: table-driven coverage of every statusbranch (
TestCalculateHealth_StatusVocabulary), theaction == actions[0]invariant, quarantined+OAuth action ordering, and status/actionlabel completeness.
internal/httpapi/get_servers_health_vocabulary_test.go:GET /api/v1/serverscarriesstatus/usable/actions, including themulti-action case.
internal/server/upstream_servers_health_status_test.go: same coveragethrough the
upstream_serversMCP tool.cmd/mcpproxy/upstream_list_status_test.go,upstream_actions_type_test.go,exit_codes_test.go: CLI STATUS/ACTIONcolumns and the new
--statusfilter.internal/tray/managers_test.go: tray-side health decoding.frontend/tests/unit/):health-status-labels.spec.ts,admin-servers-status-fallback.spec.ts,server-detail-config-health-old-core-fallback.spec.ts,server-detail-health-admin-state.spec.ts,user-servers-status-fallback.spec.ts.native/macos/MCPProxy/MCPProxyTests/):HealthVocabularyTests.swift,DashboardRoutingTests.swift,ServersViewRoutingTests.swift, plusModelsTests.swiftadditions.Verification
go build ./...andgo build -tags server -o /dev/null ./cmd/mcpproxyboth succeed.
golangci-lint run --config .github/.golangci.ymlover the touchedpackages reports only pre-existing issues in files this branch does not
touch (oauth/tray canaries already tracked separately).
-race:internal/health,internal/httpapi(TestHandleGetServers_HealthCarries*),internal/server(TestUpstreamServersList_HealthCarriesStatusVocabulary),internal/tray,internal/contracts,internal/oauth(
TestServerFieldMaskDecisions_*), andcmd/mcpproxy.npx vue-tsc --noEmitis clean and all 5 new/updated Vitestspecs (26 tests) pass.
swift buildsucceeds; the touched suites(
ModelsTests,ServersViewRoutingTests, and the newHealthVocabularyTests/DashboardRoutingTests) pass. The one failure in afull
swift testrun (AppLifecycleTests.testTheSharedJournalNeverWritesToTheRealInstanceRootUnderTests)is a pre-existing environmental flake on this shared dev machine — it
fails because a real
~/.mcpproxy/tray-lifecycle.jsonlalready existsfrom an unrelated live tray install, not from anything this branch
touches (no file under
AppLifecycle*is part of this diff).one status vocabulary:
docs/api/rest-api.mdanddocs/cli-management-commands.mdwere updated in the same PR so thedocumented contract matches all four surfaces.