Skip to content

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

Merged
Dumbris merged 2 commits into
smart-mcp-proxy:mainfrom
Ylsssq926:docs/spec-057-in-proxy-profiles
Jun 7, 2026
Merged

docs(spec): add spec 057 for in-proxy profiles (#55)#531
Dumbris merged 2 commits 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

Related smart-mcp-proxy#55

Incorporates review feedback from @Dumbris:

- Fix factual error: /mcp/all is already a live bound endpoint
  (Spec 031), not a "possible future" reservation
- Add Implementation Design section: routing via middleware+context,
  profile filtering as independent parallel check (not gated on
  enforceAgentScope), config store design, files-touched scope guard
- Convert Open Questions → Resolved Design Decisions (all three
  settled: warn-and-skip, /mcp always full union, reserve all)
- Tighten FR-011: profile slug stored in ActivityRecord.Metadata
  as metadata["profile"], matching Specs 018/026 pattern
- Add mandatory regression test for unauthenticated profile filtering
@Dumbris
Copy link
Copy Markdown
Member

Dumbris commented Jun 7, 2026

Review — accept & merge (spec-only)

Verified the load-bearing technical claims against the codebase and they hold:

  • enforceAgentScope := authCtx != nil && !authCtx.IsAdmin() (internal/server/mcp.go:1113), gate at :1142 / call-path at :1529.
  • Routing modes register a separate server instance per static mux.Handle (server.go:1670-1694), so the spec's claim that this can't be mirrored for hot-reloadable profiles (ServeMux can't de-register routes) is correct — middleware+context is the right seam.
  • The key insight is right and important: an unauthenticated /mcp/p/ connection gets AdminContext(), where enforceAgentScope is false and CanAccessServer returns true unconditionally. So profile filtering must be a parallel check, not ride the agent-scope gate. The mandatory named regression test for this is the right call.

On-vision: directly answers #55, composes by intersection with agent tokens (028) and per-user visibility, zero migration, /mcp untouched. Good scope discipline deferring active-profile state.

Two product reconciliations to settle before the implementation PR (not blockers for this spec doc):

  1. URL scheme — spec ships /mcp/p/<slug>; roadmap/blog promise /mcp/<profile>. The spec's choice is the correct engineering call (avoids the /mcp/all|code|call collision); marketing copy should be updated to match.
  2. No UI in the MVP — profiles are config-file-only; the launch narrative features a tray switcher that's explicitly out-of-scope. Recommend a Web UI "Profiles" page as the immediate fast-follow (CRUD over profiles[], copyable /mcp/p/<slug> URL) — it needs no active-state decision and is what sells "zero config."

Minor: confirm the per-user-visibility reference number (spec says 029; multiuser work is tracked as 024 elsewhere). Merging the spec.

@Dumbris Dumbris merged commit 41fe212 into smart-mcp-proxy:main Jun 7, 2026
27 checks passed
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