Skip to content

feat: allow pass hostname in docker env#6

Open
tomerqodo wants to merge 2 commits intoaugment_full_base_feat_allow_pass_hostname_in_docker_env_pr6from
augment_full_head_feat_allow_pass_hostname_in_docker_env_pr6
Open

feat: allow pass hostname in docker env#6
tomerqodo wants to merge 2 commits intoaugment_full_base_feat_allow_pass_hostname_in_docker_env_pr6from
augment_full_head_feat_allow_pass_hostname_in_docker_env_pr6

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#6

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented Jan 26, 2026

🤖 Augment PR Summary

Summary: Adds an optional SMTP client hostname override to support environments where the default HELO/EHLO identity is rejected.

Changes:

  • Introduces SMTP_LOCAL_HOSTNAME in mail configuration settings and .env examples
  • Plumbs the setting into the SMTP client via local_hostname and updated EHLO/STARTTLS behavior
  • Exposes the variable through Docker compose shared environment and updates unit/integration tests

Technical Notes: The SMTP connection now passes local_hostname into smtplib.SMTP/SMTP_SSL, and STARTTLS flow performs EHLO before and after starttls().

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread api/libs/smtp.py

assert smtp is not None
if self.use_tls and self.opportunistic_tls:
smtp.ehlo(local_host)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When SMTP_LOCAL_HOSTNAME is unset, local_host becomes "", so smtp.ehlo(local_host) sends an empty HELO/EHLO name; some servers reject that. Consider letting smtplib pick its default EHLO name when no override is configured rather than explicitly passing an empty string.

Other Locations
  • api/libs/smtp.py:36

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

client.send(_mail())

mock_smtp_cls.assert_called_once_with("smtp.example.com", 25, timeout=10)
mock_smtp_cls.assert_called_once_with("smtp.example.com", 25, timeout=10, local_hostname=ANY)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These assertions now use local_hostname=ANY, which means the tests won’t catch regressions where the configured SMTP_LOCAL_HOSTNAME value isn’t actually propagated. Consider adding at least one test case with a non-empty SMTP_LOCAL_HOSTNAME and asserting it’s passed through to the SMTP constructor/ehlo.

Other Locations
  • api/tests/unit_tests/tasks/test_mail_send_task.py:154
  • api/tests/unit_tests/tasks/test_mail_send_task.py:184
  • api/tests/unit_tests/tasks/test_mail_send_task.py:216

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants