feat: resolve eval question text for metrics referencing evals in experiments analyze#45834
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds eval-backed metric question resolution to experiment analysis output.
Changes:
- Exposes workflow helpers for parsing eval references and configurations.
- Propagates eval definitions through local and remote analysis.
- Adds metric fields, rendering, and resolution tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/evals_config.go |
Exposes eval frontmatter parsing. |
pkg/workflow/compiler_experiments.go |
Exports metric-reference parsing. |
pkg/workflow/compiler_experiments_test.go |
Updates parser tests. |
pkg/cli/experiments_command.go |
Loads and propagates eval configuration. |
pkg/cli/experiments_analyze_statistics.go |
Resolves and displays metric questions. |
pkg/cli/experiments_analyze_statistics_test.go |
Adds metric-resolution coverage. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Medium
| // Populate metric and resolve any eval reference to its question text. | ||
| if cfg.Metric != "" { | ||
| a.Metric = cfg.Metric | ||
| evalID, isEval := workflow.ParseExperimentMetricEvalReference(cfg.Metric) | ||
| if isEval && evalID != "" && evals != nil { |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are clean and well-structured. The refactoring correctly threads *workflow.EvalsConfig through the call chain, the new experimentFrontmatterResult struct is a clean bundling, and the test coverage for all edge cases (nil evals, unknown ID, plain metric, both prefix forms) is thorough. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 25 AIC · ⌖ 7.35 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (5 tests)
✅ AnalysisStrengths:
Test Inflation Note: Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
…l references in experiments analyze
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (209 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Non-blocking observations only — the core feature logic (eval question resolution, struct plumbing, test coverage) is correct and well-tested.
Review themes
Asymmetry between local and remote frontmatter loaders
loadLocalExperimentConfigs always parses evals after a successful frontmatter parse; loadRemoteExperimentConfigs gates evals parsing inside if len(cfg.ExperimentConfigs) > 0. Today this is harmless because the two always co-exist, but the structural divergence is a latent correctness issue and a maintainability trap.
Zero-value Compiler receiver in ParseEvalsFromFrontmatter
Using var c Compiler to call a method on a ~30-field struct is fragile. The method works now but there's no guard against future extensions accessing Compiler fields. The function should either be a standalone free function or document the constraint explicitly.
🔎 Code quality review by PR Code Quality Reviewer · 71.9 AIC · ⌖ 4.67 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/experiments_command.go:425
Evals silently dropped when remote workflow has no experiment configs: the guard if len(cfg.ExperimentConfigs) > 0 means evals are only parsed for the candidate that also carries experiment configs. If a workflow file declares evals: but cfg.ExperimentConfigs is empty (unexpected parse result, future structural change, etc.), evals are silently discarded with no log message. This diverges from loadLocalExperimentConfigs, which always parses evals after a successful frontmatter parse…
pkg/workflow/evals_config.go:602
ParseEvalsFromFrontmatter relies on a zero-value Compiler receiver: the implementation constructs var c Compiler and delegates to the method receiver. This works today because parseEvalsFromFrontmatter doesn't access any Compiler fields, but there is no compiler-level contract enforcing that. The Compiler struct has ~30 fields; if parseEvalsFromFrontmatter is ever extended to consult any of them (e.g. c.verbose, c.ctx), this wrapper will silently use zero-values, producing…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — one actionable finding; otherwise clean implementation.
📋 Key Themes & Highlights
Finding
- Asymmetric evals parsing in remote path:
loadRemoteExperimentConfigsonly callsParseEvalsFromFrontmatterinside theif len(cfg.ExperimentConfigs) > 0guard, whileloadLocalExperimentConfigsalways parses evals. This means a remote workflow that declares evals without experiment configs will silently lose its evals data.
Positive Highlights
- ✅ 5 well-named, well-structured test cases covering all branches (plain metric, evals prefix, eval: prefix, unknown ID, nil evals)
- ✅
experimentFrontmatterResultstruct cleanly encapsulates the new coupling between experiment configs and evals - ✅ All error paths in evals parsing are logged and non-fatal — keeps the command resilient
- ✅
ParseEvalsFromFrontmatterpublic wrapper is minimal and idiomatic; zero-valueCompileris safe to use - ✅
Metric/MetricQuestionJSON fields useomitempty— no schema breakage for existing consumers
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 32.6 AIC · ⌖ 4.51 AIC · ⊞ 6.7K
Comment /matt to run again
| @@ -396,12 +414,20 @@ func loadRemoteExperimentConfigs(repoOverride, experimentName string) map[string | |||
|
|
|||
| if len(cfg.ExperimentConfigs) > 0 { | |||
There was a problem hiding this comment.
[/codebase-design] loadRemoteExperimentConfigs only parses evals when len(cfg.ExperimentConfigs) > 0, while loadLocalExperimentConfigs always parses evals — a silent asymmetry. If a workflow declares evals without any experimentConfigs, the remote path silently drops the evals.
💡 Suggested fix
Parse evals unconditionally before the guard, mirroring the local path:
evals, evalsErr := workflow.ParseEvalsFromFrontmatter(result.Frontmatter)
if evalsErr != nil {
experimentsLog.Printf("Failed to parse evals config from %s: %v", apiPath, evalsErr)
}
if len(cfg.ExperimentConfigs) > 0 {
experimentsLog.Printf("Loaded remote configs from %s", apiPath)
return experimentFrontmatterResult{
ExperimentConfigs: cfg.ExperimentConfigs,
Evals: evals,
}
}This keeps both code paths symmetric and avoids a surprising gap.
@copilot please address this.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 5b58112: experiment analysis now preserves metric metadata for single-variant state, remote frontmatter parsing now reads evals unconditionally, and the draft ADR is completed at |
🤖 PR Triage
Score breakdown: Impact 35 + Urgency 18 + Quality 15 Exports Run §29462479491
|
|
Hey The PR is focused, includes tests in both
|
|
@copilot please run the
|
…ments-command Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed on |
|
\n@copilot please run the skill, address the unresolved review feedback (newest first: github-actions: #45834 (comment); copilot-pull-request-reviewer: #45834 (comment)), and rerun checks once the branch is up to date.\n\n> Generated by \ud83c\udf73 PR Sous Chef
|
|
🎉 This pull request is included in a new release. Release: |
The
experiments analyzecommand loadedExperimentConfig.Metricfrom workflow frontmatter but never surfaced it — silently dropping eval references likemetric: "evals.builds"that point to declared eval question IDs.Changes
pkg/workflow/ParseExperimentMetricEvalReference(was package-private)ParseEvalsFromFrontmatter— public wrapper to extractEvalsConfigwithout a compiler instancepkg/cli/experiments_command.goexperimentFrontmatterResultstruct carrying bothExperimentConfigsandEvalsConfigloadLocalExperimentConfigs/loadRemoteExperimentConfigsto also parse and return evals (best-effort; logged, non-fatal)EvalsConfigthrough tocomputeExperimentAnalysespkg/cli/experiments_analyze_statistics.gometricandmetric_questionfields toExperimentAnalysiscomputeExperimentAnalysis:evals.builds→ question text from declared evalsTests
evalsparameterevals.resolution,eval:resolution, unknown ID, nil evals