Repository navigation
fix(mobile): normalize root-path remote MCP URLs - #8329
Conversation
normalizeRemoteMcpUrl documented that a trailing slash on the path is dropped so two spellings of one endpoint are one server, but it skipped the strip when the pathname was the root. `https://host` and `https://host/` therefore stayed two servers, and the same split appeared with a query (`https://host?a=1` vs `https://host/?a=1`), so the remote server store accepted the same endpoint twice. Strip trailing slashes from the endpoint unconditionally; the replace is a no-op when there is none. Query handling is unchanged.
iscekic
added
drain
|
bot: End-to-end on the Android emulator, local dev stack, local fake account.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe only change since the previous review renames the OAuth-store test case to match its assertions, resolving the outstanding suggestion; the endpoint normalization and OAuth matching code are unchanged and remain correct. Files Reviewed (1 file)
Previous Review Summaries (2 snapshots, latest commit a9f4d0f)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a9f4d0f)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe OAuth endpoint-matching change resolves the prior warning about saved credentials; the only new finding is a test name that contradicts its own assertion. Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
The previously reported OAuth-mismatch warning is resolved by Previous review (commit d75ff93)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryChanging the normalizer also re-normalizes already-persisted URLs on load, which invalidates saved OAuth state and silently drops duplicate root-slash rows for existing installs. Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Reviewed by deepseek-v4.1-flash-1017 · Input: 31.6K · Output: 1.2K · Cached: 138K Review guidance: REVIEW.md from base branch |
The server store normalizes every persisted URL when it loads, and it now drops the trailing slash of a root path. A state entry written before that change carries the older spelling, so an exact string match stopped finding it and the person was asked to sign in again for a server whose endpoint had not changed. Compare the saved URL and the server URL through the same normalizer. Entries for other endpoints still read nothing, so a token stays on the server it was issued for, and a server with no parseable URL compares as written.
The case checks that a field written for the normalized URL lands in the entry an older spelling saved, which rewrites the entry's URL; the old name said the older spelling was kept.
What
normalizeRemoteMcpUrlinapps/mobile/src/lib/chat/remote-mcp-url.tspromises that a trailing slash on the path is dropped so two spellings of one endpoint are one server, but it skipped the strip whenurl.pathname === '/'. A root endpoint kept its slash, soremote-mcp-store.tstreatedhttps://hostandhttps://host/as two servers, and the same split happened with a query (https://host?a=1vshttps://host/?a=1).The endpoint now always has its trailing slashes removed; the replace is a no-op when there are none. Query handling, error messages, and the HTTPS/localhost policy are unchanged. The doc comment states the rule directly instead of describing it as mirroring the extension's copy, which keeps its own normalizer.
Persisted bytes are not rewritten. The server store already normalizes a stored URL when it loads, so a row saved as
https://host/now reads back ashttps://host, and two rows that differ only by the root slash load as one server, as rows differing by a path slash already did. To keep a server that is already signed in from asking for a sign-in again,remote-mcp-oauth-store.tsnow compares its saved URL with the server URL through the same normalizer: an entry written with the older spelling still matches its endpoint, and an entry written for another endpoint still reads nothing.Tests
apps/mobile/src/lib/chat/remote-mcp-url.test.ts: root-path cases — bare host vs host plus slash, repeated slashes, localhost with a port, and both spellings with a query.apps/mobile/src/lib/chat/remote-mcp-oauth-store.test.ts: a state entry saved under the older root spelling is read for the normalized URL, survives a later field write, and a different endpoint still reads nothing.apps/mobile/src/lib/chat/remote-mcp-store.test.ts: two persisted rows naming one root endpoint load as one server with its credentials.Before and after:
expected 'https://remote.example/' to be 'https://remote.example'andexpected 'https://remote.example/?base=/' to be 'https://remote.example?base=/'.Tests 2 failed | 4 passed).src/lib/chatandsrc/components/chatpure suites pass 592/592.End-to-end on Android
Local dev stack and the dev-client build on the emulator, signed in with a local fake account:
http://localhost:8815/. The saved row readshttp://localhost:8815: the trailing slash was dropped, where before the change the row kept it.http://localhost:8815/) left the add sheet open and added no row, so both spellings reach the one stored server.http://localhost:8815/ctl) closed the sheet and added its row, so the refusal above is endpoint equality rather than a stuck Save.The OAuth matching change is covered by the unit tests above; no local OAuth MCP server was stood up, so it has no in-app capture.
Screenshots are attached in a follow-up comment.
Proxy answers
The owner's proxy gave these answers in the owner's place.
Answer: Mobile only
Confidence: 0.55 · Precedent: Product scope: "explorer loop is strictly for the mobile app" (session 55c9c80332a7). Git: separate concern in its own PR (session debabfb38c82). The extension may be an end-of-life surface, which outweighs fixing the class everywhere. Reword the doc comment to state the rule itself.
Answer: Leave stored data untouched
Confidence: 0.70 · Precedent: Speed and least work: "do the least amount of work possible, with no drop in quality" (session 7f3470e0ba6f). Rewriting persisted user data is outside the work item.