Skip to content

fix(errors): keep a fail-envelope sentence out of error.code (#145) - #148

Merged
karlwaldman merged 2 commits into
mainfrom
fix/145-fail-envelope-error-code
Sep 13, 2026
Merged

karlwaldman merged 2 commits into
mainfrom
fix/145-fail-envelope-error-code

Conversation

@karlwaldman

@karlwaldman karlwaldman commented Sep 13, 2026

Copy link
Copy Markdown
Member

Summary

error_from_response copied any string data.error into error.code and error.machine_code. In the API's render_fail envelope, {"status": "fail", "data": {"error": ...}}, that field holds one of two things:

  • A machine code, with the sentence in data.message. Upper-snake examples: VALIDATION_ERROR, INTERVAL_FLOOR, WATCH_LIMIT. Lower-snake examples: invalid_code (api_validations.rb), and no_price_data and invalid_request (prices_controller.rb).
  • The sentence itself. Every V1::FuelSurchargeController 400/404 does this, and so do the rig-count, well-permit and date-validation render_fail callers.

For the second shape, error.code became a unique free-text string per request.

Fix. data.error becomes the code only when _is_machine_code accepts it: a snake-case token, all upper or all lower, ASCII alphanumerics joined by _. A sentence stays in error.message and error.code is None.

Unchanged:

  • A canonical error object, top-level or under data, still takes precedence.
  • A code with no sentence is still both code and message.
  • API-key redaction is untouched. Two new tests prove a key inside a fail sentence, and inside a fail message beside a machine code, is redacted from message, str(), raw_body and raw_text.

Parity with oilpriceapi-node #114

Same precedence and the same code/message split, with two deliberate differences:

  1. Lower-snake codes stay codes here. Node's MACHINE_CODE is upper-snake only, so it drops invalid_code / no_price_data / invalid_request from error.code. Those are real machine codes: live GET /v1/prices/latest?by_code=NOT_A_REAL_CODE_XYZ returns {"error": "invalid_code", "message": "Code 'NOT_A_REAL_CODE_XYZ' not found. ..."}, and api_validations.rb notes clients parse error. Before this PR, Python returned code == "invalid_code". Matching Node would regress that, so the gap is filed against Node instead as [P2][errors] lower-snake fail codes (invalid_code, no_price_data, invalid_request) are dropped from error.code oilpriceapi-node#117.
  2. The status key is not required. Node only reads data when status is fail/error. Python already read data without it, and a sentence-shaped data.error is now refused either way, so behaviour did not need narrowing.

Node's second #114 bug: absent field overwriting a subclass default code

Checked, not present in Python. No OilPriceAPIError subclass defines a default code (Node had NOT_FOUND_ERROR / HTTP_ERROR). code is an explicit keyword on the base __init__, and error_from_response passes common keys that no subclass defaults. The parametrized test over 400/401/402/403/404/422/429/503 pins code is None for each class on a sentence body.

tests/unit/test_error_contract.py was checked for pinned fixtures relying on the old fallback. Its fixtures use code / error.code keys, not data.error, and all pass.

Live fail bodies used as fixtures (api.oilpriceapi.com, 2026-09-13)

Request HTTP data.error Before After
GET /v1/fuel-surcharge/nope/latest 404 "Unknown carrier 'nope'. Covered carriers: odfl, ..." code = the sentence code None, message = sentence
GET /v1/fuel-surcharge/parcel/ups/history 400 "Parcel fuel-surcharge history requires a service_level parameter." code = the sentence code None
POST /v1/subscriptions with codes: [] 422 VALIDATION_ERROR, message "Codes can't be blank" VALIDATION_ERROR unchanged
POST /v1/subscriptions with interval_seconds: -5 402 INTERVAL_FLOOR, message "Your plan's minimum snapshot interval is 60s ..." INTERVAL_FLOOR unchanged
GET /v1/prices/latest?by_code=NOT_A_REAL_CODE_XYZ 400 invalid_code, message "Code 'NOT_A_REAL_CODE_XYZ' not found. ..." invalid_code unchanged
GET /v1/subscriptions/<unknown uuid> 404 canonical {"error": {"code": "NOT_FOUND", ...}} NOT_FOUND unchanged

A bad interval returns a 402 INTERVAL_FLOOR upgrade trigger, not a 422. The live 422 came from empty codes. No subscription was created by any probe; every create was refused.

Red (tests written first, run on unchanged origin/main a5304b3 source)

$ pytest tests/unit/test_fail_envelope_error_code.py -q --no-cov -rf
FAILED ...::test_sentence_in_fail_envelope_is_the_message_not_the_code[sync]
FAILED ...::test_sentence_in_fail_envelope_is_the_message_not_the_code[async]
FAILED ...::test_no_status_class_turns_a_sentence_into_a_code[400-BadRequestError]
FAILED ...::test_no_status_class_turns_a_sentence_into_a_code[401-AuthenticationError]
FAILED ...::test_no_status_class_turns_a_sentence_into_a_code[402-PaymentRequiredError]
FAILED ...::test_no_status_class_turns_a_sentence_into_a_code[403-PermissionDeniedError]
FAILED ...::test_no_status_class_turns_a_sentence_into_a_code[404-DataNotFoundError]
FAILED ...::test_no_status_class_turns_a_sentence_into_a_code[422-ValidationError]
FAILED ...::test_no_status_class_turns_a_sentence_into_a_code[429-RateLimitError]
FAILED ...::test_no_status_class_turns_a_sentence_into_a_code[503-ServerError]
FAILED ...::test_sentence_without_the_status_key_is_not_a_code_either
FAILED ...::test_key_in_a_fail_sentence_is_redacted_and_not_used_as_code[sync]
FAILED ...::test_key_in_a_fail_sentence_is_redacted_and_not_used_as_code[async]
======================== 13 failed, 12 passed in 0.54s =========================

Every failure is code holding the sentence:

   8 E       AssertionError: assert 'period is not supported by the latest endpoint; use /v1/rig-counts/historical' is None
   2 E       assert "Unknown carrier 'nope'. Covered carriers: odfl, saia, estes, xpo, abf, tforce, averitt, southeastern-freight." is None
   2 E       AssertionError: assert 'API key [REDACTED] cannot use this route' is None
   1 E       AssertionError: assert 'Parcel fuel-surcharge history requires a service_level parameter.' is None

The 12 that pass on main are the regression guards: upper-snake and lower-snake codes, code-only body, canonical precedence, and redaction beside a machine code.

Green

$ pytest tests/unit/test_fail_envelope_error_code.py -q --no-cov
============================== 25 passed in 0.31s ==============================
$ pytest tests/unit/test_fail_envelope_error_code.py tests/unit/test_error_contract.py tests/unit/test_exceptions.py tests/unit/test_prices_resource.py tests/unit/test_fuel_surcharge_resource.py tests/unit/test_subscriptions_lifecycle.py -q --no-cov
============================= 564 passed in 2.52s ==============================

Full suite. Baseline re-measured on origin/main a5304b3 with all extras installed:

passed failed skipped
origin/main 1884 3 54
this branch 1909 3 54

The 3 failures are the live tests/integration/test_demo_contract.py 429s on both runs.

  • ruff check oilpriceapi/: All checks passed!
  • mypy oilpriceapi/ --ignore-missing-imports, in a CI-equivalent .[dev] environment: Success, no issues found in 55 source files.
  • python scripts/validate_storefront_claims.py: validated 76 public surfaces.

No local refusal was added, no pydantic validator, no raw ValueError.

Merge order

This PR and #147 (#142) both add entries under ## [Unreleased] / ### Fixed in CHANGELOG.md. Merge #147 first. This branch then takes a merge of origin/main (no rebase, no force push) to resolve the CHANGELOG hunk.

Closes #145

🤖 Generated with Claude Code

https://claude.ai/code/session_015ao5paex73xXvuM424Libo

error_from_response copied a string data.error into error.code whatever it
held. In the render_fail envelope it is either a machine code (sentence in
data.message) or the sentence itself, as every fuel-surcharge 400/404 sends,
so code-based branching saw a unique free-text string per request.

data.error now becomes the code only when it is a snake-case token:
upper-snake (VALIDATION_ERROR, INTERVAL_FLOOR, WATCH_LIMIT) or lower-snake
(invalid_code, no_price_data, invalid_request, which api_validations.rb and
prices_controller.rb send). A sentence stays the message and code is None.
A canonical nested error object keeps precedence; redaction is unchanged.

Closes #145

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo
@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 3822ccee-0eff-49b7-8156-063aea6d9509


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Resolve the CHANGELOG ### Fixed conflict with #147 by keeping both entries.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo
@karlwaldman
karlwaldman merged commit 0266d08 into main Sep 13, 2026
7 checks passed
@karlwaldman
karlwaldman deleted the fix/145-fail-envelope-error-code branch September 13, 2026 21:07
karlwaldman added a commit that referenced this pull request Sep 13, 2026
Resolve the CHANGELOG ### Fixed conflict with #148 by keeping both entries.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[P2][errors] error.code is set to the whole human message for {status:"fail", data:{error:"<sentence>"}} responses

1 participant