fix(telemetry): stop four diagnostics instruments measuring themselves by Dumbris · Pull Request #1218 · smart-mcp-proxy/mcpproxy-go

Dumbris · 2026-09-06T17:11:43Z

Four fixes to instruments that turned out to be measuring themselves rather than the product. Provoked by production telemetry; each is independently verified and independently revertable. Schema bump 10 → 11.

1. `tool_count` read a cache that indexing zeroes (`ae5414787`)

GetToolCount fed the heartbeat's tool_count, which the activation funnel and every retention cut key on. It read the upstream manager's per-client tool-count cache — but both indexing paths call InvalidateAllToolCountCaches() as their last step (internal/runtime/lifecycle.go:466, :644), and the only writers that refill that cache without re-zeroing it are UI/API-triggered ListTools calls.

So the field largely reported "has the owner opened the dashboard recently", not "has this install indexed any tools" — which confounds both the 1221-connected → 914-with-tools funnel step and the day-8-14 retention split (27.3% with 100+ tools vs 12.1% with none): dashboard engagement would drive both independently.

The Web UI and REST API were never affected — they sum the durable StateView.ToolCount, which reconcile preserves with a carry-forward guard — which is why no user ever filed a "0 tools" bug.

Now reads index.Manager.GetDocumentCount(): one Bleve document per tool, durable across restarts. Per-profile indexes derive from this shared index (RebuildProfileFromShared), so profile users are not undercounted. The old cache path stays as a fallback when no index manager is wired.

Dashboard note: this changes what tool_count measures. Pre- and post-release values are not comparable; gate the funnel query on the release that ships this and re-baseline.

2. `diagnostics.error_code_counts_24h` was level-triggered (`cfb06ef3d`, `223eb645b`)

Supervisor.reconcile runs on a 30s ticker and re-classified every configured server's sticky LastError each pass with no change detection. ConnectionInfo.LastError is cleared only on a transition to Ready, so a server parked awaiting an OAuth login — making zero connection attempts — emitted 86400/30 = 2,880 "events" a day. An install's number tracked failing-server count × uptime.

Field data corroborates: MCPX_OAUTH_LOGIN_REQUIRED averaged 5,212 per install-day when non-zero; 484 install-days exceeded 2,880; days above the tick cadence averaged 3.89× it on installs with a mean of 18.8 configured servers.

Both stateview writers now capture the previous code and RetryCount at the top of the UpdateServer closure and notify only on an edge — a different classified code, or a changed RetryCount. The comparison is != not > deliberately: RetryCount also resets to 0 on Ready, so a decrease marks a new failure episode after an unobserved recovery. This also collapses the duplicate notification an event pass and a reconcile pass used to produce for one failure.

classifyAndAttach is untouched, so the standing diagnostic still renders in UI/REST/CLI. Only the telemetry notification became edge-triggered.

The companion is not optional. Edge-triggering alone deletes the "installs currently affected" signal: the counter decays, carries omitempty, and the whole Diagnostics object drops via isZero(), so a permanently parked install would emit once and then vanish from the payload — trading an inflated number for a missing one that downstream reads as zero. So this ships diagnostics.current_error_codes in the same change: MCPX code → number of configured servers currently in that state, recomputed at heartbeat time from the supervisor's live stateview and joined before the isZero() check.

Anonymity: low cardinality (the MCPX catalog), counts only, no names/URLs/commands/free text. Three independent guards — supervisor MCPX prefix filter, sanitizeMCPXCodeMap at assembly, and a new structural rule in the anonymity scanner (generalised over both maps; catalog-registered keys, non-negative ints, never echoing the offending key). 223eb645b then tightened sanitizeMCPXCodeMap to isValidMCPXCode so the producer filter matches the wire gate exactly — ScanForPII drops the entire heartbeat on a violation, so a well-shaped but uncataloged code would have silently destroyed every heartbeat for as long as that standing state persisted.

Only enabled, non-quarantined servers are counted (TestCurrentErrorCodes_ExcludesDisabledAndQuarantined) — a quarantined server is deliberately not connected and a disabled one is not wanted.

update_failure.go's MCPX_UPDATE_*_FAILED producer is deliberately untouched: those are genuine one-shot events.

SchemaVersion 10 → 11. v10-and-earlier error-code volumes are not comparable with v11 and must not be graphed as one series. The const comment and docs/features/telemetry.md both say so.

3. The classifier only knew one spelling of "http" (`79a75cf5a`, `43fdd1c2d`)

Every HTTP status/timeout/status-text arm was gated on hints.Transport == "http". But transport.DetermineTransportType returns config.Protocol verbatim when set, and "streamable-http" when not — so one transport reaches the classifier under four names config validation all accepts: streamable-http (a bare-URL server), http (registry add path, Claude Code / Gemini importers, the Add-Server modal), sse, and auto. Three of the four had every arm switched off and fell through to MCPX_UNKNOWN_UNCLASSIFIED with its "file a bug report" CTA. No test in the package used any spelling but "http", which is how it shipped.

