Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,13 @@ jobs:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v1
- uses: actions/checkout@v4

- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: 2.7.8
rubygems: latest

- name: Cache gem dependencies
uses: actions/cache@v1
with:
path: vendor/bundle
key: ${{ runner.os }}-bundler-${{ hashFiles('**/Gemfile.lock') }}
restore-keys: ${{ runner.os }}-bundler-

- name: Install dependencies
run: gem install bundler && bundle install --jobs 4 --retry 3 --path vendor/bundle
ruby-version: 3.2
bundler-cache: true

- name: Run tests
run: bundle exec rake test
161 changes: 108 additions & 53 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,83 +1,138 @@
GIT
remote: https://github.com/basecamp/house-style.git
revision: b3ad65254828e8e8019a0d9a6205aff9ad206a77
revision: 6f73ca5c3fd5002b48aeede2d980f9b8fe047d55
branch: main
specs:
rubocop-37signals (1.0.0)
rubocop
rubocop-minitest
rubocop-performance
rubocop-rails
rubocop-37signals (1.2.1)
rubocop (>= 1.72)
rubocop-minitest (>= 0.37.0)
rubocop-performance (>= 1.24)
rubocop-rails (>= 2.30)

PATH
remote: .
specs:
rspamd-ruby (1.0.0)
activesupport (>= 6.0)

GEM
remote: https://rubygems.org/
specs:
activesupport (7.0.7.2)
concurrent-ruby (~> 1.0, >= 1.0.2)
activesupport (8.1.2)
base64
bigdecimal
concurrent-ruby (~> 1.0, >= 1.3.1)
connection_pool (>= 2.2.5)
drb
i18n (>= 1.6, < 2)
json
logger (>= 1.4.2)
minitest (>= 5.1)
tzinfo (~> 2.0)
addressable (2.8.1)
public_suffix (>= 2.0.2, < 6.0)
ast (2.4.2)
concurrent-ruby (1.2.2)
crack (0.4.3)
safe_yaml (~> 1.0.0)
debug (1.7.1)
irb (>= 1.5.0)
reline (>= 0.3.1)
hashdiff (1.0.0)
i18n (1.14.1)
securerandom (>= 0.3)
tzinfo (~> 2.0, >= 2.0.5)
uri (>= 0.13.1)
addressable (2.8.9)
public_suffix (>= 2.0.2, < 8.0)
ast (2.4.3)
base64 (0.3.0)
bigdecimal (4.0.1)
concurrent-ruby (1.3.6)
connection_pool (3.0.2)
crack (1.0.1)
bigdecimal
rexml
date (3.5.1)
debug (1.11.1)
irb (~> 1.10)
reline (>= 0.3.8)
drb (2.2.3)
erb (6.0.2)
hashdiff (1.2.1)
i18n (1.14.8)
concurrent-ruby (~> 1.0)
io-console (0.6.0)
irb (1.6.2)
reline (>= 0.3.0)
json (2.6.3)
minitest (5.17.0)
parallel (1.22.1)
parser (3.2.1.0)
io-console (0.8.2)
irb (1.17.0)
pp (>= 0.6.0)
prism (>= 1.3.0)
rdoc (>= 4.0.0)
reline (>= 0.4.2)
json (2.18.1)
json-schema (6.1.0)
addressable (~> 2.8)
bigdecimal (>= 3.1, < 5)
language_server-protocol (3.17.0.5)
lint_roller (1.1.0)
logger (1.7.0)
mcp (0.8.0)
json-schema (>= 4.1)
minitest (6.0.2)
drb (~> 2.0)
prism (~> 1.5)
parallel (1.27.0)
parser (3.3.10.2)
ast (~> 2.4.1)
public_suffix (5.0.0)
rack (3.0.8)
racc
pp (0.6.3)
prettyprint
prettyprint (0.2.0)
prism (1.9.0)
psych (5.3.1)
date
stringio
public_suffix (7.0.5)
racc (1.8.1)
rack (3.2.5)
rainbow (3.1.1)
rake (13.0.1)
regexp_parser (2.7.0)
reline (0.3.2)
rake (13.3.1)
rdoc (7.2.0)
erb
psych (>= 4.0.0)
tsort
regexp_parser (2.11.3)
reline (0.6.3)
io-console (~> 0.5)
rexml (3.2.5)
rubocop (1.45.1)
rexml (3.4.4)
rubocop (1.85.1)
json (~> 2.3)
language_server-protocol (~> 3.17.0.2)
lint_roller (~> 1.1.0)
mcp (~> 0.6)
parallel (~> 1.10)
parser (>= 3.2.0.0)
parser (>= 3.3.0.2)
rainbow (>= 2.2.2, < 4.0)
regexp_parser (>= 1.8, < 3.0)
rexml (>= 3.2.5, < 4.0)
rubocop-ast (>= 1.24.1, < 2.0)
regexp_parser (>= 2.9.3, < 3.0)
rubocop-ast (>= 1.49.0, < 2.0)
ruby-progressbar (~> 1.7)
unicode-display_width (>= 2.4.0, < 3.0)
rubocop-ast (1.26.0)
parser (>= 3.2.1.0)
rubocop-minitest (0.27.0)
rubocop (>= 0.90, < 2.0)
rubocop-performance (1.16.0)
rubocop (>= 1.7.0, < 2.0)
rubocop-ast (>= 0.4.0)
rubocop-rails (2.19.1)
unicode-display_width (>= 2.4.0, < 4.0)
rubocop-ast (1.49.0)
parser (>= 3.3.7.2)
prism (~> 1.7)
rubocop-minitest (0.39.1)
lint_roller (~> 1.1)
rubocop (>= 1.75.0, < 2.0)
rubocop-ast (>= 1.38.0, < 2.0)
rubocop-performance (1.26.1)
lint_roller (~> 1.1)
rubocop (>= 1.75.0, < 2.0)
rubocop-ast (>= 1.47.1, < 2.0)
rubocop-rails (2.34.3)
activesupport (>= 4.2.0)
lint_roller (~> 1.1)
rack (>= 1.1)
rubocop (>= 1.33.0, < 2.0)
ruby-progressbar (1.11.0)
safe_yaml (1.0.5)
rubocop (>= 1.75.0, < 2.0)
rubocop-ast (>= 1.44.0, < 2.0)
ruby-progressbar (1.13.0)
securerandom (0.4.1)
stringio (3.2.0)
tsort (0.2.0)
tzinfo (2.0.6)
concurrent-ruby (~> 1.0)
unicode-display_width (2.4.2)
webmock (3.8.0)
addressable (>= 2.3.6)
unicode-display_width (3.2.0)
unicode-emoji (~> 4.1)
unicode-emoji (4.2.0)
uri (1.1.1)
webmock (3.26.1)
addressable (>= 2.8.0)
crack (>= 0.3.2)
hashdiff (>= 0.4.0, < 2.0.0)

