diff --git a/CHANGELOG.md b/CHANGELOG.md index bee258049..1950742bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The dashboard keeps a catalog's pass bars** (`actionagent`). A + product's and a set's `pass` blocks (`min_pass_rate`, `threshold`, + `gated`) parsed in `ActiveAgent::Evals::Catalog`, but neither + `ScenarioProduct` nor `ScenarioSet` kept them: an import dropped the + bars, the export left them out, the stored catalog's `pass_thresholds` + fell back to the defaults (every group gated at 100%), and the import + preview reported the product and its sets as `update` on every re-import + of an unchanged file. The bars are now stored and exported as imported, + and both import modes take the document's. Existing installs add the + columns with `bin/rails generate action_agent:install --skip` and + `bin/rails db:migrate` (`add_catalog_pass_bars`); imports keep working, + without the bars, until it runs. Bars dropped by an earlier version come + back when the original document is imported again. +- **The dashboard keeps a catalog product's named criteria lists** + (`actionagent`). A product's `criteria: { shared: [...], strict: [...] }` + parsed and round-tripped in `ActiveAgent::Evals::Catalog`, but + `ScenarioProduct` had nowhere to keep it: an import dropped the lists, the + export left them out, the stored digest never matched the records' + export, and the import preview reported the product as `update` on every + re-import of an unchanged file. The lists are now stored on the product + and exported as imported, so the documents and digests match and an + unchanged file previews as unchanged. Both import modes take the + document's lists, as they take a set's `criteria`. A set still runs on + its own `criteria`; which product lists a run uses is left for later. + Existing installs add the column with + `bin/rails generate action_agent:install --skip` and `bin/rails db:migrate` + (`add_catalog_product_criteria`); imports keep working, without the lists, + until it runs. Lists dropped by an earlier version come back when the + original document is imported again (#687). - **A self-hosted dashboard shows its own trace endpoint** (`actionagent`). The Organization page's Telemetry card showed the platform's `/v1/traces`. A self-hosted engine takes traces at `/api/traces`, and the card diff --git a/actionagent/app/models/action_agent/scenario_product.rb b/actionagent/app/models/action_agent/scenario_product.rb index 4856a6811..ec7060d6f 100644 --- a/actionagent/app/models/action_agent/scenario_product.rb +++ b/actionagent/app/models/action_agent/scenario_product.rb @@ -21,6 +21,28 @@ def metadata value.is_a?(Hash) ? value : {} end + # The product's named criteria lists, as the catalog's `criteria:` gives + # them: list name => criterion hashes. A stored value that is not such a + # mapping, and a list left with no criterion, read as no list. + # @return [Hash{String => Array}] + def criteria + value = self[:criteria] + return {} unless value.is_a?(Hash) + + value.each_with_object({}) do |(name, list), lists| + entries = Array(list).select { |criterion| criterion.is_a?(Hash) } if list.is_a?(Array) + lists[name.to_s] = entries if entries.present? + end + end + + # The bars the catalog's `pass:` declares (ActiveAgent::Evals::PassThresholds); + # a stored value that is not a mapping reads as none. + # @return [Hash{String => Object}] + def pass + value = self[:pass] + value.is_a?(Hash) ? value : {} + end + # The agent a set of this product runs against when the caller names # none: the product's agent, else the project's agent under test, else # the owner's agent named by the document. @@ -36,6 +58,8 @@ def to_document "name" => name, "description" => description, "agent" => agent_name.presence || agent&.name, + "criteria" => criteria.presence, + "pass" => pass.presence, "sets" => sets.map(&:to_document) }.merge(metadata).compact end diff --git a/actionagent/app/models/action_agent/scenario_set.rb b/actionagent/app/models/action_agent/scenario_set.rb index c19ff6955..b1202b0af 100644 --- a/actionagent/app/models/action_agent/scenario_set.rb +++ b/actionagent/app/models/action_agent/scenario_set.rb @@ -37,6 +37,14 @@ def metadata value.is_a?(Hash) ? value : {} end + # The bars the catalog's `pass:` declares (ActiveAgent::Evals::PassThresholds); + # a stored value that is not a mapping reads as none. + # @return [Hash{String => Object}] + def pass + value = self[:pass] + value.is_a?(Hash) ? value : {} + end + # The evaluation's name: the catalog, product and set keys. def evaluation_name "#{catalog.key}/#{product.key}/#{key}".first(EvaluationReportImport::MAX_EVALUATION_NAME) @@ -117,6 +125,7 @@ def to_document "description" => description, "judge" => judge.presence, "criteria" => criteria.presence, + "pass" => pass.presence, "scenarios" => scenarios.map(&:to_document) }.merge(metadata).compact end diff --git a/actionagent/app/services/action_agent/scenario_catalog_import.rb b/actionagent/app/services/action_agent/scenario_catalog_import.rb index 063481fdb..d87a44d46 100644 --- a/actionagent/app/services/action_agent/scenario_catalog_import.rb +++ b/actionagent/app/services/action_agent/scenario_catalog_import.rb @@ -181,6 +181,10 @@ def replace_products(parsed) metadata: entry.metadata, agent: resolve_agent(entry) || (merge? ? product.agent : nil), project: resolve_project(entry) || (merge? ? product.project : nil) ) + # Until add_catalog_product_criteria runs, imports keep working without the lists. + product.criteria = entry.criteria if product.has_attribute?(:criteria) + # Until add_catalog_pass_bars runs, imports keep working without the bars. + product.pass = entry.pass if product.has_attribute?(:pass) product.position = position_for(product, entry.position, offset) product.save! replace_sets(product, entry) @@ -197,6 +201,7 @@ def replace_sets(product, entry) name: set_entry.name, description: set_entry.description, judge: set_entry.judge, criteria: set_entry.criteria, metadata: set_entry.metadata ) + set.pass = set_entry.pass if set.has_attribute?(:pass) set.position = position_for(set, set_entry.position, offset) set.save! replace_scenarios(set, set_entry) diff --git a/actionagent/lib/generators/action_agent/templates/migrations/034_add_catalog_product_criteria.rb.erb b/actionagent/lib/generators/action_agent/templates/migrations/034_add_catalog_product_criteria.rb.erb new file mode 100644 index 000000000..75fce9f47 --- /dev/null +++ b/actionagent/lib/generators/action_agent/templates/migrations/034_add_catalog_product_criteria.rb.erb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class AddCatalogProductCriteria < ActiveRecord::Migration<%= migration_version %> + def up + return unless table_exists?(products_table) + return if column_exists?(products_table, :criteria) + + type = connection.adapter_name.to_s.downcase.include?("postgres") ? :jsonb : :json + add_column products_table, :criteria, type + end + + def down + return unless table_exists?(products_table) + return unless column_exists?(products_table, :criteria) + + remove_column products_table, :criteria + end + + private + + def products_table + "#{ActionAgent.table_name_prefix}scenario_products" + end +end diff --git a/actionagent/lib/generators/action_agent/templates/migrations/035_add_catalog_pass_bars.rb.erb b/actionagent/lib/generators/action_agent/templates/migrations/035_add_catalog_pass_bars.rb.erb new file mode 100644 index 000000000..756f6239c --- /dev/null +++ b/actionagent/lib/generators/action_agent/templates/migrations/035_add_catalog_pass_bars.rb.erb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class AddCatalogPassBars < ActiveRecord::Migration<%= migration_version %> + def up + type = connection.adapter_name.to_s.downcase.include?("postgres") ? :jsonb : :json + catalog_tables.each do |table| + next unless table_exists?(table) + next if column_exists?(table, :pass) + + add_column table, :pass, type + end + end + + def down + catalog_tables.each do |table| + next unless table_exists?(table) + next unless column_exists?(table, :pass) + + remove_column table, :pass + end + end + + private + + def catalog_tables + %w[scenario_products scenario_sets].map { |table| "#{ActionAgent.table_name_prefix}#{table}" } + end +end diff --git a/actionagent/test/catalog_pass_bars_test.rb b/actionagent/test/catalog_pass_bars_test.rb new file mode 100644 index 000000000..ba64d3ba0 --- /dev/null +++ b/actionagent/test/catalog_pass_bars_test.rb @@ -0,0 +1,176 @@ +# frozen_string_literal: true + +require "test_helper" + +# A catalog product's and set's `pass` bars (PassThresholds) kept by the +# dashboard: stored on import, emitted by the export, compared by the import +# preview, and read back by the stored catalog's pass_thresholds. +class CatalogPassBarsTest < ActiveSupport::TestCase + # Makes ScenarioProduct and ScenarioSet report no pass column while + # +hidden+ is set, as an install that has not run add_catalog_pass_bars does. + module WithoutPassColumn + mattr_accessor :hidden, default: false + + def has_attribute?(name) + return false if WithoutPassColumn.hidden && name.to_s == "pass" + + super + end + end + ActionAgent::ScenarioProduct.prepend(WithoutPassColumn) + ActionAgent::ScenarioSet.prepend(WithoutPassColumn) + + def setup + ActionAgent::ScenarioCatalog.destroy_all + @storage = ActionAgent.active_storage + ActionAgent.active_storage = :auto + end + + def teardown + ActionAgent.active_storage = @storage + end + + def document(product_pass: { "min_pass_rate" => 0.8 }, critical: { "min_pass_rate" => 1.0 }, informational: { "gated" => false }) + { + "catalog" => "order_suite", + "products" => [ { + "key" => "checkout", "pass" => product_pass, + "sets" => [ + { "key" => "critical", "pass" => critical, + "scenarios" => [ { "key" => "cannot_charge_twice", "prompt" => "Check duplicate charge." } ] }, + { "key" => "informational", "pass" => informational, + "scenarios" => [ { "key" => "shipping_eta", "prompt" => "Check shipping ETA." } ] } + ].map(&:compact) + }.compact ] + } + end + + def import(input = document, **options) + ActionAgent::ScenarioCatalogImport.new(owner: nil, document: input.to_yaml, **options).call + end + + def preview(input = document, **options) + ActionAgent::ScenarioCatalogImport.new(owner: nil, document: input.to_yaml, **options).preview + end + + def actions(result) + result[:changes].reject { |change| %w[unchanged keep].include?(change[:action]) }.to_h { |change| [ change[:path], change[:action] ] } + end + + test "the stored catalog gates a product's sets with the bars the document declared" do + parsed = ActiveAgent::Evals::Catalog.parse(document.to_yaml) + catalog = import.reload + + assert_equal parsed.pass_thresholds(:checkout).to_h, catalog.to_catalog.pass_thresholds(:checkout).to_h + assert_equal parsed.pass_thresholds(:checkout, :critical).to_h, catalog.to_catalog.pass_thresholds(:checkout, :critical).to_h + thresholds = catalog.to_catalog.pass_thresholds(:checkout) + assert_equal 0.8, thresholds.min_pass_rate("informational") + assert_equal 1.0, thresholds.min_pass_rate("critical") + assert thresholds.gated?("critical") + assert_not thresholds.gated?("informational") + end + + test "the bars survive the database and export as the document imported them" do + parsed = ActiveAgent::Evals::Catalog.parse(document.to_yaml) + catalog = import.reload + product = catalog.products.sole + + assert_equal({ "min_pass_rate" => 0.8 }, product.pass) + assert_equal({ "critical" => { "min_pass_rate" => 1.0 }, "informational" => { "gated" => false } }, + product.sets.to_h { |set| [ set.key, set.pass ] }) + assert_equal parsed.to_h, catalog.to_catalog.to_h + exported = ActiveAgent::Evals::Catalog.parse(catalog.export_yaml) + assert_equal parsed.to_h, exported.to_h + assert_equal catalog.digest, exported.digest + critical = product.sets.find_by!(key: "critical") + narrowed = YAML.safe_load(catalog.export_yaml(set_ids: [ critical.id ])) + assert_equal({ "min_pass_rate" => 0.8 }, narrowed.dig("products", 0, "pass")) + assert_equal({ "min_pass_rate" => 1.0 }, narrowed.dig("products", 0, "sets", 0, "pass")) + end + + test "re-importing the same document previews no change" do + import + result = preview + + assert_equal "unchanged", result.dig(:catalog, :action) + assert_empty result[:changes] + assert_equal({ "unchanged" => 5 }, result[:counts]) + end + + test "a changed or removed bar previews as an update and replaces the stored bars" do + import + stricter = document(product_pass: { "min_pass_rate" => 0.9, "threshold" => 0.7 }, informational: nil) + + assert_equal({ "checkout" => "update", "checkout/informational" => "update" }, actions(preview(stricter))) + catalog = import(stricter) + assert_equal({ "min_pass_rate" => 0.9, "threshold" => 0.7 }, catalog.products.sole.reload.pass) + assert_equal({}, catalog.sets.find_by!(key: "informational").pass) + assert_not YAML.safe_load(catalog.export_yaml).dig("products", 0, "sets", 1).key?("pass") + assert catalog.to_catalog.pass_thresholds(:checkout).gated?("informational") + end + + test "merge mode takes the document's bars, and a narrowed export merges back unchanged" do + catalog = import + critical = catalog.sets.find_by!(key: "critical") + merged = ActionAgent::ScenarioCatalogImport.new(owner: nil, document: catalog.export_yaml(set_ids: [ critical.id ]), mode: "merge") + + assert_empty actions(merged.preview) + merged.call + assert_equal({ "min_pass_rate" => 1.0 }, critical.reload.pass) + assert_equal({ "gated" => false }, catalog.sets.find_by!(key: "informational").pass) + + relaxed = document(critical: { "min_pass_rate" => 0.5 }) + relaxed["products"][0]["sets"].pop + assert_equal({ "checkout/critical" => "update" }, actions(preview(relaxed, mode: "merge"))) + import(relaxed, mode: "merge") + assert_equal({ "min_pass_rate" => 0.5 }, critical.reload.pass) + assert_equal({ "gated" => false }, catalog.sets.find_by!(key: "informational").pass) + + bare = document(product_pass: nil, critical: nil, informational: nil) + assert_equal({ "checkout" => "update", "checkout/critical" => "update", "checkout/informational" => "update" }, + actions(preview(bare, mode: "merge"))) + import(bare, mode: "merge") + assert_equal({}, catalog.products.sole.reload.pass, "the preview's update is what the import does") + assert_equal({}, critical.reload.pass) + assert_equal({}, catalog.sets.find_by!(key: "informational").pass) + end + + test "a catalog without bars imports, exports and previews as before" do + bare = document(product_pass: nil, critical: nil, informational: nil) + catalog = import(bare).reload + exported = YAML.safe_load(catalog.export_yaml) + + assert_not exported.dig("products", 0).key?("pass") + assert(exported.dig("products", 0, "sets").none? { |set| set.key?("pass") }) + assert_equal ActiveAgent::Evals::Catalog.parse(bare.to_yaml).digest, catalog.digest + assert_empty preview(bare)[:changes] + assert_equal ActiveAgent::Evals::PassThresholds.default.to_h, catalog.to_catalog.pass_thresholds(:checkout).to_h + end + + test "before the migration runs, a catalog imports and exports without the bars" do + WithoutPassColumn.hidden = true + catalog = import.reload + + assert_equal({}, catalog.products.sole.pass) + assert_nil catalog.products.sole.read_attribute_before_type_cast(:pass), "the import writes no product bars" + assert(catalog.sets.all? { |set| set.read_attribute_before_type_cast(:pass).nil? }, "the import writes no set bars") + assert_not YAML.safe_load(catalog.export_yaml).dig("products", 0).key?("pass") + assert_equal 2, catalog.scenarios.count + ensure + WithoutPassColumn.hidden = false + end + + test "a stored value that is not a mapping reads as no bars" do + catalog = import + product = catalog.products.sole + set = catalog.sets.find_by!(key: "critical") + [ nil, [], "0.8", 0.8 ].each do |stored| + product.update_column(:pass, stored) + set.update_column(:pass, stored) + assert_equal({}, product.reload.pass, stored.inspect) + assert_equal({}, set.reload.pass, stored.inspect) + assert_not product.to_document.key?("pass"), stored.inspect + assert_not set.to_document.key?("pass"), stored.inspect + end + end +end diff --git a/actionagent/test/catalog_product_criteria_test.rb b/actionagent/test/catalog_product_criteria_test.rb new file mode 100644 index 000000000..d49c57ff7 --- /dev/null +++ b/actionagent/test/catalog_product_criteria_test.rb @@ -0,0 +1,173 @@ +# frozen_string_literal: true + +require "test_helper" + +# A catalog product's named criteria lists (`criteria: { name => [...] }`) +# kept by the dashboard: stored on import, emitted by the export, compared +# by the import preview, and layered by name and criterion key in merge mode. +class CatalogProductCriteriaTest < ActiveSupport::TestCase + # Makes ScenarioProduct report no criteria column while +hidden+ is set, + # as an install that has not run add_catalog_product_criteria does. + module WithoutCriteriaColumn + mattr_accessor :hidden, default: false + + def has_attribute?(name) + return false if WithoutCriteriaColumn.hidden && name.to_s == "criteria" + + super + end + end + ActionAgent::ScenarioProduct.prepend(WithoutCriteriaColumn) + + LISTS = { + "shared" => [ { "key" => "answered", "type" => "response_present" } ], + "strict" => [ + { "key" => "answered", "type" => "response_present" }, + { "key" => "polite", "type" => "not_contains", "config" => { "pattern" => "unfortunately" } } + ] + }.freeze + + SET_CRITERIA = [ { "key" => "answered", "type" => "response_present" } ].freeze + + def setup + ActionAgent::ScenarioCatalog.destroy_all + @storage = ActionAgent.active_storage + ActionAgent.active_storage = :auto + end + + def teardown + ActionAgent.active_storage = @storage + end + + def document(lists = LISTS) + { + "catalog" => "support_desk", + "products" => [ { + "key" => "triage", "name" => "Triage", "criteria" => lists, + "sets" => [ { + "key" => "smoke", "criteria" => SET_CRITERIA, + "scenarios" => [ { "key" => "order_1", "prompt" => "Where is order 1?" } ] + } ] + }.compact ] + } + end + + def import(input = document, **options) + ActionAgent::ScenarioCatalogImport.new(owner: nil, document: input.to_yaml, **options).call + end + + def preview(input = document, **options) + ActionAgent::ScenarioCatalogImport.new(owner: nil, document: input.to_yaml, **options).preview + end + + def product_actions(result) + result[:changes].select { |change| change[:level] == "product" }.to_h { |change| [ change[:path], change[:action] ] } + end + + test "a product's named lists survive the database and export as the document imported them" do + parsed = ActiveAgent::Evals::Catalog.parse(document.to_yaml) + catalog = import.reload + + assert_equal LISTS, catalog.products.sole.criteria + assert_equal parsed.to_h, catalog.to_catalog.to_h + exported = ActiveAgent::Evals::Catalog.parse(catalog.export_yaml) + assert_equal parsed.to_h, exported.to_h + assert_equal parsed.digest, exported.digest + assert_equal catalog.digest, exported.digest + assert_equal LISTS, YAML.safe_load(catalog.export_yaml(product_ids: [ catalog.products.sole.id ])).dig("products", 0, "criteria") + assert_equal LISTS, YAML.safe_load(catalog.export_yaml(set_ids: [ catalog.sets.sole.id ])).dig("products", 0, "criteria") + end + + test "the lists reach a suite of the stored product, alongside the set's own criteria" do + catalog = import.reload + suite = catalog.to_catalog.suite_for(:triage) + + assert_equal %w[answered polite], suite.criteria(:shared, :strict).map { |criterion| criterion["key"] } + assert_equal SET_CRITERIA, catalog.sets.sole.criteria + assert_equal SET_CRITERIA, catalog.to_catalog.set(:triage, :smoke).criteria + end + + test "re-importing the same document previews no change and adds no records" do + import + result = preview + + assert_equal "unchanged", result.dig(:catalog, :action) + assert_empty result[:changes] + assert_equal({ "unchanged" => 3 }, result[:counts]) + import + assert_equal 1, ActionAgent::ScenarioProduct.count + end + + test "a changed or removed list previews as an update and replaces the stored lists" do + import + changed = LISTS.merge("strict" => [ { "key" => "answered", "type" => "response_present" } ]) + + assert_equal({ "triage" => "update" }, product_actions(preview(document(changed)))) + assert_equal changed, import(document(changed)).products.sole.reload.criteria + + only_shared = LISTS.slice("shared") + assert_equal({ "triage" => "update" }, product_actions(preview(document(only_shared)))) + assert_equal only_shared, import(document(only_shared)).products.sole.reload.criteria + + catalog = import(document(nil)) + assert_equal({}, catalog.products.sole.reload.criteria) + assert_nil YAML.safe_load(catalog.export_yaml).dig("products", 0).fetch("criteria", nil) + end + + test "merge mode takes the document's lists as it takes a set's criteria, and a narrowed export merges back unchanged" do + catalog = import + set_export = catalog.export_yaml(set_ids: [ catalog.sets.sole.id ]) + merged = ActionAgent::ScenarioCatalogImport.new(owner: nil, document: set_export, mode: "merge") + + assert_empty merged.preview[:changes].reject { |change| change[:action] == "unchanged" } + assert_equal LISTS, merged.call.products.sole.reload.criteria + + overlay = { "terse" => [ { "key" => "detailed", "type" => "min_length", "config" => { "chars" => 40 } } ] } + assert_equal({ "triage" => "update" }, product_actions(preview(document(overlay), mode: "merge"))) + assert_equal overlay, import(document(overlay), mode: "merge").products.sole.reload.criteria + assert_equal SET_CRITERIA, catalog.sets.sole.reload.criteria + + assert_equal({ "triage" => "update" }, product_actions(preview(document(nil), mode: "merge"))) + assert_equal({}, import(document(nil), mode: "merge").products.sole.reload.criteria, "the preview's update is what the import does") + end + + test "a bare criteria list on a product is stored as the default list" do + catalog = import(document(SET_CRITERIA)) + + assert_equal({ "default" => SET_CRITERIA }, catalog.products.sole.reload.criteria) + assert_equal catalog.digest, ActiveAgent::Evals::Catalog.parse(catalog.export_yaml).digest + end + + test "a catalog without product criteria imports, exports and previews as before" do + catalog = import(document(nil)).reload + exported = YAML.safe_load(catalog.export_yaml) + + assert_not exported.dig("products", 0).key?("criteria") + assert_equal ActiveAgent::Evals::Catalog.parse(document(nil).to_yaml).digest, catalog.digest + assert_empty preview(document(nil))[:changes] + end + + test "before the migration runs, a catalog imports and exports without the lists" do + WithoutCriteriaColumn.hidden = true + catalog = import.reload + product = catalog.products.sole + + assert_equal({}, product.criteria) + assert_nil product.read_attribute_before_type_cast(:criteria), "the import writes no lists" + assert_not YAML.safe_load(catalog.export_yaml).dig("products", 0).key?("criteria") + assert_equal "Where is order 1?", catalog.scenarios.sole.prompt + ensure + WithoutCriteriaColumn.hidden = false + end + + test "a stored value that is not a mapping of lists reads as no lists" do + catalog = import + product = catalog.products.sole + [ nil, [], "shared", { "shared" => "answered" }, { "shared" => [ "answered" ] } ].each do |stored| + product.update_column(:criteria, stored) + product.reload + assert_equal({}, product.criteria, stored.inspect) + assert_not product.to_document.key?("criteria"), stored.inspect + end + end +end diff --git a/actionagent/test/install_generator_test.rb b/actionagent/test/install_generator_test.rb index d9ae9b4eb..9583c43ad 100644 --- a/actionagent/test/install_generator_test.rb +++ b/actionagent/test/install_generator_test.rb @@ -551,6 +551,120 @@ class ActionAgentInstallGeneratorTest < Rails::Generators::TestCase end end + test "the product criteria upgrade follows catalog creation and is emitted again only when missing" do + run_generator [ "--skip-routes" ] + assert_migration "db/migrate/add_catalog_product_criteria.rb" + assert_operator migration_version("add_catalog_product_criteria"), :>, migration_version("create_active_agent_scenario_catalogs") + FileUtils.rm(migration_file_name("db/migrate/add_catalog_product_criteria.rb")) + before = Dir.children(File.join(destination_root, "db/migrate")) + + run_generator [ "--skip-routes" ] + emitted = Dir.children(File.join(destination_root, "db/migrate")) - before + assert_equal 1, emitted.grep(/_add_catalog_product_criteria\.rb\z/).size + run_generator [ "--skip-routes" ] + assert_equal 1, Dir.glob(File.join(destination_root, "db/migrate/*_add_catalog_product_criteria.rb")).size + end + + test "the product criteria upgrade preserves old rows under a custom prefix, reverses idempotently and skips a missing table" do + run_generator [ "--skip-routes" ] + connection = ActiveRecord::Base.connection + prefix = "product_criteria_probe_" + table = "#{prefix}scenario_products" + connection.create_table(table, force: true) { |t| t.string :key } + connection.execute("INSERT INTO #{table} (key) VALUES ('existing')") + ActionAgent.table_name_prefix = prefix + + 2.times { run_migration("add_catalog_product_criteria", :AddCatalogProductCriteria) } + assert connection.column_exists?(table, :criteria) + assert_equal [ "existing" ], connection.select_values("SELECT key FROM #{table}") + assert_nil connection.select_value("SELECT criteria FROM #{table}") + 2.times { run_migration("add_catalog_product_criteria", :AddCatalogProductCriteria, :down) } + assert_not connection.column_exists?(table, :criteria) + assert_equal [ "existing" ], connection.select_values("SELECT key FROM #{table}") + + ActionAgent.table_name_prefix = "missing_product_probe_" + run_migration("add_catalog_product_criteria", :AddCatalogProductCriteria) + run_migration("add_catalog_product_criteria", :AddCatalogProductCriteria, :down) + assert_not connection.table_exists?("missing_product_probe_scenario_products") + ensure + ActionAgent.table_name_prefix = "active_agent_" + connection&.drop_table(table, if_exists: true) if table + end + + test "the product criteria upgrade uses each adapter's JSON type without a JSON default" do + run_generator [ "--skip-routes" ] + namespace = Module.new + namespace.module_eval(File.read(migration_file_name("db/migrate/add_catalog_product_criteria.rb"))) + { "PostgreSQL" => :jsonb, "Mysql2" => :json, "Trilogy" => :json, "SQLite" => :json }.each do |adapter, type| + migration = namespace::AddCatalogProductCriteria.new + added = [] + connection = connection_without_metadata_column(adapter, added) + ActiveRecord::Migration.suppress_messages { migration.stub(:connection, connection) { migration.up } } + assert_equal [ [ "active_agent_scenario_products", :criteria, type, {} ] ], added, adapter + end + end + + test "the pass bars upgrade follows catalog creation and is emitted again only when missing" do + run_generator [ "--skip-routes" ] + assert_migration "db/migrate/add_catalog_pass_bars.rb" + assert_operator migration_version("add_catalog_pass_bars"), :>, migration_version("create_active_agent_scenario_catalogs") + FileUtils.rm(migration_file_name("db/migrate/add_catalog_pass_bars.rb")) + before = Dir.children(File.join(destination_root, "db/migrate")) + + run_generator [ "--skip-routes" ] + emitted = Dir.children(File.join(destination_root, "db/migrate")) - before + assert_equal 1, emitted.grep(/_add_catalog_pass_bars\.rb\z/).size + run_generator [ "--skip-routes" ] + assert_equal 1, Dir.glob(File.join(destination_root, "db/migrate/*_add_catalog_pass_bars.rb")).size + end + + test "the pass bars upgrade preserves old rows under a custom prefix, reverses idempotently and skips a missing table" do + run_generator [ "--skip-routes" ] + connection = ActiveRecord::Base.connection + prefix = "pass_bars_probe_" + tables = %w[scenario_products scenario_sets].map { |name| "#{prefix}#{name}" } + tables.each do |table| + connection.create_table(table, force: true) { |t| t.string :key } + connection.execute("INSERT INTO #{table} (key) VALUES ('existing')") + end + ActionAgent.table_name_prefix = prefix + + 2.times { run_migration("add_catalog_pass_bars", :AddCatalogPassBars) } + tables.each do |table| + assert connection.column_exists?(table, :pass), table + assert_equal [ "existing" ], connection.select_values("SELECT key FROM #{table}") + assert_nil connection.select_value("SELECT pass FROM #{table}") + end + 2.times { run_migration("add_catalog_pass_bars", :AddCatalogPassBars, :down) } + tables.each do |table| + assert_not connection.column_exists?(table, :pass), table + assert_equal [ "existing" ], connection.select_values("SELECT key FROM #{table}") + end + + connection.drop_table(tables.last) + 2.times { run_migration("add_catalog_pass_bars", :AddCatalogPassBars) } + assert connection.column_exists?(tables.first, :pass), "a missing sets table does not stop the products column" + run_migration("add_catalog_pass_bars", :AddCatalogPassBars, :down) + assert_not connection.table_exists?(tables.last) + ensure + ActionAgent.table_name_prefix = "active_agent_" + tables&.each { |table| connection&.drop_table(table, if_exists: true) } + end + + test "the pass bars upgrade uses each adapter's JSON type without a JSON default" do + run_generator [ "--skip-routes" ] + namespace = Module.new + namespace.module_eval(File.read(migration_file_name("db/migrate/add_catalog_pass_bars.rb"))) + { "PostgreSQL" => :jsonb, "Mysql2" => :json, "Trilogy" => :json, "SQLite" => :json }.each do |adapter, type| + migration = namespace::AddCatalogPassBars.new + added = [] + connection = connection_without_metadata_column(adapter, added) + ActiveRecord::Migration.suppress_messages { migration.stub(:connection, connection) { migration.up } } + expected = %w[active_agent_scenario_products active_agent_scenario_sets].map { |table| [ table, :pass, type, {} ] } + assert_equal expected, added, adapter + end + end + private # The real connection, reporting +adapter+ and a table without the column, diff --git a/actionagent/test/scenario_catalogs_api_test.rb b/actionagent/test/scenario_catalogs_api_test.rb index 813abf44f..ae210c79e 100644 --- a/actionagent/test/scenario_catalogs_api_test.rb +++ b/actionagent/test/scenario_catalogs_api_test.rb @@ -89,6 +89,73 @@ def body assert_includes response.body, "key: add_item" end + test "a product's named criteria lists come back from the export and preview as unchanged" do + document = <<~YAML + catalog: checkout + products: + - key: cart + agent: Cart + criteria: + shared: + - { key: answered, type: response_present } + strict: + - { key: answered, type: response_present } + - { key: polite, type: not_contains, config: { pattern: unfortunately } } + sets: + - key: uat + scenarios: + - key: add_item + prompt: Add two of item 7 to the cart. + YAML + post "/activeagents/api/scenario_catalogs", params: { document: document }, as: :json + assert_response :created + id = body["catalogs"].sole["id"] + + get "/activeagents/api/scenario_catalogs/#{id}/export" + assert_response :success + exported = ActiveAgent::Evals::Catalog.parse(response.body) + assert_equal ActiveAgent::Evals::Catalog.parse(document).digest, exported.digest + assert_equal %w[shared strict], exported.product(:cart).criteria.keys + + post "/activeagents/api/scenario_catalogs/preview", params: { document: document }, as: :json + assert_response :success + assert_empty body["previews"].sole["changes"] + end + + test "a product's and a set's pass bars come back from the export and preview as unchanged" do + document = <<~YAML + catalog: checkout + products: + - key: cart + agent: Cart + pass: { min_pass_rate: 0.8 } + sets: + - key: critical + pass: { min_pass_rate: 1.0 } + scenarios: + - key: add_item + prompt: Add two of item 7 to the cart. + - key: informational + pass: { gated: false } + scenarios: + - key: eta + prompt: When does the cart ship? + YAML + post "/activeagents/api/scenario_catalogs", params: { document: document }, as: :json + assert_response :created + id = body["catalogs"].sole["id"] + + get "/activeagents/api/scenario_catalogs/#{id}/export" + assert_response :success + exported = ActiveAgent::Evals::Catalog.parse(response.body) + assert_equal ActiveAgent::Evals::Catalog.parse(document).digest, exported.digest + assert_equal ActiveAgent::Evals::Catalog.parse(document).pass_thresholds(:cart), exported.pass_thresholds(:cart) + + post "/activeagents/api/scenario_catalogs/preview", params: { document: document }, as: :json + assert_response :success + assert_empty body["previews"].sole["changes"] + end + test "an invalid document is refused with the reason" do post "/activeagents/api/scenario_catalogs", params: { document: "catalog: x\nproducts: [{ name: nameless }]" }, as: :json diff --git a/docs/framework/dashboard.md b/docs/framework/dashboard.md index 42303c11e..ca2c03496 100644 --- a/docs/framework/dashboard.md +++ b/docs/framework/dashboard.md @@ -891,6 +891,26 @@ bin/rails db:migrate This also installs the column on a fresh dashboard. Fields discarded by an earlier version can only be restored by importing the original document again. +A product's named `criteria` lists are stored the same way. They come back +in every export that includes the product, so a re-imported unchanged +document previews as unchanged and keeps its digest. Like a set's +`criteria`, the lists are a field of the product: replace and merge mode +both take the document's, so a narrowed export merges back unchanged. +A set still runs on its own +`criteria`: the product's lists are kept, not scored. Existing installs +need the `add_catalog_product_criteria` migration (the same +`action_agent:install --skip` and `db:migrate` as above); until it runs, +imports keep working and leave the lists out. Lists dropped by an earlier +version come back when the original document is imported again. + +A product's and a set's `pass` bars are stored the same way, so the stored +catalog's `pass_thresholds` answer as the document's do and an unchanged +document previews as unchanged. Both import modes take the document's +bars, so a set imported without `pass` loses its stored one. Existing +installs need the `add_catalog_pass_bars` migration; until it runs, +imports keep working and leave the bars out. Bars dropped by an earlier +version come back when the original document is imported again. + **Permissions and gates.** Importing, re-importing, deleting, syncing, materializing and running a set change evaluation scenarios, so each needs the `replace_scenarios` [permission](#permissions); reading needs none. diff --git a/docs/framework/evaluations.md b/docs/framework/evaluations.md index baaff8840..05b6230db 100644 --- a/docs/framework/evaluations.md +++ b/docs/framework/evaluations.md @@ -1390,15 +1390,16 @@ new one is appended), so `catalog.suite_for(:triage).criteria(:shared)` reads a product's `shared` list the way `Suite#criteria` does. A set keeps a criterion without a `key`, appended as it is and once, however many layered documents restate it. The mounted dashboard stores a set's -criteria; a product's named lists stay in the YAML and do not survive its -import and export, so the import preview reports a product that carries -them as changed on every re-import, and a document the dashboard writes -back from its records leaves them out. +criteria and a product's named lists, and exports both as it imported +them, so an unchanged document re-imports with no change; running a set +still scores on the set's own criteria (see +[Scenario catalogs](/framework/dashboard#scenario-catalogs)). A product and a set may also carry a `pass` block (see [Gating a suite](#gating-a-suite)), checked as the catalog loads — a bad one raises `Catalog::InvalidDocument` naming the product or set it sits on; `suite_for` makes the product's the suite's and each set's its group's, and -`Catalog#pass_thresholds(:triage, :smoke)` reads them. Keys the format does +`Catalog#pass_thresholds(:triage, :smoke)` reads them. The mounted +dashboard stores and exports both blocks with the catalog. Keys the format does not define (`owner_team`, `repository`, …) are kept as `metadata` on the catalog, product, set or scenario they sit on. Scenario extensions are opaque data: they do not add scoring rules. They remain at their original diff --git a/test/dummy/db/migrate/034_add_catalog_product_criteria.rb b/test/dummy/db/migrate/034_add_catalog_product_criteria.rb new file mode 100644 index 000000000..6776a2726 --- /dev/null +++ b/test/dummy/db/migrate/034_add_catalog_product_criteria.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class AddCatalogProductCriteria < ActiveRecord::Migration[7.2] + def up + return unless table_exists?(products_table) + return if column_exists?(products_table, :criteria) + + type = connection.adapter_name.to_s.downcase.include?("postgres") ? :jsonb : :json + add_column products_table, :criteria, type + end + + def down + return unless table_exists?(products_table) + return unless column_exists?(products_table, :criteria) + + remove_column products_table, :criteria + end + + private + + def products_table + "#{ActionAgent.table_name_prefix}scenario_products" + end +end diff --git a/test/dummy/db/migrate/035_add_catalog_pass_bars.rb b/test/dummy/db/migrate/035_add_catalog_pass_bars.rb new file mode 100644 index 000000000..23cb5ce5c --- /dev/null +++ b/test/dummy/db/migrate/035_add_catalog_pass_bars.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class AddCatalogPassBars < ActiveRecord::Migration[7.2] + def up + type = connection.adapter_name.to_s.downcase.include?("postgres") ? :jsonb : :json + catalog_tables.each do |table| + next unless table_exists?(table) + next if column_exists?(table, :pass) + + add_column table, :pass, type + end + end + + def down + catalog_tables.each do |table| + next unless table_exists?(table) + next unless column_exists?(table, :pass) + + remove_column table, :pass + end + end + + private + + def catalog_tables + %w[scenario_products scenario_sets].map { |table| "#{ActionAgent.table_name_prefix}#{table}" } + end +end diff --git a/test/dummy/db/schema.rb b/test/dummy/db/schema.rb index 99383b9be..f18f70fea 100644 --- a/test/dummy/db/schema.rb +++ b/test/dummy/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 32) do +ActiveRecord::Schema[8.0].define(version: 35) do create_table "active_agent_agent_contexts", force: :cascade do |t| t.string "action_name", null: false t.string "agent_name", null: false @@ -682,6 +682,8 @@ t.json "metadata" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.json "criteria" + t.json "pass" t.index [ "agent_id" ], name: "index_active_agent_scenario_products_on_agent_id" t.index [ "project_id" ], name: "index_active_agent_scenario_products_on_project_id" t.index [ "scenario_catalog_id", "key" ], name: "idx_on_scenario_catalog_id_key_2062a9535b", unique: true @@ -699,6 +701,7 @@ t.integer "position" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.json "pass" t.index [ "evaluation_id" ], name: "index_active_agent_scenario_sets_on_evaluation_id" t.index [ "scenario_product_id", "key" ], name: "idx_on_scenario_product_id_key_dec134b9c6", unique: true end