Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,17 +236,55 @@ 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)
}
// `$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{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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")
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading