Repository navigation
feat(router-eval): add retained optimization gate - #6071
KooshaPari wants to merge 16 commits into
Conversation
…ation (diegosouzapw#6048) Port the release/v3.8.44 fix to main so the code-scanning alert closes on the default branch. Parse the URL and assert on the exact hostname instead of a substring match — `includes("www.kimi.com")` would also accept a hostile host like `www.kimi.com.evil.net` or `evil.net/?x=www.kimi.com` (js/incomplete-url-substring-sanitization).
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive router evaluation framework (router-eval), adding scripts for running, comparing, searching, and checking regressions, along with core library logic, fixtures, and unit tests. The review feedback highlights several critical issues: a potential crash (unhandled ENOENT) in the regression check script when patch files are missing on failure, a TypeScript compilation error in src/lib/routerEval/index.ts from passing an extra argument to asRecord, a potential NaN bug when calculating cost deltas with a zero baseline, a CLI hanging issue when running interactively without piped input, and a Repository Style Guide violation regarding the placement of scripts under the unauthorized scripts/router-eval/ directory.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function main(): void { | ||
| const args = readArgs(); | ||
| ensureReadable(args.baseline, "baseline corpus"); | ||
| ensureReadable(args.candidate, "candidate corpus"); | ||
| fs.mkdirSync(path.dirname(args.output), { recursive: true }); | ||
| fs.mkdirSync(path.dirname(args.jsonOutput), { recursive: true }); |
There was a problem hiding this comment.
If the regression gate fails (result.status !== 0), runPatchGate is skipped, meaning ensureReadable is never called on the patch files. However, writeRetainedRun is still called and attempts to copy the patch files if they are specified in args. If these files do not exist or are unreadable, fs.copyFileSync will throw an unhandled ENOENT exception, crashing the script instead of exiting cleanly with the regression exit code.
Validating and ensuring readability of the patch files at the start of main() resolves this robustness issue.
function main(): void {
const args = readArgs();
ensureReadable(args.baseline, "baseline corpus");
ensureReadable(args.candidate, "candidate corpus");
if (args.baselinePatch || args.candidatePatch) {
if (!args.baselinePatch || !args.candidatePatch) {
console.error("[router-eval] --baseline-patch and --candidate-patch must be provided together");
process.exit(2);
}
ensureReadable(args.baselinePatch, "baseline patch");
ensureReadable(args.candidatePatch, "candidate patch");
}
fs.mkdirSync(path.dirname(args.output), { recursive: true });
fs.mkdirSync(path.dirname(args.jsonOutput), { recursive: true });| asNumber(value.status, 500) >= 200 && asNumber(value.status, 500) < 400 && !value.error | ||
| ); | ||
| const timestamp = asString(value.timestamp, new Date(0).toISOString()); | ||
| const routeInput = asRecord(value.routeInput, {}); |
There was a problem hiding this comment.
The helper function asRecord is defined to accept only a single argument (value: unknown). Passing a second argument {} here will cause a TypeScript compilation error under strict type checking. Please remove the redundant second argument.
| const routeInput = asRecord(value.routeInput, {}); | |
| const routeInput = asRecord(value.routeInput); |
| candidate, | ||
| delta: { | ||
| aiq, | ||
| costUsd: costIncrease * baselineCost, |
There was a problem hiding this comment.
If baselineCost is 0 and cost is positive, costIncrease is evaluated as Infinity. Multiplying costIncrease * baselineCost (Infinity * 0) results in NaN for delta.costUsd in the comparison report.
Since costIncrease * baselineCost mathematically simplifies to cost - baselineCost, calculating the absolute cost delta directly avoids this edge-case NaN bug and simplifies the logic.
| costUsd: costIncrease * baselineCost, | |
| costUsd: cost - baselineCost, |
| @@ -0,0 +1,458 @@ | |||
| #!/usr/bin/env bun | |||
There was a problem hiding this comment.
According to the Repository Style Guide (Rule 1), all scripts must be placed strictly inside one of the allowed scripts/ subfolders: build/, dev/, check/, docs/, i18n/, or ad-hoc/. The new scripts/router-eval/ subfolder is not in this list. Please move these scripts (e.g., compare.ts, index.ts, patch-compare.ts, search.ts, trends.ts) to an allowed subfolder (such as scripts/ad-hoc/router-eval/) and update the corresponding scripts in package.json to adhere to the style guide.
References
- ALL maintenance, debugging, generation, or experimental scripts MUST be placed strictly inside one of the scripts/ subfolders (build/, dev/, check/, docs/, i18n/, ad-hoc/). (link)
| const candidate: RouterObservation[] = args.input | ||
| ? await readJsonl(args.input) | ||
| : hasCandidateDb | ||
| ? readDb(resolveDbPath(args.db), args.since, args.limit, args.dbSource) | ||
| : await readJsonl(); |
There was a problem hiding this comment.
When running the script interactively without any piped input, calling await readJsonl() without arguments will cause the CLI to hang indefinitely waiting for stdin EOF. Checking process.stdin.isTTY allows the script to exit cleanly with an error message when no input is provided.
const candidate: RouterObservation[] = args.input
? await readJsonl(args.input)
: hasCandidateDb
? readDb(resolveDbPath(args.db), args.since, args.limit, args.dbSource)
: process.stdin.isTTY
? []
: await readJsonl();…pw#6098, closes diegosouzapw#6062) Externalize ws / bufferutil / utf-8-validate in serverExternalPackages so the copilot-m365-web WebSocket masking path works at runtime (bundling ws → TypeError: b.mask is not a function → 80s chat timeout). Regression guard in next-config.test.ts. Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>
…egosouzapw#6098, closes diegosouzapw#6062)" This reverts commit e61b75f.
…egosouzapw#6123) Bumps [github/codeql-action/init](https://github.com/github/codeql-action) from 4.36.2 to 4.36.3. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@8aad20d...54f647b) --- updated-dependencies: - dependency-name: github/codeql-action/init dependency-version: 4.36.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/cache](https://github.com/actions/cache) from 6.0.0 to 6.1.0. - [Release notes](https://github.com/actions/cache/releases) - [Commits](actions/cache@v6...v6.1.0) --- updated-dependencies: - dependency-name: actions/cache dependency-version: 6.1.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…iegosouzapw#6125) Bumps [github/codeql-action/analyze](https://github.com/github/codeql-action) from 4.36.2 to 4.36.3. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@8aad20d...54f647b) --- updated-dependencies: - dependency-name: github/codeql-action/analyze dependency-version: 4.36.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ned-optimization-gate-clean
…-eval-retained-optimization-gate-clean # Conflicts: # bin/cli/commands/setup-claude.mjs # config/quality/file-size-baseline.json # open-sse/services/combo.ts # open-sse/services/combo/comboStructure.ts # tests/unit/cli/setup-claude.test.ts
|
Thank you, @KooshaPari 🙏 — closing as deferred, not rejected. This PR is the router-eval optimization gate (~6k LOC) — a 3.9.0-scale eval subsystem. It's one piece of the native-router backend workstream tracked in #5670, where I've cataloged all of these as the staged backlog for the 3.9.0/4.0 window (see my comment there). Merging these subsystem pieces individually into the hot |
Summary
Validation