refactor(agent): hold TurnModelSource across all turn paths (tinyagents Motion A, #4249)#4726
refactor(agent): hold TurnModelSource across all turn paths (tinyagents Motion A, #4249)#4726senamakel wants to merge 19 commits into
Conversation
Record the model-layer inversion baseline (create_chat_model exists as a zero-behavior-change shim; ~7 runtime create_chat_provider call sites + 25 non-test files outside inference/provider/ still name dyn Provider) ahead of completing inference-migration-plan Phase 1. Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
Move the cron model-id resolver and the accessibility vision-locate fallback off Box<dyn Provider>/create_chat_provider onto the crate ChatModel factory (create_chat_model / create_chat_model_with_model_id), part of inference migration Phase 1 (tinyhumansai#4249). Zero behavior change: the factory wraps the same provider stack via ProviderModel; the [IMAGE:] marker survives the crate Message -> host ChatMessage round-trip verbatim; cron only needs the resolved model id (built model discarded). Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
P1-8: one-shot caller migration done (cron, vision); caps.rs + inference_probe deferred. P1-9: investigation finding that plan Buckets 2-4 (routing/channels, harness, subagent) are one coupled refactor gated on a ChatModel-accepting build_turn_models (async context-window + telemetry id must be re-homed), not independently landable — must be its own reviewed PR with behavior-parity tests. Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
The plan's 'harness holds Arc<dyn ChatModel>' is architecturally blocked in Phase 1: build_turn_models needs the raw Provider for route projection (per-tier ProviderModel re-aliasing) + separate-error-slot summarizer, and the crate ChatModel has no downcast to recover it. True inversion is gated on Phase 3 (RouterProvider -> crate ModelRegistry, upstream). Documents the host-only alternative (seam newtype boundary). Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
Ground-truth finding: RouterProvider -> crate ModelRegistry is ALREADY done at the seam (assemble_turn_harness registers routes + wires RunPolicy.fallback + RequiredCapabilitiesMiddleware). The crate's missing capability-selection / capability-aware-fallback are NOT needed (caller picks the tier; fallback chains are hand-safe). So Phase 3 needs no upstream crate change. Real remaining work is host-only Motion A: move build_turn_models to the producer/factory boundary so the harness holds crate ChatModels (TurnModels), not Arc<dyn Provider>. Motion B (crate-native clients, deletes compatible*.rs) is the later LOC win. Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
- Introduced a comprehensive document outlining the referral system, including an overview, main rules, data model, migration instructions, core services, API endpoints, and payment integration details. - The document specifies the structure of referral codes, transactions, and the rewards system, ensuring clarity for developers and users. - Added migration commands for setting up the referral system in the application, enhancing the onboarding process for new users.
…vider Phase 3 / Motion A (tinyhumansai#4249): introduce the seam-owned TurnModelSource — a model-agnostic handle that builds a turn's tiered crate ChatModel set (build_turn_models) — and thread it through the native-bus channel/triage turn path instead of Arc<dyn Provider>. TurnModels now carries the provider telemetry id + native-tool/vision/context-window metadata, so run_channel_turn_via_graph reads capabilities off the built crate models and no longer names the Provider trait. AgentTurnRequest.provider -> turn_model_source; channels/triage producers wrap their resolved provider at the bus boundary; the Provider stays confined to the inference factory + seam. Zero behavior change: same ProviderModels, same registry/fallback wiring. Both Cargo worlds' lib compile green; the 3 bus integration tests compile green. (Pre-existing full --tests breakage in unrelated modules — config load, web, ollama, sandbox — is untouched.) Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
TurnModelSource landed; channel/bus turn path fully off Arc<dyn Provider> (commit 30c7dfd). Records remaining subagent + session slices and their tentacles (SubagentCheckpoint summary provider; Agent lifecycle + model_vision). Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
Phase 3 / Motion A (tinyhumansai#4249): thread TurnModelSource through the sub-agent turn path instead of Arc<dyn Provider>. agent_graph::AgentTurnRequest.provider -> turn_model_source; run_subagent_via_graph takes the source, reads vision/ native-tool caps + telemetry id off the built crate models, and resolves the context window via the source. The cap-hit SubagentCheckpoint now summarizes on a crate ChatModel (built via TurnModelSource::build_summarizer) instead of provider.chat, so it no longer names the Provider trait. Runner wraps its resolved subagent_provider at both dispatch sites. Zero behavior change: same ProviderModels, same registry/fallback; checkpoint maps ChatRequest->ModelRequest faithfully (max_tokens preserved, temperature baked). Core lib green; changed files clean under --lib --tests. (extract_tool + subagent provider.rs resolution remain as documented exit-sweep stragglers.) Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
Subagent turn path off Arc<dyn Provider> (commit 8db8887); records remaining Agent session path + extract_tool/provider.rs stragglers + exit sweep. Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
…Provider Phase 3 / Motion A (tinyhumansai#4249): ExtractFromResultTool now holds a TurnModelSource — queries the model context window through it (source.effective_context_window) and builds a summarizer crate ChatModel per call (source.build_summarizer) that it invokes, replacing provider.chat_with_system. Runner wraps the resolved extract_provider at the tool boundary. Adds TurnModelSource::is_local_provider passthrough (needed by the upcoming ParentExecutionContext migration). Core lib green; extract_tool test module clean under --lib --tests; zero behavior change. Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
Phase 3 / Motion A (tinyhumansai#4249): the final major turn path. Agent + AgentBuilder, ParentExecutionContext, and ChatTurnGraph now hold a seam TurnModelSource / built TurnModels instead of Arc<dyn Provider>: - Agent.turn_model_source builds the tiered crate ChatModel set per turn; core resolves the context window + builds up front, reads vision off the built models, and hands TurnModels to ChatTurnGraph (which no longer builds/holds a provider). - ParentExecutionContext carries the source; sub-agent inheritance / extract summarization-route / rhai model build / is_local_provider read through it. TurnModelSource gains is_local_provider() + a provider() escape hatch for the few seam-boundary sites that still resolve a raw provider inline (sub-agent provider resolution + its 9 unit tests, rhai, payload summarizer) — no agent-harness struct holds a Provider. - The session_io cap-hit checkpoint keeps its streaming provider.chat via source.provider() (crate ChatModel::invoke doesn't expose the delta sink). - Builder .provider()/.provider_arc() setters wrap into a source; Agent provider_arc() accessor -> turn_model_source(). Zero behavior change: same ProviderModels, same registry/fallback wiring. Core lib green; all touched test files (inline + 10 integration tests) compile clean; remaining --tests errors are pre-existing unrelated modules (config load, web, ollama, sandbox, reliable_tests, memory) untouched by this change. Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
…ated) Session path migrated (9112330); no agent-harness struct holds Arc<dyn Provider>. Records the remaining provider-resolution boundaries (factory, resolve, delegate, triage, builder setters) as Motion B (crate-native client) territory. Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
Formatting-only (line wrapping) applied by cargo fmt over the Motion A changes. Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
Restore two e2e specs (cron-jobs-flow, telegram-channel-flow) to upstream/main; they rode into this branch via an upstream-sync merge and are not part of the TurnModelSource migration. Claude-Session: https://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
📝 WalkthroughWalkthroughThis PR migrates numerous call sites from directly holding ChangesProvider to TurnModelSource/TurnModels seam migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d313a2c090
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| .await; | ||
| .await | ||
| .map(|r| r.text()) |
There was a problem hiding this comment.
When extract_from_result is asked to preserve literal tool-call markup from raw tool output, this new one-shot path now goes through ProviderModel::invoke and then ModelResponse::text(). That adapter parses provider text as agent tool calls before building text() even though this request advertises no tools, so outputs containing snippets like <tool_call>…</tool_call> can be stripped or moved out of the visible text and the extraction returns empty/truncated data. Keep this extraction path on a raw-text provider call, or bypass tool-call parsing for no-tool one-shot requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Low risk in practice. The migration swaps provider.chat_with_system(...) -> String for ChatModel::invoke(...).map(|r| r.text()); ModelResponse::text() returns the concatenated visible-text blocks, which matches the old raw string for the single-text-block responses these extract calls produce (the system prompt asks the model for plain extracted text, no tool calls). Worth a mock-backend spot-check for certainty, but not a blocker.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/tinyagents/mod.rs (1)
1177-1224: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStore the filtered context window, not the raw parameter.
context_windowis filtered to> 0before being applied to the primary model's profile, but the raw, unfiltered value is stored onTurnModels.context_window. A caller passingSome(0)would seeTurnModels::context_window()reportSome(0)as a known window, even though the primary was never actually capped with it — a latent trap for whichever harness-turn-path caller consumes this new accessor.🐛 Proposed fix
- let provider_id = provider.telemetry_provider_id(); - let native_tools = provider.supports_native_tools(); - let supports_vision = provider.supports_vision(); - let summary_provider = provider.clone(); - let mut primary = ProviderModel::new(provider, model, temperature); - // Record the model's context window on its capability profile (issue `#4249`, - // Phase 2) so the crate can validate input capacity before dispatch. The - // per-call output cap rides `RunConfig.max_turn_output_tokens` instead. - if let Some(window) = context_window.filter(|w| *w > 0) { - primary = primary.with_context_window(window); - } + let provider_id = provider.telemetry_provider_id(); + let native_tools = provider.supports_native_tools(); + let supports_vision = provider.supports_vision(); + let summary_provider = provider.clone(); + let context_window = context_window.filter(|w| *w > 0); + let mut primary = ProviderModel::new(provider, model, temperature); + // Record the model's context window on its capability profile (issue `#4249`, + // Phase 2) so the crate can validate input capacity before dispatch. The + // per-call output cap rides `RunConfig.max_turn_output_tokens` instead. + if let Some(window) = context_window { + primary = primary.with_context_window(window); + }🤖 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/tinyagents/mod.rs` around lines 1177 - 1224, The TurnModels context window is being stored from the raw input instead of the filtered value, so a zero or invalid window can be reported as present even though build_turn_models never applies it. Update build_turn_models to preserve the same filtered Option<u64> used for primary.with_context_window when assigning the TurnModels.context_window field, using the existing build_turn_models and TurnModels symbols to keep the stored capability in sync with the actual primary model profile.
🧹 Nitpick comments (1)
src/openhuman/tinyagents/mod.rs (1)
1226-1254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment contradicts itself on construction sites.
The struct doc states
TurnModelSourceis "Constructed in exactly one place —create_turn_model_source", butnew()'s doc says it'spubprecisely soAgentTurnRequestand its integration tests can construct it directly (confirmed by multiple test files callingTurnModelSource::new(...)directly). Worth clarifying which statement reflects the intended invariant, since this comment documents a seam boundary other engineers will rely on during the remaining Motion B work.🤖 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/tinyagents/mod.rs` around lines 1226 - 1254, The documentation for TurnModelSource is internally inconsistent about who may construct it: the type-level comment says it is created in exactly one place via create_turn_model_source, while TurnModelSource::new implies direct construction is intentionally supported by AgentTurnRequest and tests. Update the docs on TurnModelSource and new to state one clear invariant and align both comments with the actual construction sites, so the seam boundary is documented consistently.
🤖 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.
Inline comments:
In `@src/openhuman/tinyagents/mod.rs`:
- Around line 1226-1307: Move TurnModelSource and its impl out of mod.rs into a
dedicated module, since mod.rs should remain export-only and contain no business
logic. Put the struct plus its methods (new, effective_context_window,
is_local_provider, provider, build, build_summarizer) in a new module such as
turn_model_source.rs, then re-export it from mod.rs with pub use so existing
crate::openhuman::tinyagents::TurnModelSource references keep working unchanged.
---
Outside diff comments:
In `@src/openhuman/tinyagents/mod.rs`:
- Around line 1177-1224: The TurnModels context window is being stored from the
raw input instead of the filtered value, so a zero or invalid window can be
reported as present even though build_turn_models never applies it. Update
build_turn_models to preserve the same filtered Option<u64> used for
primary.with_context_window when assigning the TurnModels.context_window field,
using the existing build_turn_models and TurnModels symbols to keep the stored
capability in sync with the actual primary model profile.
---
Nitpick comments:
In `@src/openhuman/tinyagents/mod.rs`:
- Around line 1226-1254: The documentation for TurnModelSource is internally
inconsistent about who may construct it: the type-level comment says it is
created in exactly one place via create_turn_model_source, while
TurnModelSource::new implies direct construction is intentionally supported by
AgentTurnRequest and tests. Update the docs on TurnModelSource and new to state
one clear invariant and align both comments with the actual construction sites,
so the seam boundary is documented consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0d0aa44-2e07-4522-844c-2f9b4c792a2d
📒 Files selected for processing (42)
docs/tinyagents-drift-ledger.mddocs/tinyagents-phase3-router-registry-design.mdsrc/openhuman/accessibility/automate.rssrc/openhuman/accessibility/vision_click.rssrc/openhuman/agent/bus.rssrc/openhuman/agent/harness/agent_graph.rssrc/openhuman/agent/harness/fork_context.rssrc/openhuman/agent/harness/graph.rssrc/openhuman/agent/harness/session/builder/setters.rssrc/openhuman/agent/harness/session/runtime.rssrc/openhuman/agent/harness/session/runtime_tests.rssrc/openhuman/agent/harness/session/turn/core.rssrc/openhuman/agent/harness/session/turn/graph.rssrc/openhuman/agent/harness/session/turn/session_io.rssrc/openhuman/agent/harness/session/turn/tools.rssrc/openhuman/agent/harness/session/types.rssrc/openhuman/agent/harness/subagent_runner/extract_tool.rssrc/openhuman/agent/harness/subagent_runner/ops/checkpoint.rssrc/openhuman/agent/harness/subagent_runner/ops/graph.rssrc/openhuman/agent/harness/subagent_runner/ops/runner.rssrc/openhuman/agent/triage/evaluator.rssrc/openhuman/agent_orchestration/parent_context/builder.rssrc/openhuman/channels/runtime/dispatch/processor.rssrc/openhuman/cron/scheduler.rssrc/openhuman/rhai_workflows/bridge.rssrc/openhuman/skills/e2e_plumbing_tests.rssrc/openhuman/skills/e2e_run_tests.rssrc/openhuman/tinyagents/mod.rssrc/openhuman/tinyagents/payload_summarizer.rstests/agent_archivist_debug_round21_raw_coverage_e2e.rstests/agent_harness_leftovers_raw_coverage_e2e.rstests/agent_harness_public.rstests/agent_harness_raw_coverage_e2e.rstests/agent_large_round25_raw_coverage_e2e.rstests/agent_prompts_subagent_raw_coverage_e2e.rstests/agent_session_turn_raw_coverage_e2e.rstests/agent_tool_loop_raw_coverage_e2e.rstests/agent_turn_toolloop_round22_raw_coverage_e2e.rstests/calendar_grounding_e2e.rstests/composio_list_tools_stack_overflow_regression.rstests/inference_agent_raw_coverage_e2e.rstests/tools_agent_credentials_state_raw_coverage_e2e.rs
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #4726 — refactor(agent): hold TurnModelSource across all turn paths (tinyagents Motion A, #4249)
Walkthrough
This is a type-plumbing refactor that introduces a seam-owned TurnModelSource newtype wrapping Arc<dyn Provider>, and threads it through every agent turn path (channel/bus, sub-agent, session) plus the extract_from_result tool — so no agent-harness struct (Agent, AgentBuilder, ParentExecutionContext, ChatTurnGraph, both AgentTurnRequests) names the Provider trait anymore. TurnModels gains provider_id/native_tools()/supports_vision()/context_window() accessors so the harness reads capabilities off the built crate models instead of a raw provider, and the cap-hit checkpoint + extract + vision-locate calls move from provider.chat* onto crate ChatModel::invoke.
Assessment: the migration is mechanically sound and I could confirm behavior parity on the paths I was worried about (per-request max_tokens/temperature are honored by ProviderModel::invoke; cron model-id resolution is identical; usage conversion preserves charged-USD + cached tokens). No blocking issues. A handful of minor doc/dead-code cleanups remain, plus the two bot findings below which I've verified and largely agree with.
Changes
| File(s) | Summary |
|---|---|
src/openhuman/tinyagents/mod.rs |
New TurnModelSource seam type + TurnModels capability accessors (provider_id/context_window/native_tools/supports_vision). |
agent/bus.rs, agent_graph.rs, fork_context.rs, session/types.rs, session/builder/setters.rs, session/runtime.rs |
Agent/AgentBuilder/ParentExecutionContext/both AgentTurnRequests switch provider: Arc<dyn Provider> → turn_model_source: TurnModelSource. |
harness/graph.rs, session/turn/{core,graph}.rs |
Turn paths resolve context window + build TurnModels at the caller and read caps via accessors. |
subagent_runner/{extract_tool,ops/checkpoint,ops/graph,ops/runner}.rs |
Cap-hit checkpoint + extract tool invoke crate ChatModels built from the source. |
accessibility/{automate,vision_click}.rs, cron/scheduler.rs |
One-shot callers migrated onto create_chat_model* factory. |
triage/evaluator.rs, parent_context/builder.rs, channels/.../processor.rs, rhai_workflows/bridge.rs, payload_summarizer.rs |
Producers wrap the resolved provider into a source at the boundary. |
docs/tinyagents-*.md |
Phase-3 design doc + P1-8/P1-9 drift-ledger rows. |
tests/*.rs, skills/e2e_* |
Test wiring updated to construct TurnModelSource::new(...). |
Actionable comments (2)
💡 Refactor / suggestion
1. src/openhuman/tinyagents/mod.rs:1157 — TurnModels::context_window() is dead code and stores the unfiltered value
The new context_window() accessor is never read anywhere in src/ or tests/ — the two turn paths that need the window read it directly from source.effective_context_window(...) / graph.context_window, not off TurnModels. It will trip dead_code under -D warnings. Separately (matching CodeRabbit's outside-diff note): build_turn_models filters context_window to > 0 before applying it to the primary profile, but stores the raw value on the field — so if the accessor were ever used, Some(0) would report as a known window the primary was never capped with.
Suggested change: either drop the accessor + field, or store the filtered value and route both turn paths through it:
// build_turn_models — store the same filtered Option the primary profile uses
let context_window = context_window.filter(|w| *w > 0);
let mut primary = ProviderModel::new(provider, model, temperature);
if let Some(window) = context_window {
primary = primary.with_context_window(window);
}
// ...
TurnModels { /* ... */ context_window, /* ... */ }2. src/openhuman/tinyagents/mod.rs:1239 — broken rustdoc link + self-contradictory construction invariant
The struct doc links create_turn_model_source, which does not exist (the factory only has create_chat_model{,_with_model_id,_from_string}); this is a broken intra-doc link (warns, and errors under -D rustdoc::broken_intra_doc_links). It also states TurnModelSource is "Constructed in exactly one place," but new() is pub and called from ~15 producer/test sites. Pick the real invariant and fix the link.
Suggested change:
/// … every `Provider` method it needs (context-window resolution, the model
/// build) is exposed here. Constructed via [`TurnModelSource::new`] at the
/// producer/seam boundary (channel dispatch, session builder, subagent runner,
/// and their tests).
Nitpicks (2)
src/openhuman/agent/harness/builtin_definitions.rs:91andsession/tests.rs:821— doc comments still say the runner "usesparent.providerverbatim"; the field is nowparent.turn_model_source. Update the prose so it doesn't reference a removed field.src/openhuman/cron/scheduler.rs:835—create_chat_model_with_model_idbuilds and immediately discards aChatModeljust to read the resolved model id. Correct (id resolution is identical tocreate_chat_provider), but a comment noting the throwaway build, or a lighter id-only resolver later, would avoid a "why build a model we drop?" double-take. Non-blocking.
Responses to existing bot findings
- CodeRabbit — move
TurnModelSourceout ofmod.rs: agreed in principle per the canonical module shape, buttinyagents/mod.rsis already a ~1400-line file dense with business logic (build_turn_models,assemble_turn_harness, the whole seam), so this one struct isn't the marginal violation. Fine to defer to a focusedmod.rs-thinning pass; flagging so it isn't lost. - CodeRabbit — store filtered context window / doc inconsistency: confirmed and folded into actionable items #1 and #2 above.
- Codex — "preserve raw extraction text from one-shot calls": low risk. The migration swaps
provider.chat_with_system(...) -> StringforChatModel::invoke(...).map(|r| r.text());ModelResponse::text()returns the concatenated visible-text blocks, which matches the old raw string for the single-text-block responses these extract calls produce. Worth a spot-check on a mock-backend turn if you want certainty, but not a blocker.
Verified / looks good
ProviderModel::invokehonorsrequest.max_tokens.or(self.max_tokens)andrequest.temperature.unwrap_or(self.temperature)— so the checkpoint's.with_max_tokens(), and the extract/vision.with_temperature(0.0), preserve the pre-refactor bounds (checkpoint reservation-pricing pre-flight TAURI-RUST-C62 stays realistic).cron/scheduler.rs:create_chat_model_with_model_idresolves the model id via the samecreate_chat_providercall, so the cronModelSpec::Hintresolution is unchanged.usage_info_from_responsefaithfully carriescharged_amount_usd+ cached/reasoning tokens across theinvokecrossing — checkpoint cost accounting parity preserved.context_windowis passed identically into bothsource.build(...)andChatTurnGraph { context_window }insession/turn/core.rs, so the summarization-step install is unchanged.- No lingering
Provider-trait references on any agent-harness struct; theprovider()escape hatch is confined to inline resolution sites (rhai bridge, subagent provider inheritance), consistent with the Motion-B plan.
| } | ||
|
|
||
| /// The primary model's effective context window, if known. | ||
| pub(crate) fn context_window(&self) -> Option<u64> { |
There was a problem hiding this comment.
TurnModels::context_window() is dead code — no caller in src/ or tests/ reads it (the turn paths read the window from source.effective_context_window(...) / graph.context_window directly), so it will trip dead_code under -D warnings.
Also, matching CodeRabbit's outside-diff note: build_turn_models applies context_window.filter(|w| *w > 0) to the primary profile but stores the raw value on this field. If the accessor were ever wired in, Some(0) would report as a real window the primary was never capped with.\n\nEither drop the accessor + field, or store the filtered value and route both turn paths through it:\nrust\nlet context_window = context_window.filter(|w| *w > 0);\nlet mut primary = ProviderModel::new(provider, model, temperature);\nif let Some(window) = context_window {\n primary = primary.with_context_window(window);\n}\n// ...\nTurnModels { /* ... */ context_window, /* ... */ }\n
| /// single `Arc<dyn ChatModel>`. `TurnModelSource` confines that `Provider` to the | ||
| /// seam: the harness names only this type, and every `Provider` method it needs | ||
| /// (context-window resolution, the model build) is exposed here. Constructed in | ||
| /// exactly one place — [`create_turn_model_source`](crate::openhuman::inference::provider::factory::create_turn_model_source). |
There was a problem hiding this comment.
Broken rustdoc intra-doc link + self-contradictory invariant.
create_turn_model_source does not exist — the factory only exposes create_chat_model{,_with_model_id,_from_string}. This is a broken intra-doc link (warns, errors under -D rustdoc::broken_intra_doc_links). The doc also claims TurnModelSource is "Constructed in exactly one place," but new() is pub and called from ~15 producer/test sites.
Pick the real invariant and fix the link:
/// … Constructed via [`TurnModelSource::new`] at the producer/seam boundary
/// (channel dispatch, session builder, subagent runner, and their tests).
2 participants
Summary
TurnModelSource— a seam-owned, model-agnostic handle that builds a turn's tiered crateChatModelset (primary + workload-tier fallback routes + summarizer) per turn — and threads it through all three agent turn paths (channel/bus, sub-agent, session) plus theextract_from_resulttool, replacing the rawArc<dyn Provider>.Arc<dyn Provider>anymore:Agent,AgentBuilder,ParentExecutionContext,ChatTurnGraph, and bothAgentTurnRequests now carry aTurnModelSource/builtTurnModels. TheProvidertrait is confined to the inference factory + seam.ChatModelfactory.TurnModelSource::buildwraps the identical existing provider stack viaProviderModel; the crateModelRegistry+RunPolicy.fallback+ capability gating were already wired inassemble_turn_harnessand are untouched.tinyagentschange is required) and the remaining Motion-B boundaries.Problem
The agent loop already runs on
tinyagents, but the harness still held openhuman's in-houseProvidertrait objects and re-wrapped them into crateChatModels on every turn. Maintaining that duplication means every fix lands twice, and nothing could delete the ~30k-line duplicated provider stack (compatible*.rs,reliable.rs,router.rs) while agent-harness structs still named theProvidertrait. This is the pivot (issue #4249, inference-migration Phase 1 / "Motion A") that unblocks the crate-native provider consolidation.Solution
TurnModelSourcewraps the resolvedArc<dyn Provider>at the inference↔seam boundary and exposes exactly what the harness needs —build(the per-turn tiered model set),build_summarizer(one-off summary models for the sub-agent cap-hit checkpoint + extract tool),effective_context_window,is_local_provider(), and a narrowprovider()escape hatch. TheAgent(model-agnostic; builds per turn) holds the source;core.rsresolves the context window and builds the model set up front, reads vision capability off the built models, and handsTurnModelstoChatTurnGraph.SubagentCheckpointand theextract_from_resulttool were rewritten fromprovider.chat*onto crateChatModel::invoke. The streaming session cap-hit checkpoint keepsprovider.chatviasource.provider()(the crateChatModel::invokepath has no delta sink).Design decisions/tradeoffs:
resolve_subagent_providerkeeps returningArc<dyn Provider>(to avoid churning its 9 unit tests); the builder.provider()/.provider_arc()setters keep their public API and wrap into a source internally. These provider-resolution boundaries remain and are documented as Motion-B follow-ups (see## Related).Submission Checklist
TurnModelSource/crateMockModel; no behavior assertions changed (this is a type-plumbing refactor with parity preserved).N/A (author env): fullpnpm test:coverage/pnpm test:rustnot run locally (pre-existing unrelated full---testsbreakage in this branch's base — config load, web, ollama, sandbox, reliable_tests). Core lib + Tauri shell bothcargo checkgreen; all touched test files compile clean. Please rely on CI for the coverage gate.N/A: behaviour-only refactor(no feature rows change).N/A(internal harness refactor, no user-facing feature).N/A(no release-cut surface change; provider-string grammar,inference.*RPC, and streaming/cost/tool-timeline behavior are parity-locked).Closes #NNN— this is Motion A of the tracked migration; Motion B follow-up issue linked below.Impact
ProviderModels, same registry/fallback wiring; the per-turn model-set build moves from the harness graph to the producer boundary.inference.*RPC surface are unchanged.#4460thread_id task-local and$0-turncost paths were preserved. Behavior-parity (mock-backend turn) verification recommended in CI/review.Related
providers::openaiclients directly (deletescompatible*.rs), eliminating the remaining provider-resolution boundaries (factory::create_chat_provider/create_routed_provider,resolve_subagent_provider,tools/delegate.rs,triage/routing.rs, builder setters) and theTurnModelSource::provider()escape hatch. Tracked in Complete tinyagents inference migration (Motion B): crate-native provider clients, delete duplicated provider stack #4727.docs/tinyagents-phase3-router-registry-design.md,docs/tinyagents-drift-ledger.md(rows P1-8/P1-9).AI Authored PR Metadata
Linear Issue
Commit & Branch
feat/inference-model-inversionhttps://claude.ai/code/session_018MnMVgnzxchtcs1DDUxTfF
Summary by CodeRabbit
New Features
Bug Fixes
Tests