fix: wire COMMS_BASE_URL through MCP entry + URL output#2
Open
amix wants to merge 13 commits into
Open
Conversation
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 <baseUrl>` 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.
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.
doistbot
reviewed
May 22, 2026
Member
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR successfully wires up the base URL configuration throughout the MCP entry points and rewrites SDK-populated URLs, enabling proper support for staging environments and improving secrets management documentation. These changes are a great step toward making the tooling environment-agnostic, though the current module-global state approach introduces some architectural risks for direct-tool consumers and concurrent instances. A few adjustments have been suggested, including binding the base URL to the server instance, applying the URL rewrite helper consistently across all mutation tools, handling trailing slashes, and updating the logging and setup instructions to fully align with Doist's observability and internal AI standards.
…okens 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://<staging>.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.
…read/update-object 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.
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.
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.
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".
`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.
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.
Collaborator
This should probably be fixed in the SDK, rather than hacked in here. I'm making the relevant fix to the Twist SDK, I can port over the fix to the MCP one too. |
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.
Member
Author
|
Roger that 👌
…On Fri, May 22, 2026 at 4:32 PM Scott Lovegrove ***@***.***> wrote:
*scottlovegrove* left a comment (Doist/comms-mcp#2)
<#2 (comment)>
The SDK's URL helpers and entity .url fields also hardcode the prod host,
so wiring baseUrl alone still leaked prod links into tool output.
This should probably be fixed in the SDK, rather than hacked in here. I'm
making the relevant fix to the Twist SDK, I can port over the fix to the
MCP one too.
—
Reply to this email directly, view it on GitHub
<#2 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABNBDQB4BFZ3YYKRZHXTZL44BQH3AVCNFSM6AAAAACZJAUI5KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DKMJZGYYTKMZYGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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
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.
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.
Collaborator
@amix 0.2.1 of the sdk is available now that has this fix in. All entities' urls will now respect whatever the baseurl is. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
PR #1 added
COMMS_BASE_URLsupport butmain.tsnever read it — the MCP always builtCommsApiagainst prod, so staging tokens 403'd. The SDK's URL helpers and entity.urlfields also hardcode the prod host, so wiring baseUrl alone still leaked prod links into tool output.What was changed
main.tsreadsCOMMS_BASE_URL; logsComms MCP targeting <baseUrl>on startup for fast debugging.src/utils/url-helpers.tswraps the SDK URL helpers;getMcpServercallsconfigureBaseUrlso every entry point (CLI, importable tools,scripts/run-tool.ts) is covered.fetch-inbox/load-thread/load-conversationrewrite SDK-populatedentity.urlfields so staging deployments return staging links.-e KEY=VALto a wrapper-script pattern — secret never reachesclaude mcp add's argv (Doist Secrets Management).server-optionsandurl-helpers(9 total).Out of scope
Upstream fix in
@doist/comms-sdkso its helpers and entity schemas accept a runtime baseUrl. Until then this PR does string substitution against the hardcoded prod host.Refs
Targets
amix/comms-mcp-rename(#1) until that lands onmain.