feat(server): retrieve_tools filter diagnostics (spec 094) - #970
Merged
Conversation
Related #969 Spec for a compact filter_diagnostics block explaining tools omitted by annotation-based safety filters (read_only_only, exclude_destructive, exclude_open_world): matched-before-filters count, per-filter omission counts split by missing-vs-explicit annotations, bounded affected-server list, one adaptive suggestion string. Absent on the happy path; identical in full and compact modes. Items 2-3 of #969 explicitly out of scope.
…fer server names, window semantics Related #969 All 8 required changes from the cross-model review applied: omitted_servers deferred (stale-quarantine-index leak + unbounded size); FR-003 now carries a normative filter_diagnostics schema with omission rules and reason classification; candidate-window semantics replace the raw-limit claim (index normalizes limit<=0 to 20, no backfill); cause-precedence suggestion selection; FR-007 protects every existing field; all three retrieve_tools registrations enumerated and the default schema gains the filter params; FR-010 scoped to the /mcp code-execution routing response; SC-003 is an exact 600-byte serialized bound.
…03, out-of-scope invisibility Related #969
…al SC-003 fixture Related #969
…ema + quickstart Related #969
…ortened suggestions, full test matrix Related #969 Shortened FR-006 suggestion templates (163/149 chars worst case, filters named once); parity test rebuilt around a frozen pre-refactor oracle + truth table over the corrected 224-case domain; contract schema forbids zero-count reason pairs; test matrix expanded (raw-byte presence/absence in both modes, delete-key equivalence, menu-surface controlled-delta widening, cross- registration schema deep-compare, diagnostics-active TOON case, locked+ filtered coexistence, scope non-contribution); quickstart uses a controlled unannotated fixture; Go version and SC-003 byte figures corrected.
…re, corrected template arithmetic Related #969
…e goldens, window-semantics tests Related #969
…003 split to characterization phase, dependency graph fixed Related #969
…002) shouldExclude now delegates to excludeReason, which additionally reports the first filter responsible for an omission and whether the decisive annotation hint was explicitly unsafe or simply missing. Filter semantics are unchanged: the parity test embeds the pre-refactor shouldExclude body verbatim as an independent oracle and exhausts all 224 cases (28 annotation states x 8 filter combinations), asserting excluded/filterKey/explicit on every one. Related #969
…c 094 T003) Six goldens captured while the branch still has pre-feature behavior: the no-filters, filters-with-zero-omissions and filters-with-omissions conditions, each in full and compact serialization. They turn SC-002 into a byte-level guarantee instead of a key-absence check — for the omissions condition the comparison splices the filter_diagnostics member out of the raw bytes rather than round-tripping through a decoder, so the remaining bytes are the originals. Regeneration is deliberate only: UPDATE_SPEC094_GOLDENS=1. Related #969
…4 T004-T009) Every behavior test for the feature, written against pre-feature code and observed failing: presence/absence in both serialization modes, the normative FR-003 byte shape, the candidate-window semantics (a non-positive caller limit is normalized to 20 by the search layer, and visibility drops shrink the window without backfill), the missing-vs-explicit reason split including the read-only shortcut, suggestion selection over all seven filter subsets, zero-result and locked-tool coexistence, the 500-byte bound, cross-surface parity, and TOON isolation. The package deliberately does not compile at this commit: the production types land in T010. Related #969
When an annotation filter withholds a query match, the response now carries a compact block saying how many tools entered the candidate window, how many each filter withheld, and whether the cause was a missing upstream annotation (fixable) or an explicitly unsafe one (the filter working as intended), plus one actionable suggestion. Counts and one string only — never tool or server identities — which keeps the block leak-proof for a quarantined tool lingering in a stale index and bounds it at 463 bytes in the worst reachable case. The block is self-suppressing: with no filter active, or with filters that omitted nothing, the response is byte-identical to before, so no configuration knob is needed. All three retrieve_tools surfaces share the handler, so the code-execution surface gets it for free. Related #969
…rieve_tools registrations (spec 094 T011-T012) The default registration never exposed read_only_only, exclude_destructive or exclude_open_world, even though the handler has always honored them — agents on that surface simply could not discover the filters, which is where the field report's confusion began. All three registrations now build those parameters from one helper, the same anti-drift pattern retrieveToolsDetailOption already uses, and all three descriptions gain the diagnostics mention plus a fixed caveat that the counts describe one call's candidate window rather than the catalog. The menu-surface golden stays frozen; its controlled delta is widened to permit exactly these changes, and a new deep-compare test pins the three filter schemas to each other so they cannot drift apart again. Related #969
Ran the quickstart end-to-end on an isolated instance (port 18972, scratch dirs, stopped by PID): unannotated fixture tools are withheld by read_only_only with a missing_annotation count and the upstream-annotations suggestion, the key is absent once the filter is dropped, and the block is identical under detail=compact. No docs page enumerates the retrieve_tools annotation-filter parameters, so none needed the filter_diagnostics mention. Related #969
go test -race ./internal/server/... and ./internal/... are green, and golangci-lint v2 reports no issues. Two cleanups on the way there: the attribution oracle spells out "explicitly read-only" instead of negating a conjunction (staticcheck QF1001), and the serialization-mode table drops a redundant identity map. Related #969
Deploying mcpproxy-docs with
|
| Latest commit: |
6d1e758
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e562118e.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://094-filter-diagnostics.mcpproxy-docs.pages.dev |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 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 31365681290 --repo smart-mcp-proxy/mcpproxy-go
|
…ts (Codex round 1) The no-backfill test asked for a window of 20 against six matching hits, so the whole match set fit and an implementation that refilled a visibility-dropped slot from a lower-ranked hit would have passed unchanged. It now indexes six tools of strictly increasing length (six distinct BM25 scores, unambiguous ranking), takes a window of three, locks the top-ranked hit, and asserts both that no rank-4+ tool takes the freed slot and that matched_before_filters reports 2 rather than the search cap or the match set. Widening the window back to six makes every one of those assertions fire. The shape test only marshaled a hand-built struct, and every decoder here ignores unknown fields, so a handler attaching tool names or servers would have gone unnoticed. Added an assertion over a real handler response: the exact member set of the block and of each per-filter entry, plus a byte comparison of the whole block. Related #969
Related #969 The six TestSpec094Goldens comparisons failed on windows-latest: autocrlf checked the fixtures out with CRLF, appending \r to the expected bytes. The existing testdata/*.golden.json rule does not cross into spec094/.
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
When annotation-based safety filters (
read_only_only,exclude_destructive,exclude_open_world) remove query-matched tools from aretrieve_toolsresponse, the response now carries a compactfilter_diagnosticsblock — so a caller can distinguish "tool doesn't exist" from "tool exists but was withheld by a filter, and here's why". Directly addresses the field report in #969 (item 1 only; items 2–3 deferred per the reporter's own suggestion).What's in the block (FR-003, normative)
retrieve_toolsregistration now exposes the three filter params (previously only the routing-mode registrations declared them), via a sharedretrieveToolsAnnotationFilterOptions()helper so the three schemas can't drift.Design provenance
Spec → plan → tasks each cross-model (Codex) reviewed to APPROVE before the next stage (spec: 4 rounds; plan: 3; tasks: 4). Implementation is strict TDD per
specs/094-filter-diagnostics/tasks.md: frozen-oracle parity test (224 cases) → pre-feature response goldens → RED wall (every assertion observed failing) → implementation → registration helper. Note: the RED-wall commit4b212bd16intentionally does not compile in isolation (tests-before-implementation); the PR HEAD is green.Testing
go test -race ./internal/server/...— ok (562s full suite)go test -race ./internal/...— okgolangci-lint run --config .github/.golangci.yml ./...— 0 issuestests/echo-rugpull-serverunannotated fixture): filtered query returns the block withmissing_annotation:1+ suggestion; unfiltered query returns the tool with no block; compact mode byte-identical block