refactor: dedupe computeSavings via pct helper and persist partial token-benchmark results#1875
Conversation
…ken-benchmark results computeSavings reimplemented the percentage-reduction formula inline instead of using the pct helper already defined for computeAggregate. Both call sites now share pct — verified byte-identical output across normal, zero-denominator, negative-savings, and null-guard cases. main()'s per-issue loop only serialized results to stdout after the full loop completed, so a crash partway through (another issue throwing, or the optional --perf benchmarks failing) discarded every already-computed result. The loop now overwrites token-benchmark.partial.json after each issue; the file is removed once the full run succeeds and the final JSON has been printed. Fixes #1757 Impact: 5 functions changed, 17 affected
Greptile SummaryThis PR promotes the
Confidence Score: 4/5Safe to merge; the refactor is behavior-preserving and the persistence logic works correctly for its primary goal of surviving crashes in runIssueExperiment. The scripts/token-benchmark.ts — specifically the Important Files Changed
Reviews (1): Last reviewed commit: "refactor: dedupe computeSavings via pct ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function writePartialResults(results, totalIssues) { | ||
| const partialOutput = { | ||
| version: benchVersion, | ||
| date: new Date().toISOString().slice(0, 10), | ||
| model: MODEL, | ||
| runsPerIssue: RUNS, | ||
| maxTurns: MAX_TURNS, | ||
| maxBudgetUsd: MAX_BUDGET, | ||
| partial: true, | ||
| completedIssues: results.length, | ||
| totalIssues, | ||
| issues: results, | ||
| aggregate: computeAggregate(results), | ||
| perfBenchmarks: null, | ||
| }; | ||
| fs.writeFileSync(PARTIAL_RESULTS_PATH, JSON.stringify(partialOutput, null, 2)); | ||
| console.error( | ||
| ` Partial results written to ${PARTIAL_RESULTS_PATH} (${results.length}/${totalIssues} issues)`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
fs.writeFileSync throws (disk full, permission error, path not writable), the exception propagates out of the for loop through main() and into .catch(), which kills the process. The in-memory result that was just pushed to results — and any that haven't been written to a previous partial file — is lost. Wrapping in try/catch lets the run continue and logs a warning instead of aborting.
| function writePartialResults(results, totalIssues) { | |
| const partialOutput = { | |
| version: benchVersion, | |
| date: new Date().toISOString().slice(0, 10), | |
| model: MODEL, | |
| runsPerIssue: RUNS, | |
| maxTurns: MAX_TURNS, | |
| maxBudgetUsd: MAX_BUDGET, | |
| partial: true, | |
| completedIssues: results.length, | |
| totalIssues, | |
| issues: results, | |
| aggregate: computeAggregate(results), | |
| perfBenchmarks: null, | |
| }; | |
| fs.writeFileSync(PARTIAL_RESULTS_PATH, JSON.stringify(partialOutput, null, 2)); | |
| console.error( | |
| ` Partial results written to ${PARTIAL_RESULTS_PATH} (${results.length}/${totalIssues} issues)`, | |
| ); | |
| } | |
| function writePartialResults(results, totalIssues) { | |
| const partialOutput = { | |
| version: benchVersion, | |
| date: new Date().toISOString().slice(0, 10), | |
| model: MODEL, | |
| runsPerIssue: RUNS, | |
| maxTurns: MAX_TURNS, | |
| maxBudgetUsd: MAX_BUDGET, | |
| partial: true, | |
| completedIssues: results.length, | |
| totalIssues, | |
| issues: results, | |
| aggregate: computeAggregate(results), | |
| perfBenchmarks: null, | |
| }; | |
| try { | |
| fs.writeFileSync(PARTIAL_RESULTS_PATH, JSON.stringify(partialOutput, null, 2)); | |
| console.error( | |
| ` Partial results written to ${PARTIAL_RESULTS_PATH} (${results.length}/${totalIssues} issues)`, | |
| ); | |
| } catch (err) { | |
| console.error(` Warning: could not write partial results — ${err.message}`); | |
| } | |
| } |
Codegraph Impact Analysis5 functions changed → 17 callers affected across 5 files
|
1 participant
Summary
computeSavingsreimplemented the percentage-reduction formula inline instead of using thepcthelper already defined forcomputeAggregate. Promotedpctto module level and adopted it in both places — confirmed bit-for-bit identical behavior via a 10-case equivalence harness (normal, zero-cost, negative savings, zero/negative baseline, rounding edge cases).main()'s per-issue loop only serializedresultsafter the full loop completed, so a crash partway through discarded all already-computed results. AddedwritePartialResults, called after each issue, overwriting atoken-benchmark.partial.jsonsnapshot; removed only after the full run (including optional perf benchmarks) succeeds, so a late failure still leaves the per-issue safety net intact.Closes #1757
Test plan
npm test— full suite green (3519 passed, 0 failed) — this script has no existing test coverage (confirmed via grep)npx tsc --noEmit— cleanscripts/is outside Biome's lint scope by design (biome.json'sfiles.includesissrc/**/tests/**) — confirmed no lint tool applies hereStacked on #1874 (base branch
fix/issue-1756-dedupe-impact-level-rendering) — only the token-benchmark.ts/.gitignore diff is this issue's change.