Skip to content

Follow-ups from Spec 109-c health vocabulary review (#1376) #1392

Description

@Dumbris

Follow-up findings from the zcode (GLM-5.3) review of #1376 that were below the merge bar (no critical/high). Each was verified against the code; fix in small PRs.

  • low — native/macos/MCPProxy/MCPProxy/API/Models.swift:218-240; native/macos/MCPProxy/MCPProxyTests/HealthVocabularyTests.swift:108-190: native/macos/MCPProxy/MCPProxy/API/Models.swift's new HealthStatus.statusLabels/actionLabels dictionaries are hand-duplicated from internal/health/constants.go with no test tying them together. The Go->TypeScript side is guarded (cmd/generate-types/main_test.go's new TestHealthVocabularyMatchesConstants), but nothing Swift-side reads constants.go, contracts.ts, or a shared fixture — HealthVocabularyTests.swift only asserts the dictionaries against literals hardcoded in the same test file. A future label rename in constants.go (caught immediately by the Go/TS test) would silently leave macOS showing the old wording, with the whole suite green. This is the golden-fixture (status_fixtures.json / T001) enforcement contracts/health-vocabulary.md and spec.md FR-090 call for, explicitly deferred to a later PR (109-m) per plan.md — a known, tracked sequencing gap rather than a 109-c-introduced regression.
  • low — cmd/generate-types/main_test.go:79-113: cmd/generate-types/main_test.go's new TestHealthVocabularyMatchesConstants only checks that each status/action/label literal appears SOMEWHERE in the ~700-line generated TypeScript string (strings.Contains), never that it appears in the right place or bound to the right key. Several literals ('ready', 'error', 'disabled', 'login', 'restart', 'enable') already exist elsewhere in the same generated file for unrelated enums (PreflightStatusReady, ActivityStatusError, AdminStateDisabled, ServerAction), so the test can pass even if the health-specific constant/label is deleted or two labels are swapped in HEALTH_STATUS_LABELS.
  • low — internal/health/calculator.go:826-830 (call site at :579): internal/health/calculator.go: quarantinedAwaitingSignIn's own 'pending auth'/'pending_auth' → true check (lines ~827-829) is unreachable dead code after this PR's refactor. Its only call site (quarantinedOAuthLoginState, line 579) is inside a case "error", "disconnected": branch of a switch that already handles 'pending auth'/'pending_auth' in its own preceding case, so state is provably never 'pending auth' at the call site. Harmless (returns true only for states the caller already excludes) but the function's doc comment ('parked in Pending Auth' as one of its jobs) is now stale, which could mislead a future refactor into calling it directly expecting pending-auth handling.
  • low — internal/contracts/converters.go:236-475; internal/httpapi/server.go:1857-1867: Pre-existing (not introduced by this PR): internal/contracts/converters.go's ConvertGenericServersToTyped, used by GET /api/v1/servers' legacy fallback path when the management service is nil, has no health key extraction at all — the string 'health' does not appear in the file. On that code path (internal/httpapi/server.go's 'Fallback to legacy path' branch, ~line 1857), the entire health object — including this PR's new status/usable/actions as well as the pre-existing level/admin_state/summary/action — is silently dropped from the REST response. This predates 109-c and isn't a regression, but it does mean the PR's cross-surface parity claim ('every surface renders status') is not literally true on this specific, currently-untested wire path.
  • low — native/macos/MCPProxy/MCPProxyTests/ServersViewRoutingTests.swift:16-44: The new regression test ServersViewRoutingTests.swift's testShowServerDetailObserverStaysLiveWhileADetailViewIsOpen (as its own doc comment admits) is a source-text/regex assertion -- it checks that the two .onReceive lines appear as substrings between the string var body: some View { and private var serverListView in the raw file text, not a rendered SwiftUI behavioral test (no ViewInspector/snapshot harness in this package). It cannot detect the still-open failure mode A (first-mount race, MainWindow.swift instantiation timing), and could pass even if the modifiers were moved to some other always-mounted-but-wrong node (e.g. attached to .sheet content), or stop protecting anything if an .id(...) modifier were later added to the VStack (forcing SwiftUI to tear down and recreate state/subscriptions each render).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    kind/bugSomething isn't workingpriority/lowNice to have; address when bandwidth allowstriage/acceptedTriaged and accepted for the backlog

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions