fix(lab): read per-model overrides in the report the way the runtime reads them - #2077
fix(lab): read per-model overrides in the report the way the runtime reads them#2077ntdatt812 wants to merge 2 commits into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds family-aware, case-insensitive own-property lookup for model behavior overrides and exact-own lookup for selected routing maps. Tests cover matching, prototype-shaped model IDs, routing overrides, hosted-tool preferences, and fingerprint computation. ChangesModel override resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change aligns behavior-report model override lookups with runtime resolution and adds targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Reviewed as part of a four-PR batch (#2077, #2085, #2086, #2100) applying the same The prototype-chain defect is real and worth fixing. A bare index walks the prototype chain, so a routed model id of Holding on scope. One missed read: Suggested shape: One correction for the description: the throw is caught at |
|
Reviewed as part of a four-PR batch with #2085, #2086, and #2100. Verdict: hold — the migration is too broad, and it violates the contract it is enforcing. First, the good part: the prototype-chain defect is real, and for the nine maps the runtime genuinely resolves through The problem.
To land: use One correction to the description, since it will end up in the commit message: the throw is caught at Also missing: coverage for the |
리뷰 · 우선순위 47 / 80#2059가 리스트 절반을 맞춘 뒤, 리포트의 per-model 오버라이드 맵이 런타임 코드는 테스트는 와이어 #2100/#2085/#2086과 같은 배치로 보면 안 된다. 여기 hold 이유는 이 헬퍼가 두 계약을 한 함수에 넣었다는 점이다. 해결방안
이 댓글은 grok-bot이 작성했습니다 |
Review feedback on lidge-jun#2077: routing every override map through `modelRecordValue` was too broad, and for two of them it inverted the very contract the PR is enforcing. `modelPreferHostedTools` and `modelOpenRouterRouting` are deliberately exact at runtime. The adapter reads the first through `hasOwnProperty` (src/adapters/openai-responses.ts:1001) and `resolveOpenRouterRouting` reads the second through `Object.hasOwn` (src/providers/openrouter-routing.ts:89); the type documents the first as "Exact-model hosted tools" (src/types.ts:1584). Family-resolving them would make the report say a `gpt-oss` entry applies to `gpt-oss:120b` when the adapter never applies it -- the same divergence this PR removes, pointed the other way. A bare index is not the answer for those two either: it walks the prototype chain, which is the defect `modelValue` was changed to fix. `modelOpenRouterRouting` was still a bare read at behavior.ts:87 and therefore still carried that bug. Neither existing primitive fits, so this adds a third: `exactOwnValue`. Split is now nine family-aware maps through `modelValue`, two exact-own maps through `exactOwnValue`. Five tests. The two "report agrees" cases are red if either map is sent back through `modelRecordValue`; the ground-truth and control cases pass either way by design. Ground truth for the routing half is executable -- `resolveOpenRouterRouting` is exported, and it returns the entry for the exact key and undefined for the tagged sibling. Also corrects the docblock's control flow, which the review flagged: the throw is caught at src/routing/compatibility/subject.ts:125, which returns no route. `resolvePassiveRouteSubjectId`'s catch is a second backstop and does not see it.
f4213f2 to
ab42ba0
Compare
|
@lidge-jun the hold is correct on every point, and all four are fixed in The scope objection was right, and it was the sharper half of the review. And you caught one I missed: So the split is now nine family-aware maps through
That is the concrete cost of getting it wrong: an override the adapter will never apply, silently moving the subject identity. Coverage for the behaviour change is five new tests. The two "report agrees" cases go red if either map is sent back through The description correction is in too. You were right: the throw is caught at Corrected counts: the file is 21 tests, 13 pass / 8 fail against current Rebased onto Full One thing I found and deliberately did not touch. |
…reads them lidge-jun#2059 fixed the list-shaped half of the behavior report. The per-model maps are the other half, and they had the same shape of bug plus one more. `modelValue` was a bare index, `map?.[modelId]`. The runtime reads these maps through `modelRecordValue`, which checks own properties, then the pre-colon family, then a case-folded key. So the report disagreed three ways: - ollama-cloud serves `gpt-oss:120b`. With `modelMaxOutputTokens: {"gpt-oss": 1234}` the adapter puts `max_tokens: 1234` on the wire, while the report said `limits.maxOutputTokens: null`. - a differently-cased key resolved at runtime and not in the report. - the index walked the prototype chain. Model ids are operator-controlled, so one can be `constructor` or `toString`, and the row then held an Object.prototype *function*. That last one is not merely wrong data. `jcsStringify` rejects a function, so `buildBehaviorFingerprintV1` threw `unsupported value type function`, and `resolvePassiveRouteSubjectId` swallows the throw -- the subject silently never links, and Lab loses that traffic with no diagnostic. The linker's own contract says a registered implementation is "synchronous, free of side effects with respect to the request, and non-throwing"; the try/catch is the backstop, not a licence. openai-responses.ts already guards `modelPreferHostedTools` against exactly this, with a comment saying why. Nine of the ten maps the report reads are read through `modelRecordValue` at runtime; delegating to it makes the report agree with all of them at once. Fingerprints, same config, before and after: gpt-oss:120b 54154e19bd2c8ee4 -> 5c992edff35bd7e9 (was missing 1234) gpt-oss 5c992edff35bd7e9 -> 180a84b2d837619d (was missing 55555) glm-5.3 54154e19bd2c8ee4 -> 54154e19bd2c8ee4 (unchanged) constructor THROW -> 54154e19bd2c8ee4 (now computable) Only subjects whose overrides were being missed move; `resolverVersion` stays at 2 for the reason given in lidge-jun#2059. Tests extend the file lidge-jun#2059 added: the adapter's wire is asserted first, then the report is held to it, and the prototype-shaped ids get their own cases including one that the fingerprint stays computable. Seven of the nine fail on current dev. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback on lidge-jun#2077: routing every override map through `modelRecordValue` was too broad, and for two of them it inverted the very contract the PR is enforcing. `modelPreferHostedTools` and `modelOpenRouterRouting` are deliberately exact at runtime. The adapter reads the first through `hasOwnProperty` (src/adapters/openai-responses.ts:1001) and `resolveOpenRouterRouting` reads the second through `Object.hasOwn` (src/providers/openrouter-routing.ts:89); the type documents the first as "Exact-model hosted tools" (src/types.ts:1584). Family-resolving them would make the report say a `gpt-oss` entry applies to `gpt-oss:120b` when the adapter never applies it -- the same divergence this PR removes, pointed the other way. A bare index is not the answer for those two either: it walks the prototype chain, which is the defect `modelValue` was changed to fix. `modelOpenRouterRouting` was still a bare read at behavior.ts:87 and therefore still carried that bug. Neither existing primitive fits, so this adds a third: `exactOwnValue`. Split is now nine family-aware maps through `modelValue`, two exact-own maps through `exactOwnValue`. Five tests. The two "report agrees" cases are red if either map is sent back through `modelRecordValue`; the ground-truth and control cases pass either way by design. Ground truth for the routing half is executable -- `resolveOpenRouterRouting` is exported, and it returns the entry for the exact key and undefined for the tagged sibling. Also corrects the docblock's control flow, which the review flagged: the throw is caught at src/routing/compatibility/subject.ts:125, which returns no route. `resolvePassiveRouteSubjectId`'s catch is a second backstop and does not see it.
ab42ba0 to
7d6d3a6
Compare
|
Rebased again onto
A heads-up that is not about this PR. The full Both assert
It widened Worth knowing that I am on Happy to open this as its own issue if you would rather not have it sitting in a PR thread. |
|
Correction to my previous comment — I named the wrong commit, and the wrong author. I attributed the two Re-bisected with both endpoints measured rather than assumed: That commit adds Instrumented at the gate to confirm rather than infer: So this is Windows-only, it is not a WS-path defect, and it is not @Ingwannu's change. Still unrelated to this PR — but my earlier account of it was wrong and I would rather correct it in the same place I got it wrong. |
|
Thanks for this, @ntdatt812 — closing as superseded by #2140, which carries your patch applied unchanged, alongside your #2077 for the Lab behavior report. They are combined because they are one thesis on two disjoint files: model-keyed lookups must use the runtime's own resolution rules. Your tests came across verbatim, including the prototype-id cases, which are the load-bearing part — reverting just the two source files fails 13 of them. Your work is credited in that PR's description. |
2 participants
Summary
#2059 fixed the list-shaped half of the behavior report. The per-model override maps are the other half, and they had the same shape of bug plus one more.
modelValuewas a bare index:The runtime reads these maps through
modelRecordValue(src/reasoning-effort.ts:73), which checks own properties, then the pre-colon family, then a case-folded key. Nine of the ten maps the report reads go through it at runtime —modelContextWindows,modelMaxInputTokens,modelMaxOutputTokens,modelInputModalities,modelReasoningEfforts,modelDefaultReasoningEfforts,modelReasoningEffortMap,modelSupportsReasoningSummaries,modelReasoningSummaryDelivery. The tenth,modelPreferHostedTools, is read inopenai-responses.tswith its own explicithasOwnPropertyguard.The three disagreements
1. Pre-colon family. ollama-cloud serves
gpt-oss:120b. WithmodelMaxOutputTokens: {"gpt-oss": 1234}:max_tokens: 1234limits.maxOutputTokensnull2. Case folding. A differently-cased key resolves at runtime and did not in the report.
3. Prototype chain — this one is not just wrong data.
Model ids are operator-controlled, so one can be
constructorortoString. The bare index then returned anObject.prototypefunction:jcsStringifyrejects a function, sobuildBehaviorFingerprintV1threwunsupported value type function, andresolvePassiveRouteSubjectIdswallows the throw — the subject silently never links and Lab loses that traffic with no diagnostic. The linker's contract states an implementation is "synchronous, free of side effects with respect to the request, and non-throwing"; the try/catch is described there as belonging to the mechanism so the guarantee is not restated by callers — a backstop, not a licence.openai-responses.ts:996-1001already guardsmodelPreferHostedToolsagainst exactly this, and says why in a comment.Change
Two lookups, because the ten maps are not one contract.
modelValuedelegates tomodelRecordValuefor the nine family-aware maps, so the report cannot disagree with the runtime on any of them at once.A new
exactOwnValueserves the two that are deliberately exact-own at runtime:modelPreferHostedTools, read throughhasOwnPropertyatsrc/adapters/openai-responses.ts:1001and documented as "Exact-model hosted tools" atsrc/types.ts:1584, andmodelOpenRouterRouting, read throughObject.hasOwnatsrc/providers/openrouter-routing.ts:89.Family-resolving those two would be this PR's own divergence with the sign flipped — the report claiming an override applies that the adapter will never apply. A bare index is not the alternative either: it walks the prototype chain, which is the defect being fixed.
modelOpenRouterRoutingatbehavior.ts:87was still a bare read, so it carried that bug untouched by the first commit.Measured, first 16 hex of the behavior fingerprint, with
modelPreferHostedTools: {"gpt-oss": ["image_generation"]}andmodelOpenRouterRouting: {"gpt-oss": {order: ["fireworks"]}}present:gpt-oss:120bmodelRecordValue96a2ad0adbcae1deexactOwnValue5c992edff35bd7e9Blast radius, measured
Same config, before and after, first 16 hex of the behavior fingerprint:
gpt-oss:120b54154e19bd2c8ee45c992edff35bd7e9gpt-oss5c992edff35bd7e9180a84b2d837619dglm-5.354154e19bd2c8ee454154e19bd2c8ee4constructor54154e19bd2c8ee4Re-measured on the rebased head; the exact-own split does not move any of these, because none of those configs carries a hosted-tools or OpenRouter-routing override.
Only subjects whose overrides were being missed move, so
resolverVersionstays at2for the same reason as #2059 — say the word if you would rather draw a generation boundary.Tests
Extends
tests/routing-compatibility-model-matching.test.tsfrom #2059, same pattern: assert the wire the adapter really builds, then hold the report to it. Prototype-shaped ids get their own cases, including one asserting the fingerprint stays computable and that two such ids hash alike.The exact-own group adds five more. Their ground truth is executable rather than cited:
resolveOpenRouterRoutingis exported, and it returns the entry for the exact key andundefinedfor the tagged sibling.Whole file, 21 tests, against current
dev: 13 pass / 8 fail. Against this branch's first commit — i.e. with both exact-own maps wrongly family-resolved — the two "report agrees" cases go red instead. Both numbers matter: the first shows the original defect, the second shows the correction to it.Verification
Rebased onto
7a2d13a74; the branch was 31 commits behind.Not the full suite: this is a Windows machine and
bun run testpanics partway through on Bun 1.3.14 (index out of bounds: index 0, len 0), so a result from it would be a truncated log. The batch ran throughscripts/test.tsso each file keeps its isolated home.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Tests