Keep a catalog product's named criteria lists

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 `<mount>/api/traces`, and the card

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<Hash>}]
+    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

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)

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

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

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,

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
 

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.

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;