feat: harden benchmark candidate promotion - #179
Conversation
📝 WalkthroughWalkthroughSchema 25 changes retrieval benchmark promotion from fit-all diagnostics to excluded-fold evidence. It adds aggregate and partition guardrails, an untouched grouped final test, exact blockers, deterministic uncertainty intervals, stability diagnostics, and artifact/report support. ChangesPromotion evidence pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Runner as Benchmark runner
participant Search as Retrieval search
participant WeightSearch as weight-search
participant Evidence as derivePromotionEvidence
participant Report as Markdown report
Runner->>Search: execute benchmark search
Search->>WeightSearch: evaluate fusion and router holdouts
Search->>Evidence: provide excluded-fold results
Evidence->>Search: return promotion evidence
Search->>Runner: return validated results
Runner->>Report: render promotion evidence
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (6)
benchmarks/tests/retrieval.test.ts (1)
136-138: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlso assert that the final-test row was found.
This assertion checks cardinality only. It passes even if no promotion-evidence row matched the untouched final fold, in which case every candidate silently becomes
no-eligible-candidate. Add an assertion onfinalTest.presentandstability.foldsso this integration test catches a fold-label mismatch between the runner and the fold producer.💚 Proposed additional assertions
expect(artifact.promotionEvidence.length).toBe( artifact.models.length * routerFusionMethods * ROUTER_OBJECTIVES.length, ) + expect(artifact.promotionEvidence.every((row) => row.finalTest.present)).toBe(true) + expect(artifact.promotionEvidence.every((row) => row.stability.folds > 0)).toBe(true)🤖 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 `@benchmarks/tests/retrieval.test.ts` around lines 136 - 138, Add assertions alongside the promotionEvidence cardinality check in the retrieval integration test to verify the final-test result was found: assert finalTest.present is true and stability.folds contains the expected untouched final fold label. Keep the existing cardinality assertion unchanged and use the result object’s existing finalTest and stability fields.benchmarks/tests/promotion-evidence.test.ts (1)
95-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a missing final-test fold.
The tests cover a missing strategy and the fully eligible path. They do not cover the case where every required strategy is present but no row matches the final-test
strategyandfold. That path setsfinalTest.presenttofalseand blocks promotion, and it is the behavior most sensitive to the fold-label contract betweenbenchmarks/retrieval/evaluation/search.tsand the fold producer. Add a case with a mismatched final-test fold that assertspresent: falseandpromotionStatus: "no-eligible-candidate".🤖 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 `@benchmarks/tests/promotion-evidence.test.ts` around lines 95 - 134, Add a test case alongside the existing promotion evidence test using rows that include every required strategy but omit the requested final-test fold, then call derivePromotionEvidence with the mismatched strategy/fold selector. Assert finalTest.present is false and promotionStatus is "no-eligible-candidate", while preserving the existing eligible-path coverage.benchmarks/retrieval/evaluation/promotion-evidence.ts (3)
73-93: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider recording the number of resampled observations.
The interval is derived from
deltas, which holds one paired delta per fold for the given strategy and partition. If only one fold exists,lowerBoundandupperBoundequal the single delta. The report then shows a zero-width 95% interval next tobootstrapSamples: 1000, which reads as high confidence. Add an observation count toHoldoutUncertaintyso readers can distinguish a narrow interval from a single-fold degenerate interval.🤖 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 `@benchmarks/retrieval/evaluation/promotion-evidence.ts` around lines 73 - 93, Add an observation-count field to HoldoutUncertainty and populate it in bootstrapInterval using deltas.length, including the empty-input return. Ensure the generated report exposes this count alongside bootstrapSamples so single-fold intervals are distinguishable from genuinely narrow intervals.
131-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake configuration coordinate comparison independent of key order.
configValuesflattens each sub-record withObject.values, anddifferingCoordinatesthen compares the two arrays by index. This assumes bothEvidenceRouterConfigvalues enumerate their channel keys in the same order.Object.valuesorder follows insertion order, so a config rebuilt from a different source (for example after JSON round-trip or with a channel omitted) would compare mismatched channels. The loop also iterates only overleftValues.lengthand substitutes0for a missing right value, which reports a false difference instead of failing.Sort the keys explicitly so the comparison is positional by channel name, not by insertion order.
♻️ Proposed change to derive coordinates deterministically
+const sortedValues = (record: Readonly<Record<string, number>>): readonly number[] => + Object.keys(record) + .sort() + .map((key) => record[key] ?? 0) + const configValues = (config: EvidenceRouterConfig): readonly number[] => [ - ...Object.values(config.baseWeights), - ...Object.values(config.scoreInfluence), - ...Object.values(config.geometryInfluence), - ...Object.values(config.termCoverageInfluence), - ...Object.values(config.pairwiseAgreementInfluence), - ...Object.values(config.denseConfidenceInfluence), - ...Object.values(config.identifierInfluence), - ...Object.values(config.queryLengthInfluence), + ...sortedValues(config.baseWeights), + ...sortedValues(config.scoreInfluence), + ...sortedValues(config.geometryInfluence), + ...sortedValues(config.termCoverageInfluence), + ...sortedValues(config.pairwiseAgreementInfluence), + ...sortedValues(config.denseConfidenceInfluence), + ...sortedValues(config.identifierInfluence), + ...sortedValues(config.queryLengthInfluence), ]🤖 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 `@benchmarks/retrieval/evaluation/promotion-evidence.ts` around lines 131 - 158, Update configValues and differingCoordinates so configuration coordinates are derived deterministically by channel name: sort keys explicitly within each influence record before flattening, and ensure both configurations use the same coordinate ordering rather than relying on insertion order. Handle differing or missing channel keys by comparing aligned named coordinates instead of indexing one array and defaulting absent right-side values to zero.
160-172: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive selection identity from stable coordinates, not
JSON.stringify.
JSON.stringify(row.config)is sensitive to key insertion order. Two identical candidate configurations with different key order would count as two distinct selections and lowerselectionFrequency. This shares the root cause with the positional comparison inconfigValues. Reuse one canonical coordinate representation for both selection identity and neighbor distance.🤖 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 `@benchmarks/retrieval/evaluation/promotion-evidence.ts` around lines 160 - 172, Update selectionFrequency to derive each selection key from the same canonical coordinate representation used by configValues for neighbor distance, rather than JSON.stringify(row.config). Reuse the shared stable coordinate helper or representation so equivalent configurations with different key insertion order produce identical identities, while preserving the existing frequency counts and return shape.benchmarks/retrieval/evaluation/search.ts (1)
539-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
expectedStrategiesrule inbenchmarks/retrieval/evaluation/search.ts. Both search stages recompute the same required-strategy rule fromconfig.repositoryHoldoutsand the per-model distinct repository count. The shared root cause is one eligibility rule expressed twice. If one copy changes, fusion candidates and evidence-router candidates apply different promotion requirements, and the artifact reports inconsistent promotion statuses.
benchmarks/retrieval/evaluation/search.ts#L539-L545: replace the inlined computation with a call to a single module-level helper, then pass its result toderivePromotionEvidence.benchmarks/retrieval/evaluation/search.ts#L404-L410: replacehasRepositoryHoldoutsand theexpectedStrategiesternary with a call to the same helper.♻️ Proposed shared helper
const requiredStrategies = ( config: BenchmarkSearchConfig, samplesByModel: ReadonlyMap<string, readonly WeightSearchSample[]>, groupedStrategy: ValidationStrategy, ): readonly ValidationStrategy[] => { const hasRepositoryHoldouts = [...samplesByModel.values()].some( (samples) => new Set(samples.map((sample) => sample.repository)).size > 1, ) return config.repositoryHoldouts && hasRepositoryHoldouts ? [groupedStrategy, "leave-one-repository-out"] : [groupedStrategy] }🤖 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 `@benchmarks/retrieval/evaluation/search.ts` around lines 539 - 545, Extract a module-level requiredStrategies helper in benchmarks/retrieval/evaluation/search.ts that accepts config, samplesByModel, and groupedStrategy and centralizes the repository-holdout eligibility rule. At lines 404-410, replace hasRepositoryHoldouts and the expectedStrategies ternary with this helper call; at lines 539-545, replace the duplicated computation with the same helper result before passing it to derivePromotionEvidence.
🤖 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 `@benchmarks/retrieval/runner.ts`:
- Around line 222-226: Update finalTest.fold in the finalTest configuration to
match the grouped fold labels emitted by assignGroupedFolds, using the
configured fold count plus one; alternatively, adjust the promotion lookup to
use the same assignment range. Ensure the final-test lookup resolves the label
assigned to the final grouped fold.
In `@benchmarks/tests/promotion-evidence.test.ts`:
- Around line 123-125: Update the assertion covering evidence.uncertainty to
require that the array is non-empty in addition to verifying every interval has
bootstrapSamples equal to 1_000, preventing an empty array from passing
vacuously.
---
Nitpick comments:
In `@benchmarks/retrieval/evaluation/promotion-evidence.ts`:
- Around line 73-93: Add an observation-count field to HoldoutUncertainty and
populate it in bootstrapInterval using deltas.length, including the empty-input
return. Ensure the generated report exposes this count alongside
bootstrapSamples so single-fold intervals are distinguishable from genuinely
narrow intervals.
- Around line 131-158: Update configValues and differingCoordinates so
configuration coordinates are derived deterministically by channel name: sort
keys explicitly within each influence record before flattening, and ensure both
configurations use the same coordinate ordering rather than relying on insertion
order. Handle differing or missing channel keys by comparing aligned named
coordinates instead of indexing one array and defaulting absent right-side
values to zero.
- Around line 160-172: Update selectionFrequency to derive each selection key
from the same canonical coordinate representation used by configValues for
neighbor distance, rather than JSON.stringify(row.config). Reuse the shared
stable coordinate helper or representation so equivalent configurations with
different key insertion order produce identical identities, while preserving the
existing frequency counts and return shape.
In `@benchmarks/retrieval/evaluation/search.ts`:
- Around line 539-545: Extract a module-level requiredStrategies helper in
benchmarks/retrieval/evaluation/search.ts that accepts config, samplesByModel,
and groupedStrategy and centralizes the repository-holdout eligibility rule. At
lines 404-410, replace hasRepositoryHoldouts and the expectedStrategies ternary
with this helper call; at lines 539-545, replace the duplicated computation with
the same helper result before passing it to derivePromotionEvidence.
In `@benchmarks/tests/promotion-evidence.test.ts`:
- Around line 95-134: Add a test case alongside the existing promotion evidence
test using rows that include every required strategy but omit the requested
final-test fold, then call derivePromotionEvidence with the mismatched
strategy/fold selector. Assert finalTest.present is false and promotionStatus is
"no-eligible-candidate", while preserving the existing eligible-path coverage.
In `@benchmarks/tests/retrieval.test.ts`:
- Around line 136-138: Add assertions alongside the promotionEvidence
cardinality check in the retrieval integration test to verify the final-test
result was found: assert finalTest.present is true and stability.folds contains
the expected untouched final fold label. Keep the existing cardinality assertion
unchanged and use the result object’s existing finalTest and stability fields.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bf1bf3b9-1c64-47cb-9eff-10c4b5750fc4
📒 Files selected for processing (11)
CONTEXT.mdbenchmarks/README.mdbenchmarks/retrieval/evaluation/promotion-evidence.tsbenchmarks/retrieval/evaluation/report.tsbenchmarks/retrieval/evaluation/search.tsbenchmarks/retrieval/evaluation/types.tsbenchmarks/retrieval/evaluation/weight-search.tsbenchmarks/retrieval/runner.tsbenchmarks/tests/channels.test.tsbenchmarks/tests/promotion-evidence.test.tsbenchmarks/tests/retrieval.test.ts
| finalTest: { | ||
| kind: "untouched-grouped-fold", | ||
| strategy: groupedStrategy, | ||
| fold: String(config.groupedFolds), | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect grouped fold label generation and evidence-router job fold values.
set -euo pipefail
fd -e ts . benchmarks --exec rg -n -C4 'assignGroupedFolds|groupedFold|fold:' {}Repository: Lucas-Bur/pix
Length of output: 24050
🏁 Script executed:
#!/bin/bash
# Inspect grouped fold assignment bounds and promotion final-test match behavior.
set -euo pipefail
printf '--- assignGroupedFolds implementation ---\n'
cat -n benchmarks/retrieval/evaluation/folds.ts | sed -n '89,150p'
printf '\n--- promotion evidence implementation ---\n'
cat -n benchmarks/retrieval/evaluation/promotion-evidence.ts | sed -n '220,280p'
printf '\n--- static verifier for labels / final-test match ===\n'
python3 - <<'PY'
import re
from pathlib import Path
src = Path('benchmarks/retrieval/evaluation/folds.ts').read_text()
matches = re.findall(r'for\s*\([^)]+\)\s*\{\s*folds\.set\(\([^)]*\),\s*(\d+)\s*\)', src, re.S)
print('literal-fold assignments found:', ', '.join(matches))
assign = src[min(src.index('export const assignGroupedFolds'), len(src)-1):len(src)]
print('fold assignment increments zero to N-1:', 'for (let fold = 0; fold < foldCount; fold++)' in assign)
print('finalTest fold construction:', 'String(config.groupedFolds)' in Path('benchmarks/retrieval/runner.ts').read_text())
PYRepository: Lucas-Bur/pix
Length of output: 242
🏁 Script executed:
#!/bin/bash
# Inspect grouped fold assignment bounds and promotion final-test match behavior.
set -euo pipefail
printf '%s\n' '--- assignGroupedFolds implementation ---'
cat -n benchmarks/retrieval/evaluation/folds.ts | sed -n '89,150p'
printf '%s\n' ''
printf '%s\n' '--- promotion evidence implementation ---'
cat -n benchmarks/retrieval/evaluation/promotion-evidence.ts | sed -n '220,280p'
printf '%s\n' ''
printf '%s\n' '--- static verifier for labels / final-test match ==='
python3 - <<'PY'
import re
from pathlib import Path
src = Path('benchmarks/retrieval/evaluation/folds.ts').read_text()
print('assignment literal after fold set:', re.search(r'folds\.set\([^)]*,\s*(\d+)\s*\)', src).group(1))
assign = src[src.index('export const assignGroupedFolds'):]
print('fold assignment increments zero to N-1:', 'for (let fold = 0; fold < foldCount; fold++)' in assign)
runner = Path('benchmarks/retrieval/runner.ts').read_text()
print('finalTest fold construction:', 'fold: String(config.groupedFolds)' in runner)
PYRepository: Lucas-Bur/pix
Length of output: 3337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all fold assignment literals ---'
awk '
/folds\.set/ { found=1 }
found { print }
/^\}/ && found { found=0 }
' benchmarks/retrieval/evaluation/folds.ts
printf '%s\n' ''
printf '%s\n' '--- relevant runner/evidence protocol config ---'
cat -n benchmarks/retrieval/runner.ts | sed -n '200,235p'Repository: Lucas-Bur/pix
Length of output: 1735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all folds.ts literal usages and foldCount references ---'
rg -n 'fold|groups|counts|introduc|strat|selectFold|RecordFromKeys' benchmarks/retrieval/evaluation/folds.ts
printf '%s\n' ''
printf '%s\n' '--- inspect lines around selectFold implementation ---'
cat -n benchmarks/retrieval/evaluation/folds.ts | sed -n '90,140p'Repository: Lucas-Bur/pix
Length of output: 2855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fold selection initialization ---'
cat -n benchmarks/retrieval/evaluation/folds.ts | sed -n '1,80p'
printf '%s\n' ''
printf '%s\n' '--- creation calls and artifact validation tests ---'
rg -n -C3 'assignGroupedFolds|finalTest:|fold: String\(config\.groupedFolds\)|String\(groupedFolds\)' benchmarks/retrieval tests benchmarks --glob '*.ts'Repository: Lucas-Bur/pix
Length of output: 16523
Align finalTest.fold with the emitted grouped fold labels.
assignGroupedFolds can choose fold label 0 for strata, but the final-test lookup uses String(config.groupedFolds) ("3" or "5"). Rename this to String(config.groupedFolds + 1), or have promotion lookup match the assignment range.
🤖 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 `@benchmarks/retrieval/runner.ts` around lines 222 - 226, Update finalTest.fold
in the finalTest configuration to match the grouped fold labels emitted by
assignGroupedFolds, using the configured fold count plus one; alternatively,
adjust the promotion lookup to use the same assignment range. Ensure the
final-test lookup resolves the label assigned to the final grouped fold.
| expect(evidence?.uncertainty.every((interval) => interval.bootstrapSamples === 1_000)).toBe( | ||
| true, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that uncertainty is not empty.
Array.prototype.every returns true for an empty array. If buildUncertainty regressed and returned no intervals, this assertion would still pass. Add a length check so the test pins the uncertainty contract.
💚 Proposed fix for the assertion
+ expect(evidence?.uncertainty.length).toBeGreaterThan(0)
expect(evidence?.uncertainty.every((interval) => interval.bootstrapSamples === 1_000)).toBe(
true,
)📝 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.
| expect(evidence?.uncertainty.every((interval) => interval.bootstrapSamples === 1_000)).toBe( | |
| true, | |
| ) | |
| expect(evidence?.uncertainty.length).toBeGreaterThan(0) | |
| expect(evidence?.uncertainty.every((interval) => interval.bootstrapSamples === 1_000)).toBe( | |
| true, | |
| ) |
🤖 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 `@benchmarks/tests/promotion-evidence.test.ts` around lines 123 - 125, Update
the assertion covering evidence.uncertainty to require that the array is
non-empty in addition to verifying every interval has bootstrapSamples equal to
1_000, preventing an empty array from passing vacuously.
1 participant
Summary
Validation
Closes #171
Summary by CodeRabbit
New Features
Improvements
Documentation