feat(model): multi-account rotation + model picker - #421
Merged
Conversation
Contributor
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
Contributor
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds configurable API account rotation and model fallback, integrates recovery into LLM sessions and shell model management, introduces picker actions and localized UI text, centralizes skill metadata parsing, makes skill installation transactional, and exposes shared auto-vet state. ChangesAccount and model rotation
Skill metadata and transactional installation
Auto-vet state access
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AiHandler
participant LlmSession
participant RotationState
participant Provider
User->>AiHandler: Select model or account
AiHandler->>RotationState: Rebuild and apply state
AiHandler->>LlmSession: Set rotation
LlmSession->>RotationState: Resolve current credential
LlmSession->>Provider: Send chat completion
Provider-->>LlmSession: Return success or classified failure
LlmSession->>RotationState: Advance account or fallback model
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 18
🧹 Nitpick comments (9)
crates/aish-skills/src/registry/mod.rs (1)
319-336: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSilently swallowed rollback/cleanup errors can lose the skill directory without any diagnostic.
On the failure path,
remove_dir_all(&live)andrename(s, &live)both discard theirResultvialet _ =, with no logging — unlike the stash-creationrenameabove (lines 304-308) which surfaces a descriptive error. If the restorerenamefails, the skill effectively vanishes fromlivewhile the original content is stranded under the hidden.{slug}.reinstall-*stash path with no trace in the logs to explain why. The same applies to the success-path stash cleanup at line 322-324 (a failed cleanup just leaks a hidden dir silently).Consider at least logging a
tracing::warn!on these failure paths so an inconsistent post-install state is diagnosable instead of silent.🩹 Proposed fix: log rollback failures
Err(e) => { // Failure: drop the failed replacement and restore the original. - let _ = std::fs::remove_dir_all(&live); - if let Some(s) = stash { - let _ = std::fs::rename(s, &live); + if let Err(re) = std::fs::remove_dir_all(&live) { + tracing::warn!(slug = %skill.slug, error = %re, "failed to remove failed replacement"); + } + if let Some(s) = stash { + if let Err(re) = std::fs::rename(&s, &live) { + tracing::warn!(slug = %skill.slug, stash = ?s, error = %re, "failed to restore stashed skill after failed install"); + } } Err(e) }🤖 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/aish-skills/src/registry/mod.rs` around lines 319 - 336, Update the success and failure cleanup paths in the surrounding reinstall operation to log cleanup errors with tracing::warn! instead of discarding the Results. Cover stash removal, live-directory removal, and restoration rename failures, including the relevant paths and error details while preserving the existing return and rollback behavior.crates/aish-skills/src/manager.rs (1)
302-335: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the compiled frontmatter regex instead of recompiling per call.
parse_skill_metadatanow compilesFRONTMATTER_REGEXviaregex::Regex::new(...)on every invocation. This function is the shared chokepoint called from the skill loader (once perSKILL.mdduring every reload), the installer'svalidate_installed_skill(every install), and the verifier'sverify_skill_dir(every verify) — centralizing the parse also centralized (and multiplied) this recompilation cost. Compiling a regex is non-trivial relative to a simple match, and it's now on several hot/user-facing paths.♻️ Proposed fix: compile once with a lazily-initialized static
+static FRONTMATTER_RE: std::sync::LazyLock<regex::Regex> = + std::sync::LazyLock::new(|| regex::Regex::new(FRONTMATTER_REGEX).expect("valid frontmatter regex")); + pub fn parse_skill_metadata(content: &str) -> aish_core::Result<(SkillMetadata, &str)> { - let re = regex::Regex::new(FRONTMATTER_REGEX) - .map_err(|e| aish_core::AishError::Skill(format!("Invalid frontmatter regex: {}", e)))?; - let caps = re.captures(content).ok_or_else(|| { + let caps = FRONTMATTER_RE.captures(content).ok_or_else(|| { aish_core::AishError::Skill( "Invalid skill file format: must start with YAML frontmatter".into(), ) })?;Note:
INSTALL_LOCKSinregistry/mod.rsalready usesstd::sync::LazyLockfor a similar static-init pattern, so this stays consistent with the codebase's existing idiom.🤖 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/aish-skills/src/manager.rs` around lines 302 - 335, Update parse_skill_metadata to use a lazily initialized static LazyLock<regex::Regex> for FRONTMATTER_REGEX, compiling the pattern only once and reusing it for all calls. Preserve the existing invalid-regex error behavior as appropriate for static initialization, while leaving frontmatter extraction and validation unchanged.crates/aish-shell/src/app.rs (4)
5119-5164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the account name before prompting for the key.
The duplicate-name check at Line 5154 runs after the user has already typed a secret API key and an endpoint; on collision everything is thrown away and the flow restarts from scratch. Move the check directly after the name prompt.
🤖 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/aish-shell/src/app.rs` around lines 5119 - 5164, Move the existing duplicate-name check using self.config.api_accounts and name to immediately after the name prompt succeeds, before prompt_edit_value requests the API key or base endpoint. Preserve the current already_exists message and early return, and remove the later redundant check.
5481-5505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEndpoint collection + fetch is copy-pasted from the entry block.
Lines 5484-5504 repeat Lines 5373-5394 verbatim. Extract a
fn collect_endpoints(&self) -> Vec<(String, String)>and afn fetch_all_models(endpoints) -> Vec<(String, String)>so a fix to either (the dedupe key, timeout, concurrency) lands in one place.🤖 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/aish-shell/src/app.rs` around lines 5481 - 5505, Extract the duplicated endpoint-building and model-fetching logic from the entry flow and the `PanelOutcome::Submitted(SearchSelectOutcome::Action('a', _))` branch into shared `collect_endpoints` and `fetch_all_models` methods. Replace both inline blocks with calls to these methods, preserving endpoint deduplication, disabled-account filtering, API key handling, fetch timeout, and fetched-model output behavior.
5387-5394: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSerial blocking
/modelsfetches gate the panel with no feedback.Each endpoint costs up to 4s and the calls run sequentially before anything renders, so a handful of configured accounts (some unreachable) leaves
/modelfrozen for tens of seconds. Consider fetching concurrently, and/or printing a "fetching models…" line before the loop so the delay is attributable.🤖 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/aish-shell/src/app.rs` around lines 5387 - 5394, The endpoint model fetches in the loop over endpoints run serially without user feedback, delaying panel rendering. Update the fetch flow around fetch_models_from_api to run endpoint requests concurrently where supported, and emit a “fetching models…” status before waiting; preserve aggregation of each model with its originating base.
342-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRotation-state construction is duplicated and already diverging. Both sites build the same
RetryPolicy+RotationStateand apply the sameis_active()gate, but only one restores the previously active account — exactly the kind of drift a shared factory prevents (and it would remove theDefault+ field-reassign pattern flagged by Clippy'sfield_reassign_with_default).
crates/aish-shell/src/app.rs#L342-L366: extract the policy/RotationState/is_activeblock intofn rotation_state_from_config(config: &ConfigModel) -> Option<RotationState>(buildingRetryPolicywith struct-update syntax) and call it here.crates/aish-shell/src/app.rs#L5334-L5358: call the same helper, then apply only therebuild-specific step — re-selectingprevviause_account— beforeapply_rotation_state.🤖 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/aish-shell/src/app.rs` around lines 342 - 366, Extract the duplicated rotation construction into rotation_state_from_config(config: &ConfigModel) -> Option<RotationState), using RetryPolicy struct-update syntax and retaining the is_active gate. In crates/aish-shell/src/app.rs lines 342-366, replace the inline construction with this helper and set the session rotation when it returns Some. In crates/aish-shell/src/app.rs lines 5334-5358, call the same helper, re-select prev with use_account as the rebuild-specific step, then pass the result to apply_rotation_state.crates/aish-llm/src/session.rs (1)
522-542: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winError path takes the rotation lock three times, so concurrent turns can over-advance.
advance_on_error,model_exhaustion_error, andcurrenteach re-lock (Lines 532, 535, 540). If two in-flight completions on the same session both fail (agent loops share oneLlmSession), they can each advance rotation for the same outage, silently burning an extra account or fallback slot. Holding one guard for the whole failure handling would make the transition atomic.♻️ Single-guard failure handling
- let advanced = rotation.lock().unwrap().advance_on_error(kind); - if !advanced { - if kind == FailureKind::ModelError { - let msg = rotation.lock().unwrap().model_exhaustion_error(); - return Err(AishError::Llm(msg)); - } - return Err(err); - } - let next = rotation.lock().unwrap().current(&self.stream_ctx.api_base); - tracing::info!(credential = %next.label, "rotated to next credential/model"); + let next_label = { + let mut guard = rotation.lock().unwrap(); + if !guard.advance_on_error(kind) { + if kind == FailureKind::ModelError { + return Err(AishError::Llm(guard.model_exhaustion_error())); + } + return Err(err); + } + guard.current(&self.stream_ctx.api_base).label + }; + tracing::info!(credential = %next_label, "rotated to next credential/model");🤖 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/aish-llm/src/session.rs` around lines 522 - 542, Update the Err handling in the session request flow to acquire the rotation lock once and retain that guard across advance_on_error, model_exhaustion_error, and current. Use the single guard for all failure-path operations so credential/model advancement and selection are atomic, while preserving the existing return behavior.crates/aish-config/src/model.rs (2)
594-596: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
weightis accepted but never used.RotationState::pick_available_accountis plain round-robin (crates/aish-llm/src/rotation.rs), so a user settingweight: 10gets no behavior change. Either document it as reserved in the user-facing config docs or drop it until weighted selection lands, so the config surface doesn't promise something it doesn't do.🤖 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/aish-config/src/model.rs` around lines 594 - 596, Resolve the unused weight field in the configuration model: either remove weight and its default from the relevant model and config surface, or mark it explicitly as reserved in the user-facing configuration documentation until weighted selection is implemented. Ensure RotationState::pick_available_account does not imply weight is currently honored.
513-516: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAvoid persisting NUL-delimited entries in
recent_models.
crates/aish-shell/src/app.rs:5564stores model switches as"{model}\u{0}{base}", thenConfigLoader::savewritesConfigModelthroughserde_yaml_string;serde_yamldoes not support rawU+0000in scalars, so this can become invalid or non-portable YAML and can break config round-tripping or editors. Store a small struct instead, e.g.RecentModel { model, api_base }, and remove the manualsplit_once('\u{0}')parsing.🤖 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/aish-config/src/model.rs` around lines 513 - 516, Replace the String-based recent_models representation in ConfigModel with a serializable RecentModel struct containing model and api_base fields. Update the app.rs model-switch storage path to construct RecentModel values instead of concatenating with a NUL delimiter, and update consumers to access the fields directly rather than using split_once('\u{0}').
🤖 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/aish-i18n/locales/de-DE.yaml`:
- Line 289: Update the fetch_failed_subtitle translation to use neutral wording
that does not claim the endpoint is unreachable when the model list is merely
empty. Preserve the existing key and indicate only that no models are currently
available or that only current/history options can be used.
In `@crates/aish-i18n/locales/es-ES.yaml`:
- Around line 292-315: Correct the missing ñ in the Spanish locale entries
action_add, fallback_added, and manage_footer: use “añadir” and “Añadido”
consistently with the correctly accented added and no_extra_hint strings,
without changing unrelated translations.
- Around line 287-372: In crates/aish-i18n/locales/es-ES.yaml (lines 287-372),
fr-FR.yaml (lines 287-372), ja-JP.yaml (lines 287-372), and zh-CN.yaml (lines
632-717), move the picker_title through manage_empty block under shell.model,
and restore conclusion_fixed through no_evidence under shell.failure_diagnose;
apply the same correction to de-DE.yaml. Preserve all translations and existing
key contents while fixing the YAML parent nesting.
In `@crates/aish-llm/src/rotation.rs`:
- Around line 433-480: Update current_account_name() and snapshot() so they
resolve the current account through the same enabled-account fallback behavior
as current(). When current_account points to a disabled account, return the
first enabled account’s name instead of the disabled name or None, while
preserving the existing selection for enabled accounts and the empty result when
no enabled account exists.
In `@crates/aish-llm/src/session.rs`:
- Around line 498-544: Update the rotation-aware request flow around
stream_simple and the model reporting methods model_name()/resolved_model_name()
so the successfully used fallback credential/model is reflected in observability
and the shell response footer. Reuse rotation_snapshot() or record the resolved
model from the successful attempt, ensuring reporting prefers the active rotated
model while preserving primary-model behavior when no rotation is active.
In `@crates/aish-shell/src/app.rs`:
- Around line 5373-5386: Update endpoint de-duplication in the endpoint
construction flow to distinguish accounts sharing an api_base but having
different api_key values, using the (base, key) pair or equivalent account
identity. Ensure the select branch lookup and switch_endpoint_model resolve the
selected model to its intended account credentials rather than the first
endpoint inserted for that base.
- Around line 5614-5632: Add a y/N confirmation using the existing
confirm_action helper before deleting the account in the SearchSelectOutcome
action 'd' branch. Only execute api_accounts.retain and
persist_config_and_rebuild_rotation when confirmation is affirmative; otherwise
leave the stored credential unchanged, including when deletion is triggered with
an empty query.
- Around line 371-391: Reserve the synthetic "primary" account name across the
rotation-account flow: update unique_account_name and accounts_add_interactive
to reject or rename user accounts named "primary", and ensure any
account-labeling paths apply the same rule. Preserve unambiguous use_account,
current_rotation_account, rebuild_rotation, and manage-panel delete-by-name
behavior.
- Around line 5526-5554: Update the endpoint_changed handling so the outgoing
account added with prev_key and prev_base is not removed when the incoming key
is the same but the base differs. In the api_accounts retain logic, only remove
existing entries matching both the incoming credential and incoming API base, or
move the retain before the preservation push; preserve removal of the target
account without discarding the saved outgoing endpoint.
- Around line 5321-5329: Update the translation key passed to t_with_args in the
config-save warning error path to the existing shell.config_save_warning key.
Preserve the current error argument construction and eprintln! behavior, and use
the matching key already referenced elsewhere in this file.
- Around line 5286-5301: Update the account persistence block that constructs
ApiAccountConfig to always assign the verified base value to api_base, rather
than converting it to None when it matches self.config.api_base. Keep the
persisted endpoint pinned to the account’s verified provider so later changes
through switch_endpoint_model cannot redirect the account.
- Around line 5434-5445: Update the api_accounts iteration that builds
SearchSelectItem entries to skip accounts where a.disabled is true, matching the
endpoint scan’s filtering behavior. Ensure disabled accounts cannot contribute
models to the selection list or trigger switch_endpoint_model with the primary
key fallback.
- Around line 5564-5570: Replace the manual ConfigLoader::save call and
rebuild_rotation sequence in the recent-model update flow with the existing
persist_config_and_rebuild_rotation helper. Ensure the helper’s failure warning
and rotation rebuild are used while preserving the recent_models update and
switch-success behavior.
In `@crates/aish-skills/src/registry/mod.rs`:
- Around line 284-318: Update the per-slug lock acquisition in
install_transactional to recover from a poisoned mutex instead of calling unwrap
and panicking. Preserve the existing install transaction and guard lifetime,
allowing subsequent installs for the same skill slug to proceed and return
normal errors.
In `@crates/aish-tools/src/skill_registry/skill_registry.rs`:
- Around line 205-215: The /forget-approvals reset path must clear both
approval_memory and the session-scoped auto-vet decision. Update the installed
shell command to use SkillRegistry::auto_vet_handle() and reset its AtomicBool
alongside approval memory, then add an integration test verifying both decisions
are cleared together.
In `@crates/aish-ui/src/select.rs`:
- Around line 468-486: Remove bare-letter action handling from the query input
path in the select component, including the action lookup around
filtered_entries and PanelEvent::Submit; action keys must not intercept normal
search characters. Require modified/non-letter shortcuts or an explicit action
mode instead, and update the picker footer text plus related localizations to
document the new interaction.
- Around line 169-173: Normalize the key in Select's with_action method before
storing it in self.actions, so configured uppercase keys match the lowercased
characters processed by handle_event. Preserve the existing label and
builder-return behavior.
- Around line 473-484: Update the action handling in the query-empty branch to
return PanelEvent::Continue when no item is highlighted or its value cannot be
resolved, instead of submitting Action with an empty string. Preserve
PanelEvent::Submit(SearchSelectOutcome::Action(...)) only when
filtered_entries().get(self.selected) resolves to a SelectEntry::Item with a
valid item value.
---
Nitpick comments:
In `@crates/aish-config/src/model.rs`:
- Around line 594-596: Resolve the unused weight field in the configuration
model: either remove weight and its default from the relevant model and config
surface, or mark it explicitly as reserved in the user-facing configuration
documentation until weighted selection is implemented. Ensure
RotationState::pick_available_account does not imply weight is currently
honored.
- Around line 513-516: Replace the String-based recent_models representation in
ConfigModel with a serializable RecentModel struct containing model and api_base
fields. Update the app.rs model-switch storage path to construct RecentModel
values instead of concatenating with a NUL delimiter, and update consumers to
access the fields directly rather than using split_once('\u{0}').
In `@crates/aish-llm/src/session.rs`:
- Around line 522-542: Update the Err handling in the session request flow to
acquire the rotation lock once and retain that guard across advance_on_error,
model_exhaustion_error, and current. Use the single guard for all failure-path
operations so credential/model advancement and selection are atomic, while
preserving the existing return behavior.
In `@crates/aish-shell/src/app.rs`:
- Around line 5119-5164: Move the existing duplicate-name check using
self.config.api_accounts and name to immediately after the name prompt succeeds,
before prompt_edit_value requests the API key or base endpoint. Preserve the
current already_exists message and early return, and remove the later redundant
check.
- Around line 5481-5505: Extract the duplicated endpoint-building and
model-fetching logic from the entry flow and the
`PanelOutcome::Submitted(SearchSelectOutcome::Action('a', _))` branch into
shared `collect_endpoints` and `fetch_all_models` methods. Replace both inline
blocks with calls to these methods, preserving endpoint deduplication,
disabled-account filtering, API key handling, fetch timeout, and fetched-model
output behavior.
- Around line 5387-5394: The endpoint model fetches in the loop over endpoints
run serially without user feedback, delaying panel rendering. Update the fetch
flow around fetch_models_from_api to run endpoint requests concurrently where
supported, and emit a “fetching models…” status before waiting; preserve
aggregation of each model with its originating base.
- Around line 342-366: Extract the duplicated rotation construction into
rotation_state_from_config(config: &ConfigModel) -> Option<RotationState), using
RetryPolicy struct-update syntax and retaining the is_active gate. In
crates/aish-shell/src/app.rs lines 342-366, replace the inline construction with
this helper and set the session rotation when it returns Some. In
crates/aish-shell/src/app.rs lines 5334-5358, call the same helper, re-select
prev with use_account as the rebuild-specific step, then pass the result to
apply_rotation_state.
In `@crates/aish-skills/src/manager.rs`:
- Around line 302-335: Update parse_skill_metadata to use a lazily initialized
static LazyLock<regex::Regex> for FRONTMATTER_REGEX, compiling the pattern only
once and reusing it for all calls. Preserve the existing invalid-regex error
behavior as appropriate for static initialization, while leaving frontmatter
extraction and validation unchanged.
In `@crates/aish-skills/src/registry/mod.rs`:
- Around line 319-336: Update the success and failure cleanup paths in the
surrounding reinstall operation to log cleanup errors with tracing::warn!
instead of discarding the Results. Cover stash removal, live-directory removal,
and restoration rename failures, including the relevant paths and error details
while preserving the existing return and rollback behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a97116ed-e63e-422a-8574-c043198eba07
📒 Files selected for processing (19)
crates/aish-config/src/lib.rscrates/aish-config/src/model.rscrates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-llm/src/lib.rscrates/aish-llm/src/rotation.rscrates/aish-llm/src/session.rscrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-shell/src/resume_selector.rscrates/aish-skills/src/manager.rscrates/aish-skills/src/registry/mod.rscrates/aish-skills/src/registry/verifier.rscrates/aish-tools/src/skill_registry/skill_registry.rscrates/aish-ui/src/select.rs
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)
crates/aish-i18n/locales/de-DE.yaml (1)
335-335: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the malformed manage subtitle.
The current text starts with a stray
dand is grammatically incomplete, so the manage panel renders broken German text.Suggested fix
- manage_subtitle: "d markierten Eintrag loeschen" + manage_subtitle: "d: Markierten Eintrag löschen"🤖 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/aish-i18n/locales/de-DE.yaml` at line 335, Update the manage_subtitle translation in de-DE.yaml to remove the stray leading “d” and provide grammatically complete German text describing deletion of the selected entry.
🧹 Nitpick comments (1)
crates/aish-i18n/src/manager.rs (1)
306-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover all newly introduced translation keys.
This test checks only nine keys, so missing entries such as
shell.model.fetch_failed_subtitle, deletion/edit/fallback labels, mostshell.accounts.*, and relocatedshell.common.*keys would still pass and render raw key names becauset()falls back to the key itself. Maintain one complete expected-key list for this cohort.🤖 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/aish-i18n/src/manager.rs` around lines 306 - 332, Expand the expected-key list in picker_keys_present_in_all_embedded_locales to include every newly introduced translation key in the model, accounts, and relocated common-key cohorts, including fetch-failure, deletion/edit/fallback, and remaining shell.accounts entries. Keep one centralized list and continue asserting each key resolves to a translated value for every EMBEDDED_LOCALES entry.
🤖 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/aish-i18n/locales/de-DE.yaml`:
- Line 310: Update the fetch_failed_subtitle translations to use neutral wording
for an unavailable or empty model list without claiming the endpoint is
unreachable: change crates/aish-i18n/locales/de-DE.yaml lines 310-310 to the
requested neutral German wording, and update crates/aish-i18n/locales/fr-FR.yaml
lines 310-310 with the equivalent neutral French wording.
---
Outside diff comments:
In `@crates/aish-i18n/locales/de-DE.yaml`:
- Line 335: Update the manage_subtitle translation in de-DE.yaml to remove the
stray leading “d” and provide grammatically complete German text describing
deletion of the selected entry.
---
Nitpick comments:
In `@crates/aish-i18n/src/manager.rs`:
- Around line 306-332: Expand the expected-key list in
picker_keys_present_in_all_embedded_locales to include every newly introduced
translation key in the model, accounts, and relocated common-key cohorts,
including fetch-failure, deletion/edit/fallback, and remaining shell.accounts
entries. Keep one centralized list and continue asserting each key resolves to a
translated value for every EMBEDDED_LOCALES entry.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: acd79cc5-85ac-4449-a6c4-e7283c1cea52
📒 Files selected for processing (6)
crates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-i18n/src/manager.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/aish-i18n/locales/zh-CN.yaml
- crates/aish-i18n/locales/ja-JP.yaml
- crates/aish-i18n/locales/es-ES.yaml
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/aish-i18n/locales/es-ES.yaml`:
- Line 317: Update the fetch_failed_subtitle translation in
crates/aish-i18n/locales/es-ES.yaml at lines 317-317 to use neutral
unavailable/empty-list wording instead of “endpoint inalcanzable”; make the
equivalent wording change in crates/aish-i18n/locales/zh-CN.yaml at lines
672-672, without implying a connectivity failure.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b209dc26-2250-49ad-a30a-36ea54747376
📒 Files selected for processing (11)
crates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-i18n/src/manager.rscrates/aish-llm/src/rotation.rscrates/aish-shell/src/app.rscrates/aish-skills/src/registry/mod.rscrates/aish-ui/src/select.rs
💤 Files with no reviewable changes (1)
- crates/aish-i18n/locales/en-US.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/aish-i18n/src/manager.rs
- crates/aish-i18n/locales/ja-JP.yaml
- crates/aish-ui/src/select.rs
- crates/aish-llm/src/rotation.rs
- crates/aish-skills/src/registry/mod.rs
- crates/aish-shell/src/app.rs
…ank search by relevance Skill registry hardening (aish-skills / aish-tools): - parse_skill_metadata is now the single source of truth for "would the loader accept this file"; the loader, installer, and verifier all route through it so install/verify-time validation cannot drift from load-time. Rejects context=fork/subagent skills that do not declare an agent. - Reinstalls are transactional: the existing skill is stashed (same-FS atomic rename) before the replacement is downloaded + validated, and restored on failure — a bad replacement can no longer overwrite/quarantine a previously trusted skill. Path-escaping slugs are rejected before the stash path is built. Search panel (aish-ui): - SearchSelect ranks results by relevance (exact > prefix > substring) and adds an opt-in single-key Action outcome (intercepted only on an empty query so typing never triggers an action). Refs AI-Shell-Team#414 (skill hardening + search-ranking slice; the remaining AI-Shell-Team#414 items — approval memory, rotation, /export & /sessions fixes — ship with their feature PRs or a follow-up).
Restore the /model multi-account + automatic rotation feature that was deferred when the closed 0.4.0 mega-PR (AI-Shell-Team#415) was split. Ported from the original combined implementation, excluding the orthogonal approval_memory tool-scoping (separate concern). - aish-llm: new rotation engine (RotationState/RetryPolicy/FailureKind) that advances to the next API account or fallback model on 429/usage-limit/5xx/ network errors, wired into the chat-completion request path; restores the primary after its cooldown window. - aish-config: api_accounts + ApiAccountConfig, fallback_models, recent_models, fallback_revert_on_cooldown. - aish-shell: /model opens a picker spanning every configured endpoint (fetches model lists, search by name/URL, recents float up); switch_endpoint demotes the outgoing primary into api_accounts and promotes the target; interactive add-account + manage panel (add/remove accounts & fallbacks); rebuild_rotation keeps the live session in sync with config. - i18n: shell.model picker/manage keys + shell.accounts + shell.common (x6). Excludes approval_memory (independent tool-scoped approval feature) and the session commands (/export, /sessions, /fork) which ship in their own PRs.
select.rs render_footer now composes the footer from the registered (key, i18n-label) actions plus any explicit footer hint, so the action_* labels (action_add/action_manage/action_del/action_fallback) are actually displayed instead of being dead values computed but never rendered. The model picker / manage footers drop their hardcoded action descriptions (now auto-rendered from with_action) and keep only the Esc hint, so the visible hints stay in sync with the registered actions.
The picker i18n keys (picker_title/subtitle/search/...) were inserted
after the first `verify_failed:` in the YAML. For non-English locales
that is `shell.failure_diagnose.verify_failed`, because the insertion
anchor matched the English value "model validation", which is absent
from the translated values — so the fallback picked the wrong section.
The keys landed under shell.failure_diagnose.* instead of shell.model.*,
and t("shell.model.picker_title") missed, making the /model panel render
raw key strings in zh-CN/de-DE/es-ES/fr-FR/ja-JP (en-US was correct).
Move the picker + accounts + common block to after the real
shell.model verify_failed in all 5 non-en-US locales, and add a
regression test asserting the picker/accounts/common keys resolve in
every embedded locale.
Security/data-integrity: - switch_endpoint_model: scope the promote-retain on (key, base) so a same-key/different-base switch no longer drops the just-preserved outgoing endpoint; key the preserve check the same way. - accounts_add: always persist the verified api_base (Some) so a later primary-endpoint switch can't redirect a credential to another host. - model_picker: skip disabled accounts in the model list (their endpoints are excluded, so selecting one fell back to the primary key) and de-duplicate endpoints by (base, key) so same-host/different-key accounts don't collapse. Correctness/quality: - persist_config_and_rebuild_rotation: reuse the existing shell.config_save_warning key; drop the redundant shell.common one. - switch_endpoint_model: route persistence through the shared persist helper (warns on save failure instead of swallowing it). - reserve the "primary" account name so a user account can't collide with the synthetic index-0 entry. - rotation: current_account_name()/snapshot() now fall back to the first enabled account when the selected one is disabled, matching current(). - skills registry: recover from a poisoned per-slug install lock instead of panicking (a single poison used to break that slug for the process). - select panel: normalize action keys on register; don't emit an Action event when no item is selected. - manage panel: confirm (y/N) before deleting a stored credential. - es-ES: restore the tilde in añadir/Añadido. i18n: the failure_diagnose tail (conclusion_fixed..no_evidence) was stranded under shell.common in the non-English locales by the earlier splice; move it back under shell.failure_diagnose and add a regression assertion.
jexShain
force-pushed
the
feat/model-rotation
branch
from
July 29, 2026 08:07
7cc894f to
a47d418
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aish-skills/src/registry/mod.rs (1)
319-336: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCleanup/restore failures are silently swallowed, weakening the "never overwrite the original" guarantee.
Both
remove_dir_all(&live)(line 329) and the restorerename(s, &live)(line 331) discard theirResultvialet _ =. Ifremove_dir_allfails or only partially removes the failed replacement,livecan remain a non-empty directory; POSIXrename(2)then fails when the destination directory exists and isn't empty, so the restore also silently fails — leaving the original skill orphaned under the hidden.{slug}.reinstall-*stash path and a broken directory atlive. The comment at lines 279-283 states this "can never" happen, but nothing surfaces or prevents it when the twofsops themselves fail.🩹 Proposed fix: surface failures instead of swallowing them
Err(e) => { // Failure: drop the failed replacement and restore the original. - let _ = std::fs::remove_dir_all(&live); - if let Some(s) = stash { - let _ = std::fs::rename(s, &live); - } + if let Err(remove_err) = std::fs::remove_dir_all(&live) { + tracing::warn!(error = %remove_err, dir = ?live, "failed to remove failed install replacement"); + } + if let Some(s) = stash { + if let Err(restore_err) = std::fs::rename(&s, &live) { + tracing::error!( + error = %restore_err, stash = ?s, dir = ?live, + "failed to restore original skill after failed reinstall; \ + original preserved at stash path, not at its live location" + ); + } + } Err(e) }🤖 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/aish-skills/src/registry/mod.rs` around lines 319 - 336, Update the reinstall transaction’s failure branch around remove_dir_all and the stash restore rename so neither filesystem Result is discarded. Surface cleanup and restore failures through the existing error path, and ensure the original error is augmented or replaced when restoration fails so callers can detect that the original was not restored. Preserve successful cleanup and restoration behavior.
🧹 Nitpick comments (1)
crates/aish-i18n/src/manager.rs (1)
307-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover all newly introduced translation keys.
The test checks only a small representative subset, so missing keys such as
fallback_*, account state/action/verification strings, orerrors.litellm_not_installedcan still reach users as raw key names. Build the assertion list from the complete new key contract, or compare each locale’s keys against the canonical locale.🤖 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/aish-i18n/src/manager.rs` around lines 307 - 336, The picker_keys_present_in_all_embedded_locales test does not cover the complete set of newly introduced translation keys. Expand its assertion list to include every new fallback, account state/action/verification, and errors.litellm_not_installed key, or compare each embedded locale against the canonical locale’s complete key set while preserving the missing-key failure diagnostics.
🤖 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.
Outside diff comments:
In `@crates/aish-skills/src/registry/mod.rs`:
- Around line 319-336: Update the reinstall transaction’s failure branch around
remove_dir_all and the stash restore rename so neither filesystem Result is
discarded. Surface cleanup and restore failures through the existing error path,
and ensure the original error is augmented or replaced when restoration fails so
callers can detect that the original was not restored. Preserve successful
cleanup and restoration behavior.
---
Nitpick comments:
In `@crates/aish-i18n/src/manager.rs`:
- Around line 307-336: The picker_keys_present_in_all_embedded_locales test does
not cover the complete set of newly introduced translation keys. Expand its
assertion list to include every new fallback, account state/action/verification,
and errors.litellm_not_installed key, or compare each embedded locale against
the canonical locale’s complete key set while preserving the missing-key failure
diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6738a439-9809-473d-a2e9-44a8b7caf881
📒 Files selected for processing (20)
crates/aish-config/src/lib.rscrates/aish-config/src/model.rscrates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-i18n/src/manager.rscrates/aish-llm/src/lib.rscrates/aish-llm/src/rotation.rscrates/aish-llm/src/session.rscrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-shell/src/resume_selector.rscrates/aish-skills/src/manager.rscrates/aish-skills/src/registry/mod.rscrates/aish-skills/src/registry/verifier.rscrates/aish-tools/src/skill_registry/skill_registry.rscrates/aish-ui/src/select.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- crates/aish-config/src/lib.rs
- crates/aish-shell/src/ai_handler.rs
- crates/aish-llm/src/lib.rs
- crates/aish-i18n/locales/de-DE.yaml
- crates/aish-i18n/locales/es-ES.yaml
- crates/aish-shell/src/resume_selector.rs
- crates/aish-i18n/locales/en-US.yaml
- crates/aish-skills/src/manager.rs
- crates/aish-i18n/locales/ja-JP.yaml
- crates/aish-skills/src/registry/verifier.rs
- crates/aish-llm/src/rotation.rs
- crates/aish-llm/src/session.rs
- crates/aish-config/src/model.rs
- crates/aish-ui/src/select.rs
- crates/aish-shell/src/app.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restores the
/modelmulti-account + automatic rotation + model picker feature that was deferred when the closed 0.4.0 mega-PR (#415) was split into focused PRs. The original, tested implementation lives incf3da26; this PR ports the rotation subset verbatim, excluding the orthogonalapproval_memorytool-scoping (a separate concern) and the session commands (/export,/sessions,/fork— each its own PR).What's included
aish-llm — rotation engine + request-path wiring
rotation.rs(707 lines):RotationState,RetryPolicy,FailureKind,ApiAccount,ResolvedCredential,RotationSnapshot. On a recoverable failure (429 / usage-limit / 5xx / network / model error) it advances to the next API account, then the fallback model chain, and restores the primary after its cooldown window.session.rs:chat_completionnow routes through the rotation loop (fast path when no rotation);set_rotation/clear_rotation/use_rotation_account/current_rotation_account/rotation_snapshotaccessors.aish-config — schema
api_accounts: Vec<ApiAccountConfig>(extra keys under the same provider),fallback_models,recent_models,fallback_revert_on_cooldown. Additive + serde-defaulted → existing configs unaffected.aish-shell —
/modelUX/model(no arg) opens a picker spanning every configured endpoint: fetches each endpoint's model list, shows the URL beside each model, search by name or URL, recently-used entries float to the top.switch_endpoint_model: switching endpoint demotes the outgoing primary intoapi_accounts(never silently lost) and promotes the target out of the pool (no duplicate).aadds an account interactively (name/key/base + connectivity & tool-support verification);mopens a manage panel (delete accounts, add/remove fallback models).rebuild_rotationkeeps the live session in sync with config changes.i18n —
shell.modelpicker/manage keys +shell.accounts+shell.commonacross all 6 locales.Stacking
Based on #420 (
feat/bugfixes) — the picker uses theAction/with_action/with_shimmerpanel features that #420 adds toaish-ui/select.rs. Rebase tomainafter #420 merges. Disjoint from #417–#419 (different files/regions), low conflict risk.Excluded (separate PRs / future)
approval_memorytool-scoped command approval — independent feature, not requested here./export), feat(session): /fork — branch a session, preserving the original #418 (/fork), feat(shell): /sessions — browse the session tree and switch #419 (/sessions).Verification
cargo fmt --check✅cargo clippy --all-targets -- -D warnings✅cargo test --workspace✅ (incl.rotation+account_helperstests)Summary by CodeRabbit
/accountsmanagement flow.