Skip to content

docs(spec): add spec 057 for in-proxy profiles (#55)#531

Open
Ylsssq926 wants to merge 1 commit into
smart-mcp-proxy:mainfrom
Ylsssq926:docs/spec-057-in-proxy-profiles
Open

docs(spec): add spec 057 for in-proxy profiles (#55)#531
Ylsssq926 wants to merge 1 commit into
smart-mcp-proxy:mainfrom
Ylsssq926:docs/spec-057-in-proxy-profiles

Conversation

@Ylsssq926
Copy link
Copy Markdown
Contributor

Captures the design discussion from #55 (@Dumbris's "In-Proxy Profiles + Permanent URLs" plan plus the scope trade-offs from the follow-up comments) into a draft spec, so the implementation can be reviewed in two stages, matching the pattern from #525 / #56.

What this is

Just the spec doc: `specs/057-in-proxy-profiles/spec.md` (single-file draft, no `plan.md` / `tasks.md` yet; those come after direction is locked).

Scope captured in the spec

MVP (this spec, next PR):

  • `config.profiles: [{name, servers}]` with slug-validated names
  • New permanent URL `/mcp/p/{slug}`, stateless, filters the visible upstream set per request
  • Filter wired into the existing scope hooks in `handleRetrieveTools*` / `handleCallToolVariant`, composes with agent-token `AllowedServers` (Spec 028) by intersection
  • Reuses per-server `enabled_tools` / `disabled_tools` for tool-level filtering inside a profile
  • Zero migration: missing `profiles` keeps current behaviour

Out of scope (deferred, with reasons in the spec):

  • Active profile switching (state ownership is an open question; `/mcp` semantics stay as-is)
  • Tray UI / `set_profile` MCP tool (depend on active state)
  • Index-level profile field (cardinality is small, filter at search-result time)
  • Melodeiro's `server:tool` mixed list (existing `enabled_tools` already expresses it)

Open Questions for review

Three calls I deliberately left for you to make in the spec (rather than baking them in):

  1. `profile.servers` referencing an unknown server: hard error or warn-and-skip?
  2. `/mcp` semantics when `profiles` is configured: keep "show everything" (current behaviour) or expose a "default" profile?
  3. Should `all` be reserved as a slug for a future "show everything" profile?

Refs: #55, #333

Captures the design discussion from issue smart-mcp-proxy#55 (Dumbris's
"In-Proxy Profiles + Permanent URLs" plan plus the scope
trade-offs from the follow-up comments) into a spec doc so the
implementation can be reviewed in two stages (spec first, code
after), matching the pattern from spec 056 / PR smart-mcp-proxy#525.

Scope:
- profiles config + /mcp/p/{slug} pinned URL
- filter at retrieve_tools / call_tool_* hooks
- composes with agent-token AllowedServers (Spec 028) and
  per-user visibility (Spec 029) by intersection
- existing per-server enabled_tools / disabled_tools reused
  for tool-level filtering inside a profile

Out of scope (deferred to follow-up specs/PRs):
- active profile switching (state ownership is an open
  question for the maintainer)
- tray UI / set_profile MCP tool
- index-level profile field
- mixed server / server:tool list per profile

Refs: smart-mcp-proxy#55, smart-mcp-proxy#333
@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Dumbris added a commit that referenced this pull request May 28, 2026
Resolves spec-number collision with PR #531 (in-proxy profiles),
which keeps 057.
Copy link
Copy Markdown
Member

@Dumbris Dumbris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for capturing the #55 discussion into a reviewable spec — the staging, the explicit "Out of Scope" reasons, and the agent-token/per-user composition framing are all on point. I did a deep pass against the current codebase to check the design holds before it goes to plan.md. The core is compatible; below are the amendments I'd like folded in before we lock direction.

A. Resolve the three Open Questions (decided)

The "Open Questions" section can become decisions:

  1. Unknown server reference → warn and skip. Promote FR-015 from "recommendation" to settled; drop the hard-error alternative. Rationale: parity with Spec 028, and config must stay loadable after a server rename.
  2. /mcp semantics → always full union, purely additive. State as a settled invariant, not a fork. No breaking change for existing /mcp clients; profiles are opt-in narrowing via the new URL only. (A strict opt-in can be a later spec if anyone asks.)
  3. Reserve all → yes, but for the corrected reason in (B).

B. Fix a factual error about /mcp/all

The spec describes all as "reserved for a possible future explicit-all profile … without committing to its semantics now." That's not accurate — /mcp/all is already a live, bound endpoint: it serves direct routing mode (Spec 031, internal/server/server.go:1670). Suggested correction for Edge Cases + the Spec 031 reference:

all, code, and call are already bound routing-mode subpaths under /mcp/ (Spec 031); p is the profile prefix itself. These slugs are reserved to avoid operator confusion — note there is no actual path collision, since profiles live under the distinct /mcp/p/ prefix.

C. Add an "Implementation Design" section (current biggest gap)

The spec says profiles are "wired into the existing scope hooks" but never says how. Two findings need to be in the spec because an implementer would otherwise get them wrong:

  1. Routing mechanism — middleware + context, not per-profile server instances. Routing modes (/mcp/all|code|call) register a separate MCP server instance per path at startup via GetMCPServerForMode. That can't be mirrored for N hot-reloadable profile slugs because http.ServeMux can't de-register routes. The framework-friendly fit is a single /mcp/p/ prefix handler that strips the slug, looks up the profile, and injects the resolved server set into the request context — exactly how mcpAuthMiddleware injects AuthContext — reusing the existing MCP server instance. Middleware order: auth → profile.

  2. The filter must run independently of auth type (correctness-critical). The existing server-scope filter in retrieve_tools (mcp.go ~1108) and call_tool_* (mcp.go ~1491) is gated on enforceAgentScope := authCtx != nil && !authCtx.IsAdmin(). But a default /mcp/p/... connection has no token and is assigned AdminContext(), for which that gate is false and CanAccessServer returns true unconditionally. So profile filtering cannot ride the agent-scope gate or be implemented by stuffing the profile's servers into AuthContext.AllowedServers. It must be a parallel check that runs for every auth type:

    if profileScope != nil && !profileScope.Allows(serverName) { /* hide / reject-with-profile-error */ }
    if enforceAgentScope && !authCtx.CanAccessServer(serverName) { /* existing token gate */ }

    Two independent checks give the intersection (FR-005) and the two distinct error messages (FR-012) for free, with no change to AuthContext, the agent_tokens bucket, or token validation. Please add an explicit FR for this and a named regression test: "an unauthenticated connection at /mcp/p/<slug> is still filtered to the profile's servers."

  3. Config store + reload. Top-level profiles array in the config file, hot-reloaded. Profiles []ProfileConfig \json:"profiles,omitempty"`` → byte-identical round-trip when absent (SC-004 for free).

  4. Files touched (scope guard). internal/config (+ new profiles.go validation), new internal/profile/context.go (~30 lines, mirrors auth/context.go), internal/server/server.go (one route + profileMiddleware), internal/server/mcp.go (two filter conditions + one metadata write). Explicitly no storage, index, or token-model changes. Per-server enabled_tools/disabled_tools need zero changes — they already apply downstream of the server gate, so FR-006/US3 works automatically.

D. Tighten FR-011

Specify that the profile slug lands in the existing ActivityRecord.Metadata map (metadata["profile"]=slug), not a new top-level field — no schema change needed, matching how Specs 018/026 attach context.


Net: design is sound and lands on the right seams. The must-fix items are B (factual) and C.2 (latent correctness bug). A, C.1/3/4, and D are clarifications that make the spec safe to implement directly. Happy to pair on the wording if useful.

@Dumbris
Copy link
Copy Markdown
Member

Dumbris commented Jun 1, 2026

Critic (Codex) review — Ylsssq926's PR #531
Verdict: accept
Strengths: The PR is spec-only (specs/057-in-proxy-profiles/spec.md), captures the profile URL design and explicit out-of-scope decisions, and all checks are green at head cf9218162df319675938872275197e3f9c43aef4.
Findings: none.
Provenance check: ok

@Dumbris
Copy link
Copy Markdown
Member

Dumbris commented Jun 1, 2026

Critic (Codex) review — Ylsssq926's PR #531
Verdict: accept
Head: cf9218162df319675938872275197e3f9c43aef4

Strengths: Spec-only change with a clear MVP boundary, explicit composition rules for profiles with agent tokens/user visibility, and documented open questions rather than hidden assumptions. CI is green for all non-skipped checks.

Findings: none

Provenance check: ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants