Dynamic user agent #85
Conversation
WalkthroughAdds CLIENT_SETTINGS.MCP_USER_AGENT to config, implements getDynamicUserAgent to choose between MCP_USER_AGENT and USER_AGENT based on runtime context, updates MailtrapClient to call getDynamicUserAgent for the User-Agent header, and adds tests for get-agent logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Client as MailtrapClient
participant Agent as getDynamicUserAgent
participant Config as CONFIG.CLIENT_SETTINGS
participant HTTP as Axios
Caller->>Client: perform operation (e.g., instantiate/request)
Client->>Agent: getDynamicUserAgent()
Agent->>Config: read USER_AGENT and MCP_USER_AGENT
Note over Agent,Config: detect MCP via main module, cwd, call stack
alt MCP context detected
Agent-->>Client: return MCP_USER_AGENT
else Not MCP
Agent-->>Client: return USER_AGENT
end
Client->>HTTP: send request (headers["User-Agent"] = chosen value)
HTTP-->>Client: response
Client-->>Caller: return response/result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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
🧹 Nitpick comments (5)
src/config/index.ts (1)
21-21: Option: include package version in UA for better telemetry.Consider appending the library version (e.g., "mailtrap-mcp/1.2.3 (...)"). Implement in get-agent.ts to keep config static.
src/lib/get-agent.ts (4)
23-30: Make cwd-based detection case-insensitive.Paths can differ in case across platforms; normalize before matching.
Apply this diff:
function isWorkingDirectoryMCP(): boolean { try { const cwd = process.cwd(); - return cwd.includes("mailtrap-mcp") && !cwd.includes("node_modules"); + const lower = cwd.toLowerCase(); + return lower.includes("mailtrap-mcp") && !lower.includes("node_modules"); } catch { return false; } }
36-44: Broaden self-exclusion and normalize case in stack-based detection.Also exclude
node_modules/mailtrap-nodejsto avoid false positives when this lib is on the stack.Apply this diff:
function isCallStackMCP(): boolean { - const { stack } = new Error(); - - return !!( - stack && - stack.includes("mailtrap-mcp") && - !stack.includes("node_modules/mailtrap") - ); + const { stack } = new Error(); + const lower = stack?.toLowerCase(); + return !!( + lower && + lower.includes("mailtrap-mcp") && + !/node_modules\/mailtrap(-nodejs)?/.test(lower) + ); }
59-61: Allow env override and ensure robust fallback.Let users force UA selection and guard against unexpected undefineds.
Apply this diff:
-function getDynamicUserAgent(): string { - return isMailtrapMCPContext() ? MCP_USER_AGENT : USER_AGENT; -} +function getDynamicUserAgent(): string { + const override = process.env.MAILTRAP_UA?.toLowerCase(); + if (override === "mcp") return MCP_USER_AGENT; + if (override === "default") return USER_AGENT; + const ua = isMailtrapMCPContext() ? MCP_USER_AGENT : USER_AGENT; + return ua || "mailtrap-nodejs"; +}
59-61: Optional: add version to UA string.If
resolveJsonModuleis enabled, append package version for traceability (example implementation):// at top-level (outside selected range): // import { version } from "../../package.json"; // inside getDynamicUserAgent(): // const base = isMailtrapMCPContext() ? MCP_USER_AGENT : USER_AGENT; // return `${base}/${version}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/config/index.ts(1 hunks)src/lib/MailtrapClient.ts(2 hunks)src/lib/get-agent.ts(1 hunks)
🔇 Additional comments (4)
src/config/index.ts (1)
21-21: LGTM: adds MCP-specific User-Agent.Field name and value look consistent with existing config keys.
src/lib/MailtrapClient.ts (2)
9-9: LGTM: importing dynamic UA helper.
69-70: LGTM: apply dynamic User-Agent header.Header is set once per client instance, which is appropriate.
src/lib/get-agent.ts (1)
1-63: CommonJS-only packaging detected; no ESM support found.
package.json “type” is absent (defaults to CJS), tsconfig module=commonjs, and no ESM exports fields—no changes needed.
| function isMainModuleMCP(): boolean { | ||
| const mainFile = require?.main?.filename; | ||
|
|
||
| return !!( | ||
| mainFile && | ||
| mainFile.includes("mailtrap-mcp") && | ||
| !mainFile.includes("node_modules") | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix ESM ReferenceError risk from using require directly.
require?.main still throws in pure ESM because require is an unbound identifier. Guard with typeof require !== 'undefined' and fall back to process.argv[1].
Apply this diff:
function isMainModuleMCP(): boolean {
- const mainFile = require?.main?.filename;
-
- return !!(
- mainFile &&
- mainFile.includes("mailtrap-mcp") &&
- !mainFile.includes("node_modules")
- );
+ const mainFile =
+ (typeof require !== "undefined" ? require.main?.filename : undefined) ??
+ process.argv?.[1];
+ if (!mainFile) return false;
+ const lower = mainFile.toLowerCase();
+ return lower.includes("mailtrap-mcp") && !lower.includes("node_modules");
}🤖 Prompt for AI Agents
In src/lib/get-agent.ts around lines 9 to 17, avoid referencing the unbound
require in pure ESM by first checking typeof require !== 'undefined' and using
require.main?.filename only when defined, otherwise fall back to
process.argv[1]; replace const mainFile = require?.main?.filename with a safe
resolution like: if typeof require !== 'undefined' and require.main use
require.main.filename else use process.argv[1], then keep the subsequent
includes checks the same to determine mailtrap-mcp presence and node_modules
exclusion.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
src/__tests__/lib/get-agent.test.ts (12)
41-41: Use jest.spyOn instead of reassigning process.cwdReassigning globals is brittle and hard to restore. Spy on it instead.
Apply:
-process.cwd = jest.fn().mockReturnValue("/path/to/mailtrap-mcp"); +jest.spyOn(process, "cwd").mockReturnValue("/path/to/mailtrap-mcp");
54-56: Use jest.spyOn for process.cwd here as wellRepeat the spy pattern.
Apply:
-process.cwd = jest - .fn() - .mockReturnValue("/path/to/node_modules/mailtrap-mcp"); +jest.spyOn(process, "cwd").mockReturnValue( + "/path/to/node_modules/mailtrap-mcp" +);
68-70: Mock cwd error via spy, not reassignmentThis keeps the original method intact.
Apply:
-process.cwd = jest.fn().mockImplementation(() => { - throw new Error("Permission denied"); -}); +jest.spyOn(process, "cwd").mockImplementation(() => { + throw new Error("Permission denied"); +});
91-91: Same here: prefer a spy for process.cwdApply:
-process.cwd = jest.fn().mockReturnValue("/path/to/regular-app"); +jest.spyOn(process, "cwd").mockReturnValue("/path/to/regular-app");
112-112: Prefer spy over direct assignmentApply:
-process.cwd = jest.fn().mockReturnValue("/path/to/regular-app"); +jest.spyOn(process, "cwd").mockReturnValue("/path/to/regular-app");
133-133: Prefer spy over direct assignmentApply:
-process.cwd = jest.fn().mockReturnValue("/path/to/regular-app"); +jest.spyOn(process, "cwd").mockReturnValue("/path/to/regular-app");
149-149: Prefer spy over direct assignmentApply:
-process.cwd = jest.fn().mockReturnValue("/path/to/regular-app"); +jest.spyOn(process, "cwd").mockReturnValue("/path/to/regular-app");
162-165: Combine both mocks using spies and real ErrorAlso prefer returning a real Error with overridden stack to preserve instanceof and other semantics.
Apply:
-process.cwd = jest.fn().mockImplementation(() => { - throw new Error("Permission denied"); -}); -global.Error = jest.fn().mockImplementation(() => ({ - stack: undefined, -})) as any; +jest.spyOn(process, "cwd").mockImplementation(() => { + throw new Error("Permission denied"); +}); +global.Error = jest.fn().mockImplementation(() => { + const e = new originalError("Test error"); + Object.defineProperty(e, "stack", { value: undefined }); + return e; +}) as any;Tip: apply the “real Error with overridden stack” pattern to other stack-based tests, too (Lines 82-89, 102-109, 124-131, 145-147).
181-183: Prefer spy over direct assignmentApply:
-process.cwd = jest - .fn() - .mockReturnValue("/Users/user/projects/mailtrap-mcp"); +jest.spyOn(process, "cwd").mockReturnValue( + "/Users/user/projects/mailtrap-mcp" +);
195-195: Prefer spy over direct assignmentApply:
-process.cwd = jest.fn().mockReturnValue("/Users/user/projects/my-app"); +jest.spyOn(process, "cwd").mockReturnValue("/Users/user/projects/my-app");
82-89: Return a real Error instance with a mocked stackReturning a plain object breaks Error semantics. Use the original constructor and override stack.
Apply:
-global.Error = jest.fn().mockImplementation(() => ({ - stack: ` -Error: Test error - at Object.<anonymous> (/path/to/mailtrap-mcp/index.js:10:1) - at Module._compile (internal/modules/cjs/loader.js:1063:30) - `, -})) as any; +global.Error = jest.fn().mockImplementation(() => { + const e = new originalError("Test error"); + Object.defineProperty(e, "stack", { + value: ` +Error: Test error + at Object.<anonymous> (/path/to/mailtrap-mcp/index.js:10:1) + at Module._compile (internal/modules/cjs/loader.js:1063:30) + `, + }); + return e; +}) as any;
206-256: Strengthen the integration test: assert getDynamicUserAgent is usedRight now it only checks client is created. Mock get-agent and assert it’s called during client construction. Also align the config mock to default-export shape.
Apply:
- jest.doMock("../../config", () => ({ - CLIENT_SETTINGS: { + jest.doMock("../../config", () => ({ + __esModule: true, + default: { + CLIENT_SETTINGS: { SENDING_ENDPOINT: "https://send.api.mailtrap.io", BULK_ENDPOINT: "https://bulk.api.mailtrap.io", TESTING_ENDPOINT: "https://sandbox.api.mailtrap.io", GENERAL_ENDPOINT: "https://mailtrap.io", USER_AGENT: "mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)", MCP_USER_AGENT: "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)", MAX_REDIRECTS: 0, TIMEOUT: 10000, - }, - ERRORS: { + }, + ERRORS: { FILENAME_REQUIRED: "Filename is required.", CONTENT_REQUIRED: "Content is required.", SUBJECT_REQUIRED: "Subject is required.", FROM_REQUIRED: "From is required.", SENDING_FAILED: "Sending failed.", NO_DATA_ERROR: "No Data.", TEST_INBOX_ID_MISSING: "testInboxId is missing, testing API will not work.", ACCOUNT_ID_MISSING: "accountId is missing, some features of testing API may not work properly.", BULK_SANDBOX_INCOMPATIBLE: "Bulk mode is not applicable for sandbox API.", - }, - TRANSPORT_SETTINGS: { - NAME: "MailtrapTransport", - }, + }, + TRANSPORT_SETTINGS: { + NAME: "MailtrapTransport", + }, + }, })); + const getAgentMock = jest.fn(() => "ua-from-test"); + jest.doMock("../../lib/get-agent", () => ({ + __esModule: true, + default: getAgentMock, + })); + // Clear module cache and re-import jest.resetModules(); const { default: MailtrapClient } = jest.requireActual( "../../lib/MailtrapClient" ); @@ - // The User-Agent should be set in the axios instance - // We can't easily test the internal axios instance, but we can verify - // that the function is called during client creation - expect(client).toBeDefined(); + // Ensure dynamic UA is consulted during client creation + expect(client).toBeDefined(); + expect(getAgentMock).toHaveBeenCalledTimes(1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/__tests__/lib/get-agent.test.ts(1 hunks)src/lib/MailtrapClient.ts(3 hunks)src/lib/get-agent.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/get-agent.ts
- src/lib/MailtrapClient.ts
🔇 Additional comments (1)
src/__tests__/lib/get-agent.test.ts (1)
29-39: Good baseline coverage and scenariosNice coverage of defaults, cwd-based, call stack-based, and error edge cases.
| jest.mock("../../config", () => ({ | ||
| CLIENT_SETTINGS: { | ||
| USER_AGENT: | ||
| "mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)", | ||
| MCP_USER_AGENT: "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)", | ||
| }, | ||
| })); |
There was a problem hiding this comment.
Mock the default export shape of config to avoid interop breakage
The test imports CONFIG as a default export, but the mock returns a plain object. Under ts-jest/Babel interop, default may be undefined. Return { __esModule: true, default: ... }.
Apply this diff:
-jest.mock("../../config", () => ({
- CLIENT_SETTINGS: {
- USER_AGENT:
- "mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)",
- MCP_USER_AGENT: "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)",
- },
-}));
+jest.mock("../../config", () => ({
+ __esModule: true,
+ default: {
+ CLIENT_SETTINGS: {
+ USER_AGENT:
+ "mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)",
+ MCP_USER_AGENT:
+ "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)",
+ },
+ },
+}));📝 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.
| jest.mock("../../config", () => ({ | |
| CLIENT_SETTINGS: { | |
| USER_AGENT: | |
| "mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)", | |
| MCP_USER_AGENT: "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)", | |
| }, | |
| })); | |
| jest.mock("../../config", () => ({ | |
| __esModule: true, | |
| default: { | |
| CLIENT_SETTINGS: { | |
| USER_AGENT: | |
| "mailtrap-nodejs (https://github.com/railsware/mailtrap-nodejs)", | |
| MCP_USER_AGENT: | |
| "mailtrap-mcp (https://github.com/railsware/mailtrap-mcp)", | |
| }, | |
| }, | |
| })); |
🤖 Prompt for AI Agents
In src/__tests__/lib/get-agent.test.ts around lines 4 to 10, the jest.mock
currently returns a plain object which breaks default-import interop under
ts-jest/Babel; update the mock to export a proper ES module shape by returning
an object with __esModule: true and a default property containing the original
CLIENT_SETTINGS object so the imported CONFIG default is defined.
| let originalCwd: string; | ||
| let originalError: typeof Error; | ||
|
|
||
| beforeEach(() => { | ||
| // Store original values | ||
| originalCwd = process.cwd(); | ||
| originalError = global.Error; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| // Restore original values | ||
| process.cwd = jest.fn().mockReturnValue(originalCwd); | ||
| global.Error = originalError; | ||
| jest.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
Restore the real process.cwd; don’t replace it with a mock
You’re saving the cwd value (string) and then replacing process.cwd with a mock that returns that string. This leaks a mocked function into global state across the suite.
Apply this minimal fix (and prefer spy-based mocking in tests):
- let originalCwd: string;
+ let originalCwdFn: typeof process.cwd;
let originalError: typeof Error;
beforeEach(() => {
// Store original values
- originalCwd = process.cwd();
+ originalCwdFn = process.cwd;
originalError = global.Error;
});
afterEach(() => {
// Restore original values
- process.cwd = jest.fn().mockReturnValue(originalCwd);
+ process.cwd = originalCwdFn;
global.Error = originalError;
- jest.clearAllMocks();
+ jest.restoreAllMocks();
});Follow-up (recommended): replace all direct assignments to process.cwd in tests with jest.spyOn(process, "cwd").mockReturnValue(...).
📝 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.
| let originalCwd: string; | |
| let originalError: typeof Error; | |
| beforeEach(() => { | |
| // Store original values | |
| originalCwd = process.cwd(); | |
| originalError = global.Error; | |
| }); | |
| afterEach(() => { | |
| // Restore original values | |
| process.cwd = jest.fn().mockReturnValue(originalCwd); | |
| global.Error = originalError; | |
| jest.clearAllMocks(); | |
| }); | |
| let originalCwdFn: typeof process.cwd; | |
| let originalError: typeof Error; | |
| beforeEach(() => { | |
| // Store original values | |
| originalCwdFn = process.cwd; | |
| originalError = global.Error; | |
| }); | |
| afterEach(() => { | |
| // Restore original values | |
| process.cwd = originalCwdFn; | |
| global.Error = originalError; | |
| jest.restoreAllMocks(); | |
| }); |
🤖 Prompt for AI Agents
In src/__tests__/lib/get-agent.test.ts around lines 13-27, the test saves only
the cwd string and then replaces process.cwd with a mock that returns that
string, leaking a mocked function into global state; change the setup to save
the original process.cwd function (e.g., originalCwdFn = process.cwd) and in
afterEach restore the original function (process.cwd = originalCwdFn) instead of
assigning a jest.fn(), and, going forward, use jest.spyOn(process,
"cwd").mockReturnValue(...) when mocking cwd in tests so the spy can be restored
automatically.
| * Gets the appropriate User-Agent string based on the current context. | ||
| * @returns The User-Agent string for the current context | ||
| */ | ||
| function getDynamicUserAgent(): string { |
There was a problem hiding this comment.
If you ask me, bringing the MCP concern to the SDK project is a violation of the SRP 🙂 I would approach this differently and allow specifying the user agent directly via a configuration setting of the SDK client. And, override this setting in the MCP server NPM package.
It's simpler, and it's not a big deal to allow users of the SDK to override the user agent; it's fairly easy to do this anyway.
WDYT?
There was a problem hiding this comment.
if allowing users to override user agent is not a problem, then its simple solution. I thought that we don't want to enable users to change the user agent.
There was a problem hiding this comment.
I agree with @vittorius, it's cleaner to allow to override user agent, and it might be useful for other purposes (for the users), too.
Motivation
The Mailtrap Node.js SDK needed to automatically detect when it's running within a Mailtrap MCP (Model Context Protocol) environment and use an appropriate User-Agent string. This improves environment detection, integration tracking, and provides better analytics for MCP-specific usage patterns.
Changes
mailtrap-nodejsandmailtrap-mcpbased on runtime contextMCP_USER_AGENTconfiguration option to support MCP-specific User-Agent stringMailtrapClientto use dynamic User-Agent selection instead of static configurationHow to test
npm test -- src/__tests__/lib/get-agent.test.tsto verify all 12 unit tests passmailtrap-nodejsUser-Agentmailtrap-mcpUser-Agentprocess.cwd()throws exceptionsSummary by CodeRabbit
New Features
Refactor
Tests