feat(prompt): tell the agent which model runs it, its subagents, and the advisor - #860
Conversation
…the advisor
The agent had no way to know what model it was running on. That is a fact only
Rho holds: the user picks the model, and Rho can switch it mid-session.
The system prompt now names the running model and the advisor's model. Each
delegated run reports the model it used in its own output. Switching either
model appends one short line.
Model ids lead and catalog names follow, from the models.dev `name` field:
openai-codex/gpt-5.6-luna (GPT-5.6 Luna)
Names are never invented from an id, because a model can be newer than whatever
reads the prompt. Rho passes Claude Code `--model` through untouched and the
binary lists no models, so an alias like `opus` is resolved from the model the
run reports in its stream-json init frame.
Text already written is never rewritten: the `agent` and `advisor` tool
descriptions name no model, and catalog names resolve once per process.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR adds cached model display-name resolution, unified Rho and Claude model identities, model-aware prompts, model-switch notices, resolved Claude model tracking, startup metadata prefetching, and related tests and version updates. ChangesModel identity integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentBinding
participant PromptAssembly
participant InteractiveRuntime
participant AgentOutput
AgentBinding->>PromptAssembly: provide PromptModel
PromptAssembly->>InteractiveRuntime: render model-aware prompt
InteractiveRuntime->>InteractiveRuntime: record model-switch notice
InteractiveRuntime->>AgentOutput: persist run status
AgentOutput->>AgentOutput: format recorded model identity
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The detached catalog prefetch races with a process-lifetime negative cache, so the new display names can be missing for the entire first launch.
Reviewed changes across the model identity, catalog metadata, prompt assembly, Claude stream handling, delegated-run output, and interactive model-switch paths.
- Model identity and names — Adds catalog-backed names, Claude Code alias resolution, and a shared representation for prompt-facing model identity.
- Prompt and switch context — Names the running and advisor models in system context and appends notices when either changes.
- Delegated-run reporting — Captures Claude's concrete model from init frames and includes model identity in run snapshots and completions.
- Startup metadata prefetch — Collects models the session may name and starts a deduplicated models.dev refresh.
- Coverage — Adds focused tests for metadata parsing, binding parity, identity rendering, stream mapping, prompt wiring, and switch notices.
GPT Sol ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
crates/rho/src/tools/advisor/advisor_tests.rs (1)
353-354: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe last two assertions check a compile-time constant.
baselineis captured on line 344, before eitherset_modelcall, andTOOL_DESCRIPTIONis aconst. These two assertions cannot fail because of the store changes this test makes. Theassert_eq!checks on lines 347 and 352 already prove the invariance the test name claims.Either drop the two lines or assert against the post-change description so they exercise the same path.
As per coding guidelines: "avoid static-constant tests, removed-behavior tests, and string-contains tests that merely lock copied text."
🤖 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 `@crates/rho/src/tools/advisor/advisor_tests.rs` around lines 353 - 354, Remove the redundant baseline.contains assertions in the test around the set_model calls; the existing assert_eq checks already verify that the description remains unchanged, so do not add further static string-contents assertions.Source: Coding guidelines
crates/rho/src/app/interactive_runtime_tests.rs (1)
877-889: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist
last_notice_textto module scope.The same block-extraction logic already appears twice in this file, at lines 675-681 and 701-707. This nested copy makes three. Move
last_notice_textto module scope and call it fromadvisor_mode_changes_the_tool_list_without_replacing_the_sessionas well.🤖 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 `@crates/rho/src/app/interactive_runtime_tests.rs` around lines 877 - 889, Move the nested last_notice_text helper to module scope so it is defined once, then update advisor_mode_changes_the_tool_list_without_replacing_the_session and the other duplicated call sites to reuse it. Remove the local duplicate while preserving the existing user-message text extraction behavior.crates/rho/src/claude_runtime/resolved_models.rs (2)
60-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch
StreamEffectexhaustively so a new variant forces a decision.
note_stream_effectuses a let-else that discards every non-Statusvariant silently. If a future variant also carries a resolved model, this function keeps compiling and keeps dropping it.♻️ Proposed exhaustive match
- let super::stream::StreamEffect::Status(patch) = effect else { - return; - }; - if let Some(model) = &patch.claude_model { - record(requested, model); - } + match effect { + super::stream::StreamEffect::Status(patch) => { + if let Some(model) = &patch.claude_model { + record(requested, model); + } + } + // No other effect carries a resolved model today. + _other => {} + }Replace
_other => {}with the concrete variant list so the compiler flags additions.As per coding guidelines: "Match known Rust enums exhaustively so new variants require intentional handling."
🤖 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 `@crates/rho/src/claude_runtime/resolved_models.rs` around lines 60 - 67, Update note_stream_effect to use an exhaustive match on StreamEffect instead of the let-else pattern that silently ignores non-Status variants. Handle the existing concrete variants explicitly, preserving model recording for Status, so adding any future StreamEffect variant produces a compiler error requiring an intentional decision.Source: Coding guidelines
40-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider recovering from lock poisoning in
recordandlast_resolved.Both accessors call
.expect("resolved Claude model lock"). A panic anywhere inside the critical section poisons theRwLockfor the rest of the process. Every laterdescribe()on a Claude identity then panics, which turns a name-lookup detail into a session-wide failure. The stored data stays valid, because the guarded operations are plainStringinserts and clones.
test_lockalready usesunwrap_or_else(|poisoned| poisoned.into_inner()). The same treatment here keeps model naming best-effort.🛡️ Proposed poisoning recovery
- let mut store = store().write().expect("resolved Claude model lock"); + let mut store = store() + .write() + .unwrap_or_else(|poisoned| poisoned.into_inner());- let store = store().read().expect("resolved Claude model lock"); + let store = store() + .read() + .unwrap_or_else(|poisoned| poisoned.into_inner());Also applies to: 70-71
🤖 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 `@crates/rho/src/claude_runtime/resolved_models.rs` around lines 40 - 45, Update record and last_resolved to recover from poisoned RwLock guards using the same unwrap_or_else(|poisoned| poisoned.into_inner()) pattern already used by test_lock, replacing the expect-based lock acquisition while preserving the existing guarded insert and clone behavior.crates/rho/src/model_identity.rs (1)
113-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a named type for the prefetch keys.
describable_modelsreturnsVec<(String, String)>. The order of provider and model is only documented in prose, so a call site can swap them without a compile error. A small newtype or a struct withproviderandmodelfields makes the contract explicit and matches the guideline on self-documenting call sites.As per coding guidelines: "Make Rust call sites self-documenting by preferring enums, named methods, builders, or newtypes over ambiguous boolean or
Optionparameters."🤖 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 `@crates/rho/src/model_identity.rs` around lines 113 - 132, Replace the ambiguous tuple return type of describable_models with a named type containing explicit provider and model fields, and construct that type in the ModelIdentity::Rho branch. Update affected callers to access the named fields so provider/model ordering is enforced and call sites are self-documenting.Source: Coding guidelines
crates/rho/src/tools/agent/agent_tests.rs (1)
19-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThis change widens process-environment mutation in tests.
IsolatedRhoHome::newnow setsHOMEin addition toRHO_HOME. Both are process-global.crate::paths::process_env_lock()serializes only the tests that take that lock; any concurrent test that readsHOMEwithout it observes the temp directory.The repository guideline asks for injection instead of process-environment mutation. Agent discovery would need a home-directory parameter or an injected resolver to satisfy it, so this is a larger change than the current diff. Track it rather than fix it inline if that plumbing is out of scope here.
As per coding guidelines: "Do not mutate the process environment in tests; inject environment-derived values instead."
🤖 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 `@crates/rho/src/tools/agent/agent_tests.rs` around lines 19 - 41, Track this as a follow-up rather than expanding the current test change: avoid setting HOME in IsolatedRhoHome::new, and update agent discovery to accept an injected home-directory/resolver so tests can isolate ~/.rho/agents and ~/.agents/agents without process-environment mutation.Source: Coding guidelines
crates/rho/src/tui/model_actions_tests.rs (1)
309-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the duplicated fixtures to module scope.
switch_to_anthropicis now defined twice in this file, here and at lines 194-210. The credential andAppsetup inapp_on_openaialso repeats the block at lines 219-239. The pattern has repeated, so the shared mechanics can move out.Move
app_on_openaiandswitch_to_anthropicto module scope and call them fromselect_model_report_auto_edit_tool_follows_provider_change. Keep the differing policy at the call sites: the older test needs theEditTool::Autoand pinned config variations and asserts the handoff report.🤖 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 `@crates/rho/src/tui/model_actions_tests.rs` around lines 309 - 370, Hoist the shared `app_on_openai` and `switch_to_anthropic` helpers to module scope, removing their duplicate local definitions. Update `select_model_report_auto_edit_tool_follows_provider_change` and the older test to call these helpers, while retaining each test’s distinct `EditTool::Auto`, pinned configuration, and handoff-report assertions at its call site.
🤖 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 `@crates/rho-providers/src/model/display_name.rs`:
- Around line 43-53: Update model_display_name so the resolved name is
reconciled under the cache write lock: after acquiring the lock, check whether
the key was inserted by another caller and return that existing value; otherwise
insert and return the newly resolved name. Preserve the initial read fast path
and use the cache entry selected while holding the write lock.
In `@crates/rho/src/app/interactive_runtime_advisor.rs`:
- Around line 71-78: Update the changed-model comparison in the interactive
runtime advisor flow to compare the reported model identities, excluding
reasoning-only differences. Use the same provider/model identity representation
consumed by advisor_model_switch_context and ModelIdentity::describe, while
preserving notices for actual provider or model changes.
- Around line 79-84: Update the notice-handling branch in the interactive
runtime advisor around store.set_model and append_user_context_with_display to
capture the previous advisor model before switching, then restore it if
appending the notice fails. Match the adjacent enable/disable rollback behavior
and preserve the existing success return path.
In `@crates/rho/src/prompt.rs`:
- Around line 110-124: The ModelIdentity::describe output can contain control
characters, but prompt construction assumes it is single-line. Sanitize or
encode the display-name resolution used by ModelIdentity::describe so provider,
model, and catalog names cannot introduce newlines or other control characters;
this root fix covers the running-model and advisor lines at
crates/rho/src/prompt.rs:110-124 and ensures model_switch_context and
advisor_model_switch_context at crates/rho/src/prompt.rs:284-302 always emit
exactly one bracketed line.
---
Nitpick comments:
In `@crates/rho/src/app/interactive_runtime_tests.rs`:
- Around line 877-889: Move the nested last_notice_text helper to module scope
so it is defined once, then update
advisor_mode_changes_the_tool_list_without_replacing_the_session and the other
duplicated call sites to reuse it. Remove the local duplicate while preserving
the existing user-message text extraction behavior.
In `@crates/rho/src/claude_runtime/resolved_models.rs`:
- Around line 60-67: Update note_stream_effect to use an exhaustive match on
StreamEffect instead of the let-else pattern that silently ignores non-Status
variants. Handle the existing concrete variants explicitly, preserving model
recording for Status, so adding any future StreamEffect variant produces a
compiler error requiring an intentional decision.
- Around line 40-45: Update record and last_resolved to recover from poisoned
RwLock guards using the same unwrap_or_else(|poisoned| poisoned.into_inner())
pattern already used by test_lock, replacing the expect-based lock acquisition
while preserving the existing guarded insert and clone behavior.
In `@crates/rho/src/model_identity.rs`:
- Around line 113-132: Replace the ambiguous tuple return type of
describable_models with a named type containing explicit provider and model
fields, and construct that type in the ModelIdentity::Rho branch. Update
affected callers to access the named fields so provider/model ordering is
enforced and call sites are self-documenting.
In `@crates/rho/src/tools/advisor/advisor_tests.rs`:
- Around line 353-354: Remove the redundant baseline.contains assertions in the
test around the set_model calls; the existing assert_eq checks already verify
that the description remains unchanged, so do not add further static
string-contents assertions.
In `@crates/rho/src/tools/agent/agent_tests.rs`:
- Around line 19-41: Track this as a follow-up rather than expanding the current
test change: avoid setting HOME in IsolatedRhoHome::new, and update agent
discovery to accept an injected home-directory/resolver so tests can isolate
~/.rho/agents and ~/.agents/agents without process-environment mutation.
In `@crates/rho/src/tui/model_actions_tests.rs`:
- Around line 309-370: Hoist the shared `app_on_openai` and
`switch_to_anthropic` helpers to module scope, removing their duplicate local
definitions. Update `select_model_report_auto_edit_tool_follows_provider_change`
and the older test to call these helpers, while retaining each test’s distinct
`EditTool::Auto`, pinned configuration, and handoff-report assertions at its
call site.
🪄 Autofix
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 Plus
Run ID: 28b155e1-0f46-4485-b2a1-74fb42f49910
📒 Files selected for processing (34)
crates/rho-providers/src/model/display_name.rscrates/rho-providers/src/model/display_name_tests.rscrates/rho-providers/src/model/mod.rscrates/rho-providers/src/model/models_dev.rscrates/rho-providers/src/model/models_dev_tests.rscrates/rho/src/app/agent_binding.rscrates/rho/src/app/agent_binding_tests.rscrates/rho/src/app/bootstrap.rscrates/rho/src/app/interactive_runtime_advisor.rscrates/rho/src/app/interactive_runtime_tests.rscrates/rho/src/app/mod.rscrates/rho/src/app/tools_prompt.rscrates/rho/src/app/tools_prompt_tests.rscrates/rho/src/claude_runtime/mod.rscrates/rho/src/claude_runtime/one_shot.rscrates/rho/src/claude_runtime/resolved_models.rscrates/rho/src/claude_runtime/resolved_models_tests.rscrates/rho/src/claude_runtime/session.rscrates/rho/src/claude_runtime/stream/presentation.rscrates/rho/src/claude_runtime/stream/protocol.rscrates/rho/src/claude_runtime/stream/stream_protocol_tests.rscrates/rho/src/claude_runtime/stream/types.rscrates/rho/src/lib.rscrates/rho/src/model_identity.rscrates/rho/src/model_identity_tests.rscrates/rho/src/prompt.rscrates/rho/src/subagent.rscrates/rho/src/tools/advisor/advisor_tests.rscrates/rho/src/tools/advisor/mod.rscrates/rho/src/tools/agent/agent_tests.rscrates/rho/src/tools/agent/mod.rscrates/rho/src/tools/agent_output.rscrates/rho/src/tui/model_actions.rscrates/rho/src/tui/model_actions_tests.rs
Names resolved once per process and kept the answer, including a miss. The startup prefetch cannot beat the system prompt to the first lookup, so every model the prompt named pinned `None` and the names it fetched first appeared on the next launch. Catalog writes now drop that provider's resolved names, so the next text to name the model carries the name. Text already produced is untouched: an entry only changes when the catalog underneath it does.
App packaging verifies internal dependencies against crates.io, so the new `display_name` module and metadata prefetch must ship as an unpublished same-cut dependency.
There was a problem hiding this comment.
Important
The cache invalidation fix still permits an in-flight lookup to restore a stale result after the writer invalidates it.
Reviewed changes since the prior Pullfrog review at 1c04ce4, focusing on the catalog-cache fix and provider crate release.
- Invalidated display-name entries — Added provider-wide invalidation after models.dev and provider-model catalog writes, plus sequential regression coverage for cached misses.
- Released the provider API — Bumped
rho-providersto0.21.0and updated Rho's dependency, lockfile, and release manifest.
GPT Sol ｜ 𝕏
Review follow-ups. - A description is built from config ids and a downloaded catalog name, then written into one prompt line or one bracketed notice. A newline in any part added a line the executor reads as its own instruction. `describe` now replaces control characters with spaces. - A lookup that resolved before a catalog write landed could still cache its older answer over the new row, which is the miss the prefetch exists to clear. Writes now bump a generation the lookup checks before it caches. - Changing only the advisor reasoning level appended "advisor model switched to" the model the advisor already used. The comparison now uses the identity the notice reports. - A failed notice append in that branch left the store holding a reviewer the executor was never told about. It now restores the previous model, matching the enable and disable path.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at 119b8ce, covering the cache-race correction and prompt/advisor hardening.
- Guarded cache insertion — Added a catalog generation check so an in-flight pre-write lookup cannot restore stale model-name data after invalidation.
- Kept descriptions single-line — Replaced control characters in externally sourced model identity text before it reaches prompts or switch notices.
- Corrected advisor notices — Compared prompt-visible model identity to suppress reasoning-only switch notices and restored the previous advisor model when notice insertion fails.
- Expanded focused coverage — Added regression cases for single-line model descriptions and reasoning-only advisor changes.
GPT Sol ｜ 𝕏
…tices Delete the process-global Claude resolution store so PromptModel::describe is pure. Rename away from SDK ModelIdentity, share bind's model-policy path for prefetch prediction, drop the claude-cli model sentinel, move conversation switch notices into InteractiveRuntime with rollback, and route attach through the same formatter.
b793181
There was a problem hiding this comment.
Important
The new conversation-switch rollback can leave compaction bound to the rejected model.
Reviewed changes since the prior Pullfrog review at 001880f, covering the model-label ownership refactor and centralized switch handling.
- Made model labels value-based — Replaced ambient Claude alias state with
PromptModelvalues that preserve requested and resolved identities on each run. - Centralized switch notices — Moved conversation and advisor notice generation into shared model-label logic and made conversation switch failures restore the previous provider.
- Unified status rendering — Routed delegated output and TUI attachment identity lines through the same model representation, including honest unpinned-Claude labels.
- Aligned binding prediction — Shared Rho model policy application between launch binding and startup catalog prefetch prediction.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/rho/src/app/interactive_runtime.rs`:
- Around line 695-715: Update the append_user_context_with_display error path in
the interactive runtime to capture and handle refresh_compaction failure after
provider restoration instead of discarding it. Return an InvalidConfiguration
error that reports both the original switch-recording failure and compaction
rollback failure, while preserving the existing provider-rollback error path;
add a regression test covering this failed compaction rollback.
🪄 Autofix
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 Plus
Run ID: b8db6b77-79ac-4dab-b194-73861e722aa0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.release-please-manifest.jsoncrates/rho-providers/Cargo.tomlcrates/rho-providers/src/model/display_name.rscrates/rho-providers/src/model/display_name_tests.rscrates/rho-providers/src/model/models_dev.rscrates/rho-providers/src/model/provider_models.rscrates/rho/Cargo.tomlcrates/rho/src/app/agent_binding.rscrates/rho/src/app/agent_binding_tests.rscrates/rho/src/app/agent_executor.rscrates/rho/src/app/bootstrap.rscrates/rho/src/app/interactive_runtime.rscrates/rho/src/app/interactive_runtime_advisor.rscrates/rho/src/app/interactive_runtime_tests.rscrates/rho/src/app/tools_prompt.rscrates/rho/src/model_identity.rscrates/rho/src/model_identity_tests.rscrates/rho/src/prompt.rscrates/rho/src/tools/agent/agent_tests.rscrates/rho/src/tools/agent_output.rscrates/rho/src/tui/attachment/app.rscrates/rho/src/tui/attachment/app_tests.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/rho/src/app/tools_prompt.rs
- crates/rho-providers/src/model/display_name.rs
- crates/rho/src/app/bootstrap.rs
- crates/rho/src/app/interactive_runtime_advisor.rs
- crates/rho/src/tools/agent/agent_tests.rs
- crates/rho-providers/src/model/models_dev.rs
- crates/rho/src/app/interactive_runtime_tests.rs
- crates/rho/src/prompt.rs
…iction Finish the maintainability cleanup on model prompt labels: - Predict agent models only when bind would succeed; skip broken policies instead of inventing prefetch keys from unresolved selection text. - Own switch-notice wording entirely in prompt (ModelSwitchKind + compose); keep PromptModel as a pure label type. - Delete the run_prompt_model passthrough; construct labels from run status or SDK identity directly. - Share provider-switch restore in InteractiveRuntime so compaction and notice failures use one rollback path.
There was a problem hiding this comment.
Important
The latest refactor introduces no new issues, but it does not resolve the open compaction rollback concern: the restored provider's refresh_compaction() result is still discarded.
Reviewed changes since the prior Pullfrog review at b793181, covering the model-label cleanup and shared provider restoration.
- Skipped unbindable prefetch targets — Changed model prediction to return no catalog key when the corresponding agent policy cannot bind.
- Moved switch-notice policy — Relocated switch kinds and notice composition into prompt assembly while keeping
PromptModelfocused on model labels. - Removed residual wrappers — Constructed prompt models directly from SDK identity and run status at the remaining call sites.
- Shared provider restoration — Extracted provider rollback for compaction and notice failures, while leaving the existing post-restore compaction failure path unresolved.
- Expanded binding coverage — Added a focused test proving an invalid alias neither binds nor produces a speculative prefetch key.
GPT Sol ｜ 𝕏
- Move replace_provider into interactive_runtime_provider so the main runtime module stays under 1k and the switch path has one home. - Surface incomplete rollback when compaction cannot follow a restored provider after a failed model-switch notice (pullfrog/CodeRabbit). - Drop the agent test that only reasserted PromptModel::from_run_status. - Accept truncated Claude session ids on attach headers once resolved model text lengthens the identity line (PTY CI).
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at ee0866a, covering the provider-switch extraction and rollback correction.
- Restored compaction on rollback — Rebuilt compaction after restoring the previous provider when model-switch notice persistence fails, and surfaced an explicit combined error if that rebuild also fails.
- Extracted provider switching — Moved the complete provider transition into
interactive_runtime_provider.rswhile preserving transition completion, subagent selection, MCP sampling, and live-context invalidation ordering. - Adjusted focused coverage — Kept delegated model reconstruction coverage at its owner layer and updated the Claude PTY assertion for the longer resolved-model identity line.
GPT Sol ｜ 𝕏
1 participant
Summary
The agent had no way to know what model it was running on. That is a fact only Rho holds: the user picks the model, Rho can switch it mid-session, and the model itself can be newer than its own training data. The same blind spot covered its subagents and the advisor.
Now:
Model ids lead and catalog names follow, read from the models.dev
namefield:The id always leads because it is the part a reader can act on: it picks the provider route,
/modeltakes it back, and provider docs use it. A name is never invented from an id, so an unknown model shows its id alone rather than a guess.models.tomlgains adisplay_nameoverride for models no catalog knows, such as local Ollama builds.Claude Code aliases. Rho passes
--modelthrough untouched and the binary lists no models, soopusis a pointer Rho cannot follow. The only place the model behind it appears is thesystem/initframe at the start of a run, so Rho records what those frames report:That store is deliberately process-local. An alias points at whichever model is current, and a mapping saved to disk would outlive that and name a retired model with confidence.
Written text is never rewritten
The
agentandadvisortool descriptions name no model at all. Both would otherwise go stale or silently change what the caller was already told: an inheriting agent's model changes when the conversation model switches, a pinned model gains its name once the catalog prefetch lands, and/advisorswaps the reviewer without rebuilding the tool list. Every model fact instead lives where it is settled - the system prompt, a run's own output, or an appended notice.Catalog names resolve once per process, because each lookup otherwise opens a fresh sqlite connection and runs its schema statements on paths that format one per delegated run. A catalog write drops that provider's resolved names, so a name that lands during a session reaches the next text that names the model. Text already produced is untouched: an entry only changes when the catalog underneath it does.
Why names were missing for
openai-codexopenai-codexsells OpenAI models through Codex OAuth and has no models.dev entry of its own; it already readopenaiupstream, so parsing was never the problem. The gap was that the name cache is only ever filled by selecting a model, so a model the session merely names - a subagent target, an advisor - was never fetched. Startup now prefetches names for every model the session can name, in the background, with one models.dev download for the whole set rather than one per model. A warm cache does no network at all.Validation
Three defects found and fixed while validating:
run_model_linecalled the name lookup for every formatted snapshot, and each lookup opened a fresh sqlite connection and ran its schema statements. Under parallel tests that stalled the suite for minutes; it would also have hit liveagents list/status. Fixed by resolving names once per process.Review follow-ups on top of that:
describenow replaces control characters with spaces.Docs TUI proof plate
bash scripts/check_docs_ui_demo.sh --check(or--writeand committed dark SVGs + site light SVG)Test gate
rho-test-selection(failure mode, owner layer, gap).crates/rho/src/tuiunit tests are pure logic or justified below..containsonly for redaction, wire format, or security escaping.While applying the gate I merged three near-duplicate
resolved_modelstests into one table, dropped a test that only restated a match arm, and replaced prompt-prosecontainsasserts with assembly-seam asserts on the model reference.IsolatedRhoHomenow also isolatesHOME, because agent discovery reads~/.rho/agentsand a developer's own agent files changed what the catalog held.New tests
nameparsed wrong, or a blank name stored as a nameopenai-codex)--modelalias and the unpinned default collapse into one entry, or a blank report is storedinitframe states the model a run bound; another frame's model would mislabel the runadvisordescription names the reviewer and so rewrites what the executor was toldPTY exception (if any)
Breaking changes
None.
ModelMetadata::display_nameandRunStatus::claude_modelare additive with serde defaults. The models.dev cache version moves 7 to 8 so existing rows refetch and pick up names.rho-providersis cut to 0.21.0 in the same PR, because app packaging verifies internal dependencies against crates.io and the newdisplay_namemodule has to ship as an unpublished same-cut dependency.Written by Claude Opus 4.6 in Claude Code.
Summary by CodeRabbit
New Features
Bug Fixes
Tests