diagnostics.CanonicalTransport folds the HTTP family to one value and passes everything else through lower-cased. hints.For calls it, so every production hint is normalized at the one place hints are built; classifyHTTP calls it too so a hand-built ClassifierHints cannot reintroduce the bug.
matchHTTPStatusText also accepts the marker "status code: " — mcp-go's SSE connect path words its failure unexpected status code: %d, where the digits do not follow "status ". An SSE 401/502 at connect time matched nothing on every spelling, including "http".
Four new codes: MCPX_HTTP_RATE_LIMITED (429), MCPX_HTTP_4XX (the enumerated set 400/408/409/410/451 — not a blanket range, which would claim statuses a higher layer resolves better), MCPX_HTTP_CONN_RESET (typed ECONNRESET), MCPX_HTTP_CANCELED (typed context.Canceled, severity info). Plus a net.Error-with-Timeout() branch routing to the existing MCPX_HTTP_TIMEOUT, which neither existing arm could see (i/o timeout and http.Client deadlines are not context.DeadlineExceeded).
Retry behaviour is unchanged — none of the four is RetryPermanent; they keep the zero RetryClass MCPX_UNKNOWN_UNCLASSIFIED carries today (GH Deterministic, unrecoverable connection failures retry forever (55 attempts / 19h on one server) #1145). Naming these failures changes the message a user reads, nothing else.
No EOF/EPIPE branch: on stdio those are already rewritten into the handshake-exit shape and classified as MCPX_STDIO_EXIT_BEFORE_INITIALIZE; reading them here would double-classify.

43fdd1c2d fixes three defects a cross-model review found in the above, all reproduced:

The empty transport had been folded into the HTTP family, but "" is reachable in production (supervisor passes it whenever state.Config is nil), and three of the four arms it unlocked are transport-agnostic. Measured: a stdio handshake timeout returned MCPX_HTTP_TIMEOUT, a canceled stdio connect MCPX_HTTP_CANCELED, and a stdio exit whose stderr tail quoted a child's 503 returned MCPX_HTTP_5XX. "" now passes through.
matchHTTPStatusText scanned one whole marker across the string before trying the next, and "status " is a strict prefix of "status code: " — so a status quoted later outranked the one the transport reported. The scan is now positional, left to right, longest marker first at each position.
The scan continued past an unrecognised status: "request failed with status 402: status 401" answered 401 — "fix your credentials" for a billing failure. It now stops at the first well-formed status token. A four-digit run is not a token, so "status 4011" keeps scanning.

Both phrasings matter because mcp-go interpolates the raw response body into request failed with status %d: %s.

Docs: four new docs/errors/ pages, docs/errors/README.md index entries, and website/sidebars.js entries so the fix-step links resolve.

4. The registered "fixers" were placeholders (`faefdd05c`, `f34ca9630`, `c080ab981`)

internal/diagnostics/builtin_fixers.go says the advanced fixers "are registered by higher layers at startup". Nothing ever did — the only diagnostics.Register call outside the package was in a test. So all four registered fixers were the package's own placeholders: stdio_show_last_logs returned outcome=SUCCESS with the canned string "log tail unavailable in this build", and the other three returned BLOCKED under execute.

Two of those are the only action their codes offer, and they are wide in the field: MCPX_STDIO_EXIT_BEFORE_INITIALIZE (270 installs) and MCPX_STDIO_HANDSHAKE_TIMEOUT (315) both point at "Show last server log lines"; MCPX_OAUTH_LOGIN_REQUIRED (208) at "Sign in". And because the placeholder reported SUCCESS, diagnostics.fix_succeeded_24h was counting no-ops.

New internal/server/diagnostics_fixers.go registers two runtime-backed fixers from NewServerWithConfigPath:

stdio_show_last_logs → (*Server).GetServerLogs(name, 50), the same call backing GET /api/v1/servers/{id}/logs. Reusing that call rather than reading the file is the point: it runs every line through parseLogLine → scrubUpstreamText (quarantine_security list_quarantined returns env/headers/oauth secrets in plaintext, unauthenticated #1148), so the text crossing the REST API is masked by the identical scrubber. A missing/unreadable log now returns FAILED instead of a fake success.
oauth_reauth → Runtime.TriggerOAuthLogin, the same call POST /api/v1/servers/{name}/login makes. dry_run still previews only.

The two config-mutating placeholders are deliberately untouched — writing the user's config from a one-click button is a larger design question.

Gating is unchanged and verified, not assumed: destructive-flag handling, the 409 mode guard (TestDiagnosticsFix_ModeGuard still passes), ErrorPanel's window.confirm, and the existing duplicate-sign-in guard in connection_oauth.go (#975).

f34ca9630 then sized the delivery path for the new payload: ToastContainer renders the message whitespace-pre-wrap with a bounded scrollable height (a plain div folded 50 newlines into one paragraph); ErrorPanel gives a multi-line fix message a 60s dwell instead of 5s; and a new diagnosticsPreviewMaxBytes (8KB) drops the oldest lines — a tail is read bottom-up — stating how many it dropped. The newest line always survives whole. c080ab981 then makes ErrorPanel supersede its own previous long-lived preview, because the 60s dwell let tall previews stack upward past the top of the viewport, close button included.

diagnostics.fix_attempted_24h / fix_succeeded_24h are human-click-only by design — RecordFixAttempt has exactly one non-test call site, gated on mode==execute. There is no broken auto-heal loop. What changed is that a press now does something and a success now means something.

Also in this branch

CLAUDE.md: cross-model reviewer switched to gpt-6-astra per maintainer directive 2026-09-06, plus the two invocation rules (close stdin, wrap in gtimeout; an empty result is a failed review, not a clean one).

Cross-model review (opencode, gpt-6-astra)

Reviewed in four parts — telemetry accounting, the classifier, the fixers and their delivery path, and the tool_count source — then re-reviewed. Findings accepted:

#	Finding	Fix
1	The edge basis was `(code, RetryCount)`, but `SetOAuthError` advances `oauthRetryCount` and not `retryCount` — so a server failing OAuth repeatedly edged once, ever. Unbounded coalescing, not the cadence-bounded kind the comment promised.	`ConnectionInfo.LastRetryTime` joins the basis. Every failure setter stamps it; nothing else advances it; re-observing a sticky error does not touch it. Two ABA routes that needed no dropped event close with it.
2	The v11 schema bump left the per-code counters in the same BBolt key namespace, so an install upgrading mid-window would have spent its first post-upgrade day adding v11 edge increments to v10 level-triggered counts, under the v11 label.	Fresh key namespace (`code_edge_count_24h_`). Neither prefix is a prefix of the other; the v10 orphans are inert and decay-stale.
3	`MCPX_HTTP_CANCELED` is the catalog's only `severity=info` code, and `ServerDetail`'s gate accepted only warn/error — which did not hide it, but fell through to the generic red "Server Error" box. The calmest code rendered the loudest, stripped of its message, fix steps and docs link.	Gate widened to info. `ErrorPanel` already styles info calmly, so the component needed nothing.
4	The Sign in button called `Runtime.TriggerOAuthLogin` directly, skipping the management service's `checkWriteGates` — so it worked on an install with `read_only_mode` or `disable_management` set, where the REST login route refuses.	Routed through `TriggerOAuthLoginQuick`, the same call `POST /servers/{id}/login` makes. Fails closed if that service is unavailable.
5	The same fixer asserted a browser window "just opened" whether or not the launch had succeeded.	`OAuthStartResult.BrowserOpened`/`BrowserError` now drive the message, with the URL to finish manually.
6	The 8KB preview cap measured only log lines, not the header and omission notice it also renders — a full tail shipped ~8.25KB.	Framing is budgeted for. The pre-existing bound assertion also tightened from `< 3×cap` to `<= cap`, which was not asserting the cap at all.
7	`Close()` closed the Bleve index before `telemetryService.Stop()`, which performs the final graceful heartbeat — so the last heartbeat of every clean shutdown fell through to the disconnected upstream cache and reported `tool_count=0`. The very confound this PR removes, reappearing on the way out.	Index closes after the flush, and `GetToolCount` memoises the last successful count so a transient index error cannot produce a zero either.

Every fix has a bite check: the guard was verified to fail against the unfixed code with the package still building.

A further round then reverted half of #4. Routing through TriggerOAuthLoginQuick to inherit the write gates also swapped in a materially different OAuth implementation: it runs startup synchronously under its own 30-minute context (so the fix endpoint's 15s deadline stops bounding the call), its watcher cancels the callback context after ~120s (so slow 2FA loses a flow the user was told had started), HasRecentOAuthCompletion can make that watcher exit on its first poll after a recent sign-in, and it drops the SSE dispatch. The async Runtime.TriggerOAuthLogin is back, with the gate applied explicitly in the fixer using the same message text checkWriteGates uses, failing closed if the config cannot be read. The gate test is unchanged and still bites.

Separately, CI caught a defect in this branch's own test helper: newFixerTestServer registered its upstream client out of band, and the supervisor's reconcile — which diffs cfg.Servers against the manager and deletes the difference — removed it again. Passed on every developer machine, failed on CI under -race with server not found. Reproduced locally by inserting a 3s sleep, then fixed by re-registering at the call site; the two tidier-looking alternatives (a disabled entry in cfg.Servers, an enabled one) were both tried and both rejected with evidence, in the commit message.

Rejected, with reasons: a producer/scanner predicate "mismatch" that is a one-directional producer restriction and cannot cause the heartbeat drop it guards; three further ABA/staleness variants (two closed by #1, the third an over-count inherent to diffing sampled projections and documented rather than papered over); GetServerLogs ignoring the 15s deadline (per-server logs are lumberjack-rotated at 10MB); diagnostics.Register being process-global (documented; production builds one Server per process); duplicate sign-in clicks (the fixer now makes the same call as the REST login button, so the behaviour is that route's, pre-existing and unchanged here); long-lived toast previews stacking across panels (ErrorPanel has exactly one mount site, under a v-if on a single server).

Verification

Every commit records its own RED-first evidence and bite checks; see the commit messages. Aggregate: go build ./... and -tags server OK, go vet clean, gofmt clean, golangci-lint v2 with .github/.golangci.yml 0 issues, go test -race green across internal/diagnostics, internal/runtime, internal/runtime/supervisor, internal/telemetry, internal/server, internal/httpapi, internal/health, internal/upstream, plus 1178 frontend unit tests.

scripts/test-api-e2e.sh was not run — it blanket-pkills every mcpproxy core on the machine and sibling worktrees are live; no route, handler or payload schema on the HTTP surface changed. scripts/check-errors-docs-links.sh fails on the same 8 pre-existing pages as main and on none of the new codes.

🤖 Generated with Claude Code

GetToolCount feeds the heartbeat's tool_count, which the activation funnel and every retention cut are keyed on. It read the upstream manager's per-client tool-count cache — but both indexing paths call InvalidateAllToolCountCaches() as their LAST step (lifecycle.go:466, :644), and the only writers that refill that cache without re-zeroing it are UI/API-triggered ListTools calls. So tool_count was largely reporting "has the owner opened the dashboard recently", not "has this install indexed any tools". That confound sits underneath two field headlines: the 1221-connected -> 914-with-tools funnel step, and the day-8-14 retention split (27.3% with 100+ tools vs 12.1% with none) — dashboard engagement would independently drive both. The Web UI and REST API were never affected: they sum the durable StateView.ToolCount, which reconcile preserves with a carry-forward guard (supervisor.go:681-683). Only telemetry read the zeroed value, which is why no user ever filed a "0 tools" bug. Now reads index.Manager.GetDocumentCount() — one Bleve document per tool, durable across restarts, and the only source that means what the field's name says. Per-profile indexes are derived from this shared index (RebuildProfileFromShared), so there is no undercount for profile users. The old cache path stays as a fallback when no index manager is wired. Test written first and confirmed failing on HEAD with got=0 for all three assertions: internal/runtime/tool_count_index_test.go indexes 3 tools and asserts 3, adds a 4th and asserts 4, deletes one server's tools and asserts 2. Verified: go test ./internal/runtime/ (112s, ok), ./internal/telemetry/ (ok), go vet, and golangci-lint v2 with .github/.golangci.yml — 0 issues. Note for whoever reads the dashboard: this changes what tool_count measures, so pre- and post-release values are not comparable. Gate the funnel query on the release that ships this and re-baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

WHAT WAS WRONG internal/diagnostics/builtin_fixers.go:7 says the advanced fixers "are registered by higher layers at startup". Nothing ever did — the only diagnostics.Register call outside the package was in internal/httpapi/diagnostics_fix_test.go. So all four registered fixers were the package's own placeholders: - stdio_show_last_logs (builtin_fixers.go:17) returned outcome=SUCCESS with the canned string "log tail unavailable in this build". - config_migrate_deprecated (:33), server_disable_scanner (:47) and oauth_reauth (:63) returned outcome=BLOCKED under execute. Two of those are the only action their error codes offer, and they are wide in the field: MCPX_STDIO_EXIT_BEFORE_INITIALIZE 270 installs -> "Show last server log lines" MCPX_STDIO_HANDSHAKE_TIMEOUT 315 installs -> "Show last server log lines" MCPX_OAUTH_LOGIN_REQUIRED 208 installs -> "Sign in" The stdio codes are exactly the ones whose captured stderr usually names the missing binary or env var outright, so the button promised the single most useful thing and delivered a fixed string. And because the placeholder reported SUCCESS, telemetry's diagnostics.fix_succeeded_24h was counting placeholder no-ops: it was measuring the instrument, not the product. WHAT CHANGED New internal/server/diagnostics_fixers.go registers two runtime-backed fixers, installed from NewServerWithConfigPath where both dependencies are already in scope: - stdio_show_last_logs -> (*Server).GetServerLogs(name, 50), the same call backing GET /api/v1/servers/{id}/logs. The FixResult crosses the REST API, so the text is masked by the identical scrubber that endpoint uses: GetServerLogs runs every line through parseLogLine -> scrubUpstreamText (issue #1148), which is why reusing that call rather than reading the file directly is the whole point. A missing or unreadable log now returns outcome=FAILED instead of a fake success. - oauth_reauth -> Runtime.TriggerOAuthLogin, the same call POST /api/v1/servers/{name}/login makes. dry_run still previews only. The two config-mutating placeholders are deliberately untouched: writing the user's config file from a one-click button is a larger design question than wiring an existing read path. GATING IS UNCHANGED (verified, not assumed) - Destructive flags: every catalog entry that offers oauth_reauth marks the step Destructive:true except MCPX_OAUTH_LOGIN_REQUIRED's first-time "Sign in", where there is no stored credential to lose. handleInvokeFix still returns 409 for a destructive fixer with no explicit mode (internal/httpapi/diagnostics_fix.go:98) — covered by the pre-existing TestDiagnosticsFix_ModeGuard, which still passes. - Web UI: ErrorPanel.vue still renders Preview + Execute for a destructive step and routes Execute through confirmAndExecute()'s window.confirm(). Untouched by this commit. - Duplicate sign-in: not re-implemented here, because the guard already exists upstream. Client.handleOAuthAuthorization stands down when a manual flow is in flight (internal/upstream/core/connection_oauth.go:922, issue #975) and isOAuthInProgress refuses a duplicate, so a second click surfaces as an error string, not a second browser tab. - No security check or quarantine behaviour is touched. THIS DOES NOT MAKE THE COUNTERS FIRE AUTOMATICALLY, ON PURPOSE RecordFixAttempt has exactly ONE non-test call site — the fire-and-forget block at internal/httpapi/diagnostics_fix.go:140, gated on mode==execute. diagnostics.fix_attempted_24h / fix_succeeded_24h are therefore human-click-only BY DESIGN. There is no broken auto-heal loop to repair; an earlier analysis assumed one. Nobody should set a target on that ratio or read a low fix_attempted_24h as a defect — it is a count of button presses. What this commit changes is that a press now does something and that a success now means something. ASSUMPTIONS MADE (no human was asked) - Rendering: the preview emits each entry's scrubbed Message only. parseLogLine SYNTHESIZES Timestamp (time.Now()) and Level ("INFO") for any line it cannot parse — which is every raw stderr line piped from the child, i.e. the lines this button exists to show — so echoing them back would fabricate data. For an unparsed line Message is the whole original line, so nothing is lost. - No truncation of the tail. scrubUpstreamText deliberately dropped the activity-store cap for live reads (a long line is often precisely what an operator opened the log for); the 50-line window is the bound. - diagnostics.Register is process-global/last-write-wins. Production builds one Server per process; in a test binary that constructs several, the newest owns the fixers. That is the same contract the package's own init() already had, and it is documented at the registration site. VERIFIED go test ./internal/server/ -run 'TestDiagnosticFixer_' -count=1 -v BEFORE the fix: 3 of 4 FAIL, for the right reason (package compiles; failures quote the placeholder strings) — "...log tail unavailable in this build..." should not contain "unavailable in this build" Not equal: expected "failed", actual "success" (missing log file) "oauth re-auth fixer has not been wired to the OAuth coordinator in this build" should not contain "has not been wired to the OAuth coordinator" (the 4th, the oauth dry-run guard, passes on HEAD by design) AFTER the fix: 4/4 PASS (1.566s) go test ./internal/server/ -race -run 'TestDiagnosticFixer_' -count=1 ok 3.575s go test ./internal/server/ -count=1 -skip "E2E|Binary|MCPProtocol" ok 207.539s go test ./internal/httpapi/ -run TestDiagnosticsFix -count=1 -v PASS (mode-guard intact) go test ./internal/diagnostics/... -count=1 ok go build ./... OK go vet ./internal/server/... ./internal/diagnostics/... OK gofmt -l <the three touched files> clean /opt/homebrew/bin/golangci-lint run --config .github/.golangci.yml \ ./internal/server/... ./internal/diagnostics/... 0 issues git diff --stat: only the three intended files. NOT RUN: ./scripts/test-api-e2e.sh. It blanket-pkills mcpproxy cores belonging to other sessions on this machine, and this change adds no route and alters no HTTP handler. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…_codes diagnostics.error_code_counts_24h was measuring the instrument, not the product. It was level-triggered: supervisor.go:275 reconcile ticker, 30s reconcile() -> updateSnapshot() -> updateStateView() for EVERY configured server -> classifyAndAttach(...) -> notifyErrorCode(code), unconditionally (supervisor.go:769/:811, and an identical pair on the event path :1098/:1117) -> diagnostics_counters.go RecordErrorCode, which dedupes nothing ConnectionInfo.LastError is sticky — the state machine clears it only on a transition to Ready (upstream/types/types.go:301-303). So a server PARKED awaiting an OAuth login, making zero connection attempts, emitted 86400/30 = 2880 "events" a day, and an install's number tracked failing-server count times uptime rather than anything a user did. Field data corroborates: MCPX_OAUTH_LOGIN_REQUIRED averaged 5,212 per install-day when non-zero, 484 install-days exceeded 2,880, and days above the tick cadence averaged 3.89x it on installs with a mean of 18.8 configured servers. WHAT CHANGED 1. Both stateview writers in supervisor.go now capture the previous diagnostic code and RetryCount at the TOP of the UpdateServer closure, before any mutation. View.UpdateServer (stateview.go:103-143) deep-clones the snapshot and passes the previous status into the callback, so those reads are last-pass values — verified by reading the clone loop, not assumed. shouldNotifyErrorCode then propagates to notifyErrorCode only on an edge: a different classified code, or a changed RetryCount. It compares != rather than > deliberately: RetryCount advances on each failed attempt AND resets to 0 on a transition to Ready, so a decrease marks a new failure episode after a recovery no stateview pass happened to observe. Both writers share the same basis, which also collapses the duplicate notification an event pass and a reconcile pass used to produce for one failure. 2. classifyAndAttach is untouched and still runs on every pass, so the STANDING diagnostic keeps rendering in the UI, REST API and CLI. Only the telemetry notification became edge-triggered. Pinned by TestUpdateStateView_StandingDiagnosticStillRendered. 3. notifyErrorCode also refreshes last_error_code via prechurnStore.RecordLastErrorCode (runtime.go:3138-3145). I read that implementation (telemetry/prechurn.go:80-84, :148): it "persists code as the most recent diagnostic code, overwriting any prior value" — a last-value field, not a counter. Edge-triggering preserves it correctly: the last edge is still the most recent distinct code. The only behavioural difference is that with several failing servers it no longer oscillates between their codes every 30s, which is an improvement. THE COMPANION (schema v11) — not optional Edge-triggering alone DELETES the "installs currently affected" signal. error_code_counts_24h decays (readCounterWithDecay) and carries omitempty, and the whole Diagnostics object is dropped via isZero() + omitempty (diagnostics_counters.go, telemetry.go:316). A permanently parked install would emit once and then vanish from the payload entirely — trading an inflated number for a MISSING one, which downstream reads as zero. That is the same absent-counted-as-zero trap that has already produced one wrong headline here. So this ships diagnostics.current_error_codes in the same change: MCPX_ code -> number of configured servers currently in that state, recomputed at heartbeat time from the supervisor's live stateview (Supervisor.CurrentErrorCodes), wired through a nil-safe provider like the existing onboarding/IDE-count providers. It is joined into the snapshot BEFORE the isZero() check, so an install whose only signal is "N servers are broken right now" still emits a diagnostics object. Anonymity: low cardinality (the ~30-code MCPX_ catalog), counts only, no server names, URLs, commands or free text. Three independent guards, since the map now originates outside internal/telemetry: the supervisor filters on the MCPX_ prefix, sanitizeMCPXCodeMap re-filters shape/length/positivity at assembly, and the anonymity scanner gained a structural rule (the spec-095 error_code_counts_24h check, generalised over both maps, catalog-registered keys and non-negative ints, never echoing the offending key). MarshalJSON applies the same top-20 cap. Only enabled, non-quarantined servers are counted — a server the user disabled is not a problem the install has. SchemaVersion 10 -> 11. v10-and-earlier error-code volumes are NOT comparable with v11 ones and must not be graphed as one series; the const comment and docs/features/telemetry.md both say so, and the docs page (which still said v9) now documents both maps and the events-vs-standing-state distinction. NOT COVERED, DELIBERATELY: RecordErrorCode has a second producer at internal/server/update_failure.go:78 for the four MCPX_UPDATE_*_FAILED codes. The gate added here lives in the supervisor and does not touch that path. That is correct — those are genuine one-shot events (one failed update attempt, one increment), not a standing condition being re-observed. ASSUMPTION DOCUMENTED: "currently affected" excludes disabled and quarantined servers. A quarantined server is deliberately not connected and a disabled one is not wanted, so counting either would re-inflate the number in a different way. Pinned by TestCurrentErrorCodes_ExcludesDisabledAndQuarantined. TESTS — written first, run first, confirmed failing for the right reason. The tests drive sup.updateStateView(...) directly rather than reconcile(): MockUpstreamAdapter.AddServer builds a ServerState with a nil ConnectionInfo, so a reconcile-driven test never reaches classifyAndAttach and would have passed vacuously against HEAD. They also use typed errors (fmt.Errorf("...%w", syscall.ECONNREFUSED), *net.DNSError) because plain strings classify to MCPX_UNKNOWN_UNCLASSIFIED and the code-change assertion would not have distinguished anything. Before the fix: $ go test ./internal/runtime/supervisor/ -run TestUpdateStateView_ -v --- FAIL: TestUpdateStateView_ErrorCodeIsEdgeTriggered unchanging error over 10 reconcile passes produced 10 notifications ([MCPX_HTTP_CONN_REFUSED x10]), want exactly 1 --- FAIL: TestUpdateStateView_RetryCountAdvanceReNotifies (got 3, want 2) --- FAIL: TestUpdateStateView_CodeChangeReNotifies (got 3, want 2) --- PASS: TestUpdateStateView_StandingDiagnosticStillRendered $ go test ./internal/telemetry/ -run CurrentErrorCodes --- FAIL: TestScanForPII_CurrentErrorCodesShapeViolations (7/7 subtests: scanner accepted a server name / URL / uncataloged code / negative / fractional / string / non-object) After: go build ./... ok go build -tags server -o /dev/null ./cmd/mcpproxy ok go vet ./internal/runtime/... ./internal/telemetry/... 0 go test ./internal/runtime/... ./internal/telemetry/... -count=1 all ok go test ./internal/runtime/ -race -count=1 ok (170s) go test ./internal/runtime/supervisor/... ./internal/telemetry/... -race ok go test ./internal/httpapi/... -count=1 ok go test -tags server ./internal/serveredition/... -count=1 ok golangci-lint run --config .github/.golangci.yml \ ./internal/runtime/... ./internal/telemetry/... 0 issues gofmt -l internal/runtime internal/telemetry clean NOT RUN: ./scripts/test-api-e2e.sh — it blanket-pkills every running mcpproxy core, which would kill sibling agents' instances on this machine. The change is backend-internal (supervisor notification gating + heartbeat assembly) with unit, race and assembly coverage above; nothing in the HTTP surface changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…ansport The classifier's HTTP arms were gated on `hints.Transport == "http"`, an exact string a large share of servers never carry. `transport.DetermineTransportType` returns `config.Protocol` VERBATIM when it is set, and "streamable-http" when it is not, so one transport reaches the classifier under four names that config validation all accepts (internal/config/config.go:2465): - "streamable-http" — a URL server added with no explicit protocol - "http" — registry add path, Claude Code / Gemini importers, the Add-Server modal - "sse" — explicit legacy SSE - "auto" — accepted by config, passed through by hints.For Three of those four had every status, timeout and status-text arm switched off, so their failures fell through to MCPX_UNKNOWN_UNCLASSIFIED and its "please file a bug report" CTA. No test in internal/diagnostics used "streamable-http" or "sse" at all, which is how it shipped. I am deliberately NOT claiming a share of the UNKNOWN bucket for this. The heartbeat carries no per-server protocol string, so the split between spellings cannot be measured in-repo. The change stands on correctness — the same failure must not classify differently because of how a server happened to be added — and on cost: it adds no dependency, no allocation on the hot path, and no new retry behaviour. WHAT CHANGED - diagnostics.CanonicalTransport folds the HTTP family ("http", "https", "sse", "streamable-http", "streamable_http", "auto", "") to one value and passes everything else through lower-cased. hints.For calls it, so every production hint is normalized at the ONE place hints are built; classifyHTTP calls it once per invocation so a hand-built ClassifierHints cannot reintroduce the bug. classifyStdio's two gates now read the TransportStdio constant. - matchHTTPStatusText also accepts the marker "status code: ". mcp-go's SSE CONNECT path words its failure `unexpected status code: %d` (client/transport/sse.go:293, verified in the pinned v1.0.0), where the digits do not follow "status ". An SSE server's 401 or 502 at connect time therefore matched nothing at all — on EVERY transport spelling, including "http". - DiagnoseHTTPStatus maps 429 to a new MCPX_HTTP_RATE_LIMITED, and 400/408/409/ 410/451 to a new generic MCPX_HTTP_4XX. The set is enumerated, not ranged: a blanket 4xx range would claim the statuses that a higher layer resolves better than a status number can (a 401 that is really a deferred OAuth sign-in, a 4xx on the initialize POST that is really MCPX_HTTP_LEGACY_SSE). The exact status reaches the user in DiagnosticError.Cause, which is the raw error text. - Three typed branches ahead of the string matching: errors.Is(ECONNRESET) -> new MCPX_HTTP_CONN_RESET (ungated, like the ECONNREFUSED arm above it: a socket errno is evidence of a socket); net.Error with Timeout() -> the existing MCPX_HTTP_TIMEOUT, which covers `i/o timeout` and http.Client deadlines — NEITHER is context.DeadlineExceeded, so the two existing arms could not see them; errors.Is(context.Canceled) -> new MCPX_HTTP_CANCELED at severity info. - No EOF/EPIPE branch. On stdio those are already rewritten into the "server process exited before completing the MCP initialize handshake" shape by internal/upstream/core/connection_lifecycle.go and classified as MCPX_STDIO_EXIT_BEFORE_INITIALIZE; a second reading here would double-classify the same failure. RETRY BEHAVIOUR IS UNCHANGED. None of the four new codes is RetryPermanent. A 429 clears, a reset re-dials, a cancellation was ours, and the generic 4xx bucket holds 408. They keep the zero RetryClass that MCPX_UNKNOWN_UNCLASSIFIED carries today, so naming these failures changes the message a user reads and nothing about whether mcpproxy keeps retrying them (GH #1145). ASSUMPTIONS MADE (no clarification requested, per CLAUDE.md) 1. The cancellation code is named MCPX_HTTP_CANCELED, not a bare MCPX_CANCELED. Both catalog_test.go's regex and the PUBLISHED contract specs/044-diagnostics-taxonomy/contracts/catalog-schema.json require MCPX_<DOMAIN>_<SPECIFIC> against a closed domain list; a domainless code fails both. Inventing a new domain would edit a published contract for one code, so I kept it in the HTTP domain and gated the branch on the HTTP family, which keeps the name honest. 2. The empty transport joins the HTTP family. It reaches hints.For only when the server config is unavailable (supervisor.go, Config == nil), i.e. when nothing is known. That is safe because every arm this unlocks needs positive evidence of an HTTP exchange — a status number in the text, a typed net error — so a stdio-shaped failure still matches none of them. 3. My MCPX_HTTP_4XX fix step uses `mcpproxy upstream list -o json`. The obvious copy source, MCPX_CONFIG_MISSING_SECRET, suggests `mcpproxy upstream get`, which does not exist (the subcommands are list/logs/enable/disable/restart/ add/remove/add-json/patch/inspect/approve/tools/import). That pre-existing wrong command is left alone as unrelated. 4. scripts/test-api-e2e.sh was NOT run: it blanket-pkills every mcpproxy core on the machine, and sibling agent worktrees are live. This change touches no route, payload or schema; the published-catalog surface it does touch is covered by catalog_contract_test.go, which is in the run below. VERIFIED RED first, with the helper present so the failure was an assertion and not a build error (a package that does not compile proves nothing): $ go test ./internal/diagnostics/ -run TestClassify_HTTPFamilyTransportsAgree --- FAIL: .../401_in_status_text/sse --- FAIL: .../401_in_status_text/streamable-http --- FAIL: .../401_in_status_text/auto --- FAIL: .../wrapped_context_deadline/{sse,streamable-http,auto} --- FAIL: .../sse_unexpected_status_code_marker/{http,sse,streamable-http,auto} --- FAIL: .../429_rate_limited/{http,sse,streamable-http,auto} --- FAIL: .../connection_reset_by_peer/{http,sse,streamable-http,auto} i.e. the two rows the "http" column could already classify passed only in that one column, and the three rows nothing could classify failed in all four. Bite check on the source-level fix: reverting hints.For to pass the transport through unchanged fails TestFor_CanonicalizesTransport with `DetermineTransportType="streamable-http" ... want "http"` on exactly the rows that were broken, while "explicit http" and every stdio row keep passing. $ gofmt -l internal/diagnostics/ (no output) $ go build ./... exit 0 $ go vet ./internal/diagnostics/... exit 0 $ go test -race ./internal/diagnostics/... -count=1 ok internal/diagnostics 1.600s / ok internal/diagnostics/hints 1.247s 97 top-level tests pass, 0 fail $ golangci-lint run --config .github/.golangci.yml ./internal/diagnostics/... 0 issues. $ go test ./internal/runtime/... ./internal/upstream/... ./internal/httpapi/... ./internal/health/... ./internal/telemetry/... -count=1 all ok. (One flake seen once under parallel load: internal/upstream manager_docker_recovery_test.go "docker unavailable: signal: killed"; passes on its own, `docker info` exit 0, and the change touches no Docker path.) Guards added beyond the required table, because widening the family moves the HTTP arms AHEAD of classifyOAuth for the newly-included spellings: TestClassify_OAuthKeepsPrecedenceOverHTTPStatus pins that a typed ErrOAuthPending (Code() fast path, wrapped with %w through "all authentication strategies failed") and a stringified OAuth message still classify as MCPX_OAUTH_LOGIN_REQUIRED on all four transports — a sign-in prompt misreported as MCPX_HTTP_401 would send the user hunting for a credential bug instead of clicking "log in". TestClassify_LegacySSEKeepsPrecedence and TestClassify_StdioTransportUnaffected pin the other two precedences. Docs: the four new codes get real docs/errors pages plus website/sidebars.js entries, so their fix-step links resolve. scripts/check-errors-docs-links.sh still fails on the SAME 8 pre-existing gaps as on HEAD (MCPX_HTTP_TIMEOUT, MCPX_DOCKER_CLI_NOT_FOUND, MCPX_DOCKER_OCI_RUNTIME, MCPX_STDIO_SPAWN_EXEC_FORMAT and the four MCPX_UPDATE_*) and on none of the new codes; closing those is out of scope here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Follow-up to faefdd0, from cross-model review (opencode gpt-6-astra). Two findings were genuine and are fixed here; both are about the same thing — faefdd0 changed stdio_show_last_logs from returning a one-line placeholder to returning a ~50-line log tail, and nothing downstream was sized for that payload. 1. The tail was undeliverable to the user it was written for. ErrorPanel.vue puts a fix's `preview` into a toast, and ToastContainer.vue rendered the message in a plain div: default HTML whitespace handling folded all 50 newlines into one run-on paragraph, and the toast auto-dismissed after 5s. The button's entire product is text to read, so it shipped unreadable. - ToastContainer: whitespace-pre-wrap + break-words on the message, with a bounded scrollable height so a long payload cannot run off the viewport. Single-line toasts render identically to before. - ErrorPanel: a multi-line fix message gets a 60s dwell instead of the 5s default. The toast already has a close button, so the longer dwell is dismissible. One-line outcomes keep the 5s default. 2. The preview had no byte bound. diagnosticsLogTailLines caps the line COUNT only; a child MCP server may print a ~60KB blob per line (the only ceiling is bufio.Scanner's 64KB token limit inside GetServerLogs), so 50 such lines would be a multi-megabyte notification. New diagnosticsPreviewMaxBytes (8KB) drops the OLDEST lines — a tail is read bottom-up, the newest line is the one that explains the failure — and states how many it dropped and where to get the rest. The newest line always survives whole, however long. Individual lines are still never truncated, which is what #1148's scrubUpstreamText decision was actually about. Also closes a vacuous-pass gap the reviewer found in faefdd0's redaction test: asserting only that two secrets are ABSENT from the preview would pass for an implementation that never rendered the two secret-bearing lines at all. The test now pins the non-secret remainder of both lines first, so "the secret is gone" can only mean the scrubber removed it. Verified each new assertion bites: reverting the cap fails BoundsThePayload; reverting either frontend change fails two of the three new frontend specs. Reviewer findings deliberately NOT acted on, with reasons: - "GetServerLogs may exceed the caller's 15s deadline; the context is discarded." Per-server logs are lumberjack-rotated at 10MB (internal/logs/logger.go:33), so the full-file read is tens of ms. TriggerOAuthLogin returns as soon as StartManualOAuth launches its goroutine. Neither fixer blocks. - "FailureMsg discloses an absolute log path." Exact parity with the existing GET /api/v1/servers/{id}/logs handler (internal/httpapi/server.go:3741), on the same API-key-gated surface. Not a new disclosure class. - "diagnostics.Register is process-global; a second Server rebinds the fixers." True and already documented in the file. Production builds one Server per process, no test in internal/server calls t.Parallel(), and each fixer test constructs its server immediately before invoking. Pre-existing, untouched, worth knowing: a single log line over 64KB makes GetServerLogs fail outright (bufio.Scanner token limit), which the REST logs endpoint shares. The end-of-file-fixer pre-commit hook added a trailing newline to ToastContainer.vue, which had been missing one before this change. Not run: ./scripts/test-api-e2e.sh (blanket-pkills other sessions' cores; no route or handler changed) and the Playwright web-UI sweep. Frontend is covered by type-check, build, and 1178 passing unit tests including the 3 new ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…e wire gate Cross-model review (opencode gpt-6-astra) on cfb06ef found that sanitizeMCPXCodeMap enforced a strictly WEAKER predicate than the ScanForPII gate that runs downstream on the same bytes: producer: len<=64 AND ^MCPX_[A-Z0-9_]+$ (shape only) wire: len<=64 AND ^MCPX_[A-Z0-9_]+$ AND diagnostics.Has(code) ScanForPII does not redact the offending key — it drops the ENTIRE heartbeat, logs an error, and bumps the anonymity-violation counter. So an MCPX_-shaped but uncataloged code reaching the provider would have silently destroyed every heartbeat for as long as that standing state persisted. The provider (supervisor.CurrentErrorCodes) prefix-matches "MCPX_" only, so the catalog term was the one that mattered and it was the one missing. Not reachable today — all 44 declared diagnostics codes are registered — but the function exists precisely to be the backstop for a future caller, and as written the backstop was the outage. Switch it to isValidMCPXCode, which is the same predicate scanDiagCodeMap applies, so producer and wire form can no longer disagree. Tests: TestSanitizeMCPXCodeMap gains the uncataloged-but-well-shaped case, plus TestSanitizeMCPXCodeMap_MatchesScannerPredicate, which pins the invariant directly — anything sanitize keeps must be something the wire scanner accepts. Both verified to fail against the old predicate (and the package still builds when neutered, so the failure is the guard biting, not a broken build). Also documents, at shouldNotifyErrorCode, the three sampling imprecisions the review surfaced and that this change accepts: coalesced failures and recovery-ABA under-count, stale reconcile observations over-count. All are bounded by the observation cadence and are orders of magnitude below the ~2880/day/server level-triggering they replace; closing them needs failure identity at the source, which is out of scope. Fixes the stale "current version is 10" comment in payload_v7_test.go left by the v11 bump. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Round-2 cross-model review finding on the previous commit, verified and accepted. Giving a multi-line fix preview a 60s dwell created an accumulation problem the 5s default hid. The toast stack is `position: fixed` anchored to the bottom of the viewport with no height bound, so it grows upward: three or four tall previews raised inside one dwell window push the earliest ones past the top of the screen, close button included, and a scrollbar inside a message cannot bring that button back. Fixed at the source rather than on the shared container: ErrorPanel keeps the id of the long-lived preview it last raised and removes it before raising the next, so at most one tall preview per panel is alive at a time and a new log tail supersedes the one it replaces. Ordinary single-line outcome toasts are untouched and still expire on their own. Bounding the shared stack instead (max-height + overflow on the daisyUI `.toast` container) was rejected: that container is the transition-group root, and clipping it would break the slide-out leave animation for every toast in the app to fix a problem this feature created. New spec asserts three clicks inside the dwell window leave exactly one toast; verified it fails when the supersede is removed. Also confirmed in review, no change needed: the message renders through Vue's `{{ }}` interpolation, so backend-controlled log text is escaped — no injection path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…s own status Cross-model review (opencode, gpt-6-astra) of the transport-family change found three defects. All three reproduce; each fix has a bite check. 1. CanonicalTransport folded the EMPTY transport into the HTTP family. The empty string is reachable in production — internal/runtime/supervisor passes transport=="" whenever state.Config is nil — and the claim that every arm it unlocks needs "positive evidence of an HTTP exchange" was simply untrue for three of the four: a wrapped context.DeadlineExceeded, its stringified form and context.Canceled are transport-agnostic, and the status-text arm reads mcpproxy's stdio exit wrapper, whose attached stderr TAIL routinely quotes a status the CHILD process saw. Measured, with "" in the family: a stdio handshake timeout returned MCPX_HTTP_TIMEOUT, a canceled stdio connect returned MCPX_HTTP_CANCELED, and "exited before completing the MCP initialize handshake; recent stderr: request failed with status 503" returned MCPX_HTTP_5XX. "" now passes through, which is exactly the pre-change behaviour for that path; the four HTTP spellings still fold together, which was the point. 2. matchHTTPStatusText scanned one whole MARKER across the string before trying the next, and "status " is a strict prefix of "status code: ". A status quoted later therefore outranked the one the transport reported: "unexpected status code: 503; body: upstream request failed with status 401" answered MCPX_HTTP_401. The scan is now positional, left to right, longest marker first at each position. 3. The scan also continued past an UNRECOGNISED status, which is the same bug one level down: "request failed with status 402: status 401" answered MCPX_HTTP_401 — "fix your credentials" for a billing failure. It now stops at the first well-formed status TOKEN and reports that token's code, empty included. A run of four digits is not a token, so "status 4011" no longer reads as 401 and keeps scanning. Both phrasings can share one message because mcp-go interpolates the raw response BODY into `request failed with status %d: %s` (streamable_http.go:641, sse.go:630), so this is the shape the parser must survive, not a contrivance. Also, not from the reviewer: - docs/errors/README.md is the sidebar's category landing page and had not been given the four new codes, so they appeared in the sidebar but not in the index. - The MCPX_HTTP_4XX comment described the bucket as everything EXCEPT 400/408/409/410/451; those five ARE the bucket. - The 429 message promised "it will retry after the wait the server asked for". RetryAfterTransport parks only when a parseable Retry-After header is present (internal/transport/retry_after.go:181); without one the normal backoff ladder applies. Reworded to say that. Rejected: an ECONNRESET/ETIMEDOUT wrapped in an OAuth-phase message losing its OAUTH code — no production path emits that shape, and the ungated ECONNREFUSED arm it mirrors predates this change. Rejected: MCPX_HTTP_CANCELED at info severity hiding a repeating fault — no consumer branches on diagnostic severity for a health verdict (internal/health/calculator.go reads connection state), the frontend already renders info, and both the message and the docs page flag the repeat case. Verification: gofmt clean; go build ./... OK; go vet OK; golangci-lint v2 (.github/.golangci.yml) 0 issues; go test -race ./internal/diagnostics/... 453 pass / 0 fail; supervisor, managed, health, httpapi and transport all ok. check-errors-docs-links.sh fails on the same 8 pre-existing pages as main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…decline-research-d382b2

…ecline-research-d382b2

Maintainer directive 2026-09-06: use GPT-6 Astra for both code and spec/plan review; gpt-5.6-sol drops to fallback. Verified reachable the same day via `opencode run --model github-copilot/gpt-6-astra`. Also records the two invocation rules that cost time before: stdin must be closed and the call wrapped in gtimeout, and an empty result is a FAILED review rather than a clean one — opencode exits 0 with no verdict when a read is refused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

codecov-commenter · 2026-09-06T17:24:23Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 85.97122% with 39 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/server/diagnostics_fixers.go	71.11%	21 Missing and 5 partials ⚠️
internal/runtime/supervisor/supervisor.go	78.04%	6 Missing and 3 partials ⚠️
internal/runtime/runtime.go	73.33%	2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

github-actions · 2026-09-06T17:27:06Z

📦 Build Artifacts

Workflow Run: View Run
Branch: claude/new-user-decline-research-d382b2

Available Artifacts

archive-darwin-amd64 (29 MB)
archive-darwin-arm64 (27 MB)
archive-linux-amd64 (17 MB)
archive-linux-arm64 (16 MB)
archive-windows-amd64 (29 MB)
archive-windows-arm64 (26 MB)
frontend-dist-pr (0 MB)
installer-dmg-darwin-amd64 (24 MB)
installer-dmg-darwin-arm64 (21 MB)

How to Download

Option 1: GitHub Web UI (easiest)

Go to the workflow run page linked above
Scroll to the bottom "Artifacts" section
Click on the artifact you want to download

Option 2: GitHub CLI

gh run download 34261332753 --repo smart-mcp-proxy/mcpproxy-go

Note: Artifacts expire in 14 days.

… zero Cross-model review (opencode, gpt-6-astra) of the edge-triggering change returned six findings. Two are genuine and fixed here; four are rejected below with reasons. 1. UNBOUNDED COALESCING, not the bounded kind the comment promised. The edge basis was (classified code, ConnectionInfo.RetryCount). But types.StateManager.SetOAuthError increments oauthRetryCount and NOT retryCount (upstream/types/types.go:726 — types.go:183 already carries a comment about that split for a different reason). A server failing OAuth over and over therefore holds the same code AND the same RetryCount across every attempt, so it edged exactly once and then went silent forever. MCPX_OAUTH_LOGIN_REQUIRED is the widest code in the field, so this was not a corner. The comment claimed all three imprecisions were "bounded by the observation cadence". For this one that was false, which is the same class of defect as the level-triggering being fixed: an instrument describing itself wrongly. Fixed by adding ConnectionInfo.LastRetryTime to the basis. Every failure setter stamps it with time.Now() (SetError:347, SetTerminalError:400, SetPendingAuth:465, SetOAuthError:726); nothing else advances it, and re-observing a sticky error does not touch it — so a fresh value means a fresh ATTEMPT and reconcile's re-reads stay silent. It uses a field the stateview already carries (status.LastErrorTime), so no new state, no new payload field. Two ABA routes the reviewer found also close as a side effect, both of which needed no dropped event: a TryReconnectSync Reset()+Connect() that lands back on the same (code, RetryCount), and a recover-then-refail read through a state fetched after the queued connected event. Tests: TestUpdateStateView_OAuthRetriesReNotifyDespiteFrozenRetryCount (three attempts, RetryCount frozen at 0, want 3 — fails with got=1 when the time term is neutered, package still building) and TestUpdateStateView_StickyAttemptTimeDoesNotReNotify (10 reconcile passes over one unchanged failure, want 1) which pins that this did not re-open the level-triggering. The doc comment now lists the real imprecisions, including a new one this term introduces: one attempt routed through two failure setters can stamp twice, bounded at a small constant per attempt. 2. v10 COUNTS WERE ABOUT TO BE RELABELLED AS v11 ONES. The per-code counters live in a 24h sliding window persisted in BBolt, and nothing about the schema bump touched the key namespace. An install upgrading mid-window would have kept its v10 LEVEL-triggered counts — the ~2880/day/server this change exists to remove — and gone on adding v11 edge increments to them, under the v11 schema label, for up to a day. The bump says v10 and v11 are not comparable; that is worthless if v11's first day IS v10 data. diagKeyCodePrefix moves from "code_count_24h_" to "code_edge_count_24h_". Neither is a prefix of the other, so the cursor scan cannot pick up a legacy key. The orphans are inert, bounded by the catalog (~44 entries), and decay-stale within a day. Test: TestDiagnosticsCounterStore_LegacyLevelTriggeredCountsAreNotAdopted seeds a live v10 record of 2880 and asserts the v11 snapshot is empty, then that one RecordErrorCode gives exactly 1. Reverting the constant fails it with "v10 level-triggered count leaked into the v11 payload: MCPX_OAUTH_LOGIN_REQUIRED=2880". docs/features/telemetry.md says the counters restart at zero on upgrade and why. REJECTED, with reasons: - "Producer and scanner predicates disagree on zero counts, and MarshalJSON caps at 20 while scanDiagCodeMap does not." Correct as stated, and the reviewer says so itself: both are producer-side RESTRICTIONS. The invariant that matters is one-directional — anything sanitize keeps, the wire scanner must accept — and it holds. A producer that emits strictly less than the gate allows cannot cause the heartbeat drop this guards against. - Three further ABA/staleness variants (stale reconcile replay; event-path refail before a queued connected event; Reset/recreate). The last two are closed by the LastRetryTime term above. The first is an over-count already documented at shouldNotifyErrorCode and inherent to diffing two sampled projections; closing it means giving failures a monotonic identity at the source, which is a supervisor-wide change and out of scope. The comment now states it without the false boundedness claim. Verified: go build ./... and -tags server OK gofmt on the four touched files clean golangci-lint v2 (.github/.golangci.yml) telemetry + supervisor 0 issues go test ./internal/... -skip "E2E|Binary|MCPProtocol" 64 packages ok, one failure: TestResolveDockerStatusResolvableAndWorking, which passes -count=3 in isolation and on origin/main — `docker info` timing out under full-suite parallel load, the same shape as the known manager_docker_recovery_test flake. Not related to this change.

cloudflare-workers-and-pages · 2026-09-06T17:35:25Z

Deploying mcpproxy-docs with Cloudflare Pages

Latest commit:	`035599d`
Status:	✅ Deploy successful!
Preview URL:	https://a543e50a.mcpproxy-docs.pages.dev
Branch Preview URL:	https://claude-new-user-decline-rese.mcpproxy-docs.pages.dev

View logs

Round-2 cross-model review (opencode, gpt-6-astra) on the classifier change. One finding, verified and accepted; it also falsifies a claim I made when rejecting a related finding in 43fdd1c ("the frontend already renders info" — it does not). MCPX_HTTP_CANCELED is the first and so far only severity=info entry in the spec-044 catalog (registry.go:304). ServerDetail's gate accepted only warn/error, with the comment "Info-level diagnostics are ignored (shown only in verbose/admin views, per spec)". Two things are wrong with that: - Spec 044 says no such thing. FR-002 only requires a severity; FR-011 is about the macOS TRAY indicator, not the detail view. I grepped the spec for the rule and it is not there. - The exclusion hid nothing. `showDiagnosticPanel=false` falls through to the `v-else-if="server.last_error"` branch — the generic red "Server Error" box that prints the raw error string with no user message, no fix steps and no docs link. So a cancellation (a shutdown, a config reload, a manual disconnect) was rendered LOUDER than a warn-level fault and lost its explanation on the way. ErrorPanel already handles info correctly and always has — alert-info, badge-info, the neutral "Diagnostic" header (ErrorPanel.vue:163-182) — so the defect is in the gate, not the component. Widened to accept info. The generic box is a v-else-if, so the red duplicate disappears with it. Blast radius is exactly one code: nothing else in the catalog is info. The issue #1076 quarantine guard is untouched and still runs first. New spec frontend/tests/unit/server-detail-info-diagnostic.spec.ts mounts ServerDetail with a non-quarantined server carrying last_error AND the info diagnostic, and asserts the panel renders with severity "info" and the user message, and that the generic red box does NOT. Second case re-pins the quarantine suppression with the same payload. Reverting the gate fails the first with "expected false to be true"; the second keeps passing, which is what tells you the guard is independent. Verified: npx vitest run 110 files, 1181 tests, 0 fail npx vue-tsc --noEmit exit 0 the three neighbouring specs (error-panel, server-detail-quarantine-error- panel, diagnostics-fix-log-tail-toast) pass alongside the new one.

…ol_count alive at shutdown Cross-model review (opencode, gpt-6-astra) of the fixer and tool_count changes returned nine findings. Four are genuine and fixed; five are rejected below. FIXED 1. The Sign in button skipped the write gates. fixOAuthReauth called Runtime.TriggerOAuthLogin directly. POST /api/v1/servers/{id}/login does NOT — it goes through the management service, whose checkWriteGates refuses when read_only_mode or disable_management is set (internal/management/service.go). The diagnostics route's own middleware checks caller authorization, not those config gates, so an authorized diagnostics request could start an OAuth flow on an install whose owner had turned management off. Now calls the same TriggerOAuthLoginQuick the REST handler does, and FAILS CLOSED if that service is unavailable rather than falling back to the ungated call. Test: TestDiagnosticFixer_OAuthReauth_RespectsWriteGates, both gates, asserting the refusal names the gate. Reverting to the Runtime call fails both subtests with "the fixer started a sign-in while X was set". 2. It claimed a browser window had opened whether or not one had. StartManualOAuth returns as soon as its goroutine launches, and the launch can fail afterwards. TriggerOAuthLoginQuick returns an OAuthStartResult carrying BrowserOpened/BrowserError, so the message now says what happened and prints the URL to finish manually. Still a success — the flow did start — but not a fabricated one. Given the placeholder this file replaced reported success for doing nothing, a success that means something is the whole point. 3. The 8KB preview cap did not bound the preview. The line loop measured only log lines; the rendered payload also carries a header and an "older lines omitted" notice, both interpolating the server name. A tail whose lines filled the cap shipped ~8.25KB. The budget now subtracts that framing (computed with len(entries) for both counts, so the reservation is always >= the actual). The one documented way past the cap remains the single newest line, which is never dropped or truncated. Test: TestDiagnosticFixer_StdioShowLastLogs_CapCountsItsOwnFraming sizes the fixture so an unbudgeted loop keeps exactly one line more than a budgeted one; it fails at 8253 bytes without the fix. The pre-existing BoundsThePayload assertion also tightened from "< 3x the cap" to "<= the cap" — a slack multiple was not asserting the cap at all. 4. tool_count would have reported 0 in the shutdown heartbeat. Close() closed the index BEFORE telemetryService.Stop(), and Stop performs the graceful final heartbeat flush. GetDocumentCount on a closed index fails, which sent the count down the upstream-cache fallback — and the upstream clients are disconnected by then, so an install with a fully populated durable index would have signed off with tool_count=0. That is the exact structural zero this branch changed the field to remove, reappearing on the last heartbeat. Two changes: Close() now closes the index AFTER the flush (nothing in between touches it — cacheManager.Close, activityService.Stop and telemetryService.Stop are BBolt/HTTP work), and GetToolCount memoises the last SUCCESSFUL index count and reports that when the index cannot answer, so a transient error mid-rebuild cannot produce a zero either. The memo only ever holds a value the index actually returned, and a legitimate empty index stores 0 and falls through as before. Test: TestGetToolCount_ClosedIndexKeepsLastKnownCount. Neutering the memo fails it with the cache-fallback zero. REJECTED - "GetServerLogs ignores the handler's 15s deadline." Per-server logs are lumberjack-rotated at 10MB (internal/logs/logger.go), so the full-file read is tens of milliseconds. Same rejection as the first round, same reason. - "diagnostics.Register is process-global; constructing a second Server rebinds the fixers." True, already documented at the registration site. Production builds one Server per process. - "Duplicate clicks can launch two OAuth flows." Possibly — Manager. StartManualOAuth builds a fresh core client per invocation, so isOAuthInProgress is per-client. But the fixer now makes the SAME call as the REST login button, so whatever that behaviour is, it is a pre-existing property of the login route and not introduced here. The comment that claimed a stronger guarantee has been corrected rather than left standing. - "Multiple long-lived previews can still stack across ErrorPanels." Only one ErrorPanel is ever mounted: ServerDetail.vue:194 is the sole use site in frontend/src, under a v-if on a single server's diagnostic. The cross-panel case cannot arise. Bounding the shared toast container was already rejected in f34ca96 (it is the transition-group root). - "Producer/scanner predicate mismatch on zero counts." Producer-side restrictions, one-directional invariant holds — see the previous commit. Verified: go build ./... and -tags server OK gofmt on the four touched files clean golangci-lint v2 (.github/.golangci.yml) server + runtime 0 issues go test ./internal/... -skip "E2E|Binary|MCPProtocol" 65/65 packages ok, 0 failures (the earlier run's docker-info flake did not recur)

Round-3 cross-model review (opencode, gpt-6-astra) on the previous commit. Three findings, all genuine, all about the same mistake: routing the fixer through the management service's TriggerOAuthLoginQuick to inherit checkWriteGates ALSO swapped in a materially different OAuth implementation. What TriggerOAuthLoginQuick does that TriggerOAuthLogin does not: - StartManualOAuthQuick runs startup SYNCHRONOUSLY under its own 30-minute background context (manager.go), so the fix endpoint's 15s deadline stops bounding the call — resource discovery alone can sit through three 30s rate-limit waits, and the work continues after the caller disconnects. The old path launched a goroutine and returned. - Its watcher polls 60 times at 2s and then runs `defer cancel()` on the callback context, unregistering the pending callback. A user who takes more than ~120s over 2FA loses the flow they were told had started. - HasRecentOAuthCompletion accepts a completion from the last five minutes, and nothing (logout included) clears that map — so signing out and back in makes the watcher exit on its FIRST poll. - It calls CreateHTTPClient unconditionally, dropping the SSE dispatch that ForceOAuthFlowWithResult performs. An OAuth-protected legacy SSE endpoint that challenges GET but rejects POST now fails initialization instead of opening its authorization flow. The REST login route accepts all four; it has no 15s budget and is not a one-click self-heal button. This fixer must not. So the async Runtime.TriggerOAuthLogin call comes back, and the gate — the part of the previous commit that was actually right — is applied explicitly in the fixer: read config, refuse on disable_management or read_only_mode with the SAME message text management.checkWriteGates uses, so an operator gets one answer whichever surface they tried. The predicate is duplicated rather than called because checkWriteGates is unexported and the only exported entry point behind it is the wrong implementation. Fail closed if the config cannot be read. TestDiagnosticFixer_OAuthReauth_RespectsWriteGates is unchanged and still bites: neutering the two checks fails both subtests with "the fixer started a sign-in while X was set". Also reverted with it: the BrowserOpened/BrowserError message, since the async path reports neither. The wording no longer asserts that a window opened — it says where to find the URL if none did. Claiming an unverified success is precisely the placeholder behaviour this file exists to remove, and the previous commit's version of that claim was only true because it had the quick path's return value to lean on. Verified: go build ./... / go vet ./internal/server/ OK gofmt clean golangci-lint v2 (.github/.golangci.yml) server 0 issues go test ./internal/server/ ./internal/httpapi/ -skip "E2E|Binary|MCPProtocol" both ok go test -race ./internal/server/ (full package) ok 252s

Two of the fixer tests failed on CI while passing on every developer machine: "server not found: chatty-stdio", outcome=failed where success was required. Reproduced locally and fixed. CAUSE. newFixerTestServer registered the upstream client GetServerLogs resolves by calling UpstreamManager().AddServerConfig once, at construction. That adds a client the supervisor never asked for, and reconcile diffs DESIRED (cfg.Servers) against ACTUAL (the manager) and deletes the difference — so the client survives only until the next reconcile pass. On a developer machine the test finished first; on CI under -race the pass landed in between. REPRO. Adding `time.Sleep(3s)` between construction and the fixer call fails locally with the exact CI message, and passes with this change. FIX. Re-register at the call site (ensureFixerClient), which closes the window to the microseconds before InvokeFixer. Two tidier-looking alternatives were tried and rejected, both verified: - Putting the server in cfg.Servers as DISABLED does not help: reconcile removes the client for a disabled entry too. The 3s repro still fails. - Putting it there ENABLED does keep the client, and the assertions pass — but it spawns a child and a per-server log writer that outlive srv.Shutdown() and then race t.TempDir()'s RemoveAll, so four tests fail cleanup with "directory not empty". Disabled + re-register is the only combination with neither a background goroutine nor a race. Verified: go test ./internal/server/ -run TestDiagnosticFixer_ ok; the same with -race -count=2 ok; golangci-lint 0 issues; the 3s repro passes. UNRELATED, REPORTED NOT FIXED: the same CI runs show a nil-pointer panic in internal/httpapi.handleAddFromRegistry (server.go:5828) from the internal/server cross-surface consistency test. That handler dereferences cfg after AddServerFromRegistryRef returns (cfg, rerr, err) without guarding cfg==nil on the err==nil branch. The code is byte-identical to main, this branch touches neither the handler nor the registry path, and it does not reproduce locally on this branch OR on origin/main (full package, with and without -race). It needs its own issue and its own nil guard; it is not smuggled into this diff.

…ne-research-d382b2

…, dispose previews with their panel Cross-model review (gpt-6-astra) of the four-instrument fix found four things; three are changed here and one is answered in a comment. - oauth_reauth's success message sent a browserless user to "Show last server log lines". That button reads the per-server log file, but the authorization URL and the browser-launch failure are written by the client's main logger (connection_oauth.go, c.logger) and never reach that file — and the OAuth catalog entries do not offer the log-tail button anyway. The message now names `mcpproxy auth login --server=<name>`, which drives the same flow from a terminal and prints the URL when the browser cannot be opened. The text is extracted to oauthReauthStartedMessage and pinned by a test that fails against the previous wording. - The fixer test fixture registered its out-of-band client BEFORE background initialization had run. LoadConfiguredServers snapshots the manager's clients and schedules `go RemoveServer(name)` for each one not in cfg.Servers, so a client present at that snapshot carries an asynchronous removal that can land after any later re-registration; re-registering at the call site only narrowed that window. The fixture now waits for the "Server is ready" message, which backgroundInitialization publishes right after LoadConfiguredServers returns, so the client is registered after the snapshot and that remover never targets it. The call-site re-registration stays for the periodic reconcile, and the comments now say which remover each half handles. - ErrorPanel's superseding id for its long-lived preview toast lived in the component while the toast lived in the global store; ServerDetail unmounts the panel on every navigation between servers, so the id was lost, the toast stayed, and previews opened across several servers still stacked past the top of the viewport. The panel now removes its preview in onBeforeUnmount. Spec fails before the change (toast count 1, want 0) and passes after. - GetToolCount memoises the last successful index count but prefers it over the upstream cache only when it is positive; the review read the doc comment as promising the memoised value unconditionally. The behaviour is kept and the comment corrected: a memoised zero means the index genuinely held no tools, and in that state the cache cannot produce the structural zero the guard exists for — it can only report more (tools live clients hold that the index has not caught up with), which is the closer answer. Verification: go test -race ./internal/server (fixer + GetServerLogs tests, -count=2) and ./internal/runtime green; go vet and golangci-lint v2 0 issues; frontend vitest 1262/1262.

…at arrive after unmount Second cross-model review round (gpt-6-astra) on the previous fix-up. - The reworded sign-in fallback claimed `mcpproxy auth login` "prints the authorization URL when the browser cannot be opened". That is true only in standalone mode. With the daemon running — the normal case whenever the Sign in button is visible — the CLI takes daemon mode, and although the REST login response carries auth_url and browser_opened, cliclient.TriggerOAuthLogin discards both and auth_cmd prints a generic success line. The message now says what the command does (start the sign-in again) and nothing about a URL; the test forbids the promise. Surfacing auth_url through the CLI is a separate change. - ErrorPanel's unmount cleanup only disposed of a preview that already existed. A click whose request was still in flight at unmount resumed afterwards and raised a 60s toast nothing owned — the same accumulation, one navigation later. A `disposed` flag set in onBeforeUnmount now drops a result that arrives after the panel is gone. Spec drives a deferred invokeDiagnosticFix, unmounts, then resolves it: fails before the guard (toast count 1), passes after. Verification: frontend vitest 1263/1263, vue-tsc clean; go test ./internal/server message test green; go vet and golangci-lint v2 0 issues.

…nly the ownerless preview Third cross-model review round (gpt-6-astra) on the previous fix-up: the disposal guard returned before any outcome was examined, so a fix the user had submitted that then FAILED after they navigated away was never reported. api.ts folds transport errors into resolved {success:false} responses, so leaving `catch` unguarded preserved nothing. The guard is now the narrowest thing that closes the original defect: after unmount, only a successful MULTI-LINE preview — the 60s toast with nothing left to supersede or dispose of it — is dropped. A late failure, a late {success:false}, and a one-line late success ("Sign-in started") are all still raised at the default dwell; multiLine is forced false after disposal so no long-lived toast can be created without an owner, and the parent emit is skipped. Spec "still reports a failure whose request resolves after the panel unmounted" resolves outcome=failed after unmount and expects one error toast at 5000ms: fails against the previous guard (0 toasts), passes now. The earlier late-success spec still expects zero toasts. Verification: frontend vitest 1264/1264, vue-tsc clean.

Dumbris and others added 12 commits September 6, 2026 08:23

Merge branch 'fix/error-counter-edge-triggered' into claude/new-user-…

94bd8c8

…decline-research-d382b2

Merge branch 'fix/classifier-transport-family' into claude/new-user-d…

bbd5c0a

…ecline-research-d382b2

Merge branch 'fix/wire-real-diagnostic-fixers' into claude/new-user-d…

ddaec51

…ecline-research-d382b2

Algis Dumbris and others added 7 commits September 6, 2026 20:39

Merge remote-tracking branch 'origin/main' into claude/new-user-decli…

5b5d281

…ne-research-d382b2

Merge remote-tracking branch 'origin/main' into review/pr-1218

d5ab9ef

Dumbris mentioned this pull request Sep 8, 2026

fix(test): register the GetServerLogs fixture client only after PhaseReady #1228

Open

Dumbris added 2 commits September 8, 2026 21:05

Dumbris merged commit 8f1bdc9 into main Sep 8, 2026
42 checks passed

Navigation Menu

Uh oh!

fix(telemetry): stop four diagnostics instruments measuring themselves - #1218

fix(telemetry): stop four diagnostics instruments measuring themselves#1218
Dumbris merged 22 commits into
mainfrom
claude/new-user-decline-research-d382b2

Dumbris commented Sep 6, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Sep 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 6, 2026 •

edited

Loading

Uh oh!

cloudflare-workers-and-pages Bot commented Sep 6, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Dumbris commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. tool_count read a cache that indexing zeroes (ae5414787)

2. diagnostics.error_code_counts_24h was level-triggered (cfb06ef3d, 223eb645b)

3. The classifier only knew one spelling of "http" (79a75cf5a, 43fdd1c2d)

4. The registered "fixers" were placeholders (faefdd05c, f34ca9630, c080ab981)

Also in this branch

Cross-model review (opencode, gpt-6-astra)

Verification

Uh oh!

codecov-commenter commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Build Artifacts

Available Artifacts

How to Download

Uh oh!

cloudflare-workers-and-pages Bot commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying mcpproxy-docs with Cloudflare Pages

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Dumbris commented Sep 6, 2026 •

edited

Loading

1. `tool_count` read a cache that indexing zeroes (`ae5414787`)

2. `diagnostics.error_code_counts_24h` was level-triggered (`cfb06ef3d`, `223eb645b`)

3. The classifier only knew one spelling of "http" (`79a75cf5a`, `43fdd1c2d`)

4. The registered "fixers" were placeholders (`faefdd05c`, `f34ca9630`, `c080ab981`)

codecov-commenter commented Sep 6, 2026 •

edited

Loading

github-actions Bot commented Sep 6, 2026 •

edited

Loading

cloudflare-workers-and-pages Bot commented Sep 6, 2026 •

edited

Loading