Answer the review on the include work

cli/azd/extensions/azure.ai.evaluations/README.md

@@ -60,14 +60,21 @@ That holds for the configuration as a whole. It does **not** hold for a `$ref`
 on a single catalog entry: azd rebases only the path keys it owns, so a relative
 `source:` written inside `evals/evaluators/quality.yaml` still resolves against
 `azure.eval.yaml` and will not be found. An entry pulled in from its own file
-should carry the rubric under `definition:` rather than point at a second file:
+should carry the rubric rather than point at a second file — either written out
+under `definition:`, or as a `$ref` straight at the rubric, whose keys are
+spliced in and become that `definition:`:
 
 ```yaml
 evaluators:
   - $ref: ./evaluators/quality.json    # the rubric itself, not a pointer to one
     name: quality
 ```
 
+An entry declared this way is read and deployed normally, but it lives in the
+referenced file, so `azd ai eval generate` will not update it in place and says
+so rather than writing a second declaration of the same rubric beside the
+directive. Edit the referenced file, or generate under a different name.
+
 ### Repeated deploys do not create redundant versions
 
 Datasets are fingerprinted locally, because the dataset API exposes no content

cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog.go

@@ -66,50 +66,78 @@ func addEvaluatorToCatalog(cmd *cobra.Command, evalDir string, ref *project.Arti
 	})
 }
 
-// checkNameNotBehindAnInclude refuses a name whose entry lives in another file.
+// checkCatalogEntryIsEditable refuses a name this command cannot rewrite in
+// place without corrupting the entry.
 //
-// Two shapes reach this. A pure `$ref` has no name here at all, so the duplicate
-// scan had nothing to match on and appended a second entry with the same name --
-// a collision that surfaced only on the next resolving read. A `$ref` carrying
-// an overlay `name` does match, and updating it in place writes `source:` beside
-// the directive, so resolution then produces a rubric and a source and the
-// configuration is rejected for declaring it twice. Neither is editable here.
+// Four shapes reach this. A pure `$ref` has no name here at all, so the
+// duplicate scan had nothing to match on and appended a second entry with the
+// same name -- a collision that surfaced only on the next resolving read. A
+// `$ref` carrying an overlay `name` does match, and updating it in place writes
+// `source:` beside the directive, so resolution then produces a rubric and a
+// source and the configuration is rejected for declaring it twice. An entry
+// already carrying its rubric under `definition:`, and an entry pinned to a
+// registered `version:`, both fail the same way with no include involved:
+// recording the generated file leaves two rubrics, or a pin and a file, in one
+// entry. Each of those is refused on the next read -- after the generation job
+// has been billed and the file written, which is why they are refused here.
 //
 // A configuration that will not resolve is left to the commands that resolve it:
 // failing a generate over an unrelated broken include would be its own surprise.
