feat: AI-ops — quota rotation, session tree, service supervisor + interactive menus & i18n - #395
Closed
jexShain wants to merge 29 commits into
Closed
feat: AI-ops — quota rotation, session tree, service supervisor + interactive menus & i18n#395jexShain wants to merge 29 commits into
jexShain wants to merge 29 commits into
Conversation
Three AI-ops capabilities:
1. Multi-account quota rotation + model fallback (aish-llm, aish-config):
- rotation.rs: credential rotation + fallback chain state machine with
per-account cooldown, model suppression, cooldown-expiry revert
- LlmSession.chat_completion routes through rotation on recoverable
errors (429/usage-limit/5xx/network/model-error), advancing to the
next account or fallback model until recovery is exhausted
- config: api_accounts, fallback_models, fallback_revert_on_cooldown
- slash commands: /usage (status matrix), /accounts (add/remove/enable),
/fallback (chain + revert policy) with hot reload via rebuild_rotation
2. Session branch/tree (aish-session, aish-shell):
- backward-compatible schema migration: parent_session_uuid +
branch_point_message_id columns
- SessionStore::fork_session / list_children / session_roots
- slash commands: /fork (branch + switch), /sessions (tree view),
/export (markdown postmortem export)
3. Service supervisor tool (aish-tools):
- ServiceSupervisorTool (LLM tool): start/status/stop/logs/restart
- detached process (process_group) + readiness (log regex + port probe)
+ per-service JSON state files; zombie reaping via waitpid(WNOHANG)
Verified: workspace build, clippy -D warnings (0), unit tests (rotation/
session/service-supervisor), and 8/8 interactive smoke via pexpect.
No-arg invocation now opens a select menu (oh-my-pi style) instead of requiring memorized subcommands; power-user direct subcommands still work. - pick_menu helper over the inline selection panel (ChoicePanel in a real terminal, numbered stdin fallback under a non-tty harness) - /accounts: List / Add / Remove / Enable-Disable, with interactive name+key prompts and an account picker sub-menu - /fallback: List / Add / Remove / Clear / Toggle-revert, with a model prompt and a revert-policy sub-menu Verified: clippy -D warnings clean; pexpect smoke 3/3 (accounts list, accounts toggle sub-menu, fallback add+prompt).
- rotation: /model switch now rebuilds the rotation state (was silently ignored while rotation was active — the loop kept the model captured at setup); Network errors no longer burn through every account since they are environment-wide (fall through to model fallback only). - service_supervisor: signal the whole process group on stop/restart (kill -pgid) so grandchildren aren't orphaned; treat waitpid ECHILD as exited in pid_alive to stop pid-reuse from routing SIGTERM/SIGKILL onto an unrelated process; validate ready_log regex (invalid -> not ready, not silently ready); read only the log tail in the logs action instead of the whole unbounded log file. - session: make the schema migration concurrency-safe by ignoring the duplicate-column error a racing SessionStore::open would raise. Verified: cargo check (0 warnings), clippy -D warnings (0), and unit tests for aish-llm (291), aish-session (8), service_supervisor (9).
No-arg invocation of these commands now opens an interactive select menu, consistent with /accounts and /fallback. Power-user subcommands/flags are preserved verbatim; only the no-arg entry point changed. - /record: Start/Stop, dynamic on recording state - /plan: Start/Status/Exit, dynamic on plan phase - /kill_live_sessions: per-session picker + 'kill all others' - /doctor: Run diagnostics / Run with auto-fix - /audit: Recent / by user / by host / by event type (prompts for value) Verified: cargo check (0 warnings), clippy -D warnings (0), pexpect smoke 5/5 (record Start+dynamic Stop, plan/doctor menus, audit no-store safe).
…k /fork /sessions /export These commands now show localized descriptions in the slash command picker (shell.slash.<cmd>) across all 6 locales (en-US, zh-CN, de-DE, es-ES, fr-FR, ja-JP). Previously the picker fell back to the raw i18n key string.
pick_menu now takes owned (String) labels; all 11 menus (/accounts /fallback
/audit /doctor /record /plan /kill_live_sessions and their sub-menus) render
titles and option labels/descriptions via t("shell.menu.<cmd>.<key>"). Added
a shell.menu section across all 6 locales (53 keys each). Internal value ids
(list/add/remove/...) and dynamic data (account/model/session names) stay
verbatim.
…reen dump Replaces the println tree dump (one line per session, flooded the screen) with a searchable SearchSelectPanel listing roots + direct forks with depth indentation. Selecting a session switches to it; Esc cancels. Reuses the shell.resume selector title/search i18n keys.
The Normal-phase catch-all dispatched any subcommand except "exit" (including "status") to enter_plan_mode, so the menu's "Show status" option misled users into planning. Add an explicit Normal-phase "status" arm that reports the not-in-plan-mode state instead. Verified via pexpect smoke (zh-CN locale: menu 查看状态 -> status reported, no plan banner).
The Normal-phase /plan status output was English-only; now uses
t("shell.menu.plan.not_in_plan") with translations across all 6 locales.
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 change adds multi-account LLM rotation and model fallback, forkable session persistence, new shell commands and menus, localized UI text, and a service supervisor tool for managing background processes. ChangesRotation and session workflows
Background service supervisor
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Shell
participant LlmSession
participant RotationState
participant LLMProvider
Shell->>LlmSession: submit completion
LlmSession->>RotationState: resolve credential and model
LlmSession->>LLMProvider: stream completion
LLMProvider-->>LlmSession: success or classified failure
LlmSession->>RotationState: rotate account or fallback model
LlmSession-->>Shell: completion result
sequenceDiagram
participant Shell
participant ServiceSupervisorTool
participant ServiceState
participant BackgroundProcess
Shell->>ServiceSupervisorTool: start/status/stop/logs/restart
ServiceSupervisorTool->>ServiceState: load or save service state
ServiceSupervisorTool->>BackgroundProcess: spawn, probe, signal, or read logs
BackgroundProcess-->>ServiceSupervisorTool: readiness, status, or log output
ServiceSupervisorTool-->>Shell: structured tool result
Possibly related PRs
Suggested labels: 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: 5
🧹 Nitpick comments (1)
crates/aish-tools/src/service_supervisor/service_supervisor.rs (1)
319-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the readiness loop into
evaluate_ready.The loop reimplements the log-match + port-AND logic inline for the
log_re.is_some()branch, duplicating whatevaluate_ready(andlog_matches_ready) already express. The whole body can delegate toevaluate_ready, removing the branch and thelog_matches_readywrapper.♻️ Suggested simplification
loop { let log_content = if log_re.is_some() { read_log_tail(log_file, READY_LOG_TAIL_BYTES) } else { None }; let port_open = ready_port.map(port_is_open); - if let Some(re) = log_re.as_ref() { - let matched = log_content - .as_deref() - .map(|c| log_matches_ready(c, re)) - .unwrap_or(false); - let port_ok = port_open.unwrap_or(true); - if matched && port_ok { - return true; - } - } else { - // Only a port criterion (or none)... - if evaluate_ready(None, None, port_open) { - return true; - } - } + if evaluate_ready(log_content.as_deref(), log_re.as_ref(), port_open) { + return true; + } if Instant::now() >= deadline { return false; } std::thread::sleep(READY_POLL_INTERVAL); }🤖 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-tools/src/service_supervisor/service_supervisor.rs` around lines 319 - 347, Refactor the readiness polling loop to delegate all readiness decisions to evaluate_ready instead of branching on log_re.is_some(). Pass the available log content, log_re, and port_open directly to evaluate_ready, remove the inline log_matches_ready call and duplicated AND logic, and preserve the existing deadline and polling behavior.
🤖 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-session/src/store.rs`:
- Around line 240-291: Update delete_session to handle children of the deleted
session within its existing transaction: before removing the target row, promote
direct children by setting their parent_session_uuid to NULL, then continue
deleting the session and its history. Preserve the existing deletion behavior
while ensuring session_roots() can discover formerly attached children.
In `@crates/aish-shell/src/app.rs`:
- Around line 327-335: Update the extra-account construction in the api_accounts
loop to mark an account disabled whenever acct.api_key is blank, while
preserving an explicitly disabled acct.disabled value. Mirror the primary
account’s blank-key guard so keyless accounts cannot be active or selected for
rotation.
- Around line 3328-3333: Add the missing i18n entries for all shell.slash.* and
shell.menu.* keys used by the new command handlers, including /usage, /accounts,
/fallback, /fork, /sessions, and /export, across every tracked locale file.
Match the existing locale key structure and provide translated values so
commands and menus never display raw key strings.
In `@crates/aish-tools/src/service_supervisor/service_supervisor.rs`:
- Around line 379-381: Validate the value returned by get_u64 for “ready_port”
in the argument-handling flow before converting it to u16; reject values greater
than 65535 with an error instead of allowing the cast to truncate or wrap, while
preserving valid-port assignment to state.ready_port.
- Around line 434-439: Update restart_service to preserve whether the service
was previously configured before calling stop_service, since stop_service clears
started_at and pid. Pass or retain that pre-stop state for start_service so a
restart increments restart_count instead of resetting it, preserving the
documented /restart behavior.
---
Nitpick comments:
In `@crates/aish-tools/src/service_supervisor/service_supervisor.rs`:
- Around line 319-347: Refactor the readiness polling loop to delegate all
readiness decisions to evaluate_ready instead of branching on log_re.is_some().
Pass the available log content, log_re, and port_open directly to
evaluate_ready, remove the inline log_matches_ready call and duplicated AND
logic, and preserve the existing deadline and polling 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: e85c4759-cb06-4862-9649-d72df47d032a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 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-llm/src/lib.rscrates/aish-llm/src/rotation.rscrates/aish-llm/src/session.rscrates/aish-session/src/models.rscrates/aish-session/src/store.rscrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-shell/src/readline.rscrates/aish-tools/Cargo.tomlcrates/aish-tools/src/lib.rscrates/aish-tools/src/service_supervisor/prompt.rscrates/aish-tools/src/service_supervisor/service_supervisor.rs
Comment on lines
+240
to
+291
| /// Fork a new session from an existing one. | ||
| /// | ||
| /// The new session copies the parent's persisted `state` snapshot plus its | ||
| /// `model`/`api_base`/`run_user` metadata, and records `parent_uuid` plus | ||
| /// the optional `branch_point_message_id` (the history row at which the | ||
| /// branch diverges). The fork's `created_at` is the current time. | ||
| pub fn fork_session( | ||
| &self, | ||
| parent_uuid: &str, | ||
| branch_point_message_id: Option<i64>, | ||
| new_uuid: &str, | ||
| ) -> Result<SessionRecord> { | ||
| let parent = self | ||
| .get_session(parent_uuid)? | ||
| .ok_or_else(|| { | ||
| AishError::Session(format!("parent session not found: {parent_uuid}")) | ||
| })?; | ||
|
|
||
| let now = Utc::now(); | ||
| let now_str = now.to_rfc3339(); | ||
| let state_str = serde_json::to_string(&parent.state)?; | ||
|
|
||
| self.conn | ||
| .execute( | ||
| "INSERT INTO sessions | ||
| (session_uuid, created_at, model, api_base, run_user, state, | ||
| parent_session_uuid, branch_point_message_id) | ||
| VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)", | ||
| params![ | ||
| new_uuid, | ||
| now_str, | ||
| parent.model, | ||
| parent.api_base, | ||
| parent.run_user, | ||
| state_str, | ||
| parent_uuid, | ||
| branch_point_message_id, | ||
| ], | ||
| ) | ||
| .map_err(|e| AishError::Session(format!("failed to fork session: {e}")))?; | ||
|
|
||
| Ok(SessionRecord { | ||
| session_uuid: new_uuid.to_string(), | ||
| created_at: now, | ||
| model: parent.model, | ||
| api_base: parent.api_base, | ||
| run_user: parent.run_user, | ||
| state: parent.state, | ||
| parent_session_uuid: Some(parent_uuid.to_string()), | ||
| branch_point_message_id, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
fork_session introduces parent/child linkage that delete_session doesn't account for.
Once a session is forked, delete_session (unchanged, elsewhere in this file) removes only the target row and its history — it never updates children's parent_session_uuid. Deleting a forked-from parent leaves its children pointing at a nonexistent parent_session_uuid; since session_roots() filters parent_session_uuid IS NULL, those orphaned children stop appearing in /sessions (which starts from session_roots()) and become effectively unreachable, even though their rows (and conversation state) still live in the DB.
Consider either cascade-deleting descendants or promoting direct children to roots (parent_session_uuid = NULL) inside delete_session's existing transaction.
♻️ Example fix: promote children to roots on delete
pub fn delete_session(&self, uuid: &str) -> Result<()> {
let tx = self
.conn
.unchecked_transaction()
.map_err(|e| AishError::Session(format!("failed to start delete transaction: {e}")))?;
+ tx.execute(
+ "UPDATE sessions SET parent_session_uuid = NULL, branch_point_message_id = NULL
+ WHERE parent_session_uuid = ?1",
+ params![uuid],
+ )
+ .map_err(|e| AishError::Session(format!("failed to reparent children: {e}")))?;
+
tx.execute("DELETE FROM history WHERE session_uuid = ?1", params![uuid])🤖 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-session/src/store.rs` around lines 240 - 291, Update
delete_session to handle children of the deleted session within its existing
transaction: before removing the target row, promote direct children by setting
their parent_session_uuid to NULL, then continue deleting the session and its
history. Preserve the existing deletion behavior while ensuring session_roots()
can discover formerly attached children.
Comment on lines
+327
to
+335
| for acct in &config.api_accounts { | ||
| accounts.push(aish_llm::ApiAccount { | ||
| name: acct.name.clone(), | ||
| api_key: acct.api_key.clone(), | ||
| api_base: acct.api_base.clone(), | ||
| weight: acct.weight.max(1), | ||
| disabled: acct.disabled, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Extra accounts with an empty api_key aren't disabled.
The primary account is disabled when config.api_key is blank (line 325), but configured api_accounts are pushed with disabled: acct.disabled regardless of whether their key is empty. An enabled-but-keyless extra account still counts toward is_active() and burns a rotation attempt that is guaranteed to fail. Mirror the primary's guard.
🛡️ Proposed guard
for acct in &config.api_accounts {
accounts.push(aish_llm::ApiAccount {
name: acct.name.clone(),
api_key: acct.api_key.clone(),
api_base: acct.api_base.clone(),
weight: acct.weight.max(1),
- disabled: acct.disabled,
+ disabled: acct.disabled || acct.api_key.trim().is_empty(),
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for acct in &config.api_accounts { | |
| accounts.push(aish_llm::ApiAccount { | |
| name: acct.name.clone(), | |
| api_key: acct.api_key.clone(), | |
| api_base: acct.api_base.clone(), | |
| weight: acct.weight.max(1), | |
| disabled: acct.disabled, | |
| }); | |
| } | |
| for acct in &config.api_accounts { | |
| accounts.push(aish_llm::ApiAccount { | |
| name: acct.name.clone(), | |
| api_key: acct.api_key.clone(), | |
| api_base: acct.api_base.clone(), | |
| weight: acct.weight.max(1), | |
| disabled: acct.disabled || acct.api_key.trim().is_empty(), | |
| }); | |
| } |
🤖 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 327 - 335, Update the
extra-account construction in the api_accounts loop to mark an account disabled
whenever acct.api_key is blank, while preserving an explicitly disabled
acct.disabled value. Mirror the primary account’s blank-key guard so keyless
accounts cannot be active or selected for rotation.
Comment on lines
+3328
to
+3333
| Some("/usage") => self.handle_usage_command(), | ||
| Some("/accounts") => self.handle_accounts_command(&parts), | ||
| Some("/fallback") => self.handle_fallback_command(&parts), | ||
| Some("/fork") => self.handle_fork_command(), | ||
| Some("/sessions") => self.handle_sessions_command(), | ||
| Some("/export") => self.handle_export_command(&parts), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify slash + menu i18n keys exist in every locale file.
keys=(
shell.slash.usage shell.slash.accounts shell.slash.fallback
shell.slash.fork shell.slash.sessions shell.slash.export
shell.menu.fallback.title shell.menu.fallback.list shell.menu.fallback.add
shell.menu.fallback.remove shell.menu.fallback.clear shell.menu.fallback.revert
shell.menu.fallback.remove_title shell.menu.fallback.revert_title
shell.menu.fallback.on shell.menu.fallback.off
shell.menu.accounts.title shell.menu.accounts.list shell.menu.accounts.add
shell.menu.accounts.remove shell.menu.accounts.toggle
shell.menu.accounts.remove_title shell.menu.accounts.toggle_title
shell.menu.plan.not_in_plan shell.menu.plan.title
shell.menu.doctor.title shell.menu.record.title
shell.menu.kill.title shell.menu.kill.all shell.menu.audit.title
)
fd -e yaml . crates/aish-i18n/locales | while read -r f; do
echo "== $f =="
for k in "${keys[@]}"; do
rg -qF "$k" "$f" || echo "MISSING: $k"
done
doneRepository: AI-Shell-Team/aish
Length of output: 6486
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked locale files:"
git ls-files crates/aish-i18n/locales/*.yaml | sort
echo
echo "Locomotive i18n key usage in crates/aish-shell/src/app.rs around new commands:"
rg -n "shell\.slash|shell\.menu|run_slash_input_session|handle_(fallback|accounts|fork|sessions|export|usage)_command" crates/aish-i18n/locales crates/aish-shell/src/app.rs | head -n 200
echo
echo "Check whether tracked keys exist anywhere under crates/aish-i18n/locales:"
for k in shell.slash.usage shell.slash.accounts shell.slash.fallback shell.slash.fork shell.slash.sessions shell.slash.export shell.menu.fallback.title shell.menu.fallback.list shell.menu.fallback.add shell.menu.fallback.remove shell.menu.fallback.clear shell.menu.fallback.revert shell.menu.fallback.remove_title shell.menu.fallback.revert_title shell.menu.fallback.on shell.menu.fallback.off shell.menu.accounts.title shell.menu.accounts.list shell.menu.accounts.add shell.menu.accounts.remove shell.menu.accounts.toggle shell.menu.accounts.remove_title shell.menu.accounts.toggle_title shell.menu.plan.not_in_plan shell.menu.plan.title shell.menu.doctor.title shell.menu.record.title shell.menu.kill.title shell.menu.kill.all shell.menu.audit.title; do
if rg -qF "$k" crates/aish-i18n/locales; then
echo "FOUND: $k"
else
echo "MISSING: $k"
fi
doneRepository: AI-Shell-Team/aish
Length of output: 7833
Add i18n entries for the new slash commands and menus in every locale.
Many shell.slash.* and shell.menu.* keys referenced by the new commands are absent from all tracked locale files, so command descriptions and menus render the raw key strings for every 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-shell/src/app.rs` around lines 3328 - 3333, Add the missing i18n
entries for all shell.slash.* and shell.menu.* keys used by the new command
handlers, including /usage, /accounts, /fallback, /fork, /sessions, and /export,
across every tracked locale file. Match the existing locale key structure and
provide translated values so commands and menus never display raw key strings.
Comment on lines
+379
to
+381
| if let Some(rp) = get_u64(args, "ready_port") { | ||
| state.ready_port = Some(rp as u16); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ready_port is silently truncated to u16.
get_u64 returns the raw JSON integer and rp as u16 wraps values above 65535 (e.g. 65536 → port 0) instead of rejecting them. The schema advertises maximum: 65535 but it isn't enforced at runtime, so a malformed argument yields a wrong/invalid port with no error.
🛡️ Proposed validation
- if let Some(rp) = get_u64(args, "ready_port") {
- state.ready_port = Some(rp as u16);
- }
+ if let Some(rp) = get_u64(args, "ready_port") {
+ match u16::try_from(rp) {
+ Ok(port) => state.ready_port = Some(port),
+ Err(_) => {
+ return ToolResult::error(format!(
+ "`ready_port` must be between 0 and 65535, got {rp}"
+ ))
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Some(rp) = get_u64(args, "ready_port") { | |
| state.ready_port = Some(rp as u16); | |
| } | |
| if let Some(rp) = get_u64(args, "ready_port") { | |
| match u16::try_from(rp) { | |
| Ok(port) => state.ready_port = Some(port), | |
| Err(_) => { | |
| return ToolResult::error(format!( | |
| "`ready_port` must be between 0 and 65535, got {rp}" | |
| )) | |
| } | |
| } | |
| } |
🤖 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-tools/src/service_supervisor/service_supervisor.rs` around lines
379 - 381, Validate the value returned by get_u64 for “ready_port” in the
argument-handling flow before converting it to u16; reject values greater than
65535 with an error instead of allowing the cast to truncate or wrap, while
preserving valid-port assignment to state.ready_port.
Comment on lines
+434
to
+439
| let is_restart = state.started_at != 0 || state.pid.is_some(); | ||
| if is_restart { | ||
| state.restart_count = state.restart_count.saturating_add(1); | ||
| } else { | ||
| state.restart_count = 0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate relevant files"
fd -a 'service_supervisor\.rs|prompt\.rs' . | sed 's#^\./##'
echo
echo "Find start/service definitions"
rg -n "fn (start_service|stop_service|restart_service)|restart_count|started_at|pid|prompt|restart: stop then start" crates -SRepository: AI-Shell-Team/aish
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "service supervisor relevant implementation"
wc -l crates/aish-tools/src/service_supervisor/service_supervisor.rs
sed -n '1,80p' crates/aish-tools/src/service_supervisor/service_supervisor.rs
sed -n '380,465p' crates/aish-tools/src/service_supervisor/service_supervisor.rs
sed -n '540,575p' crates/aish-tools/src/service_supervisor/service_supervisor.rs
echo
echo "service supervisor prompt docs"
sed -n '1,80p' crates/aish-tools/src/service_supervisor/prompt.rs
echo
echo "focused state usages"
rg -n "restart_count|started_at|pid\.is_some|stop_service|restart_service|start_service|load_state|ServiceState::new" crates/aish-tools/src/service_supervisor/service_supervisor.rs crates/aish-tools/src/service_supervisor/prompt.rsRepository: AI-Shell-Team/aish
Length of output: 15492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
state = {
"name": "smoke",
"command": "sh",
"args": [],
"cwd": None,
"pid": 123,
"started_at": 1700000000,
"ready": False,
"restart_count": 3,
"restart_policy": "no",
"ready_log": None,
"ready_port": None,
}
def stop(s):
s["pid"] = None
s["started_at"] = 0
return s
def start_after_restart(s):
is_restart = s["started_at"] != 0 or s.get("pid") is not None
if is_restart:
s["restart_count"] = min(s["restart_count"] + 1, 4294967295)
else:
s["restart_count"] = 0
s["pid"] = 789
s["started_at"] = 1700000010
return s
step1 = stop(state.copy())
result = start_after_restart(step1)
print("after stop:", json.dumps(step1, sort_keys=True, default=str))
print("after start:", json.dumps(result, sort_keys=True, default=str))
print("restart_count incremented?", result["restart_count"] == 4)
PYRepository: AI-Shell-Team/aish
Length of output: 608
Track restarts independently of fields cleared by stop.
restart_service calls stop_service, which sets started_at = 0 and pid = None; the following start_service therefore treats the operation as a fresh start and resets restart_count to 0. Detect prior configuration before stopping that state (for example, keep a copy of the loaded service file before stopping) so /restart matches its documented “bumps restart_count” behavior.
🤖 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-tools/src/service_supervisor/service_supervisor.rs` around lines
434 - 439, Update restart_service to preserve whether the service was previously
configured before calling stop_service, since stop_service clears started_at and
pid. Pass or retain that pre-stop state for start_service so a restart
increments restart_count instead of resetting it, preserving the documented
/restart behavior.
~105 user-visible strings across /usage /accounts /fallback /fork /sessions /export (interactive prompts, confirmations, status lines, /export markdown) now use t()/t_with_args(). 96 new i18n keys added across all 6 locales (accounts 29, fallback 26, export 15, usage 12, common/fork/sessions/audit 14). ANSI color codes and theme:: wrappers preserved; variables passed via t_with_args. LLM-facing service_supervisor output intentionally kept English. Verified: cargo check + clippy -D warnings (0); aish-i18n tests (14) with all 96 keys present in every locale; pexpect smoke shows /usage output in zh-CN (was English before).
accounts_add_interactive now: - prefills api_base from the primary endpoint (user may override) - fetches the endpoint's model list (GET /models) and lets the user pick one to verify against (falls back to the primary model on fetch failure/empty) - runs check_connectivity + check_tool_support (the same probes /setup uses) and reports latency + tool-calling support - on verification failure, asks whether to save anyway or cancel 11 new i18n keys (select_model/no_models/fetch_failed/verifying/connected/ tool_yes/tool_no/verify_failed/save_anyway/cancel_add) across all 6 locales. api_base is stored only when it differs from the primary.
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-shell/src/app.rs`:
- Around line 4205-4289: Add locale YAML entries for every shell.accounts key
referenced by the wizard-add flow, including select_model, no_models,
fetch_failed, verifying, connected, tool_yes, tool_no, verify_failed,
verify_failed_title, save_anyway, and cancel_add, plus shell.common.cancelled.
Update every supported locale consistently, providing translated text or the
project’s accepted fallback convention so these keys never render raw.
🪄 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: a186fede-4168-4a3a-b94d-80fb29336fe8
📒 Files selected for processing (7)
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-shell/src/app.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/aish-i18n/locales/en-US.yaml
- crates/aish-i18n/locales/fr-FR.yaml
- crates/aish-i18n/locales/de-DE.yaml
- crates/aish-i18n/locales/es-ES.yaml
- crates/aish-i18n/locales/ja-JP.yaml
- crates/aish-i18n/locales/zh-CN.yaml
Comment on lines
+4205
to
+4289
| let model = match fetch_models_from_api(&base, &key, 10) { | ||
| Ok(models) if !models.is_empty() => { | ||
| let opts: Vec<(String, String, String)> = models | ||
| .iter() | ||
| .map(|m| (m.clone(), m.clone(), String::new())) | ||
| .collect(); | ||
| match pick_menu(&t("shell.accounts.select_model"), &opts) { | ||
| Some(m) => Some(m), | ||
| None => return, | ||
| } | ||
| } | ||
| Ok(_) => { | ||
| println!( | ||
| "{}", | ||
| t_with_args("shell.accounts.no_models", &{ | ||
| let mut args = std::collections::HashMap::new(); | ||
| args.insert("model".to_string(), self.config.model.clone()); | ||
| args | ||
| }) | ||
| ); | ||
| Some(self.config.model.clone()) | ||
| } | ||
| Err(e) => { | ||
| println!( | ||
| "{}", | ||
| t_with_args("shell.accounts.fetch_failed", &{ | ||
| let mut args = std::collections::HashMap::new(); | ||
| args.insert("error".to_string(), e); | ||
| args | ||
| }) | ||
| ); | ||
| Some(self.config.model.clone()) | ||
| } | ||
| }; | ||
|
|
||
| // Verify connectivity + tool support (same probes as /setup). | ||
| if let Some(model) = model { | ||
| println!("{}", t("shell.accounts.verifying")); | ||
| let conn = check_connectivity(&base, &key, &model, 15); | ||
| if conn.ok { | ||
| println!( | ||
| "\x1b[32m{}\x1b[0m", | ||
| t_with_args("shell.accounts.connected", &{ | ||
| let mut args = std::collections::HashMap::new(); | ||
| args.insert( | ||
| "latency".to_string(), | ||
| conn.latency_ms.unwrap_or(0).to_string(), | ||
| ); | ||
| args.insert("model".to_string(), model.clone()); | ||
| args | ||
| }) | ||
| ); | ||
| let tools = check_tool_support(&base, &key, &model, 30); | ||
| let state = t(if tools.supports { | ||
| "shell.accounts.tool_yes" | ||
| } else { | ||
| "shell.accounts.tool_no" | ||
| }); | ||
| println!(" {}", state); | ||
| } else { | ||
| let err = conn.error.unwrap_or_default(); | ||
| eprintln!( | ||
| "{}", | ||
| t_with_args("shell.accounts.verify_failed", &{ | ||
| let mut args = std::collections::HashMap::new(); | ||
| args.insert("error".to_string(), err); | ||
| args | ||
| }) | ||
| ); | ||
| let save = pick_menu( | ||
| &t("shell.accounts.verify_failed_title"), | ||
| &[ | ||
| ("save".to_string(), t("shell.accounts.save_anyway"), String::new()), | ||
| ("cancel".to_string(), t("shell.accounts.cancel_add"), String::new()), | ||
| ], | ||
| ); | ||
| match save.as_deref() { | ||
| Some("save") => {} | ||
| _ => { | ||
| println!("{}", t("shell.common.cancelled")); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify each shell.accounts.* key referenced by accounts_add_interactive exists in every locale.
keys=(
shell.accounts.name_label shell.accounts.name_desc
shell.accounts.key_label shell.accounts.key_desc
shell.accounts.base_label shell.accounts.base_desc
shell.accounts.select_model shell.accounts.no_models
shell.accounts.fetch_failed shell.accounts.verifying
shell.accounts.connected shell.accounts.tool_yes shell.accounts.tool_no
shell.accounts.verify_failed shell.accounts.verify_failed_title
shell.accounts.save_anyway shell.accounts.cancel_add
shell.common.cancelled
)
fd -e yaml . crates/aish-i18n/locales | while read -r f; do
echo "== $f =="
for k in "${keys[@]}"; do
rg -qF "$k" "$f" || echo "MISSING: $k"
done
doneRepository: AI-Shell-Team/aish
Length of output: 4194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate locale files =="
fd -a -e yaml . crates/aish-i18n/locales 2>/dev/null | sed 's#^\./##' | sort
echo "== locale files size/contents sample =="
wc -l crates/aish-i18n/locales/*.yaml
sed -n '1,220p' crates/aish-i18n/locales/en-US.yaml
sed -n '1,220p' crates/aish-i18n/locales/de-DE.yaml
echo "== exact key references across repo =="
rg -n "shell\.accounts\.(name_label|name_desc|key_label|key_desc|base_label|base_desc|select_model|no_models|fetch_failed|verifying|connected|tool_yes|tool_no|verify_failed|verify_failed_title|save_anyway|cancel_add)|shell\.common\.cancelled" . || trueRepository: AI-Shell-Team/aish
Length of output: 24989
Add the missing shell.accounts.* and shell.common.cancelled locale entries.
The new wizard-add flow uses these keys, but every locale YAML is missing them (the initial check shows all references reported as “MISSING”), so UI strings in this path would render as raw keys until translations are added.
🤖 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 4205 - 4289, Add locale YAML
entries for every shell.accounts key referenced by the wizard-add flow,
including select_model, no_models, fetch_failed, verifying, connected, tool_yes,
tool_no, verify_failed, verify_failed_title, save_anyway, and cancel_add, plus
shell.common.cancelled. Update every supported locale consistently, providing
translated text or the project’s accepted fallback convention so these keys
never render raw.
When GET /models fails (common for proxy gateways that 404 unknown paths), the previous flow silently fell back to the primary model and then failed connectivity because the endpoint didn't recognize that model name — a dead-end 404 loop. Now mirrors /setup's select_model: - fetch success: model list + a 'Custom' option - fetch empty/fail or 'Custom': prompt for a model name (prefilled with the primary model so the user can confirm or edit), then run connectivity + tool-support probes against the chosen model. 3 new i18n keys (custom_model/model_label/model_desc) x 6 locales.
When every configured model is rejected by the provider as invalid, the final error previously named only the last fallback attempted, hiding the primary model the user actually configured (e.g. user sets model=gl, sees only "model not found: glm-4.7"). Now list the primary + fallback names and point at the config so the misspelling is obvious.
Each API account can now carry its own model so multi-provider setups work without forcing one global model name. /accounts use <name> (and the interactive menu) switches the active account at runtime; the selection survives config rebuilds (add/remove/toggle). /accounts add now persists the chosen model and /accounts list shows it. Also fixes the stale slash-commands count assertion (17 -> 23).
/model with no argument now opens a searchable panel (SearchSelectPanel) listing every available model configuration — the primary model plus each API account with its own model. Selecting an entry switches the active account/model at once, unifying switch-model and switch-account into one surface (cf. oh-my-pi ModelPicker). /model <name> still switches the primary model directly.
…config /model now opens a single searchable panel that replaces /accounts and /fallback: Enter switches model/account, 'a' adds an account, 'd' deletes the highlighted one. The panel re-opens after each action so the user keeps managing without re-typing. /accounts and /fallback are redirected to the same panel; all parameter-style config (/model <name>, /accounts add ...) is removed — configuration is panel-only now (cf. oh-my-pi ModelHub). SearchSelectPanel gains Action(char, value) for single-key operations.
The /model panel now also manages the fallback model chain: fallback models appear in the list (badged), 'f' adds one via prompt, 'd' removes the highlighted fallback or account. Selecting a fallback with Enter explains it is not directly switchable. Completes the merge of /accounts and /fallback into the single panel surface.
The unified /model panel now badges the primary entry and any disabled account, so the three entry kinds (primary, extra accounts, fallback models) are visually distinguishable at a glance.
The /model panel gains 't' (enable/disable an account, restoring the capability lost when /accounts was merged) and 'e' (edit an account's model name in-place instead of remove+re-add). Footer now lists all five actions: a/d/f/t/e.
Address real-terminal feedback: (1) action keys now match case-insensitively (E/e, A/a ...) so Shift no longer silently drops the keystroke into search; (2) edit ('e') works on the primary model and fallback entries too, not just accounts; (3) a subtitle makes the primary action (Enter to switch) obvious the moment the panel opens.
Footer now lists only management actions (a add, d del, e edit, t toggle, f fallback, Esc); the switch hint (Up/Dn select, Enter switch) lives in the subtitle, avoiding the duplication that made the panel feel cluttered.
/model now lists every model the current endpoint offers (fetched live from /models) plus models you have used before (recent_models, persisted), with the active one highlighted. Enter switches the primary model in place — same endpoint/key, new model name — and records it so switching back is one keystroke. 'a' adds a multi-key account. Fixes the core pain point: one URL serving many models with no way to browse/switch/remember them.
…els+URLs /model now fetches models from every configured endpoint (primary + accounts), each entry shows its URL, the same model on two URLs stays distinct, and search matches model names or URLs. Selecting switches endpoint+key+model in one step; recent_models stores model+base pairs so history recovers the exact endpoint. Search placeholder updated.
Both commands opened the same model switcher as /model, which was confusing since their names imply account/fallback-chain management. /model is now the single configuration surface; the 'a' key inside it adds an account.
Endpoints + model lists are fetched once when the panel opens; switching models reuses the cache instead of re-fetching every endpoint on every Enter. Only adding an account (which may add a new endpoint) re-fetches.
When the model list can't be fetched (all endpoints unreachable), the subtitle now explains why only current + history are shown, instead of silently showing a near-empty list.
/model now has an 'm' action that opens a sub-panel listing all accounts and fallback entries: 'd' deletes the highlighted one, 'f' adds a fallback model, Esc returns to the model switcher. Restores the management entry point lost when /accounts and /fallback commands were removed.
The model list was built only from the live /models fetch, so a newly added account whose endpoint was slow/unreachable (or whose model name isn't in the /models response) vanished from the list and search. Now each account's configured model is also injected into the list directly, so it is always reachable.
SearchSelectPanel now sorts filtered entries by match score: exact match beats prefix beats substring, so searching glm-5 surfaces glm-5 before glm-5.2/glm-5-turbo. Ties keep original order. Enter still picks the highlighted (top) entry.
This was referenced Jul 28, 2026
17 tasks
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.
Three AI-ops capabilities plus UX/i18n polish, built on a clean
mainworktree. Two rounds of code review; all findings fixed.Features
aish-llm/rotation.rs): two-layer recovery on top of per-request HTTP retry — rotate API keys on rate/usage limits, fall back to configured models; per-account cooldown + cooldown-expiry revert. Commands:/usage,/accounts,/fallback.aish-session): backward-compatible schema migration (parent_session_uuid+branch_point_message_id, duplicate-column-safe);fork_session/list_children/session_roots. Commands:/fork,/sessions,/export [md].aish-tools/ServiceSupervisorTool):start/status/stop/logs/restarta detached long-running service with readiness (log regex + port probe), per-service JSON state, process-group signalling, zombie reaping.UX
/accounts /fallback /record /plan /kill_live_sessions /doctor /audit; power-user subcommands preserved./sessionsopens a searchable panel instead of screen-dumping the tree.Verification
cargo build --workspace,cargo clippy --workspace --all-targets -- -D warnings(0).Review
Two reviewer passes (9 findings total) — all fixed: /model switch under rotation, Network-error rotation burn, log-file OOM, process-group orphans, pid-reuse, regex validation, schema migration race, /plan status misleading entry. See individual commits.
Summary by CodeRabbit
/usage,/accounts,/fallback,/fork,/sessions, and/export, with interactive menus and extended help./audit,/doctor,/record,/plan, and enhanced/killtargeting; model changes now refresh rotation immediately.