[TEMP - do not merge] review slice: model the include on every entry core can splice - #9668
Conversation
|
Azure Pipelines: 7 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Review-only slice aligning $ref handling across evaluation configuration entries and catalog edits.
Changes:
- Models
$refon datasets, evaluators, and evals. - Prevents catalog commands from modifying included entries.
- Adds strict resolution diagnostics and regression tests.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
README.md |
Documents evaluation configuration and includes. |
schemas/azure.ai.eval.json |
Defines $ref-capable entry schemas. |
internal/cmd/catalog.go |
Guards included catalog entries. |
internal/cmd/catalog_include_test.go |
Tests evaluator include guards. |
internal/messages/messages.go |
Adds diagnostics and CLI messages. |
internal/project/config_keys_test.go |
Pins configuration keys. |
internal/project/eval_config.go |
Models $ref fields. |
internal/project/ref_on_every_entry_test.go |
Tests editable entry includes. |
internal/project/ref_resolution_test.go |
Tests include resolution behavior. |
internal/project/service_config_strict_test.go |
Tests strict service decoding. |
internal/project/service_target_eval.go |
Resolves deploy configuration and paths. |
Suppressed comments (1)
cli/azd/extensions/azure.ai.evaluations/schemas/azure.ai.eval.json:83
- This has the same nested-include mismatch as
file:: a relativesource:inside an entry-level$refresolves againstazure.eval.yaml, not the referenced file. Update the schema text so editor guidance matches the README and runtime diagnostic. [azd-code-reviewer]
"description": "Path to a local .json rubric -- a list of weighted scoring dimensions -- relative to this file. Published on deploy and fingerprinted locally so a later deploy can tell an edit here from a version published elsewhere."
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| }, | ||
| "file": { | ||
| "type": "string", | ||
| "description": "Path to a local .jsonl whose rows are uploaded on deploy, relative to this file. Deliberately not a $ref: the rows are a data artifact to publish, not a definition to splice in, and a .jsonl is neither YAML nor JSON. Omit to use a dataset already registered under this name." |
| cfg, err := EvalConfigFromService(serviceConfig, p.projectRoot(ctx)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := cfg.Validate(); err != nil { | ||
| return nil, messages.EvalConfigInvalid(err) | ||
| } | ||
|
|
||
| reconciler, err := p.newReconciler(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| baseDir := p.evalBaseDir(ctx, serviceConfig) |
| name: support-agent | ||
| ``` | ||
|
|
||
| `azd up` reconciles **datasets → evaluators → eval groups**, in that order, |
| // Progress markers from the azd style guide, so the extension's lines sit | ||
| // alongside core's without a second vocabulary. | ||
| const ( | ||
| doneMark = "(Γ£ô) Done:" // finished successfully |
| var local *azdext.LocalError | ||
| if errors.As(err, &local) { |
| for _, value := range values { | ||
| quoted = append(quoted, fmt.Sprintf("%q", value)) | ||
| } | ||
| sort.Strings(quoted) |
| if kind == "dataset" { | ||
| if decl, ok := cfg.DatasetDeclaration(name); ok { | ||
| return decl.Ref, true |
|
Thanks -- triaged all seven. Five were actionable, two were my own fault. Fixes are pushed to this branch ( This slice is still not for merging. It exists only because Copilot's re-review of the fork PR failed with "Copilot encountered an error and was unable to review this pull request", so the commit that answered the previous review merged unreviewed. Please don't spend maintainer time here.
On 3 and 4 -- the encoding findings were an artifact of this sliceThe files are clean. Verified at byte level: valid UTF-8, no BOM, zero U+FFFD, and The corruption was introduced when I built this slice by piping Fixed at the source: every file in this branch is now copied with On 5 and 6 -- checked rather than dismissedBoth predate this work: The cited That check did find one real thing the review missed, in code this PR introduces: Three further bugs, from a parallel review of the same deltaNot in the review above; the highest-severity one is the kind this feature keeps producing.
Each new guard was mutation-tested: bug reintroduced, test confirmed failing, fix restored. A test that cannot fail is worse than none, and this feature's earlier tests all stopped at decoding, which is why the inert-publish bug survived them. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go:89
- azd-code-reviewer: The resolver accepts
$refon any object (pkg/foundry/includes.go:84-89), but the edit model only adds it to the outer eval. A config with$refundersource,target, or anevaluators[]item deploys after resolution, whileOpenEvalConfigForEditstrictly rejects the same file because those nested types do not model$ref. Model these nested include shapes too, or reject them consistently before deploy; otherwise the stated deploy/edit symmetry remains incomplete.
Source *SourceDecl `yaml:"source,omitempty" json:"source,omitempty"`
EvaluationLevel string `yaml:"evaluation_level,omitempty" json:"evaluation_level,omitempty"`
MaxSamples int `yaml:"max_samples,omitempty" json:"max_samples,omitempty"`
Evaluators evalcore.EvaluatorList `yaml:"evaluators,omitempty" json:"evaluators,omitempty"`
Target *Target `yaml:"target,omitempty" json:"target,omitempty"`
cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go:494
- azd-code-reviewer: Use
errors.AsTypehere instead of the legacy target-variable pattern, as required by the repository's Go 1.26 guidance (cli/azd/AGENTS.md:369-385) and modernization check.
var errno syscall.Errno
if !errors.As(err, &errno) {
return false
}
| if spliced { | ||
| nestSplicedRubrics(resolved) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go:255
- [azd-code-reviewer]
splicedis document-wide, so any$ref—including the newly supported dataset/eval refs—causes every evaluator with a top-leveldimensionskey to be rewritten underdefinition. A hand-written invalid evaluator can therefore be silently accepted and published merely because an unrelated entry is included. Track which evaluator nodes were actually resolved from refs and only nest those nodes.
if spliced {
nestSplicedRubrics(resolved)
cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go:80
- [azd-code-reviewer] Modeling
$refonly on the outer eval still leaves deployable configurations that editing commands cannot open.ResolveFileRefsrecursively resolves any mapping, so asource:ortarget:containing$refworks on the resolved use/deploy path, whileOpenEvalConfigForEditstrict-decodes the raw object and rejects that key becauseSourceDeclandTargetdo not model it. Either model nested includes in these shapes and the schema, or reject them consistently before deploy.
Ref string `yaml:"$ref,omitempty" json:"$ref,omitempty"`
|
Round two: 1 comment plus 2 suppressed. The encoding findings are gone, which confirms they were the slice artifact and not the code. The main comment was right, and it is the best catch of either round
Reproduced before fixing. The same evaluator, byte for byte: # refused -- correct
datasets:
- name: golden
file: ./datasets/golden.jsonl
evaluators:
- name: quality
dimensions: [{ id: tone, weight: 3 }]# silently accepted -- the dataset's directive says nothing about this evaluator
datasets:
- $ref: ./parts/golden.yaml
evaluators:
- name: quality
dimensions: [{ id: tone, weight: 3 }]The first returned That is precisely what the comment above Fixed in One honest limit. When the configuration is itself behind a Suppressed 1 -- correct, but it is a spec change, not a code fixVerified all three shapes empirically. Each deploys and is then refused by the editing read:
So the comment is right that the symmetry is incomplete. I implemented it, and the change is deliberately not included. Modelling the directive on
The directive is on exactly three shapes by design, and those tests exist to stop a fourth appearing without the spec saying so. It also needs validation changes -- a reference carrying only a directive currently fails Two ways forward, and it is a product decision rather than a review fix:
Reverted and raised with the spec owner. The asymmetry is real, and I would rather it be decided than have it land quietly. Suppressed 2 --
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go:341
- [azd-code-reviewer] With the normal service-level
$ref,splicedEvaluatorsreturnsvisible=falsebeforeazure.eval.yamlis loaded, so this condition treats every resolved evaluator containingdimensionsas though that entry had its own$ref. As a result, an inline typo such asdimensions:inazure.eval.yamlis silently nested and deployed byazd up, whileLoadEvalConfigrejects the same file. I think we need to preserve entry-level$refprovenance through the outer include instead of rescuing every evaluator whenvisibleis false.
if visible && !spliced[i] {
cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go:224
- [azd-code-reviewer]
Project().Getfailures are collapsed into an empty root. The new$refguard catches includes, but an inline config with a relative datasetfile:or evaluatorsource:still reachesbaseDirUnder, which resolves it against the extension process directory. That can read the wrong same-named file or report an existing artifact as missing. I think we need to propagate the RPC error, or reject relative artifact paths whenever the project root is unavailable.
resp, err := p.azdClient.Project().Get(ctx, &azdext.EmptyRequest{})
if err != nil || resp.GetProject() == nil {
return ""
}
return resp.GetProject().GetPath()
cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go:217
- [azd-code-reviewer] This byte scan also matches
$refin comments and ordinary string values. Those files are unnecessarily unmarshaled and re-marshaled, so strict-decoder diagnostics can point at reordered generated lines instead of the lines the user authored. After parsing, return the original bytes unless the structure actually contains a$refdirective.
This issue also appears on line 341 of the same file.
if !bytes.Contains(data, []byte("$ref")) {
cli/azd/extensions/azure.ai.evaluations/README.md:116
- [azd-code-reviewer] The example is a JSON object containing a
dimensionslist, not a JSON list itself. Calling the rubric a list can lead users to omit the required object wrapper.
A custom rubric is a JSON list of weighted dimensions:
|
Closing: this was a review-only slice, never for merging. Its purpose is served -- both rounds of findings are triaged and the real fixes are merged into the branch behind #9500. Remaining known gaps are written up in |
Review-only slice. Will be closed without merging. Please do not spend maintainer time here.
The real change already merged into
feat/azure-ai-evaluations-consolidatedas #7 on thefork, which feeds #9500. It is here only because Copilot's re-review of that PR failed
with "Copilot encountered an error and was unable to review this pull request", so the
commit that answered its first review was merged without ever being reviewed.
This carries the 11 touched files at that merged state, so the diff is small enough to
review. It will not build on its own -- the rest of the extension is not here.
What to look at
internal/cmd/catalog.go-- refuses a catalog name whose entry lives in another file,both when the entry is a pure
\(no name visible) and when it carries an overlayname. The second case is the one the earlier review caught: updating it in placewrites
source:beside the directive, and resolution then yields both a spliced rubricand a source, which the next read rejects as declaring the rubric twice.
internal/project/eval_config.goandschemas/azure.ai.eval.json--\is nowmodelled on dataset and eval entries, not just evaluators, so a config that deploys can
also be opened by the commands that edit it.
internal/project/service_target_eval.go-- an include reached without a project rootis refused rather than discarded.
internal/messages/messages.go-- a relativesource:/file:inside a\'dentry resolves against the configuration, not that file. Not fixed by design; the
diagnostics now say so.