Expand Down
35 changes: 35 additions & 0 deletions lib/rspamd-ruby.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,37 @@
require "active_support/core_ext/hash/keys"
require "rspamd/client"
require "rspamd/client_stub"
require "rspamd/errors"
require "rspamd/railtie" if defined?(::Rails::Railtie)

module Rspamd
class << self
def setup(config)
@config = config.deep_symbolize_keys
@clients = {}
end

def client_for(name)
clients[name] ||= enabled? ? build_client(name) : ClientStub.new
end
Comment on lines +14 to +16
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

When enabled? is false, every call to client_for with the same name will create a new ClientStub instance only if caching works — but when setup was never called (@config is nil), enabled? returns nil (falsy) and build_client is skipped, yet a new ClientStub is created and cached. However, when enabled? is explicitly false, a new ClientStub is still created per unique name. Consider caching a single shared ClientStub instance (e.g., STUB = ClientStub.new) to avoid unnecessary allocations if client_for is called with many different names while disabled.

Copilot uses AI. Check for mistakes.

def reset!
@config = nil
@clients = {}
end

private
def clients
@clients ||= {}
Comment on lines +6 to +25
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The PR description claims "Thread-safe client management," but client_for uses Hash#[]= with ||= which is not thread-safe. In a multi-threaded environment (e.g., Puma), concurrent calls to client_for could result in race conditions where multiple Client instances are created for the same name, or worse, internal hash corruption.

If thread safety is a goal, consider using a Mutex to synchronize access to @clients, or use Concurrent::Map from the concurrent-ruby gem (which is already a transitive dependency via activesupport).

Suggested change
module Rspamd
class << self
def setup(config)
@config = config.deep_symbolize_keys
@clients = {}
end
def client_for(name)
clients[name] ||= enabled? ? build_client(name) : ClientStub.new
end
def reset!
@config = nil
@clients = {}
end
private
def clients
@clients ||= {}
require "concurrent/map"
module Rspamd
class << self
def setup(config)
@config = config.deep_symbolize_keys
@clients = Concurrent::Map.new
end
def client_for(name)
clients.fetch_or_store(name) { enabled? ? build_client(name) : ClientStub.new }
end
def reset!
@config = nil
@clients = Concurrent::Map.new
end
private
def clients
@clients ||= Concurrent::Map.new

Copilot uses AI. Check for mistakes.
end

def enabled?
@config&.dig(:enabled)
end

def build_client(name)
settings = @config.fetch(name) { raise ArgumentError, "No rspamd configuration for #{name.inspect}" }
Client.new(**settings.slice(:host, :port, :password))
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

build_client only passes host, port, and password to Client.new, but Configuration supports additional options: scheme, open_timeout, read_timeout, and user_agent. If any of these are specified in the YAML config, they'll be silently ignored. Consider forwarding all recognized options, or at minimum documenting that only these three are supported via the YAML config.

Suggested change
Client.new(**settings.slice(:host, :port, :password))
Client.new(**settings.slice(:host, :port, :password, :scheme, :open_timeout, :read_timeout, :user_agent))

Copilot uses AI. Check for mistakes.
end
end
end
39 changes: 39 additions & 0 deletions lib/rspamd/client_stub.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
require "rspamd/check/result"

