diff --git a/CHANGELOG.md b/CHANGELOG.md index bee258049..95a6c94e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **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..8f25e6e32 100644 --- a/actionagent/app/models/action_agent/scenario_product.rb +++ b/actionagent/app/models/action_agent/scenario_product.rb @@ -21,6 +21,20 @@ 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 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 +50,7 @@ def to_document "name" => name, "description" => description, "agent" => agent_name.presence || agent&.name, + "criteria" => criteria.presence, "sets" => sets.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..dad55e147 100644 --- a/actionagent/app/services/action_agent/scenario_catalog_import.rb +++ b/actionagent/app/services/action_agent/scenario_catalog_import.rb @@ -181,6 +181,8 @@ 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) product.position = position_for(product, entry.position, offset) product.save! replace_sets(product, 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/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..44b470160 100644 --- a/actionagent/test/install_generator_test.rb +++ b/actionagent/test/install_generator_test.rb @@ -551,6 +551,59 @@ 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 + 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..d36969262 100644 --- a/actionagent/test/scenario_catalogs_api_test.rb +++ b/actionagent/test/scenario_catalogs_api_test.rb @@ -89,6 +89,39 @@ 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 "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..796224299 100644 --- a/docs/framework/dashboard.md +++ b/docs/framework/dashboard.md @@ -891,6 +891,18 @@ 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. + **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..e1c15021b 100644 --- a/docs/framework/evaluations.md +++ b/docs/framework/evaluations.md @@ -1390,10 +1390,10 @@ 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; 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/schema.rb b/test/dummy/db/schema.rb index 99383b9be..d45d27c51 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: 34) 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,7 @@ t.json "metadata" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.json "criteria" 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