docs(spec): add spec 057 for in-proxy profiles (#55)#531
Conversation
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 Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Resolves spec-number collision with PR #531 (in-proxy profiles), which keeps 057.
Dumbris
left a comment
There was a problem hiding this comment.
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:
- 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.
/mcpsemantics → always full union, purely additive. State as a settled invariant, not a fork. No breaking change for existing/mcpclients; profiles are opt-in narrowing via the new URL only. (Astrictopt-in can be a later spec if anyone asks.)- 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, andcallare already bound routing-mode subpaths under/mcp/(Spec 031);pis 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:
-
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 viaGetMCPServerForMode. That can't be mirrored for N hot-reloadable profile slugs becausehttp.ServeMuxcan'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 howmcpAuthMiddlewareinjectsAuthContext— reusing the existing MCP server instance. Middleware order: auth → profile. -
The filter must run independently of auth type (correctness-critical). The existing server-scope filter in
retrieve_tools(mcp.go~1108) andcall_tool_*(mcp.go~1491) is gated onenforceAgentScope := authCtx != nil && !authCtx.IsAdmin(). But a default/mcp/p/...connection has no token and is assignedAdminContext(), for which that gate isfalseandCanAccessServerreturnstrueunconditionally. So profile filtering cannot ride the agent-scope gate or be implemented by stuffing the profile's servers intoAuthContext.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, theagent_tokensbucket, 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." -
Config store + reload. Top-level
profilesarray in the config file, hot-reloaded.Profiles []ProfileConfig \json:"profiles,omitempty"`` → byte-identical round-trip when absent (SC-004 for free). -
Files touched (scope guard).
internal/config(+ newprofiles.govalidation), newinternal/profile/context.go(~30 lines, mirrorsauth/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-serverenabled_tools/disabled_toolsneed 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.
|
Critic (Codex) review — Ylsssq926's PR #531 |
|
Critic (Codex) review — Ylsssq926's PR #531 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 |
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):
Out of scope (deferred, with reasons in the spec):
Open Questions for review
Three calls I deliberately left for you to make in the spec (rather than baking them in):
Refs: #55, #333