feat(catalog): catalog-first Add Server across all surfaces (Spec 109-j) - #1383
Merged
Merged
Conversation
Every <dialog class="modal"> now opens via showModal()/close() through a shared useDialogOpen composable instead of the `open` attribute or a `modal-open` class toggle, so a modal can never be painted over by the sidebar or header regardless of z-index (the Add Server modal bug, H4). Adds frontend/src/assets/z-index.css as the single source for the sidebar < header < dropdown < modal < toast scale and retires the ad-hoc z-40 on the sidebar's drawer-side. Also folds in two small header changes that touch the same files: Add-to-MCPProxy button state and header layout groundwork.
Cross-surface UX fixes with no dependency on later PRs in the spec:
- `/` lands on the Overview panel (interim step before Home in a later
PR); `/usage` and `/overview` stay deep-linkable (FR-051).
- Server detail and Settings tabs read `?tab=` on mount and write it
back with router.replace on change, keeping other query params
(FR-016).
- Settings is named "Settings" everywhere (sidebar, route title, H1,
document title), its tabs use line icons instead of emoji, and the
"Server Edition" tab is gated on the runtime edition
(systemStore.status.edition, now carried on every SSE status frame)
rather than on whether the config happens to have a server_edition
key (FR-056).
- Tool review states use one vocabulary — Approved / New, needs review
/ Changed, needs review — with the `awaiting` filter value removed;
the Tools page's "Needs review" stat is a link to /review, backed by
interim redirects to /servers (?status=needs_review) and
/servers/:name (?tab=tools) until a later PR ships the real review
views (FR-027, T026a).
- One pure function, contracts.AnnotationTier, computes a tool's tier
(read/write/destructive/unannotated) from its annotations; GET /tools
and GET /servers/{id}/tools carry it, and the Web Tools page and CLI
`tools list --tier` (`--risk` kept as an alias) use it instead of each
deriving their own — the CLI's old `--risk` read a field
(annotations.operation_type) that doesn't exist on a real payload, so
it matched nothing (FR-028, X11).
- Count charts (calls per tool, activity over time) use integer ticks,
and the excluded "never completed a call" group is titled "Calls to
unknown tools" (FR-074).
- "Add to MCPProxy" (Repositories + CLI `registry add`), flipping to
"Added ✓ · Open" after a successful add (FR-063).
- The header ProfileSwitcher is hidden until at least one profile
exists (FR-057).
…PR a) - ToolLabels.swift: one shared approval-state and tier label table (Approved / New, needs review / Changed, needs review; Read / Write / Destructive / Unannotated / Unknown), matching the Web/CLI vocabulary. ServerDetailView's approval badge and ToolsView's new tier badge both go through it. - ServerTool/SearchTool decode the backend-computed `tier` field (contracts.AnnotationTier) rather than deriving one from raw annotations. - ServerBrowseView: "Add to MCPProxy", flipping to "Added ✓ · Open" (opens the server) after a successful add.
make swagger, after adding contracts.Tier / Tool.Tier.
Verified findings against 109-a-quick-wins (PR-a of Spec 109).
FIXED:
- z-index: --z-header/--z-sidebar shipped inverted, so the sticky
header painted over the open mobile drawer sidebar instead of the
other way around (the same class of stacking bug this scale exists
to prevent, on the header/sidebar pair instead of sidebar/modal).
Swapped the token values, updated the doc comment and
z-index-scale.spec.ts's ordering assertion.
- useDialogOpen: never listened for the native `close` event a
showModal()-opened <dialog> fires on Escape, so the driving Vue
state desynced from the DOM the first time a user hit Escape on
Repositories.vue's three dialogs, UserTokens/UserServers/
UserActivity's dialogs, OnboardingWizard or ConnectModal — the
dialog stayed closed forever after that (no reopen). Added an
`onClose` hook plus a `close` listener guarded against looping back
on our own close() calls, wired every affected consumer's own
close/reset function through it. AddServerModal/AddSecretModal
(already safe via useModalA11y's capture-phase Escape handler) get
the same wiring for defense in depth. New test:
use-dialog-open-native-close.spec.ts.
- Search tier: GET /index/search always reported tier=unannotated
regardless of a tool's real annotations, disagreeing with the same
tool's tier on GET /servers/{id}/tools. The reported cause (a missing
"annotations" key in server.searchResultsToMaps's tool map) was one
level above the real one: the Bleve ToolDocument never stored
annotations at all, so a search hit could not carry them no matter
what the map-shaping code did. Added a stored (not indexed)
annotations_json field to the Bleve schema, round-tripped through
toolDocument/readToolMetadata, requested it in the shared search
Fields list, and now surface it in searchResultsToMaps. New tests:
bleve_annotations_test.go, search_tools_tier_test.go.
- Tools.vue: selectStatCard's toggle-to-total branch dropped the
filterApproval reset that existed before this PR removed the old
"pending" stat card. Total is the row's other "reset" gesture
(clearFilters resets both filters too); restored it. New test:
tools-total-card-resets-approval.spec.ts.
- macOS ServerBrowseView: the "Added checkmark Open" flow keyed
addedServers on the registry catalog's display name, but the backend
can assign a different name (empty name falls back to the entry id,
a conflict gets de-duplicated) and echoes it in the response body,
which AddServerResult discarded entirely. Now decoded and used,
matching the Web UI's `result.server?.name || server.name`. New
test: APIClientAddServerFromRegistryTests.swift.
- macOS ToolsView: the tier badge rendered nothing at all for a nil
tier (an older core not yet sending the field), while the Web UI
always renders one, defaulting to "Unannotated". Added
ToolRow.displayTier and used it unconditionally. New tests in
ToolLabelsTests.swift.
- cmd/mcpproxy/registry_cmd.go: registryAddErrorOutput's doc comment
was glued to registryAddMessage by a missing blank line, hiding it
from godoc. Reordered; cosmetic only.
REJECTED (false positive for this PR):
- scripts/test-api-e2e.sh's blanket `pkill -f "mcpproxy.*serve"` /
`pkill -f "launcher-server.*--port 39933"` in its cleanup trap is a
real, pre-existing operational risk (can kill another worktree's or
the user's own running instance), but git blame shows these lines
predate every commit in this PR and none of this PR's three commits
touch the file. No tasks.md for Spec 108/109 exists in this repo to
substantiate the finding's claim that removing them was a task this
PR was itself scoped to do. Left alone here rather than fixed blind
without the actual spec to guide the exact requirement (own-PID
tracking vs. a decoy-process proof) on a script every worktree's E2E
run depends on; flagged instead as a follow-up task.
DEFERRED (genuine, not implemented — infra, not a rejection):
- e2e/web-ui-sweep/visual-a11y-sweep.spec.ts has no elementFromPoint
assertion proving the z-index fix above holds in a real browser.
Adding one needs the real launcher (scripts/run-web-smoke.sh) and a
live Playwright run to confirm it actually catches the regression
rather than being vacuously true — this fix-and-verify pass could
not do that locally, so an unverified test was not committed;
flagged as a follow-up task instead.
Verification: go build ./... and -tags server both clean; go vet and
gofmt clean on touched files; go test -race on internal/index,
internal/server (CI skip regex, both bare and -tags server variants),
internal/httpapi, internal/contracts, cmd/mcpproxy, internal/config,
internal/oauth, internal/storage, internal/serveredition/* all green;
golangci-lint v2 (bare and --build-tags server) on touched packages
shows only 6 pre-existing staticcheck issues in test files this PR
never touched; frontend npm ci + npm run build (vue-tsc + vite) +
npx vitest run all green (147 files / 1393 tests, package-lock.json
unchanged); swift test full suite green except one known
environment-caused failure (AppLifecycleTests, caused by a real
~/.mcpproxy/tray-lifecycle.jsonl already present on this shared
machine from outside this session — unrelated to any file this PR
touches, reproduces identically before any of these changes).
Not pushed; branch left detached per instructions.
Verified each round-2 finding against current source before touching
anything; fixed the genuine ones test-first, deferred one with reasons
below. No false positives to reject this round.
FIXED:
1. (high) Repositories.vue: closeAddRegistry()/closeDeleteRegistry() are
wired as useDialogOpen's native-close handler (Escape/backdrop), but
their own addingRegistry/deletingRegistry guard silently skipped the
showAddRegistry/showDeleteRegistry flip. A native close already happened
in the DOM by the time that fires and cannot be undone, so the guard just
desynced Vue state from an already-closed dialog — reopening became a
no-op forever (round 1's H4 bug, reintroduced by round 1's own fix). Split
the native-close handler from the Cancel-button handler; the Cancel/
Delete-cancel buttons keep the guard (now via :disabled) since an
explicit click CAN be blocked mid-submit.
2. (medium) OnboardingWizard.vue: dismiss() (also wired as useDialogOpen's
onClose) awaited up to three sequential engagement-bookkeeping calls
before emit('close'). props.show stayed true for that whole window, so
reopening the wizard from the sidebar Setup entry was a no-op. emit
synchronously first; the bookkeeping now runs decoupled in the
background.
3. (low) ConnectModal.vue: close() (same onClose role) reset several result/
preview fields but not disconnectTarget, so Escape during the "Disconnect
X?" confirm sub-panel left it stale for the next open.
4. (medium-high) internal/runtime/lifecycle.go: annotations_json is only
written at (re)index time, but the reindex trigger is a Hash comparison
that deliberately EXCLUDES annotations (see calculateToolApprovalHash's
own comment in tool_quarantine.go: annotations are unstable across
reconnections, and hashing them in before caused false
"tool_description_changed" spam on every reconnect). So an
already-indexed, otherwise-unchanged tool could never pick up newly
observed annotations short of a manual reindex. Fixed WITHOUT touching
Hash/change-detection (which would reintroduce that exact regression):
applyDifferentialToolUpdate now tracks annotation-only diffs separately
and silently refreshes just those documents (no approval/quarantine
state, no "Tool schema changed" log, no hash rewrite). Covered by
TestApplyDifferentialToolUpdate_BackfillsAnnotationsForUnchangedTool and
...IsIdempotent.
6. (low-medium) RegistryModels.swift: RegistryAddServerErrorBody decoded a
`message` key, but writeRegistryAddError (internal/httpapi/server.go)
serializes the field as `error` — every other error envelope in this API
uses the same shape. err?.message was therefore always nil against the
real backend, so a failed Add-from-Registry always showed the generic
"HTTP 400: ..." text instead of the actual reason. Fixed the CodingKeys
mapping and added a test that asserts on the decoded message text (the
round-1 test used a stub keyed `message`, which "passed" without ever
checking the value — a green test over a broken decode).
7. (low) Same files: round 1's own comment/test claimed the backend
"de-duplicates on conflict" for a name collision. It doesn't —
AddServer (internal/server/server.go) hard-fails with duplicate_name; no
rename/suffix logic exists anywhere in that path. Corrected the
misleading comments in RegistryModels.swift and ServerBrowseView.swift,
and reworked the test's success-case stub to model the one case that IS
real (empty entry name falls back to the entry id) instead of an
invented de-dup suffix.
8. (medium) Settings.vue: hasServerEdition depends on systemStore.status,
populated asynchronously by App.vue's SSE connection in its own
onMounted. On a reload of /settings?tab=teams, Settings.vue's onMounted
could run its `?tab=` membership check before that status frame lands,
silently dropping `teams` with no retry once it arrives. Added a
one-shot retry gated on hasServerEdition, guarded so it never overrides
a tab the user picks in the meantime and is a no-op when the edition
really is personal.
DEFERRED (real, not a false positive, but out of scope for a quick-wins
fix round):
5. (low) internal/index/bleve.go: a pre-105/pre-this-PR on-disk index opens
with its OWN persisted mapping, which never declared annotations_json —
so that field falls to Bleve's dynamic-mapping default (Index=true,
contributes to _all) on a legacy index, while a freshly created index
gets the explicit Index=false mapping added in round 1. Confirmed real:
this is genuine BM25 divergence between legacy and fresh indexes.
NOT fixed here: the obvious-looking fix (disable dynamic mapping) is
actively wrong — output_schema_json has NO explicit field mapping at all
and already relies on the same dynamic-mapping fallback in every index,
old and new alike; disabling it would silently stop storing that field
on every newly created index. A correct fix needs an actual Bleve
mapping-version/migration mechanism (there is none today — the
OutputSchemaHashSchemaVersion precedent lives entirely in the BBolt
approval-record store, not the search index), which is a properly scoped
follow-up, not a round-2 quick fix.
Verification: go build ./... and -tags server; go test -race
./internal/runtime/... (192s, clean); golangci-lint v2 bare and
--build-tags server (only pre-existing, unrelated issues, e.g.
event_bus.go:887 reflect.Ptr); frontend npm run build + npx vitest run
(151 files, 1401 tests, all green; package-lock.json unmodified); swift
test (1179 tests, 1 failure — AppLifecycleTests.
testTheSharedJournalNeverWritesToTheRealInstanceRootUnderTests, a
documented pre-existing environmental flake tied to a real
~/.mcpproxy/tray-lifecycle.jsonl on this machine, unrelated to this diff
and already failing on main).
Verified each round-3 finding against current source before touching anything; fixed the genuine one test-first, confirmed one is already covered by existing tests, and reaffirmed one deferral. No false positives to reject this round — all three findings described the code accurately. FIXED: 1. (medium, T011a) scripts/test-api-e2e.sh's cleanup trap used a blanket `pkill -f "mcpproxy.*serve"` / `pkill -f "launcher-server.*--port 39933"`. Round 1 rejected this same finding on the premise that no tasks.md substantiates it being an in-scope task for this PR; that premise was false — specs/109-ux-navigation- consistency/tasks.md (present on the sibling 109-ux-navigation- consistency branch, both branched from the same commit) lists T011a explicitly under "Phase 2: PR 109-a — quick-wins". Live-verified the actual risk before fixing: this machine had a real tray-managed `mcpproxy serve` (PID 15758) and a second, unrelated worktree's test instance running at the same time — the unfixed script's blanket pkill would have killed both. Replaced both blanket kills with `descendant_pids()`, which snapshots the actual OS-process descendants of $MCPPROXY_PID (recursively, via `pgrep -P`) BEFORE touching it, then reaps only those specific PIDs (and only if still alive) as a fallback for the launcher-test fixture getting orphaned before mcpproxy's own graceful-shutdown reap path runs. Added scripts/test-api-e2e-cleanup-check.sh (the check T011a specifies): starts a decoy `mcpproxy serve` on another port with a scratch data dir, runs the full E2E script, and asserts the decoy is still alive afterward. Ran it for real: 65/66 sub-tests passed (the one failure is the pre-existing, unrelated "server-edition binary present" check, which needs a separately built ./mcpproxy-server and fails identically on main), and the decoy — plus this machine's real tray core — survived the run. 2. (low-medium, T007 Playwright half) e2e/web-ui-sweep/ visual-a11y-sweep.spec.ts had no browser-level proof that the H4 z-index fix (frontend/src/assets/z-index.css) holds under real layout and paint, only jsdom token-ordering coverage. Added "mobile drawer sidebar paints above the sticky header, not under it (H4)": opens the mobile drawer against a real launched instance and asserts via `elementFromPoint` that the drawer, not the header, is the topmost element at a point they both occupy. Verified it actually catches the regression it targets, not just a code read: temporarily re-inverted --z-header/--z-sidebar back to round-1's shipped bug, rebuilt, and reran — the new test timed out failing exactly as expected; restored the tokens and reran the full 25-test suite clean. One real finding surfaced while building this: daisyUI's drawer opens via a CSS `visibility`/`opacity` transition (`allow-discrete`, ~0.2-0.3s), so `elementFromPoint` checked on the very next frame after the checkbox flips can still legitimately see the header for a couple of frames — normal opening animation, not the H4 regression (a permanently wrong settled z-index). The assertion polls for the settled state instead of asserting immediately, so it does not flake on that transition. ALREADY COVERED (not a gap, no change needed): - The finding's other T007 half — "a vitest asserting none of Repositories.vue's three dialogs still uses the `:open` binding" — already exists: frontend/tests/unit/z-index-scale.spec.ts (added in the original PR commit 8bc8d23de, kept through round 1) has both "Repositories.vue dialogs use showModal()/close(), not :open (FR-055)" (lines 71-88, checking exactly the `:open` binding and all three data-test ids) and a broader "every <dialog class=modal> drives its open state imperatively" sweep across all 8 affected components, Repositories.vue included. Wrote a duplicate test file first, then found this and deleted it rather than ship two guards for the same invariant. DEFERRED (confirmed real, reaffirming round 2's deferral — not fixed here): 3. internal/index/bleve.go: re-verified the pre-105/pre-this-PR legacy-index dynamic-mapping divergence against current source. annotations_json still has no on-disk mapping-version guard, RebuildIndex() is still a documented no-op, and output_schema_json still has no explicit field mapping of its own — so it's still true that disabling dynamic mapping to close this gap would silently stop indexing output_schema_json on every freshly created index, on top of every index that already exists today. Round 2's reasoning holds exactly as written: a correct fix needs an actual Bleve mapping-version/migration mechanism, which is properly scoped design work, not a quick-wins round change. Flagging a follow-up task for that design work separately rather than leaving it only in this commit message. Verification: go build ./... and -tags server (-o /dev/null, per the server-tags-build-overwrite gotcha) both clean. No Go, frontend/src, or native code touched this round, so go test -race / vitest / swift test are unaffected by this diff; ran golangci-lint v2 anyway (bare and --build-tags server) — 16 and 19 pre-existing issues respectively, none in any file this round touched (reflect.Ptr inlining, deprecated TopK/Features test usage, deprecated ecdsa.PublicKey.X/Y, ParseDir deprecation — all pre-existing on files untouched by any of the three rounds). scripts/test-api-e2e-cleanup-check.sh and the new Playwright test were both run for real against a locally built instance, not just read. Not pushed; branch left detached per instructions.
Round-4 zcode findings on the E2E cleanup trap (T011a) and a stale Tools.vue stat-card bug. Confirmed genuine, fixed test-first: - F3 (medium): descendant_pids had no test of its own. Extracted it to scripts/descendant-pids.sh (sourced by test-api-e2e.sh) and added scripts/descendant-pids.test.sh, a hermetic unit test that builds a real 3-level process tree and proves the recursive walk reaches a grandchild and great-grandchild, not just direct children. Also extended test-api-e2e-cleanup-check.sh to assert a REAL orphan of the run (the launcher-test fixture, left running by test_launcher_lifecycle Step 5) is actually gone after cleanup, not just that the decoy survives. Wired both scripts into the Makefile (test-descendant-pids, test-e2e-cleanup-check). - F4 (medium): the audit_log sub-test's AUDIT_PID/AUDIT_DATA_DIR/ AUDIT_JSONL_DIR/AUDIT_SERVER_LOG were only reaped on its own fall-through path; an early exit while that sub-test is running leaked them. cleanup() now reaps them too, idempotently. - F1 (medium, partial fix): the descendant snapshot was taken once, before sending the kill signal, missing a child spawned during mcpproxy's own graceful-shutdown window. Now re-snapshotted on every poll of the wait loop while mcpproxy is confirmed still alive. The case where mcpproxy has already exited (crash/OOM) before cleanup() even runs is not fixable without reintroducing the system-wide pattern match T011a deliberately removed (kills unrelated mcpproxy instances) — documented as an accepted, narrow residual gap. - F2 (low): the reap loop now records each descendant's command name at snapshot time and re-checks it before SIGKILL, so a PID reused by an unrelated process in the snapshot-to-reap window is skipped instead of killed. - F6 (low): test-api-e2e-cleanup-check.sh's decoy port was a fixed default (only DECOY_DIR was made unique), colliding between concurrent runs. Now picks an OS-assigned free port. - F7 (low): added --max-time to the two curl calls in test-api-e2e-cleanup-check.sh that lacked it, matching every curl call in test-api-e2e.sh. - Tools.vue activeStatCard (low, flagged rounds 1 and 2, never actually fixed): it only inspected filterStatus, so an approval-only filter (filterStatus empty) still rang the Total stat card as active even though the table was filtered. Total now reads as active only when no filter narrows the table. New regression test: tools-active-stat-card-approval-only.spec.ts (reproduced the bug first, then fixed). F5 (EXIT trap not firing on SIGINT) does not hold up under a faithful reproduction: sending SIGINT to the actual process group (what a terminal's Ctrl-C really delivers) already ran cleanup() correctly on the pre-round-4 code, because the foreground child dies from the same signal, bash's `wait` unblocks, and an untrapped terminating signal still fires the EXIT trap. Verified this directly (group-wide kill -INT and kill -TERM both fired the trap on the unmodified script; only a single-PID kill -INT, which is not what a terminal sends, appeared to hang, and that appearance was itself an artifact of test harness backgrounding setting SIGINT to ignore). Kept explicit `trap ... INT TERM` handlers anyway as harmless, idiomatic defense-in-depth for a supervisor that signals only this process's PID, with an idempotent cleanup() (CLEANUP_DONE guard) so it cannot double-run. Verified locally: go build ./... clean (no Go files touched by this commit); golangci-lint v2 (bare and --build-tags server) shows only pre-existing unrelated findings; frontend build + full vitest run (152 files / 1402 tests) green; scripts/descendant-pids.test.sh green; a full scripts/test-api-e2e-cleanup-check.sh run against a real built mcpproxy/mcpproxy-server passed all 70 E2E tests, confirmed the decoy instance survived on a freshly-picked port, and confirmed the real launcher-server orphan was reaped.
Verify each round-6 finding against the code before fixing; fix genuine
ones test-first, reject/defer two that are out of scope here.
1. internal/index/bleve.go (finding 1, high): an index created before the
annotations_json field mapping existed keeps its old mapping forever
after bleve.Open (no SetMapping in vendored bleve v2.6.1), so the
differential-update backfill's annotations JSON gets indexed as free
text via bleve's dynamic-field defaults, leaking into the field-less
`_all` composite field and skewing BM25 ranking on any upgraded (not
freshly installed) deployment. A full fix needs index-mapping-version
tracking plus rebuild-from-storage, which doesn't exist yet
(RebuildIndex is a stub) -- flagged as a follow-up task instead of
building that here. Applied a bounded, tested mitigation:
warnIfMappingPredatesAnnotations logs an actionable warning on open;
toolMapping.Dynamic = false closes the whole bug class for indexes
created from now on; and an explicit output_schema_json field mapping
(previously undeclared, relying on the same dynamic fallback) closes a
second, pre-existing instance of the same leak that surfaced only once
Dynamic was turned off. New tests: bleve_mapping_migration_test.go.
2. cmd/mcpproxy/tools_cmd.go (finding 4, medium): --tier/--risk/--status/
--approval were only ever applied on the global (no --server) tools-list
path; `--server=x --tier destructive` silently printed every tool
unfiltered. Client-mode (daemon-backed) now reuses
applyGlobalToolFilters, since GET /api/v1/servers/{id}/tools shares
enrichServerTools with the global endpoint and carries the same fields.
Standalone (no-daemon) mode gets a new filterToolMetadataByTier for
--tier (computed locally via contracts.AnnotationTier) and a fast,
explicit error for --status/--approval, which need daemon-persisted
state this path has no access to. A zcode re-review of this fix caught
a residual message bug it introduced: emptying a non-empty standalone
result via --tier fell into the "server doesn't support tools"
diagnostic meant for a server with genuinely zero tools;
standaloneNoToolsMessage now tells the two cases apart.
3. native/macos ServerBrowseView.swift / RegistryModels.swift (finding 2,
medium): "Add to MCPProxy" -> "Added (checkmark) Open" state was tracked
by bare server.id, while the same file's own cross-registry search
dedupe already keys by "registry::id" (catalog ids collide across
registries, MCP-866) -- two colliding cards from different registries
could flip together. Added RepositoryServer.addedKey and switched both
addedServers call sites to it. A zcode re-review found the same
collision class still live in ForEach(results)'s SwiftUI identity and in
addingID; fixed both, plus a stale doc comment that still described the
bug this commit fixes.
4. frontend/src/components/AuthErrorModal.vue (finding 3, medium): the
last dialog left on the old v-if/div modal pattern while its four
siblings were migrated to the browser top layer (FR-055); a 401 raised
while one of those is open painted underneath it. Migrated to a native
<dialog> via the existing useDialogOpen/useModalA11y composables,
deliberately without the click-catcher backdrop form the other dialogs
use (clicking outside must not dismiss this one -- a pre-existing,
audited rule). Replaced the onMounted form-reset with a
watch(props.show), since the dialog no longer unmounts/remounts on
every open/close the way the v-if version did.
5. scripts/test-api-e2e-cleanup-check.sh (finding 5, medium): the T011a
proof's launcher-server pgrep check had no pre-run baseline, so a stray
left by an earlier crashed run or a parallel worktree's own E2E run
produced a false FAIL. Snapshots PIDs before running and diffs with
comm -13 so only a genuinely new PID counts as this run's own leak.
Rejected (not fixed here):
- Finding 6 (specs/109-ux-navigation-consistency/tasks.md T026 vs macOS
DashboardView.swift): tasks.md lives on branch docs/specs-108-109, not
this branch, so it can't be edited from here. The same tasks.md's own
"Follow-ups (not in this spec)" section already lists "a native macOS
Usage view" as deferred future work, directly contradicting T026's
in-scope macOS clause -- a spec/tasks inconsistency for whoever owns
that branch to reconcile, not a reason to invent a new macOS chart
feature inside a quick-wins PR. Flagged as a follow-up task.
Every fix is test-first: internal/index, cmd/mcpproxy and native/macos
each got new regression tests confirmed to fail before the fix (or, for
the macOS swift package which has no such build gate here, verified by
temporarily reverting and re-running) and pass after.
Reviewed with zcode (GLM), the mandated external reviewer for this
branch. It returned a clean pass on internal/index/bleve.go +
cmd/mcpproxy/tools_cmd.go (one residual message bug found and fixed, see
above), a clean pass with three genuine follow-ups on the macOS
ServerBrowseView/RegistryModels change (all three folded into this
commit), and a clean pass with no defects on
scripts/test-api-e2e-cleanup-check.sh. The AuthErrorModal.vue chunk could
not get a completed zcode pass despite five retries over ~20 minutes --
the shared zcode/GLM account was rate-limited (HTTP 429, "Rate limit
reached for requests") by concurrent sibling review sessions running the
same review round in parallel worktrees, not by anything in this diff.
Verified by hand instead, against the identical, already-shipped and
already-tested pattern in AddSecretModal.vue / AddServerModal.vue /
ConnectModal.vue / OnboardingWizard.vue, plus the full frontend build
(vue-tsc + vite) and vitest suite (152 files / 1402 tests, including
auth-single-surface.spec.ts) passing.
Local verification: go build ./...; go test -race on internal/index,
cmd/mcpproxy, internal/runtime (+ subpackages), and an internal/httpapi
tool/tier/search subset; golangci-lint v2 (bare and --build-tags server)
clean on every touched Go package, no new issues anywhere else; npm ci &&
npm run build && npx vitest run (152/152 files, 1402/1402 tests), then
package-lock.json left unchanged (no dependency changes); swift build +
swift test (one pre-existing, unrelated failure confirmed present with
and without this diff: AppLifecycleTests.
testTheSharedJournalNeverWritesToTheRealInstanceRootUnderTests).
Round 7 findings, verified against the code (zcode unavailable this round: 5-hour usage limit, resets 10:35 — reviewed manually instead, same as the finding's own note): - frontend/src/composables/useDialogOpen.ts (high, confirmed): a showModal() dialog's own Escape handling fires a native `cancel` event and, unless that event is canceled, closes the dialog — independently of useModalA11y's document-level keydown listener, whose preventDefault() only affects the keydown event and does not stop the platform's cancel/close algorithm. AuthErrorModal relies on its close() being a no-op while canClose is false, but the native path ignored that gate entirely: Escape closed the <dialog> element regardless, desyncing it from isOpen() (same bricking shape as the existing H4 fix) and defeating the "must not be dismissable" intent the whole modal exists for. Fixed by always preventing the native `cancel` event in useDialogOpen and routing every Escape-driven close through the one close() callback that already owns the permission check, the same way useModalA11y's keydown handler does. Covered by frontend/tests/unit/use-dialog-open-cancel-prevented.spec.ts, confirmed red before the fix (dispatching a synthetic `cancel` event closed the dialog) and green after. - internal/index/bleve.go (medium, confirmed, carried from round 6): round 6 only warned when an on-disk Bleve index predated the annotations_json field mapping, leaving an in-place upgrade with a silently stale mapping (and the resulting free-text/`_all` leakage) until an operator noticed the log line and deleted the directory by hand. Manager.RebuildIndex is a pre-existing unwired no-op stub, not a path to fix this. Replaced the warn-only function with one that closes the stale index, removes its directory, and creates a fresh one with the current mapping automatically — safe because index.bleve is a derived search cache, not a source of truth: an empty index makes every server's tools look newly discovered, and the normal discovery path (applyDifferentialToolUpdate) already reindexes everything as each server reconnects at startup, the same way it backfills a brand-new install. Extended bleve_mapping_migration_test.go with a regression test asserting the rebuilt index carries the current mapping and rejects free-text search on annotations_json, with no operator step. - native/macos/MCPProxy/Views/DashboardView.swift (medium, carried from rounds 5-6): re-scoped rather than implemented. Spec 109 T026's "macOS DashboardView.swift equivalent label" describes the same integer-tick + "Calls to unknown tools" fix web's CallHistogram.vue got, but macOS has no per-tool call-histogram anywhere to attach it to — the file's only chart-like element (Token Distribution) is a per-server token-SIZE bar list with no ticked axis and no "unresolved tool name" concept, a different metric keyed by server name rather than call outcome. Spec 109's own tasks.md lists "a native macOS Usage view" as a Follow-up explicitly "not in this spec" — building one now would be new scope, not a quick win. Added an explanatory comment recording that scope decision next to tokenDistributionSection so this stops being silently re-flagged every round; tracked for the eventual macOS Usage view instead. Verification: go build ./...; go test -race ./internal/index/...; golangci-lint v2 (bare and --build-tags server) clean of new issues; frontend npm ci + vitest run (153 files / 1405 tests green) + vue-tsc build; swift build + swift test (1182 tests, 1 pre-existing failure — AppLifecycleTests.testTheSharedJournalNeverWritesToTheRealInstanceRootUnderTests fails on this machine because ~/.mcpproxy/tray-lifecycle.jsonl already exists from real daily use, unrelated to and unaffected by this comment-only Swift change; reproduces identically without it).
…R-060/061/065)
- internal/registries/catalog.go: SearchAll fans out to every enabled
registry in parallel with a per-source timeout, merges, de-dups by
(source, id), ranks with the pure Rank function, and returns
unavailable[] for failed/timed-out sources. Empty query returns
official/popular sections.
- internal/secretlike: shared D13 secret-like-name heuristic.
- internal/shellwords: small quote-aware command-line tokenizer used to
split an install command into CatalogInstall{command, args}.
- internal/secret/refname.go: RefName computes the FR-065 keyring ref
name per field kind (env/header), with collision suffixing, pinned by
the shared internal/secret/testdata/ref_names.json fixture.
- internal/secret: keyring provider now reports an availability reason
(IsAvailableWithReason / Resolver.KeyringAvailability), surfaced by
GET /secrets/config as keyring_available/keyring_reason.
- internal/httpapi/catalog.go: GET /catalog/search REST endpoint (no
admin gate), with 'added' computed only over the caller's visible
servers (FR-007).
…chment (FR-064) - configimport: DetectFormat falls back to the new 'url' and 'command' formats for a single-line paste that is neither JSON nor TOML; URLParser/CommandParser map them to a remote or local/stdio server. SuggestNameFromURL/SuggestNameFromCommand derive a sensible default name (preferring the package name for npx/uvx/pipx runners). - httpapi: POST /servers/import/json?preview=true accepts url/command content and its response gains summary, tags and per-env/header secret_like/empty_or_placeholder previews, built from the already-redacted URL/Command/Args so no raw secret reaches the new fields either.
- cliclient.CatalogSearch calls GET /catalog/search. - cmd/mcpproxy/catalog_cmd.go: catalog search/show/add, daemon-first with an in-process SearchAll fallback for search; 'registry search'/'registry add' now print a deprecation notice pointing at their 'catalog' equivalent (both remain functional aliases). - registries.BuildCatalogHit/ToCatalogResult exported so a single-entry lookup (catalog show) builds the same shape SearchAll uses. Fixes a bug caught while wiring this up: the 'added' join treated every configured server as matching by install-target alone, so a registry-sourced server (source_registry_id set) could make a DIFFERENT source's identical-looking entry falsely read added=true. Only a manual add (no source_registry_id) matches by target alone; a registry-sourced one also needs its own source_registry_id to match. Fixed in both internal/httpapi/catalog.go and cmd/mcpproxy/catalog_cmd.go, with a regression test in internal/httpapi/catalog_test.go.
Each value is written to the OS keyring under its per-kind ref name
(internal/secret.RefName: <server>-env-<name> / <server>-header-<name>)
and the config gets ${keyring:<ref>} instead of the raw value. An env
var and a header of the same name write two distinct entries; a
pre-existing entry is left unchanged and the new ref gets a numeric
suffix (D28). Refuses outright, before writing anything, when the
keyring is unavailable.
- views/AddServer.vue: Catalog (default) · Paste · Import · Manual tabs, synced to ?tab= via router.replace; ?source= narrows the Catalog tab only and never selects a tab. - components/CatalogSearch.vue: searches every enabled catalog source (GET /catalog/search), renders official/popular sections for an empty query, 'Add to MCPProxy' -> 'Added ✓ · Open', and a secrets prompt for entries with required_inputs. Catalog-source text (title, description, publisher) is rendered as plain text only (D19). - components/PasteServer.vue: detects a pasted URL/command/JSON/TOML via the extended import-preview endpoint and shows its summary/tags before anything is added. - components/ManualServerForm.vue / ImportServersPanel.vue: the Manual and Import tabs. - components/SecretToggle.vue + composables/useSecretFields.ts: the shared Value/Secret toggle (FR-065) used by all three tabs that take env/header input — writes to the OS keyring via the shared refName algorithm, rolls back only the secrets a failed add itself wrote. - utils/secretRef.ts / secretLike.ts: TS ports of the Go refName/ secret-like-name helpers, pinned against the shared internal/secret/testdata/ref_names.json fixture (copied into tests/unit/fixtures/). ServerDetail.vue's old suggestSecretName (which dropped the field kind) now delegates to the shared helper. - router: /repositories redirects to /add-server?tab=catalog; TopHeader's "+ Add Server" now opens the new page. - components/CatalogSourcesSettings.vue: catalog-SOURCE management (add/edit/delete a registry) moved out of the retired Repositories.vue into Settings -> Catalog sources; server BROWSING moved to CatalogSearch.vue. The 5 Repositories-specific tests were retired or retargeted (settings-catalog-sources.spec.ts, z-index-scale.spec.ts); add-server-catalog.spec.ts now covers the FR-063 label behavior. Known gap (documented, not fixed in this PR): AddServerModal.vue and its other callers (Servers.vue, Dashboard.vue, OnboardingWizard.vue) are left as-is — retiring them fully cascades into ~9 existing test files (protocol detection, duplicate-endpoint, trust-mode, a11y) that are out of this PR's scope; TopHeader's primary entry point was the one rewired.
Omitted, it searches every enabled catalog source via registries.SearchAll (FR-060) and returns the merged, ranked ServerEntry list in the same JSON shape a single-registry search already returns — no 'added' field (that's REST-only, contracts/rest-api.md#catalog). list_registries' wording moves to 'catalog source' terminology; both tools' descriptions point at the new all-sources default. Updates the frozen toolslist goldens deliberately (search_servers/ list_registries added to both the pre-099 and pre-105 enumerated-delta gates, plus TestMenuSurface_ExactDeltaFromPreFeature's per-field assertSearchServersDelta/assertListRegistriesDelta) since this changes a built-in MCP tool's schema (registry: required -> optional).
… 109 FR-060/065) - API/CatalogModels.swift: Codable mirrors of GET /api/v1/catalog/search's DTOs (CatalogResult, CatalogInstall, CatalogInput, CatalogSections), distinct from RegistryModels' RepositoryServer (that JSON stays unchanged). - API/APIClient.swift: searchCatalog(query:source:tag:limit:) calling the new endpoint. - Models/SecretRefName.swift: Swift port of internal/secret/refname.go's RefName (FR-065), pinned against the shared ref_names.json fixture in MCPProxyTests/CatalogTests.swift. ServerDetailView's old suggestedSecretName (which dropped the field kind) now delegates to it. - Models/SecretLikeName.swift: Swift port of the D13 secret-like-name heuristic. Known gap (documented, not fixed in this PR): the Add Server sheet's Catalog/Paste tabs and the Registries-sidebar-item removal are NOT implemented — this PR lands the shared, contract-critical data layer (verified via swift test) that a follow-up UI PR builds on, mirroring the same scoped-down decision made on the Web side for AddServerModal's other callers.
…context scoping (T099) - registries: a registry's explicit isSecret:false (or omitted) on a secret-shaped name still serves secret_like:true; an explicit true passes through unchanged. - httpapi: FR-007 scoping is not agent-token-specific — a non-admin AuthTypeUser session (server edition's OAuth user identity) scoped to one server sees the same added:true/false split an agent token does.
…FR-060/065/067)
High
- secret: IsAvailableWithReason (GET /secrets/config keyring_available,
and the CLI's applySecretFlags pre-check) now consults the macOS write
gate (writesEnabled()) before the read-only probe, not after. Before
this, a headless `mcpproxy serve` or the CLI's own in-process resolver
reported keyring_available:true and then had every Store() call
immediately fail with ErrKeyringUnavailable, because only Store() (not
the probe) checked the gate.
- web(CatalogSearch): confirmAdd() closed the secrets dialog and cleared
addError unconditionally after addResult(), even on failure — addResult
never throws (api.ts always resolves {success:false}), so a failed
add-after-secret-write looked identical to success. addResult now
returns a success bool; confirmAdd only closes the dialog on success and
rolls back the just-written secret otherwise.
Medium
- web: rollbackSecrets was exported but never called by any of the three
add surfaces (CatalogSearch, PasteServer, ManualServerForm) when the
add-server call failed after a successful secret write, orphaning a
keyring entry and forcing a retry onto a -2-suffixed name. All three now
track resolveSecretFields' writtenRefs and roll them back on a later
failure.
- cli(upstream add): applySecretFlags (writes to the OS keyring) ran
before validateTrustModeFlag, so a typo'd --trust-mode orphaned an
already-written secret. Moved trust-mode validation first. Also:
applySecretFlags now rolls back its own partial writes when the second
of two --secret-env/--secret-header flags fails; runUpstreamAddDaemonMode
and runUpstreamAddConfigMode now return (added bool, err error) so a
--if-not-exists skip (nil error, added=false) is distinguishable from a
genuine add, and runUpstreamAdd rolls back any written secrets via defer
unless added ends up true (covers the daemon/config-load/save failure
paths too, not just the skip).
- mcp(search_servers): 'registry' omitted returned bare registries.ServerEntry
items with none of the catalog fields (title/publisher/verified/official/
popularity/source) contracts/mcp-tools.md requires — MCP callers got none
of the FR-060 ranking evidence REST/CLI callers already had. Added
mcpCatalogServerEntry (embeds ServerEntry, encoding/json promotes its
fields, plus the catalog fields alongside).
- httpapi(catalog): GET /catalog/search set results to the full ranked list
even when sections was populated (empty q), contradicting "Empty q →
results: []". Now emptied whenever sections is set.
- registries/httpapi/cli(catalog): source= was filtered AFTER SearchAll had
already truncated to limit, so a narrower source's real matches ranked
below an official/verified source could vanish entirely. SearchOptions
gained a Source field; SearchAll now filters before ranking/truncation.
Both httpapi.handleCatalogSearch and the CLI's catalogSearchInProcess
switched to it, dropping their duplicated post-hoc filter helpers.
Rejected
- registries(catalog): Popularity is never populated in production
(registries.ServerEntry carries no stars/installs field, and no
registry-protocol parser in this codebase fills one — confirmed only
tests ever construct a Popularity value), so the FR-060 popularity
tiebreak always compares 0-vs-0 and the "Popular" section degrades to
the same ranked list. Not fixed here: sourcing a real popularity signal
(GitHub stars API, npm download counts, or a registry-reported count)
is a new external-data integration — new calls, caching, rate-limit
handling — sized for its own spec/plan, not a bounded review-round fix.
Tests: internal/secret (macOS write-gate cases), internal/server
(search_servers catalog fields), internal/httpapi (empty-q results,
source-filter-before-truncation), cmd/mcpproxy (CLI source-filter,
applySecretFlags self-rollback, --if-not-exists added=false), frontend
vitest (CatalogSearch/PasteServer/ManualServerForm rollback-on-failure).
Verification: go build ./...; go test -race on internal/secret,
internal/registries, internal/httpapi, cmd/mcpproxy; go test (and
go test -race, -skip per CLAUDE.md) on internal/server; golangci-lint v2
bare and --build-tags server on all touched packages (0 new issues); cd
frontend && npm ci && npm run build && npx vitest run (155 files / 1428
tests green); package-lock.json unchanged.
…T109/T103) High - macOS(AddServerView): the tray shipped the catalog data layer (CatalogModels, SecretLikeName, SecretRefName) but never wired it into any UI — AddServerView only had Import/Manual tabs, so a macOS user had no way to reach the new catalog search/add-server flow the Web UI, CLI (`catalog search|show|add`) and MCP (`search_servers`) all gained in this PR. Added CatalogView.swift (aggregated GET /api/v1/catalog/search across every enabled source, Official/ Popular sections, "Add to MCPProxy" -> "Added checkmark Open") and PasteServerView.swift (paste a URL/command/config, preview-detect via POST /api/v1/servers/import/json?preview=true, fill in detected fields) as new Catalog/Paste tabs on AddServerView, ordered Catalog - Paste - Import - Manual to match the Web UI's views/AddServer.vue (T103/T109). The generic "Add Server" entry points (toolbar +, prominent button, empty-state button, dashboard shortcut) now open on Catalog by default instead of Import/Manual. - macOS(MainWindow): removed the Registries sidebar item per T109 — it's fully superseded by the new Catalog tab for discovery. Registry SOURCE management (add/edit/remove a registry) moved to a new Settings -> Catalog Sources tab (CatalogSourcesTab, replacing the old RegistriesView's "Manage registries" half); the old per-registry ServerBrowseView "Discover servers" half is removed outright, superseded by the aggregated catalog search. Updated docs/registries.md's Web UI/macOS surface bullets to match (they described the pre-109-j Repositories page and Registries sidebar tab). Both required a small backend-facing addition: a Swift port of the Web UI's resolveSecretFields (SecretFieldResolver.swift, kind-aware so an env var and a header of the same name never collide on one keyring ref, FR-065) plus the APIClient calls it needs (getSecretRefs, deleteSecret, previewImportContent) and the decode models (SecretRefEntry, ImportPreviewServer/Field). Pure assembly/field-building logic (buildValues, buildFields, makeServerConfig) is unit-tested without a network call, mirroring ManualServerForm's existing pure-seam pattern; addServerFromRegistry/searchCatalog were already present from the T104/T108 rounds and needed no changes. Not reviewed - zcode (the mandated reviewer) was unavailable for this entire round — account-wide 5-hour usage-quota exhaustion (ProviderBusinessError 1308), same block reported in round 2, not expected to clear inside this task's window. Per this task's standing instructions codex is off-limits for 2 days and only zcode was mandated, so no reviewer ran; this fix is unreviewed by an external model. Flagging rather than silently proceeding as if reviewed. Verification: swift build (clean) + swift test (1203/1203 passing, excluding one pre-existing environment-dependent failure — AppLifecycleTests' real-instance-root check fails identically on the unmodified base commit, a leftover ~/.mcpproxy/tray-lifecycle.jsonl on this machine, unrelated to this diff); go build ./... (clean, no Go files touched); golangci-lint v2 (pre- existing unrelated staticcheck/govet issues only, same as base). No frontend files touched.
Deploying mcpproxy-docs with
|
| Latest commit: |
52d671f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2b6053c6.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://109-j-catalog-add-server.mcpproxy-docs.pages.dev |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 36255433292 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
# Conflicts: # cmd/mcpproxy/tools_cmd.go # cmd/mcpproxy/tools_tier_test.go # frontend/src/components/OnboardingWizard.vue # frontend/src/components/TopHeader.vue # frontend/src/composables/useDialogOpen.ts # frontend/src/views/Repositories.vue # frontend/src/views/ServerDetail.vue # frontend/src/views/Settings.vue # frontend/src/views/Tools.vue # frontend/tests/unit/server-detail-tab-url.spec.ts # frontend/tests/unit/use-dialog-open-cancel-prevented.spec.ts # frontend/tests/unit/z-index-scale.spec.ts # internal/index/bleve.go # internal/index/bleve_mapping_migration_test.go # native/macos/MCPProxy/MCPProxy/Views/ServerBrowseView.swift # oas/docs.go
F-A/F-D (high/medium, Paste tab Add flow, Web + macOS): Add now re-parses the ORIGINAL raw pasted content server-side (new POST .../import/json apply call) instead of reconstructing the server config from the preview's redacted url/command/args — a credential embedded directly in a URL query param or an argv flag was masked for display and that masked placeholder was being baked into the real, permanently-broken server config. env_override/header_override carry the Paste tab's SecretToggle edits across the apply call, since those never appear in the preview response, and also fix F-D's separate bug where env/header edits were dropped whenever they didn't match the detected transport branch. F-B (medium, catalog "added" join): toCatalogInstall now checks InstallCmd before URL/ConnectURL, matching officialServerToEntry's documented "package wins for stdio; keep the remote as a fallback" precedence for a hybrid official-registry entry — the reversed order was reporting http/ConnectURL transport for a server that installs (and is configured) as stdio. parseInstallCommand is now quote-aware (shellwords), matching the catalog side's split, so an install command with a quoted argument still joins correctly against the configured server. F-C (medium, Web variant only): resolveSecretFields now aborts instead of silently treating a failed GET /secrets/refs as "nothing is taken", which could otherwise pick an un-suffixed ref name that overwrites a pre-existing keyring entry, and have a later failure roll back and delete it. Deferred: the underlying CLI/cross-process race (two concurrent adds computing the identical ref name) needs compare-and-set support at the keyring Store layer, which no OS keyring backend here provides portably — a real architectural gap, not a targeted fix. F-E (medium): DetectFormat's URL/single-command-line guess (FR-064) is now opt-in (ImportOptions.AllowPasteFallback / new DetectFormatStrict). Only the Paste tab (which previews the guess for a human to confirm before Add ever mutates anything) sets it; the CLI's `upstream import`, a direct REST import call, and canonical-path import all keep returning a clear "unable to detect configuration format" error for a plain one-liner instead of silently guessing it into a phantom stdio server. F-F (medium, partial): SearchAll now builds the official/popular sections from the full ranked list before truncating to `limit`, so Popular can reach its own 12-entry cap independent of the (smaller) page-size default. Deferred: no source parser populates CatalogHit.Popularity at all today, so the popularity-ranking half of this finding needs a real per-source signal (GitHub stars, Docker pulls, etc.) added across every registry source — a feature build-out, not a small/safe fix. Regenerated oas/swagger.yaml + oas/docs.go for the new ImportRequest fields (env_override, header_override, allow_paste_fallback). Verification: go build ./... clean; go vet clean; golangci-lint v2 (.github/.golangci.yml, bare and --build-tags server) clean on every touched package; go test -race on internal/registries, internal/configimport, internal/httpapi (all green) and the specific internal/server tests this change touches; frontend npm run build + npx vitest run (163 files/1460 tests) green; swift test for the touched Paste-tab/APIClient suites green. The full `internal/server` race suite (hundreds of unrelated tests) did not finish within this session due to heavy concurrent load from other sessions sharing this machine — its own touched-test subset (TestAddFromRegistry_*) passed under -race in isolation.
# Conflicts: # oas/docs.go
# Conflicts: # native/macos/MCPProxy/MCPProxy/Views/ServersView.swift # oas/docs.go
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
# Conflicts: # frontend/src/services/api.ts # oas/docs.go
The merge conflict in oas/docs.go (generated by swag from Go annotations) was resolved by regenerating from source rather than hand-merging the embedded JSON blob, keeping it in sync with the already-merged oas/swagger.yaml.
This was referenced Sep 26, 2026
Dumbris
added a commit
that referenced
this pull request
Oct 2, 2026
…s (Spec 109-m) Review round 1: the retired-name scan sees interpolated labels, symbol ids must be declared (not just mentioned), the CLI walk also checks main.go registers every group, the traceability checks hold the non-finding table rows and test-file refs to the same rules, the generator skips empty words, and the self-test refuses to build trees without a temp directory. Docs: the CHANGELOG route placeholder, the empty-query catalog description and the tools alias row. Related #1383
Dumbris
added a commit
that referenced
this pull request
Oct 2, 2026
…iew wording fix (Spec 109-m) Related #1383
github-actions Bot
pushed a commit
that referenced
this pull request
Oct 2, 2026
#1461) Spec 109-m closes the cross-surface parity work of `specs/109-ux-navigation-consistency`: it turns the terminology, attention, health wording, catalog order, parity matrix, contradiction register and finding traceability into automated checks, publishes the user docs and writes the release notes. There is no new route, flag or MCP argument, so the frozen tool-surface goldens and `oas/swagger.yaml` are unchanged. ## What changes for users - **Web UI and macOS:** a server the core reports as not usable no longer reads "Connected" in the last-resort status wording (a payload with neither a status nor a summary, for example an older core); the Web card, the ServerDetail header and the macOS tray first line now read "Unavailable". The tool tier and Activity view labels on the Web come from one exported table each, with the same words as before. - **Web UI:** the dead `NavBar.vue` (it still carried the retired "Dashboard" and "Repositories") is deleted. - **Docs (live after deploy at docs.mcpproxy.app):** new `cli/attention-command` and `cli/catalog-commands`; new `### Attention`, `### Review` and `### Catalog` sections in `api/rest-api`; `web-ui/dashboard` is now titled Home and describes the grouped sidebar, header, palette and redirects; `cli/status-command`, `cli/management-commands`, `cli/command-reference` and `features/registry-add` describe the declared CLI text changes; `features/needs-attention` is finally in the sidebar. - **Release notes (CHANGELOG, Unreleased):** the declared CLI table-text changes (JSON unchanged): `upstream list` STATUS shows the status label, `status` renames `MCP Endpoints` to `Endpoint & mode` and starts with a `Needs attention: N` line, `doctor` leads with the attention list and its diagnostics heading is `Diagnostics: N findings`. Deprecations: `registry search|add`, `upstream approve`, `security approve|reject`, `tools approve|reject`, `tools list --risk` and the old routes. Features: Home and one attention list, Review queue, Clients hub, catalog-first Add, grouped navigation. - **Generated types:** `contracts.ts` gains the tool approval, activity view and client presence enums (`ToolApprovalState`, `ActivityView`, `ClientPresenceState`). - **CI:** a new `No Unquarantine Callers` job (SC-005) and the native and frontend workflows now also run when a Go-owned golden they read by path changes. ## Spec requirements and audit findings closed here FR-090, FR-091, FR-092 and SC-001 to SC-012 (their proof); audit findings H2 (traceability path), C2 (traceability path), S2 and X12 (SC-005 mechanised), N7 (dead NavBar, retired names), S4 and S5 (SC-003 on every renderer), A3, N6 and N8 (SC-002 parity, `needs-attention` published), C1 (SC-008 order), N1, N4 and N5 (CLI and REST docs). Spec: `specs/109-ux-navigation-consistency` (tasks T144 to T149d, research D34). ## Tests added - Go: `TestSpec109TerminologyGolden`, `TestSpec109ContractsTSExactLines` (these fold in and delete 109-l's `attention_108_contract_test.go`), `TestSpec109RetiredNames`, `TestSpec109MCPRESTNames_*`, `TestAttentionParity_*` (runtime, REST, SSE) and `TestAttentionParityCLI`, `TestCatalogOrderParity_*` (REST, MCP, CLI), `TestUpstreamListForbiddenWords`, `TestSpec109RegisterDispositions`, `TestSpec109Traceability_*` (findings, FR coverage, acceptance index), `TestSpec109ParityMatrix*`, `TestParity109CLICellsResolve`, `TestParity109FlagUsageNamesTheValues`, `TestSpec109DocsPublished`. Each has a mutation or self-test showing it can fail. - vitest: `spec109-terminology`, `attention-parity`, `catalog-order-parity`, `sc003-forbidden-renderings`, `spec109-parity-matrix`. XCTest: `Spec109TerminologyTests`, `AttentionParityTests`, `CatalogOrderParityTests`, `Spec109ParityMatrixTests`, two tray cases in `CrossSurfaceRenderedLabelParityTests`. - Goldens and indexes: `internal/contracts/testdata/terminology.json`, `internal/runtime/testdata/attention_parity_{fixture,rest}.json`, `internal/registries/testdata/catalog_github_order.json`, `specs/109-ux-navigation-consistency/{parity-matrix,acceptance-index}.json`, `internal/httpapi/testdata/spec109_retired_allow.json`. - `scripts/check-no-unquarantine-callers.sh` with its self-test `scripts/check-no-unquarantine-callers.test.sh`. ## Decisions and deviations from the plan - `AllHealthStatuses()` is not in `internal/contracts`: `internal/health` imports it, so the golden test reads `health.StatusOrder` and `runtime.AttentionKinds()` directly. - The retired-name allowlist has one entry: the server-edition admin menu (`navModel.ts`, "Dashboard"), which the spec puts out of scope. - The dry run found two stale test paths (T011, T012) and two moved ones (T097, T125a), annotated with **Shipped as:** instead of rewritten. FR coverage counts a citation in a task line, its phase heading or the finding table. - `catalog add` takes `--env` only, not `--secret-env`; `contracts/cli.md` is corrected and the secret toggle's CLI form is `upstream add --secret-env|--secret-header`. - M17 (#1383 F-P) needed no change: `CatalogTests` already reads `internal/secret/testdata/ref_names.json` by path. - T150 (the live run) is described in `tasks.md`: the release-gate Playwright sweep and the macOS app against a scratch core were not run in this PR's session. Related #1383
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.
Stacked on #1378 (
109-a-quick-wins, still open) — this PR targets that branch, notmain, and should merge after it. Spec text lives ondocs/specs-108-109(PR #1379, also open):specs/109-ux-navigation-consistency/spec.md, FR-060–FR-067.Summary
Catalog-first "Add Server" (Spec 109-j): one aggregated catalog search replaces the old per-registry Repositories/Discover flow across Web UI, macOS, CLI and MCP, plus a Paste-source importer and a Value/Secret toggle for env vars and headers that stores secret values in the OS keyring instead of
mcp_config.json.User-visible changes per surface
Web UI
/add-servergets four tabs — Catalog (default) · Paste · Import · Manual — selected by?tab=./repositoriesredirects to/add-server?tab=catalog.CatalogSearch.vue): searches every enabled catalog source in parallel, sections into Official/Popular when the query is empty, shows title/publisher/verified/official badges, "Add to MCPProxy" → "Added ✓ · Open".PasteServer.vue): paste a JSON/TOML config, anhttp(s)://URL, or a shell command line; previews via/servers/import/json?preview=trueand fills in detected fields before anything is added.SecretToggle.vue), defaulting to Secret for secret-shaped names; Secret writes to the OS keyring and stores${keyring:<ref>}in config, never the raw value.CatalogSourcesSettings.vue) replaces the old Repositories "manage registries" half; the Repositories page (1149 lines) is deleted.macOS tray
CatalogView.swiftandPasteServerView.swift. The generic "Add Server" entry points (toolbar +, empty-state button, dashboard shortcut) open on Catalog by default.ServerBrowseView.swift("Discover servers" per-registry browse) is deleted outright, fully superseded by the aggregated catalog search. Registry source management moves to Settings → Catalog Sources (RegistriesView.swift), mirroring the Web UI.SecretFieldResolver.swift/SecretFieldToggleView.swiftport the same Value/Secret toggle and ref-name convention to native env/header fields.CLI
mcpproxy catalog search|show|add(FR-066), fanning out to every enabled source;registry search|addremain as deprecated aliases that print thecatalogequivalent.mcpproxy upstream addgains--secret-env NAME=VALUE/--secret-header 'Name: value'(FR-065), storing the value in the OS keyring and writing${keyring:<ref>}into config. Fails fast with an actionable message when the OS keyring write-gate isn't opted into (MCPPROXY_KEYRING_WRITE=1outside the tray), before any partial write.MCP
search_servers'registryargument is now optional (FR-067): omitted, it fans out across all sources through the same catalog backend and returns the FR-061 fields (title,publisher,verified,official,popularity,source) in the FR-060 ranking order, matching what REST/CLI callers already see.Secret ref-name convention (FR-065)
One shared rule for turning a field name into a keyring reference —
<server>-env-<NAME>/<server>-header-<Name>, lowercased, non-[a-z0-9-]runs collapsed to-, ≤64 chars,-2/-3… appended on collision — implemented three times (Gointernal/secret/refname.go, TypeScriptfrontend/src/utils/secretRef.ts, SwiftSecretRefName.swift) and pinned to one shared fixture,internal/secret/testdata/ref_names.json, decoded by all three test suites so the three implementations can't drift.Review rounds applied on this branch
Reviewed with zcode (GLM), the mandated external reviewer:
ce0be59d8): keyring write-gate ordering (probe now checks the gate before, not after);CatalogSearch.vueno longer treats a failed add as success; secret rollback wired into all three add surfaces on a later failure; CLIupstream addvalidates--trust-modebefore writing any secret, and rolls back partial--secret-env/--secret-headerwrites;search_serversMCP surface now carries the catalog fields;GET /catalog/searchemptiesresultswhensectionsis populated;source=filtering moved before ranking/truncation so a narrow source's matches can't be truncated away by a broader one.b1ddae539, T109/T103): macOSAddServerViewhad the catalog backend but no UI for it — addedCatalogView.swift/PasteServerView.swiftas Catalog/Paste tabs, ordered to match the Web UI; removed the macOS "Registries" sidebar item (superseded by Catalog), moved registry-source management to a new Settings → Catalog Sources tab, and deleted the old per-registryServerBrowseView.swift"Discover servers" flow outright; updateddocs/registries.mdto match.ab75d4716):secret_likename-rule respects an explicitisSecret:falseoverride, and FR-007 added/not-added scoping applies to any non-adminAuthTypeUsersession, not just agent tokens.Tests added
internal/registries/catalog_test.go,catalog_bench_test.go,rank_test.go;internal/secret/refname_test.go,keyring_available_test.go;internal/secretlike/secretlike_test.go;internal/httpapi/catalog_test.go,catalog_dto_test.go,import_preview_fields_test.go,secrets_keyring_available_test.go;internal/configimport/detect_url_command_test.go;internal/cliclient/catalog_test.go;cmd/mcpproxy/catalog_cmd_test.go,upstream_add_secret_test.go;internal/server/mcp_menu_surface_test.go,search_servers_all_sources_test.go.add-server-catalog.spec.ts,add-server-manual.spec.ts,add-server-paste.spec.ts,add-server-tab-url.spec.ts,secret-ref.spec.ts,secret-toggle.spec.ts,settings-catalog-sources.spec.ts,repositories-redirect.spec.ts.CatalogTests.swift,PasteServerFieldsTests.swift, plus the pre-existingServerBrowseModelsTests.swift/APIClientAddServerFromRegistryTests.swift(their contents test shared models, not the deleted view, and still compile).oas/swagger.yamlregenerated for the newGET /catalog/searchendpoint and import-previewenv/headersfields.How this was verified live
Rebased onto the current
109-a-quick-winstip (which had moved two review rounds ahead) and resolved one modify/delete conflict onServerBrowseView.swiftin favor of the deletion this PR intends, since the base's changes to that file (anaddedKeycollision fix) landed inRegistryModels.swiftinstead and merged in cleanly.After rebasing:
go build ./...clean;go vet-backedgolangci-lint(bare and--build-tags server, matching CI) showed only pre-existing issues in files this PR doesn't touch. Full Go suite green —internal/registries,internal/secret,internal/secretlike,internal/httpapi,internal/cliclient,internal/configimport,cmd/mcpproxy(plain and-race), andinternal/server(both with the CLAUDE.md-skipregex, plain and-tags server -race, matching the CI job exactly). Frontend:vue-tsc --noEmitclean,vitest rungreen (156 files / 1431 tests).Built the binary and ran a real local instance (scratch data-dir, non-default port, isolated from any other running mcpproxy):
GET /api/v1/catalog/searchwith an emptyqreturnedofficial/popularsections and emptyresultsas specified (FR-060); aq=githubsearch returned ranked, deduplicated results;mcpproxy catalog search/catalog showagainst the running daemon returned the same shape over the CLI;mcpproxy upstream add --secret-envfirst failed with the documented keyring-write-gate message with the gate unset, then succeeded withMCPPROXY_KEYRING_WRITE=1, writing${keyring:<server>-env-<name>}into the server's config and the server card came back quarantined by default, matching the FR-065/FR-063 contract. Cleaned up the added test server and its one keyring entry afterward.Known, documented gap (not fixed here — pre-existing scoping decision on this branch)
AddServerModal.vueis only rewired for the Web UI's global header "+ Add Server" button (TopHeader.vue, nowrouter.push('/add-server'));Dashboard.vue's two shortcut buttons andServers.vue's empty-state button still open the old modal (Import/Manual only, no Catalog/Paste). This was called out and deliberately deferred in the commit that added the page (466ab2e79): retiringAddServerModal.vuefully cascades into ~9 other Playwright specs (protocol detection, duplicate-endpoint, trust-mode) sized for their own PR. One concrete, verified consequence:e2e/web-ui-sweep/visual-a11y-sweep.spec.ts'sthe Add Server modal takes focus, traps Tab and closes on Escapetest drives the header button expecting the modal and now fails, since that button navigates away instead — confirmed live (./scripts/run-web-smoke.sh, 28/29 passed, this one failing, 2 skipped for no fixture upstream). This spec is the release gate's advisoryweb-ui-sweepjob, not a required check, and is unmodified by this PR. Left as-is rather than expanding this PR's surface; flagging for a follow-up (retireAddServerModal.vueeverywhere, migrateDashboard.vue/Servers.vueto/add-server, update/retire the dependent specs).Notes
109-a-quick-wins; this PR is the full catalog/add-server slice (109-j) — 109-k (activity scope filters) is a separate, independent branch.