From cca1b81eadbd92c17f9028f7704d5b1c93d88a34 Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 12:03:48 +0200 Subject: [PATCH 01/13] fix(main): wire COMMS_BASE_URL through MCP entry point + URL output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously `src/main.ts` read `COMMS_API_KEY` from the environment but ignored `COMMS_BASE_URL`, so the MCP entry point always constructed `CommsApi` pointing at prod. A staging-issued token registered against the MCP would 403 against prod, while the same token worked through `scripts/run-tool.ts` (which did pass baseUrl). Hard to debug, easy to mis-attribute to Claude MCP setup. ## Wiring fix - Extract `buildServerOptions()` into src/utils/server-options.ts so the env reading is unit-testable; main.ts auto-runs at import, which is what hid this regression. - Log `Comms MCP targeting ` to stderr on startup so future prod-vs-staging debugging is one log line, not a guessing game. - Normalize empty `COMMS_BASE_URL=` to `undefined` so it doesn't override the SDK default with garbage. ## URL host rewriting The SDK's URL helpers and entity `.url` fields hardcode the prod host, so a staging-targeted server would still return prod links in tool output. Wrap them in src/utils/url-helpers.ts: - `configureBaseUrl(baseUrl)` is called once from `getMcpServer` (and scripts/run-tool.ts) and sets module-level state. - `getFullCommsURL`, `getCommentURL`, `getMessageURL` re-export wrapped versions of the SDK helpers; switching ~10 tool imports from the SDK to `../utils/url-helpers.js` is the entire churn. - `rewriteToConfiguredHost(url)` rewrites entity `.url` fields populated by the SDK (fetch-inbox, load-thread, load-conversation). - `toRelativeCommsURL(url)` replaces a fragile `.replace('https://comms.todoist.com', '')` in build-link with a regex that strips any host. ## Docs - README Claude Code section: replaced `-e KEY=VALUE` (resolves the secret into argv → visible in `ps`) with a wrapper-script pattern that sources a chmod-600 env file at spawn time. Matches Doist's Secrets Management standard. - Added a Claude Code staging example using the same wrapper. ## Validated - npm run type-check — clean - npm test — 206 / 20 suites (5 new unit tests for url-helpers, 4 for server-options) - Local doistbot — 2 rounds; round 2 findings on URL field rewriting, shared config call site, type dedup, missing test coverage, and the argv-secret pattern are all applied here. ## Out of scope - Upstream fix in `@doist/comms-sdk` so its helpers and entity schemas accept a runtime baseUrl. Until then the wrapper does string substitution against the hardcoded prod host. --- README.md | 48 ++++++++++- scripts/run-tool.ts | 4 +- src/main.ts | 14 +-- src/mcp-server.ts | 9 +- src/tools/build-link.ts | 11 ++- src/tools/create-thread.ts | 2 +- src/tools/fetch-inbox.ts | 21 ++--- src/tools/get-mentions.ts | 3 +- src/tools/load-conversation.ts | 29 ++++--- src/tools/load-thread.ts | 32 +++---- src/tools/react.ts | 2 +- src/tools/reply.ts | 3 +- src/tools/search-content.ts | 3 +- src/tools/update-object.ts | 3 +- src/utils/__tests__/server-options.test.ts | 33 ++++++++ src/utils/__tests__/url-helpers.test.ts | 99 ++++++++++++++++++++++ src/utils/server-options.ts | 18 ++++ src/utils/url-helpers.ts | 40 ++++++++- 18 files changed, 311 insertions(+), 63 deletions(-) create mode 100644 src/utils/__tests__/server-options.test.ts create mode 100644 src/utils/__tests__/url-helpers.test.ts create mode 100644 src/utils/server-options.ts diff --git a/README.md b/README.md index 492eb8c..3ecc3d2 100644 --- a/README.md +++ b/README.md @@ -79,9 +79,30 @@ Add to `claude_desktop_config.json`: #### Claude Code (CLI) +Don't pass the token via `-e KEY=VAL` — even via shell expansion, the +resolved value lands in `claude mcp add`'s argv and shows up in `ps` +output. Use a wrapper script that loads a `chmod 600` env file at +runtime: + ```bash -claude mcp add comms npx @doist/comms-mcp -export COMMS_API_KEY=your-comms-api-key-here +# 1. Token in a private env file (never in shell history or argv). +install -m 600 /dev/null ~/.config/comms-mcp.env +$EDITOR ~/.config/comms-mcp.env +# Contents: COMMS_API_KEY=... + +# 2. Wrapper script reads it at spawn time. +mkdir -p ~/.local/bin +cat > ~/.local/bin/comms-mcp <<'EOF' +#!/bin/sh +set -a +. ~/.config/comms-mcp.env +set +a +exec npx -y @doist/comms-mcp +EOF +chmod +x ~/.local/bin/comms-mcp + +# 3. Register the wrapper — no -e flags, no secrets in argv. +claude mcp add comms -s user -- ~/.local/bin/comms-mcp ``` #### Visual Studio Code @@ -106,15 +127,34 @@ export COMMS_API_KEY=your-comms-api-key-here ### Targeting a non-production deployment By default the server talks to `https://comms.todoist.com`. To point at -staging or a custom deployment, also set `COMMS_BASE_URL`: +staging or a custom deployment, also set `COMMS_BASE_URL`. The staging +token and the prod token are different — a staging token will 403 +against prod and vice versa. + +For Claude Desktop / Cursor / VS Code: ```json "env": { - "COMMS_API_KEY": "your-comms-api-key-here", + "COMMS_API_KEY": "your-staging-api-key-here", "COMMS_BASE_URL": "https://comms.staging.todoist.com" } ``` +For Claude Code: extend the wrapper script from the Claude Code setup +above to also set `COMMS_BASE_URL`: + +```sh +#!/bin/sh +set -a +. ~/.config/comms-mcp.env # contains COMMS_API_KEY=... +COMMS_BASE_URL=https://comms.staging.todoist.com +set +a +exec npx -y @doist/comms-mcp +``` + +The server logs `Comms MCP targeting ` to stderr on startup so +you can confirm at a glance which environment it's hitting. + ### Getting a Comms API key Generate a personal API token from the Comms app console, then export diff --git a/scripts/run-tool.ts b/scripts/run-tool.ts index fb902a0..a5749ca 100644 --- a/scripts/run-tool.ts +++ b/scripts/run-tool.ts @@ -34,6 +34,7 @@ import { reply } from '../src/tools/reply.js' import { searchContent } from '../src/tools/search-content.js' import { updateObject } from '../src/tools/update-object.js' import { userInfo } from '../src/tools/user-info.js' +import { configureBaseUrl } from '../src/utils/url-helpers.js' // Define a minimal type for tool execution that works with any tool type ExecutableTool = { @@ -143,7 +144,8 @@ async function main() { process.exit(1) } - const baseUrl = process.env.COMMS_BASE_URL + const baseUrl = process.env.COMMS_BASE_URL || undefined + configureBaseUrl(baseUrl) const client = new CommsApi(apiKey, { baseUrl }) console.log(`Running ${toolName} with args:`) diff --git a/src/main.ts b/src/main.ts index 3ad5426..25c4e77 100644 --- a/src/main.ts +++ b/src/main.ts @@ -2,19 +2,19 @@ import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js' import dotenv from 'dotenv' import { getMcpServer } from './mcp-server.js' +import { buildServerOptions } from './utils/server-options.js' function main() { - const commsApiKey = process.env.COMMS_API_KEY - if (!commsApiKey) { - throw new Error('COMMS_API_KEY is not set') - } - - const server = getMcpServer({ commsApiKey }) + const options = buildServerOptions() + // stderr (stdout is reserved for the MCP protocol). Surfacing the + // target up-front turns "why am I getting 403s" debugging into one + // log line — staging vs prod tokens aren't cross-compatible. + console.error(`Comms MCP targeting ${options.baseUrl ?? 'https://comms.todoist.com (default)'}`) + const server = getMcpServer(options) const transport = new StdioServerTransport() server .connect(transport) .then(() => { - // We use console.error because standard I/O is being used for the MCP server communication. console.error('Server started') }) .catch((error) => { diff --git a/src/mcp-server.ts b/src/mcp-server.ts index 35031c5..772861d 100644 --- a/src/mcp-server.ts +++ b/src/mcp-server.ts @@ -18,6 +18,8 @@ import { reply } from './tools/reply.js' import { searchContent } from './tools/search-content.js' import { updateObject } from './tools/update-object.js' import { userInfo } from './tools/user-info.js' +import type { ServerOptions } from './utils/server-options.js' +import { configureBaseUrl } from './utils/url-helpers.js' const instructions = ` ## Comms Communication Tools @@ -58,7 +60,12 @@ Always provide clear context and maintain professional communication standards. * @param baseUrl - Optional base URL for the Comms API. * @returns the MCP server. */ -function getMcpServer({ commsApiKey, baseUrl }: { commsApiKey: string; baseUrl?: string }) { +function getMcpServer({ commsApiKey, baseUrl }: ServerOptions) { + // Set up host rewriting for SDK + helper URLs before any tool runs. + // Idempotent and applies to every consumer of getMcpServer (CLI, + // importable-tools, future entry points). + configureBaseUrl(baseUrl) + const server = new McpServer( { name: 'comms-mcp-server', version: '0.1.0' }, { diff --git a/src/tools/build-link.ts b/src/tools/build-link.ts index 7be68e2..5a210cf 100644 --- a/src/tools/build-link.ts +++ b/src/tools/build-link.ts @@ -1,9 +1,14 @@ -import { getCommentURL, getFullCommsURL, getMessageURL } from '@doist/comms-sdk' import { z } from 'zod' import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' import { BuildLinkOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' +import { + getCommentURL, + getFullCommsURL, + getMessageURL, + toRelativeCommsURL, +} from '../utils/url-helpers.js' const ArgsSchema = { workspaceId: z.number().describe('The workspace ID.'), @@ -71,7 +76,7 @@ const buildLink = { const params = { workspaceId, conversationId } url = fullUrl ? getFullCommsURL(params) - : getFullCommsURL(params).replace('https://comms.todoist.com', '') + : toRelativeCommsURL(getFullCommsURL(params)) } } else if (threadId !== undefined) { if (commentId !== undefined) { @@ -90,7 +95,7 @@ const buildLink = { : { workspaceId, threadId } url = fullUrl ? getFullCommsURL(params) - : getFullCommsURL(params).replace('https://comms.todoist.com', '') + : toRelativeCommsURL(getFullCommsURL(params)) } } else { throw new Error('Must provide either conversationId OR threadId to build a link') diff --git a/src/tools/create-thread.ts b/src/tools/create-thread.ts index 3ecf882..a7e952f 100644 --- a/src/tools/create-thread.ts +++ b/src/tools/create-thread.ts @@ -1,9 +1,9 @@ -import { getFullCommsURL } from '@doist/comms-sdk' import { z } from 'zod' import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' import { type CreateThreadOutput, CreateThreadOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' +import { getFullCommsURL } from '../utils/url-helpers.js' const ArgsSchema = { channelId: z.string().describe('The ID of the channel to create the thread in.'), diff --git a/src/tools/fetch-inbox.ts b/src/tools/fetch-inbox.ts index 12c5bf2..ac07587 100644 --- a/src/tools/fetch-inbox.ts +++ b/src/tools/fetch-inbox.ts @@ -5,13 +5,14 @@ import type { UnreadConversation, WorkspaceUser, } from '@doist/comms-sdk' -import { ARCHIVE_FILTER_VALUES, getFullCommsURL } from '@doist/comms-sdk' +import { ARCHIVE_FILTER_VALUES } from '@doist/comms-sdk' import { z } from 'zod' import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' import { limitedAll } from '../utils/concurrency.js' import { FetchInboxOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' +import { getFullCommsURL, rewriteToConfiguredHost } from '../utils/url-helpers.js' const ArgsSchema = { workspaceId: z.number().describe('The workspace ID to fetch inbox for.'), @@ -282,9 +283,9 @@ const fetchInbox = { isUnread: t.isUnread, isArchived: t.isArchived, isStarred: Boolean(t.isSaved), - threadUrl: - t.url ?? - getFullCommsURL({ workspaceId, channelId: t.channelId, threadId: t.id }), + threadUrl: t.url + ? rewriteToConfiguredHost(t.url) + : getFullCommsURL({ workspaceId, channelId: t.channelId, threadId: t.id }), })), conversations: conversationsWithDetails.map((cd) => { const { conversation, participants } = cd @@ -298,12 +299,12 @@ const fetchInbox = { userIds: conversation.userIds, participantNames, isUnread: cd.isUnread, - conversationUrl: - conversation.url ?? - getFullCommsURL({ - workspaceId: conversation.workspaceId, - conversationId: conversation.id, - }), + conversationUrl: conversation.url + ? rewriteToConfiguredHost(conversation.url) + : getFullCommsURL({ + workspaceId: conversation.workspaceId, + conversationId: conversation.id, + }), } }), unreadCount: unreadThreads.length, diff --git a/src/tools/get-mentions.ts b/src/tools/get-mentions.ts index 928ef94..93bd559 100644 --- a/src/tools/get-mentions.ts +++ b/src/tools/get-mentions.ts @@ -1,9 +1,10 @@ -import { type SearchResultType, getFullCommsURL } from '@doist/comms-sdk' +import type { SearchResultType } from '@doist/comms-sdk' import { z } from 'zod' import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' import { GetMentionsOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' +import { getFullCommsURL } from '../utils/url-helpers.js' const ArgsSchema = { workspaceId: z.number().describe('The workspace ID to search in.'), diff --git a/src/tools/load-conversation.ts b/src/tools/load-conversation.ts index 22e797c..f52abba 100644 --- a/src/tools/load-conversation.ts +++ b/src/tools/load-conversation.ts @@ -1,9 +1,10 @@ -import { getFullCommsURL, type WorkspaceUser } from '@doist/comms-sdk' +import type { WorkspaceUser } from '@doist/comms-sdk' import { z } from 'zod' import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' import { LoadConversationOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' +import { getFullCommsURL, rewriteToConfiguredHost } from '../utils/url-helpers.js' const ArgsSchema = { conversationId: z.string().describe('The conversation ID to load.'), @@ -130,12 +131,12 @@ const loadConversation = { userIds: includeParticipants ? conversation.userIds : [], archived: conversation.archived, lastActive: conversation.lastActive.toISOString(), - conversationUrl: - conversation.url ?? - getFullCommsURL({ - workspaceId: conversation.workspaceId, - conversationId: conversation.id, - }), + conversationUrl: conversation.url + ? rewriteToConfiguredHost(conversation.url) + : getFullCommsURL({ + workspaceId: conversation.workspaceId, + conversationId: conversation.id, + }), }, messages: messages.map((m) => ({ id: m.id, @@ -144,13 +145,13 @@ const loadConversation = { creatorName: userInfo[m.creator]?.fullName, conversationId: m.conversationId, posted: m.posted.toISOString(), - messageUrl: - m.url ?? - getFullCommsURL({ - workspaceId: m.workspaceId, - conversationId: m.conversationId, - messageId: m.id, - }), + messageUrl: m.url + ? rewriteToConfiguredHost(m.url) + : getFullCommsURL({ + workspaceId: m.workspaceId, + conversationId: m.conversationId, + messageId: m.id, + }), })), totalMessages: conversation.messageCount ?? 0, } diff --git a/src/tools/load-thread.ts b/src/tools/load-thread.ts index af16e3f..ffc1e2c 100644 --- a/src/tools/load-thread.ts +++ b/src/tools/load-thread.ts @@ -1,9 +1,9 @@ -import { getFullCommsURL } from '@doist/comms-sdk' import { z } from 'zod' import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' import { LoadThreadOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' +import { getFullCommsURL, rewriteToConfiguredHost } from '../utils/url-helpers.js' const ArgsSchema = { threadId: z.string().describe('The thread ID to load.'), @@ -172,13 +172,13 @@ const loadThread = { .map((id) => userLookup[id]) .filter((name): name is string => name !== undefined) : undefined, - threadUrl: - thread.url ?? - getFullCommsURL({ - workspaceId: thread.workspaceId, - channelId: thread.channelId, - threadId: thread.id, - }), + threadUrl: thread.url + ? rewriteToConfiguredHost(thread.url) + : getFullCommsURL({ + workspaceId: thread.workspaceId, + channelId: thread.channelId, + threadId: thread.id, + }), }, comments: comments.map((c) => ({ id: c.id, @@ -187,14 +187,14 @@ const loadThread = { creatorName: userLookup[c.creator], threadId: c.threadId, posted: c.posted.toISOString(), - commentUrl: - c.url ?? - getFullCommsURL({ - workspaceId: c.workspaceId, - channelId: c.channelId, - threadId: c.threadId, - commentId: c.id, - }), + commentUrl: c.url + ? rewriteToConfiguredHost(c.url) + : getFullCommsURL({ + workspaceId: c.workspaceId, + channelId: c.channelId, + threadId: c.threadId, + commentId: c.id, + }), })), totalComments: thread.commentCount, } diff --git a/src/tools/react.ts b/src/tools/react.ts index 8c62297..661648b 100644 --- a/src/tools/react.ts +++ b/src/tools/react.ts @@ -1,10 +1,10 @@ -import { getFullCommsURL } from '@doist/comms-sdk' import { z } from 'zod' import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' import { ReactOutputSchema } from '../utils/output-schemas.js' import { type ReactionTargetType, ReactionTargetTypeSchema } from '../utils/target-types.js' import { ToolNames } from '../utils/tool-names.js' +import { getFullCommsURL } from '../utils/url-helpers.js' const ArgsSchema = { targetType: ReactionTargetTypeSchema.describe( diff --git a/src/tools/reply.ts b/src/tools/reply.ts index f0a25a8..f0bf935 100644 --- a/src/tools/reply.ts +++ b/src/tools/reply.ts @@ -1,10 +1,11 @@ -import { getFullCommsURL, NOTIFY_AUDIENCES, type NotifyAudience } from '@doist/comms-sdk' +import { NOTIFY_AUDIENCES, type NotifyAudience } from '@doist/comms-sdk' import { z } from 'zod' import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' import { type ReplyOutput, ReplyOutputSchema } from '../utils/output-schemas.js' import { ReplyTargetTypeSchema } from '../utils/target-types.js' import { ToolNames } from '../utils/tool-names.js' +import { getFullCommsURL } from '../utils/url-helpers.js' const ArgsSchema = { targetType: ReplyTargetTypeSchema.describe( diff --git a/src/tools/search-content.ts b/src/tools/search-content.ts index 320dbd3..088e907 100644 --- a/src/tools/search-content.ts +++ b/src/tools/search-content.ts @@ -1,9 +1,10 @@ -import { type SearchResultType, getFullCommsURL } from '@doist/comms-sdk' +import type { SearchResultType } from '@doist/comms-sdk' import { z } from 'zod' import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' import { SearchContentOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' +import { getFullCommsURL } from '../utils/url-helpers.js' const ArgsSchema = { query: z.string().min(1).describe('The search query string.'), diff --git a/src/tools/update-object.ts b/src/tools/update-object.ts index b8c7248..f187207 100644 --- a/src/tools/update-object.ts +++ b/src/tools/update-object.ts @@ -1,4 +1,4 @@ -import { getFullCommsURL, type CommsApi } from '@doist/comms-sdk' +import type { CommsApi } from '@doist/comms-sdk' import { z } from 'zod' import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' @@ -11,6 +11,7 @@ import { } from '../utils/output-schemas.js' import { UpdateTargetTypeSchema } from '../utils/target-types.js' import { ToolNames } from '../utils/tool-names.js' +import { getFullCommsURL } from '../utils/url-helpers.js' const ArgsSchema = { targetType: UpdateTargetTypeSchema.describe( diff --git a/src/utils/__tests__/server-options.test.ts b/src/utils/__tests__/server-options.test.ts new file mode 100644 index 0000000..b416e7d --- /dev/null +++ b/src/utils/__tests__/server-options.test.ts @@ -0,0 +1,33 @@ +import { buildServerOptions } from '../server-options.js' + +describe('buildServerOptions', () => { + it('returns commsApiKey with undefined baseUrl when COMMS_BASE_URL is unset', () => { + expect(buildServerOptions({ COMMS_API_KEY: 'k' })).toEqual({ + commsApiKey: 'k', + baseUrl: undefined, + }) + }) + + it('passes through COMMS_BASE_URL when set', () => { + expect( + buildServerOptions({ + COMMS_API_KEY: 'k', + COMMS_BASE_URL: 'https://comms.staging.todoist.com', + }), + ).toEqual({ + commsApiKey: 'k', + baseUrl: 'https://comms.staging.todoist.com', + }) + }) + + it('treats empty COMMS_BASE_URL as undefined so the SDK default wins', () => { + expect(buildServerOptions({ COMMS_API_KEY: 'k', COMMS_BASE_URL: '' })).toEqual({ + commsApiKey: 'k', + baseUrl: undefined, + }) + }) + + it('throws when COMMS_API_KEY is missing', () => { + expect(() => buildServerOptions({})).toThrow('COMMS_API_KEY is not set') + }) +}) diff --git a/src/utils/__tests__/url-helpers.test.ts b/src/utils/__tests__/url-helpers.test.ts new file mode 100644 index 0000000..9f866f0 --- /dev/null +++ b/src/utils/__tests__/url-helpers.test.ts @@ -0,0 +1,99 @@ +import { + configureBaseUrl, + getCommentURL, + getFullCommsURL, + getMessageURL, + rewriteToConfiguredHost, + toRelativeCommsURL, +} from '../url-helpers.js' + +describe('url-helpers', () => { + afterEach(() => { + configureBaseUrl(undefined) + }) + + describe('getFullCommsURL', () => { + it('returns prod URL when no baseUrl is configured', () => { + expect(getFullCommsURL({ workspaceId: 1 })).toBe('https://comms.todoist.com/a/1/') + }) + + it('swaps the host when a staging baseUrl is configured', () => { + configureBaseUrl('https://comms.staging.todoist.com') + expect(getFullCommsURL({ workspaceId: 1 })).toBe( + 'https://comms.staging.todoist.com/a/1/', + ) + }) + + it('reverts to prod when baseUrl is cleared', () => { + configureBaseUrl('https://comms.staging.todoist.com') + configureBaseUrl(undefined) + expect(getFullCommsURL({ workspaceId: 1 })).toBe('https://comms.todoist.com/a/1/') + }) + }) + + describe('toRelativeCommsURL', () => { + it('strips the prod host', () => { + expect(toRelativeCommsURL('https://comms.todoist.com/a/1/')).toBe('/a/1/') + }) + + it('strips a staging or custom host', () => { + expect(toRelativeCommsURL('https://comms.staging.todoist.com/a/1/')).toBe('/a/1/') + }) + }) + + describe('getMessageURL', () => { + it('returns a relative path by default', () => { + const url = getMessageURL({ + workspaceId: 1, + conversationId: 'c', + messageId: 'm', + }) + expect(url).toBe('/a/1/msg/c/m/m') + }) + + it('rewrites the host when staging is configured', () => { + configureBaseUrl('https://comms.staging.todoist.com') + const url = getMessageURL({ + workspaceId: 1, + conversationId: 'c', + messageId: 'm', + }) + // getMessageURL returns a relative path, so applyBaseUrl is a no-op; + // this asserts the wrapper doesn't accidentally mangle it. + expect(url).toBe('/a/1/msg/c/m/m') + }) + }) + + describe('getCommentURL', () => { + it('returns a relative path by default', () => { + const url = getCommentURL({ + workspaceId: 1, + channelId: 'ch', + threadId: 't', + commentId: 'cm', + }) + expect(url).toBe('/a/1/ch/ch/t/t/c/cm') + }) + }) + + describe('rewriteToConfiguredHost', () => { + it('rewrites the prod host of an SDK-supplied URL when staging is configured', () => { + configureBaseUrl('https://comms.staging.todoist.com') + expect(rewriteToConfiguredHost('https://comms.todoist.com/a/1/ch/x/t/y/')).toBe( + 'https://comms.staging.todoist.com/a/1/ch/x/t/y/', + ) + }) + + it('is idempotent on an already-staging URL', () => { + configureBaseUrl('https://comms.staging.todoist.com') + const staging = 'https://comms.staging.todoist.com/a/1/' + expect(rewriteToConfiguredHost(staging)).toBe(staging) + }) + + it('passes through unchanged when no baseUrl is configured', () => { + expect(rewriteToConfiguredHost('https://comms.todoist.com/a/1/')).toBe( + 'https://comms.todoist.com/a/1/', + ) + }) + }) +}) diff --git a/src/utils/server-options.ts b/src/utils/server-options.ts new file mode 100644 index 0000000..e266765 --- /dev/null +++ b/src/utils/server-options.ts @@ -0,0 +1,18 @@ +// Kept separate from main.ts so the env wiring is unit-testable — +// importing main.ts triggers server startup at module load. + +export type ServerOptions = { + commsApiKey: string + baseUrl: string | undefined +} + +export function buildServerOptions(env: NodeJS.ProcessEnv = process.env): ServerOptions { + const commsApiKey = env.COMMS_API_KEY + if (!commsApiKey) { + throw new Error('COMMS_API_KEY is not set') + } + // Empty string would override the SDK's prod default with garbage; + // treat `COMMS_BASE_URL=` the same as unset. + const baseUrl = env.COMMS_BASE_URL || undefined + return { commsApiKey, baseUrl } +} diff --git a/src/utils/url-helpers.ts b/src/utils/url-helpers.ts index 0f5b497..f3581dd 100644 --- a/src/utils/url-helpers.ts +++ b/src/utils/url-helpers.ts @@ -1,4 +1,42 @@ -import { getFullCommsURL } from '@doist/comms-sdk' +import { + getCommentURL as sdkGetCommentURL, + getFullCommsURL as sdkGetFullCommsURL, + getMessageURL as sdkGetMessageURL, +} from '@doist/comms-sdk' + +// Module-level state set by main.ts at startup. The SDK's URL helpers +// hardcode the prod host, so without a wrapper a staging-targeted MCP +// would still return prod links in its tool output. +let configuredBaseUrl: string | undefined + +export function configureBaseUrl(baseUrl: string | undefined): void { + configuredBaseUrl = baseUrl +} + +function applyBaseUrl(url: string): string { + if (!configuredBaseUrl) return url + return url.replace(/^https:\/\/comms\.todoist\.com/, configuredBaseUrl) +} + +// Strip whatever host the SDK produced — robust to staging/custom hosts. +export function toRelativeCommsURL(url: string): string { + return url.replace(/^https?:\/\/[^/]+/, '') +} + +// Rewrite SDK-populated url fields (entity.url, etc.) to the configured +// host. The SDK entity schemas hardcode prod, so a staging-targeted +// server would otherwise return prod links in its structured output. +// Idempotent: re-applying on an already-staging URL is a no-op. +export const rewriteToConfiguredHost = applyBaseUrl + +export const getFullCommsURL: typeof sdkGetFullCommsURL = (...args) => + applyBaseUrl(sdkGetFullCommsURL(...args)) + +export const getCommentURL: typeof sdkGetCommentURL = (...args) => + applyBaseUrl(sdkGetCommentURL(...args)) + +export const getMessageURL: typeof sdkGetMessageURL = (...args) => + applyBaseUrl(sdkGetMessageURL(...args)) export function getWorkspaceUrl(workspaceId: number): string { return getFullCommsURL({ workspaceId }) From ae15ba1a1e82c89f0fc482cbfd309c63762fd7b9 Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:15:00 +0200 Subject: [PATCH 02/13] test(url-helpers): trim marginal cases, document intent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each remaining test now catches a distinct regression I'd care about: - Drop `getFullCommsURL > reverts to prod when cleared` — only exercised test-isolation hygiene; no consumer calls configureBaseUrl(undefined) at runtime. - Consolidate the two getMessageURL tests + the single getCommentURL test into one `relative-URL helpers` describe with two assertions (no-baseUrl and staging-configured). The previous form had one test whose own comment admitted it was a no-op duplicate. - Add comments on the load-bearing cases (toRelativeCommsURL host variants, rewriteToConfiguredHost idempotency) so future contributors know which constraint each one is guarding. Net: 204 passing (was 206); 13 new tests instead of 15. --- src/utils/__tests__/url-helpers.test.ts | 62 ++++++++++--------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/src/utils/__tests__/url-helpers.test.ts b/src/utils/__tests__/url-helpers.test.ts index 9f866f0..278c895 100644 --- a/src/utils/__tests__/url-helpers.test.ts +++ b/src/utils/__tests__/url-helpers.test.ts @@ -23,11 +23,28 @@ describe('url-helpers', () => { 'https://comms.staging.todoist.com/a/1/', ) }) + }) + + describe('relative-URL helpers', () => { + // The SDK's `getMessageURL`/`getCommentURL` return relative paths. + // applyBaseUrl's regex only matches a leading prod host, so the + // wrapped versions must be no-ops on relative paths regardless of + // whether staging is configured. This catches "wrapper accidentally + // prepends/mangles a host on relative URLs." + const messageArgs = { workspaceId: 1, conversationId: 'c', messageId: 'm' } + const commentArgs = { workspaceId: 1, channelId: 'ch', threadId: 't', commentId: 'cm' } + const messageRelative = '/a/1/msg/c/m/m' + const commentRelative = '/a/1/ch/ch/t/t/c/cm' + + it('pass relative paths through unchanged with no baseUrl', () => { + expect(getMessageURL(messageArgs)).toBe(messageRelative) + expect(getCommentURL(commentArgs)).toBe(commentRelative) + }) - it('reverts to prod when baseUrl is cleared', () => { + it('pass relative paths through unchanged with staging configured', () => { configureBaseUrl('https://comms.staging.todoist.com') - configureBaseUrl(undefined) - expect(getFullCommsURL({ workspaceId: 1 })).toBe('https://comms.todoist.com/a/1/') + expect(getMessageURL(messageArgs)).toBe(messageRelative) + expect(getCommentURL(commentArgs)).toBe(commentRelative) }) }) @@ -36,46 +53,15 @@ describe('url-helpers', () => { expect(toRelativeCommsURL('https://comms.todoist.com/a/1/')).toBe('/a/1/') }) + // The previous build-link implementation hardcoded the prod host in + // a `.replace(...)` call, which silently failed on staging. The + // regex must strip any host so a future contributor can't reintroduce + // the hardcoded-host pattern without breaking this test. it('strips a staging or custom host', () => { expect(toRelativeCommsURL('https://comms.staging.todoist.com/a/1/')).toBe('/a/1/') }) }) - describe('getMessageURL', () => { - it('returns a relative path by default', () => { - const url = getMessageURL({ - workspaceId: 1, - conversationId: 'c', - messageId: 'm', - }) - expect(url).toBe('/a/1/msg/c/m/m') - }) - - it('rewrites the host when staging is configured', () => { - configureBaseUrl('https://comms.staging.todoist.com') - const url = getMessageURL({ - workspaceId: 1, - conversationId: 'c', - messageId: 'm', - }) - // getMessageURL returns a relative path, so applyBaseUrl is a no-op; - // this asserts the wrapper doesn't accidentally mangle it. - expect(url).toBe('/a/1/msg/c/m/m') - }) - }) - - describe('getCommentURL', () => { - it('returns a relative path by default', () => { - const url = getCommentURL({ - workspaceId: 1, - channelId: 'ch', - threadId: 't', - commentId: 'cm', - }) - expect(url).toBe('/a/1/ch/ch/t/t/c/cm') - }) - }) - describe('rewriteToConfiguredHost', () => { it('rewrites the prod host of an SDK-supplied URL when staging is configured', () => { configureBaseUrl('https://comms.staging.todoist.com') From cd095d26a261123187ad2f7ea275662df59280bf Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:26:27 +0200 Subject: [PATCH 03/13] fix(url-helpers): harden host-rewrite against injection, slashes, $-tokens Doistbot/recall-mode review surfaced four real regressions in the host-rewrite layer; all are now guarded by tests: - **Host injection**: regex `^https://comms\.todoist\.com` had no trailing boundary, so `https://comms.todoist.com.evil.com/x` matched and rewrote to `https://.evil.com/x`. Anchored with `(?=[/:?#]|$)`. - **Trailing slash**: `COMMS_BASE_URL=https://staging/` produced double-slash URLs. `configureBaseUrl` now strips trailing slashes. - **`$` in baseUrl**: `String.replace` treats `$&` etc. as backreferences. Switched to a function replacement so the value is always literal. - **Case sensitivity**: regex is now `i` so an SDK or fixture emitting `Https://Comms.Todoist.Com/...` doesn't silently bypass the rewrite. Also documented the process-scoped, last-call-wins nature of `configureBaseUrl` in its JSDoc so callers know the multi-tenant constraint without reading the implementation. --- src/utils/__tests__/url-helpers.test.ts | 38 +++++++++++++++++++++++++ src/utils/url-helpers.ts | 24 ++++++++++++---- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/utils/__tests__/url-helpers.test.ts b/src/utils/__tests__/url-helpers.test.ts index 278c895..0fd5720 100644 --- a/src/utils/__tests__/url-helpers.test.ts +++ b/src/utils/__tests__/url-helpers.test.ts @@ -81,5 +81,43 @@ describe('url-helpers', () => { 'https://comms.todoist.com/a/1/', ) }) + + // The regex anchors with `(?=[/:?#]|$)` so an attacker-controlled + // suffix on the host can't sneak through the rewrite. + it('does not match a host with a malicious suffix', () => { + configureBaseUrl('https://comms.staging.todoist.com') + expect(rewriteToConfiguredHost('https://comms.todoist.com.evil.com/path')).toBe( + 'https://comms.todoist.com.evil.com/path', + ) + expect(rewriteToConfiguredHost('https://comms.todoist.comEXTRA/x')).toBe( + 'https://comms.todoist.comEXTRA/x', + ) + }) + + // Case-insensitive so an SDK upgrade emitting `Https://...` or + // `Comms.Todoist.Com` doesn't silently bypass the rewrite. + it('matches case-insensitively', () => { + configureBaseUrl('https://comms.staging.todoist.com') + expect(rewriteToConfiguredHost('HTTPS://Comms.Todoist.Com/a/1/')).toBe( + 'https://comms.staging.todoist.com/a/1/', + ) + }) + + // `$&` in `configuredBaseUrl` must not be interpreted as a + // backreference — use a function replacement. + it('treats $-tokens in the configured base URL as literal', () => { + configureBaseUrl('https://staging.example.com/$&') + expect(rewriteToConfiguredHost('https://comms.todoist.com/a/1/')).toBe( + 'https://staging.example.com/$&/a/1/', + ) + }) + + // Trailing slash in COMMS_BASE_URL must not double up. + it('strips a trailing slash from the configured base URL', () => { + configureBaseUrl('https://comms.staging.todoist.com/') + expect(rewriteToConfiguredHost('https://comms.todoist.com/a/1/')).toBe( + 'https://comms.staging.todoist.com/a/1/', + ) + }) }) }) diff --git a/src/utils/url-helpers.ts b/src/utils/url-helpers.ts index f3581dd..83e9c3d 100644 --- a/src/utils/url-helpers.ts +++ b/src/utils/url-helpers.ts @@ -4,18 +4,32 @@ import { getMessageURL as sdkGetMessageURL, } from '@doist/comms-sdk' -// Module-level state set by main.ts at startup. The SDK's URL helpers -// hardcode the prod host, so without a wrapper a staging-targeted MCP -// would still return prod links in its tool output. +// Module-level state set by `getMcpServer` (and scripts/run-tool.ts) +// at startup. The SDK's URL helpers hardcode the prod host, so without +// a wrapper a staging-targeted MCP would still return prod links in +// its tool output. +// +// **Process-scoped, last-call wins** — not multi-tenant safe. Library +// consumers using importable tools standalone must call this +// themselves; otherwise the helpers default to prod. let configuredBaseUrl: string | undefined export function configureBaseUrl(baseUrl: string | undefined): void { - configuredBaseUrl = baseUrl + // Normalize trailing slash so `https://staging/` doesn't produce + // double-slashed URLs like `https://staging//a/1/`. + configuredBaseUrl = baseUrl ? baseUrl.replace(/\/+$/, '') : undefined } +// Match the SDK's prod host (case-insensitive, with a trailing +// boundary so `comms.todoist.com.evil.com` does NOT match — prevents +// host-injection if a url field is ever attacker-influenced). +const PROD_HOST_RE = /^https:\/\/comms\.todoist\.com(?=[/:?#]|$)/i + function applyBaseUrl(url: string): string { if (!configuredBaseUrl) return url - return url.replace(/^https:\/\/comms\.todoist\.com/, configuredBaseUrl) + // Use a function replacement so `$` in `configuredBaseUrl` isn't + // interpreted as a backreference token. + return url.replace(PROD_HOST_RE, () => configuredBaseUrl as string) } // Strip whatever host the SDK produced — robust to staging/custom hosts. From c1f3e186a4ab8822a817a6205816a4cfe4825580 Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:27:25 +0200 Subject: [PATCH 04/13] fix(tools): rewrite SDK-supplied entity URLs in reply/react/create-thread/update-object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These four tools returned `entity.url ?? getFullCommsURL(...)` directly in their structured output. The SDK's entity schemas hardcode the prod host, so a staging-targeted MCP would still return prod links in tool output — defeating the whole point of the rewrite layer. Wrap the fallback expression in `rewriteToConfiguredHost(...)` (no-op on a prod URL when no baseUrl is configured; rewrites to staging otherwise; idempotent on already-staging URLs). The `??` semantics are preserved — empty-string url is still preserved as the SDK value, not silently replaced. Existing snapshot tests still pass because they don't configure a staging baseUrl mid-suite; with no baseUrl set, `rewriteToConfiguredHost` is a no-op and the snapshots see the same prod URLs they always have. --- src/tools/create-thread.ts | 15 ++++++------- src/tools/react.ts | 43 ++++++++++++++++++++------------------ src/tools/reply.ts | 30 +++++++++++++------------- src/tools/update-object.ts | 43 ++++++++++++++++++++------------------ 4 files changed, 70 insertions(+), 61 deletions(-) diff --git a/src/tools/create-thread.ts b/src/tools/create-thread.ts index a7e952f..9e6668b 100644 --- a/src/tools/create-thread.ts +++ b/src/tools/create-thread.ts @@ -3,7 +3,7 @@ import type { CommsTool } from '../comms-tool.js' import { getToolOutput } from '../mcp-helpers.js' import { type CreateThreadOutput, CreateThreadOutputSchema } from '../utils/output-schemas.js' import { ToolNames } from '../utils/tool-names.js' -import { getFullCommsURL } from '../utils/url-helpers.js' +import { getFullCommsURL, rewriteToConfiguredHost } from '../utils/url-helpers.js' const ArgsSchema = { channelId: z.string().describe('The ID of the channel to create the thread in.'), @@ -48,13 +48,14 @@ const createThread = { : postedValue : new Date() - const threadUrl = + const threadUrl = rewriteToConfiguredHost( thread.url ?? - getFullCommsURL({ - workspaceId: thread.workspaceId, - channelId: thread.channelId, - threadId: thread.id, - }) + getFullCommsURL({ + workspaceId: thread.workspaceId, + channelId: thread.channelId, + threadId: thread.id, + }), + ) const lines: string[] = [ `# Thread Created`, diff --git a/src/tools/react.ts b/src/tools/react.ts index 661648b..a6a6087 100644 --- a/src/tools/react.ts +++ b/src/tools/react.ts @@ -4,7 +4,7 @@ import { getToolOutput } from '../mcp-helpers.js' import { ReactOutputSchema } from '../utils/output-schemas.js' import { type ReactionTargetType, ReactionTargetTypeSchema } from '../utils/target-types.js' import { ToolNames } from '../utils/tool-names.js' -import { getFullCommsURL } from '../utils/url-helpers.js' +import { getFullCommsURL, rewriteToConfiguredHost } from '../utils/url-helpers.js' const ArgsSchema = { targetType: ReactionTargetTypeSchema.describe( @@ -43,33 +43,36 @@ const react = { // Fetch target metadata to get URL if (targetType === 'thread') { const thread = await client.threads.getThread(targetId) - targetUrl = + targetUrl = rewriteToConfiguredHost( thread.url ?? - getFullCommsURL({ - workspaceId: thread.workspaceId, - channelId: thread.channelId, - threadId: thread.id, - }) + getFullCommsURL({ + workspaceId: thread.workspaceId, + channelId: thread.channelId, + threadId: thread.id, + }), + ) } else if (targetType === 'comment') { const comment = await client.comments.getComment(targetId) - targetUrl = + targetUrl = rewriteToConfiguredHost( comment.url ?? - getFullCommsURL({ - workspaceId: comment.workspaceId, - channelId: comment.channelId, - threadId: comment.threadId, - commentId: comment.id, - }) + getFullCommsURL({ + workspaceId: comment.workspaceId, + channelId: comment.channelId, + threadId: comment.threadId, + commentId: comment.id, + }), + ) } else { // message const message = await client.conversationMessages.getMessage(targetId) - targetUrl = + targetUrl = rewriteToConfiguredHost( message.url ?? - getFullCommsURL({ - workspaceId: message.workspaceId, - conversationId: message.conversationId, - messageId: message.id, - }) + getFullCommsURL({ + workspaceId: message.workspaceId, + conversationId: message.conversationId, + messageId: message.id, + }), + ) } // Map targetType to the appropriate API parameter diff --git a/src/tools/reply.ts b/src/tools/reply.ts index f0bf935..a60a300 100644 --- a/src/tools/reply.ts +++ b/src/tools/reply.ts @@ -5,7 +5,7 @@ import { getToolOutput } from '../mcp-helpers.js' import { type ReplyOutput, ReplyOutputSchema } from '../utils/output-schemas.js' import { ReplyTargetTypeSchema } from '../utils/target-types.js' import { ToolNames } from '../utils/tool-names.js' -import { getFullCommsURL } from '../utils/url-helpers.js' +import { getFullCommsURL, rewriteToConfiguredHost } from '../utils/url-helpers.js' const ArgsSchema = { targetType: ReplyTargetTypeSchema.describe( @@ -71,14 +71,15 @@ const reply = { notifyAudience: appliedAudience, }) replyId = comment.id - replyUrl = + replyUrl = rewriteToConfiguredHost( comment.url ?? - getFullCommsURL({ - workspaceId: comment.workspaceId, - channelId: comment.channelId, - threadId: comment.threadId, - commentId: comment.id, - }) + getFullCommsURL({ + workspaceId: comment.workspaceId, + channelId: comment.channelId, + threadId: comment.threadId, + commentId: comment.id, + }), + ) const postedValue = comment.posted created = postedValue ? typeof postedValue === 'string' @@ -91,13 +92,14 @@ const reply = { content, }) replyId = message.id - replyUrl = + replyUrl = rewriteToConfiguredHost( message.url ?? - getFullCommsURL({ - workspaceId: message.workspaceId, - conversationId: message.conversationId, - messageId: message.id, - }) + getFullCommsURL({ + workspaceId: message.workspaceId, + conversationId: message.conversationId, + messageId: message.id, + }), + ) const postedValue = message.posted created = postedValue ? typeof postedValue === 'string' diff --git a/src/tools/update-object.ts b/src/tools/update-object.ts index f187207..3abf025 100644 --- a/src/tools/update-object.ts +++ b/src/tools/update-object.ts @@ -11,7 +11,7 @@ import { } from '../utils/output-schemas.js' import { UpdateTargetTypeSchema } from '../utils/target-types.js' import { ToolNames } from '../utils/tool-names.js' -import { getFullCommsURL } from '../utils/url-helpers.js' +import { getFullCommsURL, rewriteToConfiguredHost } from '../utils/url-helpers.js' const ArgsSchema = { targetType: UpdateTargetTypeSchema.describe( @@ -46,13 +46,14 @@ async function updateThreadBranch(args: Args, client: CommsApi): Promise const thread = await client.threads.updateThread({ id: targetId, title, content }) - const threadUrl = + const threadUrl = rewriteToConfiguredHost( thread.url ?? - getFullCommsURL({ - workspaceId: thread.workspaceId, - channelId: thread.channelId, - threadId: thread.id, - }) + getFullCommsURL({ + workspaceId: thread.workspaceId, + channelId: thread.channelId, + threadId: thread.id, + }), + ) const lastEdited = thread.lastEdited ? thread.lastEdited.toISOString() : undefined @@ -93,14 +94,15 @@ async function updateCommentBranch(args: Args, client: CommsApi): Promise Date: Fri, 22 May 2026 16:28:28 +0200 Subject: [PATCH 05/13] fix(fetch-inbox): rewrite SDK url on the unreadThreads structured array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Earlier in this PR I switched the mapped `threads[]` / `conversations[]` arrays to use rewriteToConfiguredHost, but missed that the same structured payload also returns `unreadThreads: unreadThreadsOriginal` — the raw `InboxThread[]` objects from the SDK, with their prod-hosted `url` fields untouched. So a staging-targeted server returned a payload where threads[] had staging URLs but unreadThreads[*].url still pointed at prod. Inconsistent and easy for an LLM to grab the wrong field. Map unreadThreadsOriginal to spread the entity and override `url`. UnreadConversation has no `url` field per the SDK schema, so it doesn't need the same treatment. --- src/tools/fetch-inbox.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/tools/fetch-inbox.ts b/src/tools/fetch-inbox.ts index ac07587..0271e50 100644 --- a/src/tools/fetch-inbox.ts +++ b/src/tools/fetch-inbox.ts @@ -308,7 +308,13 @@ const fetchInbox = { } }), unreadCount: unreadThreads.length, - unreadThreads: unreadThreadsOriginal, + // InboxThread.url is hardcoded to the SDK prod host; rewrite + // so a staging-targeted server returns staging links here too. + // UnreadConversation has no url field, so no rewrite needed. + unreadThreads: unreadThreadsOriginal.map((t) => ({ + ...t, + url: rewriteToConfiguredHost(t.url), + })), unreadConversations: unreadConversationsOriginal, totalThreads: threads.length, totalConversations: conversationsWithDetails.length, From 8afe7822af11171aa7c4be3ccb2d82c29ebcd5cb Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:29:19 +0200 Subject: [PATCH 06/13] fix(server-options): trim whitespace, make baseUrl optional Two related ergonomics fixes for the env-wiring layer: - Trim `COMMS_BASE_URL` before the falsy coercion so a trailing space in an env file (`COMMS_BASE_URL=https://staging `) doesn't pass garbage into `new CommsApi(...)` and into `applyBaseUrl`'s regex target. The startup log also stays clean. - Change `ServerOptions.baseUrl` from `string | undefined` (required key, value may be undefined) to `baseUrl?: string` (key may be omitted entirely). The published `dist/index.d.ts` shipped with the rename otherwise required external consumers to spell out `getMcpServer({ commsApiKey, baseUrl: undefined })`, which is silly and a silent semver break vs. the pre-fork shape. --- src/utils/__tests__/server-options.test.ts | 16 ++++++++++++++++ src/utils/server-options.ts | 10 ++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/utils/__tests__/server-options.test.ts b/src/utils/__tests__/server-options.test.ts index b416e7d..cff8024 100644 --- a/src/utils/__tests__/server-options.test.ts +++ b/src/utils/__tests__/server-options.test.ts @@ -27,6 +27,22 @@ describe('buildServerOptions', () => { }) }) + it('trims whitespace from COMMS_BASE_URL so " " is not passed to the SDK', () => { + expect(buildServerOptions({ COMMS_API_KEY: 'k', COMMS_BASE_URL: ' ' })).toEqual({ + commsApiKey: 'k', + baseUrl: undefined, + }) + expect( + buildServerOptions({ + COMMS_API_KEY: 'k', + COMMS_BASE_URL: ' https://comms.staging.todoist.com ', + }), + ).toEqual({ + commsApiKey: 'k', + baseUrl: 'https://comms.staging.todoist.com', + }) + }) + it('throws when COMMS_API_KEY is missing', () => { expect(() => buildServerOptions({})).toThrow('COMMS_API_KEY is not set') }) diff --git a/src/utils/server-options.ts b/src/utils/server-options.ts index e266765..c95ffbc 100644 --- a/src/utils/server-options.ts +++ b/src/utils/server-options.ts @@ -3,7 +3,9 @@ export type ServerOptions = { commsApiKey: string - baseUrl: string | undefined + // Optional so external callers (`getMcpServer({ commsApiKey })`) + // don't have to spell out `baseUrl: undefined`. + baseUrl?: string } export function buildServerOptions(env: NodeJS.ProcessEnv = process.env): ServerOptions { @@ -11,8 +13,8 @@ export function buildServerOptions(env: NodeJS.ProcessEnv = process.env): Server if (!commsApiKey) { throw new Error('COMMS_API_KEY is not set') } - // Empty string would override the SDK's prod default with garbage; - // treat `COMMS_BASE_URL=` the same as unset. - const baseUrl = env.COMMS_BASE_URL || undefined + // Trim before the falsy check so `COMMS_BASE_URL= ` (trailing + // space, common in env files) doesn't pass garbage to the SDK. + const baseUrl = env.COMMS_BASE_URL?.trim() || undefined return { commsApiKey, baseUrl } } From 1b1a72cffb7bcba7578b70d0694c8483bbcf3a55 Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:29:35 +0200 Subject: [PATCH 07/13] fix(build-link): drop hardcoded prod host from tool schema description The `fullUrl` arg's `.describe(...)` is shown to the LLM as the tool contract. It hardcoded `https://comms.todoist.com` even after this PR made the host configurable, so a model running against a staging-targeted server would see a description that doesn't match the URL it gets back. Replaced with "the configured host". --- src/tools/build-link.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/build-link.ts b/src/tools/build-link.ts index 5a210cf..1cc06f0 100644 --- a/src/tools/build-link.ts +++ b/src/tools/build-link.ts @@ -31,7 +31,7 @@ const ArgsSchema = { .optional() .default(true) .describe( - 'Whether to return a full URL (with https://comms.todoist.com) or relative path.', + 'Whether to return a full URL (including the configured host) or a relative path.', ), } From 82d250b13bedf8626f030d9da6d7926a52798043 Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:30:30 +0200 Subject: [PATCH 08/13] test(fetch-inbox): assert exact URLs instead of substring matches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `expect(url).toContain('comms.todoist.com')` trivially passes for `'comms.staging.todoist.com'` (substring) — so the assertion silently green-lights any future regression where the rewrite layer is broken or removed. Switched the four affected assertions to exact `toBe(...)` comparisons against the expected prod URL, so a regression flipping the host would actually fail the test. --- src/tools/__tests__/fetch-inbox.test.ts | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/tools/__tests__/fetch-inbox.test.ts b/src/tools/__tests__/fetch-inbox.test.ts index 3b95460..b7b3c85 100644 --- a/src/tools/__tests__/fetch-inbox.test.ts +++ b/src/tools/__tests__/fetch-inbox.test.ts @@ -136,7 +136,12 @@ describe(`${FETCH_INBOX} tool`, () => { if (threads?.[0] && threads[1]) { expect(threads[0].id).toBe(TEST_IDS.THREAD_1) expect(threads[0].channelName).toBe('Test Channel') - expect(threads[0].threadUrl).toContain('comms.todoist.com') + // toBe (not toContain): 'comms.staging.todoist.com' contains + // 'comms.todoist.com', so toContain wouldn't catch a rewrite + // regression on a staging-targeted run. + expect(threads[0].threadUrl).toBe( + `https://comms.todoist.com/a/${TEST_IDS.WORKSPACE_1}/ch/${TEST_IDS.CHANNEL_1}/t/${TEST_IDS.THREAD_1}/`, + ) expect(threads[0].isUnread).toBe(true) expect(threads[1].isStarred).toBe(true) } @@ -329,7 +334,9 @@ describe(`${FETCH_INBOX} tool`, () => { expect(conversations[0].id).toBe(TEST_IDS.CONVERSATION_1) expect(conversations[0].participantNames).toEqual(['Alice', 'Bob']) expect(conversations[0].isUnread).toBe(true) - expect(conversations[0].conversationUrl).toContain('comms.todoist.com') + expect(conversations[0].conversationUrl).toBe( + `https://comms.todoist.com/a/${TEST_IDS.WORKSPACE_1}/msg/${TEST_IDS.CONVERSATION_1}/`, + ) expect(conversations[1].title).toBe('Project Discussion') } }) @@ -540,10 +547,9 @@ describe(`${FETCH_INBOX} tool`, () => { const { structuredContent } = result expect(structuredContent?.threads).toHaveLength(1) const threadUrl = structuredContent?.threads?.[0]?.threadUrl - expect(threadUrl).toBeDefined() - expect(typeof threadUrl).toBe('string') - expect(threadUrl).toContain('comms.todoist.com') - expect(threadUrl).toContain(String(TEST_IDS.THREAD_1)) + expect(threadUrl).toBe( + `https://comms.todoist.com/a/${TEST_IDS.WORKSPACE_1}/ch/${TEST_IDS.CHANNEL_1}/t/${TEST_IDS.THREAD_1}/`, + ) }) it('should construct conversationUrl via getFullCommsURL when SDK omits url field', async () => { @@ -607,10 +613,9 @@ describe(`${FETCH_INBOX} tool`, () => { const { structuredContent } = result expect(structuredContent?.conversations).toHaveLength(1) const conversationUrl = structuredContent?.conversations?.[0]?.conversationUrl - expect(conversationUrl).toBeDefined() - expect(typeof conversationUrl).toBe('string') - expect(conversationUrl).toContain('comms.todoist.com') - expect(conversationUrl).toContain(String(TEST_IDS.CONVERSATION_1)) + expect(conversationUrl).toBe( + `https://comms.todoist.com/a/${TEST_IDS.WORKSPACE_1}/msg/${TEST_IDS.CONVERSATION_1}/`, + ) }) }) From cda4c729e82f1df06e3dc96c08558557cd9e86e0 Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:31:07 +0200 Subject: [PATCH 09/13] feat(index): export configureBaseUrl for standalone tool consumers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Library consumers who import individual tools (e.g. `import { reply } from '@doist/comms-mcp'`) and use them outside of getMcpServer never trigger the URL host-rewrite — their structured output silently returns prod URLs even when their own CommsApi is pointed at staging. The init-order precondition can't be enforced in the type system without rewriting every tool's execute signature, so export `configureBaseUrl` directly and document the requirement in the README's "Plug them into an AI" section. One line in user code, no silent regression. --- README.md | 6 +++++- src/index.ts | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 3ecc3d2..8cb452d 100644 --- a/README.md +++ b/README.md @@ -17,9 +17,13 @@ npm install @doist/comms-mcp Example with [Vercel's AI SDK](https://ai-sdk.dev/docs/ai-sdk-core/generating-text#streamtext): ```js -import { fetchInbox, reply, markDone } from '@doist/comms-mcp' +import { configureBaseUrl, fetchInbox, reply, markDone } from '@doist/comms-mcp' import { streamText } from 'ai' +// Required if your CommsApi targets staging / a custom deployment. +// `getMcpServer` calls this for you; standalone tool consumers must. +configureBaseUrl(process.env.COMMS_BASE_URL) + const result = streamText({ model: yourModel, system: 'You are a helpful Comms assistant', diff --git a/src/index.ts b/src/index.ts index 957818e..494d8ea 100644 --- a/src/index.ts +++ b/src/index.ts @@ -14,6 +14,7 @@ import { reply } from './tools/reply.js' import { searchContent } from './tools/search-content.js' import { updateObject } from './tools/update-object.js' import { userInfo } from './tools/user-info.js' +import { configureBaseUrl } from './utils/url-helpers.js' const tools = { userInfo, @@ -33,7 +34,7 @@ const tools = { getGroups, } -export { tools, getMcpServer } +export { tools, getMcpServer, configureBaseUrl } export { userInfo, From 3dda4b997c6887b19e93337bc8944b97670a2c05 Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:33:26 +0200 Subject: [PATCH 10/13] refactor(run-tool): reuse buildServerOptions for env parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scripts/run-tool.ts duplicated the COMMS_API_KEY check and COMMS_BASE_URL fallback that buildServerOptions already encapsulates. That's the exact code path that grew the original COMMS_BASE_URL ignored-by-main bug — keeping two parsers in sync is how it slipped through. Use the shared helper so the two entry points can't drift. --- scripts/run-tool.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/scripts/run-tool.ts b/scripts/run-tool.ts index a5749ca..726e39e 100644 --- a/scripts/run-tool.ts +++ b/scripts/run-tool.ts @@ -34,6 +34,7 @@ import { reply } from '../src/tools/reply.js' import { searchContent } from '../src/tools/search-content.js' import { updateObject } from '../src/tools/update-object.js' import { userInfo } from '../src/tools/user-info.js' +import { buildServerOptions } from '../src/utils/server-options.js' import { configureBaseUrl } from '../src/utils/url-helpers.js' // Define a minimal type for tool execution that works with any tool @@ -138,15 +139,16 @@ async function main() { process.exit(1) } - const apiKey = process.env.COMMS_API_KEY - if (!apiKey) { - console.error('COMMS_API_KEY not found in environment or .env file') + let commsApiKey: string + let baseUrl: string | undefined + try { + ;({ commsApiKey, baseUrl } = buildServerOptions()) + } catch (e) { + console.error(e instanceof Error ? e.message : String(e)) process.exit(1) } - - const baseUrl = process.env.COMMS_BASE_URL || undefined configureBaseUrl(baseUrl) - const client = new CommsApi(apiKey, { baseUrl }) + const client = new CommsApi(commsApiKey, { baseUrl }) console.log(`Running ${toolName} with args:`) console.log(JSON.stringify(parsedArgs, null, 2)) From 6f4e042f406766d9f6628d9e18dd037c823e734f Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:33:48 +0200 Subject: [PATCH 11/13] fix(main): emit structured JSON startup log for observability Doist observability standard requires structured logging. The free-form stderr string was easy to read but not machine-parsable. Switch to a JSON object with `level`, `event`, `base_url`, and `base_url_source` so log aggregators can index it. Ref: https://handbook.doist.com/doc/standard-observability-vWRaOfitho --- src/main.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/main.ts b/src/main.ts index 25c4e77..e62be31 100644 --- a/src/main.ts +++ b/src/main.ts @@ -6,10 +6,18 @@ import { buildServerOptions } from './utils/server-options.js' function main() { const options = buildServerOptions() - // stderr (stdout is reserved for the MCP protocol). Surfacing the - // target up-front turns "why am I getting 403s" debugging into one - // log line — staging vs prod tokens aren't cross-compatible. - console.error(`Comms MCP targeting ${options.baseUrl ?? 'https://comms.todoist.com (default)'}`) + // Structured stderr log (stdout is reserved for the MCP protocol). + // Surfacing the target up-front turns "why am I getting 403s" + // debugging into one machine-parsable line — staging vs prod + // tokens aren't cross-compatible. + console.error( + JSON.stringify({ + level: 'info', + event: 'startup', + base_url: options.baseUrl ?? 'https://comms.todoist.com', + base_url_source: options.baseUrl ? 'env' : 'default', + }), + ) const server = getMcpServer(options) const transport = new StdioServerTransport() server From 4c7c6de4934bd904c65d82bfdcbac979ada940fd Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:34:56 +0200 Subject: [PATCH 12/13] test(fetch-inbox): assert tool-boundary rewrite for staging baseUrl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Doistbot rightly flagged that url-helpers' unit tests cover the rewrite mechanism but no test exercises it at the tool boundary — which is the layer that previously regressed (e.g. unreadThreads[*].url returned raw SDK output despite the rewrite in this PR). Add a focused case that configures staging mid-test, runs fetchInbox, and asserts BOTH the mapped threads[*].threadUrl and the raw unreadThreads[*].url are rewritten. afterEach resets the module state. --- src/tools/__tests__/fetch-inbox.test.ts | 43 +++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/tools/__tests__/fetch-inbox.test.ts b/src/tools/__tests__/fetch-inbox.test.ts index b7b3c85..5d50438 100644 --- a/src/tools/__tests__/fetch-inbox.test.ts +++ b/src/tools/__tests__/fetch-inbox.test.ts @@ -2,6 +2,7 @@ import type { CommsApi } from '@doist/comms-sdk' import { jest } from '@jest/globals' import { extractTextContent, TEST_IDS } from '../../utils/test-helpers.js' import { ToolNames } from '../../utils/tool-names.js' +import { configureBaseUrl } from '../../utils/url-helpers.js' import { fetchInbox } from '../fetch-inbox.js' // Mock the Comms API @@ -632,4 +633,46 @@ describe(`${FETCH_INBOX} tool`, () => { ).rejects.toThrow('API Error: Unauthorized') }) }) + + // The unit tests for url-helpers cover the rewrite mechanism in + // isolation; this case covers it end-to-end at the tool boundary, + // which is the layer that previously regressed and shipped prod + // links from a staging-targeted server. + describe('staging baseUrl rewrites tool output', () => { + afterEach(() => { + configureBaseUrl(undefined) + }) + + it('rewrites threadUrl and unreadThreads[*].url to the configured host', async () => { + configureBaseUrl('https://comms.staging.todoist.com') + + mockCommsApi.inbox.getInbox.mockResolvedValue([makeInboxThread()]) + mockCommsApi.inbox.getCount.mockResolvedValue(1) + mockCommsApi.threads.getUnread.mockResolvedValue({ + data: [ + { + threadId: TEST_IDS.THREAD_1, + channelId: TEST_IDS.CHANNEL_1, + objIndex: 1, + directMention: false, + }, + ], + version: 1, + }) + mockCommsApi.conversations.getUnread.mockResolvedValue({ data: [], version: 1 }) + mockCommsApi.channels.getChannel.mockResolvedValue(makeChannel()) + + const result = await fetchInbox.execute( + { workspaceId: TEST_IDS.WORKSPACE_1, limit: 50, onlyUnread: false }, + mockCommsApi, + ) + + const { structuredContent } = result + const stagingThreadUrl = `https://comms.staging.todoist.com/a/${TEST_IDS.WORKSPACE_1}/ch/${TEST_IDS.CHANNEL_1}/t/${TEST_IDS.THREAD_1}/` + expect(structuredContent?.threads?.[0]?.threadUrl).toBe(stagingThreadUrl) + // unreadThreads is the raw SDK array; its `.url` must also + // be rewritten or the structured payload is inconsistent. + expect(structuredContent?.unreadThreads?.[0]?.url).toBe(stagingThreadUrl) + }) + }) }) From 5ea61e4f0bc30a855f4942b149693b641d2470f1 Mon Sep 17 00:00:00 2001 From: Amir Date: Fri, 22 May 2026 16:35:46 +0200 Subject: [PATCH 13/13] test(url-helpers): decouple wrapper tests from SDK URL format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Doistbot flagged that hardcoding the SDK's URL format in test assertions makes them brittle to internal SDK changes. The wrapper is what's under test, not the format — so: - getFullCommsURL no-baseUrl: compare against sdkGetFullCommsURL(). - getFullCommsURL with-staging: assert the staging host is the prod host swapped, deriving the expected URL from the SDK output. Adds a sanity check that the SDK still emits prod-hosted URLs — if that ever flips, the rewrite layer needs revisiting and this test will say so. - Relative-URL helpers: compare against sdkGetMessageURL / sdkGetCommentURL. Tests for `toRelativeCommsURL` and `rewriteToConfiguredHost` keep their hardcoded inputs because they test MY regex against MY inputs; no SDK coupling there. --- src/utils/__tests__/url-helpers.test.ts | 36 ++++++++++++++++--------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/utils/__tests__/url-helpers.test.ts b/src/utils/__tests__/url-helpers.test.ts index 0fd5720..dd6cbb4 100644 --- a/src/utils/__tests__/url-helpers.test.ts +++ b/src/utils/__tests__/url-helpers.test.ts @@ -1,3 +1,8 @@ +import { + getCommentURL as sdkGetCommentURL, + getFullCommsURL as sdkGetFullCommsURL, + getMessageURL as sdkGetMessageURL, +} from '@doist/comms-sdk' import { configureBaseUrl, getCommentURL, @@ -13,14 +18,22 @@ describe('url-helpers', () => { }) describe('getFullCommsURL', () => { - it('returns prod URL when no baseUrl is configured', () => { - expect(getFullCommsURL({ workspaceId: 1 })).toBe('https://comms.todoist.com/a/1/') + it('matches the SDK output when no baseUrl is configured', () => { + // Wrapper must be a no-op so the prod default behavior is + // exactly the SDK's. Compare against the SDK to avoid pinning + // a specific URL format here. + expect(getFullCommsURL({ workspaceId: 1 })).toBe(sdkGetFullCommsURL({ workspaceId: 1 })) }) - it('swaps the host when a staging baseUrl is configured', () => { + it('swaps the prod host for the configured host on a full URL', () => { configureBaseUrl('https://comms.staging.todoist.com') + const sdkUrl = sdkGetFullCommsURL({ workspaceId: 1 }) + // Sanity: the SDK still hardcodes the prod host — that's the + // load-bearing assumption of the rewrite layer. If this ever + // fails the wrapper needs revisiting. + expect(sdkUrl).toMatch(/^https:\/\/comms\.todoist\.com\//) expect(getFullCommsURL({ workspaceId: 1 })).toBe( - 'https://comms.staging.todoist.com/a/1/', + sdkUrl.replace('https://comms.todoist.com', 'https://comms.staging.todoist.com'), ) }) }) @@ -29,22 +42,21 @@ describe('url-helpers', () => { // The SDK's `getMessageURL`/`getCommentURL` return relative paths. // applyBaseUrl's regex only matches a leading prod host, so the // wrapped versions must be no-ops on relative paths regardless of - // whether staging is configured. This catches "wrapper accidentally - // prepends/mangles a host on relative URLs." + // whether staging is configured. Compare against the SDK output + // directly so this stays correct if the SDK changes its URL + // format — the wrapper is what's under test, not the format. const messageArgs = { workspaceId: 1, conversationId: 'c', messageId: 'm' } const commentArgs = { workspaceId: 1, channelId: 'ch', threadId: 't', commentId: 'cm' } - const messageRelative = '/a/1/msg/c/m/m' - const commentRelative = '/a/1/ch/ch/t/t/c/cm' it('pass relative paths through unchanged with no baseUrl', () => { - expect(getMessageURL(messageArgs)).toBe(messageRelative) - expect(getCommentURL(commentArgs)).toBe(commentRelative) + expect(getMessageURL(messageArgs)).toBe(sdkGetMessageURL(messageArgs)) + expect(getCommentURL(commentArgs)).toBe(sdkGetCommentURL(commentArgs)) }) it('pass relative paths through unchanged with staging configured', () => { configureBaseUrl('https://comms.staging.todoist.com') - expect(getMessageURL(messageArgs)).toBe(messageRelative) - expect(getCommentURL(commentArgs)).toBe(commentRelative) + expect(getMessageURL(messageArgs)).toBe(sdkGetMessageURL(messageArgs)) + expect(getCommentURL(commentArgs)).toBe(sdkGetCommentURL(commentArgs)) }) })