feat: rust refactor sandbox runtime#154
Merged
Merged
Conversation
Contributor
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
Contributor
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
github-actions
Bot
added
packaging
📝 WalkthroughWalkthroughThis PR rewrites and expands the security/sandbox subsystem into Rust: new aish-security crate (policy, fallback, risk/decision, sandbox daemon/worker, IPC), LLM preflight integration, tool/shell wiring updates, packaging/systemd additions, and many tests and i18n additions. Ordering and manifest/Makefile tweaks included. ChangesSecurity Infrastructure Rewrite & Integration
Sequence DiagramsequenceDiagram
participant User as User/LLM
participant Shell as Shell
participant Tool as BashTool
participant Mgr as SecurityManager
participant Fallback as FallbackEngine
participant Client as SandboxClient
participant Daemon as SandboxDaemon
participant Worker as SandboxWorker
User->>Shell: request command
Shell->>Tool: invoke (ai_command)
Tool->>Mgr: analyze_with_request(cmd, is_ai=true)
alt is_ai_command == false
Mgr-->>Tool: (Low, analysis)
else
alt sandbox enabled
Mgr->>Client: simulate(request)
Client->>Daemon: connect & send request
Daemon->>Worker: spawn worker
Worker->>Worker: build overlay, execute, collect changes
Worker-->>Daemon: result
Daemon-->>Client: response
Client-->>Mgr: SandboxResult
Mgr->>Mgr: assess_sandbox_result
else
Mgr->>Fallback: assess_disabled_command(cmd)
Fallback-->>Mgr: FallbackRuleAssessment
Mgr->>Mgr: analyze_without_sandbox
end
Mgr-->>Tool: SecurityDecision
end
Tool->>Tool: execute (PTY)
Tool-->>Shell: output
Shell-->>User: display
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
crates/aish-i18n/locales/fr-FR.yaml-292-292 (1)
292-292:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent use of accented characters in French strings.
The rest of
fr-FR.yaml(including the newpanel_title.*andalternative.*keys you just added on lines 147–172) deliberately uses unaccented ASCII transliteration (e.g.,"securite","Avertissement de securite"). This new key uses accented forms ("nécessite","sécurité"). Either align with the file’s established convention, or, if the convention is intentionally moving to proper diacritics, do so consistently across the file.✏️ Proposed fix to match the rest of the file's convention
- security_handling_required: "la commande bash nécessite un traitement de sécurité de niveau {level}" + security_handling_required: "la commande bash necessite un traitement de securite de niveau {level}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-i18n/locales/fr-FR.yaml` at line 292, The new translation key security_handling_required uses accented characters ("nécessite", "sécurité") which is inconsistent with the existing fr-FR.yaml convention of unaccented ASCII (see panel_title.* and alternative.* keys); update the value of security_handling_required to use unaccented equivalents (e.g., "la commande bash necessite un traitement de securite de niveau {level}") so it matches the rest of the file, or if you intend to adopt diacritics across the file, apply that change consistently to the other keys (panel_title.* and alternative.*).crates/aish-cli/src/uninstall.rs-18-18 (1)
18-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded
SYSTEMD_UNIT_DIRmay miss units on non-standard installs.The constant is hardcoded to
/lib/systemd/system, but the Makefile allows overridingSYSTEMD_UNITDIRat build time. If the package is installed viamake install SYSTEMD_UNITDIR=/usr/lib/systemd/system(or another path),stop_and_remove_sandbox_units()will fail to clean up the units on uninstall. Consider acceptingSYSTEMD_UNIT_DIRvia environment variable, build-time configuration, or discovering the actual path at runtime.🤖 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-cli/src/uninstall.rs` at line 18, The constant SYSTEMD_UNIT_DIR is hardcoded and can miss non-standard installs; update stop_and_remove_sandbox_units() to resolve the actual unit directory instead of relying on const SYSTEMD_UNIT_DIR: e.g., prefer a build-time override (use env! or option from Cargo build script), then fall back to an environment variable (e.g., SYSTEMD_UNIT_DIR env) and finally probe common locations or query systemd (e.g., run `systemctl show -p UnitPath` and parse UnitPath) to find the unit path at runtime; replace direct uses of SYSTEMD_UNIT_DIR in stop_and_remove_sandbox_units() with this resolved path so uninstall removes units installed to alternative directories.crates/aish-security/src/sudo.rs-14-14 (1)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
options_with_valueis missing several sudo flags that take values.Sudo accepts multiple value-taking options not in this list. The current list only includes
-u,--user,-g,--group,-h,-p,--prompt, which causes incomplete option stripping. For example,sudo -C 10 apt updatewould be stripped to10 apt updatebecause-Cis treated as a no-value option, leaving the argument to be misinterpreted as the start of the command.Missing options include:
-A/--askpass,-a/--type,-C/--close-from,-c/--class,-D/--chdir,-r/--role,-U/--other-user,-T/--command-timeout, and-R/--directory(sudoedit only). Lines 45–51 should also be extended to handle the--*=variants of these options (e.g.,--chdir=,--type=,--close-from=, etc.).🤖 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-security/src/sudo.rs` at line 14, The options_with_value list is incomplete causing value-taking flags like -C to be treated as no-value; update the options_with_value array (used in sudo.rs) to include the missing short and long options: -A/--askpass, -a/--type, -C/--close-from, -c/--class, -D/--chdir, -r/--role, -U/--other-user, -T/--command-timeout, and any sudoedit-only flags such as -R/--directory; also extend the parsing logic that strips options (the code handling the "--*=" style long options) to recognize and strip the --chdir=, --type=, --close-from=, --class=, --command-timeout=, --directory=, --other-user=, --askpass=, and similar --name=value variants so their values aren’t left as command arguments.crates/aish-security/src/policy.rs-109-119 (1)
109-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrite the template via the just-created file handle instead of re-opening through
fs::write.The current pattern opens the file with
create_new(true), immediately drops theFile, and thenfs::writereopens it. This is racy (another process can observe an empty file in the gap, or replace it via a symlink swap) and adds a redundant open. Usewrite_allon the handle returned byOpenOptions::open.🛡️ Suggested fix
- let _ = fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(path) - .and_then(|_| fs::write(path, EMPTY_POLICY_TEMPLATE)); + use std::io::Write; + let _ = fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path) + .and_then(|mut file| file.write_all(EMPTY_POLICY_TEMPLATE.as_bytes()));🤖 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-security/src/policy.rs` around lines 109 - 119, The function ensure_user_policy_template currently opens the file with fs::OpenOptions::open and then drops the File before calling fs::write, which is racy and redundant; change it to keep the File returned by OpenOptions::open and call write_all(EMPTY_POLICY_TEMPLATE.as_bytes()) on that handle (via use of std::io::Write) so the template is written to the already-created file atomically from that handle, and propagate or handle errors consistently instead of reopening the path; update references in ensure_user_policy_template to use the File handle and EMPTY_POLICY_TEMPLATE when writing.crates/aish-security/src/sandbox/degraded.rs-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExtract magic string
"fallback_deleted"to a named constant for maintainability.
degraded.rscorrectly importscrate::types::FsChange(which haskind: String), whilesandbox/types.rsdefines a separate module-privateFsChangewithkind: FsChangeKindenum. The assignment of"fallback_deleted"at line 138–140 works, but it's a magic string that should be extracted to a&'static strconstant so future renames or audits can grep for all uses.Code context
.map(|path| FsChange { path: path.clone(), kind: "fallback_deleted".to_string(), detail: None, })🤖 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-security/src/sandbox/degraded.rs` at line 7, Extract the magic string "fallback_deleted" into a named constant (e.g. const FALLBACK_DELETED: &'static str = "fallback_deleted";) and use that constant when constructing the FsChange in the map closure instead of the literal string; update the map that builds FsChange { path: path.clone(), kind: "fallback_deleted".to_string(), detail: None } to use FALLBACK_DELETED (converted to String as needed) so all occurrences are discoverable and maintainable.crates/aish-security/src/sandbox/runtime/mount.rs-80-118 (1)
80-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix lossy path-to-string conversion via
display()for bind source and overlay mount data.Linux paths are byte sequences and may contain non-UTF-8 bytes. Using
display().to_string()substitutes replacement characters for invalid UTF-8, causing the bind source (line 104) and overlaydatastring (lines 92-97) to diverge from the actual on-disk paths. This can result in wrong-source binds or failed overlay mounts when paths contain non-UTF-8 bytes.The
targetpath correctly avoids this by staying asPathBufand routing throughpath_cstring(usingOsStr::as_bytes());sourceand overlay data should follow the same approach.For the bind source, change
MountCall::sourcefromOption<String>toOption<PathBuf>and update the call tosystem_mountto usepath_cstringinstead ofoptional_cstring:Proposed fix for bind source
#[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct MountCall { - pub(crate) source: Option<String>, + pub(crate) source: Option<PathBuf>, pub(crate) target: PathBuf, pub(crate) fstype: Option<String>, pub(crate) flags: libc::c_ulong, pub(crate) data: Option<String>, }Self::Bind { source, target, recursive, } => MountCall { - source: Some(source.display().to_string()), + source: Some(source.clone()), target: target.clone(), fstype: None, flags: libc::MS_BIND | if *recursive { libc::MS_REC } else { 0 }, data: None, },fn system_mount(call: &MountCall) -> io::Result<()> { - let source = optional_cstring(call.source.as_deref())?; + let source = call.source.as_deref().map(path_cstring).transpose()?; let target = path_cstring(&call.target)?;For overlay data, the kernel's
lowerdir=,upperdir=,workdir=format does not escape,or\cleanly, so realistic mitigation is to validate that overlay paths contain only UTF-8 and are free of,and\, or document this constraint clearly.🤖 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-security/src/sandbox/runtime/mount.rs` around lines 80 - 118, to_mount_call currently converts bind source and overlay path components to String via display(), losing non-UTF8 bytes; change MountCall::source from Option<String> to Option<PathBuf> and stop using display().to_string() in to_mount_call (for Bind use source.clone() as PathBuf), update the callers (system_mount) to accept Option<PathBuf> and call path_cstring instead of optional_cstring; for overlay data, avoid building a UTF-8 string from paths — either validate/ensure overlay paths are valid UTF-8 and contain no ',' or '\' and document that constraint, or store the lower/upper/work PathBufs in MountCall and delay composing the kernel data string until you can safely convert/validate them.crates/aish-security/src/sandbox/mod.rs-15-16 (1)
15-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't swallow
current_exe()failures.If
current_exe()fails here, the daemon keeps going withworker_programunset and the real failure gets pushed downstream into worker startup.Suggested fix
pub fn run_sandbox_daemon(socket_path: Option<&Path>) -> std::io::Result<()> { let mut options = runtime::daemon::SandboxDaemonOptions::default(); - options.worker_program = std::env::current_exe().ok(); + options.worker_program = Some(std::env::current_exe()?); if let Some(socket_path) = socket_path { options.socket_path = socket_path.to_path_buf(); }🤖 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-security/src/sandbox/mod.rs` around lines 15 - 16, The code currently ignores errors from std::env::current_exe() when setting options.worker_program; change this to handle the Result instead of calling .ok(): call std::env::current_exe() and on Err return or propagate the error (or log and return Err) so the failure surfaces during sandbox setup, then set options.worker_program = Some(path) on success; update the surrounding function (the one constructing runtime::daemon::SandboxDaemonOptions) to propagate the error type if necessary.packaging/scripts/uninstall-bundle.sh-87-90 (1)
87-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep uninstall backward-compatible with older bundled binaries.
Dropping
aish-sandboxfrom the removal list will leave that binary behind for users uninstalling an older bundle after upgrading to this layout.Proposed fix
-rm -f "$(target_path "${BIN_DIR}/aish")" "$(target_path "${BIN_DIR}/aish-uninstall")" +rm -f \ + "$(target_path "${BIN_DIR}/aish")" \ + "$(target_path "${BIN_DIR}/aish-uninstall")" \ + "$(target_path "${BIN_DIR}/aish-sandbox")"🤖 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 `@packaging/scripts/uninstall-bundle.sh` around lines 87 - 90, The uninstall script dropped removal of the legacy bundled binary, leaving aish-sandbox behind; update the rm invocation that uses target_path and BIN_DIR (the line removing "${BIN_DIR}/aish" and "${BIN_DIR}/aish-uninstall") to also remove the legacy "${BIN_DIR}/aish-sandbox" so remove_systemd_units + the rm call clean up both current and older bundled binaries for backward compatibility.crates/aish-shell/src/security_panel.rs-5-10 (1)
5-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
context.targetin fallback panels.This branch currently drops the target entirely, so any decision-less security panel loses the command/path it was created for.
Proposed fix
- return SecurityPanel::fallback( - context.tool_name.clone(), - context.message.clone(), - context.mode, - ); + return SecurityPanel { + mode: context.mode, + tool_name: context.tool_name.clone(), + target: context.target.clone(), + message: context.message.clone(), + risk_level: None, + reasons: Vec::new(), + alternatives: Vec::new(), + };🤖 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/security_panel.rs` around lines 5 - 10, The fallback branch that handles a missing decision currently constructs a SecurityPanel via SecurityPanel::fallback but omits the original context.target, losing the command/path; update the call in the branch that matches let Some(decision) = context.decision.as_ref() else { ... } to pass context.target.clone() (or otherwise preserve context.target) into SecurityPanel::fallback so the fallback panel retains the original target value.
🧹 Nitpick comments (9)
crates/aish-security/src/sandbox/error.rs (1)
28-79: ⚡ Quick winAdd a round-trip test to prevent drift between
as_str,from_wire, and the serde derive.The enum has three parallel sources of truth for the wire format: (1)
#[serde(rename_all = "snake_case")], (2) the explicitas_strtable, and (3) the explicitfrom_wiretable. They currently agree, but adding a new variant requires updating all three or behavior silently diverges (e.g., serde would still encode the variant whilefrom_wirereturnsNone).Two options:
- Cheap: add an exhaustive round-trip test that asserts
from_wire(as_str(v)) == Some(v)for every variant (and matches what serde produces).- Cleaner: drop the serde derive (or drop
as_str/from_wire) and derive only one. Withserde_plainyou getto_string/from_strfrom the serde derive; or usestrum::{AsRefStr, EnumString}directly.♻️ Round-trip test (minimal-effort fix)
#[test] fn wire_strings_round_trip_for_all_variants() { use SandboxReason::*; let all = [ BadRequest, RequestTooLarge, SandboxDisabled, SandboxDisabledByPolicy, SandboxIpcUnavailable, SandboxIpcTimeout, SandboxIpcProtocolError, SandboxIpcFailed, SandboxTimeout, SandboxExecuteFailed, SandboxCleanupFailed, SandboxUnavailable, SandboxException, SandboxFailed, CwdOutsideRepoRoot, OverlayMountFailed, OverlayPermFailed, BindMountFailed, RemountRoFailed, CommandNotFound, ]; for r in all { assert_eq!(SandboxReason::from_wire(r.as_str()), Some(r)); // Optional: also assert the serde derive matches as_str. let json = serde_json::to_string(&r).unwrap(); assert_eq!(json, format!("\"{}\"", r.as_str())); } }🤖 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-security/src/sandbox/error.rs` around lines 28 - 79, The enum SandboxReason has three sources of truth for its wire representation (the serde rename, the as_str method, and from_wire), so add an exhaustive round-trip test to prevent drift: write a test (e.g., wire_strings_round_trip_for_all_variants) that iterates every SandboxReason variant and asserts SandboxReason::from_wire(r.as_str()) == Some(r) and optionally that serde_json::to_string(&r) == format!("\"{}\"", r.as_str()); this ensures as_str, from_wire, and the serde derive stay in sync for all variants.crates/aish-security/src/sudo.rs (2)
79-122: 💤 Low valueQuote-stripping loses fidelity for downstream consumers — confirm that's intended.
read_tokenstrips outer quotes and unescapes backslashes from the producedout, butstrip_sudo_prefixultimately returnsraw_l[index..](the original slice), so quoting in the returned command is preserved. The unescaped/dequotedoutis only used for option matching (opt == "--",opt.starts_with("-u"), etc.), which is appropriate. Worth confirming no caller of helpers in this module has expectations thatoutretains quoting.Also,
expect("valid char boundary")is correct becauseindexis always advanced bych.len_utf8(), but adebug_assert!plus a gracefulbreakwould be more defensible if this were ever called on indices not produced by this function.🤖 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-security/src/sudo.rs` around lines 79 - 122, read_token currently uses expect("valid char boundary") which can panic if called with a bad index; replace those expect calls with debug_assert!(index < s.len()) and gracefully break the loop (or return (out, index)) when no char can be obtained to avoid panics in release builds, and add a short comment in read_token and strip_sudo_prefix documenting that read_token intentionally dequotes/unescapes only for option matching while strip_sudo_prefix returns the original raw slice for the final command so callers should not rely on out preserving original quoting.
4-4: 💤 Low valueDetect
sudofollowed by any whitespace, not only space.
raw_l.starts_with("sudo ")misses tab-separated invocations likesudo\tapt update. Minor, but cheap to harden:♻️ Proposed fix
- if !raw_l.starts_with("sudo ") && raw_l != "sudo" { - return (command.to_string(), false, true); - } + let after_sudo = raw_l.strip_prefix("sudo"); + let detected = match after_sudo { + Some(rest) => rest.is_empty() || rest.chars().next().map_or(false, char::is_whitespace), + None => false, + }; + if !detected { + return (command.to_string(), false, true); + }🤖 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-security/src/sudo.rs` at line 4, The current check uses raw_l.starts_with("sudo ") which only matches a literal space; update the condition around raw_l to treat any whitespace after "sudo" as valid (e.g., accept "sudo", or "sudo" followed by any Unicode whitespace character). Locate the conditional that uses raw_l.starts_with("sudo ") (the one comparing raw_l to "sudo") and change it to: accept when raw_l == "sudo" OR when raw_l starts with the 4-letter prefix "sudo" and the fifth character is any whitespace (use char::is_whitespace or split_whitespace/strip_prefix-based logic) so tab/newline-separated invocations like "sudo\tapt" are detected.crates/aish-security/src/policy.rs (2)
324-333: 💤 Low valueDead conjunct in
is_valid— the firstmatches!(parse_risk(...), …)is always true.
parse_riskalways returns one ofRiskLevel::{Low, Medium, High}(the only variants), so the first half of the&&is tautological. The actual validation is solely the second half (string is one of"LOW"|"MEDIUM"|"HIGH"). Drop the first arm to make intent explicit and avoid confusing future readers.♻️ Suggested simplification
- let is_valid = matches!( - parse_risk(risk_value, RiskLevel::Low), - RiskLevel::Low | RiskLevel::Medium | RiskLevel::High - ) && risk_value.and_then(Value::as_str).is_some_and(|text| { - matches!( - text.trim().to_ascii_uppercase().as_str(), - "LOW" | "MEDIUM" | "HIGH" - ) - }); + let is_valid = risk_value.and_then(Value::as_str).is_some_and(|text| { + matches!( + text.trim().to_ascii_uppercase().as_str(), + "LOW" | "MEDIUM" | "HIGH" + ) + });🤖 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-security/src/policy.rs` around lines 324 - 333, is_valid contains a dead conjunct because parse_risk(...) always returns a RiskLevel variant, so remove the redundant matches!(parse_risk(risk_value, RiskLevel::Low), ...) check and only validate the raw string form; update the is_valid expression (the variable defined alongside mapping_get and parse_risk references) to just use risk_value.and_then(Value::as_str).is_some_and(|text| matches!(text.trim().to_ascii_uppercase().as_str(), "LOW" | "MEDIUM" | "HIGH")) so intent is explicit and the redundant call to parse_risk/RiskLevel is dropped.
431-446: ⚡ Quick winDuplicated risk-validity predicate between
parse_invalid_fallback_rulesandvalid_itemsfilter.The same
LOW|MEDIUM|HIGHstring check is open-coded here and insideparse_invalid_fallback_rules. Extract a small helper (e.g.,fn has_valid_risk(item: &Mapping) -> bool) and call it from both sites so the two filters cannot drift apart and accept/reject different sets of rules.🤖 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-security/src/policy.rs` around lines 431 - 446, The risk-validity check is duplicated between parse_invalid_fallback_rules and the valid_items filter; add a small helper function like fn has_valid_risk(item: &Mapping) -> bool that encapsulates the logic currently used (mapping_get, Value::as_str, trim, to_ascii_uppercase, and match against "LOW" | "MEDIUM" | "HIGH") and replace the inline predicate in the valid_items filter and the check inside parse_invalid_fallback_rules to call has_valid_risk(&item) so both locations share the same implementation and cannot drift apart.crates/aish-security/src/sandbox/degraded.rs (1)
102-107: 💤 Low value
cwd/repo_rootreason line is only emitted when both are present.
if let (Some(cwd), Some(repo_root)) = ...silently drops the diagnostic when only one of them is captured (e.g.,with_cwdset butwith_repo_rootnot, or vice versa). ForCwdOutsideRepoRootboth are always set, but other reasons may fill only one. Consider emitting whatever is present so the operator-facingreasonslist never silently loses information.🤖 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-security/src/sandbox/degraded.rs` around lines 102 - 107, The current block only appends a cwd/repo_root reason when both details.cwd and details.repo_root are Some, which drops info if only one is present; update the logic around details.cwd and details.repo_root so you append whatever is present: push a "cwd=..." reason when details.cwd is Some, push a "repo_root=..." reason when details.repo_root is Some, and optionally keep a combined "cwd=..., repo_root=..." message when both are Some—modify the code that inspects details.cwd and details.repo_root (the variables named details, cwd, repo_root) to handle each Some case independently.crates/aish-security/src/sandbox/ipc/client.rs (1)
132-143: 💤 Low valueMove the
use std::path::PathBuf;abovesample_request.
sample_requestreferencesPathBuf::fromat lines 136–137 but theusedeclaration is on line 143. It compiles (uses are module-scoped) but reads oddly.♻️ Suggested ordering
+ use std::path::PathBuf; + fn sample_request() -> SandboxRunRequest { SandboxRunRequest { id: "req-1".to_string(), @@ } } - - use std::path::PathBuf;🤖 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-security/src/sandbox/ipc/client.rs` around lines 132 - 143, Move the import so it appears before the function that uses it: place the existing `use std::path::PathBuf;` line above the `fn sample_request() -> SandboxRunRequest { ... }` declaration; this keeps `sample_request`'s `PathBuf::from` usage clearly supported by the import and improves readability (look for the `sample_request` function and the `use std::path::PathBuf;` statement to adjust).crates/aish-security/src/sandbox/runtime/daemon.rs (1)
513-515: 💤 Low valueRemove the unused
log_optional_fieldfunction.The function is dead code with no callers anywhere in the workspace. It was likely left over from earlier iterations of the logging implementation.
🤖 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-security/src/sandbox/runtime/daemon.rs` around lines 513 - 515, Remove the dead helper function log_optional_field: it has no callers and should be deleted to avoid unused-code noise; locate the function definition named log_optional_field in daemon.rs and remove it (you can keep or verify related helper log_field remains if used elsewhere), then run cargo check to ensure no references remain and update any imports or documentation that might have referenced it.crates/aish-security/src/sandbox/runtime/overlay.rs (1)
523-540: ⚡ Quick winThis coverage never runs without
#[test].
single_repo_plan_uses_main_overlay_and_submount_overlayslooks like an intended unit test, but it is missing the attribute, so this plan-shape regression check is currently dead.Proposed fix
+ #[test] fn single_repo_plan_uses_main_overlay_and_submount_overlays() {🤖 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-security/src/sandbox/runtime/overlay.rs` around lines 523 - 540, The function single_repo_plan_uses_main_overlay_and_submount_overlays is intended as a unit test but lacks the #[test] attribute, so add #[test] immediately above the fn declaration to ensure the test runner executes it; locate the function in overlay.rs (the OverlayPlanBuilder test block) and annotate it with #[test], then run the test suite (cargo test) to verify it now executes and catches regressions.
🤖 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/session.rs`:
- Around line 403-417: The short_circuit branches (around the short_circuit
check in session.rs) currently emit GenerationEnd/OpEnd then return
Ok(String::new()), which discards the human-readable security-block message
created by execute_tool(); change the branch so it preserves and returns that
block message instead of an empty string: after calling/inspecting
execute_tool() (or the variable that holds its block message), emit the same
events (LlmEventType::GenerationEnd and OpEnd with reason "short_circuit") and
then return Ok(block_message) (or the existing execute_tool() message) so the
security notice is delivered to the caller; apply the same fix to the duplicated
branch near the 704-718 region.
In `@crates/aish-security/Cargo.toml`:
- Line 7: The blocking libc socket calls in security_preflight() and
security_preflight_with_socket() are being invoked directly from the async
execute_tool() path; move those calls into a blocking thread by wrapping them
with tokio::task::spawn_blocking (or make the Tool trait preflight async and
await an internally spawn_blocking call). Concretely, in aish-llm/src/session.rs
where execute_tool() calls security_preflight() or
security_preflight_with_socket(), replace the direct call with spawn_blocking(||
security_preflight(...)).await (or spawn_blocking(||
security_preflight_with_socket(...)).await) and propagate the result;
alternatively update Tool::preflight to be async and implement it using
spawn_blocking so all callers use await without blocking the runtime.
In `@crates/aish-security/src/command_match.rs`:
- Around line 1-563: The file crates/aish-security/src/command_match.rs is a
dead duplicate of the fallback implementation (defines FallbackRuleAssessment,
FallbackRuleEngine, constants and tests) and should be removed; delete the
entire command_match.rs file from the repository so there is only the canonical
implementation (crate::fallback) remaining, ensuring no other modules import
crate::command_match::FallbackRuleAssessment or
crate::command_match::FallbackRuleEngine before deletion.
In `@crates/aish-security/src/manager.rs`:
- Around line 167-170: The code currently defaults a missing request.repo_root()
to PathBuf::from("/") which widens scope; instead detect absence and fail
closed: replace the unwrap_or_else default with logic that returns an error (or
propagates one) when request.repo_root() is None so repo_root is only set from
Some(Path) via request.repo_root().map(Path::to_path_buf).ok_or_else(|| /*
appropriate error */)?; update the surrounding function signature to return
Result if needed and ensure callers of analyze()/decide() propagate or handle
the error rather than proceeding with "/" as the repo root.
In `@crates/aish-security/src/resolution.rs`:
- Around line 1-152: The file crates/aish-security/src/resolution.rs is an
unused duplicate of risk.rs; delete this file and any reference to its symbols
(e.g., analyze_without_sandbox, decision_from_risk, analysis_from_fallback,
analysis_from_default_risk, normalize_risk) so the crate only uses the canonical
module (risk). Remove the file from the repo, check lib.rs, service.rs,
manager.rs and Cargo manifests for stray mod/imports referencing resolution and
delete them if present, then run cargo build and cargo test to ensure no
unresolved symbols remain. Ensure any tests or uses refer to the functions in
risk.rs instead of resolution.rs before committing.
In `@crates/aish-security/src/sandbox/assess.rs`:
- Around line 6-10: assess_sandbox_result currently ignores the _command
parameter so rules with a specific PolicyRule.command_list are matched
regardless of command; update assess_sandbox_result (and the other affected
assessment spots noted) to use the actual command string and filter candidate
rules by checking PolicyRule.command_list: if the list is empty it applies to
all commands, otherwise require that the command is present in the rule's
command_list before considering the rule a match; refer to
PolicyRule.command_list and the assess_sandbox_result function (and the same
matching logic at the other referenced assessment locations) and add this
command membership check to restore correct policy semantics.
In `@crates/aish-security/src/sandbox/runtime/collect.rs`:
- Around line 352-355: The current filter only excludes exact scaffold root
matches (SINGLE_REPO_SCAFFOLD_ROOTS mapped via normalize_absolute_path against
plan.repo_root) but not their descendants, so files created under those roots
still pass; change the logic that builds scaffold_paths and the predicate that
tests change.path (the variable change.path) to check for descendant membership
(e.g., use Path::starts_with/starts_with-equivalent against each normalized
scaffold root) and return false if any scaffold root is a prefix of change.path
rather than requiring exact equality.
In `@crates/aish-security/src/sandbox/runtime/daemon.rs`:
- Around line 92-106: run_forever currently accepts connections and calls
serve_connection serially, letting read_request block indefinitely on the
socket; set a bounded read/write deadline on each accepted stream (e.g., using a
Duration derived from SandboxDaemonOptions::limits or a dedicated
"accept-to-request" timeout in options) before calling read_request inside
serve_connection, and modify serve_connection/run_forever to handle each
connection concurrently (spawn a thread or schedule on a worker pool) so one
slow peer cannot block others; apply the same deadline-setting change to the
other serve/accept sites referenced (lines 222-246) and ensure the deadline is
cleared/extended as needed when the request progresses.
In `@crates/aish-security/src/sandbox/runtime/overlay.rs`:
- Around line 397-410: The current encode_mount_path in overlay.rs is colliding
because it joins path components with '_' (e.g., /repo/a_b vs /repo/a/b); update
encode_mount_path to produce an injective encoding by encoding each path
component unambiguously and using a safe delimiter — for example, percent-encode
each Component::Normal (escape '_' and separator chars or use
percent-encoding/URL-encoding on value.to_string_lossy()) and then join with a
single literal '/' or another delimiter that cannot appear unescaped, or
alternately prefix each component with its length before joining; keep the
special-case for empty -> "root". Ensure you modify only encode_mount_path to
apply the new encoding so overlays (upper/work dirs) are unique per original
path.
In `@crates/aish-security/src/sandbox/runtime/worker.rs`:
- Around line 152-164: The temporary root created by create_temp_root()
(temp_root) is leaked if subsequent fallible steps (resolve_payload_execution,
OverlayPlanBuilder::build, runtime.prepare_mount_namespace, setup_overlay_plan)
fail; wrap temp_root in an RAII guard (e.g., TempRootGuard or use
tempfile::TempDir) that removes the directory in Drop, or implement explicit
cleanup calls (std::fs::remove_dir_all) on every early-return path before
payload execution; replace plain temp_root usages with the guard (pass
guard.path() into OverlayPlanBuilder::new and build, into setup_overlay_plan)
and ensure the guard is not dropped while the payload is running (only
drop/disable cleanup once the sandbox has successfully started).
- Around line 535-560: The temp directories created by create_temp_root are
inheriting process umask via fs::create_dir; change this to create directories
with owner-only permissions (0o700). Replace the fs::create_dir(&path) call in
create_temp_root with a std::fs::DirBuilder configured via
std::os::unix::fs::DirBuilderExt::mode(0o700) (e.g.,
DirBuilder::new().mode(0o700).create(&path)) and keep the same error handling
that maps failures to
SandboxError::with_details(SandboxReason::OverlayPermFailed, ...); if you prefer
to keep fs::create_dir, follow it with std::fs::set_permissions(&path,
std::fs::Permissions::from_mode(0o700)) and handle/set the same SandboxError on
failure.
- Around line 217-226: The error paths that return after failing to take or
write to stdin currently return early and drop `child` without reaping it;
update the block around `let mut stdin = child.stdin.take()` and the subsequent
`serde_json::to_writer(&mut stdin, context)` and `stdin.write_all(b"\n")` error
handling to first ensure the spawned `child` process is closed/reaped (e.g.,
call `child.kill()` if still running and then `child.wait()` or otherwise wait
for exit) before constructing and returning the `SandboxError` (keep using
`SandboxError::with_details` / `SandboxReason::SandboxExecuteFailed`), so any
failing path cleans up the `child` process to avoid zombies or orphaned workers.
In `@crates/aish-security/src/service.rs`:
- Around line 1-4: Delete the unused service.rs file (which defines a duplicate
SecurityManager) to remove dead code; remove any references to it. Consolidate
resolution.rs into risk.rs by keeping the implementations of normalize_risk,
decision_from_risk, and analyze_without_sandbox in risk.rs and deleting
resolution.rs, then update imports that referred to resolution.rs to use
crate::risk::{normalize_risk, decision_from_risk, analyze_without_sandbox} (e.g.
update use lines in manager.rs and the crate export in lib.rs) so manager.rs
uses the single risk module and the public API is consistent.
In `@crates/aish-tools/src/bash.rs`:
- Around line 404-410: preflight() currently uses std::env::current_dir(), which
can differ from the shell cwd of an existing PersistentPty used by execute();
change preflight to prefer the PTY session cwd when available (e.g., get the cwd
from the PersistentPty/pty session on self via its cwd/get_cwd method or field)
and only fall back to std::env::current_dir(). Update the same pattern in the
other preflight-like block around lines 423-430, and call
security_preflight(command, pty_cwd.as_deref(), None) so relative-path checks
use the PTY's working directory.
- Around line 227-234: The detached timeout sleeper currently does an
unconditional sleep and leaks threads; change the spawned closure to return
early when the command finishes by waiting on or polling the existing
cancel_token instead of sleeping the full timeout (e.g., use
cancel_token.cancelled().wait() if available or loop with short sleeps checking
cancel_token.is_cancelled()); replace the plain
std::thread::sleep(timeout_duration) in the closure that captures
Arc::clone(&cancel_token) with a cancellation-aware wait so the thread exits
immediately when cancel_token is triggered, and apply the same change to the
other identical block (the one around lines 309-316).
In `@crates/aish-tools/src/system_diagnose.rs`:
- Around line 182-183: The diagnose agent is creating a plain BashTool via
BashTool::new() which bypasses the LlmSession preflight/confirmation flow;
replace that instantiation so the diagnose sub-session builds its BashTool from
the LlmSession factory that includes the event callback and the command
preflight/security-notice handlers (i.e., create the BashTool through the same
LlmSession/constructor used elsewhere in system_diagnose_agent so the
confirmation flow is preserved instead of using crate::bash::BashTool::new()).
In `@packaging/scripts/install-bundle.sh`:
- Around line 153-158: The script currently swallows errors from systemd
commands (systemctl daemon-reload, systemctl enable aish-sandbox.socket,
systemctl start/restart aish-sandbox.socket) so the installer can report success
even if activation fails; change the logic in the block that runs when
AISH_SKIP_SYSTEMD is off to check command exit codes and fail (exit non‑zero
with an explanatory message logged to stderr) if any of these systemctl commands
fail on the supported path (use the same conditions that check systemctl
is-active for aish-sandbox.socket/aish-sandbox.service), and remove the || true
that masks failures for daemon-reload, enable, start and restart so the
installer surfaces activation errors instead of hiding them.
---
Minor comments:
In `@crates/aish-cli/src/uninstall.rs`:
- Line 18: The constant SYSTEMD_UNIT_DIR is hardcoded and can miss non-standard
installs; update stop_and_remove_sandbox_units() to resolve the actual unit
directory instead of relying on const SYSTEMD_UNIT_DIR: e.g., prefer a
build-time override (use env! or option from Cargo build script), then fall back
to an environment variable (e.g., SYSTEMD_UNIT_DIR env) and finally probe common
locations or query systemd (e.g., run `systemctl show -p UnitPath` and parse
UnitPath) to find the unit path at runtime; replace direct uses of
SYSTEMD_UNIT_DIR in stop_and_remove_sandbox_units() with this resolved path so
uninstall removes units installed to alternative directories.
In `@crates/aish-i18n/locales/fr-FR.yaml`:
- Line 292: The new translation key security_handling_required uses accented
characters ("nécessite", "sécurité") which is inconsistent with the existing
fr-FR.yaml convention of unaccented ASCII (see panel_title.* and alternative.*
keys); update the value of security_handling_required to use unaccented
equivalents (e.g., "la commande bash necessite un traitement de securite de
niveau {level}") so it matches the rest of the file, or if you intend to adopt
diacritics across the file, apply that change consistently to the other keys
(panel_title.* and alternative.*).
In `@crates/aish-security/src/policy.rs`:
- Around line 109-119: The function ensure_user_policy_template currently opens
the file with fs::OpenOptions::open and then drops the File before calling
fs::write, which is racy and redundant; change it to keep the File returned by
OpenOptions::open and call write_all(EMPTY_POLICY_TEMPLATE.as_bytes()) on that
handle (via use of std::io::Write) so the template is written to the
already-created file atomically from that handle, and propagate or handle errors
consistently instead of reopening the path; update references in
ensure_user_policy_template to use the File handle and EMPTY_POLICY_TEMPLATE
when writing.
In `@crates/aish-security/src/sandbox/degraded.rs`:
- Line 7: Extract the magic string "fallback_deleted" into a named constant
(e.g. const FALLBACK_DELETED: &'static str = "fallback_deleted";) and use that
constant when constructing the FsChange in the map closure instead of the
literal string; update the map that builds FsChange { path: path.clone(), kind:
"fallback_deleted".to_string(), detail: None } to use FALLBACK_DELETED
(converted to String as needed) so all occurrences are discoverable and
maintainable.
In `@crates/aish-security/src/sandbox/mod.rs`:
- Around line 15-16: The code currently ignores errors from
std::env::current_exe() when setting options.worker_program; change this to
handle the Result instead of calling .ok(): call std::env::current_exe() and on
Err return or propagate the error (or log and return Err) so the failure
surfaces during sandbox setup, then set options.worker_program = Some(path) on
success; update the surrounding function (the one constructing
runtime::daemon::SandboxDaemonOptions) to propagate the error type if necessary.
In `@crates/aish-security/src/sandbox/runtime/mount.rs`:
- Around line 80-118: to_mount_call currently converts bind source and overlay
path components to String via display(), losing non-UTF8 bytes; change
MountCall::source from Option<String> to Option<PathBuf> and stop using
display().to_string() in to_mount_call (for Bind use source.clone() as PathBuf),
update the callers (system_mount) to accept Option<PathBuf> and call
path_cstring instead of optional_cstring; for overlay data, avoid building a
UTF-8 string from paths — either validate/ensure overlay paths are valid UTF-8
and contain no ',' or '\' and document that constraint, or store the
lower/upper/work PathBufs in MountCall and delay composing the kernel data
string until you can safely convert/validate them.
In `@crates/aish-security/src/sudo.rs`:
- Line 14: The options_with_value list is incomplete causing value-taking flags
like -C to be treated as no-value; update the options_with_value array (used in
sudo.rs) to include the missing short and long options: -A/--askpass, -a/--type,
-C/--close-from, -c/--class, -D/--chdir, -r/--role, -U/--other-user,
-T/--command-timeout, and any sudoedit-only flags such as -R/--directory; also
extend the parsing logic that strips options (the code handling the "--*=" style
long options) to recognize and strip the --chdir=, --type=, --close-from=,
--class=, --command-timeout=, --directory=, --other-user=, --askpass=, and
similar --name=value variants so their values aren’t left as command arguments.
In `@crates/aish-shell/src/security_panel.rs`:
- Around line 5-10: The fallback branch that handles a missing decision
currently constructs a SecurityPanel via SecurityPanel::fallback but omits the
original context.target, losing the command/path; update the call in the branch
that matches let Some(decision) = context.decision.as_ref() else { ... } to pass
context.target.clone() (or otherwise preserve context.target) into
SecurityPanel::fallback so the fallback panel retains the original target value.
In `@packaging/scripts/uninstall-bundle.sh`:
- Around line 87-90: The uninstall script dropped removal of the legacy bundled
binary, leaving aish-sandbox behind; update the rm invocation that uses
target_path and BIN_DIR (the line removing "${BIN_DIR}/aish" and
"${BIN_DIR}/aish-uninstall") to also remove the legacy "${BIN_DIR}/aish-sandbox"
so remove_systemd_units + the rm call clean up both current and older bundled
binaries for backward compatibility.
---
Nitpick comments:
In `@crates/aish-security/src/policy.rs`:
- Around line 324-333: is_valid contains a dead conjunct because parse_risk(...)
always returns a RiskLevel variant, so remove the redundant
matches!(parse_risk(risk_value, RiskLevel::Low), ...) check and only validate
the raw string form; update the is_valid expression (the variable defined
alongside mapping_get and parse_risk references) to just use
risk_value.and_then(Value::as_str).is_some_and(|text|
matches!(text.trim().to_ascii_uppercase().as_str(), "LOW" | "MEDIUM" | "HIGH"))
so intent is explicit and the redundant call to parse_risk/RiskLevel is dropped.
- Around line 431-446: The risk-validity check is duplicated between
parse_invalid_fallback_rules and the valid_items filter; add a small helper
function like fn has_valid_risk(item: &Mapping) -> bool that encapsulates the
logic currently used (mapping_get, Value::as_str, trim, to_ascii_uppercase, and
match against "LOW" | "MEDIUM" | "HIGH") and replace the inline predicate in the
valid_items filter and the check inside parse_invalid_fallback_rules to call
has_valid_risk(&item) so both locations share the same implementation and cannot
drift apart.
In `@crates/aish-security/src/sandbox/degraded.rs`:
- Around line 102-107: The current block only appends a cwd/repo_root reason
when both details.cwd and details.repo_root are Some, which drops info if only
one is present; update the logic around details.cwd and details.repo_root so you
append whatever is present: push a "cwd=..." reason when details.cwd is Some,
push a "repo_root=..." reason when details.repo_root is Some, and optionally
keep a combined "cwd=..., repo_root=..." message when both are Some—modify the
code that inspects details.cwd and details.repo_root (the variables named
details, cwd, repo_root) to handle each Some case independently.
In `@crates/aish-security/src/sandbox/error.rs`:
- Around line 28-79: The enum SandboxReason has three sources of truth for its
wire representation (the serde rename, the as_str method, and from_wire), so add
an exhaustive round-trip test to prevent drift: write a test (e.g.,
wire_strings_round_trip_for_all_variants) that iterates every SandboxReason
variant and asserts SandboxReason::from_wire(r.as_str()) == Some(r) and
optionally that serde_json::to_string(&r) == format!("\"{}\"", r.as_str()); this
ensures as_str, from_wire, and the serde derive stay in sync for all variants.
In `@crates/aish-security/src/sandbox/ipc/client.rs`:
- Around line 132-143: Move the import so it appears before the function that
uses it: place the existing `use std::path::PathBuf;` line above the `fn
sample_request() -> SandboxRunRequest { ... }` declaration; this keeps
`sample_request`'s `PathBuf::from` usage clearly supported by the import and
improves readability (look for the `sample_request` function and the `use
std::path::PathBuf;` statement to adjust).
In `@crates/aish-security/src/sandbox/runtime/daemon.rs`:
- Around line 513-515: Remove the dead helper function log_optional_field: it
has no callers and should be deleted to avoid unused-code noise; locate the
function definition named log_optional_field in daemon.rs and remove it (you can
keep or verify related helper log_field remains if used elsewhere), then run
cargo check to ensure no references remain and update any imports or
documentation that might have referenced it.
In `@crates/aish-security/src/sandbox/runtime/overlay.rs`:
- Around line 523-540: The function
single_repo_plan_uses_main_overlay_and_submount_overlays is intended as a unit
test but lacks the #[test] attribute, so add #[test] immediately above the fn
declaration to ensure the test runner executes it; locate the function in
overlay.rs (the OverlayPlanBuilder test block) and annotate it with #[test],
then run the test suite (cargo test) to verify it now executes and catches
regressions.
In `@crates/aish-security/src/sudo.rs`:
- Around line 79-122: read_token currently uses expect("valid char boundary")
which can panic if called with a bad index; replace those expect calls with
debug_assert!(index < s.len()) and gracefully break the loop (or return (out,
index)) when no char can be obtained to avoid panics in release builds, and add
a short comment in read_token and strip_sudo_prefix documenting that read_token
intentionally dequotes/unescapes only for option matching while
strip_sudo_prefix returns the original raw slice for the final command so
callers should not rely on out preserving original quoting.
- Line 4: The current check uses raw_l.starts_with("sudo ") which only matches a
literal space; update the condition around raw_l to treat any whitespace after
"sudo" as valid (e.g., accept "sudo", or "sudo" followed by any Unicode
whitespace character). Locate the conditional that uses raw_l.starts_with("sudo
") (the one comparing raw_l to "sudo") and change it to: accept when raw_l ==
"sudo" OR when raw_l starts with the 4-letter prefix "sudo" and the fifth
character is any whitespace (use char::is_whitespace or
split_whitespace/strip_prefix-based logic) so tab/newline-separated invocations
like "sudo\tapt" are detected.
🪄 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: f4dcc749-0ec5-44c5-a934-3afbebe048d9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (77)
Cargo.tomlMakefilecrates/aish-cli/Cargo.tomlcrates/aish-cli/src/main.rscrates/aish-cli/src/uninstall.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/Cargo.tomlcrates/aish-llm/src/session.rscrates/aish-llm/src/types.rscrates/aish-security/Cargo.tomlcrates/aish-security/src/command_match.rscrates/aish-security/src/decision.rscrates/aish-security/src/fallback.rscrates/aish-security/src/lib.rscrates/aish-security/src/manager.rscrates/aish-security/src/overlay.rscrates/aish-security/src/policy.rscrates/aish-security/src/resolution.rscrates/aish-security/src/risk.rscrates/aish-security/src/sandbox.rscrates/aish-security/src/sandbox/assess.rscrates/aish-security/src/sandbox/degraded.rscrates/aish-security/src/sandbox/error.rscrates/aish-security/src/sandbox/ipc/client.rscrates/aish-security/src/sandbox/ipc/mod.rscrates/aish-security/src/sandbox/ipc/protocol.rscrates/aish-security/src/sandbox/mod.rscrates/aish-security/src/sandbox/runtime/collect.rscrates/aish-security/src/sandbox/runtime/daemon.rscrates/aish-security/src/sandbox/runtime/executor.rscrates/aish-security/src/sandbox/runtime/mod.rscrates/aish-security/src/sandbox/runtime/mount.rscrates/aish-security/src/sandbox/runtime/overlay.rscrates/aish-security/src/sandbox/runtime/worker.rscrates/aish-security/src/sandbox/types.rscrates/aish-security/src/sandbox_daemon.rscrates/aish-security/src/sandbox_ipc.rscrates/aish-security/src/sandbox_worker.rscrates/aish-security/src/service.rscrates/aish-security/src/strip_sudo.rscrates/aish-security/src/sudo.rscrates/aish-security/src/types.rscrates/aish-security/tests/fixtures/phase1-invalid-policy.yamlcrates/aish-security/tests/fixtures/phase1-legacy-policy.yamlcrates/aish-security/tests/phase1_fixtures.rscrates/aish-security/tests/security_integration_test.rscrates/aish-shell/Cargo.tomlcrates/aish-shell/src/ai_handler.rscrates/aish-shell/src/app.rscrates/aish-shell/src/lib.rscrates/aish-shell/src/security_panel.rscrates/aish-tools/src/bash.rscrates/aish-tools/src/lib.rscrates/aish-tools/src/secure_bash.rscrates/aish-tools/src/system_diagnose.rsdebian/aish-sandbox.servicedebian/controldocs/rust-i18n-analysis.mddocs/rust-rewrite-progress.mdpackaging/build_bundle.shpackaging/scripts/install-bundle.shpackaging/scripts/uninstall-bundle.shpackaging/systemd/README.mdpackaging/systemd/aish-sandbox.service.inpackaging/systemd/aish-sandbox.sockettests/security/sandbox/test_daemon_isolation.pytests/security/sandbox/test_fs_changes.pytests/security/sandbox/test_ipc.pytests/security/sandbox/test_root_overlay.pytests/security/sandbox/test_sandboxd.pytests/security/sandbox/test_umount_cleanup.pytests/security/sandbox/test_worker.py
💤 Files with no reviewable changes (19)
- crates/aish-security/src/strip_sudo.rs
- tests/security/sandbox/test_sandboxd.py
- docs/rust-rewrite-progress.md
- docs/rust-i18n-analysis.md
- crates/aish-security/src/sandbox.rs
- tests/security/sandbox/test_ipc.py
- crates/aish-security/src/sandbox_daemon.rs
- debian/aish-sandbox.service
- tests/security/sandbox/test_umount_cleanup.py
- tests/security/sandbox/test_root_overlay.py
- crates/aish-security/tests/security_integration_test.rs
- crates/aish-security/src/overlay.rs
- tests/security/sandbox/test_worker.py
- crates/aish-tools/src/lib.rs
- tests/security/sandbox/test_fs_changes.py
- tests/security/sandbox/test_daemon_isolation.py
- crates/aish-tools/src/secure_bash.rs
- crates/aish-security/src/sandbox_worker.rs
- crates/aish-security/src/sandbox_ipc.rs
|
|
||
| [dependencies] | ||
| aish-core.workspace = true | ||
| libc.workspace = true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find calls into aish-security from async contexts in aish-shell.
# Expect: calls to blocking security functions wrapped in spawn_blocking.
rg -n --type rust -B 2 -A 5 \
'spawn_blocking\|aish_security::\|SecurityContext\|preflight\|sandbox' \
crates/aish-shell/src/Repository: AI-Shell-Team/aish
Length of output: 44
🏁 Script executed:
# Check if aish-shell even depends on aish-security
cat crates/aish-shell/Cargo.toml | grep -A 20 "\[dependencies\]"Repository: AI-Shell-Team/aish
Length of output: 628
🏁 Script executed:
# List files in aish-shell to see what exists
find crates/aish-shell -type f -name "*.rs" | head -20Repository: AI-Shell-Team/aish
Length of output: 776
🏁 Script executed:
# Search for any imports of aish-security anywhere in aish-shell
rg "aish.security|use.*security" crates/aish-shell/ --type rustRepository: AI-Shell-Team/aish
Length of output: 1428
🏁 Script executed:
# Check if there are any async functions in aish-shell that might interact with security
rg "async fn|#\[tokio" crates/aish-shell/ --type rust | head -20Repository: AI-Shell-Team/aish
Length of output: 426
🏁 Script executed:
# Look at security_panel.rs in detail to see what it does
cat -n crates/aish-shell/src/security_panel.rs | head -100Repository: AI-Shell-Team/aish
Length of output: 4118
🏁 Script executed:
# Check if security_panel.rs is calling any aish_security functions or just pattern matching
rg "aish_security::" crates/aish-shell/src/security_panel.rs -A 2 -B 2Repository: AI-Shell-Team/aish
Length of output: 1300
🏁 Script executed:
# Look for where SecurityAnalysis or SecurityDecision are created/obtained
rg "SecurityDecision|SecurityAnalysis" crates/aish-shell/src/ --type rust -B 3Repository: AI-Shell-Team/aish
Length of output: 840
🏁 Script executed:
# Check the imports and module structure of ai_handler.rs
head -50 crates/aish-shell/src/ai_handler.rsRepository: AI-Shell-Team/aish
Length of output: 1780
🏁 Script executed:
# Search for function calls to aish_security, not just type usage
# Look for patterns like aish_security::function_name()
rg "aish_security::\w+\(" crates/aish-shell/ --type rustRepository: AI-Shell-Team/aish
Length of output: 44
🏁 Script executed:
# Check if there are any calls to functions in ai_handler.rs that use security
rg "SecurityDecision|SecurityAnalysis|aish_security::" crates/aish-shell/src/ai_handler.rs --type rustRepository: AI-Shell-Team/aish
Length of output: 44
🏁 Script executed:
# See if build_security_panel is called from async contexts
rg "build_security_panel" crates/aish-shell/ --type rust -B 3 -A 3Repository: AI-Shell-Team/aish
Length of output: 3219
🏁 Script executed:
# Check where PreflightSecurityContext comes from - verify it's from aish-llm not aish-security
rg "PreflightSecurityContext" crates/ --type rust | head -20Repository: AI-Shell-Team/aish
Length of output: 2028
🏁 Script executed:
# Look at bash.rs to see how PreflightSecurityContext is built
rg "PreflightSecurityContext" crates/aish-tools/src/bash.rs -B 5 -A 10Repository: AI-Shell-Team/aish
Length of output: 1147
🏁 Script executed:
# Check aish-llm to see if it calls aish-security blocking functions
rg "aish_security::" crates/aish-llm/src/ --type rust | head -30Repository: AI-Shell-Team/aish
Length of output: 128
🏁 Script executed:
# Look for where SecurityDecision is created/populated
rg "SecurityDecision\s*{" crates/ --type rust -A 5 | head -50Repository: AI-Shell-Team/aish
Length of output: 166
🏁 Script executed:
# Look for SecurityManager usage to see where blocking calls happen
rg "SecurityManager" crates/ --type rust -B 3 -A 5 | head -80Repository: AI-Shell-Team/aish
Length of output: 5716
🏁 Script executed:
# Check for any calls to execute, check, analyze, or similar on security objects
rg "\.check\(|\.execute\(|\.analyze\(" crates/aish-tools/src/bash.rs -B 2 -A 2Repository: AI-Shell-Team/aish
Length of output: 600
🏁 Script executed:
# Search for where SecurityDecision actually comes from in bash.rs
cat crates/aish-tools/src/bash.rs | head -200Repository: AI-Shell-Team/aish
Length of output: 6665
🏁 Script executed:
# Look at the Tool trait to understand how execute and preflight are called
rg "trait Tool|fn execute|fn preflight" crates/aish-llm/src/ --type rust -B 2 -A 5 | head -100Repository: AI-Shell-Team/aish
Length of output: 6240
🏁 Script executed:
# Check how BashTool is used - specifically if security_preflight is called from async code
cat crates/aish-tools/src/bash.rs | grep -A 30 "fn execute"Repository: AI-Shell-Team/aish
Length of output: 3526
🏁 Script executed:
# Search for all calls to security_preflight in bash.rs
rg "security_preflight" crates/aish-tools/src/bash.rs --type rust -B 3 -A 3Repository: AI-Shell-Team/aish
Length of output: 1546
🏁 Script executed:
# Look at the execute_tool implementation in session.rs to see how preflight is called
rg "fn execute_tool" crates/aish-llm/src/session.rs -A 40Repository: AI-Shell-Team/aish
Length of output: 3650
🏁 Script executed:
# Check if preflight is called directly from async fn execute_tool
rg "preflight" crates/aish-llm/src/session.rs -B 3 -A 5Repository: AI-Shell-Team/aish
Length of output: 1290
Wrap blocking security preflight checks in spawn_blocking.
The security_preflight() and security_preflight_with_socket() functions perform synchronous socket I/O to the sandbox daemon via libc. These are called directly from the async execute_tool() in aish-llm/src/session.rs without spawn_blocking, causing the tokio runtime thread to block on syscalls. This stalls the executor under concurrent tool execution.
Move the preflight check into spawn_blocking or refactor the Tool trait's preflight to be async.
Also applies to: 13-14
🤖 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-security/Cargo.toml` at line 7, The blocking libc socket calls in
security_preflight() and security_preflight_with_socket() are being invoked
directly from the async execute_tool() path; move those calls into a blocking
thread by wrapping them with tokio::task::spawn_blocking (or make the Tool trait
preflight async and await an internally spawn_blocking call). Concretely, in
aish-llm/src/session.rs where execute_tool() calls security_preflight() or
security_preflight_with_socket(), replace the direct call with spawn_blocking(||
security_preflight(...)).await (or spawn_blocking(||
security_preflight_with_socket(...)).await) and propagate the result;
alternatively update Tool::preflight to be async and implement it using
spawn_blocking so all callers use await without blocking the runtime.
Comment on lines
+167
to
+170
| let repo_root = request | ||
| .repo_root() | ||
| .map(Path::to_path_buf) | ||
| .unwrap_or_else(|| PathBuf::from("/")); |
There was a problem hiding this comment.
Don't widen missing repo context to /.
When sandboxing is enabled, an omitted repo_root currently becomes /, so the legacy analyze() / decide() path can silently turn a repo-scoped check into a whole-filesystem sandbox run. That makes missing request metadata look valid and can produce both misleading risk decisions and very expensive overlay setup. Please degrade or fail closed when repo_root is absent instead of defaulting it to /.
🤖 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-security/src/manager.rs` around lines 167 - 170, The code
currently defaults a missing request.repo_root() to PathBuf::from("/") which
widens scope; instead detect absence and fail closed: replace the unwrap_or_else
default with logic that returns an error (or propagates one) when
request.repo_root() is None so repo_root is only set from Some(Path) via
request.repo_root().map(Path::to_path_buf).ok_or_else(|| /* appropriate error
*/)?; update the surrounding function signature to return Result if needed and
ensure callers of analyze()/decide() propagate or handle the error rather than
proceeding with "/" as the repo root.
Comment on lines
+227
to
+234
| if let Some(timeout_secs) = timeout_secs { | ||
| let timeout_token = Arc::clone(&cancel_token); | ||
| let timeout_duration = Duration::from_secs(timeout_secs); | ||
| std::thread::spawn(move || { | ||
| std::thread::sleep(timeout_duration); | ||
| timeout_token.cancel(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Let timeout threads exit when the command finishes.
Both paths spawn detached sleeper threads that live until the full timeout elapses, even if the command exits right away. Repeated timed commands with large timeouts will steadily leak sleeping threads in a long-lived shell process.
Also applies to: 309-316
🤖 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/bash.rs` around lines 227 - 234, The detached timeout
sleeper currently does an unconditional sleep and leaks threads; change the
spawned closure to return early when the command finishes by waiting on or
polling the existing cancel_token instead of sleeping the full timeout (e.g.,
use cancel_token.cancelled().wait() if available or loop with short sleeps
checking cancel_token.is_cancelled()); replace the plain
std::thread::sleep(timeout_duration) in the closure that captures
Arc::clone(&cancel_token) with a cancellation-aware wait so the thread exits
immediately when cancel_token is triggered, and apply the same change to the
other identical block (the one around lines 309-316).
Comment on lines
+404
to
+410
| fn preflight(&self, args: &serde_json::Value) -> PreflightResult { | ||
| let Some(command) = args.get("command").and_then(|c| c.as_str()) else { | ||
| return PreflightResult::Allow; | ||
| }; | ||
|
|
||
| let cwd = std::env::current_dir().ok(); | ||
| security_preflight(command, cwd.as_deref(), None) |
There was a problem hiding this comment.
Use the PTY session cwd for preflight.
preflight() always resolves against std::env::current_dir(), but execute() may run inside an existing PersistentPty whose shell cwd has already changed. Any relative-path command can therefore be assessed against the wrong directory and get the wrong allow/confirm/block decision.
Also applies to: 423-430
🤖 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/bash.rs` around lines 404 - 410, preflight() currently
uses std::env::current_dir(), which can differ from the shell cwd of an existing
PersistentPty used by execute(); change preflight to prefer the PTY session cwd
when available (e.g., get the cwd from the PersistentPty/pty session on self via
its cwd/get_cwd method or field) and only fall back to std::env::current_dir().
Update the same pattern in the other preflight-like block around lines 423-430,
and call security_preflight(command, pty_cwd.as_deref(), None) so relative-path
checks use the PTY's working directory.
Comment on lines
182
to
+183
| let mut tools: Vec<Box<dyn Tool>> = vec![ | ||
| Box::new(bash_tool), | ||
| Box::new(crate::bash::BashTool::new()), |
There was a problem hiding this comment.
Reapply command preflight inside the diagnose sub-session.
system_diagnose_agent now exposes a plain BashTool from a fresh LlmSession that only gets the event callback. That drops the new confirmation/security-notice flow for any bash command the diagnose agent decides to run, so local diagnostic commands can execute without the shell-level approval gate.
🤖 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/system_diagnose.rs` around lines 182 - 183, The
diagnose agent is creating a plain BashTool via BashTool::new() which bypasses
the LlmSession preflight/confirmation flow; replace that instantiation so the
diagnose sub-session builds its BashTool from the LlmSession factory that
includes the event callback and the command preflight/security-notice handlers
(i.e., create the BashTool through the same LlmSession/constructor used
elsewhere in system_diagnose_agent so the confirmation flow is preserved instead
of using crate::bash::BashTool::new()).
Comment on lines
+153
to
+158
| systemctl daemon-reload >/dev/null 2>&1 || true | ||
| systemctl enable aish-sandbox.socket >/dev/null 2>&1 || true | ||
| if systemctl is-active --quiet aish-sandbox.socket || systemctl is-active --quiet aish-sandbox.service; then | ||
| systemctl restart aish-sandbox.socket | ||
| systemctl restart aish-sandbox.socket >/dev/null 2>&1 || true | ||
| else | ||
| systemctl start aish-sandbox.socket | ||
| systemctl start aish-sandbox.socket >/dev/null 2>&1 || true |
There was a problem hiding this comment.
Don't hide systemd activation failures on the supported path.
At this point the script has already decided systemd is active and AISH_SKIP_SYSTEMD is off. Swallowing daemon-reload / enable / start errors means the installer can finish successfully while aish-sandbox.socket never comes up.
Suggested fix
- systemctl daemon-reload >/dev/null 2>&1 || true
- systemctl enable aish-sandbox.socket >/dev/null 2>&1 || true
+ systemctl daemon-reload >/dev/null 2>&1
+ systemctl enable aish-sandbox.socket >/dev/null 2>&1
if systemctl is-active --quiet aish-sandbox.socket || systemctl is-active --quiet aish-sandbox.service; then
- systemctl restart aish-sandbox.socket >/dev/null 2>&1 || true
+ systemctl restart aish-sandbox.socket >/dev/null 2>&1
else
- systemctl start aish-sandbox.socket >/dev/null 2>&1 || true
+ systemctl start aish-sandbox.socket >/dev/null 2>&1
fi📝 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
| systemctl daemon-reload >/dev/null 2>&1 || true | |
| systemctl enable aish-sandbox.socket >/dev/null 2>&1 || true | |
| if systemctl is-active --quiet aish-sandbox.socket || systemctl is-active --quiet aish-sandbox.service; then | |
| systemctl restart aish-sandbox.socket | |
| systemctl restart aish-sandbox.socket >/dev/null 2>&1 || true | |
| else | |
| systemctl start aish-sandbox.socket | |
| systemctl start aish-sandbox.socket >/dev/null 2>&1 || true | |
| systemctl daemon-reload >/dev/null 2>&1 | |
| systemctl enable aish-sandbox.socket >/dev/null 2>&1 | |
| if systemctl is-active --quiet aish-sandbox.socket || systemctl is-active --quiet aish-sandbox.service; then | |
| systemctl restart aish-sandbox.socket >/dev/null 2>&1 | |
| else | |
| systemctl start aish-sandbox.socket >/dev/null 2>&1 | |
| fi |
🤖 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 `@packaging/scripts/install-bundle.sh` around lines 153 - 158, The script
currently swallows errors from systemd commands (systemctl daemon-reload,
systemctl enable aish-sandbox.socket, systemctl start/restart
aish-sandbox.socket) so the installer can report success even if activation
fails; change the logic in the block that runs when AISH_SKIP_SYSTEMD is off to
check command exit codes and fail (exit non‑zero with an explanatory message
logged to stderr) if any of these systemctl commands fail on the supported path
(use the same conditions that check systemctl is-active for
aish-sandbox.socket/aish-sandbox.service), and remove the || true that masks
failures for daemon-reload, enable, start and restart so the installer surfaces
activation errors instead of hiding them.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-security/src/sandbox/assess.rs`:
- Around line 35-52: Don't return early on non-zero sandbox.exit_code; instead
build the SecurityAnalysis from the recorded changes as normal, then apply a
minimum risk floor of RiskLevel::Medium and augment that analysis with sandbox
failure metadata. Concretely: after producing the analysis based on changes, set
analysis.risk_level = std::cmp::max(analysis.risk_level, RiskLevel::Medium),
push "sandbox_execute_failed" onto analysis.reasons (if not already present),
set analysis.sandbox_off_action = Some(policy.sandbox_off_action) and fill
analysis.sandbox = SandboxStatus { enabled: true, reason:
Some("sandbox_execute_failed".to_string()), exit_code: Some(sandbox.exit_code),
..SandboxStatus::default() }, preserving the original changes vector and any
higher risk levels recorded earlier.
- Around line 71-79: The current selection logic (assigning (risk_level,
selected_hits) from high_hits/medium_hits/low_hits) drops unmatched paths so
their policy.default_risk_level and preview reasons are ignored; update the
logic in the block handling risk calculation (symbols: risk_level,
selected_hits, high_hits, medium_hits, low_hits, unmatched,
policy.default_risk_level, reasons) to fold unmatched into the aggregate: 1)
choose the highest risk among matched buckets and policy.default_risk_level when
unmatched is non-empty (i.e., risk_level = max(chosen_risk,
policy.default_risk_level) if unmatched.len()>0), 2) merge unmatched hits into
selected_hits so their entries are preserved, and 3) append unmatched
preview/reason entries into reasons so they are surfaced; apply the same fix to
the other identical block referenced (lines ~97-113).
🪄 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: 8356de7e-1dcc-4a88-9257-69d798ad88bb
📒 Files selected for processing (6)
crates/aish-llm/src/session.rscrates/aish-security/src/fallback.rscrates/aish-security/src/manager.rscrates/aish-security/src/sandbox/assess.rscrates/aish-security/src/sandbox/runtime/collect.rscrates/aish-security/src/sandbox/runtime/overlay.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/aish-llm/src/session.rs
- crates/aish-security/src/sandbox/runtime/collect.rs
- crates/aish-security/src/manager.rs
- crates/aish-security/src/sandbox/runtime/overlay.rs
- crates/aish-security/src/fallback.rs
Comment on lines
+35
to
+52
| if sandbox.exit_code != 0 { | ||
| return SecurityAnalysis { | ||
| risk_level: RiskLevel::Medium, | ||
| reasons: vec![format!( | ||
| "sandbox execution returned non-zero exit code {}; require confirmation", | ||
| sandbox.exit_code | ||
| )], | ||
| changes, | ||
| sandbox_off_action: Some(policy.sandbox_off_action), | ||
| sandbox: SandboxStatus { | ||
| enabled: true, | ||
| reason: Some("sandbox_execute_failed".to_string()), | ||
| exit_code: Some(sandbox.exit_code), | ||
| ..SandboxStatus::default() | ||
| }, | ||
| ..SecurityAnalysis::default() | ||
| }; | ||
| } |
There was a problem hiding this comment.
Preserve recorded high-risk changes on sandbox failure.
Lines 35-52 return Medium before any policy evaluation, so a command that already produced a recorded High change gets downgraded to confirm-only as soon as the sandbox exits non-zero. That weakens the policy gate exactly when the sandbox is already in a degraded state. Keep evaluating the recorded changes, then apply a Medium floor plus the "sandbox_execute_failed" reason instead of unconditionally returning.
🤖 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-security/src/sandbox/assess.rs` around lines 35 - 52, Don't
return early on non-zero sandbox.exit_code; instead build the SecurityAnalysis
from the recorded changes as normal, then apply a minimum risk floor of
RiskLevel::Medium and augment that analysis with sandbox failure metadata.
Concretely: after producing the analysis based on changes, set
analysis.risk_level = std::cmp::max(analysis.risk_level, RiskLevel::Medium),
push "sandbox_execute_failed" onto analysis.reasons (if not already present),
set analysis.sandbox_off_action = Some(policy.sandbox_off_action) and fill
analysis.sandbox = SandboxStatus { enabled: true, reason:
Some("sandbox_execute_failed".to_string()), exit_code: Some(sandbox.exit_code),
..SandboxStatus::default() }, preserving the original changes vector and any
higher risk levels recorded earlier.
Comment on lines
+71
to
+79
| let (risk_level, selected_hits) = if !high_hits.is_empty() { | ||
| (RiskLevel::High, high_hits) | ||
| } else if !medium_hits.is_empty() { | ||
| (RiskLevel::Medium, medium_hits) | ||
| } else if !low_hits.is_empty() { | ||
| (RiskLevel::Low, low_hits) | ||
| } else { | ||
| (policy.default_risk_level, Vec::new()) | ||
| }; |
There was a problem hiding this comment.
Unmatched paths are dropped from the overall risk calculation.
Once any rule matches, the aggregate decision is based only on high_hits / medium_hits / low_hits, and unmatched stops affecting risk_level. That can under-classify mixed results: e.g. one low-risk /tmp/** match plus one unmatched path still returns Low even when policy.default_risk_level is Medium or High. Please fold unmatched changes into the max-risk calculation and keep surfacing their preview in reasons even when some paths matched rules.
Also applies to: 97-113
🤖 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-security/src/sandbox/assess.rs` around lines 71 - 79, The current
selection logic (assigning (risk_level, selected_hits) from
high_hits/medium_hits/low_hits) drops unmatched paths so their
policy.default_risk_level and preview reasons are ignored; update the logic in
the block handling risk calculation (symbols: risk_level, selected_hits,
high_hits, medium_hits, low_hits, unmatched, policy.default_risk_level, reasons)
to fold unmatched into the aggregate: 1) choose the highest risk among matched
buckets and policy.default_risk_level when unmatched is non-empty (i.e.,
risk_level = max(chosen_risk, policy.default_risk_level) if unmatched.len()>0),
2) merge unmatched hits into selected_hits so their entries are preserved, and
3) append unmatched preview/reason entries into reasons so they are surfaced;
apply the same fix to the other identical block referenced (lines ~97-113).
F16shen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/aish-pty/src/persistent.rs (3)
2304-2304: LGTM: Idiomatic use ofis_some_and.This is cleaner than the previous
map_orpattern.🤖 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-pty/src/persistent.rs` at line 2304, No change required—the use of is_some_and in the loop condition (the while lines.last().is_some_and(|l| l.trim().is_empty()) expression) is already the idiomatic, cleaner replacement for the previous map_or pattern; leave this loop as-is and do not revert to map_or or manual matches.
2369-2369: LGTM: Cleaner delimiter parsing.Using
containson a byte slice is more readable than chaining multiple inequality checks.🤖 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-pty/src/persistent.rs` at line 2369, Replace the long chain of inequality checks used for delimiter detection with a single slice lookup using contains: locate the expression that checks bytes[j] against multiple delimiter bytes (currently comparing to ' ', '\n', '\r', ';', '&', '|', '<', '>', '#') and change it to use a byte slice like [b' ', b'\n', b'\r', b';', b'&', b'|', b'<', b'>', b'#'].contains(&bytes[j]), making sure to preserve the original boolean sense (negate if the original used != checks) in the surrounding condition inside the parsing function that references bytes[j].
1607-1607: LGTM: Idiomatic range check.Using
containson a range is more readable than explicit bounds checking.🤖 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-pty/src/persistent.rs` at line 1607, The match arm using Some(b) if (0x40..=0x7E).contains(&b) is already the preferred idiomatic range check and requires no change; keep this pattern as-is (the Some(b) match arm in persistent.rs) or, if desired for clarity, add a short comment explaining the allowed byte range but do not replace the contains-based check.
🤖 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-pty/src/persistent.rs`:
- Around line 1386-1409: The function drain_stdin_trailing currently only reads
a single byte (using discard[0]) after a 10ms select, but the comment/docs imply
draining multiple trailing bytes; change drain_stdin_trailing to loop:
repeatedly call select with the short timeout (tv) and when select > 0 perform a
read into a small buffer (e.g., 32 or 64 bytes) to consume as many available
bytes as possible, repeating until select returns 0 or <0, then return. Ensure
you reuse/reset the fd_set and timeval for each iteration and keep the function
name drain_stdin_trailing and the existing select/read usage to locate the code
to change.
In `@crates/aish-security/src/sandbox/runtime/worker.rs`:
- Around line 863-866: The assertion checking for "setpriv" is incorrect because
command.args contains the constant SETPRIV_PATH (e.g. "/usr/bin/setpriv"), so
replace the tautological check assert!(!command.args.iter().any(|arg| arg ==
"setpriv")) with a check that uses the same SETPRIV_PATH constant used in
sibling tests (e.g. assert!(!command.args.iter().any(|arg| arg ==
SETPRIV_PATH))) in the sudo→root test; locate the test that locks seen_commands
and inspects command.args and update the comparison to use SETPRIV_PATH (and
keep the existing assert_eq that verifies the last arg).
- Around line 433-476: The bwrap invocation needs the flags "--die-with-parent",
"--unshare-pid", and "--new-session" added to the args list so child processes
are killed on timeout and run in a separate PID namespace; modify the args
vector built before extending with SETPRIV_PATH/BASH_PATH (look around
append_runtime_bind_mounts, append_payload_home, and where PayloadCommand is
constructed) to include these three flags (order: e.g. --unshare-pid,
--new-session, --die-with-parent) so they apply to both PayloadIdentity::User
and PayloadIdentity::Root runs.
---
Nitpick comments:
In `@crates/aish-pty/src/persistent.rs`:
- Line 2304: No change required—the use of is_some_and in the loop condition
(the while lines.last().is_some_and(|l| l.trim().is_empty()) expression) is
already the idiomatic, cleaner replacement for the previous map_or pattern;
leave this loop as-is and do not revert to map_or or manual matches.
- Line 2369: Replace the long chain of inequality checks used for delimiter
detection with a single slice lookup using contains: locate the expression that
checks bytes[j] against multiple delimiter bytes (currently comparing to ' ',
'\n', '\r', ';', '&', '|', '<', '>', '#') and change it to use a byte slice like
[b' ', b'\n', b'\r', b';', b'&', b'|', b'<', b'>', b'#'].contains(&bytes[j]),
making sure to preserve the original boolean sense (negate if the original used
!= checks) in the surrounding condition inside the parsing function that
references bytes[j].
- Line 1607: The match arm using Some(b) if (0x40..=0x7E).contains(&b) is
already the preferred idiomatic range check and requires no change; keep this
pattern as-is (the Some(b) match arm in persistent.rs) or, if desired for
clarity, add a short comment explaining the allowed byte range but do not
replace the contains-based check.
🪄 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: 136398c8-c975-4005-a920-61335927d466
📒 Files selected for processing (18)
crates/aish-llm/src/langfuse.rscrates/aish-llm/src/streaming.rscrates/aish-pty/src/lib.rscrates/aish-pty/src/persistent.rscrates/aish-pty/src/session_interceptor.rscrates/aish-security/src/decision.rscrates/aish-security/src/sandbox/mod.rscrates/aish-security/src/sandbox/runtime/collect.rscrates/aish-security/src/sandbox/runtime/daemon.rscrates/aish-security/src/sandbox/runtime/executor.rscrates/aish-security/src/sandbox/runtime/mount.rscrates/aish-security/src/sandbox/runtime/overlay.rscrates/aish-security/src/sandbox/runtime/worker.rscrates/aish-security/src/sandbox/types.rscrates/aish-shell/src/app.rscrates/aish-tools/src/bash.rscrates/aish-tools/src/channel_ask_user.rscrates/aish-tools/src/channel_bash.rs
✅ Files skipped from review due to trivial changes (5)
- crates/aish-llm/src/streaming.rs
- crates/aish-tools/src/channel_bash.rs
- crates/aish-pty/src/lib.rs
- crates/aish-pty/src/session_interceptor.rs
- crates/aish-tools/src/channel_ask_user.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/aish-security/src/sandbox/mod.rs
- crates/aish-security/src/decision.rs
- crates/aish-security/src/sandbox/runtime/executor.rs
- crates/aish-security/src/sandbox/types.rs
- crates/aish-shell/src/app.rs
- crates/aish-security/src/sandbox/runtime/overlay.rs
- crates/aish-security/src/sandbox/runtime/collect.rs
- crates/aish-security/src/sandbox/runtime/daemon.rs
- crates/aish-security/src/sandbox/runtime/mount.rs
- crates/aish-tools/src/bash.rs
Comment on lines
1386
to
1409
| fn drain_stdin_trailing(stdin_fd: libc::c_int) { | ||
| let mut discard = [0u8; 1]; | ||
| loop { | ||
| let mut rfds: libc::fd_set = unsafe { std::mem::zeroed() }; | ||
| unsafe { | ||
| libc::FD_ZERO(&mut rfds); | ||
| libc::FD_SET(stdin_fd, &mut rfds); | ||
| } | ||
| let mut tv = libc::timeval { | ||
| tv_sec: 0, | ||
| tv_usec: 10_000, // 10ms | ||
| }; | ||
| let sel = unsafe { | ||
| libc::select( | ||
| stdin_fd + 1, | ||
| &mut rfds, | ||
| std::ptr::null_mut(), | ||
| std::ptr::null_mut(), | ||
| &mut tv, | ||
| ) | ||
| }; | ||
| if sel <= 0 { | ||
| break; | ||
| } | ||
| match unsafe { libc::read(stdin_fd, discard.as_mut_ptr() as *mut libc::c_void, 1) } { | ||
| 1 => { | ||
| if discard[0] == b'\n' || discard[0] == b'\r' { | ||
| break; | ||
| } | ||
| // Non-newline byte — stop draining | ||
| break; | ||
| } | ||
| _ => break, | ||
| } | ||
| let mut rfds: libc::fd_set = unsafe { std::mem::zeroed() }; | ||
| unsafe { | ||
| libc::FD_ZERO(&mut rfds); | ||
| libc::FD_SET(stdin_fd, &mut rfds); | ||
| } | ||
| let mut tv = libc::timeval { | ||
| tv_sec: 0, | ||
| tv_usec: 10_000, // 10ms | ||
| }; | ||
| let sel = unsafe { | ||
| libc::select( | ||
| stdin_fd + 1, | ||
| &mut rfds, | ||
| std::ptr::null_mut(), | ||
| std::ptr::null_mut(), | ||
| &mut tv, | ||
| ) | ||
| }; | ||
| if sel > 0 { | ||
| let _ = unsafe { libc::read(stdin_fd, discard.as_mut_ptr() as *mut libc::c_void, 1) }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check usage patterns and comments around drain_stdin_trailing
rg -n -B3 -A3 'drain_stdin_trailing' --type rustRepository: AI-Shell-Team/aish
Length of output: 2515
🏁 Script executed:
rg -n -i 'todo|fixme|bug' crates/aish-pty/src/persistent.rs | grep -A2 -B2 'drain\|stdin'Repository: AI-Shell-Team/aish
Length of output: 44
🏁 Script executed:
# Also check the docstring more carefully and surrounding context
sed -n '1383,1410p' crates/aish-pty/src/persistent.rsRepository: AI-Shell-Team/aish
Length of output: 861
Consider draining multiple trailing bytes.
The function reads at most one byte with a 10ms timeout, but the docstring says "trailing bytes" (plural). If a terminal sends \r\n (two bytes) after the confirmation read, only the first will be consumed with the current implementation. The docstring should either be updated to reflect single-byte behavior, or the function should drain multiple bytes in a loop until no more data arrives within the timeout window.
🤖 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-pty/src/persistent.rs` around lines 1386 - 1409, The function
drain_stdin_trailing currently only reads a single byte (using discard[0]) after
a 10ms select, but the comment/docs imply draining multiple trailing bytes;
change drain_stdin_trailing to loop: repeatedly call select with the short
timeout (tv) and when select > 0 perform a read into a small buffer (e.g., 32 or
64 bytes) to consume as many available bytes as possible, repeating until select
returns 0 or <0, then return. Ensure you reuse/reset the fd_set and timeval for
each iteration and keep the function name drain_stdin_trailing and the existing
select/read usage to locate the code to change.
Comment on lines
+433
to
+476
| let mut args = vec![ | ||
| "--bind".to_string(), | ||
| plan.merged_root.display().to_string(), | ||
| "/".to_string(), | ||
| "--dev".to_string(), | ||
| "/dev".to_string(), | ||
| "--proc".to_string(), | ||
| "/proc".to_string(), | ||
| "--chdir".to_string(), | ||
| plan.sandbox_cwd.display().to_string(), | ||
| ]; | ||
|
|
||
| append_runtime_bind_mounts(plan, &mut args); | ||
| append_payload_home(&mut args, payload_identity); | ||
|
|
||
| match payload_identity { | ||
| PayloadIdentity::User { uid, gid } => { | ||
| args.extend([ | ||
| SETPRIV_PATH.to_string(), | ||
| "--reuid".to_string(), | ||
| uid.to_string(), | ||
| "--regid".to_string(), | ||
| gid.to_string(), | ||
| "--clear-groups".to_string(), | ||
| "--inh-caps=-all".to_string(), | ||
| BASH_PATH.to_string(), | ||
| "-lc".to_string(), | ||
| command.to_string(), | ||
| ]); | ||
| } | ||
| PayloadIdentity::Root => { | ||
| args.extend([ | ||
| BASH_PATH.to_string(), | ||
| "-lc".to_string(), | ||
| command.to_string(), | ||
| ]); | ||
| } | ||
| } | ||
|
|
||
| PayloadCommand { | ||
| program: "bwrap".to_string(), | ||
| args, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
bubblewrap --die-with-parent grandchild cleanup requires --unshare-pid
💡 Result:
Yes: bubblewrap’s grandchild (process tree) cleanup behavior that depends on --die-with-parent requires using --unshare-pid (i.e., being in a separate PID namespace) to reliably trigger kernel-assisted cleanup for processes spawned by the COMMAND. Reasoning grounded in sources: 1) bubblewrap’s man page documents that --die-with-parent “Ensures child process (COMMAND) dies when bwrap's parent dies” and does so via PR_SET_PDEATHSIG (parent-death signal, SIGKILL) [1]. 2) bubblewrap maintainers/users report that “both --die-with-parent and --unshare-pid had to be specified for bwrap to get the kernel to properly kill … spawned child processes” (i.e., the cleanup doesn’t work for grandchildren unless PID namespace is unshared) [2]. 3) The same issue explains the mechanism: with --unshare-pid, --die-with-parent effectively kills process 1 in the sandbox PID namespace; killing PID 1 in a PID namespace causes the rest of that namespace’s processes to receive SIGKILL [2]. That’s how grandchild cleanup propagates. 4) There are also known limitations/races with --die-with-parent itself (e.g., PR_SET_PDEATHSIG can be racy and cleanup may fail in timing edge cases) [3][4], so even with --unshare-pid you shouldn’t treat it as perfectly deterministic. Practical takeaway: Use --unshare-pid alongside --die-with-parent when your COMMAND may spawn grandchildren, if you want the kernel to clean up the whole namespaced process tree when bwrap exits [2].
Citations:
- 1: https://man.archlinux.org/man/extra/bubblewrap/bwrap.1.en
- 2: --die-with-parent does not kill spawned processes unless --unshare-pid is set containers/bubblewrap#529
- 3:
--die-with-parentfails to clean up due to a race condition if the parent bwrap process is killed soon after startup containers/bubblewrap#633 - 4:
PR_SET_PDEATHSIGis racy unless there is a check for the parent having already died containers/bubblewrap#700
🏁 Script executed:
# First, locate the file and examine the relevant code section
head -500 crates/aish-security/src/sandbox/runtime/worker.rs | tail -100Repository: AI-Shell-Team/aish
Length of output: 2879
🏁 Script executed:
# Get the full context around lines 433-476
sed -n '430,480p' crates/aish-security/src/sandbox/runtime/worker.rsRepository: AI-Shell-Team/aish
Length of output: 1517
🏁 Script executed:
# Check the cleanup and timeout code around lines 593-605
sed -n '590,610p' crates/aish-security/src/sandbox/runtime/worker.rsRepository: AI-Shell-Team/aish
Length of output: 709
🏁 Script executed:
# Search for any existing --die-with-parent or --unshare-pid in the codebase
rg "\-\-(die-with-parent|unshare-pid)" crates/Repository: AI-Shell-Team/aish
Length of output: 44
🏁 Script executed:
# Search for join_output_reader implementation
rg -A 15 "fn join_output_reader" crates/aish-security/src/sandbox/runtime/worker.rsRepository: AI-Shell-Team/aish
Length of output: 529
🏁 Script executed:
# Check the full timeout and cleanup flow
sed -n '570,615p' crates/aish-security/src/sandbox/runtime/worker.rsRepository: AI-Shell-Team/aish
Length of output: 1449
🏁 Script executed:
# Look for any design documentation or comments about SingleRepo or namespace strategy
rg -B 5 -A 5 "SingleRepo" crates/aish-security/Repository: AI-Shell-Team/aish
Length of output: 5681
🏁 Script executed:
# Check what append_runtime_bind_mounts does - does it reveal additional binds?
rg -A 20 "fn append_runtime_bind_mounts" crates/aish-security/src/sandbox/runtime/worker.rsRepository: AI-Shell-Team/aish
Length of output: 681
Add --die-with-parent --unshare-pid --new-session for timeout cleanup robustness.
The bwrap invocation lacks flags needed to reliably clean up child processes on timeout. Per bubblewrap maintainers, --die-with-parent only kills spawned subprocesses when paired with --unshare-pid — the kernel uses the separate PID namespace to cascade SIGKILL from PID 1 to all grandchildren. Without both flags:
- Timeout hang risk: If the payload spawns a daemon that inherits stdout/stderr and doesn't exit on parent death, the reader threads (spawned at the timeout handler) remain blocked in
read(), andjoin_output_reader()at lines 604-605 hangs indefinitely. - Namespace exposure: Payload sees the host PID namespace via
/procand inherits the parent's controlling terminal.
Adding these three flags strengthens cleanup guarantees and process isolation without changing filesystem semantics (SingleRepo still binds /etc and runtime roots as 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-security/src/sandbox/runtime/worker.rs` around lines 433 - 476,
The bwrap invocation needs the flags "--die-with-parent", "--unshare-pid", and
"--new-session" added to the args list so child processes are killed on timeout
and run in a separate PID namespace; modify the args vector built before
extending with SETPRIV_PATH/BASH_PATH (look around append_runtime_bind_mounts,
append_payload_home, and where PayloadCommand is constructed) to include these
three flags (order: e.g. --unshare-pid, --new-session, --die-with-parent) so
they apply to both PayloadIdentity::User and PayloadIdentity::Root runs.
Comment on lines
+863
to
+866
| let command = seen_commands.lock().unwrap().pop().unwrap(); | ||
| assert!(!command.args.iter().any(|arg| arg == "setpriv")); | ||
| assert_eq!(command.args.last().map(String::as_str), Some("echo hi")); | ||
| } |
There was a problem hiding this comment.
Broken setpriv assertion in sudo→root test is a no-op.
command.args always contains SETPRIV_PATH ("/usr/bin/setpriv") rather than the bare string "setpriv", so arg == "setpriv" never matches. The negated assertion is a tautology and will pass even if a future regression made build_payload_command emit setpriv for the sudo path. Use the same SETPRIV_PATH constant the sibling tests at Lines 800 and 825 already use.
🛠️ Proposed fix
- assert!(!command.args.iter().any(|arg| arg == "setpriv"));
+ assert!(!command.args.iter().any(|arg| arg == SETPRIV_PATH));📝 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
| let command = seen_commands.lock().unwrap().pop().unwrap(); | |
| assert!(!command.args.iter().any(|arg| arg == "setpriv")); | |
| assert_eq!(command.args.last().map(String::as_str), Some("echo hi")); | |
| } | |
| let command = seen_commands.lock().unwrap().pop().unwrap(); | |
| assert!(!command.args.iter().any(|arg| arg == SETPRIV_PATH)); | |
| assert_eq!(command.args.last().map(String::as_str), Some("echo hi")); | |
| } |
🤖 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-security/src/sandbox/runtime/worker.rs` around lines 863 - 866,
The assertion checking for "setpriv" is incorrect because command.args contains
the constant SETPRIV_PATH (e.g. "/usr/bin/setpriv"), so replace the tautological
check assert!(!command.args.iter().any(|arg| arg == "setpriv")) with a check
that uses the same SETPRIV_PATH constant used in sibling tests (e.g.
assert!(!command.args.iter().any(|arg| arg == SETPRIV_PATH))) in the sudo→root
test; locate the test that locks seen_commands and inspects command.args and
update the comparison to use SETPRIV_PATH (and keep the existing assert_eq that
verifies the last arg).
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.
Background
This PR lands the Rust sandbox refactor on top of the latest
rustbranch and keeps the branch history focused after resolving upstream conflicts.Changes
aish-securitysandbox implementation with the new Rust sandbox runtime layoutpackaging/systemdand update bundle/install packaging scriptsValidation
cargo check --workspacecargo test -p aish-security -p aish-toolsRisk
cargo fmt --checkstill reports formatting issues in upstream channel-tool files outside this branch's diffSummary by CodeRabbit
New Features
Bug Fixes
Chores