diff --git a/AGENTS.md b/AGENTS.md index fd4664c..1800d56 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -37,7 +37,7 @@ A pure-Ruby client for the [cgminer](https://github.com/ckolivas/cgminer) JSON-o ├── bin/cgminer_api_client # CLI (packaged in gem) ├── lib/cgminer_api_client.rb # Entry point + module-level config (packaged) ├── lib/cgminer_api_client/ -│ ├── errors.rb # Error < StandardError, ConnectionError, TimeoutError, ApiError +│ ├── errors.rb # Error < StandardError, ConnectionError, TimeoutError, ApiError (+ ApiError#cgminer_code, #code) │ ├── miner.rb # Single-host client │ ├── miner/commands.rb # ReadOnly + Privileged.{Asc,Pga,Pool,System} │ ├── miner_pool.rb # Parallel fan-out across a pool @@ -114,6 +114,7 @@ Lib ─┼──> MinerPool ──(parallel threads)──> Miner ──> socket ### Error handling - New errors should subclass one of the existing four (`Error`, `ConnectionError`, `TimeoutError`, `ApiError`) — don't add a sibling unless you have a real reason. The hierarchy is deliberate: `ConnectionError` = "couldn't reach the miner", `ApiError` = "miner answered and rejected". Keep those semantics intact. +- **`ApiError` carries structured fields, not just a message.** `#cgminer_code` is the integer Code from cgminer's STATUS hash (or nil if the error wasn't sourced from a wire response — e.g., the `access_denied?` local guard). `#code` is a symbolic Ruby tag derived from that integer via `ApiError::CGMINER_CODES`, falling back to `:unknown`. Callers dispatch on `e.code`, never on `e.message =~ /access denied/i`. The map is intentionally conservative — only codes the test suite or production has actually observed against real cgminer 4.11.1 fixtures (`14 → :invalid_command`, `45 → :access_denied`). When you find a code worth dispatching on, add a row. - Rescue narrowly. `rescue SocketError, SystemCallError, CgminerApiClient::TimeoutError` in `Miner#available?` is the pattern — bugs like `ArgumentError` should propagate, not be silently swallowed. - `MinerPool#query` worker threads use `rescue StandardError => e` deliberately — they capture everything into a `MinerResult.failure`. One bad miner must not take down the whole pool query. diff --git a/CHANGELOG.md b/CHANGELOG.md index 1506d35..2d58bb4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- **Structured error codes on `CgminerApiClient::ApiError`.** Two new + reader methods alongside the existing `#message`: + `#cgminer_code` (the integer Code from cgminer's STATUS hash — + e.g., `45` for access denied — or `nil` for errors raised before + any wire interaction) and `#code` (a symbolic tag derived from + the integer via `ApiError::CGMINER_CODES`, falling back to + `:unknown` for codes not in the map). Callers can now + `case e.code; when :access_denied; ...` instead of parsing + English error strings. The map is intentionally conservative — + `14 → :invalid_command`, `45 → :access_denied`, the two codes + the test suite has observed against real cgminer 4.11.1 fixtures. + Backward-compatible: `raise ApiError, "msg"` still works and + `e.message` is unchanged. `Miner#check_status` now passes + `cgminer_code:` through verbatim from the wire response; + `access_denied?` (the local pre-wire guard) passes + `code: :access_denied` explicitly so the symbol matches a + real wire-side Code 45 regardless of which path raised. - **`docs/logging.md`** — short stub stating that `cgminer_api_client` is intentionally silent: no `Logger` module, no structured log events. The library raises on failure and returns result objects diff --git a/lib/cgminer_api_client/errors.rb b/lib/cgminer_api_client/errors.rb index e947c1f..8094874 100644 --- a/lib/cgminer_api_client/errors.rb +++ b/lib/cgminer_api_client/errors.rb @@ -18,6 +18,53 @@ class TimeoutError < ConnectionError; end # Raised when the miner returned a response whose STATUS field # indicates an error (cgminer status code 'E' or 'F'). The message - # contains the cgminer code and message verbatim. - class ApiError < Error; end + # contains the cgminer code and message verbatim. Two structured + # fields are also attached so callers can dispatch without parsing + # English error strings: + # + # #cgminer_code -- the integer Code from cgminer's STATUS hash + # (e.g., 45 for access denied), or nil if the + # error wasn't sourced from a cgminer response + # (e.g., the gem's own access_denied? guard + # raises before any wire interaction). + # #code -- a symbolic Ruby tag derived from cgminer_code + # via CGMINER_CODES, or :unknown if the integer + # isn't in the map. Callers can also pass an + # explicit code: kwarg when raising from a path + # that doesn't carry a cgminer integer. + # + # cgminer's MSG enum names are stable but the integers occasionally + # shift between firmware versions, so the integer is preserved + # verbatim and the symbol is best-effort. Add a row to CGMINER_CODES + # when you find a code worth dispatching on; the map is intentionally + # conservative (only codes the test suite or production has actually + # observed against real cgminer traffic). + # + # Backward compatibility: ApiError still accepts a single positional + # message argument, so `raise ApiError, "msg"` keeps working. Existing + # rescues that read .message see the same string they always did. + class ApiError < Error + CGMINER_CODES = { + 14 => :invalid_command, + 45 => :access_denied + }.freeze + + attr_reader :cgminer_code, :code + + def initialize(message = nil, cgminer_code: nil, code: nil) + # Fail loud at the library boundary on bad input. Without this guard, + # cgminer_code: "45" or 45.0 silently produces code: :unknown + # because CGMINER_CODES uses integer keys — every dispatch site + # would break with no signal. Wire-side callers that want best-effort + # coercion should pass Integer(c, exception: false) themselves. + unless cgminer_code.nil? || cgminer_code.is_a?(Integer) + raise ArgumentError, + "cgminer_code must be Integer or nil, got #{cgminer_code.class}: #{cgminer_code.inspect}" + end + + super(message) + @cgminer_code = cgminer_code + @code = (code || CGMINER_CODES[cgminer_code] || :unknown).to_sym + end + end end diff --git a/lib/cgminer_api_client/miner.rb b/lib/cgminer_api_client/miner.rb index 167b425..c04605d 100644 --- a/lib/cgminer_api_client/miner.rb +++ b/lib/cgminer_api_client/miner.rb @@ -151,7 +151,12 @@ def check_status(data) # cgminer STATUS codes: S=Success (silent), I=Info, W=Warning, # E=Error, F=Fatal. Errors and Fatals raise ApiError so callers # can distinguish them from ConnectionError (transport-level - # failures). + # failures). The wire boundary stays best-effort: if a future + # firmware ever emits Code as a JSON string instead of an integer, + # Integer(...) coerces it; if it's non-numeric, falls through to + # nil and ApiError#code becomes :unknown rather than raising + # ArgumentError mid-poll. The library boundary stays strict — + # see ApiError#initialize. case sc when 'S' # no-op: success needs no notification @@ -160,7 +165,7 @@ def check_status(data) when 'W' puts "Warning from API [#{c}]: #{msg}" else - raise ApiError, "#{c}: #{msg}" + raise ApiError.new("#{c}: #{msg}", cgminer_code: Integer(c, exception: false)) end end diff --git a/lib/cgminer_api_client/miner/commands.rb b/lib/cgminer_api_client/miner/commands.rb index d58dbce..b8d1110 100644 --- a/lib/cgminer_api_client/miner/commands.rb +++ b/lib/cgminer_api_client/miner/commands.rb @@ -186,7 +186,15 @@ def zero(which = 'All', full_summary = false) private def access_denied? - raise CgminerApiClient::ApiError, 'access denied' unless privileged + # privileged calls query(:privileged), so the wire IS hit; if + # the miner answers with STATUS=E Code 45, check_status raises + # and privileged rescues + returns false, dropping the cgminer + # integer in the rescue. Re-attach the symbolic code explicitly + # so callers can't tell which call path raised — dispatch on + # e.code works identically for "real wire denied" and + # "guard-locally denied". e.cgminer_code stays nil here because + # it was discarded by privileged's rescue. + raise CgminerApiClient::ApiError.new('access denied', code: :access_denied) unless privileged false end diff --git a/spec/cgminer_api_client/api_error_spec.rb b/spec/cgminer_api_client/api_error_spec.rb new file mode 100644 index 0000000..df04174 --- /dev/null +++ b/spec/cgminer_api_client/api_error_spec.rb @@ -0,0 +1,185 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe CgminerApiClient::ApiError do + describe 'CGMINER_CODES' do + it 'is frozen' do + expect(described_class::CGMINER_CODES).to be_frozen + end + + it 'raises FrozenError on element mutation' do + # be_frozen alone proves the flag is set, not that mutation + # actually fails — pin the immutability contract callers rely + # on (e.g., a future PR can't accidentally hand out a mutable + # reference and have it silently get patched by a consumer). + expect { described_class::CGMINER_CODES[99] = :foo } + .to raise_error(FrozenError) + end + + it 'maps the access-denied integer (45) to :access_denied' do + expect(described_class::CGMINER_CODES[45]).to eq(:access_denied) + end + + it 'maps the invalid-command integer (14) to :invalid_command' do + expect(described_class::CGMINER_CODES[14]).to eq(:invalid_command) + end + end + + describe '#initialize' do + context 'with only a positional message (legacy form)' do + let(:e) { described_class.new('boom') } + + it 'preserves the message' do + expect(e.message).to eq('boom') + end + + it 'leaves cgminer_code nil' do + expect(e.cgminer_code).to be_nil + end + + it 'sets code to :unknown' do + expect(e.code).to eq(:unknown) + end + end + + context 'with cgminer_code that maps to a known symbol' do + let(:e) { described_class.new('45: Access denied', cgminer_code: 45) } + + it 'preserves the cgminer integer' do + expect(e.cgminer_code).to eq(45) + end + + it 'derives :access_denied from the integer' do + expect(e.code).to eq(:access_denied) + end + + it 'preserves the message' do + expect(e.message).to eq('45: Access denied') + end + end + + context 'with cgminer_code that is not in the map' do + let(:e) { described_class.new('999: Strange', cgminer_code: 999) } + + it 'preserves the cgminer integer verbatim' do + expect(e.cgminer_code).to eq(999) + end + + it 'falls back to :unknown' do + expect(e.code).to eq(:unknown) + end + end + + context 'with cgminer_code nil' do + let(:e) { described_class.new('boom', cgminer_code: nil) } + + it 'sets code to :unknown' do + expect(e.code).to eq(:unknown) + end + end + + context 'with explicit code: but no cgminer_code' do + let(:e) { described_class.new('access denied', code: :access_denied) } + + it 'uses the explicit symbol' do + expect(e.code).to eq(:access_denied) + end + + it 'leaves cgminer_code nil' do + expect(e.cgminer_code).to be_nil + end + end + + context 'with both cgminer_code and explicit code' do + let(:e) { described_class.new('foo', cgminer_code: 45, code: :custom) } + + it 'lets the explicit code override the map lookup' do + expect(e.code).to eq(:custom) + end + + it 'still preserves the cgminer integer' do + expect(e.cgminer_code).to eq(45) + end + end + + context 'with explicit code as a String' do + # The constructor calls to_sym so callers can pass either a + # Symbol or a String for code: without the dispatch-site case + # statement caring which form was supplied. + let(:e) { described_class.new('boom', code: 'access_denied') } + + it 'coerces the symbol' do + expect(e.code).to eq(:access_denied) + end + end + + context 'with explicit code: nil and a mapped cgminer_code' do + # Pins the || semantics: explicit nil falls through to the map + # lookup. The "both passed" context above only covers truthy + # explicit codes, so this guards the false-trail of the precedence + # chain (code || CGMINER_CODES[cgminer_code] || :unknown). + let(:e) { described_class.new('msg', cgminer_code: 45, code: nil) } + + it 'falls through nil to the map lookup, yielding :access_denied' do + expect(e.code).to eq(:access_denied) + end + end + + context 'with non-Integer cgminer_code (library-boundary input validation)' do + # The constructor is the library's strict boundary. Without this + # guard, cgminer_code: "45" silently produces code: :unknown + # (CGMINER_CODES uses Integer keys), and every dispatch site + # breaks with no signal. Wire-side callers that want best-effort + # coercion call Integer(c, exception: false) before constructing. + it 'raises ArgumentError on a String' do + expect { described_class.new('msg', cgminer_code: '45') } + .to raise_error(ArgumentError, /cgminer_code must be Integer or nil/) + end + + it 'raises ArgumentError on a Float' do + expect { described_class.new('msg', cgminer_code: 45.0) } + .to raise_error(ArgumentError, /cgminer_code must be Integer or nil/) + end + + it 'still accepts an Integer (regression guard for the happy path)' do + expect { described_class.new('msg', cgminer_code: 45) } + .not_to raise_error + end + + it 'still accepts nil (regression guard for the local-guard path)' do + expect { described_class.new('msg', cgminer_code: nil) } + .not_to raise_error + end + end + end + + describe 'compatibility with raise/rescue' do + it 'works with raise(class, message) — the legacy two-arg form' do + expect { raise described_class, 'boom' } + .to raise_error(described_class) { |e| + expect(e.message).to eq('boom') + expect(e.cgminer_code).to be_nil + expect(e.code).to eq(:unknown) + } + end + + it 'works with raise(instance) when constructed with kwargs' do + expect { raise described_class.new('boom', cgminer_code: 45) } + .to raise_error(described_class) { |e| + expect(e.cgminer_code).to eq(45) + expect(e.code).to eq(:access_denied) + } + end + + it 'is rescued by CgminerApiClient::Error (base class)' do + expect { raise described_class, 'boom' } + .to raise_error(CgminerApiClient::Error) + end + + it 'is rescued by StandardError' do + expect { raise described_class, 'boom' } + .to raise_error(StandardError) + end + end +end diff --git a/spec/cgminer_api_client/miner/commands_spec.rb b/spec/cgminer_api_client/miner/commands_spec.rb index 49eeb7c..9a79ca0 100644 --- a/spec/cgminer_api_client/miner/commands_spec.rb +++ b/spec/cgminer_api_client/miner/commands_spec.rb @@ -193,6 +193,20 @@ instance.send(:access_denied?) end.to raise_error(CgminerApiClient::ApiError, 'access denied') end + + it 'attaches code: :access_denied so callers can dispatch without parsing the message' do + # privileged is stubbed to return false here, so access_denied? + # re-raises with the symbolic code only — cgminer_code stays + # nil because there's no wire integer to inherit (the real + # wire path inside privileged would have raised with code 45, + # but privileged's rescue dropped it). Dispatch on e.code + # works identically for both paths. + expect { instance.send(:access_denied?) } + .to raise_error(CgminerApiClient::ApiError) do |e| + expect(e.code).to eq(:access_denied) + expect(e.cgminer_code).to be_nil + end + end end context 'when privileged' do diff --git a/spec/cgminer_api_client/miner_spec.rb b/spec/cgminer_api_client/miner_spec.rb index 7c37380..bd8c69d 100644 --- a/spec/cgminer_api_client/miner_spec.rb +++ b/spec/cgminer_api_client/miner_spec.rb @@ -481,6 +481,17 @@ instance.send(:check_status, mock_response) end.to raise_error(CgminerApiClient::ApiError, '45: Access denied') end + + it 'attaches the cgminer integer code and the :access_denied symbol' do + # Block-form raise_error so a future refactor that silently + # swallows the raise fails this test (rescue-in-it would pass + # green with zero assertions executed). + expect { instance.send(:check_status, mock_response) } + .to raise_error(CgminerApiClient::ApiError) do |e| + expect(e.cgminer_code).to eq(45) + expect(e.code).to eq(:access_denied) + end + end end context 'with fatal status' do @@ -493,6 +504,18 @@ instance.send(:check_status, mock_response) end.to raise_error(CgminerApiClient::ApiError, '23: Bad command') end + + it 'attaches the cgminer integer verbatim and falls back to :unknown for unmapped codes' do + # 23 is intentionally outside CGMINER_CODES (which only maps + # codes observed in real cgminer fixtures: 14, 45); this + # exercises the :unknown fallback path. If a future PR maps + # 23, pick a different unmapped integer here. + expect { instance.send(:check_status, mock_response) } + .to raise_error(CgminerApiClient::ApiError) do |e| + expect(e.cgminer_code).to eq(23) + expect(e.code).to eq(:unknown) + end + end end end diff --git a/spec/integration/miner_integration_spec.rb b/spec/integration/miner_integration_spec.rb index 215e09d..a377ec1 100644 --- a/spec/integration/miner_integration_spec.rb +++ b/spec/integration/miner_integration_spec.rb @@ -93,6 +93,24 @@ def miner_at(port) end end + it 'attaches the cgminer integer code and :invalid_command symbol on STATUS=E for an unknown command' do + # End-to-end coverage: the wire path threads cgminer's Code 14 + # through Miner#check_status into ApiError#cgminer_code, and + # CGMINER_CODES maps it to the :invalid_command symbol. + # Pinned against real FakeCgminer fixture so a future Code-vs-MSG + # firmware drift would trip a test, not silently flip dispatch. + # Block-form raise_error matcher (not rescue-in-it) so a refactor + # that swallowed the raise would fail this example loudly. + CgminerTestSupport::FakeCgminer.with(responses: {}) do |port| + expect { miner_at(port).query(:totally_fake_command) } + .to raise_error(CgminerApiClient::ApiError) do |e| + expect(e.message).to eq('14: Invalid command') + expect(e.cgminer_code).to eq(14) + expect(e.code).to eq(:invalid_command) + end + end + end + it 'returns false from #privileged when the server responds with access denied' do responses = CgminerTestSupport::Fixtures::DEFAULT.merge( 'privileged' => CgminerTestSupport::Fixtures::PRIVILEGED_DENIED @@ -102,6 +120,23 @@ def miner_at(port) end end + it 'attaches cgminer_code: 45 and code: :access_denied when query directly hits a privileged-denied response' do + # End-to-end coverage of the wire-side access-denied path. Unlike + # #privileged (which rescues + returns false), a direct query + # against an admin-only command on an unprivileged miner surfaces + # the ApiError to the caller with the cgminer integer intact. + responses = CgminerTestSupport::Fixtures::DEFAULT.merge( + 'privileged' => CgminerTestSupport::Fixtures::PRIVILEGED_DENIED + ) + CgminerTestSupport::FakeCgminer.with(responses: responses) do |port| + expect { miner_at(port).query(:privileged) } + .to raise_error(CgminerApiClient::ApiError) do |e| + expect(e.cgminer_code).to eq(45) + expect(e.code).to eq(:access_denied) + end + end + end + it 'reports a closed port as unavailable and raises ConnectionError from #query' do # Bind and immediately release to get a definitely-closed port. server = TCPServer.new('127.0.0.1', 0)