fix(agent): allow independent temperature and topP sampling for custom Claude endpoints (#874) - #951
De-pitcher wants to merge 6 commits into
Conversation
…ers for custom Claude endpoints (libredb#874)
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cevheri
left a comment
There was a problem hiding this comment.
Thanks for taking this on. I built the branch and probed samplingFor directly, and the core of #874 is still reproducible, so I do not think this is mergeable yet.
Steps 1-5 of the issue use LLM_PROVIDER=custom, LLM_MODEL=claude-haiku-4-5 and no tuning document. There entryFor returns undefined, base falls to DEFAULT_SAMPLING, and samplingFor("claude-haiku-4-5", "investigation") still measures {"temperature":0,"topP":1}. The escape hatch only exists for an operator who writes and mounts a tuning file, and nothing tells the reporter to do that. The same happens one layer down: an entry stating only perWorkflow: { investigation: { temperature: 0.5 } } resolves to {"temperature":0.5,"topP":1}.
Two smaller things:
- tests/unit/lib/agent/model-profiles.test.ts:70 asserts a local literal equals itself and never calls samplingFor, so
it passes with the resolver change fully reverted. The behaviour to pin is samplingFor over a temperature-only
entry. - docs/llms/model-tuning.md:110 still says what you do not state resolves to the compiled default, which this PR makes
false. Code, docs and tests need to move together here.
Happy to keep reviewing once the default path is covered.
|
Thanks @cevheri for the thorough review! I have updated the implementation across the codebase to fully address all three points:
All 163 agent unit tests pass, and |
|
Thanks for the fix. All three red jobs fail on the same single test, so there is one cause to chase:
Two changes in this branch go further than #874 needs, and I would ask you to revert both:
#874 is already solved without either. Your One consequence to decide: with the default restored, a Claude entry that states only Also please restore two assertions the diff removes from existing tests: Locally, before pushing: bun tests/run-tests.ts tests/evals/database-assessment.test.ts
bun run format && bun run lint && bun run typecheck && bun run test
bun run test:coverage && bun run coverage:checkThe coverage gate is the other required job, and it never produced an lcov on this run because the test run exited first, so it is unmeasured rather than green. Minor: the new imports in |
|
Thanks @cevheri for the clear direction! I've pushed an update addressing all points:
All 163 unit tests and the 26 |
|
This looks right now. The measured file and the eval expectation are back to what was measured, both restored assertions are in, and the perWorkflow-only case is pinned by its own test rather than left implicit. CI is green across all three platforms and the coverage job. Two small things and it is ready:
|
…override behavior
|
Done! Updated in
All tests, typecheck, format, oxlint, and security checks are passing cleanly. |
Summary
Fixes #874 by making emperature and opP independently optional in samplingSchema and AgentSampling, and only forwarding defined sampling parameters to streamText. This allows operator model-tuning overlays targeting Anthropic/Claude endpoints via custom providers to specify emperature without op_p (or empty sampling for adaptive models), preventing the 400 rejection ( emperature and top_p cannot both be specified for this model).
Related Issue
Closes #874
Changes Made
Validation