From ae23a603f38b7f7e5876794181089fffaa12c47d Mon Sep 17 00:00:00 2001 From: Szymon Kurcab Date: Fri, 18 Sep 2026 20:31:05 +0200 Subject: [PATCH] fix: refuse unsendable questions before paying for them `ask` checked that the questions hash was non-empty and sent whatever was in it. `questions: { "q" => "is this urgent?" }` was serialized, posted, billed as input tokens, and came back as MissingAnswers -- a report that the answer was missing for a question that was never a question. An unknown type behaved the same way, and a choice question with no criteria earned a 422. Two ids that stringify to the same thing were worse than a wasted request. Answers come back keyed by the stringified id, so `:a` and `"a"` in one map are two questions, one answer, and a MissingAnswers naming whichever one lost. Validate the map before building the body: each value a Hash, a type from the three the API defines, instructions present, and the criteria that type requires (1..255 options for choice, 2..10 levels for score, the same bounds the builders enforce). Blank and colliding ids are refused. RequestError names the id. Hand-rolled question hashes are now held to the same rules as built ones, which is the point: the builders were the only validation, and nothing made you use them. Questions.validate! is public so a caller assembling questions elsewhere can run the same check. nil instructions are allowed through: EntryType in the official SDKs covers null for state, instructions, and criteria alike. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 11 ++ README.md | 11 +- lib/ruby_decision_model/client.rb | 29 ++++- lib/ruby_decision_model/questions.rb | 55 ++++++++++ test/client_test.rb | 13 +-- test/question_validation_test.rb | 155 +++++++++++++++++++++++++++ 6 files changed, 263 insertions(+), 11 deletions(-) create mode 100644 test/question_validation_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 846df76..13e5ed8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## Unreleased + +- `ask` validates the questions map before sending it. A value that is not a + Hash, an unknown or missing `type`, missing or empty `instructions`, missing + or wrongly sized `criteria`, a blank id, or two ids that collide once + stringified all raise `RequestError` naming the id. Previously any of these + was serialized, sent, and billed, and came back as a 422 or a + `MissingAnswers`. +- Added `Questions.validate!(question, id:)`, the same check as a public + method. + ## 0.1.0 - 2026-09-18 Provider-neutral release. One `Client`, two providers behind it. diff --git a/README.md b/README.md index 79638d6..fb6dc14 100644 --- a/README.md +++ b/README.md @@ -124,6 +124,15 @@ Answers come back typed: `Answers::Noul` (`noul`, `probabilities`), `response.nouls`, `response.choices`, and `response.scores` return the answers of one type keyed the same way as `response.answers`. +`ask` validates the whole questions map before it sends anything: every value +must be a Hash with a known `type`, `instructions`, and the `criteria` its type +requires, and two ids must not collide once stringified (`:a` and `"a"` would +be one answer for two questions). A question that cannot be answered raises +`RequestError` naming its id, rather than being billed as input tokens and +coming back as a 422 or a missing answer. Hand-rolled question hashes are held +to the same rules as the builders'; `Questions.validate!(question, id:)` runs +them on its own. + Score `probabilities` and `legend` are keyed by the wire's string level keys (`"0"`, `"1"`, ...), not by the criteria labels. Choice `probabilities` sum to approximately 1; treat them as calibrated, not normalized. @@ -172,7 +181,7 @@ return is still accepted and treated as having no headers, which means no | Error | Meaning | | --- | --- | | `ConfigurationError` | No provider could be resolved, missing api_key, unknown provider, or bad `retry:` value | -| `RequestError` | Questions hash was empty | +| `RequestError` | Questions hash was empty, or a question could not be sent as written | | `TransportError` (`TimeoutError`) | Network or timeout failure after retries, carries `#cause_error` | | `ApiError` | Non-2xx response, carries `#status`, `#body`, and `#headers` | | `Unauthorized` | 401 | diff --git a/lib/ruby_decision_model/client.rb b/lib/ruby_decision_model/client.rb index 3457726..081626d 100644 --- a/lib/ruby_decision_model/client.rb +++ b/lib/ruby_decision_model/client.rb @@ -54,7 +54,7 @@ def base_url end def ask(state:, questions:) - raise RequestError, "questions must not be empty" if questions.nil? || questions.empty? + validate_questions!(questions) body = @provider.request_body(model: @model, state: state, questions: questions) status, response_body, response_headers = perform_with_retry( @@ -65,6 +65,33 @@ def ask(state:, questions:) private + # A question the API will refuse is worth catching here: the request is + # billed by input tokens, so sending one that cannot be answered costs + # money to be told what the client already knew. + def validate_questions!(questions) + raise RequestError, "questions must not be empty" if questions.nil? || questions.empty? + + unless questions.is_a?(Hash) + raise RequestError, "questions must be a Hash of id => question, got #{questions.class}" + end + + seen = {} + questions.each_key do |raw_id| + id = raw_id.to_s + raise RequestError, "question id #{raw_id.inspect} is blank" if id.strip.empty? + + # Answers come back keyed by the stringified id, so :a and "a" would + # be one answer for two questions. + if seen.key?(id) + raise RequestError, + "question ids #{seen[id].inspect} and #{raw_id.inspect} are both #{id.inspect} on the wire" + end + + seen[id] = raw_id + Questions.validate!(questions[raw_id], id: id) + end + end + def resolve_provider(provider, api_key:, base_url:) case provider when Providers::Base diff --git a/lib/ruby_decision_model/questions.rb b/lib/ruby_decision_model/questions.rb index 5620901..41320a8 100644 --- a/lib/ruby_decision_model/questions.rb +++ b/lib/ruby_decision_model/questions.rb @@ -34,6 +34,61 @@ def score(instructions, criteria:) { "type" => "score", "instructions" => instructions, "criteria" => criteria } end + TYPES = %w[noul choice score].freeze + + # Checks one question -- built here or hand-rolled -- against the shape + # the API documents, and raises RequestError naming the offending id. + # Client#ask runs this over the whole map before it sends anything, so a + # question that cannot be answered costs nothing. + def validate!(question, id:) + raise RequestError, "question #{id.inspect} must be a Hash, got #{question.class}" unless question.is_a?(Hash) + + type = field(question, "type") + unless TYPES.include?(type.to_s) + raise RequestError, "question #{id.inspect} has type #{type.inspect}; expected one of #{TYPES.join(', ')}" + end + + validate_question_instructions!(question, id) + validate_question_criteria!(question, id, type.to_s) + end + + def field(question, name) + question.fetch(name) { question[name.to_sym] } + end + + def validate_question_instructions!(question, id) + has_key = question.key?("instructions") || question.key?(:instructions) + raise RequestError, "question #{id.inspect} is missing instructions" unless has_key + + instructions = field(question, "instructions") + # nil is what the official SDKs call an undescribed entry; anything the + # builders would reject as empty is rejected here too. + return if instructions.nil? + + case instructions + when String, Hash, Array + raise RequestError, "question #{id.inspect} has empty instructions" if instructions.empty? + else + raise RequestError, "question #{id.inspect} has instructions of type #{instructions.class}" + end + end + + def validate_question_criteria!(question, id, type) + criteria = field(question, "criteria") + + case type + when "choice" + unless criteria.is_a?(Hash) && criteria.size.between?(1, 255) + raise RequestError, "choice question #{id.inspect} needs criteria: a Hash with 1..255 entries" + end + when "score" + unless criteria.is_a?(Array) && criteria.size.between?(2, 10) + raise RequestError, "score question #{id.inspect} needs criteria: an Array with 2..10 entries" + end + end + end + private_class_method :field, :validate_question_instructions!, :validate_question_criteria! + def validate_instructions!(instructions) case instructions when String diff --git a/test/client_test.rb b/test/client_test.rb index de5e058..8eeb876 100644 --- a/test/client_test.rb +++ b/test/client_test.rb @@ -282,21 +282,16 @@ def test_malformed_score_missing_confidence_raises_invalid_response assert_includes error.message, "severity" end - def test_unsupported_question_type_raises_invalid_response + def test_unsupported_question_type_is_refused_before_the_request weird_questions = { "mystery" => { "type" => "unknown", "instructions" => "huh" } } - body = JSON.generate( - "id" => "resp_9", - "model" => "typesafe/jev-1.13", - "answers" => { "mystery" => { "type" => "unknown", "value" => "x" } }, - "usage" => {} - ) - transport = FakeTransport.new([[200, body]]) + transport = FakeTransport.new([[200, success_body]]) client = build_client(transport) - error = assert_raises(RubyDecisionModel::InvalidResponse) do + error = assert_raises(RubyDecisionModel::RequestError) do client.ask(state: {}, questions: weird_questions) end assert_includes error.message, "mystery" + assert_empty transport.calls end def test_non_hash_probabilities_fall_back_to_empty_hash diff --git a/test/question_validation_test.rb b/test/question_validation_test.rb new file mode 100644 index 0000000..898874f --- /dev/null +++ b/test/question_validation_test.rb @@ -0,0 +1,155 @@ +# frozen_string_literal: true + +require "test_helper" + +# Requests are billed by input tokens, so a question the API is going to +# refuse should never leave the process. +class QuestionValidationTest < Minitest::Test + def setup + @transport = FakeTransport.new([[200, success_body]]) + @client = RubyDecisionModel::Client.new(api_key: "test-key", transport: @transport, sleeper: no_sleep) + end + + def success_body + JSON.generate( + "answers" => { "q" => { "type" => "noul", "noul" => 0.5 } }, + "usage" => { "input_tokens" => 1, "output_tokens" => 1 } + ) + end + + def refuse(questions) + error = assert_raises(RubyDecisionModel::RequestError) { @client.ask(state: {}, questions: questions) } + assert_empty @transport.calls, "the request should not have been sent" + error + end + + # --- the map itself --- + + def test_empty_and_nil_question_maps_are_refused + refuse({}) + refuse(nil) + end + + def test_a_questions_argument_that_is_not_a_hash_is_refused + error = refuse([RubyDecisionModel::Questions.noul("Is this urgent?")]) + assert_match(/must be a Hash/, error.message) + end + + def test_a_blank_question_id_is_refused + error = refuse({ " " => RubyDecisionModel::Questions.noul("Is this urgent?") }) + assert_match(/is blank/, error.message) + end + + def test_ids_that_collide_once_stringified_are_refused + # Both arrive as "a" on the wire, so one answer would come back for two + # questions and the other would look like it went missing. + error = refuse({ :a => RubyDecisionModel::Questions.noul("First?"), + "a" => RubyDecisionModel::Questions.noul("Second?") }) + assert_match(/both "a" on the wire/, error.message) + end + + def test_symbol_ids_on_their_own_are_fine + response = @client.ask(state: {}, questions: { q: RubyDecisionModel::Questions.noul("Is this urgent?") }) + assert_in_delta 0.5, response["q"].noul + end + + # --- each question --- + + def test_a_question_that_is_not_a_hash_is_refused + error = refuse({ "q" => "is this urgent?" }) + assert_match(/must be a Hash/, error.message) + assert_match(/"q"/, error.message) + end + + def test_an_unknown_type_is_refused + error = refuse({ "q" => { "type" => "ranking", "instructions" => "Rank these" } }) + assert_match(/expected one of noul, choice, score/, error.message) + end + + def test_a_missing_type_is_refused + refuse({ "q" => { "instructions" => "Is this urgent?" } }) + end + + def test_missing_instructions_are_refused + error = refuse({ "q" => { "type" => "noul" } }) + assert_match(/missing instructions/, error.message) + end + + def test_empty_instructions_are_refused + refuse({ "q" => { "type" => "noul", "instructions" => "" } }) + refuse({ "q" => { "type" => "noul", "instructions" => [] } }) + end + + def test_instructions_of_the_wrong_type_are_refused + error = refuse({ "q" => { "type" => "noul", "instructions" => 42 } }) + assert_match(/instructions of type Integer/, error.message) + end + + def test_nil_instructions_are_allowed + # EntryType in the official SDKs covers null for state, instructions, and + # criteria alike. + response = @client.ask(state: {}, questions: { "q" => { "type" => "noul", "instructions" => nil } }) + assert_in_delta 0.5, response["q"].noul + end + + # --- criteria each type requires --- + + def test_a_choice_question_without_criteria_is_refused + error = refuse({ "q" => { "type" => "choice", "instructions" => "Which team?" } }) + assert_match(/needs criteria/, error.message) + end + + def test_a_choice_question_with_too_many_options_is_refused + criteria = (1..256).to_h { |i| [i.to_s, nil] } + refuse({ "q" => { "type" => "choice", "instructions" => "Which?", "criteria" => criteria } }) + end + + def test_a_score_question_with_one_level_is_refused + refuse({ "q" => { "type" => "score", "instructions" => "How bad?", "criteria" => ["only one"] } }) + end + + def test_a_score_question_with_eleven_levels_is_refused + refuse({ "q" => { "type" => "score", "instructions" => "How bad?", "criteria" => (1..11).map(&:to_s) } }) + end + + def test_a_noul_question_needs_no_criteria + response = @client.ask(state: {}, questions: { "q" => { "type" => "noul", "instructions" => "Urgent?" } }) + assert_in_delta 0.5, response["q"].noul + end + + def test_symbol_keyed_question_hashes_are_accepted + response = @client.ask(state: {}, questions: { "q" => { type: "noul", instructions: "Urgent?" } }) + assert_in_delta 0.5, response["q"].noul + end + + def test_questions_from_the_builders_pass + transport = FakeTransport.new([[200, JSON.generate( + "answers" => { + "a" => { "type" => "noul", "noul" => 0.5 }, + "b" => { "type" => "choice", "choice" => "x", "confidence" => 0.5, "probabilities" => {} }, + "c" => { "type" => "score", "score" => 1.0, "confidence" => 0.5, "probabilities" => {}, "legend" => {} } + }, + "usage" => {} + )]]) + client = RubyDecisionModel::Client.new(api_key: "k", transport: transport, sleeper: no_sleep) + + response = client.ask(state: {}, questions: { + "a" => RubyDecisionModel::Questions.noul("Urgent?"), + "b" => RubyDecisionModel::Questions.choice("Which?", criteria: { "x" => nil }), + "c" => RubyDecisionModel::Questions.score("How bad?", criteria: %w[low high]) + }) + + assert_equal 3, response.answers.size + end + + # --- the validator on its own --- + + def test_validate_is_callable_directly + assert_nil RubyDecisionModel::Questions.validate!( + RubyDecisionModel::Questions.noul("Urgent?"), id: "q" + ) + assert_raises(RubyDecisionModel::RequestError) do + RubyDecisionModel::Questions.validate!({ "type" => "noul" }, id: "q") + end + end +end