Repository navigation
fix(scope): retrieve_tools filters by scope before limiting (Spec 105 PR C, FR-005) - #1325
Merged
Merged
Conversation
…risk by scope before limiting (Spec 105 PR C, FR-005) Closes gaps FR005-G1..G5: a scoped agent token's retrieve_tools call now runs its search through SearchToolsScoped (filter-before-the-cut, Spec 107) instead of Search+post-filter, so an out-of-scope hit can no longer displace an authorized one from a limited window. debug.total_indexed_tools, usage_summary.top_tools and session_risk are likewise computed over the caller's authorized population only — usage stats additionally require approval (not just index membership), since a removed tool can keep a stale approved record and a pending tool stays indexed. A profile-scoped administrator on the shared-index fallback (no per-profile index available) also gets the exhaustive-filtered top-K instead of the pre-feature empty-page result (D3, a named SC-005 exception); every other administrator path — unprofiled, or a working per-profile index — is byte-for-byte unchanged, confirmed by the existing golden tests. - index: add BleveIndex/Manager.ScopedDocumentCount (server_name facet filtered by an inScope predicate), reusing the existing SearchToolsScoped from Spec 107 rather than re-deriving scoped search mechanics. - storage: add Manager.GetToolStatsFiltered — sorts and cuts to topN AFTER filtering, so a hidden or unapproved tool's usage can never evict an authorized one from the window. - server: hoist auth/profile resolution ahead of the search call, route scoped callers (and the D3 admin exception) through SearchToolsScoped, add scopedIndexedToolCount/usageStatEligible (mcp_visibility.go) and analyzeSessionRiskScoped (mcp_annotations.go). - Tests: internal/index/manager_scoped_test.go proves SearchToolsScoped is ordered-(id,score)-identical to the exhaustive Search+filter+cut derivation (D6) on a small fixture and the 527-tool LiveMCPBench snapshot; internal/server/mcp_retrieve_scope_test.go, mcp_session_risk_test.go and scope_latency_test.go cover G1-G5 and an FR-011 latency pre-check (scoped-vs-admin p95 gap ~1ms on the 527-tool snapshot, well under the 20ms budget). - ROADMAP.md regenerated (scripts/gen-roadmap.py) to reflect T069-T078. Closes T069-T078 of specs/105-agent-scope-hardening/tasks.md Phase 7. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…st and ScopedDocumentCount sizes its facet exactly (Spec 105 PR C review round 1) Codex (gpt-5.6-sol) cross-review of the FR-005 retrieve_tools scope fix found two MUST-FIX gaps in the new scoped search/count mechanics: 1. SearchToolsScoped ran the plain boolean query only, silently skipping the underscore-segment enhancement SearchTools(query, limit) applies for identifier-style queries (e.g. "work_upload_attachment" matching "work_start_task_attachment_upload"). A scoped caller searching for a real underscore-shaped tool name could get a worse-ranked or missing result where an equal-limit unscoped call would find it. Extracted the shared decision into augmentedToolSearchQuery(queryStr, probeSize), used by both SearchTools and SearchToolsScoped with probeSize = the caller's own limit, so the two can never diverge for the same (query, limit). 2. ScopedDocumentCount's server_name facet was capped at a fixed 10000 terms (copied from GetAllIndexedServerNames' "generous upper bound" pattern); past that many distinct servers, the excess would spill into Bleve's "Other" bucket and undercount an authorized population. The facet is now sized to the index's own document count — a keyword-analyzed server_name field contributes exactly one term per document, so #distinct servers <= #documents always, making this a proven exact bound rather than a large-enough guess. Added TestBleveIndex_SearchToolsScoped_AppliesUnderscoreSegmentEnhancement (internal/index/search_scoped_test.go) proving the fix; full index/server/ storage suites and the CI-equivalent golangci-lint config still pass clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Round 2 codex (gpt-5.6-sol) review returned VERDICT: CLEAN with no MUST-FIX findings (round 1's two MUST-FIX issues were fixed in the prior commit; its one non-blocking note — a stale comment in manager_scoped_test.go describing the pre-fix behavior — is corrected here too). opencode quota exhaustion was reconfirmed live on both Terra and Sol before falling back to codex, per CLAUDE.md's fallback rule. Closes T079 of specs/105-agent-scope-hardening/tasks.md — all of Phase 7 (PR C, FR-005, T069-T079) is now done. ROADMAP.md regenerated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
5132471
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bad23f62.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://105-pr-c-scope-retrieve-tool.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35498271732 --repo smart-mcp-proxy/mcpproxy-go
|
Dumbris
added a commit
that referenced
this pull request
Sep 20, 2026
…tion to done (#1327) Spec 105 PR C (#1325) and PR F (#1326) both merged; roadmap.yaml still read status: todo for both since the implementing agents intentionally left them unflipped pending merge review, per this repo's "no merge without instruction" convention for security PRs. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Sep 21, 2026
…ng holds (Spec 105 PR H1) (#1332) Spec 105 PR H1 — `scope-regression-suite` part 2 (FR-011/013/014, SC-007, gaps FR01x-G4..G7). Closes tasks T107-T116. This is the FINAL phase of the nine-PR agent-scope-hardening epic (A, B, D, E, H0, C, F, G already merged). ## Changes - **Two-fixture differential oracle** (`internal/server/scope_differential_test.go`, new): `newScopeFixture`/`runScopeScenario`/`normalizeScopeResponse` per contracts/differential-oracle.md, plus `TestScopeCoverage_EveryUserStoryScenario` registering every US1.1-1.8/US2.1-2.6/US3.1-3.4 scenario by id — some directly via the general three-server harness, most by re-running each prior PR's own dedicated differential fixture (plan.md: "H1 re-registers them by User Story id"). - **Pinned-reversal grep guard** (`internal/server/scope_pinned_reversal_guard_test.go`, new): `TestScopePinnedReversalsStayInverted` proves none of the four pre-105 insecure assertions gap-map.md's FR01x-G7 entry names have crept back to their original form, traced to their exact pre/post-105 diffs. - **Retained-effect fixtures** (`internal/server/scope_retained_effects_test.go`, new): real reproductions of 5 of 7 SC-001-excluded effects (shared-limiter contention, cross-server scan admission via the actual shadowing.cross_server impersonation-clone mechanism, prompt-name collision, global prompt cap, direct display-name collision), 1 via a companion package (`internal/upstream/manager_prompts_deadline_test.go`, new — shared vs independent prompt-refresh deadline, two discriminating regimes), 1 cited cross-package (log rotation). - **HTTP credential matrix** (`internal/server/scope_http_matrix_test.go`, new): real minted tokens over real loopback HTTP through mcpAuthMiddleware, covering every FR-014 applicability-matrix row at least once plus a content-level (raw wire-text) non-disclosure proof for retrieve_tools. - **Latency harness finalized** (`internal/server/scope_latency_test.go`): all four FR-011 operations (retrieve_tools, read_cache, prompts/list, tools/list) with real workloads, plus `.github/workflows/scope-latency.yml` (new) and `cmd/scope-latency-compare` (new, stdlib-only) for the merge-base p95 regression gate. - **Bug fix found via this suite**: `internal/logs.ReadUpstreamServerLogTail`/ `ReadUpstreamServerLogTailAttributed` dereferenced a nil `*config.LogConfig` — fixed with a TDD red/green pair (`internal/logs/nil_log_config_test.go`). - Docs and roadmap sync: `docs/features/agent-tokens.md` invariant wording finalized; `roadmap.yaml`'s `scope-refusal-shapes` task (stale `todo` despite PR G #1328 already merged) flipped to `done`. `scope-regression-suite` and the epic-level status are deliberately left for the maintainer to flip on merge. ## Testing - `go build`/`go vet` (bare + `-tags server`): clean - Full `-race` suite across internal/server, internal/upstream, internal/cache, internal/logs, cmd/scope-latency-compare: green - `golangci-lint --new-from-rev=origin/main` (bare + `--build-tags server`): 0 new issues - `./scripts/test-api-e2e.sh`: 70/70 passed - Cross-model review: opencode terra/sol/astra confirmed quota-exhausted; fell back to `codex exec --model gpt-5.6-sol`. 4 rounds, every finding verified genuine before fixing (non-vacuous confirmed by temporarily reverting the underlying guard and observing red). `VERDICT: clean` (round 4) — full history in tasks.md T116. Related #1223 #1224 #1225 #1226 #1227 #1279 #1282 #1283 #1284 #1285 #1325 #1326 #1328
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes Spec 105 PR C (FR-005, gaps FR005-G1..G5):
retrieve_toolscomputedsearch,
total, indexed counts, usage ranking and session risk over thewhole fleet before any per-request scope filter applied, so a scoped
agent token could lose an authorized top-K hit to a higher-scoring hidden
server's tool, and could learn aggregate counts/ranking/risk shaped by
servers it cannot see.
shared-index fallback — D3, a named SC-005 exception) now go through
SearchToolsScoped(filter-before-the-cut, reused from Spec 107) insteadof
Search+ post-filter. Every other administrator path is byte-for-byteunchanged (confirmed by the existing golden tests).
usage_summary.top_toolsfor a scoped caller is nowfiltered by current authorized-population membership and approval,
sorted and cut to 10 after filtering (
storage.GetToolStatsFiltered) —a removed-but-still-approved tool and an indexed-but-pending tool are both
excluded; the admin path (
GetToolStats(10)) is untouched.analyzeSessionRiskScopedrestricts the lethal-trifecta analysis to the caller's authorized servers; admin keeps the
full-fleet
analyzeSessionRisk.debug.total_indexed_toolsfor a scoped callercomes from
index.ScopedDocumentCount(aserver_namefacet filtered bythe effective scope), agreeing whether the search itself ran against the
shared index or an already-scoped per-profile index.
TestRetrieveTools_ScopeOracleexercises all of theabove together over one three-server fixture.
Cross-model review
opencode quota was exhausted on both
github-copilot/gpt-5.6-terraandgpt-5.6-sol(reconfirmed live before falling back, per this repo'sCLAUDE.md fallback rule), so review ran via
codex exec --model gpt-5.6-sol.SearchToolsScopedsilently skipped theunderscore-segment ranking enhancement
SearchToolsapplies foridentifier-style queries (e.g.
create_issue), andScopedDocumentCount'sfacet was capped at a fixed 10000 server terms. Both fixed: a shared
augmentedToolSearchQuery(queryStr, probeSize)helper now backs bothSearchToolsandSearchToolsScopedso they can never diverge for thesame query/limit; the facet is now sized to the index's own document count
(a keyword-analyzed field emits exactly one term per document, so this is
a proven exact bound, not a guess). New regression test:
TestBleveIndex_SearchToolsScoped_AppliesUnderscoreSegmentEnhancement.VERDICT: CLEAN, no MUST-FIX (one non-blocking stale-commentnote, also fixed).
Test plan
go build ./.../go build -tags server -o /dev/null ./...go vet ./.../go vet -tags server ./...go test -race ./internal/index/... ./internal/storage/...go test -race -skip "E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/server/...(targeted retrieve/scope tests) and the full un-raced package with the same skip regex (known pre-existing hang without it, documented in this repo's CLAUDE.md)/opt/homebrew/bin/golangci-lint run --config .github/.golangci.yml ./...and the--build-tags servervariant — 0 new issues (6 pre-existing, unrelated to this diff)TestRetrieveTools_ScopeLatency_ScopedVsAdmin(FR-011 pre-check, skipped under-race): scoped-vs-admin p95 gap ~1.2ms on the 527-tool LiveMCPBench snapshot, well under the 20ms budgetretrieve_full_*/TestRetrieveToolsFullMode_GoldenByteIdentitygoldens green, unregeneratedNew tests:
internal/index/manager_scoped_test.go,internal/server/mcp_retrieve_scope_test.go,internal/server/scope_latency_test.go, plus additions tointernal/index/search_scoped_test.goandinternal/server/mcp_session_risk_test.go.Not merging — leaving for review, per the sibling PRs' convention.
🤖 Generated with Claude Code