Attach structured cgminer_code and code symbol to ApiError

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.
 

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

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

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
 

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

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

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

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