feat(config): mid-session edit tool, advisor, and auto preference - #840
Conversation
Rebuild the tool list on the next turn without rewriting the system prompt, and append a context notice with the new tool schema so prompt cache stays stable while the model learns the live surface.
📝 WalkthroughWalkthroughThe runtime now supports provider-aware edit-tool switching and advisor-tool changes during sessions. It keeps the system prompt stable, updates tool specifications and diagnostics, preserves sessions, appends model-facing notices, rejects active-turn changes, resplices affected history entries, and limits syntax highlighting for long lines. ChangesRuntime tool switching
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant App
participant InteractiveRuntime
participant AppToolSet
participant Session
participant RuntimeDiagnostics
Operator->>App: select edit tool
App->>InteractiveRuntime: apply edit-tool change
InteractiveRuntime->>AppToolSet: replace advertised tool
InteractiveRuntime->>Session: rebind current session
InteractiveRuntime-->>App: return notice and tool specs
App->>RuntimeDiagnostics: update edit-tool state
App-->>Operator: display status
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![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
🤖 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_advisor.rs`:
- Around line 61-65: Make the advisor transition in the method containing
rebind_current_session atomic: save the prior registration and model, and if
append_advisor_switch_notice fails after store.set_model, restore both previous
values and rebind the prior tool list before returning the error. Add a
failure-path test covering notice persistence failure and verifying state
restoration.
In `@crates/rho/src/tools/sdk_registry.rs`:
- Around line 348-365: Update the edit-tool replacement logic around the tools
collection position lookup to match only canonical built-in names from
EditFormat::ALL, rather than using EditFormat::is_edit_tool_name, while
preserving legacy alias handling for transcript classification. Add a regression
test registering an unrelated edit_file tool and verify it is not replaced or
exposed without ToolCapability::Edit.
In `@crates/rho/src/tui/advisor_command.rs`:
- Around line 213-226: Update the advisor-mode flow around set_advisor_mode and
the set_advisor call so a runtime application error is propagated instead of
only inserting an Entry::Error. Ensure persistence and in-memory advisor state
remain unchanged on failure, either by applying the runtime transition before
saving or restoring both states, and add coverage for
AdvisorRuntime::set_advisor returning an active-run error.
In `@crates/rho/src/tui/config_actions.rs`:
- Around line 596-610: In the edit-tool switch flow, call
diagnostics.update_tools(&agent.tool_specs()) immediately after the runtime
switch succeeds and before agent.notify_edit_tool_switch(previous, edit_tool).
Keep notice insertion and error handling unchanged so the diagnostic tool list
reflects the active tool even when adding the session notice fails.
🪄 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: e33f38e5-63a0-4c5e-8436-d801452205f3
📒 Files selected for processing (17)
crates/rho/src/app/automation.rscrates/rho/src/app/interactive_runtime.rscrates/rho/src/app/interactive_runtime_advisor.rscrates/rho/src/app/interactive_runtime_startup.rscrates/rho/src/app/interactive_runtime_tests.rscrates/rho/src/app/tools_prompt.rscrates/rho/src/app/tools_prompt_tests.rscrates/rho/src/builtin_skills/rho-config/SKILL.mdcrates/rho/src/diagnostics.rscrates/rho/src/diagnostics_tests.rscrates/rho/src/prompt.rscrates/rho/src/tools/sdk_registry.rscrates/rho/src/tools/sdk_registry_tests.rscrates/rho/src/tui/advisor_command.rscrates/rho/src/tui/advisor_command_tests.rscrates/rho/src/tui/config_actions.rscrates/rho/src/tui/during_turn.rs
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes across the mid-session edit-tool and advisor state transitions, including runtime rebinding, fixed-prompt behavior, diagnostics, user-facing controls, and their tests.
- Live edit-tool switching —
/confignow replaces the advertised edit surface, persists the selection, refreshes diagnostics, and gives the model the new schema without changing the system prompt. - Live advisor switching —
/advisornow rebuilds only the tool list, preserves the session and fixed prompt, and appends enable or disable context for the next turn. - Startup prompt assembly — advisor steering is selected once from startup registration rather than maintained as dual prompt variants.
- Transition safety — active turns reject these changes, and edit-tool failures restore the prior live selection where possible.
- Coverage and documentation — focused runtime, registry, prompt, and diagnostics tests accompany updated
rho-configguidance.
GPT Sol ｜ 𝕏
Keep a built-in preferred format catalog and let edit_tool=auto resolve it for the active provider, including mid-session provider switches, without rewriting the system prompt.
matthewyjiang
changed the title
There was a problem hiding this comment.
Important
The new auto preference works in construction, but its mid-session provider-switch contract has no regression coverage.
Reviewed changes since the prior Pullfrog review, focusing on provider-based edit-format resolution and the asynchronous model-handoff integration.
- Added automatic edit preference —
edit_tool = "auto"now resolves provider-specific concrete formats while pinned choices remain fixed. - Followed provider handoffs live — model selection now updates the advertised edit tool and appends the replacement schema when Auto resolves differently.
- Updated configuration surfaces —
/config, diagnostics, built-in guidance, and documentation now expose Auto and its resolved format. - Added focused construction coverage — config parsing, preference resolution, and initial registry construction now exercise the new Auto value.
GPT Sol ｜ 𝕏
Skip language highlighting on over-long tool-card lines (Markdown inline-code is pathological in syntect) and resplice only the toggled tool entry on expand/collapse so later transcript lines stay cached.
Move edit-tool runtime transitions into a dedicated module, model pinned preferences as EditTool::Pinned, keep the system prompt format-agnostic, and document that edit_tool applies before the next turn with Auto following provider switches.
There was a problem hiding this comment.
ℹ️ No new issues found in the latest commits. The existing provider-handoff coverage thread remains open.
Reviewed changes since the prior Pullfrog review, covering the four commits after 7d028e7.
- Extended provider preferences — Added xAI to the built-in
str_replacepreference and documented the first-party harness rationale. - Kept prompts tool-agnostic — Moved advisor steering into the live tool description and consolidated edit-tool switch context generation outside the fixed system prompt.
- Made edit-tool transitions cohesive — Combined runtime rebinding, schema notice insertion, diagnostics updates, and rollback into one shared apply path.
- Optimized tool-card expansion — Added surgical history-cache resplicing and bounded syntax highlighting for long tool-output lines without rebuilding the transcript suffix.
GPT Sol ｜ 𝕏
Make advisor tool-list changes atomic across notice failures, apply advisor mode to the runtime before persisting, match only canonical built-in edit tool names when swapping surfaces, and cover the failure paths with regression tests.
There was a problem hiding this comment.
Important
The new advisor rollback test does not exercise the partial-commit failure it is intended to cover, so a failed snapshot save can still leave contradictory model-visible history.
Reviewed changes since the prior Pullfrog review, covering the hardening commit after 8ed0e8f.
- Made advisor transitions reversible — Saved prior registration and model state, propagated active-run failures, and attempted runtime rollback after notice or config persistence errors.
- Restricted edit-tool replacement — Matched only canonical built-in edit surfaces so a legacy alias-named tool is not mistaken for the swappable slot.
- Added transition regression tests — Covered advisor active-run rejection, injected notice failure, and canonical edit-tool replacement behavior.
GPT Sol ｜ 𝕏
Drive select_model_report so Auto follows the preferred format for the new provider while a pinned preference stays fixed across the change.
Format recent test and history-cache changes, drop HistoryUi::get_mut after the surgical tool-expand path stopped using it, and route set_edit_tool through edit_tool so the getter is live under -D warnings.
There was a problem hiding this comment.
ℹ️ No new issues found in the latest commit. The existing advisor snapshot-persistence atomicity thread remains open.
Reviewed changes since the prior Pullfrog review, covering the provider-handoff regression coverage added in 701b345.
- Added provider-handoff coverage — Exercised
select_model_reportto verify that Auto follows an OpenAI-to-Anthropic provider change while a pinned hashline preference remains fixed. - Exposed a focused runtime fixture — Reused the live edit-capable runtime factory from TUI tests so the new test drives the production handoff seam.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/rho/src/tui/history_cache.rs (1)
117-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPending resplice marks are dropped when an assistant append falls back to
invalidate_from.If
self.resplicecontains an index lower thanindex, the fallback callsinvalidate_from(index).invalidate_fromclearsrespliceand setsdirty_from = index. The earlier entry is then never re-rendered, so a toggled tool card keeps its stale height until an unrelated full invalidation occurs.Fold the pending marks into the suffix rebuild instead of dropping them.
🛠️ Proposed fix in `invalidate_from`
pub(super) fn invalidate_from(&mut self, index: usize) { self.appended_assistant = None; - self.resplice.clear(); - self.dirty_from = Some(self.dirty_from.map_or(index, |dirty| dirty.min(index))); + let pending = self.resplice.drain(..).min(); + let index = pending.map_or(index, |pending| pending.min(index)); + self.dirty_from = Some(self.dirty_from.map_or(index, |dirty| dirty.min(index))); }Note:
ensure_currentat Line 230 clearsresplicetogether with a full rebuild (dirty_from = Some(0)), which stays correct with this change.🤖 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/history_cache.rs` around lines 117 - 131, Update assistant_appended and invalidate_from so pending resplice marks before the invalidation index are preserved and included in the suffix rebuild rather than cleared. Ensure dirty_from is adjusted to the earliest affected index, while retaining ensure_current’s full-rebuild behavior that clears resplice.crates/rho/src/tui/tool_search.rs (1)
103-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset parser state after skipping an overlong search row.
When
source.len() > MAX_TOOL_SYNTAX_LINE_BYTES, this branch returns without rebuildingself.highlighter.BlockHighlightercarries multiline parser state, so the next short line is parsed as if the skipped line did not exist. Recreate the highlighter fromself.pathbefore returning, asDiffSyntaxdoes for skipped rows.🤖 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/tool_search.rs` around lines 103 - 110, Reset self.highlighter from self.path in the overlong-row branch before returning the unhighlighted HighlightSegment, matching the skipped-row handling in DiffSyntax; preserve the existing behavior for highlighted-line limits and short sources.
🧹 Nitpick comments (6)
crates/rho/src/tui/history_cache.rs (1)
325-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the entry rendering logic with
push_rendered_entry.Lines 325-349 repeat the rendering steps of
push_rendered_entryat Lines 438-478: hidden-entry handling,render_entry_with_options, markdown image resolution, and thecopy_columns+1 padding adjustment. Two copies of the same mapping can diverge, and a divergence would produce wrong copy targets only on the resplice path.Extract one helper that returns the rendered lines, relative code blocks, and image placement, then let both call sites position the results.
🤖 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/history_cache.rs` around lines 325 - 349, Extract the shared entry-rendering logic from the current block and push_rendered_entry into a helper that handles hidden entries, render_entry_with_options, image resolution, and copy_columns padding, returning lines, relative code blocks, and image placement. Update both call sites to use this helper, while keeping their existing responsibilities for positioning or relocating the returned results.crates/rho/src/prompt.rs (1)
281-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the label from
spec.nameinstead of a separatenameparameter.Both call sites pass the spec's own name:
tool_schema_block("advisor", spec)andtool_schema_block(current_name, spec). The extra parameter lets the printed label and the printed schema describe different tools. Usespec.nameand drop the parameter.♻️ Proposed refactor
-fn tool_schema_block(name: &str, spec: &ToolSpec) -> String { +fn tool_schema_block(spec: &ToolSpec) -> String { let schema = serde_json::to_string_pretty(&spec.input_schema).unwrap_or_else(|_| "{}".into()); format!( - "Tool schema for `{name}`:\n\ + "Tool schema for `{name}`:\n\ description:\n\ {description}\n\n\ input_schema:\n\ {schema}", + name = spec.name, description = spec.description, - schema = schema, ) }Update the two call sites to
tool_schema_block(spec).🤖 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/prompt.rs` around lines 281 - 292, Update tool_schema_block to remove the separate name parameter and derive the printed tool label from spec.name, ensuring the label always matches the schema. Adjust both call sites to pass only the ToolSpec argument.Source: Coding guidelines
crates/rho/src/config.rs (1)
282-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
all()duplicates theEditFormatvariant list and can drift.
all()hardcodes four entries.FromStraccepts any value thatEditFormat::from_config_valueresolves. If a new format is added torho-tools,FromStraccepts it, but the picker list and theFromStrerror text omit it. Consider building the pinned entries fromrho_tools::EditFormat::ALLand returning aVec<Self>or a const-sized array derived fromALL.len().🤖 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/config.rs` around lines 282 - 289, Update EditFormat’s all() method to derive pinned variants from rho_tools::EditFormat::ALL instead of hardcoding the current four formats, and adjust its return type or const array length to match ALL.len(). Preserve the Auto entry while ensuring newly added EditFormat values are included automatically and remain consistent with FromStr.crates/rho/src/tools/sdk_registry.rs (1)
371-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe mid-session rebuild duplicates the edit-tool construction policy.
coding::sdk_bundlebuilds the edit tool withmax_output_bytes,edit_tool, andmutation_observer. This block repeats the same three options. The two constructions match today. Ifsdk_bundlelater adds an option, the mid-session swap silently drops it and the replacement tool behaves differently from the startup tool.Consider exposing one constructor in
crates/rho/src/tools/coding.rs(for examplepub(super) fn edit_tool(format, max_output_bytes, observer) -> Arc<dyn Tool>) and calling it from both places.🤖 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/sdk_registry.rs` around lines 371 - 379, Centralize edit-tool construction so the mid-session rebuild and startup path share one policy. Add a reusable constructor in coding.rs that accepts the required format, max_output_bytes, and mutation observer, then update sdk_bundle and the replacement logic around self.tools[position] to call it instead of independently chaining CodingToolOptions.crates/rho/src/app/interactive_runtime_tests.rs (1)
863-894: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe header comment does not describe this test, and most assertions lock a static table.
The comment claims coverage of tool-set construction. The body never builds a tool set. It asserts the
preferred_edit_format_for_providerlookup table directly.auto_edit_tool_constructs_the_preferred_provider_formatincrates/rho/src/tools/sdk_registry_tests.rsalready covers construction.Three changes:
- Correct the
Covers:/Owner:lines to state that this checksEditToolresolution and label composition.- Drop the raw table assertions on lines 870-885. They restate the match arms in
crates/rho/src/config.rswithout exercising behavior. Keep theresolveanddisplay_labelassertions, which compose two functions.- Move the test next to the owning code,
crates/rho/src/config_load_tests.rs. It reads no interactive-runtime state.The test guidelines direct you to avoid static-constant tests and to keep tests with their owning code.
🤖 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 863 - 894, Update auto_edit_tool_resolves_preferred_format_for_provider to describe coverage of EditTool resolution and display-label composition, with the owning area identified accordingly. Remove the direct preferred_edit_format_for_provider table assertions, retain the EditTool::Auto.resolve and display_label behavior assertions, and move the test into the owning config_load_tests.rs module rather than interactive_runtime_tests.rs.Source: Coding guidelines
crates/rho/src/tui/advisor_command_tests.rs (1)
299-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a typed runtime error assertion.
The string-contains assertion only matches copied error text. Define a local active-run error type and assert that the propagated
anyhow::Errorcontains that type. Keep the saved and in-memory state assertions.As per coding guidelines, “avoid ... 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/tui/advisor_command_tests.rs` around lines 299 - 325, Replace the copied-message assertion in the active-run test around ActiveRunRuntime with a local typed active-run error, return that error from set_advisor, and assert the propagated anyhow::Error contains the typed error via downcast/typed inspection. Preserve the existing saved and in-memory state assertions.Source: Coding guidelines
🤖 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_edit_tool.rs`:
- Around line 38-51: Update set_edit_tool’s append_edit_tool_switch_notice error
path to capture failures from both restoring self.tools and
rebind_current_session().await. If either rollback step fails, compose and
return an error that reports the rollback failure instead of silently ignoring
it; preserve the original notice error when restoration succeeds.
In `@crates/rho/src/tui/config_actions.rs`:
- Around line 648-657: Update the set_edit_tool flow so the Ok(change) branch
returns the change without calling diagnostics.update_tools or insert_entry;
perform those UI effects only after apply_edit_tool persistence succeeds.
Preserve the immediate diagnostics and notice updates in the Auto
provider-switch path, which does not persist a preference.
In `@crates/rho/src/tui/tool_diff.rs`:
- Around line 133-139: Update paint_content to restart only the affected side
when an overlong row skips syntax highlighting: use the side argument to reset
the new highlighter for DiffRowKind::Added rows and the old highlighter for
removed rows, preserving the existing both-side restart behavior for context
rows.
---
Outside diff comments:
In `@crates/rho/src/tui/history_cache.rs`:
- Around line 117-131: Update assistant_appended and invalidate_from so pending
resplice marks before the invalidation index are preserved and included in the
suffix rebuild rather than cleared. Ensure dirty_from is adjusted to the
earliest affected index, while retaining ensure_current’s full-rebuild behavior
that clears resplice.
In `@crates/rho/src/tui/tool_search.rs`:
- Around line 103-110: Reset self.highlighter from self.path in the overlong-row
branch before returning the unhighlighted HighlightSegment, matching the
skipped-row handling in DiffSyntax; preserve the existing behavior for
highlighted-line limits and short sources.
---
Nitpick comments:
In `@crates/rho/src/app/interactive_runtime_tests.rs`:
- Around line 863-894: Update
auto_edit_tool_resolves_preferred_format_for_provider to describe coverage of
EditTool resolution and display-label composition, with the owning area
identified accordingly. Remove the direct preferred_edit_format_for_provider
table assertions, retain the EditTool::Auto.resolve and display_label behavior
assertions, and move the test into the owning config_load_tests.rs module rather
than interactive_runtime_tests.rs.
In `@crates/rho/src/config.rs`:
- Around line 282-289: Update EditFormat’s all() method to derive pinned
variants from rho_tools::EditFormat::ALL instead of hardcoding the current four
formats, and adjust its return type or const array length to match ALL.len().
Preserve the Auto entry while ensuring newly added EditFormat values are
included automatically and remain consistent with FromStr.
In `@crates/rho/src/prompt.rs`:
- Around line 281-292: Update tool_schema_block to remove the separate name
parameter and derive the printed tool label from spec.name, ensuring the label
always matches the schema. Adjust both call sites to pass only the ToolSpec
argument.
In `@crates/rho/src/tools/sdk_registry.rs`:
- Around line 371-379: Centralize edit-tool construction so the mid-session
rebuild and startup path share one policy. Add a reusable constructor in
coding.rs that accepts the required format, max_output_bytes, and mutation
observer, then update sdk_bundle and the replacement logic around
self.tools[position] to call it instead of independently chaining
CodingToolOptions.
In `@crates/rho/src/tui/advisor_command_tests.rs`:
- Around line 299-325: Replace the copied-message assertion in the active-run
test around ActiveRunRuntime with a local typed active-run error, return that
error from set_advisor, and assert the propagated anyhow::Error contains the
typed error via downcast/typed inspection. Preserve the existing saved and
in-memory state assertions.
In `@crates/rho/src/tui/history_cache.rs`:
- Around line 325-349: Extract the shared entry-rendering logic from the current
block and push_rendered_entry into a helper that handles hidden entries,
render_entry_with_options, image resolution, and copy_columns padding, returning
lines, relative code blocks, and image placement. Update both call sites to use
this helper, while keeping their existing responsibilities for positioning or
relocating the returned results.
🪄 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: d2fd6ec6-0b5a-4418-b537-f8ad10bff974
📒 Files selected for processing (37)
crates/rho/src/app/automation.rscrates/rho/src/app/interactive_runtime.rscrates/rho/src/app/interactive_runtime_advisor.rscrates/rho/src/app/interactive_runtime_edit_tool.rscrates/rho/src/app/interactive_runtime_startup.rscrates/rho/src/app/interactive_runtime_tests.rscrates/rho/src/app/tools_prompt.rscrates/rho/src/app/tools_prompt_tests.rscrates/rho/src/builtin_skills/rho-config/SKILL.mdcrates/rho/src/config.rscrates/rho/src/config_load_tests.rscrates/rho/src/prompt.rscrates/rho/src/tools/advisor/mod.rscrates/rho/src/tools/coding.rscrates/rho/src/tools/sdk_registry.rscrates/rho/src/tools/sdk_registry_tests.rscrates/rho/src/tui/advisor_command.rscrates/rho/src/tui/advisor_command_tests.rscrates/rho/src/tui/config_actions.rscrates/rho/src/tui/config_picker.rscrates/rho/src/tui/context_handoff.rscrates/rho/src/tui/during_turn.rscrates/rho/src/tui/feed_image.rscrates/rho/src/tui/history_cache.rscrates/rho/src/tui/history_cache_tests.rscrates/rho/src/tui/model_actions.rscrates/rho/src/tui/model_actions_tests.rscrates/rho/src/tui/prompt_turn.rscrates/rho/src/tui/syntax.rscrates/rho/src/tui/tool_diff.rscrates/rho/src/tui/tool_diff_tests.rscrates/rho/src/tui/tool_output_ui.rscrates/rho/src/tui/tool_search.rsdocs/configuration.mddocs/configuration/full-example.mddocs/tools-workspace.mddocs/tools-workspace/edit-format.md
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/rho/src/app/automation.rs
- crates/rho/src/app/interactive_runtime_startup.rs
- crates/rho/src/app/tools_prompt_tests.rs
- crates/rho/src/app/interactive_runtime_advisor.rs
Append-before-save left model-visible history ahead of a failed snapshot. Make context append restore prior history on save failure, have advisor rollback do the same, and inject the failure after a successful append so the regression covers the partial-commit path.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review, covering the final CI cleanup and snapshot-persistence rollback hardening.
- Restored model-visible history — Added an idle-session SDK history replacement primitive and now rolls back appended context when durable snapshot persistence fails.
- Exercised the partial commit — Moved the advisor failure injection after the live append and verified registration, model, runtime tools, and history all return to their prior state.
- Completed CI cleanup — Applied the formatting and lint adjustments required by the branch checks.
GPT Sol ｜ 𝕏
…paths Report rollback failures on notice errors, defer edit-tool UI mirrors until config save succeeds, preserve pending resplice marks across invalidation, and restart only the affected syntax stream on overlong rows. Also share edit-tool construction and keep Auto provider-follow from overwriting model-switch status.
There was a problem hiding this comment.
Important
A failed edit-tool config save can leave the persisted transcript different from the transcript shown in the current session.
Reviewed changes since the prior Pullfrog review, covering the hardening commit after fcb25aa.
- Surfaced edit-tool rollback failures — Reported failures while restoring the previous tool or rebinding the prior runtime after notice persistence fails.
- Deferred edit-tool UI mirrors — Moved diagnostics and visible switch notices after successful preference persistence while retaining immediate Auto provider-follow updates.
- Hardened history-cache resplicing — Preserved pending surgical invalidations and shared entry rendering between full rebuild and resplice paths.
- Kept syntax streams independent — Restarted only the affected diff side after skipping an overlong line and reset search highlighting state after the same condition.
- Tightened ownership and coverage — Centralized edit-tool construction, moved preference tests to config ownership, and aligned supporting CLI expectations.
GPT Sol ｜ 𝕏
Package verify resolves published rho-sdk 1.17.3 from crates.io, which lacks replace_history. Bump the workspace sdk cut so publish prep path-patches the unpublished surface used by advisor/edit-tool rollback.
There was a problem hiding this comment.
ℹ️ No new issues found in the latest commit. The existing edit-tool rollback history-alignment thread remains open.
Reviewed changes since the prior Pullfrog review, covering the release preparation added in 4a244e1.
- Prepared the SDK minor release — Bumped
rho-sdkfrom1.17.3to1.18.0for the additiveSession::replace_historyAPI. - Aligned workspace requirements — Updated the release manifest, lockfile, and all workspace path dependency version requirements to the same SDK release.
- Checked compatibility debt — Confirmed the additive API is a clean minor-release shape and requires no
NEXT_MAJOR(...)marker.
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/tests/automation_cli.rs`:
- Around line 87-88: Update the xAI auto-edit expectation in the automation CLI
test from str_replace to hashline, matching the resolver contract that uses
hashline for non-Anthropic providers. Do not change the resolver or
documentation unless xAI is intentionally meant to use str_replace.
🪄 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: 823782c0-f15d-4f8c-9b5d-7de82a6d4214
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.release-please-manifest.jsoncrates/rho-providers/Cargo.tomlcrates/rho-sdk/Cargo.tomlcrates/rho-sdk/src/session.rscrates/rho-sdk/src/session_tests.rscrates/rho-tools/Cargo.tomlcrates/rho/Cargo.tomlcrates/rho/src/app/interactive_runtime.rscrates/rho/src/app/interactive_runtime_advisor.rscrates/rho/src/app/interactive_runtime_edit_tool.rscrates/rho/src/app/interactive_runtime_tests.rscrates/rho/src/config.rscrates/rho/src/config_load_tests.rscrates/rho/src/prompt.rscrates/rho/src/tools/coding.rscrates/rho/src/tools/sdk_registry.rscrates/rho/src/tools/sdk_registry_tests.rscrates/rho/src/tui/advisor_command_tests.rscrates/rho/src/tui/app_state/history_ui.rscrates/rho/src/tui/config_actions.rscrates/rho/src/tui/history_cache.rscrates/rho/src/tui/model_actions_tests.rscrates/rho/src/tui/tool_diff.rscrates/rho/src/tui/tool_diff_tests.rscrates/rho/src/tui/tool_search.rscrates/rho/tests/automation_cli.rs
💤 Files with no reviewable changes (1)
- crates/rho/src/tui/app_state/history_ui.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- crates/rho/src/app/interactive_runtime_edit_tool.rs
- crates/rho/src/tui/tool_search.rs
- crates/rho/src/tools/sdk_registry_tests.rs
- crates/rho/src/config_load_tests.rs
- crates/rho/src/tools/sdk_registry.rs
- crates/rho/src/tui/tool_diff_tests.rs
- crates/rho/src/config.rs
- crates/rho/src/tui/history_cache.rs
- crates/rho/src/app/interactive_runtime_advisor.rs
- crates/rho/src/tui/advisor_command_tests.rs
- crates/rho/src/prompt.rs
- crates/rho/src/tui/tool_diff.rs
- crates/rho/src/app/interactive_runtime_tests.rs
- crates/rho/src/tui/config_actions.rs
When config save fails after a live edit-tool switch, mirror the forward and reverse transitions already written to model/display history into the transcript so UI, session context, and runtime state agree.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog run, covering the rollback-history alignment fix in f4ba0a0.
- Aligned failed-save histories — Mirrored the persisted forward and reverse edit-tool switch notices into the current transcript when config persistence fails and runtime rollback succeeds.
- Preserved partial-failure visibility — Mirrored the committed forward notice when runtime rollback itself fails, while retaining the composed error.
- Added regression coverage — Exercised a real config-write failure and checked runtime restoration, model history, transcript ordering, status, and preference diagnostics.
- Checked compatibility debt — Confirmed the delta changes only private TUI behavior and introduces no next-major API compromise.
Validation note: cargo test -p rho-coding-agent --lib -- failed_edit_tool_save_keeps_rollback_histories_aligned timed out after 120 seconds during compilation.
GPT Sol ｜ 𝕏
Windows ignores directory readonly for creates, so the rollback history alignment test could not force a config save failure there. Inject a one-shot ConfigRepository save error instead, matching other fail_next test hooks.
There was a problem hiding this comment.
Important
The new save-failure injection is not deterministic across Tokio worker threads, so the rollback regression can miss the failure path it is intended to protect.
Reviewed changes since the prior Pullfrog review, covering the test portability fix in 81cf87d.
- Replaced OS-specific failure setup — Moved edit-tool save-failure injection into
ConfigRepositoryand removed platform-dependent directory permission manipulation from the rollback test. - Preserved rollback assertions — Kept coverage of runtime restoration, model and transcript notice ordering, status, and unchanged diagnostics after a failed preference save.
- Checked compatibility debt — Confirmed the delta is test-only internal behavior and introduces no next-major API compromise.
GPT Sol ｜ 𝕏
Store the one-shot fail flag on ConfigRepository behind Arc<AtomicBool> so clones and Tokio worker hops still observe it. Assert the injected error text in the rollback test so a successful save cannot pass.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/rho/src/app/config_repository.rs (1)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGate the test-only imports.
Non-test builds remove every use of
AtomicBool,Ordering, andArc, but they retain this import. Add#[cfg(test)]to the import so normal builds do not produce unused-import diagnostics.Proposed fix
+#[cfg(test)] use std::sync::{ atomic::{AtomicBool, Ordering}, Arc, };As per coding guidelines, follow Clippy and rustfmt style in Rust.
🤖 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/config_repository.rs` around lines 3 - 6, Gate the std::sync import containing AtomicBool, Ordering, and Arc with #[cfg(test)] so it is compiled only for tests and omitted from normal builds.Source: Coding guidelines
🤖 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 `@crates/rho/src/app/config_repository.rs`:
- Around line 3-6: Gate the std::sync import containing AtomicBool, Ordering,
and Arc with #[cfg(test)] so it is compiled only for tests and omitted from
normal builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 380b0055-de0d-4b69-bfcc-61ab0e8d00f6
⛔ Files ignored due to path filters (1)
fixtures/downstream/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/rho/src/app/config_repository.rscrates/rho/src/app/config_repository_tests.rscrates/rho/src/tui/config_actions.rscrates/rho/src/tui/config_actions_tests.rs
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review, covering the save-failure injection hardening in c660b97.
- Scoped failure injection — Replaced executor-thread-local state with a repository-owned one-shot flag shared across clones.
- Hardened regression coverage — Added direct clone and one-shot behavior coverage and made the rollback test fail closed unless it observes the injected save error.
- Retired prior feedback — Verified and resolved the open test-determinism thread.
- Checked compatibility debt — Confirmed the delta remains test-only internal behavior and introduces no next-major API compromise.
Validation: injected_save_failure_is_instance_scoped_and_shared_by_clones and failed_edit_tool_save_keeps_rollback_histories_aligned both pass.
GPT Sol ｜ 𝕏
1 participant
Summary
edit_toolandadvisor_modechanges mid-session without rewriting the system prompt (prompt-cache stable). Live switches rebuild the tool list and append a context notice with the new schema.edit_tool = "auto"with a built-in preferred-format catalog per provider. Auto resolves at startup and on mid-session provider changes; pinned formats stay fixed.auto (<resolved>). Directconfig.tomledits remain restart-only.Design choices
openai-codex→apply_patch,anthropic→str_replace, elsehashline). Not user-configured yet.autoor a pinned concrete format.Validation
cargo fmt --allpython3 scripts/check_architecture.pycargo test -p rho-coding-agent --lib -- edit_tool auto_edit_tool preferred_editcargo test -p rho-coding-agent --lib -- tui::model_actions tui::context_handoff tui::configTest gate
Summary by CodeRabbit
New Features
Bug Fixes
Documentation