From 3e0a1bb44e61edbb618bacd6d2d6f316e488415d Mon Sep 17 00:00:00 2001 From: mohessie Date: Thu, 20 Aug 2026 20:24:12 +0300 Subject: [PATCH] Publish an evaluator that carries its own rubric Both publish loops selected on 'source' alone, and validation guarantees a rubric written under 'definition' comes with no source. So a ref to a rubric decoded, validated, reported nothing and published nothing, and the eval was then created against an evaluator the service had never been told about. The whole feature was inert. Every test for it stopped at decoding, which is why none of them noticed. A test now asserts a carried rubric reaches the publish set. Also: the rescue is gated on the document actually using a ref, structurally, on both routes. The CLI's no-ref fast path skipped nesting while deploy still applied it, so a hand-written entry carrying rubric keys deployed and was then refused by every command that read it -- the same asymmetry a third time. The gate is structural rather than a byte scan, so a ref mentioned in a comment no longer changes an unrelated entry's meaning. The one-resolver guard now scans the whole extension; it walked only its own package, which is not where a second caller would appear. --- .../internal/cmd/eval_group.go | 8 +- .../internal/project/eval_config.go | 7 +- .../internal/project/eval_config_store.go | 40 ++++++- .../internal/project/one_resolver_test.go | 5 +- .../internal/project/owned_evaluators_test.go | 104 ++++++++++++++++++ .../internal/project/ref_resolution_test.go | 8 +- .../internal/project/service_target_eval.go | 6 +- 7 files changed, 168 insertions(+), 10 deletions(-) create mode 100644 cli/azd/extensions/azure.ai.evaluations/internal/project/owned_evaluators_test.go diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go b/cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go index 8298b0801ff..e255a4dfa25 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go @@ -118,10 +118,14 @@ func newEvalCreateCommand() *cobra.Command { for _, ref := range eval.Evaluators { decl, ok := cfg.EvaluatorDeclaration(ref.Evaluator) // A built-in, or one already registered, has nothing local to publish. - if !ok || decl.Source == "" { + if !ok || (decl.Source == "" && decl.Definition == nil) { continue } - local := project.ResolveSource(baseDir, decl.Source) + // A rubric written out in the configuration has no file to read. + local := "" + if decl.Source != "" { + local = project.ResolveSource(baseDir, decl.Source) + } version, changed, err := reconciler.EnsureEvaluator(ctx, *decl, local) if err != nil { return messages.EvaluatorProblem(decl.Name, err) diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go index 4e46395be50..efc12749a92 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go @@ -207,12 +207,13 @@ func (c *EvalConfig) EvaluatorDeclaration(name string) (*EvaluatorDecl, bool) { return nil, false } -// CustomEvaluators are the catalog entries this configuration owns — the ones -// carrying a local source, published before the evals that name them. +// CustomEvaluators are the catalog entries this configuration owns -- the ones +// carrying a rubric, either as a local source or written out under +// `definition`, published before the evals that name them. func (c *EvalConfig) CustomEvaluators() []EvaluatorDecl { var owned []EvaluatorDecl for _, decl := range c.Evaluators { - if decl.Source == "" { + if decl.Source == "" && decl.Definition == nil { continue } owned = append(owned, decl) diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go index 258f9077de0..89f658caa4b 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go @@ -236,6 +236,13 @@ func resolveConfigRefs(data []byte, baseDir, name string) ([]byte, error) { // CLI command refused, and later the reverse. Callers differ in how they obtain // the map and what they do with it; everything between is here. func resolveEvalRefs(values map[string]any, baseDir string) (map[string]any, error) { + // Read before resolution, which removes the directive. Both routes gate the + // rescue on it so they cannot disagree: without it, the CLI's no-`$ref` fast + // path would skip nesting while the deploy path still applied it, and a + // hand-written entry carrying rubric keys would deploy and then be refused + // by every command that reads it. + spliced := containsRefDirective(values) + resolved, err := foundry.ResolveFileRefs(values, baseDir) if err != nil { return nil, messages.ResolvingServiceRefs(err) @@ -243,10 +250,41 @@ func resolveEvalRefs(values map[string]any, baseDir string) (map[string]any, err // `$ref` is a directive rather than configuration, and the strict decoder // would report the leftover as a mistyped key. delete(resolved, "$ref") - nestSplicedRubrics(resolved) + if spliced { + nestSplicedRubrics(resolved) + } return resolved, nil } +// containsRefDirective reports whether the document uses `$ref` anywhere. +// +// Structural rather than a text scan: the byte "$ref" also appears in comments +// and in prose values, and letting those decide whether an unrelated entry is +// rescued would make one entry's meaning depend on another's wording. +func containsRefDirective(value any) bool { + switch typed := value.(type) { + case map[string]any: + if _, ok := typed[refDirective]; ok { + return true + } + for _, child := range typed { + if containsRefDirective(child) { + return true + } + } + case []any: + for _, child := range typed { + if containsRefDirective(child) { + return true + } + } + } + return false +} + +// refDirective is the include key azd core owns. +const refDirective = "$ref" + // evaluatorDeclKeys are the keys an evaluator entry declares in its own right. // Anything else at entry level was spliced in by a `$ref`. var evaluatorDeclKeys = map[string]bool{ diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/one_resolver_test.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/one_resolver_test.go index d2c30404750..a9286cd9a39 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/project/one_resolver_test.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/one_resolver_test.go @@ -24,7 +24,10 @@ func TestRefsAreResolvedInOnePlace(t *testing.T) { const resolver = "resolveEvalRefs" callers := map[string][]int{} - require.NoError(t, filepath.WalkDir(".", func(path string, d os.DirEntry, err error) error { + // The whole extension, not this package: both current routes already live + // here, so the plausible place for a second caller is internal/cmd, where a + // command wanting resolution would reach for the helper directly. + require.NoError(t, filepath.WalkDir("../..", func(path string, d os.DirEntry, err error) error { if err != nil || d.IsDir() || !strings.HasSuffix(path, ".go") { return err } diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/owned_evaluators_test.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/owned_evaluators_test.go new file mode 100644 index 00000000000..28e6494ffbd --- /dev/null +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/owned_evaluators_test.go @@ -0,0 +1,104 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package project + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// An evaluator carrying its rubric is one this configuration owns, so it has to +// reach the publish loops. +// +// Both loops selected on `source` alone, and validation guarantees a rubric +// written under `definition` comes with no source. So a `$ref` to a rubric +// decoded, validated, reported nothing, and published nothing: the eval was then +// created against an evaluator the service had never been told about. Every test +// for that feature stopped at decoding, which is why none of them noticed. +func TestAnEvaluatorCarryingItsRubricIsPublished(t *testing.T) { + dir := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(dir, "evaluators"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(dir, "evaluators", "quality.json"), + []byte(`{"type":"rubric","dimensions":[{"id":"tone","weight":3}]}`), + 0o600)) + + path := filepath.Join(dir, EvalConfigBase) + require.NoError(t, os.WriteFile(path, []byte(` +evaluators: + - $ref: ./evaluators/quality.json + name: quality + - name: from-a-file + source: ./evaluators/quality.json + - name: builtin.relevance-is-not-ours + version: "3" + +evals: + - name: nightly + dataset: golden +`), 0o600)) + + cfg, err := LoadEvalConfig(path) + require.NoError(t, err) + + var owned []string + for _, decl := range cfg.CustomEvaluators() { + owned = append(owned, decl.Name) + } + + assert.Contains(t, owned, "quality", + "a rubric this configuration carries is one it owns, so it must be published") + assert.Contains(t, owned, "from-a-file") + assert.NotContains(t, owned, "builtin.relevance-is-not-ours", + "an entry that only pins a registered version has nothing to publish") +} + +// The rescue is gated on the document actually using `$ref`, and both routes +// have to gate on it the same way. +// +// The CLI returns a configuration with no `$ref` untouched so the decoder keeps +// its own line numbers. That fast path skipped the rescue while the deploy route +// still applied it, so a hand-written entry carrying rubric keys deployed and was +// then refused by every command that read it -- the same asymmetry twice over. +func TestRubricKeysWithoutARefAreRefusedOnBothRoutes(t *testing.T) { + dir := t.TempDir() + + body := ` +evaluators: + - name: quality + type: rubric + dimensions: + - id: tone + weight: 3 + +evals: + - name: nightly + dataset: golden +` + path := filepath.Join(dir, EvalConfigBase) + require.NoError(t, os.WriteFile(path, []byte(body), 0o600)) + + _, fromDisk := LoadEvalConfig(path) + require.Error(t, fromDisk, "rubric keys nobody spliced are a mistake, not a rubric") + assert.Contains(t, fromDisk.Error(), "dimensions") + + svc := serviceWith(t, map[string]any{ + "evaluators": []any{map[string]any{ + "name": "quality", + "type": "rubric", + "dimensions": []any{ + map[string]any{"id": "tone", "weight": 3}, + }, + }}, + "evals": []any{map[string]any{"name": "nightly", "dataset": "golden"}}, + }) + _, fromService := EvalConfigFromService(svc, dir) + require.Error(t, fromService, "`azd up` has to refuse what every command refuses") + assert.Contains(t, fromService.Error(), "dimensions") +} diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/ref_resolution_test.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/ref_resolution_test.go index 71e0d2978c1..71c372f124c 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/project/ref_resolution_test.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/ref_resolution_test.go @@ -168,8 +168,12 @@ evals: assert.Len(t, cfg.Evaluators[0].Definition["dimensions"], 1) } -// The rescue above is scoped to entries written as a `$ref`, so a misspelling -// in a hand-written entry is still an error rather than rubric content. +// A configuration that uses no `$ref` is never rescued, so a misspelling in a +// hand-written entry is reported rather than filed away. +// +// The `dimensions` gate is what separates the rescue from a catch-all, and it is +// exercised in nested_ref_rubric_test.go; this pins the other half, that a +// document nobody spliced into is left strictly alone. func TestAMisspelledEvaluatorKeyIsStillRejected(t *testing.T) { dir := t.TempDir() diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go index 539717d07cc..3e5a8a70ffa 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go @@ -173,7 +173,11 @@ func (p *EvalServiceTargetProvider) Deploy( // ones need no publish. for _, decl := range cfg.CustomEvaluators() { report(progress, messages.ReconcilingEvaluator(decl.Name)) - localPath := ResolveSource(baseDir, decl.Source) + // A rubric written out in the configuration has no file to read. + localPath := "" + if decl.Source != "" { + localPath = ResolveSource(baseDir, decl.Source) + } version, changed, err := reconciler.EnsureEvaluator(ctx, decl, localPath) if err != nil { return nil, messages.EvaluatorProblem(decl.Name, err)