Answer the review on the include work

[m7md7sien]

Answers the review on #9668, the slice opened because Copilot's re-review of #7 failed with "Copilot encountered an error and was unable to review this pull request" and #7 merged with that commit unreviewed.

Seven comments came back. Two were artifacts of how the slice was built and are not real. Three are fixed here. Two are pre-existing and out of scope, with evidence below. Three further bugs came from a parallel review of the same delta, and the highest-severity one was not in the review at all.

Fixed

An entry already carrying its rubric is refused by the catalog commands. addEvaluatorToCatalog matched on name and wrote source: into the entry while Definition stayed populated. No \ involved. The next read rejects the whole configuration for declaring the rubric twice, and it does so after the generation job has been billed and the file written. The guard now covers this shape alongside the two include shapes.

The ownership predicate is shared. source == "" && definition == nil existed in two independent copies: CustomEvaluators for azd up, and an inline re-implementation in eval create. Only the first had a test. Reverting the second to source == "" left the suite green and eval create silently inert, which is the bug that shipped once already. Both now call EvaluatorDecl.CarriesItsRubric, and a structural test fails the build if a third copy appears.

The project root is fetched once. It was fetched again for artifact paths after being used to resolve \, so a second failure would have passed the include guard and then resolved artifacts against the extension process's directory.

Schema paths no longer claim to be rebased. file: and source: said "relative to this file", which is wrong for an entry reached through a \ -- the hover instructed authors to write a path deployment cannot find. Both now say what actually happens and steer to definition:. The schema also expresses the two version conflicts the code already rejected.

The modernization gate. go fix -diff ./... flagged the slice walk in containsRefDirective, and lint-ext-azure-ai-evaluations.yml runs that gate against this module, so this would have failed CI on Azure#9500.

Not fixed, with reasons

Mojibake in README.md and messages.go is not real. The files on disk are valid UTF-8 with no replacement characters, and doneMark is the expected check mark. The slice was built by piping git show through PowerShell, which re-encoded the non-ASCII. My error, not the code's. The slice is now built with git checkout so the blobs are byte-identical.

errors.As and sort.Strings are pre-existing and not flagged by the gate. Both predate this work and appear throughout the extension and in cli/azd/pkg (40 and 5 occurrences). go fix -diff rewrites neither. Worth a separate sweep, not this PR.

Tests

Dataset-kind guard coverage in both include shapes, the inline-rubric refusal, and the editable-in-place case that must not be caught. Each new guard was mutation-tested: the bug reintroduced, the test confirmed failing, the fix restored.

[Copilot]

Pull request overview

This pull request improves Azure AI evaluations $ref handling, catalog safeguards, path resolution, schema documentation, and regression coverage.

Changes:

• Centralizes rubric ownership detection and catalog editability checks.
• Preserves include provenance and reuses project-root resolution.
• Updates schemas, documentation, messages, and tests.

Review findings:

• Critical (3 votes): eval_config_store.go incorrectly treats all evaluators from a referenced configuration as evaluator-level includes, potentially publishing invalid entries.
• Moderate (2 votes): catalog.go drops evaluator version pins during editability checks.
• Moderate (2 votes): The schema does not apply version-conflict validation to evaluator-specific $ref shapes.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Summary
cli/azd/extensions/azure.ai.evaluations/schemas/azure.ai.eval.json	Updates path documentation and version constraints; retains an unresolved $ref validation gap.
cli/azd/extensions/azure.ai.evaluations/README.md	Documents rubric include usage and editing behavior.
cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go	Reuses project-root resolution for deployment paths.
cli/azd/extensions/azure.ai.evaluations/internal/project/one_ownership_test.go	Enforces centralized ownership detection.
cli/azd/extensions/azure.ai.evaluations/internal/project/neighbour_ref_test.go	Tests entry-local include behavior.
cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go	Adds shared rubric ownership logic.
cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go	Tracks evaluator include entries; contains an unresolved provenance issue.
cli/azd/extensions/azure.ai.evaluations/internal/messages/messages.go	Adds catalog editability messages.
cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go	Uses shared ownership detection.
cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog.go	Guards catalog updates; version-pin handling remains unresolved.
cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog_include_test.go	Tests catalog include and inline-rubric guards.

Suppressed comments (1)

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

• This guard is reached only after runGenerations has awaited generateRubric; that function has already submitted the billed job and written the artifact before addEvaluatorToCatalog reaches this call. An inline-rubric or include case therefore still incurs the generation cost and leaves an unrecorded file before returning this new error. Move the editability check into the pre-submission planning path, keeping this catalog check only as a backstop.

	if err := checkCatalogEntryIsEditable(evalDir, cfg, kind, ref.Name); err != nil {

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[m7md7sien]

Three comments this round. One fixed, one deferred to a work item with reasoning, one the schema cannot express.

1. catalogEntryShapeOf drops decl.Version — fixed in a17f817b4.

Correct, and the same class as the inline-rubric bug in this PR: a version pin says the rubric is already registered, so writing source: beside it leaves source + version, which Validate rejects on the next read — again after the generation job has been billed and the file written. The guard now refuses a pinned evaluator. Mutation-tested.

Note the asymmetry with datasets is deliberate and now covered by a test: a dataset may hold file: and version: together, because there the version says which version to publish. Only evaluators are exclusive.

2. visible == false rescues the whole evaluator list — deferred, deliberately.

Correct, and it is the limitation I called out when fixing the document-wide gate. When the service entry is itself a $ref to an evaluation configuration, core does not report which resolved node came from which include, so per-entry provenance is not recoverable from outside the resolver. The result is the asymmetry you describe: the same file is stricter opened directly than reached through the service entry.

Not patched here, because every available fix is worse than the bug:

• refusing the rescue when visible is false breaks the layout the README documents;
• re-implementing resolution to keep provenance duplicates core and breaks the single-resolver invariant this design depends on.

The real fix is to stop round-tripping this configuration through typed structs at all. Core already ships the mechanism — foundry.YAMLDocument in includes_edit.go, comment-preserving and $ref-aware, currently with zero callers. Moving the editing path to node level removes the provenance question, removes the three $ref struct fields, and fixes a larger bug on the way: the typed round-trip today deletes every comment in the file and rewrites its indentation.

That is now a tracked work item and is being implemented next.

3. Version rules bypassed by the FileRef branch — accurate, not expressible.

The mechanism is exactly as described: a $ref entry matches FileRef, whose additionalProperties is true, so the EvaluatorDecl conditional never applies.

The suggested rule would over-restrict, though. Whether an overlay version conflicts depends on what the referenced file contains: a $ref at a rubric or at a declaration carrying source: does conflict, but a $ref at a file holding only name: plus an overlay version is a legally pinned evaluator. JSON Schema cannot see the resolved document, so forbidding version on the evaluator ref shape would red-squiggle a configuration that deploys correctly.

Between a schema that misses an invalid shape and one that rejects a valid one, the first is the lesser harm, and the runtime rejects it with a message naming the entry index and the conflict. Leaving the runtime as the authority here.