feat(eval): add --runs N flag with median scoring - #111
Closed
yogeshchoudhary147 wants to merge 10 commits into
Closed
feat(eval): add --runs N flag with median scoring#111yogeshchoudhary147 wants to merge 10 commits into
yogeshchoudhary147 wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CLI adds ChangesRepeated eval runs
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant runCli
participant spawnEval
participant collectFromTempFiles
participant aggregateRuns
participant mergeIntoOutput
runCli->>spawnEval: dispatch expanded per-run jobs
spawnEval->>collectFromTempFiles: collect temporary results
runCli->>aggregateRuns: aggregate collected results
aggregateRuns->>mergeIntoOutput: pass aggregated results
mergeIntoOutput->>runCli: save merged output
Suggested reviewers: 🚥 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/eval/tests/persistence.test.ts (1)
481-481: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical error in test description.
The word "medianes" is a typo. It should be "medians".
♻️ Proposed refactor
- it('medianes per-dimension scores', () => { + it('medians per-dimension scores', () => {🤖 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 `@packages/eval/tests/persistence.test.ts` at line 481, Update the test description in the “medianes per-dimension scores” test case to use the correctly spelled “medians”, without changing the test behavior.packages/eval/src/cli/subprocess-runner.ts (1)
44-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify
mergeIntoOutputwith the shared results merge. Its localeval_id|model|mode|toolskey joinstoolswithout sorting or deduping, so it can drift fromresultKey/mergeResults. Reuse the shared persistence helpers instead of maintaining a second dedup path.🤖 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 `@packages/eval/src/cli/subprocess-runner.ts` around lines 44 - 53, The local deduplication path in mergeIntoOutput must use the shared resultKey/mergeResults persistence helpers so tools are normalized consistently. Update packages/eval/src/cli/subprocess-runner.ts:44-53 to remove the local key logic and delegate merging to the shared helpers; update packages/eval/src/cli/index.ts:13-13 to use the same shared merge path where this flow is wired, preserving temp-file collection, final output writing, and cleanup.
🤖 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 `@packages/eval/src/cli/run.ts`:
- Around line 459-466: Update the hasErrors calculation in the run flow to
inspect the pre-aggregation allFresh results rather than merged, preserving
failures.length and treating any fresh result with status "error" as a process
failure even when aggregateRuns removes it.
In `@packages/eval/src/cli/subprocess-runner.ts`:
- Around line 25-42: Add Vitest tests for the exported collectFromTempFiles
function covering flattening valid array payloads, skipping nonexistent temp
files, and ignoring corrupt JSON while cleaning up existing files. Use temporary
file fixtures and assert the returned records for each scenario.
In `@packages/eval/src/persistence/results.ts`:
- Around line 160-177: Update medianBaselineResult so graders_passed and
grader_pass_rate are derived from a single consistent aggregated value rather
than independently calculated medians. Preserve the existing median aggregation
semantics while ensuring the resulting fields cannot imply different pass rates;
use the appropriate shared baseline count or rate relationship for the group.
---
Nitpick comments:
In `@packages/eval/src/cli/subprocess-runner.ts`:
- Around line 44-53: The local deduplication path in mergeIntoOutput must use
the shared resultKey/mergeResults persistence helpers so tools are normalized
consistently. Update packages/eval/src/cli/subprocess-runner.ts:44-53 to remove
the local key logic and delegate merging to the shared helpers; update
packages/eval/src/cli/index.ts:13-13 to use the same shared merge path where
this flow is wired, preserving temp-file collection, final output writing, and
cleanup.
In `@packages/eval/tests/persistence.test.ts`:
- Line 481: Update the test description in the “medianes per-dimension scores”
test case to use the correctly spelled “medians”, without changing the test
behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 89af2cdd-4410-4ba0-b19c-c9a5555784f6
📒 Files selected for processing (11)
packages/eval-core/src/types/results.tspackages/eval/src/cli/config.tspackages/eval/src/cli/index.tspackages/eval/src/cli/run.tspackages/eval/src/cli/subprocess-runner.tspackages/eval/src/cli/validators.tspackages/eval/src/index.tspackages/eval/src/persistence/index.tspackages/eval/src/persistence/results.tspackages/eval/tests/cli-config.test.tspackages/eval/tests/persistence.test.ts
Contributor
Author
|
Re: CodeRabbit comment on CodeRabbit flagged that We held this back intentionally — the fix is technically correct but the right behavior here is a product decision:
Happy to go with whichever direction the team prefers before merging. |
yogeshchoudhary147
force-pushed
the
feat/runs-n-median-scoring
branch
2 times, most recently
from
July 14, 2026 09:31
fc4074a to
c2d8d70
Compare
Contributor
Author
|
Resolved in 0a99fc7 with Option 3. Before |
yogeshchoudhary147
force-pushed
the
feat/runs-n-median-scoring
branch
from
July 14, 2026 10:08
0a99fc7 to
f715bfe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AGENTS.md (1)
369-379: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the documented
--model allfallback list in sync.The text says the fallback is the framework’s
KNOWN_WORKING_MODELS, but the bullets omitclaude-opus-4-6andclaude-opus-4-7, which are present in that constant. Ifmodels.knownis empty,--model allwill include them despite this documentation. Either list the fallback values accurately or label the bullets as app-specific.🤖 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 `@AGENTS.md` around lines 369 - 379, The documented --model all list conflicts with its stated KNOWN_WORKING_MODELS fallback by omitting claude-opus-4-6 and claude-opus-4-7. Update the documentation around the eval.config.js model list to include those fallback models, or explicitly label the bullets as app-specific and separately document the complete fallback values.
🤖 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 `@packages/eval/src/cli/validators.ts`:
- Around line 106-115: Update validateRuns to enforce a documented maximum
positive --runs value before returning, rejecting values above that maximum
through the existing invalid-value path. Define and reuse a named maximum
constant, and add tests covering the maximum accepted value and the first value
above it so runCli cannot expand an unbounded job list.
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 369-379: The documented --model all list conflicts with its stated
KNOWN_WORKING_MODELS fallback by omitting claude-opus-4-6 and claude-opus-4-7.
Update the documentation around the eval.config.js model list to include those
fallback models, or explicitly label the bullets as app-specific and separately
document the complete fallback values.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c70910cf-eba7-4c43-a615-d5b2ba7bcc89
📒 Files selected for processing (14)
AGENTS.mdpackages/eval-core/src/types/results.tspackages/eval/src/cli/config.tspackages/eval/src/cli/index.tspackages/eval/src/cli/run.tspackages/eval/src/cli/subprocess-runner.tspackages/eval/src/cli/validators.tspackages/eval/src/index.tspackages/eval/src/persistence/index.tspackages/eval/src/persistence/results.tspackages/eval/tests/cli-config.test.tspackages/eval/tests/persistence.test.tspackages/eval/tests/run.test.tspackages/eval/tests/subprocess-runner.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/eval/src/persistence/index.ts
- packages/eval/src/index.ts
- packages/eval/tests/subprocess-runner.test.ts
- packages/eval/src/cli/index.ts
- packages/eval/src/cli/config.ts
- packages/eval/tests/cli-config.test.ts
- packages/eval/src/persistence/results.ts
- packages/eval/src/cli/run.ts
- packages/eval/src/cli/subprocess-runner.ts
- packages/eval/tests/persistence.test.ts
Adds a --runs <n> CLI option (default: 1) that runs each eval job N times in parallel and aggregates the results using median scoring. Scores are medianed per-dimension; costs and tokens are summed across all runs. Raw individual run results are embedded in a runs[] field for debugging, and run_count records how many runs were aggregated. When runs > 1, a run index suffix is appended to each temp file name to avoid collisions, and collectFromTempFiles + aggregateRuns replaces the previous mergeIntoOutput call in the subprocess path.
- Check pre-aggregation allFresh for hasErrors so partial run failures are not silently swallowed when aggregateRuns drops error results - Derive grader_pass_rate from graders_passed/graders_total to keep both fields consistent after aggregation - Add Vitest tests for collectFromTempFiles (7 cases) - Fix typo: medianes → medians in test description
The hasErrors change (allFresh vs merged) needs further discussion — reverting until the product decision on partial-run failure semantics is resolved.
Sort and deduplicate tools in the local key function so it stays consistent with the shared resultKey helper in persistence.
Addresses CodeRabbit pre-merge check — docstring coverage was at 77.78% (below the required 80% threshold). Both internal aggregation helpers were missing JSDoc comments.
…rsion When --runs N was passed, buildSubprocessArgs forwarded it to every child process. Each child saw runs > 1, took the subprocess path, and spawned N more children — each of which did the same, causing exponential process explosion until the runner was OOM-killed (exit 137). Fix: add '--runs' to VALUE_FLAGS so it is stripped before forwarding. Subprocesses run a single job once; run-count expansion is the parent's job. Adds a regression test that would have caught this.
yogeshchoudhary147
force-pushed
the
feat/runs-n-median-scoring
branch
from
July 22, 2026 17:24
d1a62b6 to
6914817
Compare
…expansion Without an upper bound, --runs 1000 would expand to 1000 subprocesses. Adds a named MAX_RUNS constant (10), rejects values above it through the existing invalid-value path, and adds boundary tests for the max and first-over-max values.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/evals/src/persistence/results.ts (1)
229-234: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
findDroppedErrorsis O(n²) — fine at current scale, but worth a linear rewrite.For every error result it scans the full
resultsarray again to check for a same-key success, andresultKeyre-derives the string each time. Withevals × models × modes × runspotentially in the hundreds/thousands, this becomes a quadratic hot path. A single pass to build aSetof keys with successes would make this linear.♻️ Suggested O(n) rewrite
export function findDroppedErrors(results: JobResult[]): ErrorJobResult[] { - return results.filter( - (r): r is ErrorJobResult => - r.status === 'error' && results.some((s) => s.status !== 'error' && resultKey(s) === resultKey(r)), - ); + const successKeys = new Set(results.filter((r) => r.status !== 'error').map(resultKey)); + return results.filter((r): r is ErrorJobResult => r.status === 'error' && successKeys.has(resultKey(r))); }🤖 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 `@packages/evals/src/persistence/results.ts` around lines 229 - 234, Rewrite findDroppedErrors to avoid the nested results.some scan: make one pass to collect resultKey values for non-error results in a Set, then filter error results by membership in that Set. Reuse each computed key within its pass so resultKey is not repeatedly derived, while preserving the existing ErrorJobResult selection behavior.packages/evals/src/cli/subprocess-runner.ts (1)
44-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
mergeIntoOutputexport and keep deduplication in one place.
mergeIntoOutputhas no runtime callers; the CLI now usesloadResults/mergeResults/saveResultsfrom persistence. Since it is still re-exported frompackages/evals/src/index.tsandcli/index.ts, either remove the public export or move actual call sites to the persistence helpers to avoid duplicated tool-normalization behavior.🤖 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 `@packages/evals/src/cli/subprocess-runner.ts` around lines 44 - 56, Remove the unused mergeIntoOutput function and its public re-exports from packages/evals/src/index.ts and cli/index.ts. Keep the CLI flow using loadResults, mergeResults, and saveResults from persistence so tool normalization and deduplication remain centralized.
🤖 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 `@packages/evals-core/src/types/results.ts`:
- Around line 83-86: Define one consistent contract for run_count and runs in
aggregateRuns: specify whether they describe requested runs, successful runs, or
all raw outcomes, and ensure partial failures follow that contract. Update both
metadata declarations in packages/evals-core/src/types/results.ts at lines 83-86
and 148-151; add a regression test covering one successful run plus one error,
including the expected metadata and filtered results.
In `@packages/evals/src/persistence/results.ts`:
- Around line 130-160: Update medianAgentResult’s dimension aggregation to
handle runs with missing dimensions without dereferencing undefined entries. For
each representative dimension, filter the group to runs containing a dimension
at the matching index, compute the median from available scores, and preserve
the existing representative-dimension metadata and score-to-grade calculation;
ensure aggregation continues gracefully when some runs omit that dimension.
In `@packages/evals/tests/persistence.test.ts`:
- Line 17: Update the typed fixtures returned by makeBaseline, makeAgent, and
makeError to include the required judge_cost_usd and total_cost_usd fields,
using values consistent with the existing test fixture conventions while
preserving all other result data.
---
Nitpick comments:
In `@packages/evals/src/cli/subprocess-runner.ts`:
- Around line 44-56: Remove the unused mergeIntoOutput function and its public
re-exports from packages/evals/src/index.ts and cli/index.ts. Keep the CLI flow
using loadResults, mergeResults, and saveResults from persistence so tool
normalization and deduplication remain centralized.
In `@packages/evals/src/persistence/results.ts`:
- Around line 229-234: Rewrite findDroppedErrors to avoid the nested
results.some scan: make one pass to collect resultKey values for non-error
results in a Set, then filter error results by membership in that Set. Reuse
each computed key within its pass so resultKey is not repeatedly derived, while
preserving the existing ErrorJobResult selection behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e5954154-a176-4a73-85b6-e031506bc6b0
📒 Files selected for processing (14)
AGENTS.mdpackages/evals-core/src/types/results.tspackages/evals/src/cli/config.tspackages/evals/src/cli/index.tspackages/evals/src/cli/run.tspackages/evals/src/cli/subprocess-runner.tspackages/evals/src/cli/validators.tspackages/evals/src/index.tspackages/evals/src/persistence/index.tspackages/evals/src/persistence/results.tspackages/evals/tests/cli-config.test.tspackages/evals/tests/persistence.test.tspackages/evals/tests/run.test.tspackages/evals/tests/subprocess-runner.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
yogeshchoudhary147
force-pushed
the
feat/runs-n-median-scoring
branch
from
July 23, 2026 02:18
0ab6a3d to
6953935
Compare
- Add missing judge_cost_usd and total_cost_usd to makeBaseline, makeAgent, and makeError test fixtures so they satisfy the required type contract. - Guard dimensions[i] access in medianAgentResult against runs that report fewer dimensions than the representative, falling back to the representative's own score instead of throwing a TypeError. - Clarify run_count/runs JSDoc in BaselineJobResult and AgentJobResult: both fields are set whenever --runs N > 1, including when partial failures reduce successful runs to 1 (contradicting the old "absent when run_count is 1" wording). - Add regression test: 1 success + 1 error → run_count: 1, runs: [ok].
Contributor
Author
|
Closing in favor of handling this on the runner side. |
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.
Summary
--runs <n>CLI flag (default: 1, fully backward compatible) that runs each eval job N times in parallel and aggregates results using median scoringruns[]field withrun_countfor debuggingTest plan
npm run buildpassesnpm testpasses (734 tests, 0 failures)npm run lintclean on all changed packagesaggregateRuns()(15 cases) and--runsvalidation (6 cases)Summary by CodeRabbit
--runs <n>CLI option to repeat each evaluation job and compute median-based aggregated results.AGENTS.mdto document the new--runsoption.--runs.