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