test: add the day-planning eval report and judge bundle#3589
Conversation
Aggregates matrix results into JSON + Markdown under the git-ignored tmp/day-planning-eval, env-overridable so runs accumulate and can be diffed. Leads with a model leaderboard, then per-constraint rates, cost, prompt stability and failure excerpts. Rates are over applicable results only. Folding "not applicable" in either direction yields a plausible-looking number that is wrong: counting it as a pass makes the laziest model look best, counting it as a fail punishes a scenario for not exercising a dimension. A constraint nothing exercised reports no rate rather than 100%, and the leaderboard breaks ties deterministically so diffing two reports is not noise. Prompt stability is per model across scenarios, not per cell. The first report generated end to end showed an identical stable-prefix figure for every scenario, because within one cell the prompt is compared against itself and the column just restated the prompt size. Across wakes it answers the real question — what a provider could cache — and on the current prompt the answer is the entire 7.7 KB system message, with all variation in the user message. Plan quality is judged from artifacts rather than by an in-harness LLM judge, so the report emits a self-sufficient bundle: one object per cell with the scenario and its intent, the exact prompts, every tool call including rejections and their text, the persisted plan, and that run's constraint results and cost. It is bounded and states what it dropped, because a truncated bundle that stays silent reads as complete. Corpus rows carry visibleToModel — the corpus is ground truth rendered only inside the capture context, so without that flag a judge would blame the model for ignoring a dependency it never received.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughAdds an evaluation reporting layer that aggregates Daily OS Next runs into constraint metrics, leaderboards, costs, failures, and capped judge bundles, then writes JSON and Markdown reports with configurable paths. Tests and documentation cover the new behavior. ChangesEvaluation reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EvalRunResult
participant EvalReport
participant writeEvalReport
participant ReportFiles
EvalRunResult->>EvalReport: aggregate evaluation results
EvalReport->>EvalReport: build JSON and Markdown
EvalReport->>writeEvalReport: provide serialized report
writeEvalReport->>ReportFiles: create directories and write files
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/features/daily_os_next/eval/framework/eval_report_test.dart (1)
507-553: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering
scenarioMatrixand per-(model, scenario) cost grouping.
toJson()['scenarioMatrix']and the_costRowsgrouping key (which the source comments call out as deliberately a record rather than a joined string) are currently unasserted. A test with two scenarios for one model would pin both.🤖 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 `@test/features/daily_os_next/eval/framework/eval_report_test.dart` around lines 507 - 553, Extend the writeEvalReport tests to cover scenarioMatrix and per-(model, scenario) cost grouping using two scenarios for the same model. Assert the serialized toJson()['scenarioMatrix'] contents and verify _costRows keeps costs grouped separately by the record key rather than merging scenarios.test/features/daily_os_next/eval/framework/eval_report.dart (1)
359-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrompt-stability computation is duplicated in
toMarkdown().Lines 359-377 and Lines 641-656 independently derive
runs,stable,mean, and the fraction. If one changes (e.g. filtering out failed runs), JSON and Markdown silently disagree. Extracting a small private record-returning helper keeps the two renderers on one definition.♻️ Sketch
+ ({int wakes, int meanBytes, int stableBytes}) _stabilityFor(String modelId) { + final runs = results.where((r) => r.request.modelId == modelId); + return ( + wakes: runs.length, + meanBytes: _mean(runs.map((r) => utf8.encode(r.systemPrompt ?? '').length)), + stableBytes: _stablePrefixBytes(runs), + ); + }As per coding guidelines: "favor DRY design where sensible, preserve modularity and testability".
🤖 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 `@test/features/daily_os_next/eval/framework/eval_report.dart` around lines 359 - 377, Extract the duplicated prompt-stability calculation from the JSON renderer and toMarkdown() into a private helper in the surrounding report class, returning a small record with runs count, stable prefix bytes, mean system-prompt bytes, and stable fraction. Update both renderers to consume this helper so they share identical model filtering and calculations.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.
Nitpick comments:
In `@test/features/daily_os_next/eval/framework/eval_report_test.dart`:
- Around line 507-553: Extend the writeEvalReport tests to cover scenarioMatrix
and per-(model, scenario) cost grouping using two scenarios for the same model.
Assert the serialized toJson()['scenarioMatrix'] contents and verify _costRows
keeps costs grouped separately by the record key rather than merging scenarios.
In `@test/features/daily_os_next/eval/framework/eval_report.dart`:
- Around line 359-377: Extract the duplicated prompt-stability calculation from
the JSON renderer and toMarkdown() into a private helper in the surrounding
report class, returning a small record with runs count, stable prefix bytes,
mean system-prompt bytes, and stable fraction. Update both renderers to consume
this helper so they share identical model filtering and calculations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28199686-36d0-4e17-8274-d5fa7af12bbe
📒 Files selected for processing (3)
lib/features/daily_os_next/README.mdtest/features/daily_os_next/eval/framework/eval_report.darttest/features/daily_os_next/eval/framework/eval_report_test.dart
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3589 +/- ##
=======================================
Coverage 99.15% 99.15%
=======================================
Files 1782 1782
Lines 130810 130810
=======================================
Hits 129704 129704
Misses 1106 1106
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05cf746cfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Aggregate prompt stability over actual wake transcripts. It read only the final wake of each cell, so a durable retry's earlier conversations vanished from a figure claiming to cover every wake — and a cell that never reached the model contributed a zero-length prompt to the mean while being excluded from the prefix, which could report a "stable" fraction above 100%. Prefix, count and mean now come from one set of captured prompts, and the count is wakes rather than cells. Keep variants apart in the scenario matrix. Pooling them averaged a baseline pass and a variant failure into one 50% cell, hiding which planning contract produced the outcome — the entire reason the matrix has a variant axis. Serialise every wake in the judge bundle. The entry's tool calls, attempts and cost cover the whole cell, so showing only the last prompt left a judge unable to reconcile what it was reading against the totals beside it. Separate corpus visibility from decided-task referenceability. A decided task is referenceable through its own projection even on a capture-less wake, so the single flag marked its corpus row visible while its status, estimate and blockedBy had never been rendered — printing blockedBy next to "the model saw this", which is the exact wrong conclusion the flag was added to prevent. Stamp the default report basename with the run time. The documented promise is that runs accumulate and can be diffed; with a fixed name each invocation silently overwrote the last, so there was never anything to diff against. Explicit env paths still stay fixed.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/features/daily_os_next/eval/framework/eval_report.dart (1)
501-524: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPer-task
corpusRowShownnever differs from the cell-level flag — it ignores the task's own visibility.Both
corpusRowsShown(Line 510, evaluated once for the whole cell) and the per-taskcorpusRowShown(Line 519, evaluated inside thefor (final task in inputs.corpus)loop) use the exact same expressioninputs.visibleTaskIds == null. Since this never referencestask.taskId, every task in the corpus gets an identical value — the per-row flag can never betruefor a visible task whilefalsefor a hidden one within the same result, which defeats the purpose the doc comment above describes ("whether this row ... was rendered at all"). Given the field is namedvisibleTaskIds(aSet<String>?), the per-task check should test membership.🐛 Proposed fix
'blockedBy': task.blockedBy, - 'corpusRowShown': inputs.visibleTaskIds == null, + 'corpusRowShown': inputs.visibleTaskIds?.contains(task.taskId) ?? true, 'taskIdReferenceable': inputs.referenceableTaskIds.contains( task.taskId, ),This is precisely the failure mode the surrounding comment warns against — a judge would see
blockedBynext to a row it's told was never rendered (or vice versa), since the flag is constant regardless of which task is actually visible.🤖 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 `@test/features/daily_os_next/eval/framework/eval_report.dart` around lines 501 - 524, Update the per-task `corpusRowShown` field inside the `inputs.corpus` loop to determine visibility from the current `task.taskId` membership in `inputs.visibleTaskIds`, while preserving the cell-level `corpusRowsShown` behavior. Handle the nullable visibility set consistently so each row reflects whether that specific task was rendered.
🤖 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.
Outside diff comments:
In `@test/features/daily_os_next/eval/framework/eval_report.dart`:
- Around line 501-524: Update the per-task `corpusRowShown` field inside the
`inputs.corpus` loop to determine visibility from the current `task.taskId`
membership in `inputs.visibleTaskIds`, while preserving the cell-level
`corpusRowsShown` behavior. Handle the nullable visibility set consistently so
each row reflects whether that specific task was rendered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 49890295-7f84-4ed6-a0b4-759ec859633f
📒 Files selected for processing (3)
lib/features/daily_os_next/README.mdtest/features/daily_os_next/eval/framework/eval_report.darttest/features/daily_os_next/eval/framework/eval_report_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/daily_os_next/README.md
- test/features/daily_os_next/eval/framework/eval_report_test.dart
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f51c3c51c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| utf8.encode(result.systemPrompt ?? '').length + | ||
| result.userPrompts.fold<int>( |
There was a problem hiding this comment.
When a durable cell retries, these getters expose only draftWake, so meanPromptBytes and the bundle's cost.promptBytes omit every prompt sent during earlier attempts while token totals include consumption from the whole cell. This undercounts retrying models and still prevents the bundle's prompts from reconciling with its cost; sum the system and user messages across result.wakes instead.
AGENTS.md reference: AGENTS.md:L153-L155
Useful? React with 👍 / 👎.
| final key = (result.request.modelId, result.request.scenario.id); | ||
| byPair.putIfAbsent(key, () => []).add(result); |
There was a problem hiding this comment.
When a run includes a non-baseline variant, this key still pools its token totals, latency, and prompt size with the baseline for the same model/scenario. Although scenarioMatrix now separates variants, the fresh evidence here is that the cost table cannot show whether the changed planning contract made that cell cheaper or more expensive; include variant.id in this grouping and output row.
Useful? React with 👍 / 👎.
| static List<EvalFailureExcerpt> _failures(List<EvalRunResult> results) => [ | ||
| for (final result in results) | ||
| for (final constraint in result.constraints) | ||
| if (constraint.passed == false) |
There was a problem hiding this comment.
When a cell fails before persisting a plan, _failedResult produces inapplicable constraints rather than passed == false, so this filter omits the cell entirely and Markdown can report Failures: None despite failedRuns > 0. If the bounded bundle also drops that sample, the provider/configuration error text is absent from the artifacts; add run errors to the failure output independently of constraint violations.
AGENTS.md reference: AGENTS.md:L153-L155
Useful? React with 👍 / 👎.
| return '${at.year}${two(at.month)}${two(at.day)}-' | ||
| '${two(at.hour)}${two(at.minute)}${two(at.second)}'; |
There was a problem hiding this comment.
The timestamp-based fix still discards milliseconds and microseconds, so two report invocations whose generatedAt values fall in the same second resolve to identical default JSON and Markdown paths and the latter overwrites the former. This is fresh evidence that the documented accumulation guarantee remains incomplete; include subsecond precision or another unique run identifier while retaining fixed explicit overrides.
AGENTS.md reference: AGENTS.md:L153-L155
Useful? React with 👍 / 👎.
| final scenario = result.request.scenario; | ||
| final inputs = result.outcome.inputs; | ||
| return { | ||
| 'cell': result.request.label, |
There was a problem hiding this comment.
For a non-baseline or caller-defined variant, the entry contains only its ID inside cell; it omits variant.rationale and the complete effective DayAgentConfig (including fields such as energy bands and refinement rounds). A judge reading this supposedly self-sufficient entry therefore cannot know what experiment the variant represents or which full contract to assess without external code or reverse-engineering the prompt; add an explicit variant object with its rationale and captured effective configuration.
AGENTS.md reference: AGENTS.md:L153-L155
Useful? React with 👍 / 👎.
1 participant
Aggregates matrix results into JSON + Markdown, and into a bundle plan quality can be judged from without re-running. Closes
lotti3-anb.4. Test-only; the solelib/change is the feature README.Output goes to the git-ignored
tmp/day-planning-eval/, env-overridable viaDAY_PLANNING_EVAL_DIR(or per-file withDAY_PLANNING_EVAL_JSON/DAY_PLANNING_EVAL_MARKDOWN), so runs accumulate and can be diffed.Rates are over applicable results only
This is the property the whole report stands on, and both ways of getting it wrong produce a number that looks perfectly reasonable:
So a constraint nothing exercised reports
—, never 100%, and the leaderboard sorts models with no applicable results last rather than first. Ties break deterministically, or diffing two reports of the same run is noise.What reading a generated report actually caught
I generated one end to end from a scripted matrix run before finishing, which is acceptance criterion 4. Two things fell out that no unit test would have:
Stable-prefix was measured in the wrong place. It was per (model, scenario), where there is normally one run — so it compared a prompt against itself and printed an identical figure for every scenario, silently restating the system prompt size. It is now per model across scenarios, which answers the question that matters: how much of the prompt a provider could cache. On the current prompt that is the entire 7.7 KB system message, with all variation in the user message. That is
lotti3-hkb.16delivered as a by-product, as its bead anticipated.The bundle could mislead a judge. Corpus rows carried
blockedBybut nothing said whether the model was shown them. The corpus is ground truth and is rendered only inside the capture context, so on a capture-less wake (blockedWithoutCorpusexists precisely to test that) the model saw none of it. A judge readingblockedBywould have blamed the model for ignoring a dependency it never received. Rows now carryvisibleToModel.The judge bundle
Plan quality — is this a sensible day, is the ordering right, is it realistic rather than overpacked-but-legal — is judged from artifacts by a person or by Opus 5 reading the repo, not by an in-harness LLM judge that would bake scoring noise and cost into every run while saying little you can act on.
So each bundle entry is self-sufficient: the scenario and its intent (a judge cannot assess an answer without the question), the exact prompts, every tool call including rejections and their text, the persisted plan, and that run's constraint results and cost. It is bounded to the newest samples per cell and states what it dropped — a truncated bundle that stays silent reads as complete.
Verification
fvm dart analyzeclean.nullrather than zero — zero would read as "the variant changed nothing", a claim the data cannot support.Summary by CodeRabbit
New Features
Documentation
Tests