Attach structured cgminer_code and code symbol to ApiError#9
Merged
Conversation
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.
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).
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".
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%.
jramos
changed the title
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CgminerApiClient::ApiErrornow carries two structured fields alongside the existing message so callers can dispatch on a symbol instead of parsing English error strings.#cgminer_code— the integer Code from cgminer's STATUS hash (e.g.,45for access denied), ornilif the error wasn't sourced from a wire response (the gem's ownaccess_denied?local guard fires before any wire interaction, socgminer_codestays nil there).#code— symbolic Ruby tag derived fromcgminer_codeviaApiError::CGMINER_CODES, falling back to:unknownfor unmapped codes. Callers can also passcode:explicitly when raising from a path with no cgminer integer.CGMINER_CODESis intentionally conservative — only14 → :invalid_commandand45 → :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.Backward-compatible by design:
raise ApiError, "msg"still works (setscgminer_code: nil, code: :unknown).e.messageis unchanged for existing emission sites ("#{c}: #{msg}"for wire errors,"access denied"for the local guard).cgminer_monitorandcgminer_manager(rescue CgminerApiClient::ApiError) keep working without changes.Three commits
ApiError+ dedicated spec (21 examples covering legacy form, kwargs, override semantics,to_symcoercion,raise/rescuecompat). Independently bisectable — emission sites still raise with the legacy form.Miner#check_statuspassescgminer_code: cfrom the wire response;access_denied?passescode: :access_deniedexplicitly. Existing tests extended with assertions on the new fields, plus a new end-to-end integration test against FakeCgminer'sinvalid_commandfixture (Code 14) that pins dispatch against real cgminer behavior.[Unreleased] ### Addedentry + AGENTS.md "Error handling" paragraph naming the fields and the map.Test plan
bundle exec rakegreen: 251 examples (was 247, +4 new), 0 failures, rubocop cleanraise ApiError, "msg"still works and producescode: :unknowncgminer_code: 45, code: :access_denied(asserted viaMiner#check_statusunit + FakeCgminer integration)cgminer_code: 23, code: :unknown— fallback path coveredaccess_denied?guard producescgminer_code: nil, code: :access_denied— symbol matches wire-side dispatch despite no wire interactionrescue CgminerApiClient::Errorandrescue CgminerApiClient::ApiErrorconsumers in monitor/manager keep working (rescue class hierarchy unchanged)Follow-ups (not in this PR)
cgminer_manager'sCgminerCommanderandPoolManagercould nowcase e.codeto surface "access denied" vs "invalid command" differently in the audit log — out of scope for this PR; the foundation is now in place.