-func checkNameNotBehindAnInclude(evalDir string, asWritten *project.EvalConfig, kind, name string) error {
-	if ref, ok := catalogEntryRef(asWritten, kind, name); ok {
-		if ref != "" {
+func checkCatalogEntryIsEditable(evalDir string, asWritten *project.EvalConfig, kind, name string) error {
+	if entry, ok := catalogEntryShapeOf(asWritten, kind, name); ok {
+		switch {
+		case entry.ref != "":
 			return messages.CatalogNameBehindAnInclude(kind, name)
+		case entry.inlineRubric:
+			return messages.EvaluatorRubricWrittenInPlace(name)
+		case entry.pinned:
+			return messages.EvaluatorPinnedToAVersion(name)
 		}
 		return nil
 	}
 	resolved, err := project.OpenEvalConfig(evalDir)
 	if err != nil || resolved == nil {
 		return nil
 	}
-	if _, ok := catalogEntryRef(resolved, kind, name); ok {
+	if _, ok := catalogEntryShapeOf(resolved, kind, name); ok {
 		return messages.CatalogNameBehindAnInclude(kind, name)
 	}
 	return nil
 }
 
-// catalogEntryRef returns the include this entry was written as, and whether the
+// catalogEntryShape is how an entry was written, for deciding whether this
+// command may rewrite it.
+type catalogEntryShape struct {
+	// ref is the `$ref` directive the entry carries, empty when it is written
+	// out here.
+	ref string
+	// inlineRubric is an evaluator holding its rubric under `definition:`.
+	inlineRubric bool
+	// pinned is an evaluator naming a registered `version:`. A dataset may hold
+	// a file and a version together; an evaluator may not.
+	pinned bool
+}
+
+// catalogEntryShapeOf returns how the entry was written, and whether the
 // configuration names it at all.
-func catalogEntryRef(cfg *project.EvalConfig, kind, name string) (string, bool) {
+func catalogEntryShapeOf(cfg *project.EvalConfig, kind, name string) (catalogEntryShape, bool) {
 	if cfg == nil {
-		return "", false
+		return catalogEntryShape{}, false
 	}
 	if kind == "dataset" {
 		if decl, ok := cfg.DatasetDeclaration(name); ok {
-			return decl.Ref, true
+			return catalogEntryShape{ref: decl.Ref}, true
 		}
-		return "", false
+		return catalogEntryShape{}, false
 	}
 	if decl, ok := cfg.EvaluatorDeclaration(name); ok {
-		return decl.Ref, true
+		return catalogEntryShape{
+			ref:          decl.Ref,
+			inlineRubric: decl.Definition != nil,
+			pinned:       decl.Version != "",
+		}, true
 	}
-	return "", false
+	return catalogEntryShape{}, false
 }
 
 // updateCatalog applies a change to the configuration and writes it back.
@@ -142,7 +170,7 @@ func updateCatalog(
 	if created {
 		cfg = &project.EvalConfig{}
 	}
-	if err := checkNameNotBehindAnInclude(evalDir, cfg, kind, ref.Name); err != nil {
+	if err := checkCatalogEntryIsEditable(evalDir, cfg, kind, ref.Name); err != nil {
 		return err
 	}
 	if !apply(cfg) {

cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog_include_test.go

@@ -32,7 +32,7 @@ evaluators:
   - $ref: ./parts/quality.yaml
 `), 0o600))
 
-	err := checkNameNotBehindAnInclude(
+	err := checkCatalogEntryIsEditable(
 		dir, mustOpenForEdit(t, dir), "evaluator", "quality")
 
 	require.Error(t, err, "the name is taken, even though this file does not show it")
@@ -53,7 +53,7 @@ evaluators:
   - $ref: ./parts/quality.yaml
 `), 0o600))
 
-	require.NoError(t, checkNameNotBehindAnInclude(
+	require.NoError(t, checkCatalogEntryIsEditable(
 		dir, mustOpenForEdit(t, dir), "evaluator", "tone"))
 }
 
@@ -77,13 +77,141 @@ evaluators:
     name: quality
 `), 0o600))
 
-	err := checkNameNotBehindAnInclude(
+	err := checkCatalogEntryIsEditable(
 		dir, mustOpenForEdit(t, dir), "evaluator", "quality")
 
 	require.Error(t, err, "the entry is an include, so it cannot be updated in place")
 	assert.Contains(t, err.Error(), "quality")
 }
 
+// The dataset branch of the guard is its own lookup, so it gets its own tests.
+//
+// Every case above is an evaluator, and `catalogEntryShapeOf` dispatches on kind
+// before it looks anything up. A regression in the dataset branch would
+// reintroduce the duplicate entries the guard exists to prevent while the
+// evaluator tests stayed green.
+func TestGenerateRefusesADatasetNameAnIncludeAlreadyDeclares(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(dir, "parts"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "parts", "golden.yaml"),
+		[]byte("name: golden\nfile: ./datasets/golden.jsonl\n"), 0o600))
+
+	require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(`
+datasets:
+  - $ref: ./parts/golden.yaml
+`), 0o600))
+
+	err := checkCatalogEntryIsEditable(
+		dir, mustOpenForEdit(t, dir), "dataset", "golden")
+
+	require.Error(t, err, "the dataset name is taken by the included file")
+	assert.Contains(t, err.Error(), "golden")
+	assert.Contains(t, err.Error(), "dataset", "the message names the kind it refused")
+}
+
+// A dataset include carrying an overlay `name`, the shape the evaluator test
+// above covers, refused through the dataset branch.
+func TestGenerateRefusesADatasetIncludeThatCarriesItsName(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(dir, "parts"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "parts", "golden.yaml"),
+		[]byte("file: ./datasets/golden.jsonl\n"), 0o600))
+
+	require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(`
+datasets:
+  - $ref: ./parts/golden.yaml
+    name: golden
+`), 0o600))
+
+	err := checkCatalogEntryIsEditable(
+		dir, mustOpenForEdit(t, dir), "dataset", "golden")
+
+	require.Error(t, err, "the entry is an include, so it cannot be updated in place")
+	assert.Contains(t, err.Error(), "golden")
+}
+
+// An evaluator already carrying its rubric under `definition:` is refused.
+//
+// No include is involved. Recording a generated file against it writes
+// `source:` into an entry that already holds a `definition:`, and the next read
+// rejects the whole configuration for declaring the rubric twice -- after the
+// generation job has been billed and the file written. Refusing here is what
+// keeps the failure ahead of the cost.
+func TestGenerateRefusesAnEvaluatorThatAlreadyCarriesItsRubric(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(`
+evaluators:
+  - name: quality
+    definition:
+      type: rubric
+      dimensions:
+        - id: tone
+          weight: 3
+`), 0o600))
+
+	err := checkCatalogEntryIsEditable(
+		dir, mustOpenForEdit(t, dir), "evaluator", "quality")
+
+	require.Error(t, err, "there is nowhere to record a file without declaring the rubric twice")
+	assert.Contains(t, err.Error(), "quality")
+	assert.Contains(t, err.Error(), "definition", "the reader has to be told which half is already there")
+}
+
+// An entry written out here, with no include and no inline rubric, stays
+// editable -- the case the guard must not catch.
+func TestGenerateStillUpdatesAnEntryWrittenInThisFile(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(`
+datasets:
+  - name: golden
+    file: ./datasets/golden.jsonl
+evaluators:
+  - name: quality
+    source: ./evaluators/quality.json
+`), 0o600))
+
+	cfg := mustOpenForEdit(t, dir)
+	require.NoError(t, checkCatalogEntryIsEditable(dir, cfg, "evaluator", "quality"))
+	require.NoError(t, checkCatalogEntryIsEditable(dir, cfg, "dataset", "golden"))
+}
+
+// An evaluator pinned to a registered version is refused, for the same reason
+// an inline rubric is: there is nowhere to record the generated file.
+//
+// A pin says the rubric already lives in the service. Writing `source:` beside
+// it leaves the entry claiming both, which the next read rejects -- again after
+// the job has been billed and the file written.
+func TestGenerateRefusesAnEvaluatorPinnedToAVersion(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(`
+evaluators:
+  - name: quality
+    version: "3"
+`), 0o600))
+
+	err := checkCatalogEntryIsEditable(
+		dir, mustOpenForEdit(t, dir), "evaluator", "quality")
+
+	require.Error(t, err, "a pin and a file in one entry is refused on the next read")
+	assert.Contains(t, err.Error(), "quality")
+	assert.Contains(t, err.Error(), "version", "the reader has to be told what is already there")
+}
+
+// A dataset may carry a file and a version together -- the version says which
+// one to publish -- so the pin must not make it uneditable.
+func TestGenerateStillUpdatesAVersionedDataset(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(`
+datasets:
+  - name: golden
+    file: ./datasets/golden.jsonl
+    version: "4"
+`), 0o600))
+
+	require.NoError(t, checkCatalogEntryIsEditable(
+		dir, mustOpenForEdit(t, dir), "dataset", "golden"))
+}
+
 func mustOpenForEdit(t *testing.T, dir string) *project.EvalConfig {
 	t.Helper()
 	cfg, err := project.OpenEvalConfigForEdit(dir)

cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go

@@ -118,7 +118,7 @@ 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 == "" && decl.Definition == nil) {
+				if !ok || !decl.CarriesItsRubric() {
 					continue
 				}
 				// A rubric written out in the configuration has no file to read.

cli/azd/extensions/azure.ai.evaluations/internal/messages/messages.go

@@ -1432,15 +1432,43 @@ func RefNeedsAProjectRoot(service string) error {
 
 // CatalogNameBehindAnInclude reports a name declared through a `$ref`, which
 // this command cannot edit in place.
+//
+// One message for two shapes, so the cause is stated as what they share: the
+// entry lives in the referenced file. Naming only the duplicate-on-resolve case
+// would misdescribe an overlay `name`, which collides with nothing and instead
+// ends up declaring the rubric twice.
 func CatalogNameBehindAnInclude(kind, name string) error {
 	return fmt.Errorf(
-		"%s %q is already declared through a `$ref`, so this command cannot update "+
-			"it here: adding a second entry would collide with the first only after "+
-			"the include is resolved. Edit the referenced file, or generate under a "+
-			"different name",
+		"%s %q is declared in a file pulled in with `$ref`, so this command cannot "+
+			"update it here: an entry written beside the directive takes effect only "+
+			"once the include is resolved, and not as it reads. Edit the referenced "+
+			"file, or generate under a different name",
 		kind, name)
 }
 
+// EvaluatorRubricWrittenInPlace reports an evaluator whose rubric is already
+// written out under `definition:`, so there is nowhere to record a generated
+// file without declaring the rubric twice.
+func EvaluatorRubricWrittenInPlace(name string) error {
+	return fmt.Errorf(
+		"evaluator %q already carries its rubric under `definition:`, so this "+
+			"command cannot record a generated file against it: an entry holding "+
+			"both a `definition:` and a `source:` is refused on the next read. Edit "+
+			"the rubric in place, or generate under a different name",
+		name)
+}
+
+// EvaluatorPinnedToAVersion reports an evaluator pinned to a registered
+// version, which leaves nowhere to record a generated file.
+func EvaluatorPinnedToAVersion(name string) error {
+	return fmt.Errorf(
+		"evaluator %q is pinned to a registered `version:`, so this command cannot "+
+			"record a generated file against it: an entry holding both a `version:` "+
+			"and a `source:` is refused on the next read. Remove the pin to publish "+
+			"from a file, or generate under a different name",
+		name)
+}
+
 // ReadingServiceConfig reports the service entry failing to serialize.
 func ReadingServiceConfig(err error) error {
 	return fmt.Errorf("reading the eval service configuration: %w", err)

cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go

@@ -215,13 +215,27 @@ func (c *EvalConfig) EvaluatorDeclaration(name string) (*EvaluatorDecl, bool) {
 	return nil, false
 }
 
+// CarriesItsRubric reports whether this configuration owns the evaluator and
+// has to publish it, rather than referring to a built-in or to one already
+// registered under this name.
+//
+// Both fields have to be tested, and this is the only place that should test
+// them. Validation forbids declaring the rubric twice, so `definition` implies
+// an empty `source`: a selector written as `source == ""` reads as "nothing
+// local to publish" but silently drops every evaluator carrying its rubric
+// inline. That shipped once already -- the eval was created bound to an
+// evaluator the service had never been told about.
+func (d EvaluatorDecl) CarriesItsRubric() bool {
+	return d.Source != "" || d.Definition != nil
+}
+
 // 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 == "" && decl.Definition == nil {
+		if !decl.CarriesItsRubric() {
 			continue
 		}
 		owned = append(owned, decl)