Repository navigation
fix(webui): stop the UI asserting states it was never told (v0.65.0 UX audit) - #1216
Merged
Merged
Conversation
Audit F11: typing "React documentation" into the Web UI search returned
nothing, while "documentation" returned Context7. The page then said only
"No matching tools", which cannot be told apart from a typo, an empty
catalogue, or a server still sitting in quarantine.
Two causes, both in frontend/src/views/Tools.vue:
1. The filter matched the WHOLE query as one contiguous substring of a
single field (name OR description OR server_name). "react documentation"
is never a substring of any one field, so it matched nothing. Now each
whitespace-separated term is matched independently: AND across terms, OR
across fields. This is a strict superset of the old behaviour -- if the
whole query was a substring of a field, so is every one of its terms --
so no result that matched before can disappear. Still a substring filter,
NOT BM25: see "deliberately left alone" below.
2. The empty state was generic. When a search ran it now names the query,
the scope it actually covered (how many tools, across how many servers,
and whether disabled tools were in scope), a concrete next step ("try
fewer words"), and -- only when there are any -- how many quarantined
servers are not listed here, with a link to Servers. Quarantined servers
contribute zero tools to GET /api/v1/tools (internal/httpapi/server.go,
#1064) and quarantine is on by default for new servers, so a first-run
developer was searching a silently truncated catalogue with no hint of it.
The non-search empty states are unchanged apart from dropping the words
"or search query" from a branch that only renders when the search box is
empty.
Larger than the one-line filter fix, and deliberately so: reporting
stats.total would have LIED whenever another filter was also narrowing the
list (click the "Disabled" stat card, then search, and it would claim to
have searched the whole catalogue). So the filter chain is split into
searchScope (every filter except the search box) and filteredTools (the
search applied to it). All the filters are ANDed, so moving the search to
the end leaves the result identical; the empty state's numbers are now
literally true. Same reason for suppressing the "disabled tools included"
clause when a status filter excludes them.
Assumption documented per zero-interruption: a whitespace-only query used
to match every tool whose description contains a space; it now matches
everything (terms is empty, [].every() is true). More sensible, and not a
case anyone types deliberately.
Verified:
- cd frontend && ./node_modules/.bin/vitest run tests/unit/tools-search-multi-term.spec.ts
-> 8 passed. Run BEFORE the fix it was 4 failed / 2 passed, failing for the
right reasons: the multi-term query rendered no tools table, and the
tools-empty-search block did not exist. The two that passed pre-fix are the
no-regression cases (single-word query still matches; a query with one
unmatched term still excludes), so the spec is not vacuous.
- cd frontend && ./node_modules/.bin/vitest run -> 109 files, 1183 tests passed.
- cd frontend && npm run type-check -> exit 0, no output.
- cd frontend && npm run build -> exit 0 (the INEFFECTIVE_DYNAMIC_IMPORT notice
is pre-existing and concerns stores/auth.ts, untouched here).
- go build ./... -> exit 0. No Go package changed, so no go vet / golangci-lint
run was applicable.
Deliberately left alone:
- Wiring the UI to BM25. The UI does not call /api/v1/index/search at all --
api.searchTools() in frontend/src/services/api.ts is dead code, orphaned when
1ce385c folded the /search view into Tools. Pointing the box at the ranked
index is a product decision (that endpoint returns a different population and
none of the columns this table renders, and it would not compose with the
server/status/risk/approval chips), so it is reported, not built.
- api.searchTools() is NOT deleted even though the frontend no longer uses it:
the endpoint is still consumed by cmd/mcpproxy-tray and cmd/release-gate, and
removing it is unrelated churn.
- The ranking algorithm (internal/index/bleve.go) and every backend gate. The
quarantine exclusion is untouched -- this change only NAMES it.
- Header placeholder "Search tools, servers..." still only searches tools;
recommended as a follow-up, not fixed here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…w server
UX audit F07. Adding a remote HTTP server showed a disabled Docker isolation
toggle that can never apply, a permanently-disabled "Coming soon" placeholder,
and then dropped the user back where they started — while the activation work
(connect, scan, review, approve) waited on a page they had to find themselves.
WHAT CHANGED
1. AddServerModal: the Docker Isolation toggle is now `v-if="type === 'stdio'"`
instead of rendered-with-`:disabled`. Isolation is structurally impossible
for a URL upstream — internal/config/isolation_resolve.go resolves
IsolationSourceNotStdio whenever Command is empty — so the disabled control
was truthful but pure noise. This is PRESENTATION ONLY: both existing state
guards stay untouched (the watcher on formData.type that clears `isolated`,
and the submit-side write that lives inside the stdio branch), so
isolation_json still cannot reach an http add. The URL/stdio field groups
were already protocol-conditional; nothing else in the form is.
2. AddServerModal: removed the "Idle on Inactivity" toggle. JUDGMENT CALL, and
the one place I went past "hide" to "delete" — this is not a feature being
removed. It was permanently `disabled`, labelled "Coming soon", and the only
three references in the repo (v-model, form-state field, reset) were all
inside this one file; handleSubmit never read it and no backend field of
that name exists anywhere in internal/. It is exactly the "options described
as future behaviour" the audit flagged. Reverting this hunk alone is safe if
the placeholder is wanted as a roadmap signal.
3. Post-add hand-off. The modal's `added` event now carries the server name
(`emit('added', formData.name)`) and the three navigating consumers —
Dashboard.vue, Servers.vue, TopHeader.vue — push to serverDetailPath(name),
which is the page that already renders the quarantine banner, the scan
summary and the Approve action.
The navigation deliberately lives in the CONSUMERS, not in handleSubmit:
OnboardingWizard.vue also listens to `added`, and a push from inside the
modal would unmount the wizard (Dashboard.vue mounts it) mid-flow, possibly
before its markServerCompleted() lands. The wizard's handler is unchanged
and keeps its zero-arg signature and its own completion toast.
The bulk/import path (handleImport) keeps its bare `emit('added')`. A bulk
import has no single destination, so an absent name is what keeps those
consumers refreshing the list in place, exactly as before.
Dashboard.vue returns before its generic "Server Added" toast on the
navigating path. That is not tidying-while-here: the modal already toasts
"<name> has been added successfully", toasts are rendered globally, so the
second one would have followed the user onto the detail page. The import
path still gets that toast.
ASSUMPTIONS DOCUMENTED (zero-interruption policy)
- Navigating away from /servers after an add is a behaviour change on that
surface too, not only on the dashboard: the user loses the list they were
reading. That is the requested hand-off, applied consistently.
- serverDetailPath() is used rather than a hand-built path or params, because
MCP-1112 (#598) records that an unencoded '/' in an official-registry name
splits the route and falls through to the 404 catch-all.
- The emit is order-sensitive and commented as such: handleClose() blanks
formData.name, so moving the emit below it would silently publish '' and push
consumers at "/servers/". A test pins this.
VERIFIED
- frontend/tests/unit/add-server-modal-protocol-and-handoff.spec.ts (new, 7
tests) and frontend/tests/unit/add-server-handoff-consumers.spec.ts (new, 3
tests) were written first and RUN first: 5 of 7 and 2 of 3 failed against
unmodified source with real assertion failures ("expected 'Enabled...Docker
Isolation...' not to contain 'Docker Isolation'", "expected [] to deeply
equal [ 'fs-server' ]", "expected 'servers' to be 'server-detail'"), not
compile errors. The consumer spec was re-run against a stashed-source tree to
confirm it bites on HEAD.
- cd frontend && ./node_modules/.bin/vitest run — 110 files, 1185 tests passed.
- cd frontend && npm run type-check — clean.
- cd frontend && npm run build — clean (vue-tsc + vite).
- go build ./... — clean. No Go, no API payload, no contracts.ts, no swagger
regeneration: this change touches only Vue templates and handlers, so `go
vet` and golangci-lint have nothing new to look at and were not run for
Go changes that do not exist.
DELIBERATELY LEFT ALONE
- The trust-mode prose (TrustModeSelector.vue, utils/trustMode.ts). The audit
called it verbose and it is, but spec 088 FR-002 deliberately requires each
mode to state BOTH what it does to tool changes and what it does at
admission. Shortening or collapsing it is a product decision with a
security-relevant blast radius, not a UI patch.
- Form layout, field order, spacing, styling. The isolation block is hidden in
place; nothing is reordered.
- The Import tab, its preview/validation, and the canonical-path quick import.
- OnboardingWizard.vue and its "Review and approve from the Servers page"
toast — the wizard owns its own completion contract.
- Quarantine defaults, trust_mode derivation and the isolation_json payload
shape. No security, auth or scope predicate is weakened or even read.
- The existing assertion at add-server-trust-mode.spec.ts:104 that the Docker
toggle is present: it mounts with the stdio default, so it stays green and
was not "fixed".
- The Playwright web-UI sweep was NOT run (it needs a live core; the e2e script
blanket-kills other sessions' instances). Its Add-Server tab-trap test is
unaffected by construction: it filters `input:not([disabled])`, so dropping a
disabled input cannot change its count, and it never switches protocol or
submits.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ound-trip
UX audit F13. The wizard's Verify tab showed "Round-trip verified" — a green
finish-line state — off a single boolean, `first_mcp_client_ever`. That flag is
stamped from the MCP `initialize` hook (internal/server/mcp.go AddAfterInitialize
-> RecordMCPClientForActivation), i.e. from a transport handshake, before any
tool is listed let alone called. All three suggested prompts targeted mcpproxy's
own built-ins, which are recorded as `internal_tool_call` and never stamp
`first_real_tool_call_ever` — a backend test already pins that for
retrieve_tools. So a user who had never seen an upstream tool run was told the
product was verified.
WHAT CHANGED (frontend only, three files)
- OnboardingWizard.vue Verify tab now shows two separately-labelled milestones
instead of one:
1. "AI client connected" (same condition as before, honest words) — states
the handshake proves the wiring, and says so explicitly while the second
milestone is outstanding.
2. "First upstream tool call" (new, `data-test="verify-first-upstream-call"`)
— satisfied from `first_real_tool_call_ever`. Pending renders as the same
neutral 📡 idiom the tab already used, never as an error.
- One suggested prompt added, leading the list, that actually dispatches to an
upstream (retrieve_tools + call_tool_read), so at least one prompt exercises
what milestone 2 measures. The other three are unchanged.
- The new state is read from `GET /api/v1/status`, which ALREADY serves the whole
activation block to an admin caller (internal/httpapi/server.go: omitted only
for scoped agent tokens, pinned by scope_round11_test.go). No backend change,
no `make swagger`, no contracts.ts regeneration, no new config field.
api.ts `getStatus()` is only widened, optionally, to admit what the endpoint
already sends; its two existing callers are untouched.
- Fetched in the existing `onOpened()` Promise.all and refreshed on the existing
5s Verify-tab poll. No new timer.
DELIBERATE WORDING: "ran through mcpproxy and returned a result", not
"succeeded". `recordRealToolCallSuccess()` (internal/server/mcp.go:2613) is
called one statement BEFORE the isError classification at :2621, so an upstream
answering isError:true still sets the flag. The copy must not overclaim twice.
DELIBERATE FAILURE DIRECTION: a missing/failed `activation` reads as "not yet".
`fetchActivation` swallows its own errors — load-bearing, because the five
pre-existing wizard specs mock `@/services/api` with an explicit method list
that has no `getStatus`, and an unguarded throw would abort `onOpened()`.
LEFT ALONE ON PURPOSE
- The Verify tab's green tick (`tabs[].complete`), `incomplete_tab_count` and the
sidebar badge still key off the handshake only. Tying them to the upstream call
would turn an informational state into a gate and leave headless / LAN-server
installs permanently badged — the opposite of the "don't block a user who only
wants the proxy running" constraint. Nothing here gates anything; the wizard
still closes from every tab.
- No new wizard tab or step; no restyle of the panel, prompt cards or activity
list; no SSE.
- Milestone 2 renders only once milestone 1 is satisfied. Assumption made without
asking (zero-interruption): before any client has connected, "no upstream tool
call yet" is trivially implied and only adds noise to the existing
"Waiting for your first request" empty state.
- KNOWN BACKEND GAP, NOT FIXED HERE: `first_real_tool_call_ever` has exactly one
call site — the call_tool_* variant handler. Real upstream calls made through
the direct tool surface (mcp_routing.go emitActivityToolCallCompleted) or as
code_execution sub-calls never stamp it, so users who changed routing mode will
see milestone 2 stuck at "not yet". Bounded: the default routing mode is the
stamping path, and the row gates nothing. It is a separate defect needing its
own Go tests; folding it in would blow the diff budget. Filed as follow-up.
- Rejected: deriving the state from the already-loaded recent-activity list
(`type === 'tool_call'`). It holds only the last 5 rows and the log is pruned
at 7 days / 10 000 records, so the row would flicker and then silently regress.
The lifetime BBolt flag is the only correct source for an "ever" state.
VERIFIED
- New spec at frontend/tests/unit/onboarding-wizard-verify-states.spec.ts (the
only path the runner executes). Written first; all 7 cases failed against the
unmodified component for the right reasons — the panel really rendered
"Round-trip verified", neither milestone row existed, and the prompt list had
no upstream-dispatching entry. Green after the change.
- Bite-checked the anti-regression assertion by neutering the conditional caveat:
1 failed / 6 passed, then restored.
- Full frontend suite: 109 files, 1182 tests passed.
- `npm run type-check` (vue-tsc --noEmit) clean; `npm run build` clean;
`go build ./...` exit 0. No Go files touched, so no go vet / golangci-lint
package to run.
- Not run: the Playwright web-ui sweep. It only locates [data-test="close-wizard"]
to dismiss the wizard and never opens the Verify tab, so it would prove nothing
about this change. The vitest spec is the guard.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cross-model review (opencode gpt-6-astra) findings on 74cfd74. 1. The new empty state gave advice for the wrong control whenever the other filters had already reduced the scope to nothing. Reproduced from a live mount: with an all-enabled catalogue, click the "Disabled" stat card and then search, and the page rendered No tools match "kubernetes" Searched 0 tools across 0 servers. Every word has to match — try fewer words. Nothing was searched, so no wording of the query can help; the status filter is the cause. The zero-scope case now says so instead. The reviewer independently enumerated the same boundary (searchScope.length === 0) across the filter/quarantine combinations. 2. The rationale comments in Tools.vue and the spec cited "react documentation" as the example the fix repairs. It does not: no field of the sample catalogue contains "react", so that query returns nothing before and after. Replaced with "context7 documentation", which is the case the test actually asserts -- terms spanning server_name and description, matched by neither field alone. 3. The multi-word zero-result test asserted only that the table was absent, which an implementation rendering nothing at all would also satisfy. It now asserts the explanatory empty state is reached and quotes the query. New guard added: "does not blame the query when the other filters left nothing to search". Verified to bite -- removing only the new v-if leaves it the single failure in the file (1 failed / 8 passed). Verified: - cd frontend && ./node_modules/.bin/vitest run tests/unit/tools-search-multi-term.spec.ts -> 9 passed. - cd frontend && ./node_modules/.bin/vitest run -> 109 files, 1184 tests passed. - cd frontend && npm run type-check -> exit 0. npm run build -> exit 0. - go build ./... -> exit 0. frontend "npm run lint" is a no-op stub in this repo. Review findings rejected, with reasons: - "A character-splitting predicate would pass the suite." Argument about a hypothetical implementation, not a defect in this one; no realistic regression writes that. - "Tests 1/2 prove only that echo's DESCRIPTION is absent, not its row." The table renders {{ tool.description || '—' }} in a cell, so the absence of that string does establish the row is gone. - "'2 tools' would also match '12 tools'." No such string is reachable from the fixture; test 7 already anchors the phrasing as "1 tool across 1 server". - "Test 5 does not prove the count exceeds one." The number comes from serversStore.serverCount.quarantined, a store getter with its own coverage. - "Both fixtures are enabled, so the 'disabled tools included' clause is asserted as a label, not as behaviour." True, but adding a disabled fixture shifts the scope counts asserted in two other tests; churn out of proportion to the risk on a claim that is about the filter, not the data. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g is blind Review round on 34f4047. That commit split the Verify tab's dishonest "Round-trip verified" into two honest milestones — but the second milestone reintroduced the same class of untruth in the opposite direction, and its own commit message says so: `first_real_tool_call_ever` is stamped at exactly one site (internal/server/mcp.go:2613, the call_tool_* handler), so under routing_mode=direct or code_execution — neither of which stamps it — the new row told a user who had made dozens of real upstream calls "No upstream tool call yet", forever. Shipping a green claim off a handshake and shipping a red claim off a blind flag are the same defect wearing different colours. The backend gap stays open (it needs Go tests and a separate diff). What changes here is that the UI stops making a claim it cannot substantiate. WHAT CHANGED (same two frontend files; no Go, no swagger, no contracts.ts) - `firstRealToolCallEver` becomes tri-state `boolean | null`. `null` is "we cannot tell": no activation block on the wire, or the fetch failed. - New `routingMode` ref, read from the SAME `GET /api/v1/status` response that already carries `activation` — no extra request, no new timer. The Go handler resolves it (servedRoutingMode, internal/httpapi/server.go:1160), so it is always one of retrieve_tools | direct | code_execution. - New `upstreamCallState` computed with three outcomes: satisfied — the flag latched. A truthful LIFETIME fact under any routing mode, so it is still shown after a mode switch. pending — flag false AND routing_mode is the one that actually stamps it. unknown — everything else. The row does not render at all, and the milestone-1 caveat ("It does not yet mean a tool has run") is withheld with it, since that phrasing asserts the same negative. - `fetchActivation` no longer collapses a missing `activation` block to false. The flag is monotonic in the core, so once seen true it early-returns rather than letting a later poll that omits the block un-latch a reached milestone. routingMode is assigned BEFORE that return, so it never goes stale. RENDERED AND READ, all five states: default mode / not yet → pending row, caveat shown (truthful) default mode / done → "First upstream tool call …", caveat retired direct mode / flag false → no row, no caveat ← the fix direct mode / latched → satisfied row (lifetime fact still true) no activation block → no row, no caveat ← the fix VERIFIED - Spec grows 7 → 11 cases, still at frontend/tests/unit/ (the only path the runner executes). The 4 new/rewritten cases cover direct + code_execution blindness, the latched-then-omitted poll, and a failed fetch. - Bite-checked, not assumed: reverting the component to 34f4047 fails exactly those 5 cases; reverting to main fails all 11. Separately confirmed the absence-only assertion is not vacuous — neutering the milestone block so it renders nothing fails 6 of 7 on empty-DOMWrapper, not silently passes. - Full frontend suite: 109 files, 1186 tests passed. - `npm run type-check` (vue-tsc --noEmit) clean; `npm run build` clean; `go build ./...` exit 0. No Go touched. - Cross-model review (opencode, gpt-6-astra) raised the blind-routing-mode false negative and the activation-block reset independently; re-review of this diff returns no blocking findings. REJECTED FROM THAT REVIEW - Making the suggested prompts mode-aware. The three pre-existing prompts are annotated with default-mode tool names too; that is a pre-existing issue and fixing the whole list is outside a minimal review round. - Guarding fetchActivation's assignment with the onOpened() `seq` staleness check. Every other fetcher in this component (fetchClients, fetchRecentActivity) assigns without it; the flag is monotonic, so a reordered response cannot produce a wrong terminal state, and the 5s poll re-converges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cross-model review (opencode gpt-6-astra) caught a TOCTOU in the new post-add hand-off: handleSubmit snapshotted the name into `serverData`, awaited `serversStore.addServer(serverData)`, then re-read the reactive `formData.name` to build the `added` payload. The name input carries no `:disabled` binding and Cancel stays live while `loading` is true, so the field is editable across that await. Typing in it mid-flight made the modal emit a name the request never carried, and the consumers push straight at it — landing the user on ServerDetail's "Server not found" for a server that was never created, while the one they did create sat unopened. Emit the `serverData` snapshot instead. This also dissolves the fragile ordering coupling to `handleClose()` (which blanks `formData.name`), so the guard test is reworded to state what it actually protects now. Test bites: 'emits the name that was SENT, not one edited while the add was in flight' fails on the previous commit with `- "alpha" / + "beta"` while asserting the request carried 'alpha'. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s "absent"
UX audit F08/F10/F12, all on the server detail screen. One theme: the page
said "nothing here" in three places where the truth was "deliberately held
back", "intentionally off", or "nothing was written". Copy and one error
branch; no security predicate, endpoint or contract is touched.
F08 — Tools tab empty state (frontend/src/views/ServerDetail.vue)
A server quarantined at admission is connected:false and its tools are kept
out of both the state snapshot and the search index by design
(internal/runtime/tool_quarantine.go). `GET /servers/{id}/tools` therefore
returns [], and the tab rendered the generic "No tools available / Server
must be connected to view tools." — the same words a broken server gets —
while the Security tab was asking the operator to review those very
definitions. Blaming the disconnection is doubly misleading: the
disconnection is part of the quarantine. The empty state is now
quarantine-aware ("Tools withheld for review") with a button through to the
Security tab, reusing the existing openSecurityTab handler. Same markup and
classes; two strings and one button.
The sentence deliberately names NO count. `quarantine.pending_count` counts
tools awaiting review, which is not the count of tools being withheld:
QuarantineServer (internal/runtime/lifecycle.go) purges the index but keeps
the server's existing approval records, so quarantining a trusted server
with 20 approved tools and 2 newly discovered ones reports pending_count 2
while all 22 are withheld. No payload field carries the withheld total for a
quarantined server, and a sentence naming the wrong number is the exact
class of bug this commit exists to remove.
F10 — Health and Admin-state tiles (frontend/src/views/ServerDetail.vue)
internal/health/calculator.go deliberately reports a quarantined or disabled
server as level "healthy" ("quarantined is intentional, not broken"). The
Health tile prints the LEVEL, so it rendered a green "Healthy" on precisely
the servers no client can reach — and could pair it with a "Sign-in
required" sub-line, a combination only reachable on a quarantined/disabled
server. The tile's one word now leads with the admin state ("Blocked" /
"Off"); the observed health keeps its own vocabulary on the sub-line, which
already prints health.summary. Neither reads text-success any more (both
fall to the existing neutral default, so healthLevelTone needed no edit).
The hard-coded "set by you" caption under Admin state was false for the
DEFAULT quarantine-on-add path, so a quarantined server now reads "awaiting
your review".
F12 — missing per-server log file (internal/server/server.go)
GET /api/v1/servers/{id}/logs returned HTTP 500 "log file not found …
(server may not have run yet)" for any server with no log file. That claim
is false: the file is created lazily by lumberjack and level-gated
(internal/logs/logger.go), so a connected, healthy server that logged
nothing at or above the configured level has no file at all — on any
transport, not just remote ones (internal/upstream/core/client.go creates a
per-server logger for every upstream). The ENOENT branch now returns an
empty (non-nil, so it marshals as []) result and the UI falls into its
normal empty state, whose sentence now says what the file actually holds and
points at Activity for tool calls. Every genuinely broken case — unknown
server, path escape, unreadable file — still errors.
Assumption documented per the zero-interruption rule: F10's tile change
re-mixes the two axes the earlier F11 fix separated. The admin fact wins the
tile's single word because that word is the answer to "can my client use
it?", and the health fact is not lost — it stays on the sub-line directly
beneath. The narrower alternative (fix only the colour, keep the word
"Healthy") would have left the tile saying the opposite of the truth.
Verified
go build ./... clean
go vet ./internal/server/... clean
golangci-lint (v2, .github/.golangci.yml) ./internal/server/... 0 issues
go test ./internal/server -run TestGetServerLogs_ -v 2 passed
(and again under -race)
go test ./internal/server -skip 'E2E|Binary|MCPProtocol|…' ok 184s
go test ./internal/httpapi/... ok
frontend vitest run (whole suite) 110 files / 1188 passed
frontend npm run type-check clean
scripts/run-web-smoke.sh (Playwright, with mcpfixture) 30 passed, incl.
"server detail opens and exposes the security tab"
Live, isolated core on :18099 with a quarantined http upstream (the exact
audit shape: quarantined, connected:false, quarantine:null) — page reads
"Blocked / Quarantined for review", "Quarantined / awaiting your review",
"Tools withheld for review", and the CTA opens the Security tab; a
connected fixture server with 2 tools and no log file returns
200 {"logs":[],"count":0} where it previously returned the 500.
Cross-model review (opencode, github-copilot/gpt-6-astra) plus a local
bite-check of every new behaviour found three things, all fixed here:
* the `toolsEmptyHeading` change had NO positive assertion anywhere —
reverting it to a constant left the suite 13/13 green. Both quarantined
cases and both non-quarantined controls now assert the heading text.
* `pending_count` could understate the withheld set (above); the count is
gone, and a test pins that the sentence contains no digit at all even
when the payload carries pending/changed/blocked counts.
* the "Degraded/Unhealthy" preservation control only exercised Unhealthy;
it now covers both, so the new admin-state switch sitting in front of
the level switch cannot mask either.
Rejected in review: keying the new copy off `adminStateLabel` rather than
`server.quarantined` (both derive from the same serverStatus.Quarantined —
runtime.go:2465 vs :2616, server.go:1510 vs :1452 — so they cannot
disagree), and dropping the Logs paragraph as out of scope (F12 IS the logs
finding).
Each fix was driven by a test that was run and observed to fail first: the
Go test failed with the literal "server may not have run yet" error, and the
two frontend specs failed on 'Healthy' != 'Blocked' / 'Healthy' != 'Off' and
on the missing data-test hooks. Every new behaviour was then re-checked by
neutering it in the working tree and confirming a red suite: heading,
uncounted sentence, CTA, admin-state caption, health-tile switch, and the Go
ENOENT branch each turn at least one test red when reverted. Both frontend
specs also carry assertions that PASSED before the change (the
enabled-healthy tile, the disconnected and connected non-quarantined copy),
so they are not vacuous.
Deliberately NOT done
- Not unhiding the held-tool banner/list for a server-quarantined upstream.
frontend/src/utils/toolQuarantine.ts:32 returns [] on purpose, documented
and covered by tests/unit/tool-quarantine-banner.spec.ts. Reversing it is
a product decision (it would offer per-tool Approve buttons that leave the
server blocked anyway), not a bug fix.
- Not adding an endpoint to surface the scanned definitions. For a server
quarantined at admission and never approved they are genuinely
unretrievable: approval records are never written (the only writer,
checkToolApprovals, sits behind the serverEligibleForIndexing guard in
internal/runtime/lifecycle.go), and the scanner's tools.json is a deleted
temp dir. Populating that surface needs either persisted scan definitions
or a REST twin of MCP inspect_quarantined — new security surface, so a
spec, not this patch. The copy now tells the truth in the meantime.
- Not touching the Tools-overview Enabled/Pending-Approval counters
(frontend/src/views/Tools.vue, internal/httpapi/server.go) — that is the
overview page, not this screen.
- Not touching FlaggedToolsPanel's "Not in the current tool list" label; for
a quarantined server it is literally accurate.
- Not changing the per-server log level gate or making upstreams log more.
Growing every user's log directory to fix a wording bug is the wrong
trade; the wording was the bug.
- No contracts.ts edit, no `make swagger`: no API payload field changed, and
the handler's annotation already documented 200 + GetServerLogsResponse.
- No test for the Logs empty-state sentence: it is a static string on a
pre-existing, unconditional branch with no logic to bite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… log fallback Two findings from reviewing the four merged UX branches together — neither is visible from any single branch, which is why the per-branch reviews missed them. 1. A live probe against a built instance: a quarantined stdio server whose command does not exist reports status="error", connected=false and last_error="Command ... not found on the spawn PATH", yet health.level comes back "healthy" with action "approve" — internal/health/calculator.go returns early on Quarantined before any connection check. Every fault alert on the server page is also deliberately suppressed while quarantined (issue #1076), because mcpproxy dials quarantined servers for scanning and that attempt reliably leaves a spurious error behind. The net effect was that the Tools tab said "approve the server to list them" and nothing else contradicted it. Approving hands the user a second failure. The copy now names the connection blocker and points at Configuration, only when the server is actually disconnected — a connected quarantined server keeps the original sentence, pinned by a control test. Deliberately NOT changed: calculator.go. Moving the connection checks ahead of the quarantine branch would return admin_state=enabled and replace the approve action, which is wrong; and the Configuration → Health card renders the raw payload on purpose (it is the same shape the macOS tray consumes), so it should keep showing what the backend actually said. 2. The merged Go change makes a missing per-server log file return 200 with an empty list instead of an error. The macOS tray's loadLogs() read its on-disk fallback only from `catch`, so that empty success silently skipped the fallback and dropped historical local logs from the view. It now falls back on an empty result too. Verified: the two new frontend tests fail on the pre-fix tree (the target for the right reason — the control passes both ways, and a first attempt failed on a bad selector and was corrected rather than left green); full frontend suite 1221 passed / 116 files; vue-tsc clean; `swift build` in native/macos/MCPProxy completes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…(audit F06) Behind the "Authentication Required" modal every surface still renders, and three of them stated as fact things the UI had never been able to read. On one fresh install, unauthenticated vs authenticated at the same moment: before, unauthenticated Setup ✓ | MCPPROXY stopped | 0 connected | 0 tools authenticated, same box Setup 2 | MCPPROXY active | 0 connected | 0 tools The "Setup ✓" was the worst of the three: `incompleteTabCount` reads `state?.incomplete_tab_count ?? 0`, so "never fetched" and "nothing left to do" both arrive as 0, and since nothing ever moves `state` off null this was not a pre-fetch flash — it was permanent for any install whose fetch kept failing. `onboarding.loading` cannot stand in for it: false before the first fetch and false again after a failed one. Three gates, all using flags that already existed: - SidebarNav: `setupStateKnown` (`onboardingStore.state !== null`). Unknown gets the plain "Setup" label the incomplete branch already renders; pulse and badge were already gated on `setupIncomplete` and stay off. - Dashboard overview counts: gated on `serversStore.loaded`, the flag added for exactly this distinction and set in the single `applyServerList` funnel that both the fetch and the SSE path go through. Unknown renders an em-dash. - Dashboard run state: gated on `systemStore.status !== null`, the same shape as the `dockerStatus` / `quarantineEnabled` rows 40 lines below. `isRunning` falls back to false when status is null, so a demonstrably running proxy was labelled "stopped" in error red. Also corrects the auth screen's recovery instructions, which are worse than useless when stale. Verified each against the code and by running them: - `mcpproxy doctor` never prints the API key -- `cmd/mcpproxy/doctor_cmd.go` has zero matches for apikey/api_key, and a live run confirms it. Replaced with `mcpproxy status`, whose "Web UI" line is a ready-to-open URL with the key embedded even without `--show-key`, plus `--web-url` for the URL alone. - The startup log carries the key in full only on the run that generated it (`main.go` `if wasGenerated`); later starts log `api_key_prefix` masked. - The tray item is "Open Web UI in Browser" (macOS) / "Open Web Control Panel" (Go tray, Windows), not "Open Web UI"; and the macOS status item has a menu assigned, so a plain click opens it -- "right-click" was misleading. Documented assumptions (no human asked, per the zero-interruption rule): - Unknown drops the tick rather than getting its own affordance (skeleton, muted dash, "locked" treatment). Dropping it is the honest minimum; inventing a new state for the sidebar is a product call, not a bug fix. - The unknown run-state row is hidden entirely rather than showing a placeholder, matching the two sibling rows that already do this. - The bullet about `~/.mcpproxy/mcp_config.json` does not add the caveat that a key from `MCPPROXY_API_KEY` is never written to the file: whoever set that env var already has the key, and the `mcpproxy status` half of the same bullet covers them. - This is a user-visible change to a shipped Spec-046-v2 surface: the ✓ now appears slightly later (once the state is actually known) rather than immediately on mount. Deliberately NOT changed: the Spec-046 rule that "Close for now" (engaged) shows ✓ while tabs are still outstanding -- that is the documented engagement design for headless/LAN installs, not an F06 unknown-state bug. Contrary to the investigation brief, the Go/Windows tray does NOT open an unauthenticated Web UI: `OpenWebUI` takes the URL from `/api/v1/info`, socket callers get `auth.AdminContext()` (non-anonymous), and `buildWebUIURLWithAPIKey` therefore appends the key server-side. Verified live: `/api/v1/info` returns `.../ui/?apikey=<key>`. The bullet is left promising an authenticated window for both trays, because that is what both do. Tests: new frontend/tests/unit/first-load-unknown-state.spec.ts asserts all three states per surface (unknown / known-good / known-bad) so a harness that never resolves the fetch cannot pass it vacuously. Proven red first against unmodified code with behavioural failures, not missing selectors: expected 'Setup✓' not to contain '✓' expected 'AI Agents ...' not to match /0\s*connected/ expected 'AI Agents ...' not to contain 'stopped' auth-single-surface.spec.ts had frozen the stale `mcpproxy doctor` string; its assertion now names `mcpproxy status` and forbids `doctor`. Verified: full frontend suite 1228 passed (115 files), npm run type-check clean, go build ./... and make build exit 0, plus a live isolated instance on a scratch data-dir confirming both the unauthenticated and authenticated renders above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…anner gate contradicting itself UX audit F09. Adding a second upstream at an endpoint that is already configured (a second `https://mcp.context7.com/mcp` under a new name) is accepted in silence, and the first the user hears of it is the scanner reporting the NEW server as `dangerous`: two entries on one endpoint expose byte-identical tool names AND descriptions, so detect.shadowing.cross_server emits one hard-tier "possible impersonation" finding per tool. The resulting Server Detail screen then said two opposite things about those same findings. Reproduced end to end on a scratch instance (127.0.0.1:18521, own --data-dir and --config, killed afterwards): a single POST /api/v1/servers of the duplicate produced `status: dangerous, finding_counts {dangerous: 2}` from the automatic admission baseline scan, unprompted, on a config that listed only the original. POST .../security/approve without force returned `409 server has 2 dangerous (hard-tier) finding(s)`. Two halves, both frontend-only. No backend, no contract, no `make swagger` (re-ran `make build`, which runs generate-types + swagger: zero diff). 1. Add-time observation. AddServerModal's manual tab now names the configured server that already holds the endpoint being typed, as a neutral note. Submit stays enabled; nothing is merged, reused or blocked. Matching is exact string equality after trim (url for http; command AND the full args list for stdio) — case folding, trailing-slash normalisation and query stripping are each a judgement call about what "the same endpoint" means and none has been made, so the rule catches the real case (a copy-pasted URL) and cannot accuse the wrong server. It can MISS: the server list masks credential-shaped url/command/args values (internal/oauth/serverfields.go), so a secret-bearing endpoint never matches. A hint, not a guarantee. ASSUMPTION, documented rather than asked: this covers the WEB-UI DOOR ONLY. The MCP `upstream_servers add` handler, the CLI and the import path still say nothing — covering them needs a warning field on the add response (generator + swagger + a second implementation), which is no longer a diff a reviewer reads in minutes. 2. Gate wording. One render carried both "Informational — these findings do not block the tool." (FlaggedToolsPanel, mirrored in FindingChip) and "Force-approving bypasses the scanner gate." (approve dialog) — about the findings behind the 409. The first sentence was false. Its source, non-goal 9 of docs/research/tpa-scanner-inline-ui-design.md, read scan_informational.go's "drives NO gating whatsoever" as a claim about the FINDINGS; it is a claim about the TRIGGER (the baseline scan neither quarantines nor auto-approves), and the same comment says the verdict is stored "through the normal scan-summary path" — exactly what ApproveServer blocks on. It is false again under `trust_mode: scan`, where a non-clean verdict holds the individual tool (tool_quarantine.go, held_reason `scan_findings`). The panel receives neither `quarantined` nor the effective trust mode, so it now claims nothing about this server's state — only what is unconditionally true of the tier: "Dangerous (hard-tier) findings block server approval." The soft-tier chip keeps its "review-only (soft-tier)" reassurance, which is where the reassurance was actually wanted. The richer variant — pass the two facts in as props and render one of three sentences — is a new component contract and a copy decision, so it was NOT taken; the withdrawn non-goal records it as the open option. The dialogs stop saying "the scanner gate", a phrase the UI never defined, and say what force approval does: "skips the scan-based approval gate and unquarantines this server". Worded without naming findings on purpose — the same paragraph renders in the no-scan mode, where there are none, and force skips that refusal too. ScanReport's Force Approve confirmation counted the wrong number: it read raw `summary.critical` while its own gate predicate reads `finding_counts.dangerous`, so it offered to bypass the gate "despite 0 critical finding(s)" for the two findings the backend was refusing on. Both now share one computed, worded with the backend's own noun. Deliberately NOT changed: detect/checks/shadowing.go. Making the clone branch endpoint- or identity-aware is the identity-aware design the audit warned about and a maintainer decision (the file header records the prior MCP-3520 decisions on this branch). Nothing here touches isBlockingFinding, ApproveServer, the quarantine default, or the force path — security posture is unchanged. Also not addressed, and not fixed by this: after force-approving the duplicate, the ORIGINAL server flips to `dangerous` on its next scan and the two permanently accuse each other. The add-time note only reduces how often a user gets there. Verified: `cd frontend && ./node_modules/.bin/vitest run` 117 files / 1236 tests green; `npm run type-check` clean; `go build ./...` and `go vet ./internal/...` clean (no Go source touched); `cd website && ./prepare-docs.sh && DOCS_VERSION=v0.65.0 npm run build` succeeds and the corrected non-goal renders in the published llms-full.txt (the research page itself is outside the docusaurus include allowlist, so it has no HTML route). Live: the note fires on the real modal with submit enabled, the panel and the dialog now agree on one screen, and the confirm reads "2 dangerous finding(s)" against a backend that returns exactly that count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…of F06) Cross-model review (opencode gpt-6-astra) caught the one claim in the rewritten auth modal that is still false: `(Windows: "Open Web Control Panel")` attributes the Go tray's menu title to Windows. Verified: internal/tray/tray.go carries `//go:build !nogui && !headless && !linux`, release.yml copies the built `mcpproxy-tray` into the darwin tarballs (line 707), and the Homebrew formula installs it with `bin.install "mcpproxy-tray" if OS.mac?`. So a macOS tarball or Homebrew user sees "Open Web Control Panel", and only the DMG's Swift bundle says "Open Web UI in Browser". Attributing one to Windows writes a fresh false statement onto the screen this change exists to make honest — the exact defect class of audit F06. Both labels are now named without an OS claim, and the spec pins the regression: both strings present, `Windows:` absent. Proven red against the previous text (`expected … not to contain "Windows:"`). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…swap Review round 1 on 52cb1a6 (opencode gpt-6-astra + a reversion sweep over every hunk). Three defects, each with a test that fails without the fix. 1. The stdio match ignored working_dir. handleSubmit SENDS working_dir on every stdio add, so it is part of the endpoint this form describes: `node server.js` in /projects/a and the same line in /projects/b are two different programs with two different toolsets. The note named one as "already configured at this endpoint" and claimed the scan would flag them as clones of each other — both false. 2. Neither branch checked the transport, so a populated-but-inactive field matched. Reproduced in the test: a stdio server carrying a stale `url` (an entry converted from http without deleting it) was named as the occupant of a typed HTTPS endpoint. The guard added is NEGATIVE — it excludes the opposite family only — because a positive `protocol === formData.type` filter would MISS the primary case: an existing remote entry is commonly recorded as `streamable-http`, `sse` or `auto` while this modal always sends `http`. 3. ScanReport's force-approve success toast still read "despite critical findings" — three lines below the confirm() the previous commit corrected, using the same severity bucket the gate never consults. Both sentences about one action now carry the gate's own count and noun. Also: ServerDetail.vue's approve-dialog paragraph was the one changed hunk in 52cb1a6 with NO test behind it — reverting it to "Force-approving bypasses the scanner gate." left all 1236 tests green, so the half of the fix that lands on the screen F09 was reported against was free to drift back. Pinned at the surface a user reaches (quarantined server, banner Approve, read the dialog). The api mock in scan-report-force-approve-count.spec.ts stubbed `securityApproveServer` (the STORE action) rather than `securityApprove` (the api method), so the approve call was never exercised; corrected. NOT taken, from the same review: renaming `hasUnresolvedCritical` (diff noise, no behaviour), and wording the confirm's legacy fallback differently when `finding_counts` is absent — that number is what the UI's own gate predicate already treats as the blocking count, and no separate truth exists on a payload that predates the field. Verified: `cd frontend && ./node_modules/.bin/vitest run` 118 files / 1240 tests green; `npm run type-check` clean; `go build ./...` and `go vet ./internal/...` clean; `cd website && ./prepare-docs.sh && DOCS_VERSION=v0.65.0 npm run build` succeeds (175 documents) — docs/research has no HTML route under the docusaurus include allowlist, and the corrected non-goal 9 reaches build/llms-full.txt with the withdrawn wording inside its strikethrough. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Cross-model review round 1 on the integrated branch (opencode gpt-6-astra, six file-named briefs). Four of the six came back with findings; all four are the SAME defect class this PR exists to close — copy asserting something the code cannot substantiate — reintroduced by the fixes themselves. Each is pinned by a test that was run against the pre-fix tree and observed to fail with a behavioural assertion error. 1. ServerDetail — the withheld-tools caveat fired on every quarantined server. It was keyed on `!connected`, which is the DESIGNED state of a quarantined server, not evidence of a fault: the supervisor disconnects any quarantined server without an active inspection exemption and refuses to dial it (supervisor.go — ActionDisconnect on `Quarantined && !IsInspectionExempted`, ActionConnect gated on the same). So the default, healthy, quarantined-on-add case — the case the screen exists to serve — was told "approving it will not list tools on its own. Check the connection error under Configuration", and sent hunting for an error that does not exist, while approval genuinely was its whole remedy. Now keyed on `last_error`, and hedged to "may not be enough": a stale error from an earlier dial is not proof approval will fail. Configuration is still the right pointer because it renders `last_error` verbatim, which is where the #1076-suppressed fault is legible. This one is my own integration commit's defect, not one of the six branches'. 2. AddServerModal — the duplicate-endpoint note predicted a scanner verdict it cannot compute: "their tools will appear twice, and the security scan will flag each as a possible clone of the other". Endpoint equality establishes neither. A second entry may carry different credentials or a different OAuth identity — the legitimate reason to add one, as the note's own first line says — and expose a different toolset; and even on identical tools cloneDescriptions (detect/checks/shadowing.go) needs three tokens in BOTH descriptions plus 85%/70% overlap to fire, so short or empty descriptions never match. Both consequences are now conditional. 3. Tools — the zero-scope empty state had two causes and handled one. On an EMPTY catalogue (every server quarantined, or none connected: both return no tools from GET /api/v1/tools) it said "the other active filters excluded every tool ... Clear them to search the full list" with no filter set at all — advice that is both false and unfollowable. Split on `allTools.length`. 4. OnboardingWizard — "No upstream tool call yet" is a claim about the user that the flag cannot back even under retrieve_tools, the mode the `pending` state gates on. That mode still exposes code_execution (mcp_routing.go: "available but not the primary workflow"), whose sub-calls do not stamp the flag, and /mcp/all and /mcp/code are mounted unconditionally — "regardless of config" (server.go) — so a client aimed at the direct surface makes real upstream calls that never reach the stamping handler. Now "No upstream tool call RECORDED yet": a true statement about what mcpproxy measured. The same row also told the user "the prompts below search and inspect mcpproxy itself" while the very same change had added a first prompt that dispatches to an upstream — disclaiming the one prompt that satisfies the milestone. It now points at it. Also corrected two assertions in onboarding-wizard-verify-states.spec.ts that the reworded copy had made VACUOUS: they forbade the exact string "No upstream tool call yet", which no longer exists in any state, so they would have passed even with the pending row rendering. Re-pointed at the live prefix. Both kept their `exists()` assertion throughout, which is why the rename did not leave the file unguarded. Comments-only, from the same review, no behaviour change: the tri-state doc comment claimed a failed fetch yields `null`, which is true only of a fetch that has never succeeded — a later failure deliberately keeps the last value (the row would otherwise flicker off on every dropped poll, and the 5s poll re-converges). Documented what the code does. NOT taken: making that failed-fetch path actually revert to `null`. The reviewer is right that the UI then shows a stale "not yet" during an outage, but blinking the row off on every dropped poll is worse for a state that gates nothing and self-corrects in 5s. Bite-checked, not assumed: with the four source files reverted to the pre-fix tree and the new assertions in place, exactly the 5 new tests fail, each on a behavioural mismatch — expected 'ℹcontext7 is already configured at th…' to match /if they/i expected '📡No upstream tool call yet — the pro…' to contain 'recorded' expected 'Tools withheld for review…' to contain 'approve the server' expected 'Tools withheld for review…' to match /may not be enough/i expected 'No tools match "kubernetes" Nothing w…' to contain 'There are no tools to search' — not on missing selectors. Both ServerDetail cases also keep a control that passed before and after (connected-quarantined, and the two non-quarantined branches), so the new switch cannot mask either arm. Verified: frontend vitest 119 files / 1252 tests pass (1247 + the 5 new); npm run type-check clean. No Go, Swift, API payload or contracts.ts touched by this commit.
Deploying mcpproxy-docs with
|
| Latest commit: |
0f508aa
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fcb92620.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://integration-webui-ux-fixes.mcpproxy-docs.pages.dev |
Member
Author
The Windows build failure is pre-existing, not from this PR
The single failing test: Not attributable to this branch: this PR changes no file under Root cause. token := fmt.Sprintf("pid=%d\nstarted=%s\n", os.Getpid(), time.Now().UTC().Format(time.RFC3339Nano))The test acquires A and B from the same process, microseconds apart. Windows' wall clock has ~15.6 ms granularity, so A dedicated task is fixing this on 🤖 Addressed by Claude Code |
Cross-model review round 2 (opencode gpt-6-astra), on the round-1 fixes
themselves. One finding, and it is the round-1 defect one level down: the
new empty-catalogue branch replaced a false claim about FILTERS with a
false claim about SERVERS.
"There are no tools to search — no connected server is currently exposing
any" is not what an empty list means. GET /api/v1/tools returns success
with `tools: []` and `partial: true` when a server's tool fetch FAILED —
internal/httpapi/server.go sets Partial/FailedServers on a genuine fetch
error and returns everything it could still gather. So on exactly that
payload the copy asserted "there are none" where the truth was "we could
not read them": the opposite diagnosis, and it contradicted the
partial-results warning already rendering at the top of the same page.
The sentence now states only the observed catalogue and adds the partial
case as its own clause instead of guessing a cause for it.
New test covers the shape the round-1 test did not: success + empty tools
+ partial + failed_servers. Bite-checked by deleting only the new
`v-if="partial"` clause — it is then the single failure in the file
(1 failed / 10 passed), on
expected 'No tools match "kubernetes" There are…' to contain
'could not be read'
The round-1 test grew an assertion that the plain empty catalogue does
NOT carry the partial clause, so the two branches pin each other.
Verified: frontend vitest 119 files / 1253 tests pass; npm run type-check
clean. Frontend-only.
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 34048133467 --repo smart-mcp-proxy/mcpproxy-go
|
Maintainer directive: a server whose command does not exist must show the error. It did not. CalculateHealth short-circuited on `input.Quarantined` and returned level=healthy / summary="Quarantined for review" / action=approve, discarding whatever the connection state said. The Web UI compounded it: issue #1076 suppressed EVERY fault alert on a quarantined server. Both were defensible in isolation and wrong together — a quarantined stdio server pointed at a nonexistent binary reported status="error" with a full spawn failure in last_error, while the page showed a calm "approve me" and hid the diagnostic panel. Approving such a server just fails again. Being disconnected is NOT the signal, and an earlier attempt at this got that wrong: the supervisor deliberately disconnects a quarantined server and refuses to dial it (ActionDisconnect on `Quarantined && !IsInspectionExempted`, supervisor.go:505), so connected=false is the designed state of every quarantined server. A transport FAULT is different — the scanner dials them under an inspection exemption, so a missing binary or dead host is genuinely observable. The gate is therefore state=="error" && last_error != "". The admin contract is deliberately unchanged: still admin_state=quarantined, still action=approve, so the review flow, the tray's quarantine handling and IsHealthy's ordinary-quarantine reading all keep working. Only the level and the summary stop claiming the server is fine. This is why the connection checks were NOT simply moved ahead of the quarantine branch — those arms return admin_state=enabled and replace the approve action, which would be wrong. The UI change is the same narrowing: quarantineSuppressesFaultAlerts now follows health.level instead of blanket-suppressing, so #1076's noise stays hidden and a real fault surfaces. Tests written first and confirmed failing for the right reason (level "healthy" want "unhealthy"; empty detail; summary still "Quarantined for review"). Both controls pass in both directions: an ordinary quarantined server across states "", "disconnected" and "idle" stays healthy and calm, and a disabled server with a stale error keeps its intentional reading. Verified live on a built instance: broken-stdio now returns level=unhealthy / admin=quarantined / action=approve with the spawn failure in detail, while a working quarantined server is untouched at level=healthy / "Quarantined for review". The page renders the full MCPX_STDIO_EXIT_BEFORE_INITIALIZE panel with its cause, remediation, log-tail fixer and docs link, and still shows the quarantine banner with Run scan / Approve. go test ./internal/health/ ok · ./internal/server/ ok 198s · golangci-lint v2 0 issues · vitest 1255 tests / 120 files · vue-tsc clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 8, 2026
Dumbris
added a commit
that referenced
this pull request
Sep 9, 2026
…Ready (#1228) TestGetServerLogs_MissingFileReturnsEmptyNotError (#1216) flaked on a loaded machine during the full `go test -race ./internal/server/...` run with "server not found: never-logged", while passing every time in isolation. CAUSE. newLogsTestServer registered the upstream client by calling UpstreamManager().AddServerConfig at construction. Background initialization then runs LoadConfiguredServers, which snapshots the manager's clients, diffs them against cfg.Servers and removes the difference in goroutines. A client added before that diff is on the removal list. On an idle machine the test finished first; under load the removal landed in between. The re-register-at-call-site pattern from diagnostics_fixers_test.go (ensureFixerClient, #1218) is necessary but NOT sufficient here: it fixes "server not found", but the removal's own "Disconnecting from server" line creates the per-server log file this test asserts is absent, so the test still failed 2/100 under -race with `Should be empty, but was [...]`. The fixer tests never hit that because they seed the log file themselves. FIX. Wait for the runtime to report PhaseReady before registering. The phase flips only after LoadConfiguredServers has taken its snapshot, so a client added afterwards is never on a removal list, and the supervisor's periodic reconcile only removes names it found in a config snapshot. The call-site re-registration (ensureLogsClient) is kept as well, mirroring the fixer tests; re-registering an unchanged config keeps the existing client and logs nothing. Assertions are unchanged. REPRO. A 3s sleep between construction and the call fails on the old fixture and passes with this change. Verified: go test -race -count=5 -run 'TestGetServerLogs_' ./internal/server ok same with -count=50, twice, under 6 CPU hogs 200/200 golangci-lint (.github/.golangci.yml) 0 issues
3 tasks done
Dumbris
added a commit
that referenced
this pull request
Sep 24, 2026
…ecks (#1358) Dashboard's usage-view first-run CTA used opacity-60 on inherited text color instead of the themed base-content token, producing low-contrast text against bg-base-200 in some themes. Switch to text-base-content/60. The web-ui-sweep servers-list and Activity/Usage checks assumed a non-empty fleet; a sweep run without a fixture upstream renders the first-run empty states instead of the KPI tiles and usage tile (#1049/#1216), which made those two checks fail. Anchor on whichever state the instance should show and assert accordingly instead of assuming a populated fleet.
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
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.
What this is
Fixes for the findings of a first-time-developer UX audit of MCPProxy v0.65.0. One theme runs through nearly all of them: the Web UI stated things it had not been told, or described a state with words that meant the opposite of the truth. A green "Healthy" on a server no client can reach. A "Round-trip verified" finish line stamped off a transport handshake. A "Setup ✓" that was really "we never fetched". "No tools available — server must be connected" on a server whose tools were deliberately withheld for security review.
Almost all of it is copy and render-gating in
frontend/src/. There is exactly one backend change (7 lines ininternal/server/server.go) and one Swift change that follows from it.This is an integration of six focused branches, each pushed separately if anyone would rather review them one at a time:
fix/webui-readiness-and-review-surface,fix/webui-search-expectations,fix/webui-add-server-handoff,fix/webui-setup-verify-honesty,fix/webui-auth-and-loading-state,fix/duplicate-endpoint-clarity.The findings, and what changed
F08/F10/F12 — the server detail screen said "absent" where the truth was "withheld", "off", or "nothing written"
Health tile (F10).
internal/health/calculator.godeliberately reports a quarantined or disabled server as levelhealthy— "quarantined is intentional, not broken". The tile printed that level, so it rendered a green Healthy on precisely the servers no client can reach, and could pair it with a "Sign-in required" sub-line, a combination only reachable on a quarantined/disabled server. The tile's single word now leads with the admin state (Blocked / Off); the observed health keeps its own vocabulary on the sub-line beneath, which already printedhealth.summary. Neither readstext-successany more. The hard-coded "set by you" caption under Admin state was false for the default quarantine-on-add path, so a quarantined server now reads "awaiting your review".Tools tab empty state (F08). A server quarantined at admission is
connected:falseand its tools are kept out of both the state snapshot and the search index by design (internal/runtime/tool_quarantine.go).GET /servers/{id}/toolstherefore returns[], and the tab rendered the same generic "No tools available / Server must be connected" a genuinely broken server gets — while the Security tab was asking the operator to review those very definitions. It is now quarantine-aware ("Tools withheld for review") with a button through to the Security tab. The sentence deliberately names no count:quarantine.pending_countcounts tools awaiting review, which is not the count of tools withheld (quarantining a trusted server with 20 approved and 2 new tools reports 2 while all 22 are withheld), and no payload field carries the real total.Missing per-server log file (F12) — the one backend change.
GET /api/v1/servers/{id}/logsreturned HTTP 500"log file not found … (server may not have run yet)"for any server with no log file. That claim is false: the file is created lazily by lumberjack and is level-gated (internal/logs/logger.go), so a connected, healthy server that logged nothing at or above the configured level has no file at all — on any transport. The ENOENT branch now returns an empty (non-nil, so it marshals as[]) result and the UI falls into its normal empty state. Every genuinely broken case — unknown server, path escape, unreadable file — still errors.F11 — tool search blamed the query for what the filters had done
Typing "React documentation" returned nothing while "documentation" returned Context7: the filter matched the whole query as one contiguous substring of a single field. Terms are now matched independently (AND across terms, OR across fields) — a strict superset of the old behaviour, so no result that matched before can disappear. Still a substring filter, not BM25; wiring the box to
/api/v1/index/searchis a product decision and is reported, not built.The empty state now names the query and the scope it actually searched — how many tools, across how many servers, whether disabled tools were in scope — and, only when there are any, how many quarantined servers are not listed here. Quarantined servers contribute zero tools to
GET /api/v1/toolsand quarantine is on by default, so a first-run developer was searching a silently truncated catalogue with no hint of it.Reporting
stats.totalwould have lied whenever another filter was also narrowing the list, so the filter chain is split intosearchScope(every filter except the search box) andfilteredTools. All filters are ANDed, so the result is identical and the numbers are now literally true. A separate branch handles the case where a status filter left nothing to search — no wording of the query can help there, so the empty state names the filter as the cause instead.F07 — Add Server showed dead controls, then abandoned the user
The Docker isolation toggle rendered disabled for URL upstreams, where isolation is structurally impossible (
internal/config/isolation_resolve.goresolvesIsolationSourceNotStdiowhenever Command is empty). It is nowv-if="type === 'stdio'". Presentation only — both existing state guards are untouched, soisolation_jsonstill cannot reach an http add. The permanently-disabled "Coming soon" Idle on Inactivity toggle is removed;handleSubmitnever read it and no backend field of that name exists.After a successful add the user was dropped back where they started while the activation work (connect, scan, review, approve) waited on a page they had to find. The
addedevent now carries the server name and the three navigating consumers push toserverDetailPath(name). The navigation lives in the consumers, not inhandleSubmit—OnboardingWizard.vuealso listens toadded, and a push from inside the modal would unmount the wizard mid-flow. The bulk-import path keeps its bareemit('added'): a bulk import has no single destination.F13 — the setup wizard called a handshake a verified round-trip
The Verify tab showed a green "Round-trip verified" off a single boolean,
first_mcp_client_ever, which is stamped from the MCPinitializehook — a transport handshake, before any tool is listed let alone called. All three suggested prompts targeted mcpproxy's own built-ins, which record asinternal_tool_calland never stampfirst_real_tool_call_ever. A user who had never seen an upstream tool run was told the product was verified.It is now two separately-labelled milestones: "AI client connected" (same condition, honest words) and "First upstream tool call". One suggested prompt was added that actually dispatches to an upstream. The copy says "ran through mcpproxy and returned a result", not "succeeded", because
recordRealToolCallSuccess()is called one statement before theisErrorclassification.And then the fix was corrected for the same defect in the opposite direction.
first_real_tool_call_everhas exactly one call site (thecall_tool_*handler), so underrouting_mode=directorcode_executionthe new row told a user who had made dozens of real upstream calls "No upstream tool call yet", forever. Shipping a green claim off a handshake and a red claim off a blind flag are the same defect wearing different colours. The state is now tri-state and routing-mode aware, read from the sameGET /api/v1/statusresponse that already carriesactivation— satisfied (a lifetime fact, true under any mode), pending (only under the mode that actually stamps it), unknown (the row and its caveat do not render at all). The backend gap is real and still open; it needs Go tests and its own diff.F06 — the first load asserted state it had not been told
Behind the "Authentication Required" modal every surface still rendered, and three of them stated as fact things the UI had never been able to read. On one fresh install, unauthenticated vs authenticated at the same moment:
The "Setup ✓" was the worst:
incompleteTabCountreadsstate?.incomplete_tab_count ?? 0, so "never fetched" and "nothing left to do" both arrive as 0 — and since nothing movesstateoff null, this was permanent for any install whose fetch kept failing, not a pre-fetch flash. Three gates were added using flags that already existed:setupStateKnown,serversStore.loaded, andsystemStore.status !== null. Unknown renders a plain label or an em-dash rather than a confident zero.The auth screen's recovery instructions were also corrected, each verified against the code and by running it:
mcpproxy doctornever prints the API key (doctor_cmd.gohas zero matches forapikey/api_key) — replaced withmcpproxy status; the startup log carries the key in full only on the run that generated it; the tray labels are named without the OS claim they previously carried (the tray is a build split,//go:build !nogui && !headless && !linux, not an OS split — a macOS tarball or Homebrew user sees "Open Web Control Panel", only the DMG's Swift bundle says "Open Web UI in Browser").F09 — a duplicate endpoint was accepted in silence, then the gate contradicted itself
Adding a second upstream at an already-configured endpoint is accepted silently, and the first the user hears of it is the scanner reporting the new server as
dangerous: two entries on one endpoint expose byte-identical tool names and descriptions, sodetect.shadowing.cross_serveremits a hard-tier "possible impersonation" finding per tool. Reproduced end to end on a scratch instance: a singlePOST /api/v1/serversof the duplicate producedstatus: dangerous, finding_counts {dangerous: 2}unprompted, andPOST …/security/approvewithout force returned409 server has 2 dangerous (hard-tier) finding(s).Two halves, both frontend-only:
Add-time observation. The manual tab now names the configured server already holding the endpoint being typed, as a neutral note. Submit stays enabled; nothing is merged, reused or blocked. Matching is exact string equality after trim —
urlfor http; command, the full args list andworking_dirfor stdio (node server.jsin two directories is two different programs), with a negative transport guard so a staleurlleft on a converted stdio entry cannot be named as the occupant of a typed HTTPS endpoint. It can miss: the server list masks credential-shaped values, so a secret-bearing endpoint never matches. A hint, not a guarantee. Web-UI door only — the MCPupstream_servers addhandler, the CLI and the import path still say nothing; covering them needs a warning field on the add response.Gate wording. One render carried both "Informational — these findings do not block the tool" and "Force-approving bypasses the scanner gate", about the findings behind that 409. The first sentence was false — its source read
scan_informational.go's "drives NO gating whatsoever" as a claim about the findings, when it is a claim about the trigger. The panel receives neitherquarantinednor the effective trust mode, so it now claims nothing about this server's state, only what is unconditionally true of the tier. The dialogs stop saying "the scanner gate", a phrase the UI never defined. AndScanReport's Force Approve confirmation counted the wrong number — rawsummary.criticalwhile its own gate predicate readsfinding_counts.dangerous— so it offered to bypass the gate "despite 0 critical finding(s)" for the two findings the backend was refusing on.Not changed:
detect/checks/shadowing.go. Making the clone branch endpoint- or identity-aware is a maintainer decision. Nothing here touchesisBlockingFinding,ApproveServer, the quarantine default, or the force path — security posture is unchanged. Also not fixed: after force-approving a duplicate, the original server flips todangerouson its next scan and the two permanently accuse each other. The add-time note only reduces how often a user gets there.Two cross-branch defects, visible only once the branches were merged together
A live probe found a quarantined stdio server with a nonexistent command reporting
status=error, connected=false, yethealth.levelcame backhealthywith actionapprove(the calculator returns early on Quarantined), and every fault alert on the page is deliberately suppressed while quarantined (Web UI: quarantined server shows a red "file a bug report" error panel above its own calm quarantine banner #1076). Net effect: the Tools tab said "approve the server to list them" and nothing contradicted it — approving would hand the user a second failure. The copy now names the connection blocker only when the server is actually disconnected; a connected quarantined server keeps the original sentence, pinned by a control test.The Go change above makes a missing log file an empty success. The macOS tray's
loadLogs()read its on-disk fallback only fromcatch, so that empty success silently skipped the fallback and dropped historical local logs from the view. It now falls back on an empty result too.Verification
go build ./...,go vet ./internal/...— cleangolangci-lint run --config .github/.golangci.yml ./internal/...(v2, the config CI uses) — 0 issuesgo test ./internal/server/ -skip "E2E|Binary|MCPProtocol"— ok, 196scd frontend && ./node_modules/.bin/vitest run— 1,247 tests / 119 files passnpm run type-check(vue-tsc --noEmit) — clean200 {"logs":[],"count":0}where it previously returned the 500), and the duplicate-endpoint add against a backend returning exactly the count the confirm now quotes.Every behavioural change is backed by a test that was run and observed to fail first, and the new specs each carry assertions that passed before the change so they are not vacuous. Several were additionally bite-checked by neutering the fix in the working tree and confirming a red suite — that check is what caught two hunks (the
toolsEmptyHeadingchange and the approve-dialog paragraph) that had no assertion behind them at all and were free to drift back.No API payload field changed, so no
contracts.tsregeneration and nomake swagger(make buildre-run: zero diff). No new dependencies.🤖 Generated with Claude Code