feat(telemetry): preflight baseline counters (filter-diagnostics engagement + availability blocks) - #995
Merged
Merged
Conversation
…gement + availability blocks) Phase 0 of the required-tools-preflight roadmap. These counters ship ONE RELEASE AHEAD of the preflight feature on purpose: the metric preflight is meant to move is "how often does an agent silently fail to reach a tool that exists", and without a live pre-feature window there is nothing to compare the post-feature numbers against — "did preflight help?" would degrade into an argument about anecdotes. Shipping the measurement first buys a real before/after out of the normal release cadence. Two counter families, both on the existing Phase-H 24h-counter substrate (BBolt bucket, encodeCounter/readCounterWithDecay, closed enum keys, capped cardinality, counts only): 1. Filter-diagnostics engagement (spec 094 shipped with zero usage measurement): filter_diag_emitted_24h at the attach site, filter_diag_missing_annotation_24h / filter_diag_explicit_24h summed from the counts the block already computes, and filter_diag_followed_24h — a later retrieve_tools call in the same MCP session that dropped or relaxed a filter the block blamed. The follow-through note is in-memory, per session, TTL-bounded, capacity-capped and consumed on first use; it holds only the filter keys the response already named, and never leaves the process. 2. Availability / discovery-omission classes, derived from the STRUCTURED decision points rather than from error strings: every call site of the single policy-decision funnel now declares a reason key from a closed enum, feeding availability_block_24h plus availability_block_reasons_24h; and discovery_omission_24h counts retrieve_tools responses that withheld locked or quarantined matches the caller could not see. Privacy: counts only. The operator-facing block reason — which embeds server and tool names — stays in the activity log; a key outside the enum is folded into "other" at write time, filtered again at read time and in MarshalJSON, and a new anonymity-scanner rule (preflight_field_invalid) blocks the heartbeat if a non-enum key ever reaches the serialized payload. Counters are gated at EVENT time, so nothing is persisted while telemetry is opted out. The whole preflight sub-object is omitted when every counter is zero, so an install that never trips one emits a payload shape-identical to today's. Related #969
Deploying mcpproxy-docs with
|
| Latest commit: |
e9c5f23
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ce61b023.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://telemetry-preflight-baseline.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 31906076942 --repo smart-mcp-proxy/mcpproxy-go
|
…l review Related #969 Cross-model (opencode/gpt-5.6-sol) review of the Phase-0 baseline counters found five accounting defects. All are in the *attribution* of the counters, not their privacy posture — no payload shape changed. Filter-diagnostics follow-through (in-memory session notes): - Two retrieve_tools calls of one session can be in flight at once and finish out of order. A late-landing EARLIER call overwrote the note describing the block the agent most recently received, which both compared the next call against the wrong filter set and back-dated the TTL. noteFilterDiagnostics now keeps the newer note. - The read side had the mirror defect: a call that STARTED before the note was written passed the TTL check (a negative age is never > TTL), counted itself as a follow-up to a block it never saw, and consumed the note the genuinely later call needed. consumeFilterDiagFollowUp now leaves such a note in place. - Refreshing a session that already had a note ran the capacity prune even though replacing a key cannot grow the map, evicting an unrelated session's still-eligible note. Emission accounting: - The emitted counter and the note were hooked at the ATTACH site, before tool_response_limit truncation, which can cut the block back out of the delivered payload (SimpleTruncate is a plain tail cut). A block the agent never received was counted as emitted and could be "followed". Both now run after truncation, guarded by filterDiagnosticsSurvived. Since emitted is the denominator of the followed/emitted ratio, overcounting it understated the one number these counters exist to measure. Availability block reasons: - Direct mode funnels server-quarantine, pending approval, changed approval and plain not-callable through one emit site, so every direct-mode block was counted as tool_not_callable. directBlockReasonKey now derives the key from the gate that fired, mirroring directToolCallabilityResult's branches. - Direct-mode agent-token scope and permission denials returned with no policy decision at all — no activity record and no counter. They now emit the same decision the call_tool_* variants emit at the equivalent gate; sessionID and requestID resolution moved above those gates so the records stay correlatable. Reviewer findings not acted on: the opt-out check/send non-atomicity in Service.sendHeartbeat is a pre-existing property of the whole telemetry service (the preflight sink already applies the strictest existing gate, event-time opt-out), and the synchronous BBolt writes follow the established precedent — handleRetrieveToolsWithMode already performs two unconditional counter commits per call on main. Both belong in their own change.
…t block Related #969 Round-3 findings from the cross-model review, both against the round-2 fixes. Partial blocks no longer count as emitted. Checking only that the delivered text still contains the "filter_diagnostics" key was too weak: both truncation paths cut the serialized response at a byte offset and append a plain-text notice, so a cut can land inside the block and leave the key with an unterminated value the agent cannot act on. filterDiagnosticsSurvived now verifies the block's object is balanced and closed within the delivered text. Parsing the payload as a whole is not an option — the appended notice means the delivered text is never valid JSON, so that check would have reported false for every truncated response and traded overcounting for undercounting. Notes are stamped at DELIVERY time rather than request start. Overlapping calls in one session can finish out of order, so start time ordered the notes backwards: a call that started first but returned last is the response the agent saw last, yet its note was rejected as stale. Delivery time makes the follow-through comparison a plain causality test — a reaction can only come from a call that started after the block reached the agent — which also removes the equal-start-timestamp edge case, since the two sides are now sampled at different moments by construction. The TTL is measured from delivery too, which is what the 15-minute window was always meant to mean.
Related #969 Table-drives the hasCompleteJSONObject cases, adding the escape-sequence ones: an escaped backslash must be consumed as data so the quote after it genuinely closes the string, and a payload cut immediately after one must read as incomplete. A scanner that desynchronised on those would call a truncated diagnostics block complete and count an emission the agent never received.
Related #969 Every counter key carries its own window start, so the aggregate availability_block_24h and the per-reason keys it summarises decayed independently. A reason first seen at t=23h opened a window that outlived the total's: at t=25h the total reset to 0 while that reason still reported 1, emitting a payload whose reason counts sum to more than the total they split. The docs call that field "the same total split by reason", so the two disagreeing corrupts exactly the baseline ratios these counters exist to establish. The total is now derived at snapshot time as the sum of the reason counts instead of being stored. Every block bumps exactly one reason key (unknown and over-budget keys fold into "other"), so the sum IS the total by construction and the two can no longer drift apart. This also drops one BBolt write per blocked call. There is deliberately no availability_block_24h storage key any more. Test: TestPreflightSnapshot_TotalAlwaysMatchesReasonSplit, which fails against the previous stored-total implementation.
Dumbris
enabled auto-merge (squash)
August 15, 2026 20:47
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Dumbris
added a commit
that referenced
this pull request
Aug 15, 2026
…y check with reason taxonomy (spec 098) (#996) Related #969 Implements item 3 of #969 (the reporter-confirmed shape: cron/CI-first, REST+CLI, stat-only): a deterministic, side-effect-free availability check for a caller-supplied list of tool IDs, answering per-ID with a machine-readable reason instead of failing silently at discovery time. Lands on top of #995 (baseline counters), which shipped one release ahead per the Phase-0 decision. ## What - **`POST /api/v1/preflight`** — `{tools:[{id, pin_hash?}], profile?, policy?, wait_ms?}` → per-ID verdicts. HTTP 200 whenever the check executed (the verdict is data); 400 validation / 503 runtime-or-persistence failure. Standard `APIResponse` envelope. - **`mcpproxy tools preflight`** — typed exit codes `0` ready · `10` retryable (backoff) · `11` blocked (page operator) · `12` unknown ID (fix manifest); worst class wins. A cron wrapper branches without parsing JSON. - **One shared eligibility evaluator** (`internal/preflight`): closed 15-code reason taxonomy with a fixed precedence chain; existence = index ∨ approval record (the runtime de-indexes blocked/pending/changed tools — index absence alone proves nothing); zero upstream I/O and zero runtime mutation, structurally (the evaluator cannot reach a transport; asserted by instrumented-transport + state-snapshot tests). - **Four-path dispatch consolidation** (FR-002 no-skew): call_tool variants, direct mode, code_execution and stored scripts all consume the same gate. Notable hardening: **scripts previously enforced no per-tool policy at all** — a code_execution/stored script could call a quarantined tool; now refused like every other path. - **Transparency**: every executed preflight writes a durable activity record (synchronous, before the 200 — persistence failure ⇒ 503), request-ID-correlated and browsable via `mcpproxy activity list --request-id …` and the Web UI. - **Disclosure tiers**: operator (API key/socket/pipe) gets full diagnosis incl. `sha256/v{N}:{hex}` hash pins; agent tokens get scope-silence (out-of-scope, unconfigured — byte-indistinguishable `not_found`; no hashes; `did_you_mean` never crosses scope). Stale token ProfilePin now intersects to deny-all. - **Zero MCP-surface change**: `tools/list` byte-identical to merge-base across all three routing modes (golden-snapshot test, goldens captured from origin/main). ## Verification - **Sabotage E2E matrix** (`testdata/preflight_sabotage_matrix.json`): 23 scenario cells against a real binary + live fixture upstreams — quarantine flip, rug-pull drift, block, config-deny, disable, SIGSTOP/kill, mid-indexing, annotation cells per filter, unknown id/server, hash + schema-version mismatch, PendingAuth, profile scope at both tiers — each asserting exact {reason, retryable, action} + the activity record by request ID; a reflection test forbids enum codes without a cell. - Full gates: `go test -race ./...` (79 pkgs), server-edition build/tests/lint, 65/65 API E2E, golangci v2 both tag sets, swagger + generate-types idempotent, frontend build + 605 unit tests. - code_execution + stored scripts: byte-identical transcripts vs main on a 9-probe harness; live no-skew check across all four dispatch paths. ## Review Spec, plan, and full diff cross-model reviewed (opencode / gpt-5.6-sol): spec 3 rounds → APPROVE; plan 25 findings (12 P1) incorporated → APPROVE; code round 1 = 11 findings (6 P1, all genuine, each fix verified by restoring the old code), round 2 = 1 regression (startup-window 503 → `server_initializing`), round 3 → **APPROVE**. ## Docs New `docs/features/tools-preflight.md` (taxonomy, precedence, tiers, transparency, cron/GHA/n8n recipes, code-exec composition), REST + CLI references, README "How AI Agents Work Through MCPProxy" section with a preflight-gated automation example. Known follow-up (deliberately out of scope, pre-existing): `resolveActiveProfile` warn-skips a stale ProfilePin on the live session path — preflight is now stricter than dispatch there; the session path deserves its own reviewed fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related #969
Phase 0 of the required-tools-preflight roadmap: baseline counters that must ship one release ahead of the preflight feature (#969 item 3) so a genuine live before/after exists for the 60-day effectiveness review.
What
Two counter families, both following the existing Phase-H 24h-counter pattern (BBolt-backed, closed enum keys, capped cardinality, opt-out respected, counts only — never tool/server names or free text), flushed into the heartbeat as
payload.Preflight:Spec 094 filter-diagnostics engagement (shipped in v0.55.0 with zero usage measurement until now):
filter_diag_emitted_24h,filter_diag_missing_annotation_24h,filter_diag_explicit_24hfilter_diag_followed_24h— in-memory per-session note (15-min TTL, 256-session cap, never persisted, never leaves the process) detecting when a diagnostics suggestion was acted onAvailability / discovery-omission substrate (what the preflight's silent-vs-reasoned metric will read):
emitActivityPolicyDecisiongains a structured closed-enumreasonKeyacross all 14 call sites (no error-string parsing)availability_block_24h+ per-reason map (12 closed keys),discovery_omission_24hPrivacy
Four independent enforcement layers: write-time normalization (unknown →
other), snapshot filter, MarshalJSON filter + cap, and a new anonymity-scanner rule (preflight_field_invalid) that blocks the heartbeat if a non-enum key ever reaches the payload — the sub-object is a closed key set pinned by test to exactly what MarshalJSON emits. Opt-out enforced at event time (strictest existing posture).Testing
go test -racegreen: internal/telemetry, internal/server (CI-shaped), internal/runtime, serveredition (-tags server)TestRegistryAddCLIE2E(reproduces identically on pristine origin/main)Docs: docs/features/telemetry.md updated (incl. the honest caveat that
followed/emittedis a lower bound — sessions without an MCP session id can never incrementfollowed).