Add Sandbox messages API #82
Open
Conversation
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new Mailtrap::SandboxMessagesAPI class is introduced with comprehensive CRUD operations and specialized endpoints for sandbox message management. The client is refactored with centralized HTTP client creation, unified response parsing, and improved error handling. A SandboxMessage data model is added alongside extensive test coverage including API interaction fixtures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 19
🤖 Fix all issues with AI agents
In `@lib/mailtrap/client.rb`:
- Around line 281-284: The handle_response method triggers a RuboCop
Metrics/AbcSize violation; either add AbcSize to the existing RuboCop disable
comment for handle_response or refactor the success branch into a small helper
to reduce ABC complexity—specifically extract the success handling (the when
Net::HTTPOK, Net::HTTPCreated branch that chooses between
json_response(response.body) and custom_parser.call(response.body)) into a
private method (e.g., parse_success_response(response, custom_parser)) and call
that from handle_response to bring AbcSize under the threshold.
- Line 248: The method perform_request currently has six parameters and triggers
Metrics/ParameterLists; refactor it to accept a single options hash or a Request
object to reduce parameter count: replace the multiple keyword params (method:,
host:, path:, query_params:, body:, custom_parser:) with one param (e.g.,
request_options or request) and update callers to pass a hash or instantiated
Request (containing method, host, path, query_params, body, custom_parser);
alternatively, if refactoring callers is impractical, add a scoped RuboCop
directive to disable Metrics/ParameterLists for perform_request only. Ensure you
update the method signature, usages, and any documentation to match the new
options object name.
In `@lib/mailtrap/sandbox_message.rb`:
- Line 54: The YARD doc comment incorrectly references "SendingDomain" for the
SandboxMessage class; update the comment on the SandboxMessage
attribute/hash-returning method so it describes SandboxMessage (e.g., "The
SandboxMessage attributes as a hash") instead of "SendingDomain" and keep the
return type as [Hash]; locate the comment in class SandboxMessage and replace
the incorrect identifier.
In `@lib/mailtrap/sandbox_messages_api.rb`:
- Around line 128-134: Update the YARD docstring for get_message_as_eml: replace
the incorrect "Get mail headers" description and the `@return` type with a correct
description indicating it retrieves the full message in EML format (e.g., "Get
message as EML") and adjust the `@return` to reflect the raw EML response (e.g.,
String or raw response) while keeping the `@param` and @!macro api_errors intact;
locate the doc above def get_message_as_eml(message_id) which calls
client.get("#{base_path}/#{message_id}/body.eml", custom_parser: ->(response) {
response }).
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_delete/returns_deleted_project_id.yml`:
- Around line 78-81: The recorded fixture JSON string contains PII in the
to_email field ("to_email":"yahor.vaitsiakhouski@railsware.com"); replace that
real address with a safe placeholder (e.g., "to_email":"user@example.com")
inside the string value and scan the same JSON blob for any other personal
identifiers (from_email, from_name, any human-readable names within the string)
and substitute with non-identifying placeholders, ensuring you preserve valid
JSON escaping and the overall string format (the surrounding "string": '...') so
the cassette remains valid for tests.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_forward_message/returns_success.yml`:
- Around line 6-8: Replace the real-company test email in the cassette payload
with an RFC 2606 reserved address: update the YAML body's string value
'{"email":"example@railsware.com"}' to use a reserved domain such as
'{"email":"test@example.com"}' (and similarly replace any other occurrences of
"railsware.com" in the same file) so test fixtures use a non-production/reserved
test email.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_forward_message/when_sandbox_message_does_not_exist/raises_not_found_error.yml`:
- Around line 6-8: The cassette's request body string contains a non-generic
email "example@railsware.com"; update the YAML value under the body -> string
key to use a generic placeholder like "test@example.com" so the fixture does not
expose organizational email patterns (update the string value in the cassette
where body.encoding and body.string are defined).
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_get_mail_headers/returns_headers.yml`:
- Line 77: The fixture contains PII: the JSON string value under the YAML
"string" that includes headers.to contains the real email
"yahor.vaitsiakhouski@railsware.com"; replace that address with a neutral
placeholder (e.g., test@example.com) inside the embedded JSON (the
'{"headers":...}' value) so the headers.to field is masked, ensure any necessary
escaping/quoting in the YAML string is preserved and update the cassette
accordingly.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_get_message_as_eml/returns_html_message.yml`:
- Around line 79-85: The EML fixture contains a real recipient in the "To:"
header inside the YAML value for the "string" field (the long MIME string);
replace the real address yahor.vaitsiakhouski@railsware.com with a non-sensitive
placeholder such as test@example.com (or recipient@example.org) in that "string"
value so the cassette no longer contains PII, preserving the surrounding MIME
formatting and boundaries (e.g., the existing boundary=... and multipart
content).
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_get_message_as_eml/when_sandbox_message_does_not_exist/raises_not_found_error.yml`:
- Around line 57-68: Replace sensitive operational header values in the VCR
fixture YAML by scrubbing the X-Request-Id, X-Cloud-Trace-Context, and Cf-Ray
header values and replacing them with deterministic placeholders (e.g.
<X-REQUEST-ID>, <X-CLOUD-TRACE-CONTEXT>, <CF-RAY>) in the recorded response
headers block; apply the same replacement to the other occurrence mentioned (the
headers at the other record location). Ensure the cassette’s headers now only
contain placeholder values for those three keys so the fixture contains no real
request/trace identifiers.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_get_raw_message/returns_raw_message.yml`:
- Around line 75-81: The recorded cassette contains a real email in the YAML
string value (the "string" field) — replace the PII
`yahor.vaitsiakhouski@railsware.com` with a sanitized test address like
`test@example.com` (or `user@mailtrap.io`) inside the raw message string, ensure
any other real-identifying data in that same "string" value is similarly
sanitized, then save the updated fixtures file so the VCR cassette no longer
contains real user PII.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_get/maps_response_data_to_SandboxMessage_object.yml`:
- Around line 78-81: Replace the real recipient address found in the fixture
string: locate the "to_email" field value that currently contains
"alex.b@railsware.com" in the YAML cassette content and change it to a non-PII
placeholder such as "test@example.com" or "<RECIPIENT_EMAIL>" so the recorded
response no longer contains personal data.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_list/maps_response_data_to_SandboxMessage_objects.yml`:
- Around line 78-97: The VCR cassette contains real email PII inside the JSON
string value under the YAML key "string" in
maps_response_data_to_SandboxMessage_objects.yml; open that file and replace the
actual addresses (e.g. alex.d@railsware.com, yahor.vaitsiakhouski@railsware.com)
with generic placeholders (e.g. user1@example.com, user2@example.com) everywhere
they appear in the JSON payload string, ensure other similar PII in that
"string" block (if any) is also sanitized, save the updated cassette and run the
test suite to confirm no fixtures break.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_list/with_parameters/maps_response_data_to_SandboxMessage_objects.yml`:
- Line 5: Replace the real email and search term in the cassette with
placeholders: update the URI query parameter "search=alex.d" to a non-PII
placeholder (e.g., "search=REDACTED") and replace any "to_email" values like
"alex.d@railsware.com" with a placeholder (e.g., "user@example.com" or
"REDACTED_EMAIL"); apply the same replacements for the other occurrences
referenced (lines 78-81) so no real email addresses or personal names remain in
the recorded HTTP interactions or URL query strings.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_list/with_parameters/when_last_id_includes_more_messages/maps_response_data_to_SandboxMessage_objects.yml`:
- Around line 78-85: The YAML cassette's recorded JSON in the string field
contains real PII emails (alex.d@railsware.com and
yahor.vaitsiakhouski@railsware.com); locate the large "string" value in the
fixture and replace those addresses with generic placeholders (e.g.,
user1@example.com, user2@example.com) preserving the JSON structure/escaping and
all other fields (ids, paths, timestamps) so tests remain valid; ensure you
update every occurrence of those two addresses inside the "string" value.
- Line 5: The URI in the cassette contains a PII-bearing query parameter value
("search=alex.d"); update the recorded URI value under the uri key so the search
parameter is anonymized (e.g., replace "search=alex.d" with "search=REDACTED" or
a generic non-identifying value like "search=sample") while keeping other
parameters (last_id, account/inbox ids) unchanged; edit the YAML entry where uri
is defined to use the sanitized query string so tests still map responses but no
actual username fragments are stored.
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_update/maps_response_data_to_SandboxMessage_object.yml`:
- Around line 78-81: The cassette contains real PII in the JSON string under the
"to_email" field (value "alex.b@railsware.com"); open the YAML fixture and
replace that email with a generic placeholder (e.g., "user@example.com")
preserving the surrounding JSON quoting/escaping and any adjacent
whitespace/newlines so the string remains valid; ensure no other occurrences of
the same real address remain in this fixture (search for "alex.b@railsware.com")
and update them similarly.
In `@spec/mailtrap/sandbox_messages_api_spec.rb`:
- Line 25: Replace the real email literal 'alex.b@railsware.com' used as the
to_email expectation with a non-PII placeholder (e.g., 'user@example.com') and
do the same for the other occurrences of that literal elsewhere in the spec;
locate the string by searching for the to_email key and the exact email literal
(and any identical string occurrences) in sandbox_messages_api_spec and swap
them for a placeholder or generated test email (e.g., using a factory or Faker)
so tests no longer contain real PII.
- Around line 391-395: Update the test descriptions to use consistent
terminology: replace occurrences of "project" with "message" in the example
block that asserts deletion and in the following context; specifically update
the it description that currently reads 'returns deleted project id' to 'returns
deleted message id' and change the context label 'when project does not exist'
to 'when message does not exist' so the text matches the API under test
(references: the example using delete[:id] and sandbox_message_id).
🧹 Nitpick comments (2)
spec/mailtrap/sandbox_message_spec.rb (1)
29-35: Consider using reserved IP for test data.The IP address
193.62.62.184appears to be a real routable IP. For test fixtures, consider using a reserved documentation IP like192.0.2.1(RFC 5737 TEST-NET-1) to avoid any potential conflicts.This is a minor nit and not blocking.
lib/mailtrap/sandbox_messages_api.rb (1)
100-133: Consider extracting the repeated identity lambda.The identity lambda
->(response) { response }is duplicated across five methods. Extracting it to a constant would improve maintainability and make the intent clearer.♻️ Suggested refactor
Add a constant at the class level:
PASSTHROUGH_PARSER = ->(response) { response }Then use it in each method:
def get_text_message(message_id) - client.get("#{base_path}/#{message_id}/body.txt", custom_parser: ->(response) { response }) + client.get("#{base_path}/#{message_id}/body.txt", custom_parser: PASSTHROUGH_PARSER) end
lib/mailtrap/sandbox_messages_api.rb
Outdated
Comment on lines
128
to
134
| # Get mail headers | ||
| # @param message_id [Integer] The Sandbox message ID | ||
| # @return [Hash] mail headers of the message. | ||
| # @!macro api_errors | ||
| def get_message_as_eml(message_id) | ||
| client.get("#{base_path}/#{message_id}/body.eml", custom_parser: ->(response) { response }) | ||
| end |
There was a problem hiding this comment.
Incorrect documentation for get_message_as_eml.
The YARD comment says "Get mail headers" but this method retrieves the message as EML format. This appears to be a copy-paste error.
📝 Proposed fix
- # Get mail headers
+ # Get message as EML
# `@param` message_id [Integer] The Sandbox message ID
- # `@return` [Hash] mail headers of the message.
+ # `@return` [String] message in EML format.
# @!macro api_errors
def get_message_as_eml(message_id)
client.get("#{base_path}/#{message_id}/body.eml", custom_parser: ->(response) { response })
end🤖 Prompt for AI Agents
In `@lib/mailtrap/sandbox_messages_api.rb` around lines 128 - 134, Update the YARD
docstring for get_message_as_eml: replace the incorrect "Get mail headers"
description and the `@return` type with a correct description indicating it
retrieves the full message in EML format (e.g., "Get message as EML") and adjust
the `@return` to reflect the raw EML response (e.g., String or raw response) while
keeping the `@param` and @!macro api_errors intact; locate the doc above def
get_message_as_eml(message_id) which calls
client.get("#{base_path}/#{message_id}/body.eml", custom_parser: ->(response) {
response }).
spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_delete/returns_deleted_project_id.yml
Outdated
Show resolved
Hide resolved
| http_interactions: | ||
| - request: | ||
| method: get | ||
| uri: https://mailtrap.io/api/accounts/1111111/inboxes/4288340/messages?last_id=5273671999&search=alex.d |
There was a problem hiding this comment.
URL query parameter contains potential PII.
The search parameter search=alex.d reveals a partial username/name pattern.
🤖 Prompt for AI Agents
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_list/with_parameters/when_last_id_includes_more_messages/maps_response_data_to_SandboxMessage_objects.yml`
at line 5, The URI in the cassette contains a PII-bearing query parameter value
("search=alex.d"); update the recorded URI value under the uri key so the search
parameter is anonymized (e.g., replace "search=alex.d" with "search=REDACTED" or
a generic non-identifying value like "search=sample") while keeping other
parameters (last_id, account/inbox ids) unchanged; edit the YAML entry where uri
is defined to use the sanitized query string so tests still map responses but no
actual username fragments are stored.
Comment on lines
+78
to
+85
| string: '[{"id":5273671945,"inbox_id":4288340,"subject":"Hello from Mailtrap","sent_at":"2026-01-04T11:17:56.575-12:00","from_email":"reply@demomailtrap.co","from_name":"","to_email":"alex.d@railsware.com","to_name":"","email_size":677,"is_read":false,"created_at":"2026-01-04T11:17:56.579-12:00","updated_at":"2026-01-04T11:17:56.797-12:00","template_id":0,"template_variables":null,"html_body_size":29,"text_body_size":20,"human_size":"677 | ||
| Bytes","html_path":"/api/testing_message_parts/QEVuQwFAEAD6SO5PqWHlAWWad6MRonhtsuZxmCo4xzuFGE0LxKt6KDddCoGCmfx8kJdAWyxONzRlvqhB5mnkHxhicF6lbIh6dxpbBW8fNKU4wFjIE2F2d6+IvRnXDgBVNvWDZ+PIEjg=/body.html","txt_path":"/api/testing_message_parts/QEVuQwFAEAD6SO5PqWHlAWWad6MRonhtsuZxmCo4xzuFGE0LxKt6KDddCoGCmfx8kJdAWyxONzRlvqhB5mnkHxhicF6lbIh6dxpbBW8fNKU4wFjIE2F2d6+IvRnXDgBVNvWDZ+PIEjg=/body.txt","raw_path":"/api/testing_message_parts/QEVuQwFAEAD6SO5PqWHlAWWad6MRonhtsuZxmCo4xzuFGE0LxKt6KDddCoGCmfx8kJdAWyxONzRlvqhB5mnkHxhicF6lbIh6dxpbBW8fNKU4wFjIE2F2d6+IvRnXDgBVNvWDZ+PIEjg=/body.raw","download_path":"/api/testing_message_parts/QEVuQwFAEAD6SO5PqWHlAWWad6MRonhtsuZxmCo4xzuFGE0LxKt6KDddCoGCmfx8kJdAWyxONzRlvqhB5mnkHxhicF6lbIh6dxpbBW8fNKU4wFjIE2F2d6+IvRnXDgBVNvWDZ+PIEjg=/body.eml","html_source_path":"/api/testing_message_parts/QEVuQwFAEAD6SO5PqWHlAWWad6MRonhtsuZxmCo4xzuFGE0LxKt6KDddCoGCmfx8kJdAWyxONzRlvqhB5mnkHxhicF6lbIh6dxpbBW8fNKU4wFjIE2F2d6+IvRnXDgBVNvWDZ+PIEjg=/body.htmlsource","blacklists_report_info":{"result":"success","domain":"demomailtrap.co","ip":"104.21.25.146","report":[{"name":"BACKSCATTERER","url":"http://www.backscatterer.org/index.php","in_black_list":false},{"name":"BARRACUDA","url":"http://barracudacentral.org/rbl","in_black_list":false},{"name":"Spamrbl | ||
| IMP-SPAM","url":"http://antispam.imp.ch/?lng=1","in_black_list":false},{"name":"Wormrbl | ||
| IMP-SPAM","url":"http://antispam.imp.ch/?lng=1","in_black_list":false},{"name":"LASHBACK","url":"http://blacklist.lashback.com/","in_black_list":false},{"name":"NIXSPAM","url":"https://www.heise.de/ix/NiX-Spam-DNSBL-and-blacklist-for-download-499637.html","in_black_list":false},{"name":"PSBL","url":"https://psbl.org/","in_black_list":false},{"name":"SORBS-SPAM","url":"http://www.sorbs.net/lookup.shtml","in_black_list":false},{"name":"SPAMCOP","url":"http://spamcop.net/bl.shtml","in_black_list":false},{"name":"TRUNCATE","url":"http://www.gbudb.com/truncate/index.jsp","in_black_list":false}]},"smtp_information":{"ok":false}},{"id":5273671571,"inbox_id":4288340,"subject":"Hello | ||
| from Mailtrap","sent_at":"2026-01-04T11:17:40.025-12:00","from_email":"reply@demomailtrap.co","from_name":"","to_email":"alex.d@railsware.com","to_name":"","email_size":677,"is_read":false,"created_at":"2026-01-04T11:17:40.029-12:00","updated_at":"2026-01-04T11:17:40.208-12:00","template_id":0,"template_variables":null,"html_body_size":29,"text_body_size":20,"human_size":"677 | ||
| Bytes","html_path":"/api/testing_message_parts/QEVuQwFAEAAwMccBahxwpCSHvGNpOmK54J4cObh1DdN9Wixmdbc%2FzsVUvM0ac986GgLvzHzXUmQG0Dp+qyvCjLu62542pDz7ORIh4XQlmVjD7Ey0bN1IPtRIYPQgHXf+ldLmjrn1eEw=/body.html","txt_path":"/api/testing_message_parts/QEVuQwFAEAAwMccBahxwpCSHvGNpOmK54J4cObh1DdN9Wixmdbc%2FzsVUvM0ac986GgLvzHzXUmQG0Dp+qyvCjLu62542pDz7ORIh4XQlmVjD7Ey0bN1IPtRIYPQgHXf+ldLmjrn1eEw=/body.txt","raw_path":"/api/testing_message_parts/QEVuQwFAEAAwMccBahxwpCSHvGNpOmK54J4cObh1DdN9Wixmdbc%2FzsVUvM0ac986GgLvzHzXUmQG0Dp+qyvCjLu62542pDz7ORIh4XQlmVjD7Ey0bN1IPtRIYPQgHXf+ldLmjrn1eEw=/body.raw","download_path":"/api/testing_message_parts/QEVuQwFAEAAwMccBahxwpCSHvGNpOmK54J4cObh1DdN9Wixmdbc%2FzsVUvM0ac986GgLvzHzXUmQG0Dp+qyvCjLu62542pDz7ORIh4XQlmVjD7Ey0bN1IPtRIYPQgHXf+ldLmjrn1eEw=/body.eml","html_source_path":"/api/testing_message_parts/QEVuQwFAEAAwMccBahxwpCSHvGNpOmK54J4cObh1DdN9Wixmdbc%2FzsVUvM0ac986GgLvzHzXUmQG0Dp+qyvCjLu62542pDz7ORIh4XQlmVjD7Ey0bN1IPtRIYPQgHXf+ldLmjrn1eEw=/body.htmlsource","blacklists_report_info":{"result":"success","domain":"demomailtrap.co","ip":"104.21.25.146","report":[{"name":"BACKSCATTERER","url":"http://www.backscatterer.org/index.php","in_black_list":false},{"name":"BARRACUDA","url":"http://barracudacentral.org/rbl","in_black_list":false},{"name":"Spamrbl | ||
| IMP-SPAM","url":"http://antispam.imp.ch/?lng=1","in_black_list":false},{"name":"Wormrbl | ||
| IMP-SPAM","url":"http://antispam.imp.ch/?lng=1","in_black_list":false},{"name":"LASHBACK","url":"http://blacklist.lashback.com/","in_black_list":false},{"name":"NIXSPAM","url":"https://www.heise.de/ix/NiX-Spam-DNSBL-and-blacklist-for-download-499637.html","in_black_list":false},{"name":"PSBL","url":"https://psbl.org/","in_black_list":false},{"name":"SORBS-SPAM","url":"http://www.sorbs.net/lookup.shtml","in_black_list":false},{"name":"SPAMCOP","url":"http://spamcop.net/bl.shtml","in_black_list":false},{"name":"TRUNCATE","url":"http://www.gbudb.com/truncate/index.jsp","in_black_list":false}]},"smtp_information":{"ok":false}}]' |
There was a problem hiding this comment.
Response body contains real email addresses (PII).
The recorded response contains real employee email addresses:
alex.d@railsware.comyahor.vaitsiakhouski@railsware.com
These should be replaced with generic placeholders (e.g., user1@example.com, user2@example.com) to avoid exposing personal information in a public repository. The full name pattern in yahor.vaitsiakhouski is particularly identifiable.
As per coding guidelines: "Act as a data privacy officer. Carefully read all the vcr cassettes with recorded HTTP interactions and try to identify sensitive data that could potentially be recorded. It can be anything from PII to credentials."
🤖 Prompt for AI Agents
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_list/with_parameters/when_last_id_includes_more_messages/maps_response_data_to_SandboxMessage_objects.yml`
around lines 78 - 85, The YAML cassette's recorded JSON in the string field
contains real PII emails (alex.d@railsware.com and
yahor.vaitsiakhouski@railsware.com); locate the large "string" value in the
fixture and replace those addresses with generic placeholders (e.g.,
user1@example.com, user2@example.com) preserving the JSON structure/escaping and
all other fields (ids, paths, timestamps) so tests remain valid; ensure you
update every occurrence of those two addresses inside the "string" value.
Comment on lines
+78
to
+81
| string: '{"id":5273448410,"inbox_id":4288340,"subject":"Hello from Mailtrap","sent_at":"2026-01-04T04:55:50.867-12:00","from_email":"reply@demomailtrap.co","from_name":"","to_email":"alex.b@railsware.com","to_name":"","email_size":691,"is_read":true,"created_at":"2026-01-04T04:55:50.871-12:00","updated_at":"2026-01-04T04:55:56.618-12:00","template_id":0,"template_variables":null,"html_body_size":29,"text_body_size":20,"human_size":"691 | ||
| Bytes","html_path":"/api/testing_message_parts/QEVuQwFAEABC6ODCerNYpR2yAx7xrEH6%2Ftn5yHfn6zIHYoQ9Ce0V5e9PB+Fy64WrVklr1BTc8WNmeft20TH7pytYbFWIzBx61dHgATF0KMjaFS1whA7iqWor%2FinVxeyLbZbskDsXojw=/body.html","txt_path":"/api/testing_message_parts/QEVuQwFAEABC6ODCerNYpR2yAx7xrEH6%2Ftn5yHfn6zIHYoQ9Ce0V5e9PB+Fy64WrVklr1BTc8WNmeft20TH7pytYbFWIzBx61dHgATF0KMjaFS1whA7iqWor%2FinVxeyLbZbskDsXojw=/body.txt","raw_path":"/api/testing_message_parts/QEVuQwFAEABC6ODCerNYpR2yAx7xrEH6%2Ftn5yHfn6zIHYoQ9Ce0V5e9PB+Fy64WrVklr1BTc8WNmeft20TH7pytYbFWIzBx61dHgATF0KMjaFS1whA7iqWor%2FinVxeyLbZbskDsXojw=/body.raw","download_path":"/api/testing_message_parts/QEVuQwFAEABC6ODCerNYpR2yAx7xrEH6%2Ftn5yHfn6zIHYoQ9Ce0V5e9PB+Fy64WrVklr1BTc8WNmeft20TH7pytYbFWIzBx61dHgATF0KMjaFS1whA7iqWor%2FinVxeyLbZbskDsXojw=/body.eml","html_source_path":"/api/testing_message_parts/QEVuQwFAEABC6ODCerNYpR2yAx7xrEH6%2Ftn5yHfn6zIHYoQ9Ce0V5e9PB+Fy64WrVklr1BTc8WNmeft20TH7pytYbFWIzBx61dHgATF0KMjaFS1whA7iqWor%2FinVxeyLbZbskDsXojw=/body.htmlsource","blacklists_report_info":{"result":"success","domain":"demomailtrap.co","ip":"172.67.134.80","report":[{"name":"BACKSCATTERER","url":"http://www.backscatterer.org/index.php","in_black_list":false},{"name":"BARRACUDA","url":"http://barracudacentral.org/rbl","in_black_list":false},{"name":"Spamrbl | ||
| IMP-SPAM","url":"http://antispam.imp.ch/?lng=1","in_black_list":false},{"name":"Wormrbl | ||
| IMP-SPAM","url":"http://antispam.imp.ch/?lng=1","in_black_list":false},{"name":"LASHBACK","url":"http://blacklist.lashback.com/","in_black_list":false},{"name":"NIXSPAM","url":"https://www.heise.de/ix/NiX-Spam-DNSBL-and-blacklist-for-download-499637.html","in_black_list":false},{"name":"PSBL","url":"https://psbl.org/","in_black_list":false},{"name":"SORBS-SPAM","url":"http://www.sorbs.net/lookup.shtml","in_black_list":false},{"name":"SPAMCOP","url":"http://spamcop.net/bl.shtml","in_black_list":false},{"name":"TRUNCATE","url":"http://www.gbudb.com/truncate/index.jsp","in_black_list":false}]},"smtp_information":{"ok":false}}' |
There was a problem hiding this comment.
Response body contains a real email address (PII).
The response contains to_email":"alex.b@railsware.com" which should be replaced with a generic placeholder like user@example.com.
As per coding guidelines for VCR cassettes: identify sensitive data including PII.
🤖 Prompt for AI Agents
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_update/maps_response_data_to_SandboxMessage_object.yml`
around lines 78 - 81, The cassette contains real PII in the JSON string under
the "to_email" field (value "alex.b@railsware.com"); open the YAML fixture and
replace that email with a generic placeholder (e.g., "user@example.com")
preserving the surrounding JSON quoting/escaping and any adjacent
whitespace/newlines so the string remains valid; ensure no other occurrences of
the same real address remain in this fixture (search for "alex.b@railsware.com")
and update them similarly.
| sent_at: '2026-01-04T04:55:50.867-12:00', | ||
| from_email: 'reply@demomailtrap.co', | ||
| from_name: '', | ||
| to_email: 'alex.b@railsware.com', |
There was a problem hiding this comment.
PII detected: Real email addresses in test expectations.
The test expectations contain what appear to be real email addresses (e.g., alex.b@railsware.com). Consider using placeholder emails (e.g., user@example.com) to avoid exposing real user data. The same applies to other occurrences on lines 142 and 363.
🤖 Prompt for AI Agents
In `@spec/mailtrap/sandbox_messages_api_spec.rb` at line 25, Replace the real
email literal 'alex.b@railsware.com' used as the to_email expectation with a
non-PII placeholder (e.g., 'user@example.com') and do the same for the other
occurrences of that literal elsewhere in the spec; locate the string by
searching for the to_email key and the exact email literal (and any identical
string occurrences) in sandbox_messages_api_spec and swap them for a placeholder
or generated test email (e.g., using a factory or Faker) so tests no longer
contain real PII.
…ap/mailtrap-ruby into MT-19857-sandbox-messages
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/mailtrap/sandbox_messages_api.rb`:
- Around line 55-66: In the list method, ensure last_id takes precedence over
page by only adding one of them to query_params: if last_id is provided, add
query_params[:last_id] = last_id and do not add :page; otherwise add :page when
present. Update the logic around query_params population in the list method
(referencing the variables last_id, page and the query_params hash) so that
:last_id and :page are mutually exclusive and :last_id overrides :page as
documented.
- Around line 16-22: The initialize method currently uses
ENV.fetch('MAILTRAP_ACCOUNT_ID') which raises KeyError and breaks the documented
`@raise` [ArgumentError] contract; change the default to
ENV['MAILTRAP_ACCOUNT_ID'] so the explicit guard (raise ArgumentError,
'account_id is required') for account_id in initialize(inbox_id, account_id =
ENV['MAILTRAP_ACCOUNT_ID'], client = Mailtrap::Client.new) will run when the env
var is missing or nil, leaving the inbox_id guard (raise ArgumentError,
'inbox_id is required') and the rest of the initializer unchanged.
Comment on lines
16
to
22
| # @param inbox_id [Integer] The inbox ID | ||
| # @param account_id [Integer] The account ID | ||
| # @param client [Mailtrap::Client] The client instance | ||
| # @raise [ArgumentError] If account_id is nil | ||
| def initialize(inbox_id, account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID'), client = Mailtrap::Client.new) | ||
| raise ArgumentError, 'account_id is required' if account_id.nil? | ||
| raise ArgumentError, 'inbox_id is required' if inbox_id.nil? |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/mailtrap/sandbox_messages_api.rb | head -30Repository: mailtrap/mailtrap-ruby
Length of output: 1217
🏁 Script executed:
# Let's also check if there are any other references to account_id handling
rg "account_id" lib/mailtrap/sandbox_messages_api.rb -A 2 -B 2Repository: mailtrap/mailtrap-ruby
Length of output: 806
🏁 Script executed:
# Verify the behavior of ENV.fetch vs ENV[]
# ENV.fetch raises KeyError if key doesn't exist
# ENV[] returns nil if key doesn't exist
python3 << 'EOF'
# Simulate Ruby's ENV.fetch behavior
# ENV.fetch('KEY') raises KeyError if not found
# ENV['KEY'] returns nil if not found
print("ENV.fetch behavior: raises KeyError when env var not found")
print("ENV['KEY'] behavior: returns nil when env var not found")
print("\nThe review claim:")
print("- Using ENV.fetch means KeyError is raised (not ArgumentError)")
print("- The `@raise` documentation claims ArgumentError will be raised")
print("- This is a mismatch between documented behavior and actual behavior")
EOFRepository: mailtrap/mailtrap-ruby
Length of output: 394
🏁 Script executed:
# Check YARD documentation in the file
rg "@raise" lib/mailtrap/sandbox_messages_api.rb -B 5 -A 1Repository: mailtrap/mailtrap-ruby
Length of output: 756
Align error behavior with the YARD contract.
Line 20 uses ENV.fetch, which raises KeyError when the environment variable is unset, but the documentation declares @raise [ArgumentError]. To match the documented contract, use ENV['MAILTRAP_ACCOUNT_ID'] so the explicit ArgumentError check on line 21 actually executes when the account ID is missing or nil.
💡 Proposed fix
- def initialize(inbox_id, account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID'), client = Mailtrap::Client.new)
+ def initialize(inbox_id, account_id = ENV['MAILTRAP_ACCOUNT_ID'], client = Mailtrap::Client.new)
raise ArgumentError, 'account_id is required' if account_id.nil?
raise ArgumentError, 'inbox_id is required' if inbox_id.nil?📝 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.
Suggested change
| # @param inbox_id [Integer] The inbox ID | |
| # @param account_id [Integer] The account ID | |
| # @param client [Mailtrap::Client] The client instance | |
| # @raise [ArgumentError] If account_id is nil | |
| def initialize(inbox_id, account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID'), client = Mailtrap::Client.new) | |
| raise ArgumentError, 'account_id is required' if account_id.nil? | |
| raise ArgumentError, 'inbox_id is required' if inbox_id.nil? | |
| # `@param` inbox_id [Integer] The inbox ID | |
| # `@param` account_id [Integer] The account ID | |
| # `@param` client [Mailtrap::Client] The client instance | |
| # `@raise` [ArgumentError] If account_id is nil | |
| def initialize(inbox_id, account_id = ENV['MAILTRAP_ACCOUNT_ID'], client = Mailtrap::Client.new) | |
| raise ArgumentError, 'account_id is required' if account_id.nil? | |
| raise ArgumentError, 'inbox_id is required' if inbox_id.nil? |
🤖 Prompt for AI Agents
In `@lib/mailtrap/sandbox_messages_api.rb` around lines 16 - 22, The initialize
method currently uses ENV.fetch('MAILTRAP_ACCOUNT_ID') which raises KeyError and
breaks the documented `@raise` [ArgumentError] contract; change the default to
ENV['MAILTRAP_ACCOUNT_ID'] so the explicit guard (raise ArgumentError,
'account_id is required') for account_id in initialize(inbox_id, account_id =
ENV['MAILTRAP_ACCOUNT_ID'], client = Mailtrap::Client.new) will run when the env
var is missing or nil, leaving the inbox_id guard (raise ArgumentError,
'inbox_id is required') and the rest of the initializer unchanged.
i7an
reviewed
Feb 6, 2026
lib/mailtrap/client.rb
Outdated
| end | ||
|
|
||
| def parse_response(response) | ||
| return json_response(response.body) if response['Content-Type']&.include?('application/json') |
Contributor
There was a problem hiding this comment.
please see all the places where json_response is used and update those as well.
Contributor
Author
There was a problem hiding this comment.
the only other place is handling the errors, I think it should be kept.
Contributor
There was a problem hiding this comment.
Please explain why. I can find places in our code where plain error response is returned e.g. mailsend_messages#index.
Contributor
Author
There was a problem hiding this comment.
You are right - done.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_delete/returns_deleted_project_id.yml`:
- Around line 78-81: The fixture contains PII in the JSON string where the
"to_email" field is set to "bob.s@railsware.com"; open the YAML cassette entry
that contains the long JSON string and replace that email value with a safe
placeholder (e.g., "user@example.com") so the "to_email" key no longer contains
real personal data; verify the surrounding JSON string encoding/quoting remains
valid after the substitution and run the tests to ensure the cassette still
loads correctly.
In `@spec/mailtrap/sandbox_messages_api_spec.rb`:
- Around line 330-334: The example spec description is incorrect: update the
RSpec example description for the test using get_message_as_eml from 'returns
html message' to a correct label like 'returns eml message' or 'returns message
as eml' so the it description matches the assertion checking EML content
(references: get_message_as_eml and the example string passed to it).
🧹 Nitpick comments (2)
spec/mailtrap/sandbox_messages_api_spec.rb (1)
56-66: Repetitive error-handling test pattern could use a shared example.The not-found error assertion block is duplicated ~12 times across this file. A shared example like
it_behaves_like 'raises not found error'would reduce duplication and make future changes easier.lib/mailtrap/client.rb (1)
279-304: Add explicitNet::HTTPNotFoundhandler for consistent error formatting.The 404 response falls through to the generic
Net::HTTPClientErrorcase (sinceNet::HTTPNotFoundis a subclass of it), producing["client error '{"error":"Not Found"}'"]instead of the clean["Not Found"]extracted byresponse_errors. While tests pass via substring matching, the error message format is inconsistent with 400/401/403 handling. Add an explicitNet::HTTPNotFoundbranch usingresponse_errorsfor consistency:♻️ Proposed fix
when Net::HTTPTooManyRequests raise Mailtrap::RateLimitError, ['too many requests'] + when Net::HTTPNotFound + raise Mailtrap::Error, response_errors(response) when Net::HTTPClientError raise Mailtrap::Error, ["client error '#{response.body}'"]
spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_delete/returns_deleted_project_id.yml
Show resolved
Hide resolved
i7an
reviewed
Feb 10, 2026
| require_relative 'sandbox_message' | ||
|
|
||
| module Mailtrap | ||
| class SandboxMessagesAPI |
Contributor
There was a problem hiding this comment.
Inboxes API but Sandbox Messages API. Let's discuss today.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_get_message_as_eml/returns_eml_message.yml`:
- Around line 79-85: Replace the real email address "bob.s@railsware.com" inside
the recorded EML string in the VCR cassette (the multi-line value under the
string key in the Mailtrap_SandboxMessagesAPI
_get_message_as_eml/returns_eml_message.yml fixture) with a non-PII placeholder
such as "test@example.com", ensuring the replacement preserves quoting/escaping
and does not alter other parts of the MIME payload.
🧹 Nitpick comments (1)
lib/mailtrap/client.rb (1)
313-320: Edge case:response_errorsmay return an empty array for unexpected JSON shapes.If the JSON response contains neither
:errorsnor:error,Array(nil)returns[], which would raise an exception with an empty/blank message. Consider a fallback:Suggested improvement
def response_errors(response) if json_response?(response) parsed_body = json_response(response.body) - Array(parsed_body[:errors] || parsed_body[:error]) + errors = Array(parsed_body[:errors] || parsed_body[:error]) + errors.empty? ? [response.body] : errors else [response.body] end end
spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_eml_body/returns_eml_message.yml
Show resolved
Hide resolved
i7an
reviewed
Feb 10, 2026
| # @param account_id [Integer] The account ID | ||
| # @param client [Mailtrap::Client] The client instance | ||
| # @raise [ArgumentError] If account_id is nil | ||
| def initialize(inbox_id, account_id = ENV.fetch('MAILTRAP_ACCOUNT_ID'), client = Mailtrap::Client.new) |
Contributor
There was a problem hiding this comment.
I reckon the order should be account_id, inbox_id, client. or maybe its better to use named arguments. though its inconsistent with other APIs. Lets discuss.
Contributor
There was a problem hiding this comment.
And there is inbox_id in the client :)
Probably will address in the new major release.
lib/mailtrap/sandbox_messages_api.rb
Outdated
| raise ArgumentError, 'account_id is required' if account_id.nil? | ||
| raise ArgumentError, 'inbox_id is required' if inbox_id.nil? | ||
|
|
||
| @account_id = account_id |
Contributor
There was a problem hiding this comment.
can you use super(account_id, client)?
lib/mailtrap/sandbox_messages_api.rb
Outdated
| # @param message_id [Integer] The Sandbox message ID | ||
| # @return [Hash] Spam report | ||
| # @!macro api_errors | ||
| def get_spam_score(message_id) |
Contributor
There was a problem hiding this comment.
get_ prefix should be avoided.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_mail_headers/returns_headers.yml`:
- Around line 77-80: The fixture contains a real email in the YAML string under
the "to" header (the JSON string:
'"headers":{...,"to":"bob.s@railsware.com",...}'); replace that value with a
non-PII placeholder such as "test@example.com" while preserving the surrounding
quoting/escaping and YAML structure in returns_headers.yml, and then run a quick
search for "bob.s@railsware.com" across vcr cassettes to update any other
occurrences to the same placeholder.
Comment on lines
+77
to
+80
| string: '{"headers":{"date":"Tue, 30 Dec 2025 16:55:50 +0000","from":"reply@demomailtrap.co","to":"bob.s@railsware.com","subject":"Hello | ||
| from Mailtrap","mime_version":"1.0","content_type":"multipart/alternative; | ||
| boundary=123","bcc":"not | ||
| available for your plan"}}' |
There was a problem hiding this comment.
Real email address recorded in fixture response body — PII risk.
The to field contains bob.s@railsware.com, which appears to be a real person's email address. This should be replaced with a placeholder (e.g., test@example.com) to avoid leaking PII in the repository. As per coding guidelines, "Act as a data privacy officer. Carefully read all the vcr cassettes with recorded HTTP interactions and try to identify sensitive data that could potentially be recorded. It can be anything from PII to credentials."
🤖 Prompt for AI Agents
In
`@spec/fixtures/vcr_cassettes/Mailtrap_SandboxMessagesAPI/_mail_headers/returns_headers.yml`
around lines 77 - 80, The fixture contains a real email in the YAML string under
the "to" header (the JSON string:
'"headers":{...,"to":"bob.s@railsware.com",...}'); replace that value with a
non-PII placeholder such as "test@example.com" while preserving the surrounding
quoting/escaping and YAML structure in returns_headers.yml, and then run a quick
search for "bob.s@railsware.com" across vcr cassettes to update any other
occurrences to the same placeholder.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Motivation
Add Sandbox Messages API
Changes
Sandbox::Messageclass withSandboxMessageAPIendpointsSummary by CodeRabbit
New Features
Tests