From 446b25c82afdc4a8c38b8df8d4de7880495358e6 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sun, 20 Sep 2026 23:09:50 +0300 Subject: [PATCH] test(scope): differential regression suite proves agent-scope-hardening holds (Spec 105 PR H1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/scope-latency.yml | 130 +++++ ROADMAP.md | 10 +- cmd/scope-latency-compare/main.go | 137 +++++ cmd/scope-latency-compare/main_test.go | 168 ++++++ docs/features/agent-tokens.md | 28 +- internal/logs/attribution.go | 11 +- internal/logs/logger.go | 11 +- internal/logs/nil_log_config_test.go | 41 ++ internal/server/scope_differential_test.go | 496 ++++++++++++++++++ internal/server/scope_http_matrix_test.go | 418 +++++++++++++++ internal/server/scope_latency_test.go | 308 +++++++++++ .../scope_pinned_reversal_guard_test.go | 169 ++++++ .../server/scope_retained_effects_test.go | 456 ++++++++++++++++ .../server/testdata/scope_latency/README.md | 24 + .../upstream/manager_prompts_deadline_test.go | 243 +++++++++ roadmap.yaml | 3 +- specs/105-agent-scope-hardening/tasks.md | 30 +- 17 files changed, 2654 insertions(+), 29 deletions(-) create mode 100644 .github/workflows/scope-latency.yml create mode 100644 cmd/scope-latency-compare/main.go create mode 100644 cmd/scope-latency-compare/main_test.go create mode 100644 internal/logs/nil_log_config_test.go create mode 100644 internal/server/scope_differential_test.go create mode 100644 internal/server/scope_http_matrix_test.go create mode 100644 internal/server/scope_pinned_reversal_guard_test.go create mode 100644 internal/server/scope_retained_effects_test.go create mode 100644 internal/server/testdata/scope_latency/README.md create mode 100644 internal/upstream/manager_prompts_deadline_test.go diff --git a/.github/workflows/scope-latency.yml b/.github/workflows/scope-latency.yml new file mode 100644 index 000000000..8f7312164 --- /dev/null +++ b/.github/workflows/scope-latency.yml @@ -0,0 +1,130 @@ +name: Scope Latency (Spec 105 FR-011) + +# T112a: the FR-011 merge-base regression gate. internal/server's +# scope_latency_test.go measures administrator p95 for four operations +# (retrieve_tools, read_cache, prompts/list, tools/list) on the 527-tool +# LiveMCPBench snapshot; this workflow runs that SAME test file — HEAD's +# copy, unmodified — against both the merge-base revision and HEAD, then +# fails if administrator p95 regressed by more than max(10%, 5ms) on any +# operation, or if either run produced no measurement for one. +# +# Why HEAD's test file runs against the merge-base checkout, not the +# merge-base's own copy: the test file is test-only and backward compatible +# with the pre-feature production API (it calls handleRetrieveTools, +# handleReadCache, server.HandleMessage and directServer.HandleMessage, +# none of which this feature's PRs changed the signature of), so copying it +# over the merge-base checkout compiles and runs there unchanged — this is +# the ONLY way to compare "the same measurement code" across two revisions +# whose production code differs (research D10). +# +# Non-race, single job, reference (GitHub-hosted ubuntu-latest) runner: the +# race detector's instrumentation overhead swamps the microsecond/millisecond +# deltas this measures (scope_latency_test.go's raceEnabled skip already +# documents this), and a self-hosted "reference runner" is not available to +# this repository, so ubuntu-latest is the closest available reference +# baseline, run non-race for the same reason the in-repo tests are. +# +# A generous, well-reasoned threshold (10% relative or 5ms absolute, per +# FR-011) is used deliberately over a tighter one: this epic already hit real +# CI flakiness from over-tight allocation/timing assertions on PRs F and G +# today (2026-09-20) — see CLAUDE.md's cross-model-review note on that PR's +# history. This workflow is NOT (yet) a required status check; a tight gate +# that flakes is worse than no gate, so start generous and tighten only with +# real multi-run evidence. + +on: + pull_request: + paths: + - "internal/server/**" + - "internal/index/**" + - "cmd/scope-latency-compare/**" + - ".github/workflows/scope-latency.yml" + +permissions: + contents: read + +jobs: + scope-latency: + name: FR-011 merge-base regression + runs-on: ubuntu-latest + # Advisory only: a flaky or newly-added perf gate must not block merges + # before it has a track record. Flip to false once the maintainer has + # watched it green on a handful of real PRs. + continue-on-error: true + steps: + - name: Checkout HEAD + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + path: head + fetch-depth: 0 + + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version: "1.26" + cache: true + cache-dependency-path: head/go.sum + + - name: Determine merge-base + id: mergebase + working-directory: head + run: | + git fetch origin "${{ github.event.pull_request.base.sha }}" --depth=1 + base_sha=$(git merge-base "${{ github.event.pull_request.base.sha }}" HEAD) + echo "sha=$base_sha" >> "$GITHUB_OUTPUT" + echo "Merge-base: $base_sha" + + - name: Checkout merge-base into a second directory + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + path: base + ref: ${{ steps.mergebase.outputs.sha }} + + - name: Copy HEAD's latency test + fixtures over the merge-base checkout + run: | + set -euo pipefail + cp head/internal/server/scope_latency_test.go base/internal/server/scope_latency_test.go + rm -rf base/internal/server/testdata/scope_latency + if [ -d head/internal/server/testdata/scope_latency ]; then + cp -R head/internal/server/testdata/scope_latency base/internal/server/testdata/scope_latency + fi + # scope_fixture_test.go, mcp_retrieve_scope_test.go (loadDeferredLargeCorpus, + # createTestProxyWithRuntime) and friends are shared test helpers the + # latency file calls into; the merge-base checkout already has its OWN + # copies of those (they predate this PR on every commit in the merge + # order — Phase 1's T001/T002 landed with PR A, long before H1), so + # nothing else needs copying across. A future PR that adds a NEW + # shared helper the latency file depends on must add it here too. + + - name: Run scope latency measurements (merge-base) + working-directory: base + env: + SCOPE_LATENCY_RESULTS_FILE: ${{ github.workspace }}/base-results.txt + run: | + go test ./internal/server/... \ + -run 'TestRetrieveTools_ScopeLatency_ScopedVsAdmin|TestScopeLatency_' \ + -v -timeout 10m + + - name: Run scope latency measurements (HEAD) + working-directory: head + env: + SCOPE_LATENCY_RESULTS_FILE: ${{ github.workspace }}/head-results.txt + run: | + go test ./internal/server/... \ + -run 'TestRetrieveTools_ScopeLatency_ScopedVsAdmin|TestScopeLatency_' \ + -v -timeout 10m + + - name: Compare administrator p95 across revisions + working-directory: head + run: | + go run ./cmd/scope-latency-compare "${{ github.workspace }}/base-results.txt" "${{ github.workspace }}/head-results.txt" + + - name: Upload results + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: scope-latency-results + path: | + base-results.txt + head-results.txt + retention-days: 14 diff --git a/ROADMAP.md b/ROADMAP.md index 002eff4c4..e62c9e9f7 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -242,8 +242,8 @@ graph LR classDef done fill:#1f7a1f,stroke:#0d3d0d,color:#ffffff; classDef todo fill:#6e7781,stroke:#3d4248,color:#ffffff; - class scope_fix_target_tier,scope_fix_tail_log,scope_fix_set_profile,scope_fix_read_cache,scope_fix_prompts_profile_url,scope_retrieve_tools,scope_direct_publication,scope_selectable_profile_predicate,scope_cache_legacy_invalidation,scope_log_attribution,scope_target_identity_producers,scope_fix_stored_script_admin done; - class scope_refusal_shapes,scope_regression_suite todo; + class scope_fix_target_tier,scope_fix_tail_log,scope_fix_set_profile,scope_fix_read_cache,scope_fix_prompts_profile_url,scope_retrieve_tools,scope_direct_publication,scope_refusal_shapes,scope_selectable_profile_predicate,scope_cache_legacy_invalidation,scope_log_attribution,scope_target_identity_producers,scope_fix_stored_script_admin done; + class scope_regression_suite todo; ``` | Task | Status | Refs | @@ -255,7 +255,7 @@ graph LR | FR-006 + FR-004 (deleted pin): aggregated prompts authorized by canonical registration owner; profile URL / set_profile stop enumerating on a deleted pin | 🟢 Done | #1227 | | FR-005: retrieve_tools filters by scope BEFORE limiting; indexed counts, usage ranking, debug output and session risk computed over the authorized population only | 🟢 Done | #1325 | | FR-008: direct-surface definitions take owner and tier from their own registration identity at every publication seam, both skew directions, full and deferred | 🟢 Done | #1326 | -| FR-010: scope-first refusal precedence; dispatch denials and 'available servers' never name hidden servers; describe_tool not-found and alias resolution computed over the authorized corpus | ⚪ Todo | — | +| FR-010: scope-first refusal precedence; dispatch denials and 'available servers' never name hidden servers; describe_tool not-found and alias resolution computed over the authorized corpus | 🟢 Done | #1328 | | FR-003/FR-004 remainder: selectable-profile predicate for UNPINNED tokens on /mcp/p/, /mcp/p, /mcp/p/ and set_profile; identical status+body across missing / deleted / not-selectable / pin-mismatch / no-profiles (#1225 + #1227 follow-up lists) | 🟢 Done | #1283 | | FR-002 + FR-001 remainder: legacy/unstamped and internal (registry, guesser) cache entries refused for every caller and durably invalidated; monotone recursive provenance; existence-non-disclosing refusal on MCP and REST (#1226 follow-up list) | 🟢 Done | #1282 | | FR-007 remainder: per-record canonical log ownership (a/b vs a_b share one file), filter-before-limit + authorized lines_returned, subject-bound OAuth-callback logging, canonical container ownership in Docker cleanup (#1224 follow-up list) | 🟢 Done | #1284 | @@ -888,7 +888,7 @@ graph LR | Web UI + macOS app UX audit | In progress | P0 | — | | | | Release qualification gate (auto-QA matrix blocks the tag) | In progress | P0 | — | [081-release-qa-gate](./specs/081-release-qa-gate/) | | | Action log / transparency — info at a glance | In progress | P1 | — | | | -| Agent-token scope hardening: every MCP request authorized by its own scope (spec 105) | In progress | P1 | 82/112 (73%) | [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | | +| Agent-token scope hardening: every MCP request authorized by its own scope (spec 105) | In progress | P1 | 94/113 (83%) | [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | | | Token-efficiency benchmark: measured savings, published results | In progress | P1 | 62/64 (97%) | [103-token-bench](./specs/103-token-bench/) | | | Telemetry identity & data quality (machine_id + CI-filter hardening) | In progress | P1 | — | | | | Telemetry v7: honest funnel + churn instrumentation | In progress | P1 | — | [080-telemetry-v7-churn](./specs/080-telemetry-v7-churn/) | | @@ -1034,6 +1034,6 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [102-schema-deferred](./specs/102-schema-deferred/) | `shipped` | 89/89 (100%) | | [103-token-bench](./specs/103-token-bench/) | `shipped` | 62/64 (97%) | | [104-auto-routing-mode](./specs/104-auto-routing-mode/) | — | — | -| [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | `in-flight` | 82/112 (73%) | +| [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | `in-flight` | 94/113 (83%) | | [106-security-residual-fixes](./specs/106-security-residual-fixes/) | `shipped` | 18/19 (95%) | | [107-server-edition-sso-hardening](./specs/107-server-edition-sso-hardening/) | `shipped` | 126/126 (100%) | diff --git a/cmd/scope-latency-compare/main.go b/cmd/scope-latency-compare/main.go new file mode 100644 index 000000000..10c37df25 --- /dev/null +++ b/cmd/scope-latency-compare/main.go @@ -0,0 +1,137 @@ +// Command scope-latency-compare is the T112a merge-base gate for Spec 105 +// FR-011: it compares the administrator p95 latency for each of the four +// scope-hardening operations (retrieve_tools, read_cache, prompts/list, +// tools/list) between two revisions and fails if HEAD regressed by more than +// max(10%, 5ms) on any operation, or if either side is missing a +// measurement for any operation. +// +// Input: two files, each produced by internal/server's scope_latency_test.go +// (via SCOPE_LATENCY_RESULTS_FILE) as newline-separated "operation=nanoseconds" +// pairs. No third-party dependency — stdlib only, per research D10. +// +// Usage: scope-latency-compare +package main + +import ( + "bufio" + "fmt" + "os" + "strconv" + "strings" + "time" +) + +// requiredOperations is FR-011's fixed operation list; every result file +// MUST report a measurement for every one of these, or the comparison fails +// closed (T112a: "fails if either side yields no measurement for any +// operation"). +var requiredOperations = []string{"retrieve_tools", "read_cache", "prompts/list", "tools/list"} + +const minAbsoluteBudget = 5 * time.Millisecond +const relativeBudget = 0.10 // 10% + +func main() { + if len(os.Args) != 3 { + fmt.Fprintln(os.Stderr, "usage: scope-latency-compare ") + os.Exit(2) + } + baseFile, headFile := os.Args[1], os.Args[2] + + base, err := readResults(baseFile) + if err != nil { + fmt.Fprintf(os.Stderr, "reading merge-base results %s: %v\n", baseFile, err) + os.Exit(1) + } + head, err := readResults(headFile) + if err != nil { + fmt.Fprintf(os.Stderr, "reading HEAD results %s: %v\n", headFile, err) + os.Exit(1) + } + + lines, failures := compare(base, head, baseFile, headFile) + for _, line := range lines { + fmt.Println(line) + } + if len(failures) > 0 { + fmt.Fprintln(os.Stderr, "\nscope-latency-compare: FAIL") + for _, f := range failures { + fmt.Fprintln(os.Stderr, " - "+f) + } + os.Exit(1) + } + fmt.Println("\nscope-latency-compare: PASS — every operation within FR-011's merge-base regression budget") +} + +// compare evaluates every required operation and returns (a) one +// human-readable report line per operation that had a measurement on both +// sides, and (b) the list of failure reasons — missing measurements or a +// regression exceeding max(10%, 5ms). baseFile/headFile are used only to +// name the missing side in a failure message. +func compare(base, head map[string]int64, baseFile, headFile string) (lines, failures []string) { + for _, op := range requiredOperations { + baseNS, baseOK := base[op] + headNS, headOK := head[op] + if !baseOK { + failures = append(failures, fmt.Sprintf("%s: no measurement on the merge-base side (%s)", op, baseFile)) + continue + } + if !headOK { + failures = append(failures, fmt.Sprintf("%s: no measurement on the HEAD side (%s)", op, headFile)) + continue + } + + baseDur := time.Duration(baseNS) + headDur := time.Duration(headNS) + budget := time.Duration(float64(baseDur) * relativeBudget) + if budget < minAbsoluteBudget { + budget = minAbsoluteBudget + } + regression := headDur - baseDur + status := "OK" + if regression > budget { + status = "REGRESSION" + failures = append(failures, fmt.Sprintf( + "%s: administrator p95 regressed by %s (base=%s head=%s), exceeding the max(10%%, 5ms) budget of %s", + op, regression, baseDur, headDur, budget)) + } + lines = append(lines, fmt.Sprintf("%-16s base=%-12s head=%-12s delta=%-12s budget=%-10s %s", + op, baseDur, headDur, regression, budget, status)) + } + return lines, failures +} + +// readResults parses a scope_latency_test.go results file: one +// "operation=nanoseconds" pair per line, blank lines ignored. Multiple lines +// for the same operation (a test rerun, or -count>1) keep the LAST value. +func readResults(path string) (map[string]int64, error) { + f, err := os.Open(path) + if err != nil { + return nil, err + } + defer f.Close() + + out := make(map[string]int64) + scanner := bufio.NewScanner(f) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" { + continue + } + parts := strings.SplitN(line, "=", 2) + if len(parts) != 2 { + return nil, fmt.Errorf("malformed line %q (want operation=nanoseconds)", line) + } + ns, err := strconv.ParseInt(strings.TrimSpace(parts[1]), 10, 64) + if err != nil { + return nil, fmt.Errorf("malformed duration in line %q: %w", line, err) + } + out[strings.TrimSpace(parts[0])] = ns + } + if err := scanner.Err(); err != nil { + return nil, err + } + if len(out) == 0 { + return nil, fmt.Errorf("no measurements found in %s", path) + } + return out, nil +} diff --git a/cmd/scope-latency-compare/main_test.go b/cmd/scope-latency-compare/main_test.go new file mode 100644 index 000000000..dfd2aa24e --- /dev/null +++ b/cmd/scope-latency-compare/main_test.go @@ -0,0 +1,168 @@ +package main + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func writeResultsFile(t *testing.T, dir, name string, lines map[string]int64) string { + t.Helper() + path := filepath.Join(dir, name) + f, err := os.Create(path) + require.NoError(t, err) + defer f.Close() + for op, ns := range lines { + _, err := f.WriteString(op + "=" + itoa64(ns) + "\n") + require.NoError(t, err) + } + return path +} + +func itoa64(n int64) string { + if n == 0 { + return "0" + } + neg := n < 0 + if neg { + n = -n + } + var b []byte + for n > 0 { + b = append([]byte{byte('0' + n%10)}, b...) + n /= 10 + } + if neg { + b = append([]byte{'-'}, b...) + } + return string(b) +} + +func TestReadResults_ParsesOperationNanosecondPairs(t *testing.T) { + dir := t.TempDir() + path := writeResultsFile(t, dir, "results.txt", map[string]int64{ + "retrieve_tools": 1_000_000, + "read_cache": 2_000_000, + }) + got, err := readResults(path) + require.NoError(t, err) + assert.Equal(t, int64(1_000_000), got["retrieve_tools"]) + assert.Equal(t, int64(2_000_000), got["read_cache"]) +} + +func TestReadResults_EmptyFileErrors(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "empty.txt") + require.NoError(t, os.WriteFile(path, nil, 0o644)) + _, err := readResults(path) + assert.Error(t, err, "an empty results file means the test run produced no measurements — that must fail closed, not silently no-op") +} + +func TestReadResults_MalformedLineErrors(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "bad.txt") + require.NoError(t, os.WriteFile(path, []byte("not-a-valid-line\n"), 0o644)) + _, err := readResults(path) + assert.Error(t, err) +} + +// TestCompare_WithinBudget_NoRegression is the OK case: HEAD is faster than +// base, well within the relative-or-5ms budget. +func TestCompare_WithinBudget_NoRegression(t *testing.T) { + base := map[string]int64{ + "retrieve_tools": int64(10 * time.Millisecond), + "read_cache": int64(10 * time.Millisecond), + "prompts/list": int64(1 * time.Millisecond), + "tools/list": int64(1 * time.Millisecond), + } + head := map[string]int64{ + "retrieve_tools": int64(10*time.Millisecond + 500*time.Microsecond), // +0.5ms, well under 5ms + "read_cache": int64(9 * time.Millisecond), // faster + "prompts/list": int64(1 * time.Millisecond), + "tools/list": int64(1 * time.Millisecond), + } + lines, failures := compare(base, head, "base.txt", "head.txt") + assert.Empty(t, failures, "no operation should exceed the max(10%%, 5ms) budget") + assert.Len(t, lines, 4) +} + +// TestCompare_RelativeBudgetGovernsLargeBaselines: a base of 100ms allows up +// to +10ms (10% > 5ms absolute floor) before it is a regression. +func TestCompare_RelativeBudgetGovernsLargeBaselines(t *testing.T) { + base := map[string]int64{ + "retrieve_tools": int64(100 * time.Millisecond), + "read_cache": int64(100 * time.Millisecond), + "prompts/list": int64(1 * time.Millisecond), + "tools/list": int64(1 * time.Millisecond), + } + withinBudget := map[string]int64{ + "retrieve_tools": int64(109 * time.Millisecond), // +9ms < +10ms (10%) + "read_cache": int64(100 * time.Millisecond), + "prompts/list": int64(1 * time.Millisecond), + "tools/list": int64(1 * time.Millisecond), + } + _, failures := compare(base, withinBudget, "base.txt", "head.txt") + assert.Empty(t, failures) + + overBudget := map[string]int64{ + "retrieve_tools": int64(112 * time.Millisecond), // +12ms > +10ms (10%) + "read_cache": int64(100 * time.Millisecond), + "prompts/list": int64(1 * time.Millisecond), + "tools/list": int64(1 * time.Millisecond), + } + _, failures = compare(base, overBudget, "base.txt", "head.txt") + require.Len(t, failures, 1) + assert.Contains(t, failures[0], "retrieve_tools") +} + +// TestCompare_AbsoluteFloorGovernsSmallBaselines: a base of 1ms allows only +// +5ms (the absolute floor, since 10% of 1ms is far below it) — a 5.1ms +// regression on a sub-millisecond operation must still fail. +func TestCompare_AbsoluteFloorGovernsSmallBaselines(t *testing.T) { + base := map[string]int64{ + "retrieve_tools": int64(1 * time.Millisecond), + "read_cache": int64(1 * time.Millisecond), + "prompts/list": int64(1 * time.Millisecond), + "tools/list": int64(1 * time.Millisecond), + } + head := map[string]int64{ + "retrieve_tools": int64(1 * time.Millisecond), + "read_cache": int64(1 * time.Millisecond), + "prompts/list": int64(6*time.Millisecond + 200*time.Microsecond), // +5.2ms > 5ms floor + "tools/list": int64(1 * time.Millisecond), + } + _, failures := compare(base, head, "base.txt", "head.txt") + require.Len(t, failures, 1) + assert.Contains(t, failures[0], "prompts/list") +} + +// TestCompare_MissingMeasurementFailsClosed: FR-011's "fails if either side +// yields no measurement for any operation" — a required operation absent +// from either file is a failure, never a skip. +func TestCompare_MissingMeasurementFailsClosed(t *testing.T) { + full := map[string]int64{ + "retrieve_tools": int64(1 * time.Millisecond), + "read_cache": int64(1 * time.Millisecond), + "prompts/list": int64(1 * time.Millisecond), + "tools/list": int64(1 * time.Millisecond), + } + missingOnHead := map[string]int64{ + "retrieve_tools": int64(1 * time.Millisecond), + "read_cache": int64(1 * time.Millisecond), + "prompts/list": int64(1 * time.Millisecond), + // tools/list intentionally absent. + } + _, failures := compare(full, missingOnHead, "base.txt", "head.txt") + require.Len(t, failures, 1) + assert.Contains(t, failures[0], "tools/list") + assert.Contains(t, failures[0], "HEAD side") + + _, failures = compare(missingOnHead, full, "base.txt", "head.txt") + require.Len(t, failures, 1) + assert.Contains(t, failures[0], "tools/list") + assert.Contains(t, failures[0], "merge-base side") +} diff --git a/docs/features/agent-tokens.md b/docs/features/agent-tokens.md index d68f8bf65..3e6d44ffb 100644 --- a/docs/features/agent-tokens.md +++ b/docs/features/agent-tokens.md @@ -357,14 +357,26 @@ stdio) keep every capability they have today; the exceptions where an administrator's answer deliberately differs from a token's are named and tested one by one. -> **Rollout status.** This invariant is being landed surface by surface as the -> agent-scope hardening series (Spec 105) merges; each release's notes list the -> surfaces it closes. The rules on this page that are stated as present-tense -> guarantees — the stored-script rules below, the REST doors listed above and -> the `read_cache` rule — are enforced by the version that documents them. Until -> the series is complete, a listing or suggestion on a surface not yet covered -> can still name an out-of-scope resource; treat that as a known gap, not a -> configuration mistake. +> **Rollout status.** The agent-scope hardening series (Spec 105, nine PRs — +> `A` exact target-tier identity, `B` cache legacy/internal-entry refusal, +> `D` selectable-profile predicate, `E` per-record log attribution, `H0` +> stored-script enumeration, `C` scoped `retrieve_tools`, `F` direct-surface +> publication identity, `G` scope-first refusal shapes, and `H1` — the +> regression suite this page's guarantees are proven against, covering +> User Stories 1–3 by id) has shipped its code for every functional +> requirement (FR-001 through FR-014) across those PRs, and every acceptance +> scenario in User Stories 1–3 now has a proving test registered by id +> (`TestScopeCoverage_EveryUserStoryScenario`, +> `internal/server/scope_differential_test.go`). Most of those tests re-run +> the dedicated, per-FR differential fixture each PR shipped for its own +> gaps (e.g. `mcp_retrieve_scope_test.go` for FR-005, `mcp_direct_skew_test.go` +> for FR-008); a smaller number run directly against the general two-fixture +> `{a, b, a__b}` harness this file introduces. The rules on this page are +> stated as present-tense guarantees on that basis, not because every one is +> proven through the general three-server harness specifically. A hidden +> server can still influence what an authorized caller experiences only +> through the **retained, documented effects** named below — never by +> being named, listed or dispatched to. > **Who counts as an administrator.** The admin API key, the tray over the > local socket, native stdio, an in-process caller — and, under the default diff --git a/internal/logs/attribution.go b/internal/logs/attribution.go index 8a030c025..b7259347c 100644 --- a/internal/logs/attribution.go +++ b/internal/logs/attribution.go @@ -189,8 +189,17 @@ func ReadUpstreamServerLogTailAttributed(config *config.LogConfig, serverName st lines = 500 } + // A nil config (a caller that built config.Config{} directly rather than + // through config.DefaultConfig(), which always sets Logging) must fall + // through to GetLogFilePathWithDir's own empty-LogDir OS-default + // behavior, never dereference a nil pointer — same fix as the whole-file + // reader (logger.go). + logDir := "" + if config != nil { + logDir = config.LogDir + } filename := serverLogFilename(serverName) - logFilePath, err := GetLogFilePathWithDir(config.LogDir, filename) + logFilePath, err := GetLogFilePathWithDir(logDir, filename) if err != nil { return nil, fmt.Errorf("failed to get log file path for server %s: %w", serverName, err) } diff --git a/internal/logs/logger.go b/internal/logs/logger.go index 925938f08..6c5946c6e 100644 --- a/internal/logs/logger.go +++ b/internal/logs/logger.go @@ -509,9 +509,16 @@ func ReadUpstreamServerLogTail(config *config.LogConfig, serverName string, line lines = 500 } - // Get log file path + // Get log file path. A nil config (a caller that built config.Config{} + // directly rather than through config.DefaultConfig(), which always sets + // Logging) must fall through to GetLogFilePathWithDir's own empty-LogDir + // OS-default behavior, never dereference a nil pointer. + logDir := "" + if config != nil { + logDir = config.LogDir + } filename := serverLogFilename(serverName) - logFilePath, err := GetLogFilePathWithDir(config.LogDir, filename) + logFilePath, err := GetLogFilePathWithDir(logDir, filename) if err != nil { return nil, fmt.Errorf("failed to get log file path for server %s: %w", serverName, err) } diff --git a/internal/logs/nil_log_config_test.go b/internal/logs/nil_log_config_test.go new file mode 100644 index 000000000..f7f29858b --- /dev/null +++ b/internal/logs/nil_log_config_test.go @@ -0,0 +1,41 @@ +package logs + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestReadUpstreamServerLogTail_NilConfigDoesNotPanic and its attributed +// sibling below: a *config.LogConfig of nil (a minimal test/library caller +// that constructs config.Config{} directly rather than through +// config.DefaultConfig(), which always sets Logging) must not crash the +// reader — GetLogFilePathWithDir already falls back to the OS default log +// directory for an EMPTY LogDir string (paths.go:109-112); the bug was +// dereferencing config.LogDir on a nil config before ever reaching that +// fallback. Found by internal/server's scope_http_matrix_test.go (Spec 105 +// PR H1, T111), which drives handleTailLog over real HTTP against +// profileTestEnv's config (built as a struct literal — e2e_test.go:34 — +// leaving Logging nil): the panic surfaced as mcp-go's recovered +// "-32603 panic recovered in upstream_servers tool handler", not a full +// server crash, but it is still a genuine defect — a legitimate tail_log +// call for EITHER caller kind (this affects the whole-file admin reader +// identically, so it is a robustness gap, not a scope/security issue) fails +// with a confusing internal error instead of the sensible empty/OS-default +// behavior every other empty-LogDir caller gets. +func TestReadUpstreamServerLogTail_NilConfigDoesNotPanic(t *testing.T) { + assert.NotPanics(t, func() { + _, err := ReadUpstreamServerLogTail(nil, "some-server", 10) + // No log file exists at the OS-default location for this made-up + // server name, so an empty result (not an error) is expected — + // mirroring the existing empty-LogDir/missing-file behavior. + assert.NoError(t, err) + }) +} + +func TestReadUpstreamServerLogTailAttributed_NilConfigDoesNotPanic(t *testing.T) { + assert.NotPanics(t, func() { + _, err := ReadUpstreamServerLogTailAttributed(nil, "some-server", 10) + assert.NoError(t, err) + }) +} diff --git a/internal/server/scope_differential_test.go b/internal/server/scope_differential_test.go new file mode 100644 index 000000000..75d06b5cb --- /dev/null +++ b/internal/server/scope_differential_test.go @@ -0,0 +1,496 @@ +package server + +import ( + "context" + "encoding/json" + "regexp" + "sort" + "strconv" + "sync" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" +) + +// Spec 105 PR H1 — scope-regression-suite part 2 (FR-011/013/014, SC-007, +// gaps FR01x-G4..G7). This file is the two-fixture differential oracle +// (contracts/differential-oracle.md) plus the SC-007 coverage registry that +// ties every spec.md acceptance scenario to the test(s) that prove it. +// +// Design note (plan.md §4, "Two-fixture differential oracle"): PRs A-G each +// shipped standalone, rigorous tests against the Phase-1 fixtures for their +// own FR (scope_cache_fixtures_test.go, mcp_retrieve_scope_test.go, +// scope_target_tier_matrix_test.go, mcp_direct_publication_identity_test.go, +// mcp_direct_skew_test.go, mcp_direct_underscore_test.go, +// mcp_tail_log_scope_test.go, mcp_prompt_scope_test.go, profile_tool_test.go, +// profile_integration_test.go, mcp_auth_scope_test.go, ...). "H1 re-registers +// them by User Story id" (plan.md Delivery Structure, PR H1 row) — that is +// what reuseScopeCoverage below does: it runs the SAME already-verified test +// again as a named subtest of TestScopeCoverage_EveryUserStoryScenario, so a +// regression in any of them also fails the coverage gate, and records the +// US id -> test name mapping the inventory table below documents statically. +// newScopeFixture/runScopeScenario is the general two-fixture harness the +// contract also asks for; it is used directly for the scenarios that did not +// already have a dedicated differential fixture (US1.5's full oracle +// composition, US1.8's error-text identity, and the coverage self-test). +// +// --------------------------------------------------------------------------- +// SC-007 inventory table: US id -> proving test(s) -> `-run` pattern +// --------------------------------------------------------------------------- +// +// US1.1 cached response redemption under broader auth (FR-001) +// TestScopeCacheFixture_PinnedTokenRESTDispatchAndRedemptionParity +// TestScopeCacheFixture_ProfiledAdminChildNotRedeemableByPinnedAgent +// TestScopeCacheFixture_RecursiveChildOnREST +// TestScopeCacheFixture_EmptyGrantAgentIsDenyAllOnRedemption +// TestScopeCacheFixture_HeldCallKeepsDispatchTimeSnapshot +// TestAuthorization_CallerKindFirst +// -run TestScopeCoverage_EveryUserStoryScenario/US1\.1 +// US1.2 legacy/internal cache entries refused + durably invalidated (FR-002) +// TestScopeCacheFixture_UpgradeRecordRefusedAndAbsentAfterRestart +// TestScopeCacheFixture_FreshInternalEntryRefusedForEveryCaller +// -run TestScopeCoverage_EveryUserStoryScenario/US1\.2 +// US1.3 set_profile effective intersection / non-selectable ≡ nonexistent (FR-003) +// TestHandleSetProfile_ScopedTokenSelectIntersectsAllowedServers +// TestHandleSetProfile_ScopedTokenDisjointProfileIndistinguishableFromUnknown +// TestHandleSetProfile_ScopedTokenDisjointProfilePresentVsAbsentIdentical +// TestHandleSetProfile_ScopedTokenUnknownSlugDoesNotEnumerateAllProfiles +// TestHandleSetProfile_PinnedZeroReachRefusedLikeDeletedPin +// -run TestScopeCoverage_EveryUserStoryScenario/US1\.3 +// US1.4 profile URL deleted/missing/non-selectable pin, /mcp/p, /mcp/p/ (FR-004) +// TestProfile_PinnedTokenURLEnforcement +// TestProfile_DeletedPinDoesNotEnumerateProfiles +// TestProfile_ScopedUnpinnedRefusalUniform +// TestProfile_PinnedRefusalUniform +// TestProfile_PinnedRefusalIndependentOfFleet +// TestProfile_PinnedZeroReachURLRefusedUniformly +// -run TestScopeCoverage_EveryUserStoryScenario/US1\.4 +// US1.5 retrieve_tools oracle: stats+debug+session_risk (FR-005) +// TestScopeDifferential_RetrieveToolsFullOracle (this file) +// TestRetrieveTools_ScopeOracle (mcp_retrieve_scope_test.go) +// -run TestScopeCoverage_EveryUserStoryScenario/US1\.5 +// US1.6 aggregated prompts authorized by registration identity (FR-006) +// TestAggregatedPrompt_ScopeUsesCanonicalOwner +// TestUnstampedPrompt_WithheldFromListAndGet_ForAdminAndAgent +// TestAggregatedPrompt_LateEnableStillFiltered +// -run TestScopeCoverage_EveryUserStoryScenario/US1\.6 +// US1.7 tail_log / per-server ops, a/b vs a_b, own-server attribution (FR-007) +// TestTailLog_CollidingLogFile_ScopedTokenGetsOnlyOwnRecords +// TestTailLog_CollidingLogFile_DifferentialWithHiddenCoOwner +// internal/logs.TestReadUpstreamServerLogTail_AttributedOnly_* (separate package; not re-invoked here) +// -run TestScopeCoverage_EveryUserStoryScenario/US1\.7 +// US1.8 dispatch-denial / available-servers naming (FR-010 G1/G7) +// TestScopeDifferential_NoLiveClientAvailableServers (this file) +// TestHandleCallToolVariant_NoLiveClient_AvailableServersFilteredByScope +// TestHandleCallToolVariant_PinWiderThanToken_IndistinguishableFromNonexistent +// -run TestScopeCoverage_EveryUserStoryScenario/US1\.8 +// US2.1-US2.3 {read} token above tier / variant mismatch (FR-009 retrieve table) +// TestScopeTargetTier_RetrieveTable, TestScopeTargetTier_RetrieveTable_ReadDestructive +// -run TestScopeCoverage_EveryUserStoryScenario/US2\.1-3 +// US2.4 paired names erase (approved) vs ns:erase (denied/unapproved) +// TestScopeTargetTier_PairedNames_Retrieve, _Direct, _Nested +// -run TestScopeCoverage_EveryUserStoryScenario/US2\.4 +// US2.5 unresolved registration identity refused for every caller (D4) +// TestScopeTargetTier_UnresolvedIdentity +// -run TestScopeCoverage_EveryUserStoryScenario/US2\.5 +// US2.6 nested script call above tier, refusal envelope survives script success +// TestScopeTargetTier_NestedTable +// -run TestScopeCoverage_EveryUserStoryScenario/US2\.6 +// US3.1 origin-flip: every seam's listing carries its producing identity +// TestDirectPublication_OriginFlip_DefinitionMatchesProducingIdentity +// TestSkew_OriginFlipNeverSplitsScopeFromDispatch +// -run TestScopeCoverage_EveryUserStoryScenario/US3\.1 +// US3.2 unparseable names / built-ins positively identified +// TestDirectPublication_SeamAddition_StructurallyAwkwardNamesNeverListedOutOfScope +// TestDirectUnderscoreServer_SteadyState, TestDirectUnderscoreServer_SeamVariant +// TestSkew_AddedNameBeforeItsCatalogEntry +// -run TestScopeCoverage_EveryUserStoryScenario/US3\.2 +// US3.3 same-owner tier change read->destructive withheld once registered +// TestDirectPublication_TierChangeWithheldDuringSeam_DeferredMode +// TestSkew_ReadScopedTokenNeverHasADestructiveCallAdmitted +// -run TestScopeCoverage_EveryUserStoryScenario/US3\.3 +// US3.4 dispatch follows the handler registered at each seam; refusal replaced +// TestDirectPublication_InSeamCallEnvelopeMatchesUnregisteredName +// -run TestScopeCoverage_EveryUserStoryScenario/US3\.4 +// +// Retained-effect scenarios (SC-001 exclusions, spec.md:114,161) are in +// scope_retained_effects_test.go, run via runRetainedEffectScenario. The +// grep guard proving none of the four pinned-reversal assertions FR01x-G7 +// named (gap-map.md §7, §3) survived in their original form is +// TestScopePinnedReversalsStayInverted, below. + +// --------------------------------------------------------------------------- +// Generic two-fixture harness +// --------------------------------------------------------------------------- + +// scopeFixture is the {a, b, a__b} (full=true) or {a}-only (full=false) +// environment every registered scenario compares. Server "a" is the +// authorized content every scenario's caller may see; "b" and "a__b" (full +// fixture only) carry a sentinel string in their tool name and description +// that must never reach the a-only token's response. +type scopeFixture struct { + proxy *MCPProxyServer + rt *runtime.Runtime + sentinels []string + full bool +} + +// newScopeFixture builds the fixture (contracts/differential-oracle.md). +// full=true adds hidden servers "b" and "a__b", each with one tool carrying +// a distinctive sentinel in its name and description; full=false is server +// "a" alone, letter-for-letter identical to the full fixture's "a". +func newScopeFixture(t *testing.T, full bool) *scopeFixture { + t.Helper() + servers := []*config.ServerConfig{{Name: "a", Enabled: true}} + if full { + servers = append(servers, + &config.ServerConfig{Name: "b", Enabled: true}, + &config.ServerConfig{Name: "a__b", Enabled: true}, + ) + } + proxy, rt := createTestProxyWithRuntime(t, servers) + f := &scopeFixture{proxy: proxy, rt: rt, full: full} + + startCountingUpstream(t, proxy, rt, "a", + readSpec("read_thing"), writeSpec("write_thing"), destructiveSpec("destroy_thing"), + readSpec("erase"), writeSpec("ns:erase")) + + if full { + sentB := "SENTINEL_scopeB_6f1c9a" + sentAB := "SENTINEL_scopeAB_2d9e41" + f.sentinels = []string{sentB, sentAB} + startCountingUpstream(t, proxy, rt, "b", + toolSpec{Name: sentB + "_tool", Description: "Handles " + sentB, + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}) + startCountingUpstream(t, proxy, rt, "a__b", + toolSpec{Name: sentAB + "_tool", Description: "Handles " + sentAB, + Annotations: &config.ToolAnnotations{ReadOnlyHint: boolPtr(true)}}) + // Usage history on hidden tools (cross-model review round 1): US1.5's + // retrieve oracle claim is meaningless if nothing in the fixture + // actually differs on the fleet-wide path a leak would take — + // usage_summary's top_tools is fleet-wide by construction + // (mcp_retrieve_scope_test.go FR005-G2), so seeding real usage on a + // hidden tool gives TestScopeDifferential_RetrieveToolsFullOracle an + // actual discriminator: if the scope filter on usage ranking ever + // regressed, "b"'s sentinel tool would outrank "a"'s and this + // fixture's normalized(narrow) != normalized(full) would catch it. + require.NoError(t, proxy.storage.IncrementToolUsage("b:"+sentB+"_tool")) + require.NoError(t, proxy.storage.IncrementToolUsage("b:"+sentB+"_tool")) + require.NoError(t, proxy.storage.IncrementToolUsage("b:"+sentB+"_tool")) + } + return f +} + +// scopeCoverage records every US id proven by this test binary run, guarded +// by scopeCoverageMu since subtests may run in parallel. +var ( + scopeCoverageMu sync.Mutex + scopeCoverage = map[string]string{} // usID -> the *testing.T name that proved it +) + +func registerScopeCoverage(t *testing.T, usID string) { + t.Helper() + scopeCoverageMu.Lock() + defer scopeCoverageMu.Unlock() + scopeCoverage[usID] = t.Name() +} + +// scopeNormalizers strip only NONDETERMINISTIC fields (contracts doc: +// "strips only nondeterministic fields ... sorts unordered lists. Seeded +// usage counts are deterministic and are compared."): ISO-8601-ish +// timestamps, long hex ids/keys (cache keys, request ids), and durations. +var scopeNormalizers = []*regexp.Regexp{ + regexp.MustCompile(`"(?:timestamp|created_at|last_accessed|expires_at|started_at|completed_at|connected_at|last_seen)":"[^"]*"`), + regexp.MustCompile(`\b[0-9a-fA-F]{32,64}\b`), + regexp.MustCompile(`"request_id"\s*:\s*"[^"]*"`), + regexp.MustCompile(`\b\d+(\.\d+)?(ms|µs|ns)\b`), +} + +func normalizeScopeResponse(s string) string { + for _, re := range scopeNormalizers { + s = re.ReplaceAllString(s, "NORM") + } + return s +} + +// runScopeScenario runs fn against the full and narrow fixtures with the SAME +// a-only token, asserts the normalised outputs are identical and that no +// sentinel leaked into the full fixture's raw response, and registers usID +// as covered. +func runScopeScenario(t *testing.T, usID string, fn func(t *testing.T, f *scopeFixture, ctx context.Context) string) { + t.Helper() + t.Run(usID, func(t *testing.T) { + registerScopeCoverage(t, usID) + full := newScopeFixture(t, true) + narrow := newScopeFixture(t, false) + mkCtx := func() context.Context { return agentCtx([]string{"a"}, allPerms, "") } + + outFull := fn(t, full, mkCtx()) + outNarrow := fn(t, narrow, mkCtx()) + + require.Equal(t, normalizeScopeResponse(outNarrow), normalizeScopeResponse(outFull), + "US %s: the a-only token's response must be identical between the {a,b,a__b} and {a}-only fixtures after normalisation", usID) + for _, s := range full.sentinels { + require.NotContains(t, outFull, s, "US %s: response must not contain sentinel %q", usID, s) + } + }) +} + +// reuseScopeCoverage registers usID as proven by an ALREADY-EXISTING, +// dedicated test elsewhere in this package (see the inventory table above) +// and re-runs it as a subtest, so a regression in that test also fails +// TestScopeCoverage_EveryUserStoryScenario. This is the re-registration +// plan.md's PR H1 row describes: "PRs A-G ship standalone tests on the +// Phase-1 fixtures; H1 re-registers them by User Story id." +func reuseScopeCoverage(t *testing.T, usID string, existing ...func(t *testing.T)) { + t.Helper() + reuseScopeCoverageMulti(t, []string{usID}, existing...) +} + +// reuseScopeCoverageMulti is reuseScopeCoverage for the case where several US +// ids are proven by the SAME underlying test(s) (e.g. US2.1-US2.3 are all +// cells of the one generated 54-cell retrieve table) — the test runs once, +// every listed id is registered as covered by it. +func reuseScopeCoverageMulti(t *testing.T, usIDs []string, existing ...func(t *testing.T)) { + t.Helper() + t.Run(joinIDs(usIDs), func(t *testing.T) { + for _, id := range usIDs { + registerScopeCoverage(t, id) + } + for _, fn := range existing { + fn(t) + } + }) +} + +func joinIDs(ids []string) string { + out := ids[0] + for _, id := range ids[1:] { + out += "_" + id + } + return out +} + +// --------------------------------------------------------------------------- +// US1.5 — retrieve_tools oracle (include_stats + debug + session_risk) +// --------------------------------------------------------------------------- + +// TestScopeDifferential_RetrieveToolsFullOracle is the US1.5 acceptance +// scenario run through the general two-fixture harness: an a-only token +// calling retrieve_tools with include_stats, debug and session_risk all on +// must see byte-identical scope-independent fields (usage summary shape, +// indexed counts, session risk) whether or not b/a__b exist, and no +// sentinel. The ranking-dependent fields (tools, total, filter_diagnostics) +// are EXCLUDED here by construction: "a" carries exactly the same content in +// both fixtures and the query term is chosen to match only "a" tools, so the +// ranked window itself is expected to already be identical — this is the +// SC-001 "not displaced" case, not a substitute for the per-fixture oracle +// derivation TestRetrieveTools_ScopeOracle (mcp_retrieve_scope_test.go) +// performs with a genuinely displacing hidden tool. +func TestScopeDifferential_RetrieveToolsFullOracle(t *testing.T) { + runScopeScenario(t, "US1.5", func(t *testing.T, f *scopeFixture, ctx context.Context) string { + // Seed matching usage on "a"'s own tool so usage_summary.top_tools + // has real, scope-independent content to compare (not just an empty + // list both fixtures trivially share) — the hidden discriminator is + // newScopeFixture's usage on "b"'s sentinel tool, which must never + // surface here regardless of relative counts (mcp_retrieve_scope_test.go's + // TestRetrieveTools_ScopeOracle is the dedicated FR005-G1..G5 oracle + // derivation test for the ranking-displacement case; this one proves + // the SAME invariant through the general two-fixture harness with a + // non-ranking discriminator: usage_summary and session_risk). + require.NoError(t, f.proxy.storage.IncrementToolUsage("a:read_thing")) + + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{ + "query": "read_thing", "limit": float64(10), + "include_stats": true, "debug": true, + } + result, err := f.proxy.handleRetrieveTools(ctx, req) + require.NoError(t, err) + require.False(t, result.IsError, "retrieve_tools must not error for the a-only token: %v", result.Content) + return stripRankingDependentFields(t, resultText(t, result)) + }) +} + +// stripRankingDependentFields removes the fields SC-001/the retrieve oracle +// (spec.md:114, contracts/differential-oracle.md) explicitly EXCLUDES from +// cross-fixture equality — the ranked window itself (`tools`, `total`), +// `filter_diagnostics`, and any per-tool `score` — before a scenario +// compares the rest byte-for-byte. Those fields are proven correct +// separately, per fixture, against the retrieve oracle's own derivation +// (TestRetrieveTools_ScopeOracle, mcp_retrieve_scope_test.go); comparing +// them here across fixtures would be WRONG per the contract even when this +// fixture's narrow query happens not to create a ranking difference — a +// hidden document is allowed to change a legitimate BM25 score. +func stripRankingDependentFields(t *testing.T, raw string) string { + t.Helper() + var payload map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(raw), &payload)) + delete(payload, "tools") + delete(payload, "total") + delete(payload, "filter_diagnostics") + out, err := json.Marshal(payload) + require.NoError(t, err) + return string(out) +} + +// --------------------------------------------------------------------------- +// US1.8 — dispatch denial / available-servers naming +// --------------------------------------------------------------------------- + +// TestScopeDifferential_NoLiveClientAvailableServers is the US1.8 scenario: +// when the target server has no live client, the error text an a-only token +// receives must be identical whether or not hidden servers b/a__b exist in +// the fleet, and must never name them. +func TestScopeDifferential_NoLiveClientAvailableServers(t *testing.T) { + runScopeScenario(t, "US1.8", func(t *testing.T, f *scopeFixture, ctx context.Context) string { + // Disconnect "a"'s live client so the no-live-client branch fires, + // without removing it from storage/StateView (the branch's + // precondition: registered, no client). + f.proxy.upstreamManager.RemoveServer("a") + + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"name": "a:read_thing", "args": map[string]interface{}{}} + result, err := f.proxy.handleCallToolVariant(ctx, req, contracts.ToolVariantRead) + require.NoError(t, err) + require.True(t, result.IsError, "no live client must be refused") + return resultText(t, result) + }) +} + +// --------------------------------------------------------------------------- +// Coverage registry +// --------------------------------------------------------------------------- + +// requiredScopeScenarios is every acceptance-scenario id spec.md names for +// User Stories 1-3 (US1.1-1.8, US2.1-2.6, US3.1-3.4). +func requiredScopeScenarios() []string { + var ids []string + for i := 1; i <= 8; i++ { + ids = append(ids, "US1."+strconv.Itoa(i)) + } + for i := 1; i <= 6; i++ { + ids = append(ids, "US2."+strconv.Itoa(i)) + } + for i := 1; i <= 4; i++ { + ids = append(ids, "US3."+strconv.Itoa(i)) + } + return ids +} + +// TestScopeCoverage_EveryUserStoryScenario is H1's independent test +// (tasks.md Phase 10): it passes only when US1.1-1.8, US2.1-2.6, US3.1-3.4 +// are all registered — each as a subtest that either runs the general +// two-fixture harness directly or re-runs the dedicated per-PR differential +// test(s) the inventory table above names. A coverage gap (a US id nothing +// registers) or a regression in any re-run test fails this test. +func TestScopeCoverage_EveryUserStoryScenario(t *testing.T) { + // Reset the package-global registry first (cross-model review round 1): + // this test must prove coverage from its OWN registrations, never rely on + // another top-level test (e.g. TestScopeDifferential_RetrieveToolsFullOracle, + // run earlier in source order) having already populated an entry this + // test never registers itself. `go test -run TestScopeCoverage_EveryUserStoryScenario` + // alone always started from an empty map anyway (a fresh process); this + // guards the full-package run too. + scopeCoverageMu.Lock() + scopeCoverage = map[string]string{} + scopeCoverageMu.Unlock() + + // US1 — a server-restricted token cannot learn about other servers. + reuseScopeCoverage(t, "US1.1", + TestScopeCacheFixture_PinnedTokenRESTDispatchAndRedemptionParity, + TestScopeCacheFixture_ProfiledAdminChildNotRedeemableByPinnedAgent, + TestScopeCacheFixture_RecursiveChildOnREST, + TestScopeCacheFixture_EmptyGrantAgentIsDenyAllOnRedemption, + TestScopeCacheFixture_HeldCallKeepsDispatchTimeSnapshot, + // internal/cache.TestAuthorization_CallerKindFirst also proves this + // scenario's kind-first rule, but lives in a different Go package + // (internal/cache) and so cannot be re-invoked from here; it is + // exercised by its own package's `go test ./internal/cache/...` run. + ) + reuseScopeCoverage(t, "US1.2", + TestScopeCacheFixture_UpgradeRecordRefusedAndAbsentAfterRestart, + TestScopeCacheFixture_FreshInternalEntryRefusedForEveryCaller, + ) + reuseScopeCoverage(t, "US1.3", + TestHandleSetProfile_ScopedTokenSelectIntersectsAllowedServers, + TestHandleSetProfile_ScopedTokenDisjointProfileIndistinguishableFromUnknown, + TestHandleSetProfile_ScopedTokenDisjointProfilePresentVsAbsentIdentical, + TestHandleSetProfile_ScopedTokenUnknownSlugDoesNotEnumerateAllProfiles, + TestHandleSetProfile_PinnedZeroReachRefusedLikeDeletedPin, + ) + reuseScopeCoverage(t, "US1.4", + TestProfile_PinnedTokenURLEnforcement, + TestProfile_DeletedPinDoesNotEnumerateProfiles, + TestProfile_ScopedUnpinnedRefusalUniform, + TestProfile_PinnedRefusalUniform, + TestProfile_PinnedRefusalIndependentOfFleet, + TestProfile_PinnedZeroReachURLRefusedUniformly, + ) + reuseScopeCoverage(t, "US1.5", + TestScopeDifferential_RetrieveToolsFullOracle, + TestRetrieveTools_ScopeOracle, + ) + reuseScopeCoverage(t, "US1.6", + TestAggregatedPrompt_ScopeUsesCanonicalOwner, + TestUnstampedPrompt_WithheldFromListAndGet_ForAdminAndAgent, + TestAggregatedPrompt_LateEnableStillFiltered, + ) + reuseScopeCoverage(t, "US1.7", + TestTailLog_CollidingLogFile_ScopedTokenGetsOnlyOwnRecords, + TestTailLog_CollidingLogFile_DifferentialWithHiddenCoOwner, + ) + reuseScopeCoverage(t, "US1.8", + TestScopeDifferential_NoLiveClientAvailableServers, + TestHandleCallToolVariant_NoLiveClient_AvailableServersFilteredByScope, + TestHandleCallToolVariant_PinWiderThanToken_IndistinguishableFromNonexistent, + ) + + // US2 — a read-only token cannot execute above its tier. US2.1-2.3 are + // all cells of the one generated 54-cell retrieve table (T009); it runs + // once and proves all three. + reuseScopeCoverageMulti(t, []string{"US2.1", "US2.2", "US2.3"}, TestScopeTargetTier_RetrieveTable) + reuseScopeCoverage(t, "US2.4", + TestScopeTargetTier_PairedNames_Retrieve, + TestScopeTargetTier_PairedNames_Direct, + TestScopeTargetTier_PairedNames_Nested, + ) + reuseScopeCoverage(t, "US2.5", TestScopeTargetTier_UnresolvedIdentity) + reuseScopeCoverage(t, "US2.6", TestScopeTargetTier_NestedTable) + + // US3 — a listing never returns a definition its own publication would + // not authorize. + reuseScopeCoverage(t, "US3.1", + TestDirectPublication_OriginFlip_DefinitionMatchesProducingIdentity, + TestSkew_OriginFlipNeverSplitsScopeFromDispatch, + ) + reuseScopeCoverage(t, "US3.2", + TestDirectPublication_SeamAddition_StructurallyAwkwardNamesNeverListedOutOfScope, + TestDirectUnderscoreServer_SteadyState, + TestDirectUnderscoreServer_SeamVariant, + TestSkew_AddedNameBeforeItsCatalogEntry, + ) + reuseScopeCoverage(t, "US3.3", + TestDirectPublication_TierChangeWithheldDuringSeam_DeferredMode, + TestSkew_ReadScopedTokenNeverHasADestructiveCallAdmitted, + ) + reuseScopeCoverage(t, "US3.4", + TestDirectPublication_InSeamCallEnvelopeMatchesUnregisteredName, + ) + + scopeCoverageMu.Lock() + missing := make([]string, 0) + for _, id := range requiredScopeScenarios() { + if _, ok := scopeCoverage[id]; !ok { + missing = append(missing, id) + } + } + scopeCoverageMu.Unlock() + sort.Strings(missing) + require.Empty(t, missing, "coverage gap: the following User Story scenarios have no registered proving test: %v", missing) +} diff --git a/internal/server/scope_http_matrix_test.go b/internal/server/scope_http_matrix_test.go new file mode 100644 index 000000000..a300ce07d --- /dev/null +++ b/internal/server/scope_http_matrix_test.go @@ -0,0 +1,418 @@ +package server + +import ( + "bytes" + "encoding/json" + "fmt" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Spec 105 PR H1, T111: the HTTP credential matrix (FR-014, +// contracts/differential-oracle.md "HTTP matrix"). Real agent tokens, minted +// through profile_integration_test.go's mintProfileAgentToken (T035 — +// generalised from mintPinnedToken; H1 reuses it, as plan.md's PR D row +// says it would), driving every fixed MCP surface over real loopback HTTP — +// this is the one test in the whole Spec 105 suite that goes through +// mcpAuthMiddleware end to end rather than an injected auth.AuthContext. +// +// FR-014's applicability matrix: +// +// operation | retrieve (/mcp,/mcp/call) | direct (/mcp/all) | code (/mcp/code) +// read_cache | yes | n/a | n/a +// set_profile | yes | n/a | yes +// retrieve_tools metadata | yes | n/a | yes (no detail) +// prompts list/get | yes | yes | yes +// tail_log / per-server ops | yes | n/a | yes +// describe_tool | yes | yes | n/a +// stored-script resolution | yes (code exec enabled) | n/a | yes +// execution tier call_tool_* direct-name dispatch nested call_tool +// publication filtering | n/a | yes | n/a +// +// `yes` cells are driven through the real HTTP surface with a scoped token +// and assert the same non-disclosure the in-process differential tests +// prove (no `E2E` in this file's test names — the CI skip regex would drop +// it, per contracts doc). `n/a` cells assert the tool name is simply +// unregistered on that surface (`-32602`), which is registration-level +// topology, not a scope decision, and is the SAME for every caller. + +// httpMatrixEnv is a minimal real HTTP environment: two upstream servers, +// "allowed" (the scoped token's own) and "hidden" (outside its grant), each +// with one tool carrying a sentinel, wired through a real *Server with a +// live TCP listener. +type httpMatrixEnv struct { + *profileTestEnv + token string // scoped to "research-srv" only (profileTestEnv's own naming) + sessionIDs map[string]string // per-PATH Mcp-Session-Id: each routing-mode endpoint is its own mcpserver.MCPServer with its own session namespace, so a session minted on /mcp is meaningless on /mcp/all +} + +func newHTTPMatrixEnv(t *testing.T) *httpMatrixEnv { + t.Helper() + env := newProfileTestEnv(t) + token := mintProfileAgentToken(t, env, "http-matrix-agent", + []string{"research-srv"}, []string{"read"}, "") + return &httpMatrixEnv{profileTestEnv: env, token: token, sessionIDs: map[string]string{}} +} + +// rpc POSTs one JSON-RPC request to path (relative to the base URL, e.g. +// "/mcp/all") with the scoped token's bearer credential and returns the +// decoded envelope plus the HTTP status code. +func (e *httpMatrixEnv) rpc(t *testing.T, path, method string, params map[string]interface{}) (map[string]interface{}, int) { + t.Helper() + body := map[string]interface{}{"jsonrpc": "2.0", "id": 1, "method": method} + if params != nil { + body["params"] = params + } + data, err := json.Marshal(body) + require.NoError(t, err) + + req, err := http.NewRequest(http.MethodPost, e.baseURL+path, bytes.NewReader(data)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + req.Header.Set("Authorization", "Bearer "+e.token) + if sid := e.sessionIDs[path]; sid != "" { + req.Header.Set("Mcp-Session-Id", sid) + } + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + if sid := resp.Header.Get("Mcp-Session-Id"); sid != "" { + e.sessionIDs[path] = sid + } + + var envelope map[string]interface{} + if resp.StatusCode == http.StatusOK { + require.NoError(t, json.NewDecoder(resp.Body).Decode(&envelope)) + } + return envelope, resp.StatusCode +} + +// initializeOn sends the initialize handshake on path with the scoped +// token, required before most surfaces accept a second call on the same +// connection-less HTTP JSON-RPC transport used here. +func (e *httpMatrixEnv) initializeOn(t *testing.T, path string) { + t.Helper() + _, status := e.rpc(t, path, "initialize", map[string]interface{}{ + "protocolVersion": "2024-11-05", + "capabilities": map[string]interface{}{}, + "clientInfo": map[string]interface{}{"name": "scope-http-matrix", "version": "1.0"}, + }) + require.Equal(t, http.StatusOK, status, "initialize must succeed on %s for a validly scoped token", path) +} + +// callTool sends one tools/call for name on path and returns its envelope. +func (e *httpMatrixEnv) callTool(t *testing.T, path, name string) map[string]interface{} { + t.Helper() + envelope, status := e.rpc(t, path, "tools/call", map[string]interface{}{ + "name": name, + "arguments": map[string]interface{}{}, + }) + require.Equal(t, http.StatusOK, status) + return envelope +} + +// unregisteredToolIsNA asserts the "n/a" cell shape: calling a tool name +// this surface never registers at all is refused with the JSON-RPC-LEVEL +// `-32602` "tool ... not found" mcp-go emits for any unregistered name — +// topology, identical for every caller kind, never a scope decision (never +// the application-level `isError:true` a REGISTERED tool's handler can also +// return for bad arguments, which would make this check pass vacuously for +// a tool that exists but was called wrong — cross-model review round 1 +// caught exactly that gap in an earlier version of this function). Proven +// by comparing against a name guaranteed not to exist on ANY surface: the +// two must produce the SAME error code and an equivalent message shape. +func (e *httpMatrixEnv) unregisteredToolIsNA(t *testing.T, path, toolName string) { + t.Helper() + e.initializeOn(t, path) + + envelope := e.callTool(t, path, toolName) + errObj, hasError := envelope["error"].(map[string]interface{}) + require.True(t, hasError, "n/a cell %s on %s: expected a JSON-RPC-level error (not an application-level result), got %v", toolName, path, envelope) + code, _ := errObj["code"].(float64) + assert.Equal(t, float64(-32602), code, "n/a cell %s on %s: expected the mcp-go unregistered-tool code -32602, got %v (%v)", toolName, path, code, errObj) + msg := fmt.Sprint(errObj["message"]) + assert.True(t, strings.Contains(msg, "not found") || strings.Contains(msg, "unknown tool"), + "n/a cell %s on %s: expected an unregistered-tool refusal, got %q", toolName, path, msg) + + // Positive control: a name that provably does not exist anywhere gets + // the SAME code and an equivalent message — proving the n/a cell is + // registration-level topology, not a disguised scope refusal that + // happens to share wording. + nonexistentEnvelope := e.callTool(t, path, "definitely-does-not-exist-anywhere-"+toolName) + nonexistentErr, ok := nonexistentEnvelope["error"].(map[string]interface{}) + require.True(t, ok, "control: a definitely-nonexistent tool must also be a JSON-RPC error on %s, got %v", path, nonexistentEnvelope) + nonexistentCode, _ := nonexistentErr["code"].(float64) + assert.Equal(t, nonexistentCode, code, "n/a cell %s on %s: must carry the SAME error code as a definitely-nonexistent tool", toolName, path) +} + +// TestScopeHTTPMatrix_RetrieveSurfaceHasReadCacheSetProfileTailLog is the +// `yes` column for the retrieve surface (/mcp, /mcp/call): read_cache, +// set_profile and tail_log are all registered and reachable by a scoped +// token over real HTTP. +func TestScopeHTTPMatrix_RetrieveSurfaceHasReadCacheSetProfileTailLog(t *testing.T) { + env := newHTTPMatrixEnv(t) + for _, path := range []string{"/mcp", "/mcp/call"} { + t.Run(path, func(t *testing.T) { + env.initializeOn(t, path) + for _, tool := range []string{"read_cache", "set_profile", "upstream_servers", "retrieve_tools"} { + envelope, status := env.rpc(t, path, "tools/call", map[string]interface{}{ + "name": tool, + "arguments": minimalArgsFor(tool), + }) + require.Equal(t, http.StatusOK, status, "%s on %s must be a registered tool (HTTP 200, JSON-RPC envelope)", tool, path) + _, hasError := envelope["error"] + assert.False(t, hasError, "%s on %s: expected the tool to be REGISTERED (an application-level refusal inside `result` is fine; a JSON-RPC method-level error is not) — envelope: %v", tool, path, envelope) + } + }) + } +} + +// TestScopeHTTPMatrix_DirectSurfacePublicationFiltering is the `yes` cell for +// direct-surface (/mcp/all) publication filtering: a scoped token's +// tools/list on /mcp/all must list ONLY tools its own grant authorizes — +// the hidden server's tool, and its sentinel, must never appear on the wire. +func TestScopeHTTPMatrix_DirectSurfacePublicationFiltering(t *testing.T) { + env := newHTTPMatrixEnv(t) + env.initializeOn(t, "/mcp/all") + envelope, status := env.rpc(t, "/mcp/all", "tools/list", map[string]interface{}{}) + require.Equal(t, http.StatusOK, status) + result, ok := envelope["result"].(map[string]interface{}) + require.True(t, ok, "tools/list must return a result: %v", envelope) + tools, _ := result["tools"].([]interface{}) + require.NotEmpty(t, tools, "the scoped token must see its own research-srv tools") + for _, raw := range tools { + tool, _ := raw.(map[string]interface{}) + name, _ := tool["name"].(string) + assert.False(t, strings.Contains(name, "deploy"), + "a research-srv-only token must never see deploy-srv's tools on the direct surface: %s", name) + assert.False(t, strings.HasPrefix(name, "deploy_"), name) + } +} + +// TestScopeHTTPMatrix_CodeSurfaceHasSetProfileAndRetrieveNoDetail is the +// `yes` column for the code surface (/mcp/code): set_profile and +// retrieve_tools are registered there too (retrieve_tools without the +// `detail` override, per the applicability matrix note). +func TestScopeHTTPMatrix_CodeSurfaceHasSetProfileAndRetrieve(t *testing.T) { + env := newHTTPMatrixEnv(t) + env.initializeOn(t, "/mcp/code") + for _, tool := range []string{"set_profile", "retrieve_tools"} { + envelope, status := env.rpc(t, "/mcp/code", "tools/call", map[string]interface{}{ + "name": tool, + "arguments": minimalArgsFor(tool), + }) + require.Equal(t, http.StatusOK, status, "%s on /mcp/code must be registered", tool) + _, hasError := envelope["error"] + assert.False(t, hasError, "%s on /mcp/code: expected the tool to be REGISTERED — envelope: %v", tool, envelope) + } +} + +// TestScopeHTTPMatrix_NACellsAreUnregisteredNotScopeRefusals drives the +// applicability matrix's `n/a` cells: read_cache and tail_log +// (upstream_servers) do not exist on /mcp/all at all — calling them is the +// SAME unregistered-tool shape a nonexistent name gets, never a +// scope-specific refusal (which would itself be a disclosure that the tool +// exists elsewhere). +func TestScopeHTTPMatrix_NACellsAreUnregisteredNotScopeRefusals(t *testing.T) { + env := newHTTPMatrixEnv(t) + env.unregisteredToolIsNA(t, "/mcp/all", "read_cache") + env.unregisteredToolIsNA(t, "/mcp/all", "upstream_servers") + env.unregisteredToolIsNA(t, "/mcp/all", "set_profile") +} + +// TestScopeHTTPMatrix_ProfileURLSurfaceUniformRefusal is FR-014's +// `/mcp/p/` row: a scoped, UNPINNED token reaching a profile URL whose +// server set does not intersect its own grant gets the SAME uniform +// non-disclosing refusal a nonexistent slug gets — driven over real HTTP, +// re-proving TestProfile_ScopedUnpinnedRefusalUniform's invariant through +// this file's own token-minting path (mintProfileAgentToken rather than +// mintPinnedToken) as the HTTP-matrix contract asks for. +func TestScopeHTTPMatrix_ProfileURLSurfaceUniformRefusal(t *testing.T) { + env := newHTTPMatrixEnv(t) // token allowed=[research-srv], unpinned + disjoint, disjointStatus := env.rpc(t, "/mcp/p/deploy", "initialize", map[string]interface{}{ + "protocolVersion": "2024-11-05", "capabilities": map[string]interface{}{}, + "clientInfo": map[string]interface{}{"name": "t", "version": "1"}, + }) + nonexistent, nonexistentStatus := env.rpc(t, "/mcp/p/does-not-exist", "initialize", map[string]interface{}{ + "protocolVersion": "2024-11-05", "capabilities": map[string]interface{}{}, + "clientInfo": map[string]interface{}{"name": "t", "version": "1"}, + }) + require.Equal(t, http.StatusNotFound, disjointStatus) + require.Equal(t, http.StatusNotFound, nonexistentStatus) + assert.Equal(t, nonexistent["error"], disjoint["error"], + "a non-selectable profile ('deploy', disjoint from this token's grant) must be indistinguishable from a nonexistent slug") + + // Positive control: the token's own selectable profile URL is reachable + // (route matches, not the uniform refusal). + own, ownStatus := env.rpc(t, "/mcp/p/research", "initialize", map[string]interface{}{ + "protocolVersion": "2024-11-05", "capabilities": map[string]interface{}{}, + "clientInfo": map[string]interface{}{"name": "t", "version": "1"}, + }) + assert.Equal(t, http.StatusOK, ownStatus, "the token's own selectable profile URL must be reachable: %v", own) +} + +// TestScopeHTTPMatrix_PromptsListGetAcrossAllThreeSurfaces is FR-014's +// "prompts list/get" row — the one row the matrix marks `yes` on EVERY +// surface. Cross-model review round 2: the matrix wasn't covering this row +// at all. +func TestScopeHTTPMatrix_PromptsListGetAcrossAllThreeSurfaces(t *testing.T) { + env := newHTTPMatrixEnv(t) + for _, path := range []string{"/mcp", "/mcp/all", "/mcp/code"} { + t.Run(path, func(t *testing.T) { + env.initializeOn(t, path) + envelope, status := env.rpc(t, path, "prompts/list", map[string]interface{}{}) + require.Equal(t, http.StatusOK, status, "prompts/list on %s must be registered (HTTP 200)", path) + _, hasError := envelope["error"] + assert.False(t, hasError, "prompts/list on %s: expected REGISTERED (method-level error means unregistered) — envelope: %v", path, envelope) + + // prompts/get on a definitely-nonexistent name must be a plain + // application refusal reachable through the registered method + // (not a method-level -32601 "method not found" — that would + // mean prompts/get itself is unregistered on this surface). + getEnvelope, getStatus := env.rpc(t, path, "prompts/get", map[string]interface{}{"name": "definitely-nonexistent-prompt-xyz"}) + require.Equal(t, http.StatusOK, getStatus) + if errObj, ok := getEnvelope["error"].(map[string]interface{}); ok { + code, _ := errObj["code"].(float64) + assert.NotEqual(t, float64(-32601), code, "prompts/get on %s: -32601 would mean the METHOD itself is unregistered on this surface, not just the prompt name", path) + } + }) + } +} + +// TestScopeHTTPMatrix_DescribeToolRetrieveAndDirectCodeNA is FR-014's +// "describe_tool" row: yes on retrieve and direct, n/a on code. +func TestScopeHTTPMatrix_DescribeToolRetrieveAndDirectCodeNA(t *testing.T) { + env := newHTTPMatrixEnv(t) + for _, path := range []string{"/mcp", "/mcp/all"} { + t.Run(path+"_yes", func(t *testing.T) { + env.initializeOn(t, path) + envelope, status := env.rpc(t, path, "tools/call", map[string]interface{}{ + "name": "describe_tool", + "arguments": map[string]interface{}{"ids": []string{"research-srv:search_papers"}}, + }) + require.Equal(t, http.StatusOK, status) + _, hasError := envelope["error"] + assert.False(t, hasError, "describe_tool on %s: expected REGISTERED — envelope: %v", path, envelope) + }) + } + t.Run("/mcp/code_na", func(t *testing.T) { + env.unregisteredToolIsNA(t, "/mcp/code", "describe_tool") + }) +} + +// TestScopeHTTPMatrix_TailLogOperationRetrieveAndCode is FR-014's +// "tail_log / per-server ops" row driven with the EXPLICIT tail_log +// operation (not just upstream_servers' default `list`), on the two +// surfaces the matrix marks `yes`. +func TestScopeHTTPMatrix_TailLogOperationRetrieveAndCode(t *testing.T) { + env := newHTTPMatrixEnv(t) + for _, path := range []string{"/mcp", "/mcp/code"} { + t.Run(path, func(t *testing.T) { + env.initializeOn(t, path) + envelope, status := env.rpc(t, path, "tools/call", map[string]interface{}{ + "name": "upstream_servers", + "arguments": map[string]interface{}{"operation": "tail_log", "name": "research-srv", "lines": float64(10)}, + }) + require.Equal(t, http.StatusOK, status, "tail_log on %s must be registered", path) + _, hasError := envelope["error"] + assert.False(t, hasError, "tail_log on %s: expected REGISTERED — envelope: %v", path, envelope) + }) + } +} + +// TestScopeHTTPMatrix_ExecutionTierRetrieveCallToolVariants is FR-014's +// "execution tier" row on the retrieve surface: call_tool_read/write/destructive +// are all registered and reachable by a scoped token over real HTTP (the +// in-process differential tests, scope_target_tier_matrix_test.go's 54-cell +// table, prove the actual tier-enforcement logic in depth; this file's job +// is proving the HTTP/session path to reach them at all). +func TestScopeHTTPMatrix_ExecutionTierRetrieveCallToolVariants(t *testing.T) { + env := newHTTPMatrixEnv(t) + env.initializeOn(t, "/mcp") + for _, variant := range []string{"call_tool_read", "call_tool_write", "call_tool_destructive"} { + t.Run(variant, func(t *testing.T) { + envelope, status := env.rpc(t, "/mcp", "tools/call", map[string]interface{}{ + "name": variant, + "arguments": map[string]interface{}{"name": "research-srv:search_papers", "args": map[string]interface{}{}}, + }) + require.Equal(t, http.StatusOK, status, "%s must be registered", variant) + _, hasError := envelope["error"] + assert.False(t, hasError, "%s: expected REGISTERED (an application-level insufficient-permission refusal inside `result` is fine and expected for write/destructive here) — envelope: %v", variant, envelope) + }) + } +} + +// TestScopeHTTPMatrix_RetrieveToolsNonDisclosureOverRealHTTP is the +// content-level non-disclosure proof cross-model review rounds 2 and 3 +// asked for: driven through mcpAuthMiddleware over real loopback HTTP (not +// an injected auth.AuthContext), a scoped token's retrieve_tools response +// must never contain hidden deploy-srv's tool names anywhere in the RAW +// wire text — a raw substring check, not merely a structural JSON field +// check, so a leak into an unexpected field (a suggestion, a diagnostic, an +// error message) would still be caught. +func TestScopeHTTPMatrix_RetrieveToolsNonDisclosureOverRealHTTP(t *testing.T) { + env := newHTTPMatrixEnv(t) + env.initializeOn(t, "/mcp") + // The query text itself is echoed back verbatim in debug/query_analysis + // fields (harmless — echoing the CALLER's own input is not a + // disclosure), so the leak-check needles below are deliberately chosen + // to be tool/server IDENTIFIERS that do not appear in the query text, + // never the query words themselves. + envelope, status := env.rpc(t, "/mcp", "tools/call", map[string]interface{}{ + "name": "retrieve_tools", + "arguments": map[string]interface{}{"query": "deployment rollback process", "limit": float64(20), "include_stats": true, "debug": true}, + }) + require.Equal(t, http.StatusOK, status) + raw, err := json.Marshal(envelope) + require.NoError(t, err) + rawText := string(raw) + + for _, needle := range []string{"deploy_app", "deploy-srv"} { + assert.NotContains(t, rawText, needle, + "a research-srv-only token's retrieve_tools response must never mention hidden deploy-srv's %q anywhere in the wire envelope, even though the query is deliberately chosen to match deploy-srv's rollback tool's description (\"Rollback deployment\")", needle) + } + + // Positive control: an unrelated query that DOES match the token's own + // authorized content proves real results flow through this same HTTP + // path (the deploy-matching query above may legitimately return zero + // hits once scope-filtered, which would make the absence check above + // vacuous on its own). + controlEnvelope, controlStatus := env.rpc(t, "/mcp", "tools/call", map[string]interface{}{ + "name": "retrieve_tools", + "arguments": map[string]interface{}{"query": "search academic papers", "limit": float64(20)}, + }) + require.Equal(t, http.StatusOK, controlStatus) + controlResult, ok := controlEnvelope["result"].(map[string]interface{}) + require.True(t, ok, "control query must succeed: %v", controlEnvelope) + controlRaw, err := json.Marshal(controlResult) + require.NoError(t, err) + assert.Contains(t, string(controlRaw), "search_papers", + "control: the token's OWN authorized tool must be findable through this exact HTTP path, or the absence checks above prove nothing") +} + +// minimalArgsFor returns just-enough arguments for a tool call to reach its +// handler (not necessarily to succeed) — this file only asserts REGISTRATION +// (the tool exists, JSON-RPC accepted it), not the application-level +// success/failure the in-process differential tests already cover in depth. +func minimalArgsFor(tool string) map[string]interface{} { + switch tool { + case "read_cache": + return map[string]interface{}{"key": strings.Repeat("0", 64)} + case "set_profile": + return map[string]interface{}{} + case "upstream_servers": + return map[string]interface{}{"operation": "list"} + case "retrieve_tools": + return map[string]interface{}{"query": "test", "limit": float64(5)} + case "code_execution": + return map[string]interface{}{"code": "return 1;"} + default: + return map[string]interface{}{} + } +} diff --git a/internal/server/scope_latency_test.go b/internal/server/scope_latency_test.go index 781764948..38827240e 100644 --- a/internal/server/scope_latency_test.go +++ b/internal/server/scope_latency_test.go @@ -2,17 +2,54 @@ package server import ( "context" + "fmt" + "os" "sort" + "sync" "testing" "time" "github.com/mark3labs/mcp-go/mcp" + mcpserver "github.com/mark3labs/mcp-go/server" + "github.com/mark3labs/mcp-go/server/servertest" "github.com/stretchr/testify/require" + "go.uber.org/zap" "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream" ) +// scopeLatencyResultsFileEnv names the file .github/workflows/scope-latency.yml +// (T112a) points at when it runs this suite on the merge-base and HEAD +// checkouts in turn: when set, every operation below appends one +// "=" line, so the workflow's Go comparison +// program (cmd/scope-latency-compare, T112a) can diff the two revisions' +// administrator p95 without parsing test output. Unset (the default, every +// local/PR run) this is a no-op — the in-run scoped-vs-admin delta assertion +// below is the gate that always runs. +const scopeLatencyResultsFileEnv = "SCOPE_LATENCY_RESULTS_FILE" + +var scopeLatencyResultsMu sync.Mutex + +// recordAdminLatencyResult appends "=" to the file named by +// scopeLatencyResultsFileEnv, if set. +func recordAdminLatencyResult(t *testing.T, op string, adminP95 time.Duration) { + t.Helper() + path := os.Getenv(scopeLatencyResultsFileEnv) + if path == "" { + return + } + scopeLatencyResultsMu.Lock() + defer scopeLatencyResultsMu.Unlock() + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + require.NoError(t, err) + defer f.Close() + _, err = fmt.Fprintf(f, "%s=%d\n", op, adminP95.Nanoseconds()) + require.NoError(t, err) +} + // Spec 105 PR C, T078 (FR-011 pre-check). // // SearchToolsScoped (index/bleve.go) pages the ranked result exhaustively @@ -96,6 +133,7 @@ func TestRetrieveTools_ScopeLatency_ScopedVsAdmin(t *testing.T) { t.Logf("retrieve_tools p95 over %d timed calls (527-tool snapshot, %d servers, scope=1 server): admin=%s scoped=%s gap=%s", timed, len(servers), p95Admin, p95Scoped, gap) + recordAdminLatencyResult(t, "retrieve_tools", p95Admin) const budget = 20 * time.Millisecond require.LessOrEqualf(t, gap, budget, @@ -103,6 +141,276 @@ func TestRetrieveTools_ScopeLatency_ScopedVsAdmin(t *testing.T) { budget, p95Admin, p95Scoped, gap) } +// Spec 105 PR H1, T112: finalises the FR-011 latency harness over the +// remaining three operations (retrieve_tools is T078/T078 above) — 20 +// warm-up + 200 timed calls per caller, p95(scoped) - p95(admin) <= 20ms per +// operation. Frozen fixtures: a 10-page cache entry and a 50-prompt set, +// built deterministically in code (not randomised) beside the existing +// 527-tool LiveMCPBench snapshot at internal/server/testdata/scope_latency/ +// — see that directory's README for why the fixtures are generated in code +// rather than as separate serialized files (T112a's merge-base job copies +// this whole test file across revisions, which carries the generator code +// with it exactly as a serialized file would). Skipped under -race for the +// same reason as T078. + +// measureLatency runs warm-up then timed calls of fn under ctx and returns +// the timed durations. +func measureLatency(t *testing.T, ctx context.Context, warmup, timed int, fn func(context.Context) error) []time.Duration { + t.Helper() + for i := 0; i < warmup; i++ { + require.NoError(t, fn(ctx)) + } + durations := make([]time.Duration, 0, timed) + for i := 0; i < timed; i++ { + start := time.Now() + require.NoError(t, fn(ctx)) + durations = append(durations, time.Since(start)) + } + return durations +} + +// assertScopedWithinBudget is the FR-011 SC-006 assertion shared by every +// operation below. +func assertScopedWithinBudget(t *testing.T, op string, scoped, admin []time.Duration) { + t.Helper() + pScoped, pAdmin := p95(scoped), p95(admin) + gap := pScoped - pAdmin + t.Logf("%s p95 over %d timed calls: admin=%s scoped=%s gap=%s", op, len(scoped), pAdmin, pScoped, gap) + recordAdminLatencyResult(t, op, pAdmin) + const budget = 20 * time.Millisecond + require.LessOrEqualf(t, gap, budget, + "%s: scoped p95 must not exceed admin's by more than FR-011's %s budget (got admin=%s scoped=%s gap=%s)", + op, budget, pAdmin, pScoped, gap) +} + +// TestScopeLatency_ReadCache_ScopedVsAdmin: a frozen, oversized cache entry +// (10 truncated pages' worth of content) produced by the SAME scoped caller +// that reads it back, so both callers hit a live, redeemable key — the +// comparison is about the AUTHORIZATION CHECK's own cost, not about one +// caller hitting a refusal fast-path the other doesn't. +func TestScopeLatency_ReadCache_ScopedVsAdmin(t *testing.T) { + if testing.Short() { + t.Skip("integration — 440 read_cache calls") + } + if raceEnabled { + t.Skip("timing is meaningless under the race detector's instrumentation overhead") + } + proxy := createTestMCPProxyServer(t) + require.NoError(t, proxy.storage.SaveUpstreamServer(&config.ServerConfig{Name: "a", Enabled: true})) + scopedCtx := agentCtx([]string{"a"}, []string{auth.PermRead}, "") + + // A frozen "10-page" entry: 10 records of deterministic padded content, + // large enough that read_cache pages it rather than returning it whole. + records := make([]string, 0, 10*50) + for page := 0; page < 10; page++ { + for i := 0; i < 50; i++ { + records = append(records, `{"id":"`+recordID(page, i)+`","note":"frozen scope-latency fixture padding padding padding padding"}`) + } + } + content := "[" + joinStrings(records, ",") + "]" + stamp := proxy.cacheAuthorization(scopedCtx) + const key = "scope-latency-frozen-key-0000000000000000000000000000000000000" + require.NoError(t, proxy.cacheManager.StoreAs(key, "retrieve_tools", + map[string]interface{}{"query": "frozen"}, content, "", len(records), stamp)) + + call := func(ctx context.Context) error { + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"key": key, "offset": float64(0), "limit": float64(50)} + result, err := proxy.handleReadCache(ctx, req) + if err != nil { + return err + } + if result.IsError { + return errIsError + } + return nil + } + + adminDurations := measureLatency(t, adminCtx(), 20, 200, call) + scopedDurations := measureLatency(t, scopedCtx, 20, 200, call) + assertScopedWithinBudget(t, "read_cache", scopedDurations, adminDurations) +} + +// connectLatencyPromptUpstream wires ONE real streamable-HTTP upstream +// exposing the given prompts and runs RefreshPrompts once. Deliberately +// self-contained (does not call scope_retained_effects_test.go's +// connectPromptUpstream): .github/workflows/scope-latency.yml (T112a) copies +// ONLY this file plus testdata/scope_latency/ over a merge-base checkout +// that does not have scope_retained_effects_test.go at all (that file is +// new in this same PR) — a cross-file helper dependency would make the +// copied file fail to compile there (cross-model review round 1 caught this +// exact break). internal/server/scope_fixture_test.go (created by an +// EARLIER, already-merged PR) is present on both sides, but its helpers are +// tool-oriented, not prompt-oriented, so this file carries its own minimal +// prompt-upstream wiring instead of depending on either. +func connectLatencyPromptUpstream(t *testing.T, proxy *MCPProxyServer, server string, prompts []mcp.Prompt) { + t.Helper() + proxy.config.EnablePrompts = true + proxy.config.AggregateUpstreamPrompts = true + qOff := false + proxy.config.QuarantineEnabled = &qOff + + um := upstream.NewManager(zap.NewNop(), proxy.config, nil, secret.NewResolver(), nil) + t.Cleanup(func() { um.DisconnectAll() }) + mcpSrv := mcpserver.NewMCPServer(server, "1.0.0-test", mcpserver.WithPromptCapabilities(true)) + for _, p := range prompts { + text := p.Description + mcpSrv.AddPrompt(p, func(_ context.Context, _ mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + return &mcp.GetPromptResult{Messages: []mcp.PromptMessage{ + {Role: mcp.RoleAssistant, Content: mcp.TextContent{Type: "text", Text: text}}, + }}, nil + }) + } + testServer := servertest.NewTestStreamableHTTPServer(mcpSrv) + t.Cleanup(testServer.Close) + require.NoError(t, um.AddServerConfig(server, &config.ServerConfig{ + Name: server, Protocol: "streamable-http", URL: testServer.URL, Enabled: true, + })) + client, ok := um.GetClient(server) + require.True(t, ok) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + require.NoError(t, client.Connect(ctx)) + cancel() + + proxy.upstreamManager = um + proxy.RefreshPrompts() +} + +// TestScopeLatency_PromptsList_ScopedVsAdmin: a frozen 50-prompt set (one +// real upstream connection, so RefreshPrompts runs the actual aggregation +// pipeline once), then 220 in-process prompts/list calls per caller kind. +func TestScopeLatency_PromptsList_ScopedVsAdmin(t *testing.T) { + if testing.Short() { + t.Skip("integration — 440 prompts/list calls") + } + if raceEnabled { + t.Skip("timing is meaningless under the race detector's instrumentation overhead") + } + proxy, _ := createTestProxyWithRuntime(t, nil) + prompts := make([]mcp.Prompt, 0, 50) + for i := 0; i < 50; i++ { + prompts = append(prompts, mcp.Prompt{Name: "frozen_prompt_" + recordID(0, i), Description: "frozen scope-latency fixture prompt"}) + } + connectLatencyPromptUpstream(t, proxy, "a", prompts) + + scopedCtx := agentCtx([]string{"a"}, []string{auth.PermRead}, "") + call := func(ctx context.Context) error { + raw := proxy.server.HandleMessage(ctx, []byte(`{"jsonrpc":"2.0","id":1,"method":"prompts/list"}`)) + if raw == nil { + return errIsError + } + return nil + } + adminDurations := measureLatency(t, adminCtx(), 20, 200, call) + scopedDurations := measureLatency(t, scopedCtx, 20, 200, call) + assertScopedWithinBudget(t, "prompts/list", scopedDurations, adminDurations) +} + +// TestScopeLatency_ToolsList_ScopedVsAdmin: the direct surface's tools/list +// over the 527-tool LiveMCPBench snapshot, in-process through +// directServer.HandleMessage. +// +// Cross-model review (round 1) caught that an earlier version of this test +// only indexed the corpus into bleve (proxy.index.BatchIndexTools) — the +// SEARCH surface's data source — while the direct surface's tools/list reads +// from p.directServer's REGISTERED tool set, built by RefreshDirectModeTools +// from upstreamManager.DiscoverTools, which requires LIVE connected clients. +// Without a live connection per server the direct catalog stayed at the +// handful of built-ins, so the "527-tool" claim measured an empty listing. +// Fixed by connecting one real streamable-HTTP stub server per distinct +// ServerName in the corpus (grouped, not one-by-one) and publishing them +// through the real RefreshDirectModeTools rebuild ONCE before the timed +// loop — the 220 in-process tools/list calls per caller then read the +// already-published in-memory registry (no network in the timed path), +// which is the listing/filtering cost this test is meant to measure. +func TestScopeLatency_ToolsList_ScopedVsAdmin(t *testing.T) { + if testing.Short() { + t.Skip("integration — 70 real upstream connections + 440 tools/list calls") + } + if raceEnabled { + t.Skip("timing is meaningless under the race detector's instrumentation overhead") + } + tools := loadDeferredLargeCorpus(t) + + byServer := make(map[string][]*config.ToolMetadata) + var serverOrder []string + for _, tool := range tools { + if _, ok := byServer[tool.ServerName]; !ok { + serverOrder = append(serverOrder, tool.ServerName) + } + byServer[tool.ServerName] = append(byServer[tool.ServerName], tool) + } + require.Greater(t, len(serverOrder), 10, "fixture: the snapshot must name more than a handful of servers") + + proxy, rt := createTestProxyWithRuntime(t, nil) + qOff := false + proxy.config.QuarantineEnabled = &qOff // a listing-throughput fixture has no interest in per-tool approval state + + for _, serverName := range serverOrder { + specs := make([]toolSpec, 0, len(byServer[serverName])) + for _, tool := range byServer[serverName] { + specs = append(specs, readSpec(tool.Name)) + } + startCountingUpstream(t, proxy, rt, serverName, specs...) + } + proxy.RefreshDirectModeTools() + + listed := proxy.directServer.ListTools() + require.Greater(t, len(listed), 500, + "fixture: the direct catalog must actually carry the ~527-tool corpus, not just the built-ins (got %d)", len(listed)) + + scopedCtx := agentCtx([]string{serverOrder[0]}, []string{auth.PermRead}, "") + call := func(ctx context.Context) error { + raw := proxy.directServer.HandleMessage(ctx, []byte(`{"jsonrpc":"2.0","id":1,"method":"tools/list"}`)) + if raw == nil { + return errIsError + } + return nil + } + adminDurations := measureLatency(t, adminCtx(), 20, 200, call) + scopedDurations := measureLatency(t, scopedCtx, 20, 200, call) + assertScopedWithinBudget(t, "tools/list", scopedDurations, adminDurations) +} + +var errIsError = &latencyIsError{} + +type latencyIsError struct{} + +func (*latencyIsError) Error() string { return "call returned an application-level error result" } + +// recordID formats a deterministic, zero-padded id from (page, i) without +// pulling in fmt.Sprintf on a hot path used 20,000+ times across this file's +// three fixture builders. +func recordID(page, i int) string { + return itoaPadded(page) + "-" + itoaPadded(i) +} + +func itoaPadded(n int) string { + s := "" + if n == 0 { + return "00" + } + for n > 0 { + s = string(rune('0'+n%10)) + s + n /= 10 + } + for len(s) < 2 { + s = "0" + s + } + return s +} + +func joinStrings(items []string, sep string) string { + out := "" + for i, it := range items { + if i > 0 { + out += sep + } + out += it + } + return out +} + // p95 returns the 95th-percentile duration, sorting a copy so the caller's // slice order is left intact. func p95(durations []time.Duration) time.Duration { diff --git a/internal/server/scope_pinned_reversal_guard_test.go b/internal/server/scope_pinned_reversal_guard_test.go new file mode 100644 index 000000000..dc6d436a1 --- /dev/null +++ b/internal/server/scope_pinned_reversal_guard_test.go @@ -0,0 +1,169 @@ +package server + +import ( + "os" + "regexp" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Spec 105 PR H1, T113: gap-map.md's FR01x-G7 entry (narrowed claim, §3) +// names FOUR tests that pinned pre-105 (insecure) behaviour as their +// assertion, not merely "referenced" it in prose — the ones SC-007 counts as +// the gate the parallel branches merged ahead of the spec left behind: +// +// 1. internal/server/mcp_direct_skew_test.go — the forward-flip seam's +// listing used to assert the OLD owner's definition was "still listed" +// (FR-008 G1). Inverted in Spec 105 PR F (#1326 / e33d6a139); the exact +// assertion and its literal message ("so it is still listed") were +// removed, not merely negated. +// 2. internal/server/mcp_call_tool_target_tier_test.go — an undiscovered +// ("ghost") tool on a known server used to be granted the destructive +// tier and reach dispatch, asserted by `assert.Contains(t, text, "No +// client found")` inside what is now +// TestCallToolRead_UndiscoveredTool_RefusedForEveryCaller (renamed from +// TestCallToolRead_UndiscoveredTool_RequiresDestructiveTier). Inverted in +// Spec 105 PR A (#1279 / bac8f6b0d, research D4). +// 3. internal/server/mcp_read_cache_authz_test.go — a revoked/narrower +// redemption used to get its own disclosing body, asserted by +// `assert.Contains(t, resultText(t, result), "not readable with this +// credential")`. Inverted in Spec 105 PR B (#1282 / 46de8038c): the body +// now collapses into the same "cache key not found" a nonexistent key +// produces; the old wording survives only as an explanatory comment. +// 4. internal/cache/authorization_test.go — TestAuthorization_CouldHaveProduced +// used to deny a URL/session-profile-bound administrator reading an +// unscoped administrator entry ({"admin bound to a URL profile cannot +// read an unscoped admin entry", admin, adminInProfile, false}). +// Inverted in Spec 105 PR B (research D5, caller-kind-first): the case +// was renamed to end "(kind first)" and its verdict flipped to true. +// +// This file scans the actual test SOURCE TEXT (not the compiled package — +// #4 lives in a different Go package and cannot be imported/called from +// here) for the literal markers that would only be present if the assertion +// reverted to its pre-105 form. Comments that merely DOCUMENT the old +// wording (as #3's does) are not a violation; only a live assertion is. + +// mustReadSourceFile reads a test source file for guard scanning. It uses a +// path relative to this package's directory (internal/server), so `t.Run` +// working-directory quirks under `go test ./...` (always the package dir) +// are exactly what this relies on. +func mustReadSourceFile(t *testing.T, relPath string) string { + t.Helper() + data, err := os.ReadFile(relPath) + require.NoError(t, err, "guard fixture: %s must exist", relPath) + return string(data) +} + +// nonCommentLinesContain reports whether any line of src that is not a `//` +// comment (leading whitespace then `//`) contains needle. To resist the +// simplest evasion (splitting a literal across a Go string-concatenation: +// `"not readable " + "with this credential"`), it first strips `" + "`/`"+"` +// sequences between adjacent quoted segments so a split literal collapses +// back to its concatenated form before matching. This is deliberately NOT +// an AST-based check (cross-model review round 1 asked for one; a full +// token-stream/AST guard is a bigger lift than this PR's remaining budget +// justifies for a regression guard whose realistic threat model is +// ACCIDENTAL reintroduction via a bad merge/rebase/cherry-pick, not a +// deliberately adversarial rewrite designed to evade grep). +func nonCommentLinesContain(src, needle string) bool { + collapsed := stringConcatCollapseRE.ReplaceAllString(src, "") + for _, line := range strings.Split(collapsed, "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "//") { + continue + } + if strings.Contains(line, needle) { + return true + } + } + return false +} + +// stringConcatCollapseRE matches a closing quote, optional whitespace, a `+`, +// optional whitespace, and an opening quote — the shape of two adjacent Go +// string literals being concatenated — so nonCommentLinesContain can undo a +// literal deliberately split to dodge a plain substring search. +var stringConcatCollapseRE = regexp.MustCompile(`"\s*\+\s*"`) + +// extractFunc returns the source of a single top-level Go function by name, +// from its `func (` line up to (but not including) the next top-level +// `func ` line, or the end of the file. +func extractFunc(t *testing.T, src, name string) string { + t.Helper() + re := regexp.MustCompile(`(?m)^func ` + regexp.QuoteMeta(name) + `\(`) + loc := re.FindStringIndex(src) + require.NotNil(t, loc, "guard fixture: function %s must still exist in this file", name) + rest := src[loc[1]:] + nextRe := regexp.MustCompile(`(?m)^func `) + if end := nextRe.FindStringIndex(rest); end != nil { + return rest[:end[0]] + } + return rest +} + +// TestScopePinnedReversalsStayInverted is T113's grep guard: it fails if any +// of the four assertions the doc comment above names has reverted to its +// original, pre-105 form. +func TestScopePinnedReversalsStayInverted(t *testing.T) { + t.Run("mcp_direct_skew_test.go: forward-flip seam no longer lists the stale owner's definition", func(t *testing.T) { + src := mustReadSourceFile(t, "mcp_direct_skew_test.go") + assert.NotContains(t, src, "so it is still listed", + "the pre-105 assertion that the forward-flip seam still lists the OLD owner's definition must not reappear (FR-008 G1, Spec 105 PR F)") + // Structural companion (not text-message-based): the pre-105 + // assertion's SHAPE was `require.Contains(t, f.listed(oldOnly), + // display, ...)` — a LIVE (non-Not) Contains against the + // forward-flip fixture's listing. Requiring the fixed file to + // instead use NotContains against that same listing call resists a + // reversal that keeps the assertion live but drops or rewords its + // message. + liveAdmit := regexp.MustCompile(`\b(?:require|assert)\.Contains\(\s*t,\s*f\.listed\(oldOnly\)`) + assert.False(t, liveAdmit.MatchString(src), + "a live (non-Not) Contains against f.listed(oldOnly) must not reappear regardless of its message (FR-008 G1, Spec 105 PR F)") + notAdmit := regexp.MustCompile(`\b(?:require|assert)\.NotContains\(\s*t,\s*f\.listed\(oldOnly\)`) + assert.True(t, notAdmit.MatchString(src), + "guard fixture: the inverted NotContains assertion against f.listed(oldOnly) must still exist somewhere in this file") + }) + + t.Run("mcp_call_tool_target_tier_test.go: undiscovered tool never reaches dispatch", func(t *testing.T) { + src := mustReadSourceFile(t, "mcp_call_tool_target_tier_test.go") + require.Contains(t, src, "func TestCallToolRead_UndiscoveredTool_RefusedForEveryCaller(", + "the inverted test must still exist under its post-105 name (was TestCallToolRead_UndiscoveredTool_RequiresDestructiveTier)") + body := extractFunc(t, src, "TestCallToolRead_UndiscoveredTool_RefusedForEveryCaller") + // Whitespace-tolerant (a reformatted `assert.Contains(\n\tt, text,\n\t"No client found")` + // must still be caught), but still requires the literal "No client + // found" quoted string next to a live (non-Not) Contains call — see + // the file-level doc comment for why this stops short of an AST + // check. + liveAdmit := regexp.MustCompile(`(?s)\bassert\.Contains\(\s*t,\s*text,\s*"No client found"\s*\)`) + assert.False(t, liveAdmit.MatchString(body), + "the pre-105 assertion that an undiscovered tool's call reaches dispatch (\"No client found\") must not reappear (FR-009 G4, Spec 105 PR A, research D4)") + }) + + t.Run("mcp_read_cache_authz_test.go: revoked redemption collapses into the nonexistent-key body", func(t *testing.T) { + src := mustReadSourceFile(t, "mcp_read_cache_authz_test.go") + assert.False(t, nonCommentLinesContain(src, `"not readable with this credential"`), + "the pre-105 disclosing body \"not readable with this credential\" must not be asserted again outside of an explanatory comment (FR-001 G5, Spec 105 PR B)") + assert.Contains(t, src, "cache key not found", + "the collapsed non-disclosing body must still be asserted somewhere in this file") + }) + + t.Run("internal/cache/authorization_test.go: caller-kind-first administrator redemption", func(t *testing.T) { + src := mustReadSourceFile(t, "../cache/authorization_test.go") + // Structural, not text-label-based (cross-model review round 1: the + // original check matched one exact case-name string and would miss + // a relabeled row asserting the same wrong verdict): find the table + // row whose PRODUCER is `admin` and whose READER is `adminInProfile` + // — {producer=admin, reader=X, want=bool} is TestAuthorization_CouldHaveProduced's + // case-literal shape — and require its verdict to be `true` + // (caller-kind-first: an administrator qualifies for ANY snapshot + // regardless of its own profile binding), whatever the case NAME + // says. + rowRE := regexp.MustCompile(`\{"[^"]*",\s*admin,\s*adminInProfile,\s*(true|false)\}`) + match := rowRE.FindStringSubmatch(src) + require.NotNil(t, match, "guard fixture: the {producer=admin, reader=adminInProfile} table row must still exist in TestAuthorization_CouldHaveProduced") + assert.Equal(t, "true", match[1], + "the pre-D5 verdict (a profile-bound administrator refused an unscoped administrator entry, verdict=false) must not reappear under ANY case-name label (FR-001 G6, Spec 105 PR B, research D5)") + }) +} diff --git a/internal/server/scope_retained_effects_test.go b/internal/server/scope_retained_effects_test.go new file mode 100644 index 000000000..83375be17 --- /dev/null +++ b/internal/server/scope_retained_effects_test.go @@ -0,0 +1,456 @@ +package server + +import ( + "context" + "strings" + "sync" + "testing" + "time" + + "github.com/mark3labs/mcp-go/mcp" + mcpserver "github.com/mark3labs/mcp-go/server" + "github.com/mark3labs/mcp-go/server/servertest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/upstream" +) + +// Spec 105 PR H1, T108a: the retained-effect mode of the differential oracle +// (contracts/differential-oracle.md, spec.md:114,161, SC-001's exclusion +// list). These are documented, DELIBERATELY UNCHANGED behaviours — a hidden +// server can still influence an authorized caller's outcome through shared +// infrastructure — so they are NOT run through runScopeScenario's +// cross-fixture byte-equality (that assertion would legitimately fail for +// every one of them, which is exactly why the spec excludes them from +// SC-001). Instead each fixture asserts: +// +// (a) every item returned is owned by an authorized server, +// (b) no hidden definition or content and no sentinel appears, +// (c) the documented outcome (the specific refusal / pending / shared-effect +// state) is observed. +// +// Two of the seven named effects (prompt-name collision, the global prompt +// cap) depend on Go MAP ITERATION ORDER inside upstream.Manager.ListPrompts +// (internal/upstream/manager_prompts.go) — which server's prompt "wins" a +// display-name collision, or which server's prompts survive the +// maxAggregatedPrompts backstop, is randomised per process by design. Their +// fixtures assert the ORDER-INDEPENDENT invariant (a)+(b) rather than which +// specific outcome occurred on this run — asserting a specific winner would +// be exactly the kind of flaky gate CLAUDE.md warns against (PRs F and G +// already hit real flakiness from over-tight assertions today). +// +// Direct display-name collision (spec.md:115) is already pinned, +// deterministically, by internal/server/mcp_direct_catalog_test.go (gap-map +// "already satisfied" list); TestScopeRetainedEffect_DirectDisplayCollision +// below re-runs it as this suite's named fixture for that effect. Shared log +// rotation/retention (FR-007) is pinned by +// internal/logs.TestReadUpstreamServerLogTail_AttributedOnly_ForcedRotationSharedHistory +// — a different Go package, cited here rather than re-invoked (same +// constraint as the US1.7 inventory entry in scope_differential_test.go). +// +// Shared prompt-refresh deadline (spec.md:115) is NOT reproduced live at +// THIS layer: MCPProxyServer.RefreshPrompts (mcp_routing.go) wraps the WHOLE +// fleet's upstream.Manager.ListPrompts call in one hardcoded +// `context.WithTimeout(context.Background(), 30*time.Second)` — not a config +// value, not overridable by a test — so exercising the 30-SECOND deadline +// specifically through RefreshPrompts would mean this suite either waits out +// 30 real seconds or forks the production code path to accept an injectable +// one, either a worse trade than proving the mechanism one layer down (an +// over-tight or artificially-slow gate is exactly what CLAUDE.md's PR F/G +// flakiness lesson warns against). ListPrompts (manager_prompts.go) itself +// takes its ctx from the CALLER and does nothing 30-second-specific — the +// production deadline is just a longer instance of the same sequential- +// calls-sharing-one-context mechanism — so the actual named fixture lives at +// that layer, with an artificially SHORT injected deadline, in +// internal/upstream/manager_prompts_deadline_test.go: +// TestManager_ListPrompts_SharedNotPerServerDeadline +// (verified non-vacuous: a generous deadline in place of the short one makes +// both peers' prompts appear and the test correctly fails). The multi-server +// sequential-aggregation PATH is additionally exercised, deterministically +// and without any injected delay, by +// TestScopeRetainedEffect_PromptNameCollision and +// TestScopeRetainedEffect_GlobalPromptCap below, which both run REAL +// multi-server ListPrompts aggregation through RefreshPrompts at this layer. + +// runRetainedEffectScenario is the alternative assertion mode for the +// documented retained effects (contracts/differential-oracle.md +// "Retained-effect mode"). fn builds its own fixture (each effect has a +// distinct one, per spec.md's fixture recipes) and returns the raw response +// text plus the sentinel(s) that must never appear in it. +func runRetainedEffectScenario(t *testing.T, name string, fn func(t *testing.T) (responseText string, sentinels []string)) { + t.Helper() + t.Run(name, func(t *testing.T) { + text, sentinels := fn(t) + for _, s := range sentinels { + assert.NotContains(t, text, s, "retained effect %q: no hidden content or sentinel may appear", name) + } + }) +} + +// --------------------------------------------------------------------------- +// 1. Shared-limiter contention — global capacity 1, no queue, a held call on +// hidden "b" makes a call to authorized "a" fail with the existing +// "proxy-wide limit saturated" response. +// --------------------------------------------------------------------------- + +func TestScopeRetainedEffect_SharedLimiterContention(t *testing.T) { + runRetainedEffectScenario(t, "shared-limiter-contention", func(t *testing.T) (string, []string) { + one, zero := 1, 0 + proxy, rt := createTestProxyWithRuntimeCfg(t, + []*config.ServerConfig{{Name: "a", Enabled: true}, {Name: "b", Enabled: true}}, + func(c *config.Config) { c.MaxConcurrentRequests = &one; c.QueueSize = &zero }) + + startCountingUpstream(t, proxy, rt, "a", readSpec("read_thing")) + sentinel := "SENTINEL_limiter_b_4a1f" + bUp := startCountingUpstream(t, proxy, rt, "b", readSpec("held")) + + started := make(chan struct{}) + release := make(chan struct{}) + var startOnce sync.Once + bUp.mcpSrv.AddTool(mcp.Tool{Name: "held", Description: "Handles " + sentinel, InputSchema: mcp.ToolInputSchema{Type: "object"}}, + func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { + startOnce.Do(func() { close(started) }) + <-release + return mcp.NewToolResultText("ok"), nil + }) + + wildcard := agentCtx([]string{"*"}, allPerms, "") + done := make(chan *mcp.CallToolResult, 1) + go func() { + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"name": "b:held", "args": map[string]interface{}{}} + res, err := proxy.handleCallToolVariant(wildcard, req, contracts.ToolVariantRead) + require.NoError(t, err) + done <- res + }() + + select { + case <-started: + case <-time.After(10 * time.Second): + close(release) + t.Fatal("the held call on b never started") + } + + // The a-only token's call to "a" must now be refused: the global + // slot is saturated and there is no queue. + aOnly := agentCtx([]string{"a"}, allPerms, "") + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"name": "a:read_thing", "args": map[string]interface{}{}} + result, err := proxy.handleCallToolVariant(aOnly, req, contracts.ToolVariantRead) + require.NoError(t, err) + require.True(t, result.IsError, "the call to 'a' must be shed while 'b' holds the only global slot") + text := resultText(t, result) + assert.Contains(t, text, "proxy-wide", "the documented outcome is the proxy-wide saturation refusal, not a scope refusal") + assert.Contains(t, text, "saturated") + + close(release) + <-done + return text, []string{sentinel} + }) +} + +// --------------------------------------------------------------------------- +// 2. Cross-server security-scan admission — real discovery with an +// established baseline on "a", then an added tool that scans clean +// without hidden "b" but has a matching near-identical peer on "b" under +// trust_mode: scan. +// --------------------------------------------------------------------------- + +// TestScopeRetainedEffect_CrossServerScanAdmission reproduces the ACTUAL +// shadowing mechanism (internal/security/detect/checks/shadowing.go +// "impersonation clone" shape), not just ordinary hidden-tool filtering. +// Cross-model review (round 1) caught that an earlier version of this test +// never created a same-name pair with near-identical descriptions and never +// asserted the pending outcome — it only proved retrieve_tools' ordinary +// scope filter, which every other differential test already covers. +// +// The real mechanism (internal/runtime/tool_quarantine.go +// checkToolApprovals, "new tool" branch, scanMode case): a NEW tool on a +// trust_mode:scan server with an already-established baseline runs +// scanChangeIsClean, which feeds the synchronous TPA scanner every OTHER +// connected server's CURRENT tools as peer context +// (collectPeerToolMetadata, sourced from the live StateView snapshot) so +// the shadowing.cross_server check can fire. That check's "impersonation +// clone" shape (shadowing.go cloneDescriptions) flags a tool whose NAME and +// DESCRIPTION both near-duplicate another server's tool of the same name — +// exactly the fixture spec.md names: "a same-name near-identical tool on +// hidden b". A non-clean verdict holds the new tool PENDING (fail closed) +// instead of auto-approving it via "scan-approved". +// +// Fixture: hidden "b" already exposes "sync_database" with a description at +// pass 1 (so it is a live StateView peer by the time "a" is rediscovered); +// "a" establishes its own unrelated baseline at pass 1, then gains its OWN +// "sync_database" tool — same name, a near-duplicate (word-order-shuffled) +// description — on pass 2, driven through the REAL discovery producer +// (up.serve + rt.RefreshServerTools, the same recipe +// TestToolGate_LegacyCollapsedRecord_* uses) so the scan gate genuinely +// runs with "b" as a live peer. +func TestScopeRetainedEffect_CrossServerScanAdmission(t *testing.T) { + runRetainedEffectScenario(t, "cross-server-scan-admission", func(t *testing.T) (string, []string) { + proxy, rt := createTestProxyWithRuntimeCfg(t, + []*config.ServerConfig{ + {Name: "a", Enabled: true, TrustMode: string(config.TrustModeScan)}, + {Name: "b", Enabled: true, TrustMode: string(config.TrustModeScan)}, + }, nil) + + sentinel := "SENTINEL_scanadmission_b_5c7a" + const sharedToolName = "sync_database" + // Both descriptions share the SAME 15-token base (only reordered), + // so cloneDescriptions' token-containment bar (shared >= 0.85 of the + // smaller set AND >= 0.7 of the larger) clears comfortably even + // after "b"'s description picks up 4 extra, non-overlapping tokens + // from the sentinel: shared=15, smaller=15, larger=19 -> 15/15=1.0, + // 15/19=0.79 — both above their respective bars. + aDescription := "Synchronize production database records to nightly encrypted cold storage backup bucket automatically without manual intervention" + bDescription := "Synchronize database records to production nightly encrypted cold storage backup bucket automatically without manual intervention — " + sentinel + + // "b" exposes the peer tool from the start — it must be a live + // StateView entry when "a"'s pass-2 scan gathers cross-server + // context. + startCountingUpstream(t, proxy, rt, "b", toolSpec{ + Name: sharedToolName, Description: bDescription, + Annotations: &config.ToolAnnotations{DestructiveHint: boolPtr(true)}, + }) + + // "a" establishes its own baseline (pass 1, no peer collision yet). + aUp := startCountingUpstream(t, proxy, rt, "a", readSpec("baseline_tool")) + + // Pass 2: "a" gains the same-name, near-duplicate-description tool, + // driven through the REAL discovery producer. rt.RefreshServerTools + // reads through the RUNTIME's own upstream manager (createTestProxyWithRuntimeCfg + // wires two — proxy's and the runtime's — exactly as + // runRuntimeDiscovery's doc comment explains), so the runtime + // manager must be connected to "a" itself before the refresh, the + // same two-manager dance TestToolGate_LegacyCollapsedRecord_* uses. + aUp.serve(toolSpec{ + Name: sharedToolName, Description: aDescription, + Annotations: &config.ToolAnnotations{DestructiveHint: boolPtr(true)}, + }) + runRuntimeDiscovery(t, proxy, rt, aUp) + + record, err := proxy.storage.GetToolApproval("a", sharedToolName) + require.NoError(t, err, "the new tool must have its own approval record after pass 2") + assert.Equal(t, storage.ToolApprovalStatusPending, record.Status, + "the shadowing check's impersonation-clone shape must hold a's new tool pending — a clean scan-approve here means the fixture's descriptions are not near-duplicate enough to trigger shadowing.cross_server") + // isToolCallable is deliberately the SEARCH-visibility filter only + // (mcp.go's isExactToolCallable checks Disabled, never pending — + // documented there as "quarantine is deliberately NOT gated here"); + // the DISPATCH gate is evaluateToolGate, which is what actually + // blocks a pending tool from executing. + assert.False(t, proxy.evaluateToolGate("a", sharedToolName).callable(), + "a pending tool must not be dispatchable — the retained effect changed a's OWN discovery/dispatch outcome, exactly as spec.md documents") + + // Ownership invariant: whatever the gate decided, an a-only token's + // discovery of "a" must never surface "b"'s content, name or + // sentinel. + ctx := agentCtx([]string{"a"}, allPerms, "") + req := mcp.CallToolRequest{} + req.Params.Arguments = map[string]interface{}{"query": "synchronize database", "limit": float64(10)} + result, err := proxy.handleRetrieveTools(ctx, req) + require.NoError(t, err) + require.False(t, result.IsError) + text := resultText(t, result) + assert.NotContains(t, text, "b:"+sharedToolName, "an a-only token must never see hidden b's tool by name") + return text, []string{sentinel} + }) +} + +// --------------------------------------------------------------------------- +// 3. Prompt-name collision and 4. global prompt cap +// (internal/upstream/manager_prompts.go maxAggregatedPrompts=1000). Both +// depend on Go's randomised map iteration order inside +// upstream.Manager.ListPrompts, so the fixtures assert the +// order-independent invariant rather than a specific winner. +// --------------------------------------------------------------------------- + +// connectPromptUpstream wires a fresh upstream.Manager (mirroring +// TestAggregatedPrompt_ScopeUsesCanonicalOwner's pattern) with one real +// streamable-HTTP upstream per server name, each serving the given prompts, +// and installs it as proxy.upstreamManager. +func connectPromptUpstream(t *testing.T, proxy *MCPProxyServer, servers map[string][]mcp.Prompt) { + t.Helper() + proxy.config.EnablePrompts = true + proxy.config.AggregateUpstreamPrompts = true + qOff := false + proxy.config.QuarantineEnabled = &qOff + + um := upstream.NewManager(zap.NewNop(), proxy.config, nil, secret.NewResolver(), nil) + t.Cleanup(func() { um.DisconnectAll() }) + for name, prompts := range servers { + mcpSrv := mcpserver.NewMCPServer(name, "1.0.0-test", mcpserver.WithPromptCapabilities(true)) + for _, p := range prompts { + text := p.Description + mcpSrv.AddPrompt(p, func(_ context.Context, _ mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + return &mcp.GetPromptResult{Messages: []mcp.PromptMessage{ + {Role: mcp.RoleAssistant, Content: mcp.TextContent{Type: "text", Text: text}}, + }}, nil + }) + } + testServer := servertest.NewTestStreamableHTTPServer(mcpSrv) + t.Cleanup(testServer.Close) + require.NoError(t, um.AddServerConfig(name, &config.ServerConfig{ + Name: name, Protocol: "streamable-http", URL: testServer.URL, Enabled: true, + })) + client, ok := um.GetClient(name) + require.True(t, ok) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + require.NoError(t, client.Connect(ctx)) + cancel() + } + proxy.upstreamManager = um + proxy.RefreshPrompts() +} + +func TestScopeRetainedEffect_PromptNameCollision(t *testing.T) { + runRetainedEffectScenario(t, "prompt-name-collision", func(t *testing.T) (string, []string) { + proxy, _ := createTestProxyWithRuntime(t, nil) + sentinel := "SENTINEL_promptcollide_9b2e" + // Two distinct (server, prompt) pairs flatten to the SAME display + // name "a__b__shared" — spec.md's own collision shape: server "a" + // with prompt "b__shared", and hidden server "a__b" with prompt + // "shared". + connectPromptUpstream(t, proxy, map[string][]mcp.Prompt{ + "a": {{Name: "b__shared", Description: "Authorized prompt"}}, + "a__b": {{Name: "shared", Description: sentinel}}, + }) + + ctx := agentCtx([]string{"a"}, allPerms, "") + registered := proxy.server.ListPrompts() + var names strings.Builder + for name := range registered { + names.WriteString(name) + names.WriteString(" ") + } + + // Ownership invariant: whichever server's prompt won the collision, + // fetching it through the REGISTERED handler (which runs the same + // authorize() check the real prompts/get RPC does — calling the raw + // getPromptAggregated helper directly would bypass that check and + // prove nothing about caller-facing disclosure) must never yield + // hidden a__b's content to this a-only token. + if entry, ok := registered["a__b__shared"]; ok { + result, err := entry.Handler(ctx, mcp.GetPromptRequest{Params: mcp.GetPromptParams{Name: "a__b__shared"}}) + if err == nil { + for _, msg := range result.Messages { + if tc, ok := msg.Content.(mcp.TextContent); ok { + assert.NotContains(t, tc.Text, sentinel) + } + } + } + } + return names.String(), []string{sentinel} + }) +} + +// TestScopeRetainedEffect_GlobalPromptCap stresses the ACTUAL 1000-prompt +// aggregate backstop (internal/upstream/manager_prompts.go +// maxAggregatedPrompts), not just its arithmetic. Cross-model review (round +// 1) caught that a single hidden server's 250 prompts never reaches the +// aggregate cap at all — maxPromptsPerServer (200) clamps it to ~200 before +// the aggregate cap is ever evaluated, so `len(registered) <= 1002` held +// trivially regardless of whether the aggregate cap worked. Fixed by using +// SIX hidden servers at 200 prompts each (1200 upstream prompts total, +// comfortably over the 1000 cap even after every one individually clears +// the per-server cap) plus one authorized prompt on "a" — the aggregate cap +// can only be exercised by CROSSING the per-server ceiling with multiple +// servers, so this shape is the only one that actually proves it. +func TestScopeRetainedEffect_GlobalPromptCap(t *testing.T) { + runRetainedEffectScenario(t, "global-prompt-cap", func(t *testing.T) (string, []string) { + proxy, _ := createTestProxyWithRuntime(t, nil) + sentinel := "SENTINEL_promptcap_7e3d" + + servers := map[string][]mcp.Prompt{ + "a": {{Name: "authorized_prompt", Description: "Authorized prompt"}}, + } + const hiddenServers = 6 + const promptsPerHiddenServer = 200 // == maxPromptsPerServer; each server alone clears the per-server cap + totalUpstreamAdvertised := 1 // "a"'s one authorized prompt + for s := 0; s < hiddenServers; s++ { + name := "hidden" + itoaPadded(s) + prompts := make([]mcp.Prompt, 0, promptsPerHiddenServer) + for i := 0; i < promptsPerHiddenServer; i++ { + prompts = append(prompts, mcp.Prompt{Name: "p" + itoaPadded(s) + "_" + promptIndexName(i), Description: sentinel}) + } + servers[name] = prompts + totalUpstreamAdvertised += promptsPerHiddenServer + } + require.Greater(t, totalUpstreamAdvertised, 1000, + "fixture: total upstream-advertised prompts must exceed maxAggregatedPrompts for this to test the aggregate cap at all") + connectPromptUpstream(t, proxy, servers) + + registered := proxy.server.ListPrompts() + const builtinPromptCount = 2 // setup-new-mcp-server, troubleshoot-mcp-server + require.LessOrEqual(t, len(registered), 1000+builtinPromptCount, + "the maxAggregatedPrompts backstop must still apply with %d upstream-advertised prompts present", totalUpstreamAdvertised) + require.Less(t, len(registered), totalUpstreamAdvertised+builtinPromptCount, + "the cap must actually have TRUNCATED something — %d advertised prompts must not all have been registered", totalUpstreamAdvertised) + + // Content check (cross-model review round 2: checking only NAMES was + // vacuous — the sentinel lives exclusively in prompt DESCRIPTIONS, + // which never appear in a bare name list regardless of correctness). + // This effect has no scoped-vs-hidden CALLER distinction the way the + // other retained effects do (the cap is a numeric ceiling applied + // before any authorization check, not a disclosure path) — the + // meaningful, non-vacuous claim is narrower: WHEN "a"'s own single + // authorized prompt survives the cap (order-dependent, like every + // other assertion in this file that depends on map iteration), an + // "a"-only-scoped token fetching it through the REGISTERED handler + // (which runs the real authorize() check, per + // TestScopeRetainedEffect_PromptNameCollision's established pattern) + // gets ITS OWN content, never a hidden server's sentinel-tagged one. + var contentText strings.Builder + const aPromptName = "a__authorized_prompt" + if entry, ok := registered[aPromptName]; ok { + aOnly := agentCtx([]string{"a"}, allPerms, "") + result, err := entry.Handler(aOnly, mcp.GetPromptRequest{Params: mcp.GetPromptParams{Name: aPromptName}}) + require.NoError(t, err, "the a-only token must be able to fetch its OWN authorized prompt when it survives the cap") + for _, msg := range result.Messages { + if tc, ok := msg.Content.(mcp.TextContent); ok { + contentText.WriteString(tc.Text) + } + } + assert.NotContains(t, contentText.String(), sentinel, + "a's own prompt content must never contain a hidden server's sentinel") + } + + var names strings.Builder + for name := range registered { + names.WriteString(name) + names.WriteString(" ") + } + return names.String() + " " + contentText.String(), []string{sentinel} + }) +} + +func promptIndexName(i int) string { + const digits = "0123456789" + if i == 0 { + return "p0" + } + var b []byte + for i > 0 { + b = append([]byte{digits[i%10]}, b...) + i /= 10 + } + return "p" + string(b) +} + +// --------------------------------------------------------------------------- +// 5. Direct display-name collision — fleet-wide collision admission on the +// direct surface (a hidden server can withhold an authorized entry). +// Already pinned, deterministically, by mcp_direct_catalog_test.go; this +// re-runs that coverage as this suite's named fixture (parity with how +// scope_differential_test.go re-registers per-PR tests by name). +// --------------------------------------------------------------------------- + +func TestScopeRetainedEffect_DirectDisplayNameCollision(t *testing.T) { + t.Run("direct-display-name-collision", func(t *testing.T) { + TestBuildDirectCatalog_WithholdsCollidingDisplayNames(t) + }) +} diff --git a/internal/server/testdata/scope_latency/README.md b/internal/server/testdata/scope_latency/README.md new file mode 100644 index 000000000..cd4ca9d03 --- /dev/null +++ b/internal/server/testdata/scope_latency/README.md @@ -0,0 +1,24 @@ +# Spec 105 PR H1 — scope latency fixtures (FR-011, T112/T112a) + +The four FR-011 operations' "frozen" fixtures: + +- **`retrieve_tools` / `tools/list`**: the existing 527-tool LiveMCPBench + snapshot at `../mcp_routing_deferred_tokens_test.go`'s + `deferredLargeCorpusPath` (shared with the Spec 102/083 fleet-scale + tests — not duplicated here). +- **`read_cache`**: a 10-page cache entry. +- **`prompts/list`**: a 50-prompt set. + +The `read_cache` and `prompts/list` fixtures are generated **deterministically +in Go code** inside `../scope_latency_test.go` (`recordID`/`itoaPadded` +helpers), not as separate serialized files in this directory. This is a +deliberate choice, not an oversight: `.github/workflows/scope-latency.yml` +(T112a) copies HEAD's `scope_latency_test.go` (and this directory, if it +holds any files) over the merge-base checkout before comparing both +revisions, so a fixture defined in the test file's own source is carried +across the comparison exactly as a serialized file under this directory +would be — with the added benefit that there is no risk of the code and a +separate data file drifting apart. Any future PR that DOES add a serialized +fixture under this directory (e.g. a captured real LiveMCPBench prompt +export) should keep it here so the same copy-across-revisions mechanism +picks it up unchanged. diff --git a/internal/upstream/manager_prompts_deadline_test.go b/internal/upstream/manager_prompts_deadline_test.go new file mode 100644 index 000000000..2b44a1bce --- /dev/null +++ b/internal/upstream/manager_prompts_deadline_test.go @@ -0,0 +1,243 @@ +package upstream + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/mark3labs/mcp-go/mcp" + mcpserver "github.com/mark3labs/mcp-go/server" + "github.com/mark3labs/mcp-go/server/servertest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Spec 105 PR H1, T108a: the "shared prompt-refresh deadline" retained +// effect (spec.md:115) — internal/server.MCPProxyServer.RefreshPrompts wraps +// the WHOLE fleet's Manager.ListPrompts call in one +// context.WithTimeout(context.Background(), 30*time.Second) (hardcoded, not +// config, not test-injectable), and ListPrompts visits every connected +// server SEQUENTIALLY against that SAME shared context — so a slow server +// can exhaust the deadline before a later one in iteration order is ever +// visited, and that later server's prompts silently drop out of the +// published set until the next refresh. +// +// Reproducing the 30-second production deadline live would mean this test +// either waits out 30 real seconds or forks RefreshPrompts to accept an +// injectable one (internal/server/scope_retained_effects_test.go's header +// comment explains why neither is worth it). This test proves the SAME +// mechanism — sequential per-server calls sharing one caller-supplied +// context — directly against Manager.ListPrompts with an artificially short +// deadline, which RefreshPrompts's real 30s call is only a longer instance +// of: ListPrompts itself takes ctx from its caller and does nothing +// 30-second-specific. +// +// Cross-model review round 1's fixture used ONE slow server + one fast one +// and only asserted the slow server's own prompts never appeared — round 2 +// correctly flagged that this does not distinguish a SHARED deadline from +// N INDEPENDENT-SEQUENTIAL per-server deadlines: either design would also +// keep the slow server's own late prompts out. Round 3 additionally noted +// that in THIS fixture's specific parameter regime (a 60ms budget far +// shorter than the 300ms slow delay), an independent-CONCURRENT design +// (every server racing against its OWN fresh 60ms, in parallel) is ALSO +// indistinguishable from shared-sequential here: neither lets any 300ms +// server finish inside a 60ms window, so both designs equally admit only +// the fast server and bound elapsed to ~60ms. This fixture therefore rules +// out independent-SEQUENTIAL (via the elapsed-time bound below: summing N +// independent 60ms budgets in turn would take ~Nx60ms, not ~60ms) but NOT +// independent-CONCURRENT — that gap is closed by the companion fixture, +// TestManager_ListPrompts_SharedBudgetCannotFitAllServersEvenWithGenerousDeadline +// below, whose 150ms-budget/100ms-delay parameters are chosen specifically +// so an independent-concurrent design (every server's own generous 150ms +// budget easily fits its 100ms of work, all racing in parallel) WOULD let +// every server succeed, while the real shared-sequential mechanism cannot. +// The two fixtures together rule out both alternative designs. +// +// Go map iteration order (m.clients) is genuinely random and not +// controllable from a test, so this fixture uses ENOUGH slow servers (four, +// each individually slower than the shared deadline) that REGARDLESS of +// which one m.clients visits first, that first slow server alone consumes +// the entire shared budget. The deterministic, order-independent invariants +// this proves: +// +// - no slow server's own prompts EVER appear (true under shared, +// independent-sequential AND independent-concurrent designs alike in +// this parameter regime — this alone is round 1's weaker claim); +// - AT MOST ONE server's prompts appear in total, and only when that one +// server (fast or one particular slow one) happened to be visited +// FIRST — an independent-SEQUENTIAL-per-server-deadline design would +// instead let EVERY slow server eventually succeed on its own fresh +// budget in turn, so seeing more than one server's prompts would prove +// that alternative rather than sharing; +// - total elapsed time stays close to ONE deadline's worth of budget +// regardless of how many slow servers are configured — four +// independent 60ms budgets applied sequentially would sum to ~240ms if +// each is exhausted in turn (the mechanism this test rules out), while +// one SHARED 60ms budget bounds the whole call to ~60ms however many +// servers are configured. +func TestManager_ListPrompts_SharedNotPerServerDeadline(t *testing.T) { + m := newTestManager(t) + + const slowServerDelay = 300 * time.Millisecond + const sharedDeadline = 60 * time.Millisecond // shorter than slowServerDelay, longer than a loopback round trip + const slowServerCount = 4 // enough that whichever one m.clients visits first, that one alone exhausts the shared budget + + slowServerNames := make(map[string]bool, slowServerCount) + for i := 0; i < slowServerCount; i++ { + id := fmt.Sprintf("slow%d", i) + name := fmt.Sprintf("slow-server-%d", i) + promptName := fmt.Sprintf("slow_prompt_%d", i) + slowServerNames[name+":"+promptName] = true + + slowUpstream := mcpserver.NewMCPServer(name, "0.0.1", + mcpserver.WithPromptCapabilities(true), + mcpserver.WithHooks(func() *mcpserver.Hooks { + h := &mcpserver.Hooks{} + h.AddBeforeListPrompts(func(ctx context.Context, _ any, _ *mcp.ListPromptsRequest) { + select { + case <-time.After(slowServerDelay): + case <-ctx.Done(): + } + }) + return h + }()), + ) + slowUpstream.AddPrompt(mcp.NewPrompt(promptName, mcp.WithPromptDescription("from a slow server")), + func(_ context.Context, _ mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + return &mcp.GetPromptResult{Messages: []mcp.PromptMessage{{Role: mcp.RoleAssistant, Content: mcp.TextContent{Type: "text", Text: "ok"}}}}, nil + }) + slowServer := servertest.NewTestStreamableHTTPServer(slowUpstream) + t.Cleanup(slowServer.Close) + require.NoError(t, m.AddServerConfig(id, &config.ServerConfig{ + Name: name, Protocol: "streamable-http", URL: slowServer.URL, Enabled: true, + })) + client, ok := m.GetClient(id) + require.True(t, ok) + connectCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + require.NoError(t, client.Connect(connectCtx)) + cancel() + } + + addConnectedTestServer(t, m, "fast", "fast-server", "fast_prompt") + const fastPromptName = "fast-server:fast_prompt" + + start := time.Now() + ctx, cancel := context.WithTimeout(context.Background(), sharedDeadline) + defer cancel() + prompts, err := m.ListPrompts(ctx) + elapsed := time.Since(start) + require.NoError(t, err, "a per-client failure (including a context-deadline error) is logged and skipped, never returned") + + names := make(map[string]bool, len(prompts)) + for _, p := range prompts { + names[p.Name] = true + } + t.Logf("shared-deadline fixture (%d slow servers): elapsed=%s prompts=%v", slowServerCount, elapsed, names) + + for name := range slowServerNames { + assert.False(t, names[name], "no slow server's own prompts may ever survive the shared deadline, got %q present", name) + } + assert.LessOrEqualf(t, len(prompts), 1, + "AT MOST ONE server's prompts may appear: with %d slow servers configured (each individually slower than the shared deadline), an INDEPENDENT-SEQUENTIAL-per-server-deadline design would let every one of them eventually succeed in turn on its own fresh budget — seeing %d servers' worth of prompts (%v) would prove that alternative, not sharing. (The independent-CONCURRENT alternative is ruled out separately by TestManager_ListPrompts_SharedBudgetCannotFitAllServersEvenWithGenerousDeadline's parameters, not this one.)", slowServerCount, len(prompts), names) + if len(prompts) == 1 { + assert.True(t, names[fastPromptName], + "the one server whose prompts survived must be the fast one — a slow server surviving would mean its own 300ms call completed inside the shared 60ms budget, which is impossible unless the budget were not actually shared/bounded") + } + // Four INDEPENDENT 60ms per-server budgets applied sequentially (the + // design this test rules out) would sum to ~240ms as each slow server + // exhausts its own allowance in turn; a SHARED 60ms budget bounds the + // WHOLE call to ~60ms regardless of how many slow servers are + // configured. The generous ceiling (2x the shared deadline, still far + // below the 4x-per-server-budget sum) absorbs scheduler/loopback jitter + // without weakening what it rules out. + assert.Less(t, elapsed, 2*sharedDeadline, + "the shared deadline must bound the WHOLE ListPrompts call to roughly ONE deadline's worth of time regardless of how many slow servers are configured — %s with %d slow servers looks like %d independent per-server budgets summing up, not one shared budget", + elapsed, slowServerCount, slowServerCount) +} + +// TestManager_ListPrompts_SharedBudgetCannotFitAllServersEvenWithGenerousDeadline +// is the SECOND discriminating regime cross-model review round 3 asked for. +// Round 2's fixture (a 60ms deadline far shorter than the 300ms slow delay) +// cannot tell a SHARED-SEQUENTIAL budget apart from N INDEPENDENT, +// CONCURRENTLY-RACED per-server deadlines of the same size: both designs +// would let only the fast server (or whichever happens to start first) +// through and bound total elapsed to ~one deadline, because EVERY slow +// server individually exceeds even its own hypothetical full budget in +// either design. +// +// This regime instead uses a deadline BETWEEN one and two slow servers' +// worth of work (150ms, with five 100ms-each slow servers, no fast server +// at all): an INDEPENDENT-CONCURRENT design would give every one of the five +// slow servers its own fresh 150ms budget, racing in parallel — 100ms of +// real work comfortably fits, so all five would succeed, in ~100ms total. +// An INDEPENDENT-SEQUENTIAL design would eventually let all five succeed +// too (each gets a fresh budget when its turn comes), just taking ~500ms +// total. Only the ACTUAL shared-sequential design — one 150ms budget spent +// as servers are visited in turn — admits at most ONE full slow server +// (100ms) before the shared clock has too little left (150-100=50ms) for a +// second 100ms server, so it can never seat more than one or two (a second +// one only if it happened to be far enough along when the deadline hit, +// which this test does not rely on): asserting "well under all five +// succeeded" rules out BOTH alternative designs at once, closing the gap +// round 3 identified in the single-regime version of this test. +func TestManager_ListPrompts_SharedBudgetCannotFitAllServersEvenWithGenerousDeadline(t *testing.T) { + m := newTestManager(t) + + const perServerDelay = 100 * time.Millisecond + const sharedDeadline = 150 * time.Millisecond // fits ONE full slow server, not two + const slowServerCount = 5 + + for i := 0; i < slowServerCount; i++ { + id := fmt.Sprintf("budget-slow%d", i) + name := fmt.Sprintf("budget-slow-server-%d", i) + promptName := fmt.Sprintf("budget_slow_prompt_%d", i) + + slowUpstream := mcpserver.NewMCPServer(name, "0.0.1", + mcpserver.WithPromptCapabilities(true), + mcpserver.WithHooks(func() *mcpserver.Hooks { + h := &mcpserver.Hooks{} + h.AddBeforeListPrompts(func(ctx context.Context, _ any, _ *mcp.ListPromptsRequest) { + select { + case <-time.After(perServerDelay): + case <-ctx.Done(): + } + }) + return h + }()), + ) + slowUpstream.AddPrompt(mcp.NewPrompt(promptName, mcp.WithPromptDescription("from a budget-test slow server")), + func(_ context.Context, _ mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + return &mcp.GetPromptResult{Messages: []mcp.PromptMessage{{Role: mcp.RoleAssistant, Content: mcp.TextContent{Type: "text", Text: "ok"}}}}, nil + }) + slowServer := servertest.NewTestStreamableHTTPServer(slowUpstream) + t.Cleanup(slowServer.Close) + require.NoError(t, m.AddServerConfig(id, &config.ServerConfig{ + Name: name, Protocol: "streamable-http", URL: slowServer.URL, Enabled: true, + })) + client, ok := m.GetClient(id) + require.True(t, ok) + connectCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + require.NoError(t, client.Connect(connectCtx)) + cancel() + } + + start := time.Now() + ctx, cancel := context.WithTimeout(context.Background(), sharedDeadline) + defer cancel() + prompts, err := m.ListPrompts(ctx) + elapsed := time.Since(start) + require.NoError(t, err) + + t.Logf("budget fixture (%d servers x %s each, shared budget %s): elapsed=%s succeeded=%d/%d", + slowServerCount, perServerDelay, sharedDeadline, elapsed, len(prompts), slowServerCount) + + assert.LessOrEqualf(t, len(prompts), 2, + "a %s shared budget must not fit more than ~1 of %d servers each needing %s: got %d succeeded — both an independent-CONCURRENT design (every server gets its own fresh %s, all fit) and an independent-SEQUENTIAL design (every server eventually gets a fresh %s) would let ALL %d succeed, which this must rule out", + sharedDeadline, slowServerCount, perServerDelay, len(prompts), sharedDeadline, sharedDeadline, slowServerCount) + assert.Less(t, elapsed, time.Duration(slowServerCount)*perServerDelay, + "elapsed %s must be far below the %s an independent-sequential design would take to eventually seat all %d servers", + elapsed, time.Duration(slowServerCount)*perServerDelay, slowServerCount) +} diff --git a/roadmap.yaml b/roadmap.yaml index d9bae7d96..fa7d68267 100644 --- a/roadmap.yaml +++ b/roadmap.yaml @@ -441,8 +441,9 @@ epics: depends_on: [] - id: scope-refusal-shapes title: "FR-010: scope-first refusal precedence; dispatch denials and 'available servers' never name hidden servers; describe_tool not-found and alias resolution computed over the authorized corpus" - status: todo + status: done priority: P1 + pr: "#1328" depends_on: [scope-retrieve-tools] - id: scope-selectable-profile-predicate title: "FR-003/FR-004 remainder: selectable-profile predicate for UNPINNED tokens on /mcp/p/, /mcp/p, /mcp/p/ and set_profile; identical status+body across missing / deleted / not-selectable / pin-mismatch / no-profiles (#1225 + #1227 follow-up lists)" diff --git a/specs/105-agent-scope-hardening/tasks.md b/specs/105-agent-scope-hardening/tasks.md index d4fa88745..5ae50a7a1 100644 --- a/specs/105-agent-scope-hardening/tasks.md +++ b/specs/105-agent-scope-hardening/tasks.md @@ -271,18 +271,24 @@ **Goal**: the two-fixture differential oracle, HTTP credential matrix and latency test exist, cover every user-story scenario by id, and prove no pinned-reversal test survived. **Independent test**: `TestScopeCoverage_EveryUserStoryScenario` passes only when US1.1–1.8, US2.1–2.6, US3.1–3.4 are all registered. -- [ ] T107 [US1] `newScopeFixture(t, full)`, `runScopeScenario(t, usID, fn)`, `normalizeScopeResponse` (nondeterministic fields only), per-fixture retrieve-oracle derivation (`Search(Size=docCount)` filtered then cut) asserted for every SC-001-excluded field, coverage registry + `TestScopeCoverage_EveryUserStoryScenario` per contracts/differential-oracle.md — `internal/server/scope_differential_test.go` (new) -- [ ] T108 [US1] Register every US1.x scenario (retrieve oracle incl. `include_stats+debug+session_risk`, describe, call_tool refusal, read_cache, set_profile, profile URL, tail_log, stored scripts, prompts) against both fixtures — `internal/server/scope_differential_test.go` -- [ ] T108a [P] [US1] Retained-effect mode `runRetainedEffectScenario` + one named fixture per SC-001 exclusion (`spec.md:114,161`): shared-limiter contention (global capacity 1, no queue, held call on hidden `b`, call to `a` → "proxy-wide limit saturated"), cross-server scan admission (real discovery, baseline on `a`, same-name near-identical tool on hidden `b` under `trust_mode: scan` → `a`'s addition pending via shadowing), prompt-name collision, global prompt cap, direct display-name collision, shared prompt-refresh deadline, shared log rotation/retention — each asserting authorized ownership of everything returned, no hidden content/sentinel, and the documented outcome — `internal/server/scope_retained_effects_test.go` (new) -- [ ] T109 [P] [US2] Register US2.1–2.6 (retrieve 54-cell + direct + nested tables from T009, nested envelope through the real runtime, unresolved identity for every caller) — `internal/server/scope_differential_test.go` -- [ ] T110 [P] [US3] Register US3.1–3.4 (origin flip, unparseable names, tier change, refusal text) reusing `newSkewFixture` — `internal/server/scope_differential_test.go` -- [ ] T111 [US1] HTTP credential matrix: `mintAgentToken` (T035) × surfaces `{/mcp, /mcp/all, /mcp/code, /mcp/call, /mcp/p/, aliases}` × operations; `yes` cells run the differential runner, `n/a` cells assert `-32602` unregistered; no `E2E` in the name; one env per fixture — `internal/server/scope_http_matrix_test.go` (new) -- [ ] T112 [P] [US1] Finalise `scope_latency_test.go` (T078) over `retrieve_tools`, `read_cache` (frozen 10-page entry), `prompts/list` (frozen 50-prompt set), `tools/list`: 20 warm-up + 200 timed per caller, `p95(scoped) − p95(admin) ≤ 20ms` per operation, `-race` skip — `internal/server/scope_latency_test.go` + frozen fixtures under `internal/server/testdata/scope_latency/` -- [ ] T112a [P] [US1] Merge-base gate: `.github/workflows/scope-latency.yml` (non-race, reference runner, on PRs touching `internal/server`/`internal/index`) checks out merge-base into a second directory, copies head's `scope_latency_test.go` + `testdata/scope_latency/` over it (test-only, backward compatible: the test compiles against the pre-feature API), runs both in one job, fails on administrator p95 regression > max(10%, 5 ms) per operation AND fails if either side yields no measurement for any operation; comparison in Go, no new dependency (D10) -- [ ] T113 [US1] SC-007 inventory table (US id → test name → `-run` pattern) in the file header of `internal/server/scope_differential_test.go`; `grep` guard test that none of the four pinned-reversal assertions (gap-map §7) still exist in their original form -- [ ] T114 [P] [US1] Docs: final invariant wording + retained-effects list reconciled with what shipped in `docs/features/agent-tokens.md` -- [ ] T115 Common verification across all touched packages; roadmap: flip the eight `scope-*` tasks to `done` with PR numbers in `roadmap.yaml:430-469` and regenerate `ROADMAP.md` (`python3 scripts/gen-roadmap.py`); `python3 scripts/gen-roadmap.py --check-github` -- [~] T116 Astra rounds on FR-011/013/014 + FR01x-G4…G7; quote final `VERDICT:` +- [x] T107 [US1] `newScopeFixture(t, full)`, `runScopeScenario(t, usID, fn)`, `normalizeScopeResponse` (nondeterministic fields only), per-fixture retrieve-oracle derivation (`Search(Size=docCount)` filtered then cut) asserted for every SC-001-excluded field, coverage registry + `TestScopeCoverage_EveryUserStoryScenario` per contracts/differential-oracle.md — `internal/server/scope_differential_test.go` (new) +- [x] T108 [US1] Register every US1.x scenario (retrieve oracle incl. `include_stats+debug+session_risk`, describe, call_tool refusal, read_cache, set_profile, profile URL, tail_log, stored scripts, prompts) against both fixtures — `internal/server/scope_differential_test.go` +- [x] T108a [P] [US1] Retained-effect mode `runRetainedEffectScenario` + one named fixture per SC-001 exclusion (`spec.md:114,161`): shared-limiter contention (global capacity 1, no queue, held call on hidden `b`, call to `a` → "proxy-wide limit saturated"), cross-server scan admission (real discovery — `startCountingUpstream` baseline pass then `up.serve`+`runRuntimeDiscovery` pass 2 — same-name, near-duplicate-description tool on hidden `b` under `trust_mode: scan` → `a`'s addition genuinely held PENDING via the shadowing.cross_server impersonation-clone check; verified non-vacuous by swapping in a dissimilar description and confirming the test goes red), prompt-name collision, global prompt cap (six hidden servers × 200 prompts each, so the fixture actually crosses the 1000-prompt aggregate ceiling rather than being clamped by the 200-per-server cap first), direct display-name collision, shared prompt-refresh deadline (named fixture at `internal/upstream/manager_prompts_deadline_test.go` — `TestManager_ListPrompts_SharedNotPerServerDeadline`, an artificially-short-deadline reproduction of the same sequential-shared-context mechanism the 30s production path uses, since the 30s deadline itself is hardcoded and not test-injectable), shared log rotation/retention (cited, cross-package) — each asserting authorized ownership of everything returned, no hidden content/sentinel, and the documented outcome — `internal/server/scope_retained_effects_test.go` (new). Cross-model review round 1 caught that the first versions of the cross-server-scan-admission and global-prompt-cap fixtures were vacuous (never created the actual shadowing condition; never crossed the aggregate cap) and that the prompt-refresh-deadline effect had only a comment, not a fixture — all three rebuilt for real. Round 2 caught two more: the deadline fixture (then one slow + one fast server) didn't distinguish a SHARED deadline from N independent per-server deadlines — rebuilt with four slow servers (enough that whichever is visited first alone exhausts the shared budget) asserting at-most-one-server's-prompts-survive and total elapsed stays near ONE deadline regardless of server count, never ~4x; and global-prompt-cap's sentinel check inspected prompt NAMES while the sentinel lives only in DESCRIPTIONS (vacuous) — fixed to fetch "a"'s own prompt content through its registered handler (the real `authorize()` path) when it survives the cap and check the sentinel is absent from the served text. Round 3: the single-regime deadline fixture (60ms budget vs 300ms delay) still could not distinguish shared-sequential from N independent-but-CONCURRENTLY-raced per-server deadlines of the same size (both designs bound elapsed to ~one deadline and admit only the fast server in that parameter regime) — added a SECOND, complementary fixture, `TestManager_ListPrompts_SharedBudgetCannotFitAllServersEvenWithGenerousDeadline`: five 100ms-delay servers under a single 150ms budget (fits ~1 server, not all 5) — an independent-concurrent design would let all 5 succeed in parallel (~100ms), an independent-sequential design would eventually seat all 5 (~500ms); only shared-sequential seats ~1. Verified non-vacuous (widened to 2s, got 5/5, reverted) and non-flaky (10x `-count=10`, consistently 1/5, ~151ms). Also strengthened `TestScopeHTTPMatrix_RetrieveToolsNonDisclosureOverRealHTTP` (new): a raw wire-text substring check over real HTTP proving hidden deploy-srv's tool identifiers never leak into a scoped token's retrieve_tools response (query deliberately chosen to match deploy-srv's content), with an unrelated positive-control query proving the same HTTP path returns real authorized content. +- [x] T109 [P] [US2] Register US2.1–2.6 (retrieve 54-cell + direct + nested tables from T009, nested envelope through the real runtime, unresolved identity for every caller) — `internal/server/scope_differential_test.go` +- [x] T110 [P] [US3] Register US3.1–3.4 (origin flip, unparseable names, tier change, refusal text) reusing `newSkewFixture` — `internal/server/scope_differential_test.go` +- [x] T111 [US1] HTTP credential matrix: real tokens minted via `mintProfileAgentToken` (profile_integration_test.go, T035 — `mintAgentToken` was already taken by an unrelated Spec 107 helper of a different signature, so H1 reuses the actual generalised minter under its real name) × surfaces `{/mcp, /mcp/all, /mcp/code, /mcp/call, /mcp/p/}` × operations; `yes` cells run through real HTTP with session continuity, `n/a` cells assert the actual JSON-RPC-level `-32602 tool ... not found` envelope (not any application-level `isError:true`, which a registered tool's own handler can also return for bad arguments — round-1 finding, fixed) against a definitely-nonexistent-name positive control; no `E2E` in the name; one env per fixture — `internal/server/scope_http_matrix_test.go` (new). Round 2: expanded to cover every FR-014 row at least once — prompts list/get on all three surfaces, describe_tool (yes on retrieve+direct, n/a confirmed on code), tail_log via the explicit operation (not just upstream_servers' default `list`), and the three call_tool_* execution-tier variants on retrieve. This expansion's tail_log cell FOUND A REAL BUG: `internal/logs.ReadUpstreamServerLogTail`/`ReadUpstreamServerLogTailAttributed` dereferenced a nil `*config.LogConfig` (a caller building `config.Config{}` directly rather than through `config.DefaultConfig()`, which always sets `Logging`) — mcp-go's panic recovery turned it into a confusing `-32603` instead of crashing, but it was a genuine defect affecting BOTH admin and scoped readers identically (not a scope/security issue, a robustness gap). Fixed with a TDD red/green pair (`internal/logs/nil_log_config_test.go`) plus the minimal nil-guard in both functions. Remaining scope note: still not every operation × surface × alias cell exhaustively (stored-script resolution and nested code-execution dispatch are proven in-process by mcp_code_scripts_test.go / scope_target_tier_matrix_test.go's nested table, not re-driven over HTTP here) — the FR-005/008/010 differential fixtures in scope_differential_test.go prove the underlying non-disclosure behavior in-process; this file's job is proving the SAME auth/session mechanics hold over real HTTP for every row at least once. +- [x] T112 [P] [US1] Finalise `scope_latency_test.go` (T078) over `retrieve_tools`, `read_cache` (frozen 10-page entry), `prompts/list` (frozen 50-prompt set), `tools/list`: 20 warm-up + 200 timed per caller, `p95(scoped) − p95(admin) ≤ 20ms` per operation, `-race` skip — `internal/server/scope_latency_test.go` + `internal/server/testdata/scope_latency/README.md` (the fixtures are generated deterministically in the test file itself, not as separate serialized files — see the README) +- [x] T112a [P] [US1] Merge-base gate: `.github/workflows/scope-latency.yml` (non-race, `ubuntu-latest` — no dedicated "reference runner" available to this repo) checks out merge-base into a second directory, copies head's `scope_latency_test.go` + `testdata/scope_latency/` over it (test-only, backward compatible: the test compiles against the pre-feature API — `scope_latency_test.go` was made deliberately self-contained, with its OWN local `connectLatencyPromptUpstream` helper, after round-1 review caught that an earlier version called `connectPromptUpstream` from `scope_retained_effects_test.go`, a file that does not exist at merge-base and so would have failed the copied file's compile), runs both in one job, fails on administrator p95 regression > max(10%, 5 ms) per operation AND fails if either side yields no measurement for any operation; comparison via new `cmd/scope-latency-compare` (Go, stdlib only, no new dependency, D10; unit-tested). `continue-on-error: true` — not a required check yet, per the CLAUDE.md flakiness lesson from PRs F/G. The full merge-base mechanism (archive checkout, file copy, build, run, produce results) was verified locally end-to-end against this PR's actual merge-base commit (75cfc27b7) before relying on the CI job to prove it. +- [x] T113 [US1] SC-007 inventory table (US id → test name → `-run` pattern) in the file header of `internal/server/scope_differential_test.go`; `grep` guard test (`TestScopePinnedReversalsStayInverted`, `internal/server/scope_pinned_reversal_guard_test.go`) that none of the four pinned-reversal assertions gap-map.md's FR01x-G7 entry names still exist in their original form +- [x] T114 [P] [US1] Docs: final invariant wording + retained-effects list reconciled with what shipped in `docs/features/agent-tokens.md` +- [x] T115 Common verification across all touched packages; roadmap: `scope-refusal-shapes` was still `status: todo` despite PR G (#1328) having merged — flipped to `done` with its PR number; the other seven `scope-*` tasks were already `done`; `scope-regression-suite` (this PR) intentionally left `status: todo` until merge; regenerated `ROADMAP.md` (`python3 scripts/gen-roadmap.py`); `python3 scripts/gen-roadmap.py --check-github` clean +- [x] T116 Cross-model review on FR-011/013/014 + FR01x-G4…G7 (opencode terra/sol/astra confirmed quota-exhausted live before round 1; fell back to `codex exec --model gpt-5.6-sol --sandbox read-only`, stdin closed, `gtimeout`-wrapped, brief split into a file the CLI reads itself). 4 rounds, all genuine findings verified against production/test code before fixing (no false-positive acceptances): + - Round 1 (7 MUST-FIX/SHOULD-FIX across 8 review areas): US1.5's retrieve-oracle scenario was vacuous (no hidden-influencing usage data, byte-compared ranking fields the contract excludes) — fixed with real usage seeding + `stripRankingDependentFields`; the pinned-reversal guard was trivially bypassed — hardened with string-concat collapsing, whitespace-tolerant regex, and a structural (not text-label) table-row parse; `CrossServerScanAdmission` and `GlobalPromptCap` retained-effect fixtures were vacuous (never created the real shadowing condition; never crossed the aggregate cap) — rebuilt against the actual `shadowing.cross_server` impersonation-clone mechanism and a 6-hidden-server-×-200-prompt shape that genuinely exceeds `maxAggregatedPrompts`; the HTTP matrix's `unregisteredToolIsNA` accepted any application-level `isError` as proof of non-registration — fixed to require the actual `-32602` envelope; the `tools/list` latency fixture measured an empty direct catalog — fixed to publish the real 527-tool corpus through connected upstreams; the CI workflow's merge-base checkout couldn't compile the copied test file — fixed by making `scope_latency_test.go` fully self-contained; docs overclaimed "every FR has a three-server differential-oracle test" — reworded. Every fix verified non-vacuous by temporarily reverting the underlying guard/logic and confirming the test goes red (git diff clean afterward in every case). + - Round 2 (2 MUST-FIX, 1 SHOULD-FIX): the shared-deadline fixture (1 slow + 1 fast server) didn't distinguish shared from independent-per-server deadlines — rebuilt with 4 slow servers; the HTTP matrix's new rows only proved tool *registration*, not FR-014's matrix — expanded to cover prompts/describe_tool/tail_log/execution-tier explicitly; this expansion's tail_log cell **found and fixed a genuine pre-existing nil-pointer panic** in `internal/logs.ReadUpstreamServerLogTail(Attributed)` (nil `*config.LogConfig` dereference, affecting admin and scoped readers identically — a robustness gap, not a scope bug), fixed with a TDD red→green pair; `GlobalPromptCap`'s sentinel check inspected prompt names while the sentinel lives only in descriptions — fixed to check served content through the real registered handler. + - Round 3 (2 MUST-FIX, 1 NIT): the round-2 deadline fixture (60ms budget vs 300ms delay) still couldn't distinguish shared-sequential from independent-CONCURRENT per-server deadlines in that parameter regime — added a second, complementary fixture (5×100ms servers, one 150ms shared budget) whose parameters specifically defeat that alternative; the HTTP matrix still lacked real content-level non-disclosure proof over HTTP — added a raw-wire-text substring check on `retrieve_tools` with a query deliberately matching hidden content, plus an unrelated positive control; gofmt NIT fixed. + - Round 4: both prior MUST-FIXes confirmed resolved ("the complementary 150ms/100ms fixture distinguishes shared-sequential behavior from both independent-concurrent and independent-sequential deadlines"; "the real-HTTP retrieve_tools test provides content-level non-disclosure through authentication middleware... further per-row content assertions have insufficient marginal value"). One NIT (a stale comment in the deadline test file, written before round 3's second fixture existed, overclaiming what the first fixture alone rules out) — fixed. + - **`VERDICT: clean`** (round 4, quoted in full): *"Both prior MUST-FIX findings are resolved: The complementary 150 ms/100 ms fixture distinguishes shared-sequential behavior from both independent-concurrent and independent-sequential deadlines. The real-HTTP retrieve_tools test provides content-level non-disclosure through authentication middleware, with an authorized positive control. Combined with the differential suite and existing direct/profile HTTP checks, further per-row content assertions have insufficient marginal value."* + - Full `-race` suite (internal/server, internal/upstream, internal/cache, internal/logs, cmd/scope-latency-compare) green after every round's fixes; one pre-existing, unrelated flaky allocation-count test (`TestProfileMiddleware_RefusalWorkIndependentOfFleet`, a PR-D test this PR does not touch) failed once under concurrent-session load on the shared build host and passed cleanly in isolation immediately after — not attributable to this PR. `go build`/`go vet` (bare + `-tags server`) clean throughout; `golangci-lint --new-from-rev=origin/main` (bare + `--build-tags server`) 0 issues at every checkpoint; `gofmt -l` clean on every touched/new file. ---