fix(inference): forward raw BYOK model ids to provider construction instead of collapsing to a managed tier#4629
Conversation
…nstead of collapsing to a managed tier An agent/node pinning a raw model id (e.g. `claude-opus-4`) silently ran on `reasoning-v1` because `make_openhuman_backend` collapsed every model string that wasn't a known managed tier. Non-empty, non-`hint:` ids that aren't a managed tier are now forwarded verbatim to the OpenHuman backend, which is authoritative over their validity; the tinyflows simple-LLM path forwards the node's pinned model rather than the role default via a new `resolve_completion_model` helper. Managed-tier and `hint:*` resolution are unchanged. Closes tinyhumansai#4598
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an ChangesBYOK model passthrough
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2371f6afd9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/inference/provider/factory.rs (1)
1232-1253: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUntrimmed
modelbypasses tier matching, causing whitespace-padded tiers to be treated as raw passthrough.
is_known_openhuman_tier(&model)here is checked against the raw (non-trimmed)modelstring, unlikeis_raw_passthrough_model, which trims first. Ifconfig.default_modelcarries incidental whitespace (e.g." reasoning-v1 "), it won't matchis_known_openhuman_tier, so it falls into theelsebranch and is forwarded verbatim to the backend as an "unrecognized" raw id instead of being resolved to the known tier — a behavior regression versus the previous fallback-to-reasoning-v1path for at least that edge case.Consider reusing
is_raw_passthrough_modelhere for consistent trimming semantics:🔧 Suggested fix
- if is_known_openhuman_tier(&model) { - model - } else { + if !is_raw_passthrough_model(&model) { + model + } else { // Unrecognised NON-empty model id — a raw/BYOK model the user🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/inference/provider/factory.rs` around lines 1232 - 1253, The tier check in the model-selection branch is using the raw `model` string, so whitespace-padded known tiers can be misclassified as raw passthrough. Update the `factory.rs` logic around `is_known_openhuman_tier` to use the same trimmed semantics as `is_raw_passthrough_model` (or trim before matching) so values like `default_model` with incidental spaces still resolve to canonical tiers instead of falling into the raw/BYOK path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/inference/provider/factory.rs`:
- Around line 1232-1253: The tier check in the model-selection branch is using
the raw `model` string, so whitespace-padded known tiers can be misclassified as
raw passthrough. Update the `factory.rs` logic around `is_known_openhuman_tier`
to use the same trimmed semantics as `is_raw_passthrough_model` (or trim before
matching) so values like `default_model` with incidental spaces still resolve to
canonical tiers instead of falling into the raw/BYOK path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e32d45c6-8b48-47c5-b20d-fd60f1051305
📒 Files selected for processing (4)
src/openhuman/inference/provider/factory.rssrc/openhuman/inference/provider/factory_tests.rssrc/openhuman/inference/provider/mod.rssrc/openhuman/tinyflows/caps.rs
…odes too Addresses review feedback on tinyhumansai#4598: harness_model_default_override wrapped every node model in hint:chat, collapsing a raw BYOK id (e.g. claude-opus-4) onto the managed chat tier for agent nodes routed through run_via_harness. Raw passthrough ids are now forwarded verbatim (guarded by is_raw_passthrough_model), matching the simple OpenHumanLlm::complete path: they reach the harness chat role, inherit config.default_model, and make_openhuman_backend forwards them to the backend.
CodeGhost21
left a comment
There was a problem hiding this comment.
Review
Bug fix is correct and well-tested. Fixes the silent collapse of raw/BYOK model ids onto reasoning-v1 at make_openhuman_backend and threads the same fix through the tinyflows simple-LLM path and the harness override. The semantic tradeoff (invalid pinned ids now surface a backend error rather than silent downgrade) is the right #4598 behaviour and is called out in the PR body.
Verified
- Traced the backend wire boundary at
src/openhuman/inference/provider/openhuman_backend.rs:32-50.resolve_modeltrims and substitutes for empty at every entry (chat/embed/complete, lines 150/164/176), and existing tests (resolve_model_trims_surrounding_whitespace,resolve_model_preserves_non_empty_value_verbatim) already cover the padded/custom-id shapes. So a padded" claude-opus-4 "forwarded verbatim from the factory is trimmed before the wire — no user-visible breakage. - Both call sites (
OpenHumanLlm::completeandharness_model_default_override) use the sameis_raw_passthrough_modelpredicate, so the simple-LLM lane and the harness lane stay symmetric.
Non-blocking follow-ups
- Stale doc comment in an unrelated file —
src/openhuman/inference/provider/openhuman_backend.rs:27-31still reads "Mirrors the same fallbackmake_openhuman_backendalready applies whendefault_modelis missing" and points atfactory.rs:404. That framing was accurate before this PR — the factory fell back for both empty and unrecognised ids. After this PR the two sites diverge: onlyresolve_modelstill substitutes (for empty), the factory now forwards unrecognised non-empty ids verbatim. Worth a one-line tightening so the next reader doesn't think the fallback still mirrors. - Optional style tightening — the 6-line "model is guaranteed non-empty here" block inside
make_openhuman_backendgoes beyond theCLAUDE.md"one short line max" rule for inline comments. The rationale is genuinely non-obvious, but most of it duplicates the doc comment onis_raw_passthrough_model; a one-liner referencing that would suffice. - Optional cross-reference — see inline comment on
resolve_completion_model.
Verdict: approve. None of the above block merge.
| /// role — in that case the pinned id, not the role default, is the model the | ||
| /// user selected, so it is forwarded verbatim (issue #4598). Managed tiers and | ||
| /// every `hint:*` alias fall through to `resolved_model` unchanged. | ||
| fn resolve_completion_model(node_model: Option<&str>, resolved_model: String) -> String { |
There was a problem hiding this comment.
Nit: the sister function harness_model_default_override already cross-references this one ("Mirrors the per-node routing [OpenHumanLlm::complete] applies via [resolve_completion_model]"), but the reference is one-way. Adding a symmetric back-reference here ("mirrored on the harness path by [harness_model_default_override]") would keep future readers from touching only one lane and drifting the two apart.
CodeGhost21
left a comment
There was a problem hiding this comment.
LGTM. Fix is correct at both call sites (factory + tinyflows), backend handles the wire boundary cleanly, tests cover the raw/tier/hint/empty/whitespace matrix. The follow-ups I noted in my earlier review are all cosmetic — none block merge.
CodeGhost21
left a comment
There was a problem hiding this comment.
LGTM. Fix is correct at both call sites (factory + tinyflows), backend resolve_model handles the wire boundary cleanly, tests cover the raw/tier/hint/empty/whitespace matrix. The follow-ups I noted in my earlier review are all cosmetic — none block merge.
Summary
claude-opus-4) silently ran onreasoning-v1because the provider factory collapsed every model string that wasn't a known managed tier.make_openhuman_backendnow forwards a non-empty, non-hint:, non-managed-tier model id verbatim to the OpenHuman backend (which is authoritative over its validity) instead of substituting the default.resolve_completion_modelhelper, rather than only the role's default model.hint:*resolution are unchanged. Added Rust unit coverage.Problem
inference/provider/factory.rs::make_openhuman_backendcollapsed every unrecognized model string toMODEL_REASONING_V1. A raw BYOK id set on an agent node's custom-model field therefore never reached provider construction — the node silently ran on the configured default rather than the model the user selected. Both the simple-LLM path (tinyflows/caps.rs, which mapped the node model to a role and discarded the raw id) and the harness path went through this helper. Flagged P2 bychatgpt-codex-connectoron #4578.Solution
is_raw_passthrough_model()— a non-empty string that is neither ahint:*alias nor a known managed tier.make_openhuman_backend, such ids are forwarded verbatim (with a debug log) instead of collapsing toreasoning-v1. The empty-string fallback is retained.caps.rs,resolve_completion_model(node_model, resolved_model)returns the pinned raw id when the node pinned one, else the role-resolved default; wired intoOpenHumanLlm::completeaftercreate_chat_provider.configured_agent_modeland now benefits from the factory passthrough — no harness code change was required (verified against the code, not assumed).resolve_modelpreserves non-empty ids verbatim, so the raw id reaches the wire and the backend validates it.Intended semantic tradeoff (call out in review): a genuinely stale/garbage pinned id (e.g. a mistyped tier) now reaches the backend and returns a clear error instead of silently running on
reasoning-v1. This is the correct #4598 behaviour — honour the user's explicit selection and fail loud rather than silently downgrade.Submission Checklist
hint:*invariance;resolve_completion_modelover raw / managed / hint / none / whitespace inputs.N/A: behaviour-only bugfix, no feature row change.## Related—N/A: behaviour-only bugfix.N/A: does not touch release-cut surfaces.Closes #4598.Impact
CLI/desktop inference. Per-node BYOK model overrides are now honoured on agent nodes. A stale/invalid pinned id now surfaces a backend error rather than silently downgrading to a managed tier. No migration implications.
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/byok-model-passthrough-4598Summary by CodeRabbit
New Features
hint:*values keep their existing behavior.Bug Fixes