feat(shell): /export, /sessions, /fork + session/approval/rotation hardening - #415
Closed
jexShain wants to merge 1 commit into
Closed
feat(shell): /export, /sessions, /fork + session/approval/rotation hardening#415jexShain wants to merge 1 commit into
jexShain wants to merge 1 commit into
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:
📝 WalkthroughWalkthroughThis PR adds account rotation and model fallback, session forking and Markdown export, tool-scoped approval memory, interactive shell workflows, skill validation, picker actions, token reporting, and localized strings. ChangesRuntime recovery and approvals
Session and shell workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
jexShain
force-pushed
the
feat/0.4.0-session-management
branch
from
July 28, 2026 09:59
5cfdeba to
7b5e175
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
crates/aish-skills/src/registry/mod.rs (1)
252-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate validate+rollback logic between
installandinstall_with_cancel.Both methods repeat the same pattern: reserve check → find adapter →
pre_existedsnapshot → quarantine → adapter call chained withvalidate_installed_skill→rollback_fresh_installon error. This PR had to update both in lockstep to add the new validation step, which is a signal the logic should be extracted into a shared private helper (parameterized over the actual adapter call) to avoid future drift between the two paths.♻️ Sketch of a shared helper
fn install_via<F>(&self, skill: &RegistrySkill, target_dir: &Path, run: F) -> Result<InstallResult> where F: FnOnce(&dyn RegistryAdapter, &RegistrySkill, &Path) -> Result<InstallResult>, { Self::check_reserved(&skill.slug)?; let adapter = self.find_adapter(&skill.registry).ok_or_else(|| { aish_core::AishError::Skill(format!("No adapter for registry '{}'", skill.registry)) })?; let pre_existed = target_dir.join(&skill.slug).exists(); pre_quarantine(target_dir, &skill.slug)?; run(adapter, skill, target_dir) .and_then(|r| validate_installed_skill(&r.dir).map(|_| r)) .map_err(|e| { rollback_fresh_install(pre_existed, target_dir, &skill.slug); 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 252 - 302, Extract the shared reserve-check, adapter lookup, quarantine, validation, and rollback flow from install and install_with_cancel into a private install_via helper parameterized by the adapter operation. Update both public methods to delegate to this helper while passing their respective install calls, preserving cancellation behavior and existing error rollback semantics.crates/aish-shell/src/app.rs (1)
356-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild
RetryPolicyusing struct-update syntax. The project already disablesclippy::field_reassign_with_default, so this won’t fail CI, but replacing the default-then-assign pattern with
aish_llm::RetryPolicy { revert_on_cooldown: ..., ..Default::default() }keeps this construction less verbose and avoidslet mut policywhen mutation isn’t needed.🤖 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 356 - 357, The RetryPolicy constructions in crates/aish-shell/src/app.rs at lines 356-357 and 4573-4574 should use struct-update syntax with revert_on_cooldown initialized from config.fallback_revert_on_cooldown and remaining fields from Default::default(). Remove the mutable default-then-assignment pattern at both sites.crates/aish-llm/src/session.rs (3)
3296-3299: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStub keys on the full URL, not the host, unlike the real
WebFetchTool.
WebFetchTool::approval_keynormalizes and returns onlyhost_str(), so in productionhttps://example.com/aandhttps://example.com/bshare one approval. Keying on the whole URL here makes the comment ("key on the URL/host") slightly misleading; returning the host would exercise the same shape as the real tool (theexample.orgassertion still passes).🤖 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 3296 - 3299, Update the stub’s approval_key method to parse the url argument and return the normalized host, matching WebFetchTool::approval_key rather than the full URL. Preserve None for missing or invalid URLs and keep host-only approval behavior so different paths on the same host share a key.
1636-1642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
RememberSessionis a silent no-op when the tool has no approval key.Tools that return
Nonefromapproval_key(e.g. aweb_fetchcall with an unparseable URL) still let the user pick "remember", but nothing is stored and the next identical call re-prompts. A debug/trace log here would make that visible instead of looking like the memory silently failed.🤖 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 1636 - 1642, Update the RememberSession handling in the approval-choice flow to emit a debug or trace log when approval_memory exists but memory_key is None, indicating that no approval key was available and the choice was not persisted. Keep the existing remember behavior unchanged when a key is present.
498-544: 🚀 Performance & Scalability | 🔵 TrivialRotation loop looks correct; telemetry still attributes the primary model.
Termination is sound (
advance_on_erroris monotone over accounts/fallbacks), the fast path is preserved, and non-recoverable errors surface immediately. One gap: the request is issued withcred.model, butself.stream_ctxis untouched, so the Langfuse generation spans (self.stream_ctx.resolved_model()) andmodel_name()keep reporting the primary model even when a fallback answered. Consider surfacing the effective model alongside the response so traces attribute the right 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 498 - 544, Update the successful rotation path around stream_simple so the effective credential/model, specifically cred.model, is surfaced with the response and available to Langfuse telemetry. Ensure generation spans and model_name()/resolved_model() use the model that actually answered, including fallback models, rather than the primary model stored in self.stream_ctx.crates/aish-session/src/store.rs (1)
16-23: 🗄️ Data Integrity & Integration | 🔵 TrivialConsider indexing
parent_session_uuid.
list_childrenandsession_rootsfilter on this column, anddelete_sessionupdates by it; all are full table scans today. Session tables stay small, so this is a forward-looking note rather than a current problem.🤖 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 16 - 23, Evaluate adding an index on the sessions table’s parent_session_uuid column to support the filtering in list_children and session_roots and the update performed by delete_session. Define the index alongside the existing session schema or migration setup, preserving current behavior for sessions with NULL parent_session_uuid.crates/aish-llm/src/rotation.rs (1)
161-182: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNetwork keywords shadow explicit 5xx codes.
The network branch runs before the server-error branch, so a provider message like
API error 502: bad gateway, upstream connection resetclassifies asNetworkandadvance_on_errorreturnsfalse— no account rotation or model fallback for a genuine 5xx. Consider checking the explicit status codes / 5xx wording first, then falling through to the generic connection keywords.♻️ Reorder the 5xx check ahead of the generic network keywords
+ if lower.contains("500") + || lower.contains("502") + || lower.contains("503") + || lower.contains("504") + || lower.contains("server error") + || lower.contains("overloaded") + || lower.contains("service unavailable") + || lower.contains("bad gateway") + || lower.contains("internal error") + { + return Some(Self::ServerError); + } if lower.contains("timeout") || lower.contains("timed out") || lower.contains("connection") || lower.contains("connect") || lower.contains("reset") || lower.contains("unreachable") || lower.contains("broken pipe") { return Some(Self::Network); } - if lower.contains("500") - || lower.contains("502") - || lower.contains("503") - || lower.contains("504") - || lower.contains("server error") - || lower.contains("overloaded") - || lower.contains("service unavailable") - || lower.contains("bad gateway") - || lower.contains("internal error") - { - return Some(Self::ServerError); - }🤖 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/rotation.rs` around lines 161 - 182, Reorder the classification checks in the error-mapping function containing the Network and ServerError branches so explicit 5xx indicators, including status codes and server-error wording, are evaluated before generic connection keywords. Preserve the existing keyword sets and return values, ensuring messages that contain both a 5xx signal and connection text classify as ServerError.
🤖 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/en-US.yaml`:
- Around line 859-863: Update the model picker footer localization to include a
localized “m manage” hint in picker_footer for
crates/aish-i18n/locales/en-US.yaml lines 859-863, es-ES.yaml lines 504-508,
fr-FR.yaml lines 504-508, ja-JP.yaml lines 504-508, zh-CN.yaml lines 859-863,
and de-DE.yaml lines 504-508, preserving each locale’s existing wording and
formatting.
In `@crates/aish-session/src/store.rs`:
- Around line 594-621: Update the doc comments for migrate_schema and
add_column_if_missing to describe the actual unconditional ALTER TABLE behavior
and duplicate-column error handling, removing the inaccurate claim that columns
are detected via PRAGMA table_info. Preserve the existing concurrent-safe
migration implementation.
- Around line 253-284: Update fork_session to wrap the parent lookup and child
INSERT in a single unchecked_transaction, matching the transaction pattern used
by delete_session. Perform get_session and the insert through the transaction,
commit only after both succeed, and preserve the existing error handling and
returned SessionRecord behavior.
- Around line 179-185: Update the child-session reparenting SQL in the
transaction to also set branch_point_message_id to NULL when clearing
parent_session_uuid, ensuring promoted root sessions retain no reference to
deleted history. Keep the existing error handling and session selection
unchanged.
In `@crates/aish-shell/src/app.rs`:
- Around line 5787-5790: Update handle_model_command to inspect _parts and
preserve direct model selection when a model name argument is provided, while
retaining model_picker_panel for the no-argument /model form. Keep the
documented /model [name] behavior and existing argument handling conventions.
In `@crates/aish-shell/src/readline.rs`:
- Around line 52-57: Register /accounts and /fallback in the shared
SLASH_COMMANDS list in crates/aish-shell/src/readline.rs, preserving their
existing localized implementations and descriptions. Update the expected command
count to 23 at crates/aish-shell/src/readline.rs:836 and
crates/aish-shell/tests/slash_popup_commands.rs:33.
In `@crates/aish-skills/src/manager.rs`:
- Around line 302-335: Update parse_skill_metadata to reject SkillMetadata
values with empty name or description after YAML deserialization, restoring the
validation previously performed by verify_skill_dir. Keep the existing
context=subagent agent requirement unchanged so loader, installer, and verifier
all enforce non-empty metadata consistently.
In `@crates/aish-ui/src/select.rs`:
- Around line 468-477: Update the action-key handling in the select input path
around filtered_entries and PanelEvent::Submit so printable characters are not
interpreted as actions while normal search input is active. Require the existing
modifier or dedicated action mode before matching self.actions, preserving
ordinary character input for filtering and retaining action submission only in
explicit action mode.
---
Nitpick comments:
In `@crates/aish-llm/src/rotation.rs`:
- Around line 161-182: Reorder the classification checks in the error-mapping
function containing the Network and ServerError branches so explicit 5xx
indicators, including status codes and server-error wording, are evaluated
before generic connection keywords. Preserve the existing keyword sets and
return values, ensuring messages that contain both a 5xx signal and connection
text classify as ServerError.
In `@crates/aish-llm/src/session.rs`:
- Around line 3296-3299: Update the stub’s approval_key method to parse the url
argument and return the normalized host, matching WebFetchTool::approval_key
rather than the full URL. Preserve None for missing or invalid URLs and keep
host-only approval behavior so different paths on the same host share a key.
- Around line 1636-1642: Update the RememberSession handling in the
approval-choice flow to emit a debug or trace log when approval_memory exists
but memory_key is None, indicating that no approval key was available and the
choice was not persisted. Keep the existing remember behavior unchanged when a
key is present.
- Around line 498-544: Update the successful rotation path around stream_simple
so the effective credential/model, specifically cred.model, is surfaced with the
response and available to Langfuse telemetry. Ensure generation spans and
model_name()/resolved_model() use the model that actually answered, including
fallback models, rather than the primary model stored in self.stream_ctx.
In `@crates/aish-session/src/store.rs`:
- Around line 16-23: Evaluate adding an index on the sessions table’s
parent_session_uuid column to support the filtering in list_children and
session_roots and the update performed by delete_session. Define the index
alongside the existing session schema or migration setup, preserving current
behavior for sessions with NULL parent_session_uuid.
In `@crates/aish-shell/src/app.rs`:
- Around line 356-357: The RetryPolicy constructions in
crates/aish-shell/src/app.rs at lines 356-357 and 4573-4574 should use
struct-update syntax with revert_on_cooldown initialized from
config.fallback_revert_on_cooldown and remaining fields from Default::default().
Remove the mutable default-then-assignment pattern at both sites.
In `@crates/aish-skills/src/registry/mod.rs`:
- Around line 252-302: Extract the shared reserve-check, adapter lookup,
quarantine, validation, and rollback flow from install and install_with_cancel
into a private install_via helper parameterized by the adapter operation. Update
both public methods to delegate to this helper while passing their respective
install calls, preserving cancellation behavior and existing error rollback
semantics.
🪄 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: 318690ae-b271-4d61-9080-f228a2cd7a22
📒 Files selected for processing (28)
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/approval_memory.rscrates/aish-llm/src/lib.rscrates/aish-llm/src/rotation.rscrates/aish-llm/src/session.rscrates/aish-llm/src/types.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-shell/src/resume_selector.rscrates/aish-shell/src/token_store.rscrates/aish-shell/tests/slash_popup_commands.rscrates/aish-skills/src/manager.rscrates/aish-skills/src/registry/mod.rscrates/aish-skills/src/registry/verifier.rscrates/aish-tools/Cargo.tomlcrates/aish-tools/src/skill_registry/skill_registry.rscrates/aish-tools/src/web_fetch/web_fetch.rscrates/aish-ui/src/select.rs
jexShain
force-pushed
the
feat/0.4.0-session-management
branch
from
July 29, 2026 01:36
7b5e175 to
f6d7c0f
Compare
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-skills/src/registry/mod.rs`:
- Around line 262-271: The reinstall flow must preserve the existing trusted
skill when replacement installation or validation fails. In
crates/aish-skills/src/registry/mod.rs lines 262-271, stage and validate the
replacement outside the live directory, then atomically swap it in on success
and restore the prior directory and marker state on failure; apply the same
transactional behavior to the cancellable path at lines 292-299. Update the test
at lines 547-575 to assert the original SKILL.md remains unchanged and
.untrusted is absent after a failed reinstall.
🪄 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: b3c1a60a-9c2b-4164-bf89-99599cd14b66
📒 Files selected for processing (28)
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/approval_memory.rscrates/aish-llm/src/lib.rscrates/aish-llm/src/rotation.rscrates/aish-llm/src/session.rscrates/aish-llm/src/types.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-shell/src/resume_selector.rscrates/aish-shell/src/token_store.rscrates/aish-shell/tests/slash_popup_commands.rscrates/aish-skills/src/manager.rscrates/aish-skills/src/registry/mod.rscrates/aish-skills/src/registry/verifier.rscrates/aish-tools/Cargo.tomlcrates/aish-tools/src/skill_registry/skill_registry.rscrates/aish-tools/src/web_fetch/web_fetch.rscrates/aish-ui/src/select.rs
🚧 Files skipped from review as they are similar to previous changes (25)
- crates/aish-shell/src/token_store.rs
- crates/aish-shell/tests/slash_popup_commands.rs
- crates/aish-tools/src/web_fetch/web_fetch.rs
- crates/aish-shell/src/readline.rs
- crates/aish-session/src/models.rs
- crates/aish-tools/src/skill_registry/skill_registry.rs
- crates/aish-llm/src/lib.rs
- crates/aish-llm/src/types.rs
- crates/aish-tools/Cargo.toml
- crates/aish-shell/src/resume_selector.rs
- crates/aish-ui/src/select.rs
- crates/aish-i18n/locales/ja-JP.yaml
- crates/aish-i18n/locales/en-US.yaml
- crates/aish-shell/src/ai_handler.rs
- crates/aish-i18n/locales/zh-CN.yaml
- crates/aish-llm/src/approval_memory.rs
- crates/aish-llm/src/rotation.rs
- crates/aish-i18n/locales/de-DE.yaml
- crates/aish-config/src/model.rs
- crates/aish-skills/src/manager.rs
- crates/aish-i18n/locales/fr-FR.yaml
- crates/aish-llm/src/session.rs
- crates/aish-session/src/store.rs
- crates/aish-shell/src/app.rs
- crates/aish-i18n/locales/es-ES.yaml
jexShain
force-pushed
the
feat/0.4.0-session-management
branch
from
July 29, 2026 01:53
f6d7c0f to
8793d8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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)
284-333: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPre_quarantine failure still destroys the stashed original — restore logic double-fires.
inspect_erralready restoresstash -> livewhenpre_quarantinefails, but the outerErrarm (Line 325-329) unconditionally re-runs generic failure cleanup: it deleteslive(the just-restored original) and then tries to rename the stash again — except the stash no longer exists on disk (already consumed byinspect_err), so that rename silently fails vialet _ =. Net result: the previously-installed skill directory is permanently lost, which is the exact regression the previous review on this file was raised for, now reachable viapre_quarantinefailure instead of adapter/validate failure.🐛 Proposed fix: return early on pre_quarantine failure instead of falling through to generic cleanup
- let result = pre_quarantine(target_dir, &skill.slug) - .inspect_err(|_| { - if let Some(s) = &stash { - let _ = std::fs::rename(s, &live); - } - }) - .and_then(|_| match cancel { - None => adapter.install(skill, target_dir), - Some(flag) => adapter.install_with_cancel(skill, target_dir, flag), - }) - .and_then(|r| validate_installed_skill(&r.dir).map(|_| r)); + if let Err(e) = pre_quarantine(target_dir, &skill.slug) { + if let Some(s) = &stash { + let _ = std::fs::rename(s, &live); + } + return Err(e); + } + let result = match cancel { + None => adapter.install(skill, target_dir), + Some(flag) => adapter.install_with_cancel(skill, target_dir, flag), + } + .and_then(|r| validate_installed_skill(&r.dir).map(|_| r));Please also add a regression test analogous to
install_failed_reinstall_keeps_previous_skill_dirthat forcespre_quarantineto fail during a reinstall (e.g. via an invalid slug) and asserts the original survives untouched.🤖 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 284 - 333, Prevent install_transactional from running generic failure cleanup after pre_quarantine fails: handle that error separately, return it immediately after restoring the stashed directory, and only use the outer Err cleanup for adapter installation or validation failures. Add a regression test analogous to install_failed_reinstall_keeps_previous_skill_dir that forces pre_quarantine to fail during reinstall (for example with an invalid slug) and verifies the original skill directory remains intact.
🧹 Nitpick comments (3)
crates/aish-llm/src/rotation.rs (3)
674-680: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer struct-update syntax here.
clippy::field_reassign_with_defaultflagslet mut x = Default::default(); x.field = ....♻️ Proposed tweak
- let mut policy = RetryPolicy::default(); - policy.enabled = false; + let policy = RetryPolicy { + enabled: false, + ..RetryPolicy::default() + };🤖 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/rotation.rs` around lines 674 - 680, Update the disabled_policy_never_rotates test to construct RetryPolicy with struct-update syntax, setting enabled to false inline and filling remaining fields from RetryPolicy::default(); remove the mutable initialization and subsequent field assignment while preserving the existing rotation assertion.
283-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
current()andsnapshot()disagree when the selected account is disabled.
current()falls back to the first enabled account, whilesnapshot()returnscurrent_account: Nonefor that same state, so/tokenwould show no account while requests are actually going out under a real credential. Deriving both from one helper avoids the drift.♻️ Sketch
- pub fn current(&self, default_base: &str) -> ResolvedCredential { - let acct = self - .accounts - .get(self.current_account) - .filter(|a| !a.disabled) - .or_else(|| self.enabled_accounts().next().map(|(_, a)| a)) + fn effective_account(&self) -> Option<&ApiAccount> { + self.accounts + .get(self.current_account) + .filter(|a| !a.disabled) + .or_else(|| self.enabled_accounts().next().map(|(_, a)| a)) + } + + pub fn current(&self, default_base: &str) -> ResolvedCredential { + let acct = self + .effective_account()Then use
self.effective_account().map(|a| a.name.clone())insnapshot().Also applies to: 461-465
🤖 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/rotation.rs` around lines 283 - 311, Introduce a shared `effective_account()` helper that selects the current enabled account or falls back to the first enabled account, matching `current()` while preserving the default behavior when none exists. Update `current()` and `snapshot()` to derive their account information from this helper, including `snapshot()` using the effective account name instead of reporting `None` for a disabled selection.
499-707: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for
on_success/revert_on_cooldownorreset.The revert-to-primary path is the one piece of state machinery driven by wall-clock time and is untested. Injecting the
now: Instant(or a small clock seam) intoon_successwould make it testable without sleeps.🤖 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/rotation.rs` around lines 499 - 707, The rotation tests lack coverage for the time-dependent revert-to-primary behavior and reset handling. Add deterministic tests around RotationState::on_success and revert_on_cooldown using an injectable Instant or small clock seam instead of sleeps, verifying cooldown expiry restores the primary state and reset clears the relevant rotation state.
🤖 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-llm/src/rotation.rs`:
- Around line 417-431: Update use_account to remove only the selected account’s
entry from account_cooldowns after setting current_account, while preserving
cooldown state for all other accounts; do not clear the entire cooldown map
during manual account switching.
In `@crates/aish-skills/src/registry/mod.rs`:
- Around line 290-300: Validate skill.slug against the same path-safety rules
before the live path is joined or any reinstall stashing occurs in
install_transactional. Apply the validation to both fresh and existing registry
skills, rejecting ., .., separators, and NUL before live.exists(),
reinstall_stash_path(), rename, or cleanup/restore operations.
---
Outside diff comments:
In `@crates/aish-skills/src/registry/mod.rs`:
- Around line 284-333: Prevent install_transactional from running generic
failure cleanup after pre_quarantine fails: handle that error separately, return
it immediately after restoring the stashed directory, and only use the outer Err
cleanup for adapter installation or validation failures. Add a regression test
analogous to install_failed_reinstall_keeps_previous_skill_dir that forces
pre_quarantine to fail during reinstall (for example with an invalid slug) and
verifies the original skill directory remains intact.
---
Nitpick comments:
In `@crates/aish-llm/src/rotation.rs`:
- Around line 674-680: Update the disabled_policy_never_rotates test to
construct RetryPolicy with struct-update syntax, setting enabled to false inline
and filling remaining fields from RetryPolicy::default(); remove the mutable
initialization and subsequent field assignment while preserving the existing
rotation assertion.
- Around line 283-311: Introduce a shared `effective_account()` helper that
selects the current enabled account or falls back to the first enabled account,
matching `current()` while preserving the default behavior when none exists.
Update `current()` and `snapshot()` to derive their account information from
this helper, including `snapshot()` using the effective account name instead of
reporting `None` for a disabled selection.
- Around line 499-707: The rotation tests lack coverage for the time-dependent
revert-to-primary behavior and reset handling. Add deterministic tests around
RotationState::on_success and revert_on_cooldown using an injectable Instant or
small clock seam instead of sleeps, verifying cooldown expiry restores the
primary state and reset clears the relevant rotation state.
🪄 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: 106cb608-c302-4073-98d2-001e83f22a77
📒 Files selected for processing (28)
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/approval_memory.rscrates/aish-llm/src/lib.rscrates/aish-llm/src/rotation.rscrates/aish-llm/src/session.rscrates/aish-llm/src/types.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-shell/src/resume_selector.rscrates/aish-shell/src/token_store.rscrates/aish-shell/tests/slash_popup_commands.rscrates/aish-skills/src/manager.rscrates/aish-skills/src/registry/mod.rscrates/aish-skills/src/registry/verifier.rscrates/aish-tools/Cargo.tomlcrates/aish-tools/src/skill_registry/skill_registry.rscrates/aish-tools/src/web_fetch/web_fetch.rscrates/aish-ui/src/select.rs
🚧 Files skipped from review as they are similar to previous changes (25)
- crates/aish-shell/tests/slash_popup_commands.rs
- crates/aish-llm/src/lib.rs
- crates/aish-shell/src/token_store.rs
- crates/aish-tools/Cargo.toml
- crates/aish-config/src/lib.rs
- crates/aish-session/src/models.rs
- crates/aish-tools/src/skill_registry/skill_registry.rs
- crates/aish-llm/src/types.rs
- crates/aish-skills/src/registry/verifier.rs
- crates/aish-shell/src/resume_selector.rs
- crates/aish-shell/src/readline.rs
- crates/aish-skills/src/manager.rs
- crates/aish-config/src/model.rs
- crates/aish-i18n/locales/zh-CN.yaml
- crates/aish-i18n/locales/de-DE.yaml
- crates/aish-i18n/locales/fr-FR.yaml
- crates/aish-ui/src/select.rs
- crates/aish-i18n/locales/es-ES.yaml
- crates/aish-llm/src/session.rs
- crates/aish-llm/src/approval_memory.rs
- crates/aish-i18n/locales/ja-JP.yaml
- crates/aish-i18n/locales/en-US.yaml
- crates/aish-session/src/store.rs
- crates/aish-shell/src/ai_handler.rs
- crates/aish-shell/src/app.rs
jexShain
force-pushed
the
feat/0.4.0-session-management
branch
from
July 29, 2026 02:16
8793d8d to
196d5ff
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aish-llm/src/session.rs (1)
465-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRotation retry loop has no unit-test seam in this file.
The
#[cfg(test)] test_chat_responsesshort-circuit at the top ofchat_completion(lines 473-479) returns beforeself.rotationis even checked, so scripting responses viaset_test_chat_responsesalways bypasses the new rotation branch entirely. There's currently no way to unit-test the rotation retry/advance-on-error/exhaustion behavior from this file — onlyRotationState's standalone logic inrotation.rsis testable.Consider adding a rotation-aware test seam (e.g. a
#[cfg(test)]queue consumed inside the loop, right beforestream_simple, so failure classification and account/model advancement can be exercised end-to-end here.♻️ Sketch of a possible test seam
async fn chat_completion(...) -> Result<LlmResponse, AishError> { #[cfg(test)] if let Some(ref queue) = self.test_chat_responses { let mut q = queue.lock().expect("test chat response queue poisoned"); if !q.is_empty() { return q.remove(0); } } let Some(rotation) = &self.rotation else { ... }; loop { let cred = { ... }; + #[cfg(test)] + if let Some(ref queue) = self.test_rotation_responses { + let mut q = queue.lock().unwrap(); + if !q.is_empty() { + match q.remove(0) { + Ok(resp) => { rotation.lock().unwrap().on_success(); return Ok(resp); } + Err(err) => { /* run through the same classify/advance logic below */ } + } + } + } ...🤖 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 465 - 545, Adjust the #[cfg(test)] test_chat_responses seam in chat_completion so it does not return before rotation is evaluated. For the rotation path, consume queued test responses immediately before stream_simple inside the retry loop, allowing failures to pass through FailureKind classification, advance_on_error, and exhaustion handling; retain the existing direct-response behavior for the no-rotation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/aish-llm/src/session.rs`:
- Around line 465-545: Adjust the #[cfg(test)] test_chat_responses seam in
chat_completion so it does not return before rotation is evaluated. For the
rotation path, consume queued test responses immediately before stream_simple
inside the retry loop, allowing failures to pass through FailureKind
classification, advance_on_error, and exhaustion handling; retain the existing
direct-response behavior for the no-rotation path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d197a9f2-30ae-40f5-9cba-2d93c5849fcc
📒 Files selected for processing (28)
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/approval_memory.rscrates/aish-llm/src/lib.rscrates/aish-llm/src/rotation.rscrates/aish-llm/src/session.rscrates/aish-llm/src/types.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-shell/src/resume_selector.rscrates/aish-shell/src/token_store.rscrates/aish-shell/tests/slash_popup_commands.rscrates/aish-skills/src/manager.rscrates/aish-skills/src/registry/mod.rscrates/aish-skills/src/registry/verifier.rscrates/aish-tools/Cargo.tomlcrates/aish-tools/src/skill_registry/skill_registry.rscrates/aish-tools/src/web_fetch/web_fetch.rscrates/aish-ui/src/select.rs
🚧 Files skipped from review as they are similar to previous changes (25)
- crates/aish-shell/src/readline.rs
- crates/aish-shell/src/token_store.rs
- crates/aish-llm/src/lib.rs
- crates/aish-shell/src/resume_selector.rs
- crates/aish-config/src/lib.rs
- crates/aish-tools/src/skill_registry/skill_registry.rs
- crates/aish-config/src/model.rs
- crates/aish-shell/src/ai_handler.rs
- crates/aish-i18n/locales/de-DE.yaml
- crates/aish-shell/tests/slash_popup_commands.rs
- crates/aish-llm/src/types.rs
- crates/aish-skills/src/registry/verifier.rs
- crates/aish-tools/src/web_fetch/web_fetch.rs
- crates/aish-i18n/locales/es-ES.yaml
- crates/aish-i18n/locales/fr-FR.yaml
- crates/aish-i18n/locales/ja-JP.yaml
- crates/aish-i18n/locales/zh-CN.yaml
- crates/aish-skills/src/manager.rs
- crates/aish-llm/src/rotation.rs
- crates/aish-ui/src/select.rs
- crates/aish-i18n/locales/en-US.yaml
- crates/aish-llm/src/approval_memory.rs
- crates/aish-skills/src/registry/mod.rs
- crates/aish-session/src/store.rs
- crates/aish-shell/src/app.rs
…n hardening Session management commands: - /export: export current session (conversation + command history) to Markdown (AI-Shell-Team#411) - /sessions: interactive session-tree browser with recursive descent + switch (AI-Shell-Team#412) - /fork: branch the current session into a new one, preserving context (AI-Shell-Team#413) Data layer (aish-session): - fork_session / session_roots / list_children + parent_session_uuid and branch_point_message_id schema migration (additive, backward compatible) - delete_session reparents children to root so a forked parent delete no longer orphans its descendants out of the tree view Hardening & fixes (AI-Shell-Team#414): - /sessions switch now persists the current session state (no cwd loss) - /export escapes backticks in the history table and surfaces history-fetch errors instead of silently exporting an empty table - skill metadata validation unified via parse_skill_metadata — rejects context=fork/subagent skills that do not declare an agent - Tool::approval_key default + per-host web_fetch approval memory + tool-scoped approval memory (prevents cross-tool approval leakage) - SearchSelect relevance ranking (exact > prefix > substring) + single-key actions - credential rotation: network errors surface without demoting the primary model; context-length errors no longer misclassified as usage limits Closes AI-Shell-Team#411, AI-Shell-Team#412, AI-Shell-Team#413, AI-Shell-Team#414
jexShain
force-pushed
the
feat/0.4.0-session-management
branch
from
July 29, 2026 02:53
196d5ff to
cf3da26
Compare
This was referenced Jul 29, 2026
jexShain
added a commit
to jexShain/aish
that referenced
this pull request
Jul 29, 2026
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.
jexShain
added a commit
that referenced
this pull request
Jul 29, 2026
* fix(skills): unified metadata validation + transactional reinstall; rank 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 #414 (skill hardening + search-ranking slice; the remaining #414 items — approval memory, rotation, /export & /sessions fixes — ship with their feature PRs or a follow-up). * feat(model): multi-account rotation + model picker Restore the /model multi-account + automatic rotation feature that was deferred when the closed 0.4.0 mega-PR (#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. * refactor(ui): render panel action hints from registered labels 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. * fix(i18n): place picker keys under shell.model in all locales 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. * fix(model): address rotation/picker review comments 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.
Merged
3 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.
Summary
Delivers session-management commands (
/export,/sessions,/fork), the supporting data layer, and a batch of hardening/fixes. All tracked under the 0.4.0 milestone.Closes #411, #412, #413, #414
New commands
/export [md]([Feature]: /export 导出当前会话为 Markdown #411) — export the current session (metadata + AI conversation + command history) toaish-session-<short>.md./sessions([Feature]: /sessions 交互式会话树浏览与切换 #412) — interactive session-tree browser; depth-first descent shows roots and all descendants, submit switches (persisting current state first so no cwd is lost)./fork([Feature]: /fork 从当前会话分叉新会话 #413) — branch the current session into a new one that copies the parent's context/state, recordingparent_session_uuid+branch_point_message_id.Data layer (
aish-session)fork_session/session_roots/list_children.parent_session_uuid/branch_point_message_id(additive, backward compatible — verified against a legacy DB).delete_sessionnow reparents children to root before deleting, so a forked-parent delete no longer orphans its descendants out of the tree.Hardening & fixes (#414)
/sessionsswitch persists the current session snapshot (previously lost cwd since the last AI turn)./exportescapes backticks in the history table and surfaces history-fetch errors instead of silently exporting an empty table.parse_skill_metadata— rejectscontext=fork/subagentskills that don't declare anagent.Tool::approval_keydefault method + per-hostweb_fetchapproval memory + tool-scoped approval memory (prevents cross-tool approval leakage).SearchSelectrelevance ranking (exact > prefix > substring) + single-key actions.aish-llm): network errors now surface without demoting the primary model for the cooldown window; context-length errors (context_length_exceeded, …) are no longer misclassified as usage limits.Verification
cargo test --workspace— all green (the only pre-existing failure,readline::tests::test_slash_commands_formatcount mismatch, is fixed in this PR).delete_session_reparents_children_to_root, rotationclassifies_context_length_as_non_recoverable+network_failure_surfaces_without_demoting_primary_model.Notes
upstream/main(78117a9); single commit,crates/only (no docs/superpowers).service_supervisortooling that was in the working tree was intentionally excluded from this PR.Summary by CodeRabbit
/token./fork,/sessions, and/export, plus enhanced model/account and fallback-chain management.