From b0a024c2f97147f9ae56eb10e72fd6b557506c65 Mon Sep 17 00:00:00 2001 From: Justin Ramos Date: Sat, 25 Apr 2026 07:09:13 -0700 Subject: [PATCH 1/4] Attach structured cgminer_code + symbolic code to ApiError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ApiError now carries two structured fields alongside the existing message: #cgminer_code (the integer Code from cgminer's STATUS hash, or nil) and #code (a symbolic Ruby tag derived from cgminer_code via CGMINER_CODES, falling back to :unknown). Callers can dispatch on the symbol instead of parsing the English message; existing rescues that read .message are unaffected because raise(ApiError, "msg") still works. The CGMINER_CODES map is intentionally conservative — only 14 (:invalid_command) and 45 (:access_denied), the two error codes the test suite has actually observed against real cgminer 4.11.1 fixtures. Add a row when a new code earns dispatch-site interest. Wire emission sites are not yet updated; that ships in the next commit so the constructor refactor is independently bisectable. --- lib/cgminer_api_client/errors.rb | 41 ++++++- spec/cgminer_api_client/api_error_spec.rb | 137 ++++++++++++++++++++++ 2 files changed, 176 insertions(+), 2 deletions(-) create mode 100644 spec/cgminer_api_client/api_error_spec.rb diff --git a/lib/cgminer_api_client/errors.rb b/lib/cgminer_api_client/errors.rb index e947c1f..a5782a4 100644 --- a/lib/cgminer_api_client/errors.rb +++ b/lib/cgminer_api_client/errors.rb @@ -18,6 +18,43 @@ 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) + super(message) + @cgminer_code = cgminer_code + @code = (code || CGMINER_CODES[cgminer_code] || :unknown).to_sym + end + end 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..deb0873 --- /dev/null +++ b/spec/cgminer_api_client/api_error_spec.rb @@ -0,0 +1,137 @@ +# 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 '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 to_sym in the constructor coerces strings so callers can + # pass either form without the dispatch-site case statement + # caring which. + let(:e) { described_class.new('boom', code: 'access_denied') } + + it 'coerces the symbol' do + expect(e.code).to eq(:access_denied) + 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 From 85497d9106e766504a3a566637901233811e3b35 Mon Sep 17 00:00:00 2001 From: Justin Ramos Date: Sat, 25 Apr 2026 07:10:11 -0700 Subject: [PATCH 2/4] Wire ApiError emission sites to attach cgminer_code + code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two emission sites now populate the structured fields added in the previous commit: * Miner#check_status (the wire path, when cgminer responds with STATUS=E or STATUS=F) passes cgminer_code: c so the integer flows through verbatim. CGMINER_CODES then derives the symbol — :invalid_command for 14, :access_denied for 45, :unknown for everything else. * Miner::Commands::Privileged#access_denied? (the local guard that fires before any wire interaction when the miner isn't privileged) passes code: :access_denied explicitly, since there's no cgminer integer to attach. Callers see the same symbol regardless of whether the access-denied verdict came from the local guard or from a real wire-side Code 45. Tests updated to cover both paths plus an end-to-end integration test against FakeCgminer's invalid_command fixture (Code 14), which pins the dispatch against real cgminer 4.11.1 behavior. The :unknown fallback is also tested via the existing fatal-status synthetic fixture (Code 23, intentionally unmapped). --- lib/cgminer_api_client/miner.rb | 6 ++++-- lib/cgminer_api_client/miner/commands.rb | 6 +++++- spec/cgminer_api_client/miner/commands_spec.rb | 10 ++++++++++ spec/cgminer_api_client/miner_spec.rb | 17 +++++++++++++++++ spec/integration/miner_integration_spec.rb | 14 ++++++++++++++ 5 files changed, 50 insertions(+), 3 deletions(-) diff --git a/lib/cgminer_api_client/miner.rb b/lib/cgminer_api_client/miner.rb index 167b425..1c8a3fc 100644 --- a/lib/cgminer_api_client/miner.rb +++ b/lib/cgminer_api_client/miner.rb @@ -151,7 +151,9 @@ 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). cgminer_code: passes the integer through verbatim + # so ApiError can derive a symbolic #code via its CGMINER_CODES + # map (callers dispatch on the symbol, not on the English Msg). case sc when 'S' # no-op: success needs no notification @@ -160,7 +162,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: c) end end diff --git a/lib/cgminer_api_client/miner/commands.rb b/lib/cgminer_api_client/miner/commands.rb index d58dbce..dfb4b7e 100644 --- a/lib/cgminer_api_client/miner/commands.rb +++ b/lib/cgminer_api_client/miner/commands.rb @@ -186,7 +186,11 @@ def zero(which = 'All', full_summary = false) private def access_denied? - raise CgminerApiClient::ApiError, 'access denied' unless privileged + # The local guard fires before any wire interaction, so there's + # no cgminer integer to attach — pass code: explicitly so + # callers see the same :access_denied symbol they'd see for a + # real wire-side STATUS Code 45 response. + raise CgminerApiClient::ApiError.new('access denied', code: :access_denied) unless privileged false end diff --git a/spec/cgminer_api_client/miner/commands_spec.rb b/spec/cgminer_api_client/miner/commands_spec.rb index 49eeb7c..1849de4 100644 --- a/spec/cgminer_api_client/miner/commands_spec.rb +++ b/spec/cgminer_api_client/miner/commands_spec.rb @@ -193,6 +193,16 @@ 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 + # The local guard fires before any wire interaction, so + # cgminer_code stays nil — but the symbolic code must match + # what a real wire-side Code 45 would produce. + instance.send(:access_denied?) + rescue CgminerApiClient::ApiError => e + expect(e.code).to eq(:access_denied) + expect(e.cgminer_code).to be_nil + 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..2c4d3d9 100644 --- a/spec/cgminer_api_client/miner_spec.rb +++ b/spec/cgminer_api_client/miner_spec.rb @@ -481,6 +481,13 @@ 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 + instance.send(:check_status, mock_response) + rescue CgminerApiClient::ApiError => e + expect(e.cgminer_code).to eq(45) + expect(e.code).to eq(:access_denied) + end end context 'with fatal status' do @@ -493,6 +500,16 @@ 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 not in CGMINER_CODES — only codes the test suite has + # observed against real cgminer 4.11.1 fixtures (14, 45) are + # mapped, so this exercises the :unknown fallback path. + instance.send(:check_status, mock_response) + rescue CgminerApiClient::ApiError => e + expect(e.cgminer_code).to eq(23) + expect(e.code).to eq(:unknown) + end end end diff --git a/spec/integration/miner_integration_spec.rb b/spec/integration/miner_integration_spec.rb index 215e09d..6c46c2c 100644 --- a/spec/integration/miner_integration_spec.rb +++ b/spec/integration/miner_integration_spec.rb @@ -93,6 +93,20 @@ 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. + CgminerTestSupport::FakeCgminer.with(responses: {}) do |port| + miner_at(port).query(:totally_fake_command) + rescue CgminerApiClient::ApiError => e + expect(e.cgminer_code).to eq(14) + expect(e.code).to eq(:invalid_command) + 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 From 566192f7465ee187fdc1e3b9a61181bae6a41209 Mon Sep 17 00:00:00 2001 From: Justin Ramos Date: Sat, 25 Apr 2026 07:10:48 -0700 Subject: [PATCH 3/4] Document ApiError#cgminer_code and #code in CHANGELOG and AGENTS CHANGELOG entry for [Unreleased] under Added explains the two new reader methods, the conservative CGMINER_CODES map, and the backward-compatibility posture (raise ApiError, "msg" still works, e.message unchanged). AGENTS.md gains a paragraph under "Error handling" naming the fields, the map (14, 45), and the dispatch posture: callers case e.code, never parse e.message. The errors.rb tree-line also picks up "+ ApiError#cgminer_code, #code". --- AGENTS.md | 3 ++- CHANGELOG.md | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) 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 From 3cc739f19f03fcad3a7276f4beb58b6d4a717dd6 Mon Sep 17 00:00:00 2001 From: Justin Ramos Date: Sat, 25 Apr 2026 07:39:20 -0700 Subject: [PATCH 4/4] Address PR #9 review: harden tests + fail-fast on bad cgminer_code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven items from a five-agent /pr-review-toolkit:review-pr pass: A. Replace rescue-in-it pattern (4 sites) with block-form raise_error matcher. The old form silently passes if the call stops raising — exactly the regression these tests exist to defend against. Sites: miner_spec.rb (Code 45 + Code 23), commands_spec.rb (access_denied?), miner_integration_spec.rb (FakeCgminer Code 14). B. Fail loud at the library boundary on non-Integer cgminer_code. ApiError.new("msg", cgminer_code: "45") now raises ArgumentError instead of silently producing code: :unknown. Miner#check_status stays best-effort via Integer(c, exception: false) so a future firmware emitting Code as a JSON string falls through to nil (and :unknown) without raising mid-poll. Library boundary strict; wire boundary lenient. C. Fix factual error in access_denied? comment + spec mirror. The old text claimed "no wire interaction"; in fact privileged hits the wire and discards the integer in its rescue. New comment names the real mechanism so future readers don't hunt for a wire-skipping path that doesn't exist. D. Tighten CGMINER_CODES freeze test to assert FrozenError on element mutation. be_frozen alone proves the flag is set; this pins the immutability contract callers actually rely on. E. Add precedence test for code: nil + cgminer_code: 45 → :access_denied. Pins the false-trail of the || chain that the existing "both passed" test (with truthy code:) didn't cover. F. Fix misleading to_sym-location comment in api_error_spec.rb. Old text said coercion is at the dispatch site; production does it in the constructor. Plus: end-to-end FakeCgminer integration test for the wire-side access-denied path (Code 45 + :access_denied), e.message assertion on the invalid-command integration test, and a forward-rot note on the unmapped-23 test. Tests: 251 → 258 examples (+7). Rubocop clean. Coverage 99.7%. --- lib/cgminer_api_client/errors.rb | 10 ++++ lib/cgminer_api_client/miner.rb | 11 ++-- lib/cgminer_api_client/miner/commands.rb | 12 +++-- spec/cgminer_api_client/api_error_spec.rb | 54 +++++++++++++++++-- .../cgminer_api_client/miner/commands_spec.rb | 18 ++++--- spec/cgminer_api_client/miner_spec.rb | 28 ++++++---- spec/integration/miner_integration_spec.rb | 29 ++++++++-- 7 files changed, 129 insertions(+), 33 deletions(-) diff --git a/lib/cgminer_api_client/errors.rb b/lib/cgminer_api_client/errors.rb index a5782a4..8094874 100644 --- a/lib/cgminer_api_client/errors.rb +++ b/lib/cgminer_api_client/errors.rb @@ -52,6 +52,16 @@ class ApiError < Error 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 diff --git a/lib/cgminer_api_client/miner.rb b/lib/cgminer_api_client/miner.rb index 1c8a3fc..c04605d 100644 --- a/lib/cgminer_api_client/miner.rb +++ b/lib/cgminer_api_client/miner.rb @@ -151,9 +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). cgminer_code: passes the integer through verbatim - # so ApiError can derive a symbolic #code via its CGMINER_CODES - # map (callers dispatch on the symbol, not on the English Msg). + # 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 @@ -162,7 +165,7 @@ def check_status(data) when 'W' puts "Warning from API [#{c}]: #{msg}" else - raise ApiError.new("#{c}: #{msg}", cgminer_code: c) + 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 dfb4b7e..b8d1110 100644 --- a/lib/cgminer_api_client/miner/commands.rb +++ b/lib/cgminer_api_client/miner/commands.rb @@ -186,10 +186,14 @@ def zero(which = 'All', full_summary = false) private def access_denied? - # The local guard fires before any wire interaction, so there's - # no cgminer integer to attach — pass code: explicitly so - # callers see the same :access_denied symbol they'd see for a - # real wire-side STATUS Code 45 response. + # 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 diff --git a/spec/cgminer_api_client/api_error_spec.rb b/spec/cgminer_api_client/api_error_spec.rb index deb0873..df04174 100644 --- a/spec/cgminer_api_client/api_error_spec.rb +++ b/spec/cgminer_api_client/api_error_spec.rb @@ -8,6 +8,15 @@ 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 @@ -95,15 +104,54 @@ end context 'with explicit code as a String' do - # The to_sym in the constructor coerces strings so callers can - # pass either form without the dispatch-site case statement - # caring which. + # 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 diff --git a/spec/cgminer_api_client/miner/commands_spec.rb b/spec/cgminer_api_client/miner/commands_spec.rb index 1849de4..9a79ca0 100644 --- a/spec/cgminer_api_client/miner/commands_spec.rb +++ b/spec/cgminer_api_client/miner/commands_spec.rb @@ -195,13 +195,17 @@ end it 'attaches code: :access_denied so callers can dispatch without parsing the message' do - # The local guard fires before any wire interaction, so - # cgminer_code stays nil — but the symbolic code must match - # what a real wire-side Code 45 would produce. - instance.send(:access_denied?) - rescue CgminerApiClient::ApiError => e - expect(e.code).to eq(:access_denied) - expect(e.cgminer_code).to be_nil + # 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 diff --git a/spec/cgminer_api_client/miner_spec.rb b/spec/cgminer_api_client/miner_spec.rb index 2c4d3d9..bd8c69d 100644 --- a/spec/cgminer_api_client/miner_spec.rb +++ b/spec/cgminer_api_client/miner_spec.rb @@ -483,10 +483,14 @@ end it 'attaches the cgminer integer code and the :access_denied symbol' do - instance.send(:check_status, mock_response) - rescue CgminerApiClient::ApiError => e - expect(e.cgminer_code).to eq(45) - expect(e.code).to eq(:access_denied) + # 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 @@ -502,13 +506,15 @@ end it 'attaches the cgminer integer verbatim and falls back to :unknown for unmapped codes' do - # 23 is not in CGMINER_CODES — only codes the test suite has - # observed against real cgminer 4.11.1 fixtures (14, 45) are - # mapped, so this exercises the :unknown fallback path. - instance.send(:check_status, mock_response) - rescue CgminerApiClient::ApiError => e - expect(e.cgminer_code).to eq(23) - expect(e.code).to eq(:unknown) + # 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 6c46c2c..a377ec1 100644 --- a/spec/integration/miner_integration_spec.rb +++ b/spec/integration/miner_integration_spec.rb @@ -99,11 +99,15 @@ def miner_at(port) # 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| - miner_at(port).query(:totally_fake_command) - rescue CgminerApiClient::ApiError => e - expect(e.cgminer_code).to eq(14) - expect(e.code).to eq(:invalid_command) + 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 @@ -116,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)