fix(lab): report the model gates the way the adapters actually match them - #2059
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 (2)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe routing compatibility check now matches tagged model IDs against bare entries in provider lists. Tests validate the ChangesModel List Compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized fix aligns reported model-gate values with adapter behavior and includes targeted passing tests; no actionable merge-blocking risk remains beyond 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 |
|
Checklist is at 3/4. Box 1 is open on purpose rather than left over — I would rather ask than tick something I cannot stand behind. "All CI tests are green on my local testing" — this is a Windows machine. What I did run on the exact head of this branch is listed under Verification in the description — typecheck clean, the new tests green, and a batch of the surrounding suite green, plus the same tests run against unpatched If box 1 is meant as "the tests you ran locally are green", that is true and I will tick it on a word from you. If it is meant literally as the full suite, it cannot be honestly ticked from Windows and I would rather it stay open than be wrong. |
…them
`resolveProductionBehaviorValues` is documented as "authoritative effective
values emitted by the production route/model/adapter resolver", and its hash
is the behavior fingerprint that keys Lab evidence. Ten of its rows are
membership tests over the provider's `no*Models`-style lists, and they went
through a local
function includesModel(list, modelId) {
return Array.isArray(list) && list.includes(modelId);
}
while every runtime gate those rows describe matches through `modelInList`,
which also accepts a bare entry for a tagged id.
ollama-cloud serves `gpt-oss:120b`, `qwen3-coder:480b`, `qwen3.5:397b` and
`gemma4:31b`, and the same registry row writes the bare `gpt-oss` into
noVisionModels -- the bare-prefix form is how these lists are meant to be
written. So with `noTemperatureModels: ["gpt-oss"]` the adapter omitted
temperature from the request for `gpt-oss:120b` while the report said
`sampling.omitTemperature: false`. Same for omitTopP, omitPenalties,
reasoning.budgetMode, reasoning.splitMode, reasoning.toggleMode,
reasoning.supported, reasoning.replayMode's two flags, and
tools.choiceRestrictions.
The blast radius is confined to subjects that were being described wrongly.
Fingerprints measured on the same config, before and after:
gpt-oss:120b 54154e19bd2c8ee4 -> 45a73577c5c257f9 (was wrong)
gpt-oss 45a73577c5c257f9 -> 45a73577c5c257f9 (unchanged)
glm-5.3 54154e19bd2c8ee4 -> 54154e19bd2c8ee4 (unchanged)
Note the first line against the third: a model whose sampling gates were
applied hashed identically to one where they were not. It now hashes with
`gpt-oss`, the id it actually behaves like. Recorded evidence for unaffected
subjects keeps its fingerprint, so `resolverVersion` is left at 2 rather than
invalidating every recorded subject globally -- say the word if you would
rather draw a clean generation boundary and I will bump it.
Tests assert the wire the adapter really builds first, then hold the report to
that same wire, so the pair cannot drift apart silently. The five report cases
fail on current dev.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
99a2924 to
c49cb11
Compare
|
Merged as bd3aa31. One residual gap recorded from independent review (pre-existing on |
2 participants
Summary
resolveProductionBehaviorValuesis documented as "authoritative effective values emitted by the production route/model/adapter resolver", andbuildBehaviorFingerprintV1hashes it into the behavior fingerprint that keys Lab evidence.Ten of its rows are membership tests over the provider's
no*Models-style lists, and they went through a local helper:Every runtime gate those rows describe matches through
modelInListinstead, which also accepts a bare entry for a tagged id:sampling.omitTemperature/omitTopP/omitPenaltiesopenai-chat.ts113–115, 1326, 1329, 1398, 1401reasoning.supportedreasoning-effort.ts:107reasoning.replayMode.preserveContent/.placeholderopenai-chat.ts691, 700, 714, 760, 774–775reasoning.splitMode/toggleMode/budgetModeopenai-chat.ts1315, 1370, 1380tools.choiceRestrictionsopenai-chat.ts:1321All ten use
modelInList. The report used exact match for all ten.Why it bites
ollama-cloudserves tagged ids verbatim —gpt-oss:120b,qwen3-coder:480b,qwen3.5:397b,gemma4:31b— and the same registry row writes the baregpt-ossintonoVisionModels, so the bare-prefix form is how these lists are meant to be written.With
noTemperatureModels: ["gpt-oss"], forgpt-oss:120b:temperaturefrom the upstream requestsampling.omitTemperature: falseThe authoritative record contradicts the wire, and that contradiction is hashed into the fingerprint Lab uses to link observations.
Blast radius, measured
Same config, before and after, first 16 hex of the behavior fingerprint:
gpt-oss:120b54154e19bd2c8ee445a73577c5c257f9gpt-oss45a73577c5c257f945a73577c5c257f9glm-5.354154e19bd2c8ee454154e19bd2c8ee4Read the first row against the third: on
dev, a model whose sampling gates were applied hashed identically to one where they were not. It now hashes withgpt-oss, the id it actually behaves like.On
resolverVersion#1956 established the rule — "adding hashed keys without it silently made new fingerprints incomparable to recorded ones". I left
resolverVersionat2on purpose: unlike a key addition, this only moves subjects that were being described incorrectly, and the table above shows unaffected subjects keep their fingerprint, so their recorded evidence stays linkable. If you would rather draw a clean generation boundary anyway, say so and I will bump it in this PR.Tests
tests/routing-compatibility-model-matching.test.ts. The first case asserts the wire the adapter really builds for the tagged model; the rest hold the report to that same wire, so the two cannot drift apart silently. A control asserts an unlisted model still reportsfalse.The five report cases fail on current
dev(2 pass / 5 fail) and pass here (7 / 0).Verification
Run on the exact head of this branch:
I did not run 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 rather than a result. The batch above went throughscripts/test.tsso each file keeps its isolated home.Summary by CodeRabbit
Bug Fixes
gpt-oss:120b, including accurate handling of supported request options and restrictions.Tests
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.