feat(eval,cli): describe metrics in prose and restructure eval output - #3453
Merged
Merged
Conversation
`veryfront eval` printed one line per metric keyed by factory name, so `agent.calledTool` never said which tool the eval asserted on. That parameter lives in the config the metric factory captured, so the label is built there rather than reconstructed in the CLI. - `src/eval/metric-labels.ts` maps a metric name plus its config to prose for all built-in metrics, and returns undefined for unknown names so callers fall back to the raw name instead of guessing. - `createMetric` is the single choke point every metric funnels through, so the label attaches there and rides on the existing but unused `EvalMetricResult.label` field into `EvalMetricSummary`. The JSON report carries it too, not just the terminal. - Metrics sharing a summary row disagree on their label when they assert on different tools. The label is dropped in that case rather than crediting the row to whichever ran first. The summary key is left alone because baseline.ts derives the same key independently, so splitting rows would silently drop data in baseline comparisons. Output is restructured to match: header lines are column-aligned and glyph-free, `●` now marks metric assertions and nothing else, and artifact paths are dimmed. `Report directory` and `Report markdown` collapse into one `Report:`. The `--report` path is relabeled `Report JSON:` because that flag writes JSON, not markdown, and previously shared the `Report:` label with the markdown line.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughEval metrics now derive human-readable labels, preserve them through report aggregation, and render them in revised single, suite, and comparison CLI output. Tests cover label formatting, aggregation conflicts, report names, and artifact paths. ChangesEval reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EvalRunner
participant MetricFormatter
participant EvalReport
participant EvalCLI
EvalRunner->>MetricFormatter: formatEvalMetricLabel(name, config)
MetricFormatter-->>EvalRunner: human-readable label
EvalRunner->>EvalReport: metric result with label
EvalReport-->>EvalCLI: summary rows and artifact details
EvalCLI-->>EvalCLI: print formatted eval output
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
kwakayama
enabled auto-merge
August 7, 2026 10:50
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/commands/eval/command.ts`:
- Around line 808-810: Update printLine and its callers in the suite,
single-eval, and model-comparison paths to respect the active --quiet mode by
routing output through the established quiet-aware helper or suppressing
rendering when quiet is enabled. Preserve normal human-readable output
otherwise, and add coverage verifying veryfront eval --quiet emits no report
output.
In `@src/eval/metric-labels.ts`:
- Around line 61-73: Update the metric label cases in the label-rendering
function, specifically agent.calledTool, agent.notCalledTool, and
agent.toolCallCount, to pass configured tool names through elide() before
interpolation. Add a focused test using a long tool name that verifies the
rendered labels remain elided and do not wrap CLI output.
- Around line 51-59: Update the answer.contains and answer.regex branches in the
metric-label formatter to always return generic labels without including
configured text or regex patterns, removing the elide-based interpolation.
Adjust the corresponding metric-label tests to stop asserting that raw patterns
appear in output.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5be1fbbb-2b5b-4938-8b15-a0598649b701
📒 Files selected for processing (13)
cli/commands/eval/command.test.tscli/commands/eval/command.tsdocs/api-reference/veryfront/eval.mdsrc/eval/metric-labels.test.tssrc/eval/metric-labels.tssrc/eval/metrics.test.tssrc/eval/metrics.tssrc/eval/report.test.tssrc/eval/report.tssrc/eval/run-report.test.tssrc/eval/run-report.tssrc/eval/runner.test.tssrc/eval/types.ts
Comment on lines
+808
to
+810
| function printLine(text: string): void { | ||
| console.log(` ${text}`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve --quiet behavior.
printLine() writes directly to console.log. The suite, single-eval, and model-comparison paths now use this helper. veryfront eval --quiet can emit human-readable report output because this path bypasses the logger.
Use a quiet-aware output helper, or return before rendering human output when quiet mode is active. Add coverage for veryfront eval --quiet.
As per coding guidelines, "Keep CLI global flags and exit codes consistent: --json/-j, --output/-o, --yes/-y, --quiet/-q...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/commands/eval/command.ts` around lines 808 - 810, Update printLine and
its callers in the suite, single-eval, and model-comparison paths to respect the
active --quiet mode by routing output through the established quiet-aware helper
or suppressing rendering when quiet is enabled. Preserve normal human-readable
output otherwise, and add coverage verifying veryfront eval --quiet emits no
report output.
Source: Coding guidelines
Comment on lines
+51
to
+59
| case "answer.contains": { | ||
| const text = readString(config, "text"); | ||
| return text ? `Answer contained "${elide(text)}"` : "Answer contained the expected text"; | ||
| } | ||
| case "answer.regex": { | ||
| const pattern = readString(config, "pattern"); | ||
| return pattern | ||
| ? `Answer matched pattern ${elide(pattern)}` | ||
| : "Answer matched the expected pattern"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not emit configured answer text or regex patterns.
Lines 53 and 57 put raw metric configuration into report and CLI labels. Expected answers and patterns can contain customer data or secrets. Elision only shortens the value. It does not redact the value.
Use a generic label for these metrics. Update src/eval/metric-labels.test.ts to remove the assertion that requires the raw pattern in output.
Proposed fix
case "answer.contains": {
- const text = readString(config, "text");
- return text ? `Answer contained "${elide(text)}"` : "Answer contained the expected text";
+ return "Answer contained the expected text";
}
case "answer.regex": {
- const pattern = readString(config, "pattern");
- return pattern
- ? `Answer matched pattern ${elide(pattern)}`
- : "Answer matched the expected pattern";
+ return "Answer matched the expected pattern";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case "answer.contains": { | |
| const text = readString(config, "text"); | |
| return text ? `Answer contained "${elide(text)}"` : "Answer contained the expected text"; | |
| } | |
| case "answer.regex": { | |
| const pattern = readString(config, "pattern"); | |
| return pattern | |
| ? `Answer matched pattern ${elide(pattern)}` | |
| : "Answer matched the expected pattern"; | |
| case "answer.contains": { | |
| return "Answer contained the expected text"; | |
| } | |
| case "answer.regex": { | |
| return "Answer matched the expected pattern"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/eval/metric-labels.ts` around lines 51 - 59, Update the answer.contains
and answer.regex branches in the metric-label formatter to always return generic
labels without including configured text or regex patterns, removing the
elide-based interpolation. Adjust the corresponding metric-label tests to stop
asserting that raw patterns appear in output.
Source: Coding guidelines
Comment on lines
+61
to
+73
| case "agent.calledTool": { | ||
| const tool = readString(config, "tool"); | ||
| return tool ? `Agent called tool "${tool}"` : "Agent called the expected tool"; | ||
| } | ||
| case "agent.notCalledTool": { | ||
| const tool = readString(config, "tool"); | ||
| return tool ? `Agent did not call tool "${tool}"` : "Agent avoided the excluded tool"; | ||
| } | ||
| case "agent.toolCallCount": { | ||
| const tool = readString(config, "tool"); | ||
| return tool | ||
| ? `Agent call count for tool "${tool}" was in range` | ||
| : "Agent tool call count was in range"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Elide configured tool names.
MAX_PARAM_LENGTH applies to rendered metric parameters, but lines 63, 67, and 72 render tool without elide. A long configured tool name can wrap the CLI output.
Apply elide(tool) at each interpolation. Add a focused long-tool test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/eval/metric-labels.ts` around lines 61 - 73, Update the metric label
cases in the label-rendering function, specifically agent.calledTool,
agent.notCalledTool, and agent.toolCallCount, to pass configured tool names
through elide() before interpolation. Add a focused test using a long tool name
that verifies the rendered labels remain elided and do not wrap CLI output.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
veryfront evalprinted one line per metric keyed by factory name, soagent.calledToolnever said which tool the eval asserted on. It also put the●glyph on every line, including report paths, which made nothing stand out.Before
After
Where the label comes from
The parameter that makes an assertion specific lives in the config the metric factory captured, so the label is built there rather than reconstructed in the CLI:
src/eval/metric-labels.ts(new) maps a metric name plus its config to prose for all built-in metrics —agent.calledTool→Agent called tool "calculator",ops.cost→Cost stayed under $0.05,knowledge.recallAtK→Knowledge recall@2. Unknown names returnundefinedso callers fall back to the raw name instead of guessing. Long parameters (the scaffold's regexes) elide at 40 chars so a metric line stays on one row.createMetricis the single choke point every metric funnels through, so the label attaches there and rides onEvalMetricResult.label— a field that already existed and was unused.EvalMetricSummary.labelcarries it into the JSON report, not just the terminal.name, soEval:heads the block withAssistant smoke testrather than the generated id.Known limitation
Two
calledToolassertions in one eval share a summary row keyedname:family:severity. No single label describes both, so it is dropped there and the row falls back toagent.calledTool.The summary key is deliberately unchanged:
src/eval/baseline.ts:12derives the same key independently, so splitting rows would silently drop data in baseline comparisons.Other output changes
Report directoryandReport markdowncollapse into oneReport:pointing at the markdown.--reportis relabeledReport JSON:. That flag writes the JSON summary, not markdown, and previously shared theReport:label with the markdown line.Report directorylines are gone from suite output; the directory is the report path's parent.Testing
src/eval/+cli/commands/eval/— 19 passed, 209 steps, 0 failedextensions/ext-eval-report-mlflow/(the only other consumer of these types) — passingdeno lint,deno fmt --check,lint:module-boundaries,lint:dependency-boundaries,lint:cli-boundary,lint:barrel-jsdoc,lint:core-deps,docs:api-reference:check— all passingNew tests cover the label formatter (including fallback and elision paths), label propagation into the summary, the conflict-drop rule, and the rendered CLI lines. Writing the formatter test caught a prototype-pollution bug in the lookup table: a metric named
toStringreturnedObject.prototype.toStringas its label. Fixed with a null-prototype object.The regenerated
docs/api-reference/veryfront/eval.mdcontains only source-link line shifts from these edits.Summary by CodeRabbit
New Features
Bug Fixes
Documentation