feat(attention): one needs-attention list for every surface (Spec 109-d) - #1382
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.
…FR-002) internal/runtime.Compute derives sign_in_required, missing_secret, config_error, server_error, server_review, tool_review and client_never_seen items from in-memory server/client state - one function every surface reads from. A debounced subscriber recomputes on servers.changed and arms a threshold timer for the earliest pending time-based crossing (connecting/error at 60s, client unseen at 5m), publishing attention.changed only when the item id set changes. Wire types (AttentionItem/AttentionSubject/AttentionFix) live in internal/contracts, mirroring contracts.HealthStatus beside internal/health.CalculateHealth.
GET /attention (both editions): admins see every item, a scoped caller
(agent token, non-admin server-edition user session) sees only items
whose server passes CanEnumerateServer and never a client item -
branches on auth.IsScopedCaller, never Type == AuthTypeAgent (FR-007).
attention.changed is classified alongside servers.changed as
per-subscriber-rendered (never dropped by eventVisibleToCaller);
renderAttentionChangedForCaller narrows {count, ids} the same way, and
handleSSEEvents suppresses a frame whose narrowed id set is unchanged
since the last one that subscriber received (FR-006).
oas/swagger.yaml + oas/docs.go via 'make swagger'; frontend/src/types gains Attention* interfaces and kind constants generated from internal/contracts/attention.go.
…-003/FR-004) 'mcpproxy attention' prints the #/KIND/SUBJECT/SUMMARY/FIX table or 'All clear'; always exits 0 (a report, not a check). 'status' gains a fixed first line, 'Needs attention: N (run mcpproxy attention)' or 'none'. 'doctor' prints the same list as its own first section, and its diagnostics count is now 'Diagnostics: N findings' - 'need attention' refers only to the FR-001 count on every surface.
… (Spec 109 FR-051) Dashboard.vue is retired: Home.vue (renamed) drops the local serversNeedingAttention/loadPendingTools predicates for the shared AttentionList component, backed by stores/attention.ts (GET /attention + live SSE attention.changed). A UsageSummaryStrip sits below the topology normally and moves above it when the list is empty. Routing: '/' -> Home, '/usage' -> Usage.vue as its own page (no longer a Dashboard panel), '/overview' redirects to '/'. TopHeader gets a needs-attention pill (hidden at 0, popover with the first 5 + See all); SidebarNav's Dashboard entry is renamed Home and carries the same count as a badge.
…R-005) APIClient.attention() (GET /api/v1/attention) and AppState.attention, kept live over SSE attention.changed (CoreProcessManager refetches on the event, same as servers.changed's fallback path). The tray's 'Needs Attention' group and Home's attention section are both built from this one list. DashboardView.swift is renamed HomeView.swift (Dashboard -> Home); its fix dispatch is extracted into HomeAttentionAction, a plain testable model. The one-click approveTools call the old AttentionRow ran for a quarantine review action is removed - review now only navigates to the server's detail view, mirroring the tray's own rule that quarantine review is never a one-click approve from a list.
Kinds, ranks and fixes; where each surface reads it; why review fixes never call approve/unquarantine directly.
Fixes five verified findings from the round-1 review of the needs-attention feature (109-d-needs-attention): - cmd/mcpproxy/doctor_cmd.go: doctor's "All systems operational" verdict was gated only on the diagnostics `total_issues` counter, so a quarantined server (or any other FR-001 attention item) with no other diagnostics finding made doctor print the attention section followed immediately by a contradicting all-clear banner, and skipped the "Diagnostics: N findings" section via the early return. Now gated on both counters; the Diagnostics section always prints. - internal/runtime/attention.go: every fix.target was built with fmt.Sprintf on the raw, unescaped server/client name. An official-registry name containing '/' (e.g. "io.github.owner/repo", MCP-1112/#598 — the same bug class already fixed on the frontend in serverRoute.ts) split the path and 404'd on the catch-all route instead of opening the exact fix screen (FR-005). Names are now percent-encoded (url.PathEscape for path segments, url.QueryEscape for the /clients?focus= query value). - native/macos/.../HomeAttentionAction.swift: the `reload_hint` verb ("How to restart") was a true no-op (`case "reload_hint": break`), while Home renders a prominent button for it — clicking gave no navigation, no alert, no feedback, violating the "never a dead link" rule the `review` verb already follows via its interim navigation. Added AppState.pendingReloadHint, set by performFix and surfaced as an alert in HomeView with the item's own restart guidance, until 109-h ships the real /clients?focus=<id> screen. - frontend/src/stores/attention.ts: fetchAttention had no sequence/ticket guard, unlike stores/servers.ts's issueSeq/appliedSeq precedent, so three independent onMounted callers (SidebarNav, TopHeader, AttentionList) plus the SSE-triggered silent refetch could race and let a stale response overwrite a newer one. Added the same ticket-guard pattern. - frontend/src/stores/system.ts: nothing resynced attention state after an SSE reconnect, so a threshold-crossing event (server_error, client_never_seen, ...) missed during a drop was lost until an unrelated change happened to fire a fresh one. es.onopen now re-dispatches the same mcpproxy:attention-changed window event the live handler dispatches, on both the initial connect and every retry. Also closes the T057 test-coverage gaps a sixth finding raised: added SSE live-update coverage for Home/header pill/sidebar badge (previously every EventSource mock was inert), a server_review/tool_review fix-link assertion (previously only `login` was checked), and an item ordering fixture that is neither rank-sorted nor alphabetical (the previous fixture used rank 10 for every item with names already alphabetized, so it passed under any order-preserving or accidentally-correct sort). Rejected: the UsageSummaryStrip finding (calls-today/blocked/errors tiles linking to /activity?view=calls&from=-24h[&status=...], which Activity.vue does not yet parse). Verified against tasks.md: 109-d's own dependency graph requires 109-k (activity-scope-filters) to merge first ("109-k --> 109-d"), and 109-k's useScopeQuery is what teaches Activity.vue to read these params. This is the documented intermediate state of a stacked branch, not a defect introduced by 109-d, and 109-k is already being handled in its own branch/session. Verification: go build ./...; go test -race on cmd/mcpproxy + internal/runtime/... (all pass); golangci-lint v2 (bare and --build-tags server) on the full repo — 16/19 pre-existing issues respectively, none in touched files; frontend npm run build + npx vitest run — 141/141 files, 1351/1351 tests pass; swift test — 1 pre-existing environmental failure (AppLifecycleTests, unrelated to any touched file, reproduced in isolation against unmodified state: the shared machine's real ~/.mcpproxy/tray-lifecycle.jsonl already exists from a concurrent session), 1193/1194 otherwise pass.
Verified finding: Spec 109 task T059 (SC-011 benchmark, internal/runtime/attention_bench_test.go, 100 servers / 1,000 tools, p95 <= 20ms for the GET /attention path) had no corresponding test anywhere in the repo, unlike T062/T064 which explicitly defer to a named sibling PR. Genuine gap: nothing guarded Compute against an O(servers*tools) regression at scale. Added internal/runtime/attention_bench_test.go: - buildAttentionBenchFleet: 100 servers / 1,000 tools fixture spanning every health/quarantine branch Compute has (sign_in_required, quarantined, tool_review pending+changed, error, needs_secret, ready), plus 50 never-seen clients. - TestComputeSC011LargeFleetP95: runs Compute 200 times over the fixture and asserts p95 <= 20ms (measured ~62us/op, ~300x headroom in this environment) — a plain `go test`, not only `-bench`, so it runs in every CI lane. - BenchmarkComputeSC011LargeFleet: testing.B companion for -bench/-benchmem profiling. Removed the old BenchmarkAttentionComputeLargeFleet from attention_input_test.go: it covered a similar 100-server shape but with no scale-to-1,000-tools fixture and no timing assertion, so it did not satisfy T059 and duplicated the new dedicated file once added. Local verification: go build ./..., go build -tags server ./..., go test -race ./internal/runtime/... (full package, cached subpackages included), golangci-lint v2 (.github/.golangci.yml) bare and with --build-tags server on internal/runtime and on the whole repo — the only findings on both runs are pre-existing (internal/runtime/event_bus.go govet:inline, plus 15 unrelated repo-wide staticcheck/govet issues), none touching the changed files. No frontend or native macOS files changed, so those verification steps do not apply.
Deploying mcpproxy-docs with
|
| Latest commit: |
494233c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://17f10ed9.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://109-d-needs-attention.mcpproxy-docs.pages.dev |
TestMutatingServerRoutes_AdminAllowed panicked (nil embedded management.Service) because mockManagementService never overrode RestartAll/EnableAll/DisableAll added for the /servers/*_all routes, so calls through the mock fell through to the nil interface. TestNoDoorPublishesARawServerLeaf flagged computeServerItems as a new raw-leaf door for AttentionItem.Detail. The value is attentionServerDetail's own output (transport + URL host only, per data-model.md §4) rather than operator config, so it is recorded in rawServerLeafDoors with that reason, matching the existing "not operator-supplied" exemptions. Fixes the Build Binaries / Unit Tests / Server Edition failures on PR #1382 (Spec 109-d).
|
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 36382405870 --repo smart-mcp-proxy/mcpproxy-go
|
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.
…F6,F7)
F1 (medium): attentionSubscriber.state was never pruned when a server
disappeared from servers.changed. A re-added server with the same name
inherited the deleted server's stale {status, since}, so a fast-failing
respawn could report server_error instantly with a wrong Since,
bypassing FR-002's 60s threshold. Prune state to the current server set
on every recompute. Test: TestAttentionSubscriberPrunesStateOnServerRemoval.
F2 (medium): a transient GET /attention failure left
attentionStore.loaded false forever on the Web UI — Home's 30s
auto-refresh interval never re-fetched attention, so the list, header
pill and sidebar badge stayed blank until an unrelated SSE event or a
route remount. Added a silent attentionStore.fetchAttention(true) to
the interval. Test: recovers from a failed initial fetch on the next
30s auto-refresh tick (home-attention.spec.ts).
F3 (medium): this PR removes the Dashboard Usage/Overview switcher
without leaving any in-app link to /usage; the Monitor/Usage sidebar
entry is owned by a later PR (109-i, FR-050). Added a "View usage"
link to the usage summary strip Home already renders, keeping /usage
reachable in the interim without touching sidebar work 109-i owns.
Test: usage-summary-strip-link.spec.ts.
F5 (medium): `mcpproxy attention -o <bad-format>` silently exited 0
with "All clear" whenever the daemon had 0 items, because the
All-clear early return ran before the formatter (and its format
validation) was constructed — the identical typo correctly failed
against a non-empty list. Construct/validate the formatter
unconditionally before either output path. Test:
TestAttentionCmd_InvalidFormatRejectedEvenWhenAllClear.
F6 (medium): T060's documented test-only env hook
MCPPROXY_ATTENTION_NEVER_SEEN_AFTER (used by quickstart.md's
live-verification recipe for the 5-minute client_never_seen
threshold) was never read anywhere. Wired it into
newAttentionSubscriber (parsed, ignored if unparseable) and documented
it in docs/features/needs-attention.md. Test:
TestNewAttentionSubscriberHonorsNeverSeenAfterEnvHook.
F7 (medium, coverage gap): SC-011 ("GET /api/v1/attention p95 <= 20ms")
had no test timing the actual HTTP route -- internal/runtime's bench
only times Compute() building the snapshot. Added
TestGetAttentionHandlerSC011P95 in internal/httpapi, timing
handleGetAttention itself (scope filtering + JSON encoding) over the
same 100-server/1,000-tool/50-client fixture shape.
F4 (medium) deferred: macOS CoreProcessManager.refreshAttention()
swallows a GET /attention error (incl. 404 against a pre-Spec-109
core) with no fallback, so a version-skewed tray shows a permanent
false all-clear. The pre-109 fallback (deriving attention from
GET /servers) was FR-003's serversNeedingAttention, retired by this
spec specifically so no surface re-derives its own predicate; reintroducing
it would conflict with that invariant. A correct fix needs a
distinguishable "attention unknown" state threaded through AppState
and the tray/Home UI (not just a swallowed catch), which is a genuine
UI/state design change, not a small one -- left for a follow-up rather
than rushed here. The window is narrow in practice (self-update
version-skew), matching the finding's own framing.
Verification: go build ./... and -tags server both clean; go vet
clean; golangci-lint v2 (.github/.golangci.yml) bare and
--build-tags server show only 2 pre-existing, untouched-file issues
(event_bus.go govet inline, contracts_test.go staticcheck deprecation)
matching a known CI canary, no new issues; go test -race on
internal/runtime, internal/httpapi, cmd/mcpproxy all green; go test
-race -tags server across serveredition/config/oauth/server/httpapi/storage
(CLAUDE.md -skip regex) green; frontend npm ci + vitest run (169 files,
1450 tests) + npm run build (vue-tsc + vite) all green,
package-lock.json unchanged; swift test in native/macos/MCPProxy: 1
pre-existing failure (AppLifecycleTests.testTheSharedJournalNeverWritesToTheRealInstanceRootUnderTests)
unrelated to this diff -- no Swift files are touched here, and the
test depends on this machine's real ~/.mcpproxy instance root having
no tray-lifecycle.jsonl, which a live install writes independently of
any test run.
# Conflicts: # cmd/mcpproxy/status_cmd.go # frontend/src/services/api.ts # oas/docs.go
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 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.
Keeps Needs attention consistent across REST/SSE, Web Home and navigation, the macOS tray and Home view, and CLI status. This resync also closes tenant-session access, login retry, stale macOS state, and tray action-target gaps found during review.
Fixes Spec 109 findings A3, N6 and N8. Scope and ownership: Spec 109.
Local verification: Go build and runtime/HTTP race tests; server-tagged race suites; frontend build and 201 Vitest files (1,689 tests); Swift suite (1,312 tests with an isolated
MCPPROXY_HOME).