feat: add native POP3 receiver adapters (MailKit + Rebex) - #187
Merged
Conversation
Port the external Pop3 receiver adapter into two in-process native adapters, selectable per subscription like other native adapters: - NativePop3Receiver: free, MailKit-based (MIT). No license required. - NativeRebexPop3Receiver: Rebex-based parity port of the existing external adapter. Registered only when REBEX_LICENSE_KEY is set, so it stays hidden from the adapter picker without a license. The key is read from the environment rather than per-subscription config. Connection is hardcoded to standard implicit-SSL POP3 (port 995), matching the original external adapter; Port/UseSsl are internal-only seams (InternalsVisibleTo) used by tests, not user-facing config. Adds an in-process fake POP3 server and 7 behavior tests per adapter (list/batch-size/body/attachment-utf8/attachment-base64/unknown-encoding/ delete). Rebex tests report Inconclusive when REBEX_LICENSE_KEY is unset so CI stays green without a license. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWhat changed:
Risk: risk:medium Security-sensitive areas touched:
Test coverage impact:
Operational concerns:
WalkthroughAdds two POP3 native receiver implementations (MailKit-based ChangesPOP3 Receiver Implementations
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SW.Bitween.NativeAdapters/Pop3Receiver/NativePop3Receiver.cs`:
- Around line 19-68: Add cancellation/timeout support to the POP3 network flow
in NativePop3Receiver so MailKit calls do not block indefinitely. Update
Initialize, ListFiles, GetFile, DeleteFile, and Finalize to accept and pass a
CancellationToken (or use a bounded timeout) through _client.ConnectAsync,
AuthenticateAsync, GetMessageAsync, DeleteMessageAsync, and DisconnectAsync.
Make the same change pattern in NativeRebexPop3Receiver and propagate token
support from the INativeInfolinkReceiver boundary.
- Around line 33-37: The ListFiles method in NativePop3Receiver can crash when
_options.BatchSize is negative because Math.Min(_client!.Count,
_options.BatchSize) may produce a negative count that gets passed into
Enumerable.Range. Update ListFiles to defensively clamp the computed count to
zero or otherwise ensure it is non-negative before calling Enumerable.Range,
keeping the behavior aligned with how the Rebex-based implementation tolerates
negative batch sizes.
- Around line 19-25: The Initialize method in NativePop3Receiver can leak a
connected Pop3Client when AuthenticateAsync fails after ConnectAsync succeeds.
Update Initialize to handle failures by disposing or disconnecting the _client
if ConnectAsync/AuthenticateAsync throws, and ensure the client is left in a
clean state before rethrowing. Use the NativePop3Receiver.Initialize flow around
_client, ConnectAsync, and AuthenticateAsync to place the cleanup in the failure
path.
In `@SW.Bitween.NativeAdapters/Pop3Receiver/Pop3ReceiverInput.cs`:
- Around line 18-19: The BatchSize property in Pop3ReceiverInput currently has
no validation, so negative values can flow into NativePop3Receiver.ListFiles and
break Enumerable.Range. Add a positive-range validation attribute to BatchSize
(for example, a minimum of 1) and keep the existing default value intact so
invalid input is rejected before it reaches ListFiles.
In `@SW.Bitween.NativeAdapters/RebexPop3Receiver/NativeRebexPop3Receiver.cs`:
- Around line 19-26: Initialize in NativeRebexPop3Receiver has the same
partial-initialization leak risk as NativePop3Receiver: if _pop3.ConnectAsync
succeeds and _pop3.LoginAsync fails, the connected Pop3 instance is left
undisposed. Update Initialize to wrap the connection/login sequence in
try/catch, and on any failure dispose or disconnect _pop3 before rethrowing so
the method leaves no partially initialized connection behind. Use the existing
_pop3, ConnectAsync, and LoginAsync flow as the fix point.
- Around line 43-57: The GetFile method in NativeRebexPop3Receiver is performing
an unnecessary raw-message download via _pop3.GetMessageAsync(sequenceNumber,
attachment.FileName), which can write to a sender-controlled path. Remove that
call and keep using attachment.GetContentStream() in GetFile to read the
attachment bytes directly, preserving the existing XchangeFile flow without
touching the local filesystem.
In `@SW.Bitween.NativeAdapters/RebexPop3Receiver/RebexPop3ReceiverInput.cs`:
- Around line 6-23: `RebexPop3ReceiverInput` is duplicating the same DTO
contract as `Pop3ReceiverInput`, so extract the shared fields into a common base
type and have both adapter inputs inherit from it. Move the `Host`, `Username`,
`Password`, `BatchSize`, and `ResponseEncoding` properties (with their existing
attributes/defaults) into a shared class such as `Pop3ReceiverInputBase`, then
keep `RebexPop3ReceiverInput` and `Pop3ReceiverInput` as thin wrappers to
preserve their adapter-specific types.
In `@SW.Bitween.UnitTests/FakePop3Server.cs`:
- Around line 43-59: The fire-and-forget call in AcceptLoop ignores exceptions
from HandleClient, so failures are lost and tests may just hang. Update
AcceptLoop/FakePop3Server to observe and log or aggregate task faults from
HandleClient, ideally by storing the task and attaching a continuation or
awaiting it in a coordinated way so any read/write/protocol exceptions are
surfaced instead of disappearing silently.
- Around line 146-164: The RETR handler in FakePop3Server.HandleRetr is emitting
an extra empty line because splitting the message after normalizing CRLF leaves
a trailing empty element. Update the loop to avoid writing that final blank
entry (for example by skipping empty trailing lines or otherwise iterating only
over the real message lines), while still preserving dot-stuffing and the final
"." terminator.
In `@SW.Bitween.UnitTests/NativePop3ReceiverTests.cs`:
- Around line 14-28: The NativePop3ReceiverTests scaffolding is duplicated with
NativeRebexPop3ReceiverTests, so the shared test setup and test bodies should be
centralized. Extract the common helpers like Settings and the receiver creation
logic into a shared base test class or reusable helper/factory, and have the
Native/Rebex-specific tests only supply the receiver type via a factory or
override. Keep the duplicated test method logic in one place so both receiver
implementations reuse the same assertions without maintaining two copies.
- Around line 30-126: The POP3 receiver tests do not guarantee cleanup if an
assertion or awaited call fails, so the connection can be left open. Update the
test methods in NativePop3ReceiverTests to ensure receiver.Finalize() always
runs by wrapping the Initialize/ListFiles/GetFile/DeleteFile sequence in a
try/finally block (or equivalent cleanup helper) using the receiver variable.
Keep the existing assertions intact, but make sure each test disposes the
receiver even when Assert calls fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: simplify9/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bec7ab96-0e54-4fa1-bd55-d9b6c30e8bed
📒 Files selected for processing (10)
SW.Bitween.NativeAdapters/Pop3Receiver/NativePop3Receiver.csSW.Bitween.NativeAdapters/Pop3Receiver/Pop3ReceiverInput.csSW.Bitween.NativeAdapters/RebexPop3Receiver/NativeRebexPop3Receiver.csSW.Bitween.NativeAdapters/RebexPop3Receiver/RebexPop3ReceiverInput.csSW.Bitween.NativeAdapters/SW.Bitween.NativeAdapters.csprojSW.Bitween.NativeAdapters/ServiceCollectionExtensions.csSW.Bitween.UnitTests/FakePop3Server.csSW.Bitween.UnitTests/NativePop3ReceiverTests.csSW.Bitween.UnitTests/NativeRebexPop3ReceiverTests.csSW.Bitween.UnitTests/Pop3TestMessages.cs
📜 Review details
🔇 Additional comments (12)
SW.Bitween.UnitTests/NativeRebexPop3ReceiverTests.cs (2)
26-138: 📐 Maintainability & Code QualityDuplicate of scaffolding/cleanup concerns in
NativePop3ReceiverTests.cs.
Settings,CreateReceiver, and all six test bodies mirrorNativePop3ReceiverTests.csverbatim (only the receiver type differs), and the same lack of try/finally around Initialize/Finalize applies here too.
19-24: LGTM!License gating matches
NativeRebexPop3Receiver.LicenseKeyEnvironmentVariable.SW.Bitween.UnitTests/NativePop3ReceiverTests.cs (1)
100-111: LGTM!SW.Bitween.NativeAdapters/Pop3Receiver/NativePop3Receiver.cs (1)
39-63: LGTM!SW.Bitween.NativeAdapters/RebexPop3Receiver/NativeRebexPop3Receiver.cs (2)
68-81: LGTM!
28-32: 🗄️ Data Integrity & IntegrationNo issue:
DisconnectAsync(false)commits pending deletions
DisconnectAsync(false)is the commit path for POP3; it sends the equivalent ofQUITand preservesDeleteFilechanges. The rollback path isDisconnectAsync(true), so this change does not silently drop deletions.> Likely an incorrect or invalid review comment.SW.Bitween.NativeAdapters/ServiceCollectionExtensions.cs (1)
3-4: LGTM!Also applies to: 35-43
SW.Bitween.NativeAdapters/SW.Bitween.NativeAdapters.csproj (2)
12-15: LGTM!
16-21: 🔒 Security & PrivacyNo package-version action needed MailKit 4.17.0, Rebex.Mail 6.0.8060, and Rebex.Pop3 6.0.8060 are published NuGet versions and currently show no OSV advisories.
SW.Bitween.UnitTests/FakePop3Server.cs (2)
1-42: LGTM!
166-171: LGTM!SW.Bitween.UnitTests/Pop3TestMessages.cs (1)
1-38: LGTM!
Comment on lines
+19
to
+68
| public async Task Initialize() | ||
| { | ||
| _client = new Pop3Client(); | ||
| var sslOptions = UseSsl ? SecureSocketOptions.SslOnConnect : SecureSocketOptions.None; | ||
| await _client.ConnectAsync(_options.Host, Port, sslOptions); | ||
| await _client.AuthenticateAsync(_options.Username, _options.Password); | ||
| } | ||
|
|
||
| public async Task Finalize() | ||
| { | ||
| await _client!.DisconnectAsync(true); | ||
| _client.Dispose(); | ||
| } | ||
|
|
||
| public Task<IEnumerable<string>> ListFiles() | ||
| { | ||
| var count = Math.Min(_client!.Count, _options.BatchSize); | ||
| return Task.FromResult(Enumerable.Range(0, count).Select(i => i.ToString())); | ||
| } | ||
|
|
||
| public async Task<XchangeFile> GetFile(string fileId) | ||
| { | ||
| var index = int.Parse(fileId); | ||
| var message = await _client!.GetMessageAsync(index); | ||
|
|
||
| var attachment = message.Attachments.FirstOrDefault(); | ||
| if (attachment == null) | ||
| return new XchangeFile(message.TextBody ?? message.HtmlBody ?? string.Empty, message.Subject); | ||
|
|
||
| using var memoryStream = new MemoryStream(); | ||
| if (attachment is MessagePart rfc822) | ||
| await rfc822.Message.WriteToAsync(memoryStream); | ||
| else | ||
| await ((MimePart)attachment).Content.DecodeToAsync(memoryStream); | ||
|
|
||
| var buffer = memoryStream.ToArray(); | ||
|
|
||
| return _options.ResponseEncoding.ToLower() switch | ||
| { | ||
| "base64" => new XchangeFile(Convert.ToBase64String(buffer), message.Subject), | ||
| "utf8" => new XchangeFile(Encoding.UTF8.GetString(buffer), message.Subject), | ||
| _ => throw new ArgumentException( | ||
| $"Unknown {nameof(Pop3ReceiverInput.ResponseEncoding)} '{_options.ResponseEncoding}'") | ||
| }; | ||
| } | ||
|
|
||
| public async Task DeleteFile(string fileId) | ||
| { | ||
| await _client!.DeleteMessageAsync(int.Parse(fileId)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
No timeout/cancellation on any network call.
Initialize, ListFiles, GetFile, DeleteFile, and Finalize all invoke MailKit async calls without a CancellationToken. A hung/unreachable POP3 server blocks indefinitely, tying up the calling thread. Same pattern applies to NativeRebexPop3Receiver. Worth adding cancellation-token support (and a per-call timeout) at the INativeInfolinkReceiver boundary if this receiver runs on request/worker threads with SLAs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.NativeAdapters/Pop3Receiver/NativePop3Receiver.cs` around lines 19
- 68, Add cancellation/timeout support to the POP3 network flow in
NativePop3Receiver so MailKit calls do not block indefinitely. Update
Initialize, ListFiles, GetFile, DeleteFile, and Finalize to accept and pass a
CancellationToken (or use a bounded timeout) through _client.ConnectAsync,
AuthenticateAsync, GetMessageAsync, DeleteMessageAsync, and DisconnectAsync.
Make the same change pattern in NativeRebexPop3Receiver and propagate token
support from the INativeInfolinkReceiver boundary.
Comment on lines
+33
to
+37
| public Task<IEnumerable<string>> ListFiles() | ||
| { | ||
| var count = Math.Min(_client!.Count, _options.BatchSize); | ||
| return Task.FromResult(Enumerable.Range(0, count).Select(i => i.ToString())); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Negative BatchSize crashes ListFiles.
Math.Min(_client!.Count, _options.BatchSize) can be negative, and Enumerable.Range(0, count) throws ArgumentOutOfRangeException for a negative count (contrast with Rebex's .Take(), which tolerates negatives). Clamp defensively:
- var count = Math.Min(_client!.Count, _options.BatchSize);
+ var count = Math.Max(0, Math.Min(_client!.Count, _options.BatchSize));📝 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
| public Task<IEnumerable<string>> ListFiles() | |
| { | |
| var count = Math.Min(_client!.Count, _options.BatchSize); | |
| return Task.FromResult(Enumerable.Range(0, count).Select(i => i.ToString())); | |
| } | |
| public Task<IEnumerable<string>> ListFiles() | |
| { | |
| var count = Math.Max(0, Math.Min(_client!.Count, _options.BatchSize)); | |
| return Task.FromResult(Enumerable.Range(0, count).Select(i => i.ToString())); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.NativeAdapters/Pop3Receiver/NativePop3Receiver.cs` around lines 33
- 37, The ListFiles method in NativePop3Receiver can crash when
_options.BatchSize is negative because Math.Min(_client!.Count,
_options.BatchSize) may produce a negative count that gets passed into
Enumerable.Range. Update ListFiles to defensively clamp the computed count to
zero or otherwise ensure it is non-negative before calling Enumerable.Range,
keeping the behavior aligned with how the Rebex-based implementation tolerates
negative batch sizes.
Comment on lines
+18
to
+19
| [DefaultValue(50)] | ||
| public int BatchSize { get; set; } = 50; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
No bound validation on BatchSize.
Negative values pass through unvalidated and cause NativePop3Receiver.ListFiles (Line 35) to crash via Enumerable.Range(0, count). Consider adding [Range(1, int.MaxValue)].
Proposed fix
+ [Range(1, int.MaxValue)]
[DefaultValue(50)]
public int BatchSize { get; set; } = 50;📝 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
| [DefaultValue(50)] | |
| public int BatchSize { get; set; } = 50; | |
| [Range(1, int.MaxValue)] | |
| [DefaultValue(50)] | |
| public int BatchSize { get; set; } = 50; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.NativeAdapters/Pop3Receiver/Pop3ReceiverInput.cs` around lines 18
- 19, The BatchSize property in Pop3ReceiverInput currently has no validation,
so negative values can flow into NativePop3Receiver.ListFiles and break
Enumerable.Range. Add a positive-range validation attribute to BatchSize (for
example, a minimum of 1) and keep the existing default value intact so invalid
input is rejected before it reaches ListFiles.
Comment on lines
+19
to
+26
| public async Task Initialize() | ||
| { | ||
| Rebex.Licensing.Key = Environment.GetEnvironmentVariable(LicenseKeyEnvironmentVariable); | ||
| _pop3 = new Pop3(); | ||
| var sslMode = UseSsl ? SslMode.Implicit : SslMode.None; | ||
| await _pop3.ConnectAsync(_options.Host, Port, sslMode); | ||
| await _pop3.LoginAsync(_options.Username, _options.Password); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Same partial-init leak risk as NativePop3Receiver.
If LoginAsync throws after ConnectAsync succeeds, _pop3 stays connected but the exception unwinds past any explicit cleanup; wrap in try/catch and dispose on failure, mirroring the fix suggested for NativePop3Receiver.Initialize.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.NativeAdapters/RebexPop3Receiver/NativeRebexPop3Receiver.cs`
around lines 19 - 26, Initialize in NativeRebexPop3Receiver has the same
partial-initialization leak risk as NativePop3Receiver: if _pop3.ConnectAsync
succeeds and _pop3.LoginAsync fails, the connected Pop3 instance is left
undisposed. Update Initialize to wrap the connection/login sequence in
try/catch, and on any failure dispose or disconnect _pop3 before rethrowing so
the method leaves no partially initialized connection behind. Use the existing
_pop3, ConnectAsync, and LoginAsync flow as the fix point.
Comment on lines
+6
to
+23
| public class RebexPop3ReceiverInput | ||
| { | ||
| [Required] | ||
| public string Host { get; set; } = string.Empty; | ||
|
|
||
| [Required] | ||
| public string Username { get; set; } = string.Empty; | ||
|
|
||
| [Required] | ||
| [Secure] | ||
| public string Password { get; set; } = string.Empty; | ||
|
|
||
| [DefaultValue(50)] | ||
| public int BatchSize { get; set; } = 50; | ||
|
|
||
| [DefaultValue("utf8")] | ||
| public string ResponseEncoding { get; set; } = "utf8"; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Duplicate DTO shape with Pop3ReceiverInput.
Identical to SW.Bitween.NativeAdapters/Pop3Receiver/Pop3ReceiverInput.cs. Consider a shared base (e.g. Pop3ReceiverInputBase) with Host/Username/Password/BatchSize/ResponseEncoding to keep the two adapters' contracts in sync as they evolve.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.NativeAdapters/RebexPop3Receiver/RebexPop3ReceiverInput.cs` around
lines 6 - 23, `RebexPop3ReceiverInput` is duplicating the same DTO contract as
`Pop3ReceiverInput`, so extract the shared fields into a common base type and
have both adapter inputs inherit from it. Move the `Host`, `Username`,
`Password`, `BatchSize`, and `ResponseEncoding` properties (with their existing
attributes/defaults) into a shared class such as `Pop3ReceiverInputBase`, then
keep `RebexPop3ReceiverInput` and `Pop3ReceiverInput` as thin wrappers to
preserve their adapter-specific types.
Comment on lines
+43
to
+59
| private async Task AcceptLoop(CancellationToken ct) | ||
| { | ||
| try | ||
| { | ||
| while (!ct.IsCancellationRequested) | ||
| { | ||
| var client = await _listener.AcceptTcpClientAsync(ct); | ||
| _ = HandleClient(client, ct); | ||
| } | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| } | ||
| catch (ObjectDisposedException) | ||
| { | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Fire-and-forget HandleClient swallows exceptions.
_ = HandleClient(client, ct) (line 50) is unobserved. Faults inside HandleClient (write/read errors, protocol edge cases) disappear silently, making failing tests hard to diagnose (they'll typically just hang/time out).
♻️ Suggested fix
var client = await _listener.AcceptTcpClientAsync(ct);
- _ = HandleClient(client, ct);
+ _ = HandleClient(client, ct).ContinueWith(t =>
+ {
+ if (t.Exception != null)
+ Console.Error.WriteLine($"FakePop3Server client handler failed: {t.Exception}");
+ }, TaskContinuationOptions.OnlyOnFaulted);📝 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
| private async Task AcceptLoop(CancellationToken ct) | |
| { | |
| try | |
| { | |
| while (!ct.IsCancellationRequested) | |
| { | |
| var client = await _listener.AcceptTcpClientAsync(ct); | |
| _ = HandleClient(client, ct); | |
| } | |
| } | |
| catch (OperationCanceledException) | |
| { | |
| } | |
| catch (ObjectDisposedException) | |
| { | |
| } | |
| } | |
| private async Task AcceptLoop(CancellationToken ct) | |
| { | |
| try | |
| { | |
| while (!ct.IsCancellationRequested) | |
| { | |
| var client = await _listener.AcceptTcpClientAsync(ct); | |
| _ = HandleClient(client, ct).ContinueWith(t => | |
| { | |
| if (t.Exception != null) | |
| Console.Error.WriteLine($"FakePop3Server client handler failed: {t.Exception}"); | |
| }, TaskContinuationOptions.OnlyOnFaulted); | |
| } | |
| } | |
| catch (OperationCanceledException) | |
| { | |
| } | |
| catch (ObjectDisposedException) | |
| { | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.UnitTests/FakePop3Server.cs` around lines 43 - 59, The
fire-and-forget call in AcceptLoop ignores exceptions from HandleClient, so
failures are lost and tests may just hang. Update AcceptLoop/FakePop3Server to
observe and log or aggregate task faults from HandleClient, ideally by storing
the task and attaching a continuation or awaiting it in a coordinated way so any
read/write/protocol exceptions are surfaced instead of disappearing silently.
Comment on lines
+146
to
+164
| private async Task HandleRetr(StreamWriter writer, string argument) | ||
| { | ||
| if (!int.TryParse(argument, out var num) || num < 1 || num > _messages.Count || _deleted.Contains(num)) | ||
| { | ||
| await writer.WriteLineAsync("-ERR no such message"); | ||
| return; | ||
| } | ||
|
|
||
| var message = _messages[num - 1]; | ||
| await writer.WriteLineAsync($"+OK {Encoding.ASCII.GetByteCount(message)} octets"); | ||
|
|
||
| foreach (var messageLine in message.Replace("\r\n", "\n").Split('\n')) | ||
| { | ||
| var stuffed = messageLine.StartsWith('.') ? "." + messageLine : messageLine; | ||
| await writer.WriteLineAsync(stuffed); | ||
| } | ||
|
|
||
| await writer.WriteLineAsync("."); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Extra trailing blank line written before RETR terminator.
All test fixtures (Pop3TestMessages.Plain, WithAttachment) end with \r\n. After Replace("\r\n","\n").Split('\n'), the trailing terminator produces an extra empty string element, so a spurious blank line is transmitted right before the . terminator on every RETR. It happens to be tolerated by MIME/base64 parsers here (trailing whitespace/epilogue is ignored), but it's not faithful to the real message bytes and is a latent trap for future assertions on exact body length/content.
🐛 Suggested fix
- foreach (var messageLine in message.Replace("\r\n", "\n").Split('\n'))
+ var lines = message.Replace("\r\n", "\n").Split('\n');
+ if (lines.Length > 0 && lines[^1].Length == 0)
+ lines = lines[..^1];
+
+ foreach (var messageLine in lines)
{
var stuffed = messageLine.StartsWith('.') ? "." + messageLine : messageLine;
await writer.WriteLineAsync(stuffed);
}📝 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
| private async Task HandleRetr(StreamWriter writer, string argument) | |
| { | |
| if (!int.TryParse(argument, out var num) || num < 1 || num > _messages.Count || _deleted.Contains(num)) | |
| { | |
| await writer.WriteLineAsync("-ERR no such message"); | |
| return; | |
| } | |
| var message = _messages[num - 1]; | |
| await writer.WriteLineAsync($"+OK {Encoding.ASCII.GetByteCount(message)} octets"); | |
| foreach (var messageLine in message.Replace("\r\n", "\n").Split('\n')) | |
| { | |
| var stuffed = messageLine.StartsWith('.') ? "." + messageLine : messageLine; | |
| await writer.WriteLineAsync(stuffed); | |
| } | |
| await writer.WriteLineAsync("."); | |
| } | |
| private async Task HandleRetr(StreamWriter writer, string argument) | |
| { | |
| if (!int.TryParse(argument, out var num) || num < 1 || num > _messages.Count || _deleted.Contains(num)) | |
| { | |
| await writer.WriteLineAsync("-ERR no such message"); | |
| return; | |
| } | |
| var message = _messages[num - 1]; | |
| await writer.WriteLineAsync($"+OK {Encoding.ASCII.GetByteCount(message)} octets"); | |
| var lines = message.Replace("\r\n", "\n").Split('\n'); | |
| if (lines.Length > 0 && lines[^1].Length == 0) | |
| lines = lines[..^1]; | |
| foreach (var messageLine in lines) | |
| { | |
| var stuffed = messageLine.StartsWith('.') ? "." + messageLine : messageLine; | |
| await writer.WriteLineAsync(stuffed); | |
| } | |
| await writer.WriteLineAsync("."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.UnitTests/FakePop3Server.cs` around lines 146 - 164, The RETR
handler in FakePop3Server.HandleRetr is emitting an extra empty line because
splitting the message after normalizing CRLF leaves a trailing empty element.
Update the loop to avoid writing that final blank entry (for example by skipping
empty trailing lines or otherwise iterating only over the real message lines),
while still preserving dot-stuffing and the final "." terminator.
Comment on lines
+14
to
+28
| private static Dictionary<string, string> Settings(string encoding = "utf8", int batchSize = 50) => new() | ||
| { | ||
| ["Host"] = "127.0.0.1", | ||
| ["Username"] = "user", | ||
| ["Password"] = "pass", | ||
| ["ResponseEncoding"] = encoding, | ||
| ["BatchSize"] = batchSize.ToString() | ||
| }; | ||
|
|
||
| private static NativePop3Receiver CreateReceiver(FakePop3Server server, string encoding = "utf8", int batchSize = 50) | ||
| { | ||
| var receiver = new NativePop3Receiver { Port = server.Port, UseSsl = false }; | ||
| receiver.InitializeStartupValues(Settings(encoding, batchSize)); | ||
| return receiver; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated test scaffolding across Native/Rebex test classes.
Settings and CreateReceiver here are byte-for-byte identical to their counterparts in NativeRebexPop3ReceiverTests.cs (same for all six test method bodies), differing only in the receiver type. Consider extracting a shared abstract base test class (or a helper taking a Func<FakePop3Server, INativeInfolinkReceiver> factory) to avoid maintaining two copies of the same test logic going forward.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.UnitTests/NativePop3ReceiverTests.cs` around lines 14 - 28, The
NativePop3ReceiverTests scaffolding is duplicated with
NativeRebexPop3ReceiverTests, so the shared test setup and test bodies should be
centralized. Extract the common helpers like Settings and the receiver creation
logic into a shared base test class or reusable helper/factory, and have the
Native/Rebex-specific tests only supply the receiver type via a factory or
override. Keep the duplicated test method logic in one place so both receiver
implementations reuse the same assertions without maintaining two copies.
Comment on lines
+30
to
+126
| [TestMethod] | ||
| public async Task ListFiles_ReturnsOneEntryPerMessage() | ||
| { | ||
| using var server = new FakePop3Server(new[] { Pop3TestMessages.Plain, Pop3TestMessages.WithAttachment }); | ||
| var receiver = CreateReceiver(server); | ||
|
|
||
| await receiver.Initialize(); | ||
| var files = (await receiver.ListFiles()).ToList(); | ||
| await receiver.Finalize(); | ||
|
|
||
| Assert.AreEqual(2, files.Count); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task ListFiles_RespectsBatchSize() | ||
| { | ||
| using var server = new FakePop3Server(new[] | ||
| { Pop3TestMessages.Plain, Pop3TestMessages.WithAttachment, Pop3TestMessages.Plain }); | ||
| var receiver = CreateReceiver(server, batchSize: 2); | ||
|
|
||
| await receiver.Initialize(); | ||
| var files = (await receiver.ListFiles()).ToList(); | ||
| await receiver.Finalize(); | ||
|
|
||
| Assert.AreEqual(2, files.Count); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task GetFile_WithoutAttachment_ReturnsBodyText() | ||
| { | ||
| using var server = new FakePop3Server(new[] { Pop3TestMessages.Plain }); | ||
| var receiver = CreateReceiver(server); | ||
|
|
||
| await receiver.Initialize(); | ||
| var files = (await receiver.ListFiles()).ToList(); | ||
| var file = await receiver.GetFile(files[0]); | ||
| await receiver.Finalize(); | ||
|
|
||
| StringAssert.Contains(file.Data, "Hello, this is the body text."); | ||
| Assert.AreEqual("Plain Message", file.Filename); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task GetFile_WithAttachment_ReturnsAttachmentAsUtf8() | ||
| { | ||
| using var server = new FakePop3Server(new[] { Pop3TestMessages.WithAttachment }); | ||
| var receiver = CreateReceiver(server, "utf8"); | ||
|
|
||
| await receiver.Initialize(); | ||
| var files = (await receiver.ListFiles()).ToList(); | ||
| var file = await receiver.GetFile(files[0]); | ||
| await receiver.Finalize(); | ||
|
|
||
| Assert.AreEqual(Pop3TestMessages.AttachmentDecoded, file.Data); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task GetFile_WithAttachment_ReturnsAttachmentAsBase64() | ||
| { | ||
| using var server = new FakePop3Server(new[] { Pop3TestMessages.WithAttachment }); | ||
| var receiver = CreateReceiver(server, "base64"); | ||
|
|
||
| await receiver.Initialize(); | ||
| var files = (await receiver.ListFiles()).ToList(); | ||
| var file = await receiver.GetFile(files[0]); | ||
| await receiver.Finalize(); | ||
|
|
||
| Assert.AreEqual(Convert.ToBase64String(Encoding.UTF8.GetBytes(Pop3TestMessages.AttachmentDecoded)), file.Data); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task GetFile_WithUnknownEncoding_Throws() | ||
| { | ||
| using var server = new FakePop3Server(new[] { Pop3TestMessages.WithAttachment }); | ||
| var receiver = CreateReceiver(server, "unknown-encoding"); | ||
|
|
||
| await receiver.Initialize(); | ||
| var files = (await receiver.ListFiles()).ToList(); | ||
|
|
||
| await Assert.ThrowsExceptionAsync<ArgumentException>(() => receiver.GetFile(files[0])); | ||
| await receiver.Finalize(); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task DeleteFile_MarksMessageDeletedOnServer() | ||
| { | ||
| using var server = new FakePop3Server(new[] { Pop3TestMessages.Plain }); | ||
| var receiver = CreateReceiver(server); | ||
|
|
||
| await receiver.Initialize(); | ||
| var files = (await receiver.ListFiles()).ToList(); | ||
| await receiver.DeleteFile(files[0]); | ||
| await receiver.Finalize(); | ||
|
|
||
| CollectionAssert.Contains(server.DeletedMessageNumbers.ToList(), 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
No cleanup on assertion failure.
None of the test methods wrap receiver.Initialize()/GetFile/DeleteFile in try/finally. If an Assert fails mid-test (e.g. Line 40, Line 68), receiver.Finalize() is skipped, leaking the client's socket/connection until GC. Given tests spin up real TCP listeners (FakePop3Server), consider a finally { await receiver.Finalize(); } pattern to avoid accumulating orphaned connections/threads across a failing run.
Example pattern
- await receiver.Initialize();
- var files = (await receiver.ListFiles()).ToList();
- await receiver.Finalize();
-
- Assert.AreEqual(2, files.Count);
+ await receiver.Initialize();
+ try
+ {
+ var files = (await receiver.ListFiles()).ToList();
+ Assert.AreEqual(2, files.Count);
+ }
+ finally
+ {
+ await receiver.Finalize();
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.UnitTests/NativePop3ReceiverTests.cs` around lines 30 - 126, The
POP3 receiver tests do not guarantee cleanup if an assertion or awaited call
fails, so the connection can be left open. Update the test methods in
NativePop3ReceiverTests to ensure receiver.Finalize() always runs by wrapping
the Initialize/ListFiles/GetFile/DeleteFile sequence in a try/finally block (or
equivalent cleanup helper) using the receiver variable. Keep the existing
assertions intact, but make sure each test disposes the receiver even when
Assert calls fail.
AhmadRAbuhussein
approved these changes
Jul 2, 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.
Port the external Pop3 receiver adapter into two in-process native adapters, selectable per subscription like other native adapters:
Connection is hardcoded to standard implicit-SSL POP3 (port 995), matching the original external adapter; Port/UseSsl are internal-only seams (InternalsVisibleTo) used by tests, not user-facing config.
Adds an in-process fake POP3 server and 7 behavior tests per adapter (list/batch-size/body/attachment-utf8/attachment-base64/unknown-encoding/ delete). Rebex tests report Inconclusive when REBEX_LICENSE_KEY is unset so CI stays green without a license.