feat: auto-discover inboxes on connect (v1.6.0) - #7
Conversation
- Add inbox auto-discovery during MCP handshake with server instructions - Implement async factory pattern for HelpScoutMCPServer - Add multi-status search default (active, pending, closed) with Promise.allSettled - Optimize startup by skipping testConnection when inbox discovery succeeds - Sanitize error messages in fallback instructions to prevent token leakage - Deprecate searchInboxes and listAllInboxes tools (inboxes now in instructions) - Update best practices prompt for new workflow - Add migration documentation for breaking changes
📝 WalkthroughWalkthroughBumps project to v1.6.0; adds Inbox Auto-Discovery during server creation via a new async factory Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HelpScoutMCPServer
participant HelpScoutAPI
participant Instructions
Client->>HelpScoutMCPServer: await HelpScoutMCPServer.create()
HelpScoutMCPServer->>HelpScoutAPI: GET /mailboxes (inbox discovery)
HelpScoutAPI-->>HelpScoutMCPServer: mailboxes list (names & IDs)
HelpScoutMCPServer->>Instructions: Build server instructions with discovered inbox IDs
Instructions-->>HelpScoutMCPServer: instructions string
HelpScoutMCPServer-->>Client: resolved instance (with discoveredInboxes)
sequenceDiagram
participant LLMAgent
participant searchConversations
participant HelpScoutAPI
participant ResultMerger
LLMAgent->>searchConversations: query (no status)
par parallel status searches
searchConversations->>HelpScoutAPI: GET /conversations?status=active
searchConversations->>HelpScoutAPI: GET /conversations?status=pending
searchConversations->>HelpScoutAPI: GET /conversations?status=closed
end
HelpScoutAPI-->>searchConversations: results (each status)
searchConversations->>ResultMerger: merge & dedupe by conversationId
ResultMerger->>ResultMerger: sort by createdAt desc
ResultMerger->>ResultMerger: apply requested size limit
ResultMerger-->>searchConversations: merged results + statusesSearched
searchConversations-->>LLMAgent: return merged results and statusesSearched
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/__tests__/index.test.ts:
- Around line 414-433: The test assigns an unused variable "server" from
HelpScoutMCPServer.create() which triggers the CI failure; fix it by removing
the unused binding and just awaiting the call (replace "const server = await
HelpScoutMCPServer.create();" with "await HelpScoutMCPServer.create();") so the
create() side effects run but no unused variable remains; ensure this change is
applied in the failing test block that asserts logger.warn and Server mock
instructions.
🧹 Nitpick comments (6)
src/prompts/index.ts (2)
254-254: Inconsistency with v1.6.0 inbox-discovery workflow.This prompt still instructs agents to use
searchInboxesto get inbox IDs when an inbox is mentioned by name. However, the updatedhelpscout-best-practicesprompt now directs agents to use inbox IDs from server instructions (auto-discovered on connect).Consider updating this prompt to align with the new workflow, or note that
searchInboxesis deprecated and server instructions should be preferred.♻️ Suggested update
-3. ${inboxId ? '' : 'IMPORTANT: If the user mentioned a specific inbox by name, you MUST first use "searchInboxes" to get the inbox ID.\n\n4. '}Search for conversations using the "searchConversations" tool with these parameters: +3. ${inboxId ? '' : 'IMPORTANT: If the user mentioned a specific inbox by name, check the server instructions for auto-discovered inbox IDs.\n\n4. '}Search for conversations using the "searchConversations" tool with these parameters:
306-306: Same inconsistency with inbox-discovery workflow.Similar to
searchLast7Days, this prompt instructs usingsearchInboxeswhich is deprecated in v1.6.0. Consider updating to reference server instructions for consistency.♻️ Suggested update
-2. ${inboxId ? '' : 'CRITICAL: If the user mentioned a specific inbox by name (e.g., "support inbox"), you MUST first use "searchInboxes" to get the inbox ID.\n\n3. '}Search for conversations with urgent-related tags using the "searchConversations" tool.${timeFilter} +2. ${inboxId ? '' : 'CRITICAL: If the user mentioned a specific inbox by name (e.g., "support inbox"), check the server instructions for auto-discovered inbox IDs.\n\n3. '}Search for conversations with urgent-related tags using the "searchConversations" tool.${timeFilter}README.md (1)
165-170: Add language specifier to fenced code block.The static analysis tool flagged this code block as missing a language specification. Since this shows example server instructions (plain text output), use
textorplaintextas the language identifier.📝 Suggested fix
-``` +```text ## Available Inboxes (3 total) - "Support Inbox" (ID: 12345) - "Sales Inquiries" (ID: 67890) - "Billing Questions" (ID: 24680)</details> </blockquote></details> <details> <summary>src/tools/index.ts (1)</summary><blockquote> `635-637`: **Consider extracting the default limit constant.** The fallback `|| 50` appears twice and matches `DEFAULT_PAGE_SIZE`. Using the constant would improve maintainability. <details> <summary>♻️ Suggested improvement</summary> ```diff - if (conversations.length > (input.limit || 50)) { - conversations = conversations.slice(0, input.limit || 50); + const effectiveLimit = input.limit || TOOL_CONSTANTS.DEFAULT_PAGE_SIZE; + if (conversations.length > effectiveLimit) { + conversations = conversations.slice(0, effectiveLimit); }src/index.ts (2)
66-67:validateConfig()is called twice during normal startup.
validateConfig()is called indiscoverAndBuildInstructions()(line 67) and again instart()(line 179). While config validation is idempotent and fast, the duplication is unnecessary.Consider removing the call in
start()since discovery (which always runs first viacreate()) already validates, or document why the redundancy is intentional (e.g., if someone callsstart()on a manually constructed instance in future).Also applies to: 178-179
97-99: UUID redaction in error messages should be reconsidered as it removes useful support reference IDs.The regex
[A-Za-z0-9_-]{20,}will redact Help Scout'slogRef(UUID format, 36 characters with hyphens), which is essential debugging information users and support need to reference—it's not a credential. Since Help Scout API tokens and keys appear in Authorization headers rather than error messages, the broad 20+ character matching is overly aggressive. The second pattern\/[^\s]+/gis similarly broad but unlikely to match Help Scout API error content.Consider using a more targeted approach that specifically matches known token/key patterns (e.g., Bearer token format
Bearer [a-zA-Z0-9._-]+, Basic auth patterns) rather than all 20+ character strings. Test against actual Help Scout error responses to ensure UUIDs and other legitimate context are preserved.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
DockerfileREADME.mdhelpscout-mcp-extension/manifest.jsonmcp.jsonpackage.jsonsrc/__tests__/index.test.tssrc/__tests__/prompts.test.tssrc/__tests__/tools.test.tssrc/index.tssrc/prompts/index.tssrc/tools/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/__tests__/index.test.ts (3)
src/index.ts (1)
HelpScoutMCPServer(21-233)src/utils/helpscout-client.ts (1)
helpScoutClient(533-533)src/utils/logger.ts (1)
logger(54-54)
src/index.ts (3)
src/schema/types.ts (1)
Inbox(172-172)src/utils/config.ts (1)
validateConfig(65-103)src/utils/helpscout-client.ts (1)
PaginatedResponse(50-62)
🪛 GitHub Actions: CI/CD Pipeline
src/__tests__/index.test.ts
[error] 422-422: eslint: '@typescript-eslint/no-unused-vars' - 'server' is assigned a value but never used.
🪛 GitHub Check: test (18.x)
src/__tests__/index.test.ts
[failure] 422-422:
'server' is assigned a value but never used
🪛 GitHub Check: test (20.x)
src/__tests__/index.test.ts
[failure] 422-422:
'server' is assigned a value but never used
🪛 markdownlint-cli2 (0.18.1)
README.md
165-165: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (18)
mcp.json (1)
4-4: LGTM!Version bump to 1.6.0 is consistent with the other manifest files in this PR.
Dockerfile (1)
56-59: LGTM!Docker image metadata version label is consistent with the v1.6.0 release.
package.json (1)
3-3: LGTM!Package version bump to 1.6.0 aligns with the release across all manifests.
src/__tests__/prompts.test.ts (1)
134-142: LGTM!Test expectations correctly reflect the updated prompt content that emphasizes inbox auto-discovery and server instructions over the previous "Golden Rule" workflow.
src/__tests__/tools.test.ts (1)
1070-1137: LGTM!Test correctly validates the new v1.6.0 multi-status search behavior:
- Mocks all three status endpoints (active, pending, closed)
- Verifies
statusesSearchedincludes all statuses when no explicit status is providedhelpscout-mcp-extension/manifest.json (2)
5-5: LGTM!Version bump is consistent with the coordinated v1.6.0 release.
54-60: LGTM!The updated description correctly reflects the new auto-discovery behavior, guiding users that inbox IDs are now automatically discovered on connect rather than requiring manual lookup via
listAllInboxes.src/prompts/index.ts (1)
119-238: LGTM!The updated best practices prompt effectively reflects the new v1.6.0 workflow:
- Emphasizes auto-discovered inboxes available in server instructions
- Provides clear guidance on matching inbox names to IDs
- Correctly notes that searches now include all statuses by default
- Includes helpful disambiguation guidance for ambiguous inbox names
README.md (1)
20-42: Clear and comprehensive release notes.The v1.6.0 changelog is well-structured with clear feature bullets, previous release context, and explicit migration guidance for the breaking async factory pattern change. The note that "No action required for most users" appropriately sets expectations for MCP protocol consumers vs. programmatic users.
src/__tests__/index.test.ts (3)
132-144: Good coverage of inbox discovery integration.The test properly verifies that:
helpScoutClient.getis called with/mailboxesduring creation- The Server receives instructions containing the discovered inbox name
This validates the core auto-discovery contract.
164-172: Correctly validates the optimized startup flow.The test verifies that
testConnectionis skipped when inbox discovery succeeds, confirming the optimization that avoids redundant API calls. The updated log message assertion matches the new behavior.
184-202: Test correctly simulates the fallback path.The test properly mocks
getto reject (discovery failure), which triggers the fallback indiscoverAndBuildInstructions()that returns emptyinboxes: []. Thenstart()detects emptydiscoveredInboxesand callstestConnection(), which is mocked to return false. This correctly exercises the intended flow: discovery failure → fallback with empty inboxes → connection test during start → startup failure with process.exit(1).src/tools/index.ts (3)
88-88: Clear deprecation messaging.The deprecation notice effectively communicates that inboxes are now auto-discovered on connect and provides clear guidance on when this tool might still be useful (long sessions needing refresh).
589-651: Well-designed multi-status parallel search with graceful degradation.Good implementation choices:
Promise.allSettledensures partial failures don't abort the entire search- Deduplication by
conv.idprevents duplicate results across statuses- Failed statuses are logged and excluded from
searchedStatuses- Client-side sorting maintains consistent ordering after merge
The pagination note correctly informs consumers that merged results have different semantics than single-status queries.
684-704: Response structure updated appropriately for multi-status search.The
statusesSearchedarray replaces the singularstatusfield, correctly reflecting the new multi-status default behavior. The search guidance messages are helpful for troubleshooting empty results.src/index.ts (3)
52-58: Clean async factory pattern implementation.The static
create()method correctly encapsulates the async discovery phase, stores discovered inboxes on the instance, and provides clear logging. This is a well-documented breaking change that enables the auto-discovery feature.
64-109: Inbox discovery with secure fallback handling.The implementation correctly:
- Validates config before API calls
- Builds dynamic instructions with discovered inbox list
- Falls back gracefully if discovery fails
- Sanitizes error messages to prevent credential leakage
182-199: Smart optimization: skip connection test when discovery succeeded.This correctly avoids a redundant API call when inbox discovery already proved connectivity. The conditional logging clearly distinguishes between the two code paths.
- Add workflow table to server instructions for better tool selection - Reduce tool descriptions by ~60% (shorter, verb+resource format) - Reduce argument descriptions by ~62% - Add structuredConversationFilter to mcp.json - Add 29 edge case and stress tests for v1.6 features Total token reduction: ~48% for MCP Tool Search indexing
- Remove unused 'server' variable in index.test.ts - Fix unused import in v1.6-edge-cases.test.ts - Remove unused 'response' variable in v1.6-stress.test.ts - Prefix unused forEach params with underscore - Remove unnecessary escape characters
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/__tests__/v1.6-edge-cases.test.ts`:
- Around line 74-88: The test currently imports HelpScoutMCPServer but never
uses it; instantiate HelpScoutMCPServer, start or initialize it (call the
server's start/init method), trigger the inbox discovery flow (e.g., call the
method that performs mailbox discovery or make the same HTTP request the server
would make), await the operation so the nock-delayed response is exercised,
assert that the call resolves without throwing (or that the server handled the
timeout gracefully), and finally stop/close the server; reference
HelpScoutMCPServer, mockOAuthToken, nock, and baseURL to locate where to add the
instantiation/start, trigger of discovery, await/assert, and teardown.
In `@src/__tests__/v1.6-stress.test.ts`:
- Around line 89-93: The test creates an unused variable response from
JSON.parse((result.content[0] as any).text); remove this unused assignment or
use it in an assertion; specifically update the test block that contains the
expect(result).toBeDefined() assertion to either delete the response line or
replace the expect with assertions that validate fields from response (e.g.,
checking parsed values), referencing the variables response and result to locate
the change.
- Around line 228-241: The test "should handle very long query strings" has an
unused forEach callback parameter `status` causing a lint warning; either rename
it to `_status` to indicate intentional non-use or, better, use it to make the
nock expectations specific by matching the status query param on the mocked
endpoint (adjust the nock call on nock(baseURL).get('/conversations').query(...)
to assert the status, e.g., by passing an object or predicate that checks for
the status value), and keep `longQuery` as the search body used in the request
assertions.
- Around line 254-267: The test declares specialQuery with unnecessary escaped
quotes and uses Array.prototype.forEach with an unused arrow parameter named
status; fix by simplifying specialQuery to a normal string (remove redundant
backslash escapes or use a template literal) and replace the forEach callback
with a simple for...of loop (for (const status of ['active','pending','closed'])
{ ... }) so the linter no longer reports an unused variable; make these changes
around the specialQuery declaration and the loop where .forEach(status => { ...
}) is used.
🧹 Nitpick comments (1)
src/index.ts (1)
106-122: Path redaction regex may over-match URLs and other content.The regex
/\/[^\s]+/gwill match any/followed by non-whitespace, which could inadvertently redact parts of URLs (e.g.,https://api.helpscout.net/v2becomeshttps:[PATH]) or other legitimate content in error messages.Consider a more targeted pattern that specifically matches filesystem paths:
♻️ Suggested improvement
const safeError = rawError .replace(/[A-Za-z0-9_-]{20,}/g, '[REDACTED]') // Redact long alphanumeric strings (tokens/keys) - .replace(/\/[^\s]+/g, '[PATH]'); // Redact file paths + .replace(/(?:\/[\w.-]+){2,}/g, '[PATH]'); // Redact filesystem paths (2+ segments)This would match paths like
/Users/secret/configbut preserve URLs and single slashes.
| it('should handle network timeout during inbox discovery', async () => { | ||
| mockOAuthToken(); | ||
|
|
||
| // Simulate timeout | ||
| nock(baseURL) | ||
| .get('/mailboxes') | ||
| .query(true) | ||
| .delayConnection(10000) | ||
| .reply(200, { _embedded: { mailboxes: [] } }); | ||
|
|
||
| const { HelpScoutMCPServer } = await import('../index.js'); | ||
|
|
||
| // Should handle timeout gracefully (may take a while) | ||
| // We're testing that it doesn't crash | ||
| }, 15000); |
There was a problem hiding this comment.
Test is incomplete - no server creation or assertion.
The test imports HelpScoutMCPServer but never uses it. The test should create the server and verify timeout handling.
🔧 Proposed fix
it('should handle network timeout during inbox discovery', async () => {
mockOAuthToken();
// Simulate timeout
nock(baseURL)
.get('/mailboxes')
.query(true)
.delayConnection(10000)
.reply(200, { _embedded: { mailboxes: [] } });
const { HelpScoutMCPServer } = await import('../index.js');
- // Should handle timeout gracefully (may take a while)
- // We're testing that it doesn't crash
+ // Should handle timeout gracefully - server should still be created
+ const server = await HelpScoutMCPServer.create();
+ expect(server).toBeDefined();
}, 15000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should handle network timeout during inbox discovery', async () => { | |
| mockOAuthToken(); | |
| // Simulate timeout | |
| nock(baseURL) | |
| .get('/mailboxes') | |
| .query(true) | |
| .delayConnection(10000) | |
| .reply(200, { _embedded: { mailboxes: [] } }); | |
| const { HelpScoutMCPServer } = await import('../index.js'); | |
| // Should handle timeout gracefully (may take a while) | |
| // We're testing that it doesn't crash | |
| }, 15000); | |
| it('should handle network timeout during inbox discovery', async () => { | |
| mockOAuthToken(); | |
| // Simulate timeout | |
| nock(baseURL) | |
| .get('/mailboxes') | |
| .query(true) | |
| .delayConnection(10000) | |
| .reply(200, { _embedded: { mailboxes: [] } }); | |
| const { HelpScoutMCPServer } = await import('../index.js'); | |
| // Should handle timeout gracefully - server should still be created | |
| const server = await HelpScoutMCPServer.create(); | |
| expect(server).toBeDefined(); | |
| }, 15000); |
🧰 Tools
🪛 GitHub Check: test (18.x)
[failure] 84-84:
'HelpScoutMCPServer' is assigned a value but never used
🪛 GitHub Check: test (20.x)
[failure] 84-84:
'HelpScoutMCPServer' is assigned a value but never used
🤖 Prompt for AI Agents
In `@src/__tests__/v1.6-edge-cases.test.ts` around lines 74 - 88, The test
currently imports HelpScoutMCPServer but never uses it; instantiate
HelpScoutMCPServer, start or initialize it (call the server's start/init
method), trigger the inbox discovery flow (e.g., call the method that performs
mailbox discovery or make the same HTTP request the server would make), await
the operation so the nock-delayed response is exercised, assert that the call
resolves without throwing (or that the server handled the timeout gracefully),
and finally stop/close the server; reference HelpScoutMCPServer, mockOAuthToken,
nock, and baseURL to locate where to add the instantiation/start, trigger of
discovery, await/assert, and teardown.
1 participant
Summary
This PR implements inbox auto-discovery on server connect and optimizes tool descriptions for MCP Tool Search compliance.
Key Changes
Inbox Auto-Discovery
searchConversationsnow searches all statuses (active, pending, closed) by default usingPromise.allSettledfor resilient partial failure handlingHelpScoutMCPServerrefactored to useawait HelpScoutMCPServer.create()instead of constructortestConnection()when inbox discovery already proved API connectivitysearchInboxesandlistAllInboxesremain functional but deprecatedMCP Tool Search Optimization
structuredConversationFilterto mcp.jsonBreaking Changes
HelpScoutMCPServernow requires async factory:await HelpScoutMCPServer.create()searchConversationsresponse includesstatusesSearchedarray instead ofstatusstring when searching without explicit statusFiles Changed
src/index.tssrc/tools/index.tssrc/prompts/index.tsmcp.jsonsrc/__tests__/v1.6-*.tsREADME.mdTest Plan
npm run build)npm run type-check)Related Issues
Summary by CodeRabbit
New Features
Improvements
Deprecated
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.