Conversation
Rsync already mocked @rsyncd OK. An ftp:// proxy and a canceled ctx must fail closed, and a closed port must not be reported as unauthenticated access.
|
Warning Review limit reachedNext included review available in 15 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. Your 39 included PR review attempts over the past 7 days set your current allowance at 8 reviews per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
Claude Review
Critical issues
None.
Security
No security concerns flagged.
Test coverage
No critical issues — LGTM pending human review. Test-only PR; the three added cases correctly exercise real Test()/CheckUnauth() paths (bad proxy scheme yields connection error, canceled context yields error, closed port is not a finding) and match the plugin error-wrapping contract.
There was a problem hiding this comment.
Codex Review
Critical Issues
None.
Security
No security concerns flagged.
Suggestions
internal/plugins/rsync/rsync_test.go:35: The cancellation test can pass from an ordinary connection-refused error even if context handling breaks. Asserterrors.Is(r.Error, context.Canceled)to verify the intended behavior.
Focused tests could not run because the environment’s filesystem is read-only, including Go’s cache directories.
Reviewed by Codex (gpt-5.6-sol)
There was a problem hiding this comment.
Gemini Review
No critical issues — LGTM pending human review.
Critical Issues
None.
Security
No security concerns flagged.
Suggestions
- Verify specific error in context cancellation test: In
TestPlugin_Test_CanceledContextNoServer(internal/plugins/rsync/rsync_test.goat line 39), instead of only asserting that the error is non-nil withassert.NotNil(t, r.Error), consider verifying that the returned error specifically wraps the context cancellation error. Sincebrutus.WrapConnErroruses%wto wrap the underlying network/dial error, you can assert this using:This guarantees that the test fails precisely due to the context cancellation rather than any other unexpected connection error.assert.ErrorIs(t, r.Error, context.Canceled)
Reviewed by Gemini (gemini-3.8-flash)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1dea066c0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| assert.False(t, r.Success) | ||
| require.NotNil(t, r.Error) | ||
| assert.Contains(t, r.Error.Error(), "connection error") |
There was a problem hiding this comment.
Verify that the invalid proxy causes the failure
If Plugin.Test starts ignoring ProxyURL, the direct connection to the usually closed 127.0.0.1:873 still produces a wrapped "connection error", so this test passes without detecting the proxy regression it is intended to cover. Assert that the error reports the unsupported ftp scheme, or use a live target whose direct path would succeed.
Useful? React with 👍 / 👎.
| r := (&Plugin{}).Test(ctx, "127.0.0.1:1", "mod", "p", time.Second, brutus.PluginConfig{}) | ||
| assert.False(t, r.Success) | ||
| assert.NotNil(t, r.Error) |
There was a problem hiding this comment.
Verify that cancellation causes the failure
If the rsync connection path stops forwarding or honoring ctx, dialing the closed port still returns a non-nil error and satisfies both assertions, so this test would not catch the cancellation regression it is named to cover. Check the wrapped error with assert.ErrorIs(t, r.Error, context.Canceled) so an ordinary connection refusal cannot satisfy the test.
Useful? React with 👍 / 👎.
Summary
Rsync `Test()`/`CheckUnauth` never exercised a bad proxy, canceled context, or closed-port unauth.
Test plan