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)