diff --git a/CHANGELOG.md b/CHANGELOG.md index eef36d88..706d7bb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## [Unreleased] +### Phase 8 — MCP bridge: preserve API path prefix on relative URLs (AI-049) (2026-06-16) + +- **Fix.** The MCP bridge dropped the API **path prefix** (e.g. `/api`) on every request when `TEXTSTACK_API_URL` carried one (`https://textstack.app/api`). `TextStackApiClient` built relative URLs with a LEADING slash (`/search`, `/me/highlights/{id}`, `/books/{id}/ask`) and `HttpClient.BaseAddress` had NO trailing slash, so .NET resolved the leading-slash relative URI from the HOST ROOT and dropped `/api` — the bridge hit the SPA (`https://textstack.app/search`, 301 → HTML → `JsonException` → "upstream unavailable") instead of the API. Silently green in unit tests (fake handler ignores `BaseAddress`) and in prod-internal (`http://api:8080`, no prefix); confirmed live against prod. Fixed two ways: `McpBridgeCore.BaseUri(...)` now trailing-slash-normalizes the base (idempotent — no double slash), and `TextStackApiClient` + `DeviceFlowTokenProvider` build relative URIs leading-slash-stripped (`auth/device/code`, `auth/device/token`, `auth/refresh-mobile`), so the prefix segment survives instead of being treated as a replaceable file. +- **Regression test** (`tests/TextStack.UnitTests/McpApiPrefixTests.cs`, CI-safe, no network). Wires the REAL `TextStackApiClient` through `ServiceCollection().AddHttpClient<>(BaseUri("http://localhost/api"))` + a capturing primary handler (exactly as `McpBridgeCore.AddApiClient` does) and pins the ABSOLUTE resolved `request.RequestUri` for a public tool (`search_books` → `http://localhost/api/search?q=…`), a user-scoped GET (`list_my_highlights` → `…/api/me/highlights/{id}`), a user-scoped POST (`ask_book` → `…/api/books/{id}/ask`), and the device-flow `/auth/device/code` URL — all KEEPING `/api`. Plus focused `BaseUri` helper asserts (trailing-slash append, idempotent, no-prefix host). These FAIL on the old leading-slash / no-trailing-slash code (prefix dropped to `http://localhost/…`) and PASS now. No `ITool` added (StudyBuddy set-equality stays green). + ### Phase 8 — MCP remote HTTP (streamable) transport (AI-049) (2026-06-16) A REMOTE HTTP transport for the MCP server so clients connect over HTTP behind the existing nginx + Cloudflare tunnel (no new cloud), as a new localhost-only Docker service. **stdio behavior is byte-identical** — the http transport is additive. Critical design point: remote HTTP is **multi-user** — each connection authenticates via its OWN `Authorization: Bearer` header (the AI-050 device-flow JWT pasted into the client config), NOT a server-side token cache. diff --git a/backend/src/Ai/TextStack.Ai.Mcp/Auth/DeviceFlowTokenProvider.cs b/backend/src/Ai/TextStack.Ai.Mcp/Auth/DeviceFlowTokenProvider.cs index 9e1816cb..65ee8d80 100644 --- a/backend/src/Ai/TextStack.Ai.Mcp/Auth/DeviceFlowTokenProvider.cs +++ b/backend/src/Ai/TextStack.Ai.Mcp/Auth/DeviceFlowTokenProvider.cs @@ -180,7 +180,7 @@ private async Task StartOrJoinDeviceFlowAsync(CancellationToken ct) // callers. Throws on failure (caller clears the slot to make it retryable). private async Task RequestDeviceCodeAsync() { - using var request = new HttpRequestMessage(HttpMethod.Post, "/auth/device/code"); + using var request = new HttpRequestMessage(HttpMethod.Post, "auth/device/code"); using var response = await _http.SendAsync(request, CancellationToken.None); if (!response.IsSuccessStatusCode) throw new InvalidOperationException("could not start TextStack device authorization"); @@ -243,7 +243,7 @@ private async Task PollLoopAsync(DeviceCodeResponse code) private async Task PollOnceAsync(string deviceCode) { - using var request = new HttpRequestMessage(HttpMethod.Post, "/auth/device/token") + using var request = new HttpRequestMessage(HttpMethod.Post, "auth/device/token") { Content = JsonContent.Create(new DeviceTokenRequestBody( "urn:ietf:params:oauth:grant-type:device_code", deviceCode)), @@ -282,7 +282,7 @@ private async Task PollOnceAsync(string deviceCode) { try { - using var request = new HttpRequestMessage(HttpMethod.Post, "/auth/refresh-mobile") + using var request = new HttpRequestMessage(HttpMethod.Post, "auth/refresh-mobile") { Content = JsonContent.Create(new RefreshRequestBody(refreshToken)), }; diff --git a/backend/src/Ai/TextStack.Ai.Mcp/Http/TextStackApiClient.cs b/backend/src/Ai/TextStack.Ai.Mcp/Http/TextStackApiClient.cs index 5e9fa9b5..d226895b 100644 --- a/backend/src/Ai/TextStack.Ai.Mcp/Http/TextStackApiClient.cs +++ b/backend/src/Ai/TextStack.Ai.Mcp/Http/TextStackApiClient.cs @@ -218,11 +218,17 @@ public async Task GetVocabularyAsync( // Public route: Host header only (site + EN default-language resolution). private HttpRequestMessage PublicRequest(HttpMethod method, string url) { - var request = new HttpRequestMessage(method, url); + var request = new HttpRequestMessage(method, Relative(url)); request.Headers.Host = _siteHost; return request; } + // Build a relative URI that PRESERVES any path prefix on BaseAddress (e.g. + // ".../api/"). A leading slash would make HttpClient resolve from the host + // root and drop the prefix, so strip it; BaseAddress is normalized to end + // with "/" at registration so the prefix segment isn't treated as a file. + private static Uri Relative(string url) => new(url.TrimStart('/'), UriKind.Relative); + // User-scoped route: Host header + Bearer. Asks the token provider; a // non-Authorized result throws McpUnauthorizedException (carrying the // verification URL/code for Pending, or the message for Failed) up front so @@ -234,7 +240,7 @@ private async Task AuthorizedRequestAsync(HttpMethod method, switch (token) { case TokenResult.Authorized a: - var request = new HttpRequestMessage(method, url); + var request = new HttpRequestMessage(method, Relative(url)); request.Headers.Host = _siteHost; request.Headers.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue("Bearer", a.AccessToken); diff --git a/backend/src/Ai/TextStack.Ai.Mcp/McpBridgeCore.cs b/backend/src/Ai/TextStack.Ai.Mcp/McpBridgeCore.cs index 45e2ab6e..1aeb5e43 100644 --- a/backend/src/Ai/TextStack.Ai.Mcp/McpBridgeCore.cs +++ b/backend/src/Ai/TextStack.Ai.Mcp/McpBridgeCore.cs @@ -29,10 +29,19 @@ internal static class McpBridgeCore public static void AddApiClient(IServiceCollection services, McpBridgeOptions options) => services.AddHttpClient(http => { - http.BaseAddress = new Uri(options.ApiBaseUrl, UriKind.Absolute); + http.BaseAddress = BaseUri(options.ApiBaseUrl); http.Timeout = TimeSpan.FromSeconds(McpTimeoutSeconds()); }); + /// + /// Absolute base URI with a guaranteed trailing slash so a path prefix + /// (e.g. https://textstack.app/api) is kept when relative request + /// URIs are resolved — without it, the last segment ("api") is treated as a + /// file and replaced, dropping the prefix. + /// + public static Uri BaseUri(string apiBaseUrl) => + new(apiBaseUrl.EndsWith('/') ? apiBaseUrl : apiBaseUrl + "/", UriKind.Absolute); + /// /// The shared MCP server handlers. Both hosts register the SAME pair; identity /// (and lifetime) is supplied by whatever the diff --git a/backend/src/Ai/TextStack.Ai.Mcp/McpHosts.cs b/backend/src/Ai/TextStack.Ai.Mcp/McpHosts.cs index 08ab731b..41def564 100644 --- a/backend/src/Ai/TextStack.Ai.Mcp/McpHosts.cs +++ b/backend/src/Ai/TextStack.Ai.Mcp/McpHosts.cs @@ -68,7 +68,7 @@ private static void AddTokenProviderStdio(IServiceCollection services, McpBridge const string deviceFlowClient = "device-flow"; services.AddHttpClient(deviceFlowClient, http => { - http.BaseAddress = new Uri(options.ApiBaseUrl, UriKind.Absolute); + http.BaseAddress = McpBridgeCore.BaseUri(options.ApiBaseUrl); http.DefaultRequestHeaders.Host = options.SiteHost; // Env-overridable (TEXTSTACK_MCP_TIMEOUT_SECONDS, default 15) — same helper // the typed TextStackApiClient uses, so the auth client never drifts. diff --git a/tests/TextStack.UnitTests/McpApiPrefixTests.cs b/tests/TextStack.UnitTests/McpApiPrefixTests.cs new file mode 100644 index 00000000..d7d1a8db --- /dev/null +++ b/tests/TextStack.UnitTests/McpApiPrefixTests.cs @@ -0,0 +1,208 @@ +using System.Net; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using TextStack.Ai.Mcp; +using TextStack.Ai.Mcp.Auth; +using TextStack.Ai.Mcp.Http; +using TextStack.Ai.Mcp.Tools; + +namespace TextStack.UnitTests; + +/// +/// AI-049 regression: the bridge dropped the API PATH PREFIX (e.g. /api) when +/// TEXTSTACK_API_URL carried one. Relative request URLs were built with a +/// LEADING slash (/search, /me/highlights/{id}, ...) and +/// had NO trailing slash — so .NET resolved the +/// leading-slash relative URI from the HOST ROOT and dropped /api, hitting the +/// SPA (301 → HTML → JsonException → "upstream unavailable") instead of the API. +/// +/// The fix: guarantees a single trailing slash and +/// 's relative URIs are leading-slash-stripped. These +/// tests pin PREFIX PRESERVATION on the ABSOLUTE resolved request.RequestUri — +/// they FAIL on the old leading-slash / no-trailing-slash code and PASS now. +/// +/// Existing MCP tests use a prefix-LESS BaseAddress (https://api.example/), so +/// the bug never manifested there; this file is the missing prefixed-base coverage. +/// Introduces NO ITool (StudyBuddy set-equality stays green). +/// +public class McpApiPrefixTests +{ + private const string SiteHost = "textstack.test"; + private const string Prefix = "http://localhost/api"; + private const string Edition = "33333333-3333-3333-3333-333333333333"; + + // Captures the LAST request as the real HttpClient saw it (RequestUri is absolute + // once sent through a client with a BaseAddress). + private sealed class CapturingHandler(HttpResponseMessage response) : HttpMessageHandler + { + public HttpRequestMessage? LastRequest { get; private set; } + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + LastRequest = request; + return Task.FromResult(response); + } + } + + private static HttpResponseMessage Json(string body, HttpStatusCode status = HttpStatusCode.OK) => + new(status) { Content = new StringContent(body, System.Text.Encoding.UTF8, "application/json") }; + + private static System.Text.Json.JsonElement Args(string json) => + System.Text.Json.JsonDocument.Parse(json).RootElement; + + // Builds the REAL TextStackApiClient exactly as McpBridgeCore.AddApiClient does — + // through a ServiceCollection().AddHttpClient(...) with BaseUri(prefix) and the + // capturing primary handler — so the BaseAddress normalization + DI typed-client + // path are exercised end-to-end (not a hand-rolled HttpClient). + private static (McpToolCatalog catalog, CapturingHandler handler) BuildPrefixedCatalog( + HttpResponseMessage response, string? token = "tok-123") + { + var handler = new CapturingHandler(response); + var options = new McpBridgeOptions { ApiBaseUrl = Prefix, SiteHost = SiteHost, McpToken = token }; + + var services = new ServiceCollection(); + services.AddSingleton(options); + services.AddSingleton(new StaticEnvTokenProvider(options)); + services + .AddHttpClient(http => http.BaseAddress = McpBridgeCore.BaseUri(options.ApiBaseUrl)) + .ConfigurePrimaryHttpMessageHandler(() => handler); + + var provider = services.BuildServiceProvider(); + var api = provider.GetRequiredService(); + return (new McpToolCatalog(api), handler); + } + + // ── BaseUri helper: trailing-slash normalization (no double slash) ──────────── + + [Fact] + public void BaseUri_PrefixWithoutTrailingSlash_AppendsSingleSlash() + { + Assert.Equal("http://x/api/", McpBridgeCore.BaseUri("http://x/api").ToString()); + } + + [Fact] + public void BaseUri_PrefixWithTrailingSlash_StaysSingleSlash() + { + // Idempotent — must NOT produce a double trailing slash. + Assert.Equal("http://x/api/", McpBridgeCore.BaseUri("http://x/api/").ToString()); + } + + [Fact] + public void BaseUri_NoPrefixHost_StillTrailingSlash() + { + Assert.Equal("http://x/", McpBridgeCore.BaseUri("http://x").ToString()); + } + + // ── BaseUri + relative client URI ⇒ absolute URL KEEPS the prefix ───────────── + + [Theory] + [InlineData("search?q=alice", "http://localhost/api/search?q=alice")] + [InlineData("me/highlights/abc", "http://localhost/api/me/highlights/abc")] + public void BaseUri_CombinedWithRelative_PreservesPrefix(string relative, string expected) + { + // Mirrors how HttpClient resolves request.RequestUri against BaseAddress: the + // relative URI has NO leading slash (as the client now builds it), so the + // prefix segment survives instead of being treated as a replaceable file. + var resolved = new Uri(McpBridgeCore.BaseUri(Prefix), new Uri(relative, UriKind.Relative)); + + Assert.Equal(expected, resolved.ToString()); + } + + // ── end-to-end: public tool (search_books) keeps /api on the wire ───────────── + + [Fact] + public async Task SearchBooks_PrefixedBaseUrl_KeepsApiPrefix_OnAbsoluteRequestUri() + { + var (catalog, handler) = BuildPrefixedCatalog(Json("""{"total":0,"items":[]}""")); + + var result = await catalog.CallAsync("search_books", Args("""{"query":"alice"}"""), CancellationToken.None); + + Assert.NotEqual(true, result.IsError); + var uri = handler.LastRequest!.RequestUri!; + Assert.True(uri.IsAbsoluteUri); // resolved through the client's BaseAddress + // The /api prefix MUST survive. Old code produced http://localhost/search?... . + Assert.Equal("http://localhost/api/search?q=alice", uri.AbsoluteUri); + Assert.Equal(SiteHost, handler.LastRequest.Headers.Host); + } + + // ── end-to-end: user-scoped tool (list_my_highlights) keeps /api on the wire ── + + [Fact] + public async Task ListMyHighlights_PrefixedBaseUrl_KeepsApiPrefix_OnAbsoluteRequestUri() + { + var (catalog, handler) = BuildPrefixedCatalog(Json("[]")); + + var result = await catalog.CallAsync( + "list_my_highlights", Args($$"""{"editionId":"{{Edition}}"}"""), CancellationToken.None); + + Assert.NotEqual(true, result.IsError); + var uri = handler.LastRequest!.RequestUri!; + Assert.True(uri.IsAbsoluteUri); + // Old code → http://localhost/me/highlights/{id} (prefix dropped, 301→HTML). + Assert.Equal($"http://localhost/api/me/highlights/{Edition}", uri.AbsoluteUri); + Assert.Equal("Bearer tok-123", handler.LastRequest.Headers.Authorization?.ToString()); + } + + // ── end-to-end: POST user-scoped tool (ask_book) keeps /api on the wire ─────── + + [Fact] + public async Task AskBook_PrefixedBaseUrl_KeepsApiPrefix_OnAbsoluteRequestUri() + { + var (catalog, handler) = BuildPrefixedCatalog( + Json("""{"answer":"x","citations":[],"lastReadOrd":1,"insufficient":false}""")); + + var result = await catalog.CallAsync( + "ask_book", + Args($$"""{"editionId":"{{Edition}}","question":"what happens?"}"""), + CancellationToken.None); + + Assert.NotEqual(true, result.IsError); + var uri = handler.LastRequest!.RequestUri!; + Assert.True(uri.IsAbsoluteUri); + Assert.Equal($"http://localhost/api/books/{Edition}/ask", uri.AbsoluteUri); + } + + // ── device-flow URLs are fixed the same way (leading slash dropped) ─────────── + + // Answers /auth/device/code with a valid device code; records the request URI. + private sealed class DeviceCodeHandler : HttpMessageHandler + { + private readonly TaskCompletionSource _captured = new(TaskCreationOptions.RunContinuationsAsynchronously); + public Uri? FirstRequestUri { get; private set; } + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + FirstRequestUri ??= request.RequestUri; + _captured.TrySetResult(); + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent( + """{"device_code":"DC","user_code":"UC","verification_uri":"https://x/device","verification_uri_complete":"https://x/device?code=UC","expires_in":600,"interval":600}""", + System.Text.Encoding.UTF8, "application/json"), + }); + } + + public Task WaitForFirstRequestAsync() => _captured.Task; + } + + [Fact] + public async Task DeviceFlow_PrefixedBaseUrl_KeepsApiPrefix_OnDeviceCodeUrl() + { + var handler = new DeviceCodeHandler(); + // Built exactly as McpHosts wires the device-flow named client: BaseUri(prefix). + var http = new HttpClient(handler) { BaseAddress = McpBridgeCore.BaseUri(Prefix) }; + var cache = new TokenCache(Path.Combine(Path.GetTempPath(), "ts-mcp-" + Guid.NewGuid().ToString("N") + ".json")); + var provider = new DeviceFlowTokenProvider( + http, cache, NullLogger.Instance, + time: null, minPollInterval: TimeSpan.FromMilliseconds(5)); + + // Cold call → starts a device flow (the /auth/device/code POST), returns Pending. + var token = await provider.GetTokenAsync(CancellationToken.None); + Assert.IsType(token); + + await handler.WaitForFirstRequestAsync(); + + // Old code → http://localhost/auth/device/code (prefix dropped). Fixed: kept. + Assert.Equal("http://localhost/api/auth/device/code", handler.FirstRequestUri!.AbsoluteUri); + } +}