module Rspamd
class ClientStub
HAM_RESULT = Check::Result.new(
"score" => 0.0,
"required_score" => 15.0,
"action" => "no action",
"is_skipped" => false,
"symbols" => {},
"urls" => [],
"emails" => []
).freeze
Comment on lines +5 to +13
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

HAM_RESULT is a single shared Check::Result instance returned by check. While the result object is frozen, the nested data hash/arrays (and e.g. symbols) remain mutable, so any consumer mutation will leak across calls/threads. Consider returning a fresh Check::Result per call, or deep-freezing/duplicating the nested data before exposing it.

Copilot uses AI. Check for mistakes.
Comment on lines +5 to +13
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

HAM_RESULT is frozen and returned directly from check. If any caller mutates the returned result (e.g., adding symbols or modifying attributes), it will raise a FrozenError. While freezing is generally good for a shared constant, the real Client#check returns a fresh Result each time. This behavioral difference could cause subtle breakage when switching between Client and ClientStub. Consider returning HAM_RESULT.dup from check to match the real client's semantics.

Copilot uses AI. Check for mistakes.

def ping
true
end

def check(message, headers: {})
HAM_RESULT
end

def spam!(message)
true
end

def ham!(message)
true
end

def add_fuzzy(message, flag: 1, weight: 1)
true
end

def delete_fuzzy(message, flag: 1)
true
end
end
end
11 changes: 11 additions & 0 deletions lib/rspamd/railtie.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
module Rspamd
class Railtie < ::Rails::Railtie
initializer "rspamd.setup" do
config_path = ::Rails.root.join("config/rspamd.yml")

if config_path.exist?
Rspamd.setup(::Rails.application.config_for(:rspamd))
Comment on lines +3 to +7
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The initializer runs during Rails initialization but does not use an after_initialize hook. Depending on the Rails boot order, config_for should work here, but using config.after_initialize is the more conventional pattern when setting up application-level services to ensure the full application config is available. Additionally, if config/rspamd.yml exists but lacks an entry for the current environment, config_for will raise an error — consider rescuing or documenting this behavior.

Suggested change
initializer "rspamd.setup" do
config_path = ::Rails.root.join("config/rspamd.yml")
if config_path.exist?
Rspamd.setup(::Rails.application.config_for(:rspamd))
initializer "rspamd.setup" do |app|
config_path = ::Rails.root.join("config/rspamd.yml")
if config_path.exist?
app.config.after_initialize do
begin
Rspamd.setup(app.config_for(:rspamd))
rescue KeyError => e
if defined?(::Rails) && ::Rails.respond_to?(:logger) && ::Rails.logger
::Rails.logger.warn("Rspamd configuration for environment #{::Rails.env} is missing in config/rspamd.yml: #{e.message}")
end
end
end

Copilot uses AI. Check for mistakes.
end
end
end
end
4 changes: 3 additions & 1 deletion rspamd-ruby.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ Gem::Specification.new do |s|
s.summary = "Client for Rspamd's HTTP API"
s.homepage = "https://github.com/basecamp/rspamd-ruby"

s.required_ruby_version = ">= 2.7.8"
s.required_ruby_version = ">= 3.2"

s.add_dependency "activesupport", ">= 6.0"
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

Adding activesupport as an unconditional runtime dependency is a heavy requirement for what is currently used only for deep_symbolize_keys in Rspamd::Rails. This forces all consumers of this gem (including non-Rails projects) to pull in activesupport and its transitive dependencies.

Consider either:

  1. Making activesupport an optional dependency and only requiring it in the Rails-specific code path, or
  2. Implementing the deep_symbolize_keys logic inline (it's a small recursive method) to avoid the dependency entirely for the core gem.
Suggested change
s.add_dependency "activesupport", ">= 6.0"
s.add_development_dependency "activesupport", ">= 6.0"

Copilot uses AI. Check for mistakes.

s.add_development_dependency "rake", "~> 13.0"
s.add_development_dependency "minitest", "> 5.11"
Expand Down
46 changes: 46 additions & 0 deletions test/client_stub_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
require "test_helper"

class Rspamd::ClientStubTest < Minitest::Test
def setup
@stub = Rspamd::ClientStub.new
end

def test_ping_returns_true
assert @stub.ping
end

def test_check_returns_ham_result
result = @stub.check("message body")
assert result.ham?
assert_not result.spam?
assert_equal 0.0, result.score
assert_equal 15.0, result.required_score
assert_equal "no action", result.action
end

def test_check_accepts_headers
result = @stub.check("message body", headers: { "Settings-Id" => "outbound" })
assert result.ham?
end

def test_spam_returns_true
assert @stub.spam!("message body")
end

def test_ham_returns_true
assert @stub.ham!("message body")
end

def test_add_fuzzy_returns_true
assert @stub.add_fuzzy("message body")
end

def test_delete_fuzzy_returns_true
assert @stub.delete_fuzzy("message body")
end

private
def assert_not(value)
assert !value
end
end
Loading
Loading