More sandbox tools#68
Merged
Merged
Conversation
…nboxes CRUD and clean
mklocek
requested review from
IgorDobryn,
VladimirTaytor,
leonid-shevtsov and
piobeny
📝 WalkthroughWalkthroughAdds a new exported helper Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ToolModule
participant RequireClient
participant MailtrapAPI
User->>ToolModule: invoke tool (e.g., createSandboxInbox)
ToolModule->>RequireClient: requireClient("sandbox inboxes")
RequireClient->>RequireClient: validate MAILTRAP_API_TOKEN (+ optional ACCOUNT_ID)
alt validation OK
RequireClient-->>ToolModule: Mailtrap client instance
ToolModule->>MailtrapAPI: mailtrap.testing.inboxes.create(...)
MailtrapAPI-->>ToolModule: API response
ToolModule-->>User: formatted success content
else validation fails
RequireClient-->>ToolModule: throw env error
ToolModule-->>User: error content (isError: true)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 8
🧹 Nitpick comments (3)
src/tools/sandbox/schemas/createSandboxInbox.ts (1)
4-5: Constrainproject_idto integer values.Using
type: "number"allows fractional IDs (e.g.1.5). For ID fields, schema-level integer validation avoids avoidable downstream API errors.Suggested patch
project_id: { - type: "number", + type: "integer", + minimum: 1, description: "ID of the project to create the inbox in", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/sandbox/schemas/createSandboxInbox.ts` around lines 4 - 5, The schema's project_id field currently uses type: "number" which permits non-integer values; update the createSandboxInbox schema so project_id is constrained to integers (e.g., use type: "integer" or add an integer validation rule) ensuring the field only accepts whole-number IDs; modify the project_id entry in the exported schema object in createSandboxInbox.ts accordingly so downstream APIs receive only integer IDs.src/tools/sandbox/updateSandboxInbox.ts (1)
29-32: Type assertion is overly strict.The cast
params as { name: string; emailUsername: string }implies both fields are always present, but the logic only guarantees at least one. This won't cause runtime issues but misrepresents the actual type.🔧 Suggested fix
const inbox = await mailtrap.testing.inboxes.updateInbox( inbox_id, - params as { name: string; emailUsername: string } + params as { name?: string; emailUsername?: string } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/sandbox/updateSandboxInbox.ts` around lines 29 - 32, The current cast forces both fields to be present when calling mailtrap.testing.inboxes.updateInbox, but only one or the other may exist; change the call to pass a partial/optional payload instead of asserting both fields exist—e.g., build a payload object from params containing only present keys or cast to Partial<{ name: string; emailUsername: string }>, and pass that to mailtrap.testing.inboxes.updateInbox (referencing inbox_id, params, and the updateInbox call).src/tools/sendingDomains/__tests__/createSendingDomain.test.ts (1)
10-12: Minor style inconsistency in mock setup.The mock factory includes
() => mockClienton line 11, whilebeforeEachon line 20 also callsmockReturnValue(mockClient). Other test files in this PR (e.g.,createTemplate.test.ts,getSendingStats.test.ts) use justjest.fn()in the mock factory and rely solely onbeforeEachto set the return value. Consider aligning for consistency:♻️ Optional: Align with other test files
jest.mock("../../../client", () => ({ - requireClient: jest.fn(() => mockClient), + requireClient: jest.fn(), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/sendingDomains/__tests__/createSendingDomain.test.ts` around lines 10 - 12, Change the mock factory in the jest.mock call so it returns a plain jest.fn() for requireClient instead of `() => mockClient`, and then continue to set its return value in the test setup (the existing beforeEach that calls `mockReturnValue(mockClient)`); update the mock declaration for requireClient in the jest.mock block to `requireClient: jest.fn()` so the test follows the same pattern as createTemplate.test.ts and getSendingStats.test.ts and relies on beforeEach's mockReturnValue to provide mockClient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/sandbox/__tests__/cleanSandboxInbox.test.ts`:
- Around line 25-27: afterEach currently uses Object.assign(process.env,
originalEnv) which only overwrites existing keys and leaves any env vars added
by tests behind; update the afterEach to fully restore the environment by (1)
iterating over Object.keys(process.env) and deleting any key not present in
originalEnv, and then (2) iterating over Object.keys(originalEnv) and setting
process.env[key] = originalEnv[key]; do this inside the same afterEach block
(referencing afterEach, process.env and originalEnv in
cleanSandboxInbox.test.ts) so no test-added env vars leak between tests.
In `@src/tools/sandbox/__tests__/createSandboxInbox.test.ts`:
- Around line 54-56: Update the test to stop asserting raw SMTP credentials by
replacing the plaintext password check on result.content[0].text with a
redaction assertion: ensure the text contains a labeled password field (e.g.,
"SMTP Password") but does not contain the literal "pass456" and instead matches
a masked/placeholder pattern (for example contains "SMTP Password: " followed by
asterisks or "[REDACTED]"); keep the other assertions (SMTP Username and host)
intact and reference the existing result variable and the test in
createSandboxInbox.test.ts that inspects result.content[0].text to implement
this change.
In `@src/tools/sandbox/getSandboxInbox.ts`:
- Around line 35-37: The tool getSandboxInbox is returning inbox.password in
plaintext in the response; remove or redact that secret before returning output
(e.g., omit inbox.password or replace it with a masked value like "***" or
"[REDACTED]") and ensure any place that builds the response (the template
strings including `• SMTP Password: ${inbox.password}`) is updated to use the
masked/omitted value; keep username and domain as needed but do not expose
inbox.password anywhere in the returned string or logs.
- Around line 18-23: The current conversion of inboxIdRaw to resolvedInboxId in
getSandboxInbox allows 0, decimals and empty-string-derived 0; change the
validation to require a positive integer: first trim and reject empty/whitespace
inboxIdRaw, parse to a Number, then ensure Number.isInteger(resolvedInboxId) &&
resolvedInboxId > 0 before proceeding (throw the same error message if it
fails). Update the validation around the resolvedInboxId variable in
getSandboxInbox to perform these checks so only positive integers are accepted.
In `@src/tools/sandbox/schemas/updateSandboxInbox.ts`:
- Around line 4-18: The schema currently allows requests with only inbox_id and
no updates; modify the validation to require at least one update field by adding
an anyOf/oneOf constraint that enforces either "name" or "email_username" be
present (for example add anyOf: [{ required: ["name"] }, { required:
["email_username"] }]) while keeping the existing required: ["inbox_id"] and
additionalProperties: false so requests must include inbox_id and at least one
of the update fields.
In `@src/tools/templates/createTemplate.ts`:
- Line 12: The template client is being created with requireAccountId disabled;
change the requireClient call that instantiates mailtrap in createTemplate.ts
(the symbol "requireClient" and the variable "mailtrap") to enforce account-id
validation by removing or setting the option so requireAccountId is true
(restore the account-id requirement used for
templates/stats/email-logs/sandbox/list-show/sending-domains).
In `@src/tools/templates/deleteTemplate.ts`:
- Line 8: The templates client is being created with requireAccountId: false
which bypasses MAILTRAP_ACCOUNT_ID validation; update the requireClient call
that assigns mailtrap (the line with requireClient("templates", {
requireAccountId: false })) to require the account id (remove the false flag or
set requireAccountId: true) so the MAILTRAP_ACCOUNT_ID guard is enforced for
template operations.
In `@src/tools/templates/listTemplates.ts`:
- Around line 5-7: The templates tool is bypassing MAILTRAP_ACCOUNT_ID
validation by calling requireClient("templates", { requireAccountId: false });
change the call to use the default validation (call requireClient("templates")
with no options) so the account ID check is enforced before invoking
mailtrap.templates.getList(); update any related references in this module that
rely on the previous bypass to assume MAILTRAP_ACCOUNT_ID is present.
---
Nitpick comments:
In `@src/tools/sandbox/schemas/createSandboxInbox.ts`:
- Around line 4-5: The schema's project_id field currently uses type: "number"
which permits non-integer values; update the createSandboxInbox schema so
project_id is constrained to integers (e.g., use type: "integer" or add an
integer validation rule) ensuring the field only accepts whole-number IDs;
modify the project_id entry in the exported schema object in
createSandboxInbox.ts accordingly so downstream APIs receive only integer IDs.
In `@src/tools/sandbox/updateSandboxInbox.ts`:
- Around line 29-32: The current cast forces both fields to be present when
calling mailtrap.testing.inboxes.updateInbox, but only one or the other may
exist; change the call to pass a partial/optional payload instead of asserting
both fields exist—e.g., build a payload object from params containing only
present keys or cast to Partial<{ name: string; emailUsername: string }>, and
pass that to mailtrap.testing.inboxes.updateInbox (referencing inbox_id, params,
and the updateInbox call).
In `@src/tools/sendingDomains/__tests__/createSendingDomain.test.ts`:
- Around line 10-12: Change the mock factory in the jest.mock call so it returns
a plain jest.fn() for requireClient instead of `() => mockClient`, and then
continue to set its return value in the test setup (the existing beforeEach that
calls `mockReturnValue(mockClient)`); update the mock declaration for
requireClient in the jest.mock block to `requireClient: jest.fn()` so the test
follows the same pattern as createTemplate.test.ts and getSendingStats.test.ts
and relies on beforeEach's mockReturnValue to provide mockClient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7724020e-8f8a-425a-a955-192e20a0b4d9
📒 Files selected for processing (53)
src/client.tssrc/server.tssrc/tools/emailLogs/__tests__/getEmailLogMessage.includeContent.test.tssrc/tools/emailLogs/__tests__/getEmailLogMessage.test.tssrc/tools/emailLogs/__tests__/listEmailLogs.test.tssrc/tools/emailLogs/getEmailLogMessage.tssrc/tools/emailLogs/listEmailLogs.tssrc/tools/sandbox/__tests__/cleanSandboxInbox.test.tssrc/tools/sandbox/__tests__/createProject.test.tssrc/tools/sandbox/__tests__/createSandboxInbox.test.tssrc/tools/sandbox/__tests__/deleteProject.test.tssrc/tools/sandbox/__tests__/deleteSandboxInbox.test.tssrc/tools/sandbox/__tests__/getSandboxInbox.test.tssrc/tools/sandbox/__tests__/listProjects.test.tssrc/tools/sandbox/__tests__/updateSandboxInbox.test.tssrc/tools/sandbox/cleanSandboxInbox.tssrc/tools/sandbox/createProject.tssrc/tools/sandbox/createSandboxInbox.tssrc/tools/sandbox/deleteProject.tssrc/tools/sandbox/deleteSandboxInbox.tssrc/tools/sandbox/getSandboxInbox.tssrc/tools/sandbox/index.tssrc/tools/sandbox/listProjects.tssrc/tools/sandbox/schemas/cleanSandboxInbox.tssrc/tools/sandbox/schemas/createProject.tssrc/tools/sandbox/schemas/createSandboxInbox.tssrc/tools/sandbox/schemas/deleteProject.tssrc/tools/sandbox/schemas/deleteSandboxInbox.tssrc/tools/sandbox/schemas/getSandboxInbox.tssrc/tools/sandbox/schemas/listProjects.tssrc/tools/sandbox/schemas/updateSandboxInbox.tssrc/tools/sandbox/updateSandboxInbox.tssrc/tools/sendEmail/__tests__/sendEmail.test.tssrc/tools/sendEmail/sendEmail.tssrc/tools/sendingDomains/__tests__/createSendingDomain.test.tssrc/tools/sendingDomains/__tests__/deleteSendingDomain.test.tssrc/tools/sendingDomains/__tests__/getSendingDomain.test.tssrc/tools/sendingDomains/__tests__/listSendingDomains.test.tssrc/tools/sendingDomains/createSendingDomain.tssrc/tools/sendingDomains/deleteSendingDomain.tssrc/tools/sendingDomains/getSendingDomain.tssrc/tools/sendingDomains/listSendingDomains.tssrc/tools/stats/__tests__/getSendingStats.test.tssrc/tools/stats/getSendingStats.tssrc/tools/templates/__tests__/createTemplate.test.tssrc/tools/templates/__tests__/deleteTemplate.test.tssrc/tools/templates/__tests__/listTemplates.test.tssrc/tools/templates/__tests__/updateTemplate.test.tssrc/tools/templates/createTemplate.tssrc/tools/templates/deleteTemplate.tssrc/tools/templates/listTemplates.tssrc/tools/templates/updateTemplate.tssrc/types/mailtrap.ts
mklocek
force-pushed
the
more-sandbox-tools
branch
from
2459fc3 to
ca7538f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tools/stats/__tests__/getSendingStats.test.ts (1)
129-146: Environment manipulation is redundant with the mock.Since
requireClientis fully mocked to throw, thedelete process.env.MAILTRAP_ACCOUNT_IDat line 130 has no effect on test behavior—the mock determines the outcome. Consider removing the env manipulation for clarity, or add a comment explaining it's there for documentation purposes.This is a minor observation and doesn't affect test correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/stats/__tests__/getSendingStats.test.ts` around lines 129 - 146, The test currently deletes process.env.MAILTRAP_ACCOUNT_ID but the mocked requireClient already throws, so remove the redundant line "delete process.env.MAILTRAP_ACCOUNT_ID" (or replace it with a one-line comment) to avoid confusion; locate the setup in the test case for getSendingStats and adjust around the requireClient mock so only the mock-driven failure remains, keeping assertions that expect the error and that mockClient.stats.get was not called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tools/stats/__tests__/getSendingStats.test.ts`:
- Around line 129-146: The test currently deletes
process.env.MAILTRAP_ACCOUNT_ID but the mocked requireClient already throws, so
remove the redundant line "delete process.env.MAILTRAP_ACCOUNT_ID" (or replace
it with a one-line comment) to avoid confusion; locate the setup in the test
case for getSendingStats and adjust around the requireClient mock so only the
mock-driven failure remains, keeping assertions that expect the error and that
mockClient.stats.get was not called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce2033ed-1524-45f5-84be-a1cc152edd64
📒 Files selected for processing (25)
src/tools/emailLogs/__tests__/getEmailLogMessage.includeContent.test.tssrc/tools/emailLogs/__tests__/getEmailLogMessage.test.tssrc/tools/emailLogs/__tests__/listEmailLogs.test.tssrc/tools/emailLogs/getEmailLogMessage.tssrc/tools/emailLogs/listEmailLogs.tssrc/tools/sendEmail/__tests__/sendEmail.test.tssrc/tools/sendEmail/sendEmail.tssrc/tools/sendingDomains/__tests__/createSendingDomain.test.tssrc/tools/sendingDomains/__tests__/deleteSendingDomain.test.tssrc/tools/sendingDomains/__tests__/getSendingDomain.test.tssrc/tools/sendingDomains/__tests__/listSendingDomains.test.tssrc/tools/sendingDomains/createSendingDomain.tssrc/tools/sendingDomains/deleteSendingDomain.tssrc/tools/sendingDomains/getSendingDomain.tssrc/tools/sendingDomains/listSendingDomains.tssrc/tools/stats/__tests__/getSendingStats.test.tssrc/tools/stats/getSendingStats.tssrc/tools/templates/__tests__/createTemplate.test.tssrc/tools/templates/__tests__/deleteTemplate.test.tssrc/tools/templates/__tests__/listTemplates.test.tssrc/tools/templates/__tests__/updateTemplate.test.tssrc/tools/templates/createTemplate.tssrc/tools/templates/deleteTemplate.tssrc/tools/templates/listTemplates.tssrc/tools/templates/updateTemplate.ts
✅ Files skipped from review due to trivial changes (6)
- src/tools/templates/updateTemplate.ts
- src/tools/templates/tests/updateTemplate.test.ts
- src/tools/emailLogs/tests/getEmailLogMessage.includeContent.test.ts
- src/tools/sendingDomains/tests/getSendingDomain.test.ts
- src/tools/sendingDomains/createSendingDomain.ts
- src/tools/templates/tests/createTemplate.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/tools/sendEmail/tests/sendEmail.test.ts
- src/tools/sendingDomains/getSendingDomain.ts
- src/tools/sendingDomains/tests/createSendingDomain.test.ts
- src/tools/sendingDomains/listSendingDomains.ts
- src/tools/sendingDomains/tests/listSendingDomains.test.ts
- src/tools/emailLogs/tests/listEmailLogs.test.ts
- src/tools/sendingDomains/tests/deleteSendingDomain.test.ts
- src/tools/templates/tests/deleteTemplate.test.ts
piobeny
approved these changes
Mar 25, 2026
IgorDobryn
approved these changes
Mar 26, 2026
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.
Changes
Images and GIFs
Summary by CodeRabbit
New Features
Refactor
Tests
Types/Schemas