Keep a catalog's pass bars through dashboard import

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

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<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 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

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

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)

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/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

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