fix(eval,cli): honor --quiet and keep metric labels behind export redaction - #3454
Conversation
…action Review follow-ups on the metric-label change. - Moving eval output off `cliLogger` onto `console.log` also moved it off the logger's level, so `veryfront eval --quiet` printed the full report where it previously printed nothing. `printLine` and `printBlankLine` now check `isQuiet()` directly, with a test asserting no report output. - `EvalMetricResult.label` and `EvalMetricSummary.label` restate the metric's configured parameter, which is the same class of detail `evidence` carries. `redactEvalReportForExport` stripped evidence but passed labels straight through to third-party exporters, and never redacted summary metrics at all. Both now leave on the same terms as evidence. Terminal output is unaffected. - Tool names are elided like the other inline parameters, so a long tool name cannot wrap a metric 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. |
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe eval CLI suppresses informational output in quiet mode. Agent metric labels elide long tool names. Exported reports remove metric labels when metric evidence is disabled. Tests and API source references are updated. ChangesEval output controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/api-reference/veryfront/extensions.md (1)
654-655: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the label redaction contract.
includeMetricEvidence: falsenow removeslabelfrom record metrics and summary metrics. These rows only update source links. Update the publicEvalReportExportRedactiondocumentation, or its generated source comment, to state that labels follow metric evidence visibility.As per coding guidelines, public behavior changes must update relevant documentation and generated references.
🤖 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 `@docs/api-reference/veryfront/extensions.md` around lines 654 - 655, Update the public EvalReportExportRedaction documentation or its generated source comment to state that when includeMetricEvidence is false, label fields are removed from both record and summary metrics while source links remain. Ensure the relevant generated API reference reflects this contract.Source: Coding guidelines
🤖 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.test.ts`:
- Around line 1258-1287: Update the test setup around runEvalCommand to capture
the original values of VERYFRONT_API_TOKEN, VERYFRONT_PROJECT_SLUG,
VERYFRONT_EVAL_EXPORT, VERYFRONT_EVAL_EXPORTERS, and XDG_CONFIG_HOME before the
try block, then restore each value in finally before removing projectDir and
configHome; preserve whether each variable was originally unset rather than
restoring empty strings.
In `@src/extensions/eval/eval-report-exporter.ts`:
- Around line 176-180: Replace the em dash in the comment near the
redacted.label deletion with a colon or comma, preserving the comment’s meaning
and ensuring no em dash or en dash remains in the TypeScript file.
---
Nitpick comments:
In `@docs/api-reference/veryfront/extensions.md`:
- Around line 654-655: Update the public EvalReportExportRedaction documentation
or its generated source comment to state that when includeMetricEvidence is
false, label fields are removed from both record and summary metrics while
source links remain. Ensure the relevant generated API reference reflects this
contract.
🪄 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: e7d8361a-71dc-4c11-b04d-9b6748e8d094
📒 Files selected for processing (7)
cli/commands/eval/command.test.tscli/commands/eval/command.tsdocs/api-reference/veryfront/extensions.mdsrc/eval/metric-labels.test.tssrc/eval/metric-labels.tssrc/extensions/eval/eval-report-exporter.test.tssrc/extensions/eval/eval-report-exporter.ts
Follow-ups from review. `includeMetricEvidence` now governs metric labels as well as evidence payloads, on both record and summary metrics. Both public declarations of `EvalReportExportRedaction` say so, and the generated API reference is regenerated to match. AGENTS.md forbids em dash and en dash characters in public comments. Two comments added by this work used them; both now use a colon or a comma.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/eval/report.ts`:
- Around line 287-288: Update the comment near the summary-row grouping logic to
state that metrics share a summary row only when their complete key matches:
result.name, result.family, and result.severity. Preserve the existing behavior
of dropping the row when no single label describes all grouped metrics.
🪄 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: 2ec8ac95-a6cf-4c3d-bca7-013571792865
📒 Files selected for processing (5)
docs/api-reference/veryfront/eval.mddocs/api-reference/veryfront/extensions.mdsrc/eval/report.tssrc/eval/types.tssrc/extensions/eval/eval-report-exporter.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api-reference/veryfront/extensions.md
- src/extensions/eval/eval-report-exporter.ts
Metrics share a summary row when name, family, and severity all match, not on name alone. The comment said "same name", which understated the condition it was explaining.
Follow-up to #3453. Three review findings landed after that PR was already in the merge queue, so they ship here.
1.
--quietregression (user-facing)#3453 moved eval report output off
cliLoggerontoconsole.logso the●glyph could mark metric assertions and nothing else. That also moved the output off the logger's level.--quiet/-qraises the canonical log level toWARN(cli/utils/index.ts:150), which used to suppress everycliLogger.infoline in the report — soveryfront eval --quietprinted nothing. After #3453 it printed the entire report.printLineandprintBlankLinenow checkisQuiet()directly. Warnings still print, matching theWARNlevel quiet mode selects.Confirmed by writing the test first: it failed on the merged code with
Result: 1/1 passed (100%)where[]was expected, and passes now.2. Metric labels bypassed the export redaction boundary
redactEvalReportForExportstripsexplanationandevidencefrom metric results unlessincludeMetricEvidenceis set, so that detail does not reach third-party exporters by default. The newlabelrestates the metric's configured parameter — the asserted tool name, the expected text, the regex — which is the same class of detailevidencecarries, and it passed straight through.Two gaps, both fixed:
redactMetricResultsleftEvalMetricResult.labelintact.redactEvalReportForExportnever redactedreport.summary.metricsat all. That was previously harmless becauseEvalMetricSummarycarried no sensitive fields; feat(eval,cli): describe metrics in prose and restructure eval output #3453 gave itlabel.Both now leave on the same terms as
evidence. Terminal output is unaffected — this boundary only governs what reaches an exporter.CodeRabbit's version of this finding was to drop the configured text and pattern from labels entirely. I did not take that: naming the parameter is the point of the feature, and it is what the terminal output was asked for. Scoping it to the export boundary keeps the CLI useful and respects the policy the repo already defines.
3. Tool names were not elided
elide()was applied toanswer.containstext andanswer.regexpatterns but not to tool names, so a long tool name could wrap a metric line. Applied consistently now, with the elision test extended to cover all five inline parameters.Testing
src/eval/,cli/commands/eval/,src/extensions/eval/,extensions/ext-eval-report-mlflow/— 21 passed, 253 steps, 0 faileddeno lint,deno fmt --check,docs:api-reference:check— passingBoth fixes are covered by tests written before the fix and verified failing first.
Summary by CodeRabbit