fix(security): restore sandbox block reasons and wire security panel (#408, #409) - #426
Conversation
Write human-facing rule.reason into sandbox assessment results and fall back impact_description to reason, then annotate format_security_message with rule id/paths so Block text no longer degrades to a generic HIGH level sentence. Fixes AI-Shell-Team#408
Connect confirmation/security_notice callbacks to the rich security panel, separate Reason/Rule/Paths rows, and draw a complete confirm box before reading input so bottom borders are not delayed until keypress. Reserve viewport space via ScrollUp instead of inserting blank lines. Fixes AI-Shell-Team#409
|
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 |
|
Template check passed. Thanks for updating the pull request description. |
📝 WalkthroughWalkthroughThe change propagates matched rule reasons, identities, and paths through sandbox assessment and Bash messages. It adds structured localized security-panel rows and routes interactive and non-interactive security flows through centralized shell rendering. ChangesSecurity panel enrichment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxAssessment
participant BashPreflight
participant SecurityPanel
participant ShellApp
participant Operator
SandboxAssessment->>BashPreflight: provide rule reason and impact description
BashPreflight->>SecurityPanel: provide primary message, rule IDs, and paths
ShellApp->>SecurityPanel: render security context panel
SecurityPanel->>Operator: display localized rows and approval choices
Operator->>ShellApp: return approval verdict
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Satisfy CI format-check for the AI-Shell-Team#408/AI-Shell-Team#409 branch.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/aish-shell/src/security_panel.rs (1)
343-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case covering degraded sandbox together with matched paths.
The current tests only exercise a clean decision. The gap flagged at lines 76-101 (fallback hint suppressed when
matched_pathsis non-empty) is not covered. Add a context whereanalysis.sandbox.reason = Some("sandbox_ipc_unavailable"),analysis.sandbox.enabled = false, andmatched_pathsis non-empty, then assert both the paths row and the fallback-hint row are present.🤖 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 343 - 395, Add a test context for degraded sandbox behavior with analysis.sandbox.reason set to "sandbox_ipc_unavailable", analysis.sandbox.enabled false, and non-empty matched_paths. Exercise security_panel_rows and assert that both the matched paths row and the fallback-hint row are present, covering the path where the hint must not be suppressed by matched paths.crates/aish-tools/src/bash/bash.rs (1)
264-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the annotation format an explicit shared contract.
annotate_security_messageencodes rule id and paths into the message string as" (H-001; paths: /etc)".crates/aish-shell/src/security_panel.rs::strip_message_annotationparses that same suffix back out with a heuristic (rfind(" (")plus apaths:/H/M/Lprefix check). Today the strip path only runs when noSecurityDecisionis attached, so the two never disagree in practice. If a future change routes an annotated message through the fallback path, the heuristic can strip a legitimate parenthetical or miss a real annotation.Prefer keeping the structured fields (
matched_rule,matched_paths) as the single source and letting the panel format them, instead of round-tripping through text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-tools/src/bash/bash.rs` around lines 264 - 296, The security annotation is encoded in text and later heuristically parsed by strip_message_annotation, creating an implicit format contract. Refactor annotate_security_message and the related security-panel flow to preserve and pass matched_rule and matched_paths as structured data, then format them in the panel; remove the annotation round-trip and heuristic stripping while preserving the current displayed rule-id and path information.crates/aish-shell/src/app.rs (1)
10498-10515: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClamp the wrap width before calling
wrap_text.
widthis accepted fromCOLUMNSwithout a lower bound, andwidth.saturating_sub(14)can be0.wrap_textreturns the source text unchanged when the width is0, so no termination issue is present here. The main effect is that the estimate at line 10512 and the rendered lines at line 10592 can use a different wrapping width frominner_width, which changes approval-panel layout.🛡️ Proposed fix
- for (_, value) in rows { + let wrap_width = width.saturating_sub(14).max(20); + for (_, value) in rows { let safe_value = sanitize_for_display(value); if safe_value.is_empty() { lines += 1; continue; } for raw_line in safe_value.lines() { - let wrapped = wrap_text(raw_line, width.saturating_sub(14)); + let wrapped = wrap_text(raw_line, wrap_width); lines += wrapped.lines().count().max(1); } }Apply the matching change at line 10592 so the estimate and the render stay in sync.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-shell/src/app.rs` around lines 10498 - 10515, Clamp the wrapping width consistently in estimate_security_panel_lines and the corresponding render path so both use the same bounded width derived from inner_width. Update the wrap_text call near the rendering logic as well as the estimate, preserving synchronized approval-panel layout for small terminal widths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aish-shell/src/security_panel.rs`:
- Around line 178-193: Update strip_message_annotation so annotation detection
validates an explicit rule-ID shape instead of accepting any suffix part
beginning with H, M, or L. Preserve paths: handling and stripping only when the
parenthetical contains the expected identifiers such as H-001, M-001, or L-001,
while leaving legitimate text like “Host unreachable” unchanged.
- Around line 76-101: Make the matched-paths and degraded-sandbox hint rows
independent in the security panel: change the fallback-hint branch following the
matched-paths handling so it can also run when paths are present. In that
branch, reuse the existing reason value computed earlier in the panel instead of
calling primary_panel_reason again, while preserving the existing note-empty and
duplicate checks.
---
Nitpick comments:
In `@crates/aish-shell/src/app.rs`:
- Around line 10498-10515: Clamp the wrapping width consistently in
estimate_security_panel_lines and the corresponding render path so both use the
same bounded width derived from inner_width. Update the wrap_text call near the
rendering logic as well as the estimate, preserving synchronized approval-panel
layout for small terminal widths.
In `@crates/aish-shell/src/security_panel.rs`:
- Around line 343-395: Add a test context for degraded sandbox behavior with
analysis.sandbox.reason set to "sandbox_ipc_unavailable",
analysis.sandbox.enabled false, and non-empty matched_paths. Exercise
security_panel_rows and assert that both the matched paths row and the
fallback-hint row are present, covering the path where the hint must not be
suppressed by matched paths.
In `@crates/aish-tools/src/bash/bash.rs`:
- Around line 264-296: The security annotation is encoded in text and later
heuristically parsed by strip_message_annotation, creating an implicit format
contract. Refactor annotate_security_message and the related security-panel flow
to preserve and pass matched_rule and matched_paths as structured data, then
format them in the panel; remove the annotation round-trip and heuristic
stripping while preserving the current displayed rule-id and path information.
🪄 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: 8ae95c42-f5b2-4f83-a0f1-f80148680012
📒 Files selected for processing (10)
crates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-security/src/sandbox/assess.rscrates/aish-shell/src/app.rscrates/aish-shell/src/security_panel.rscrates/aish-tools/src/bash/bash.rs
Keep Paths and degraded-sandbox Note independent so users still see when assessment fell back, tighten annotation stripping to H/M/L-### ids, and clamp narrow-terminal wrap width for estimate/render parity.
1 participant
Summary
rule.reasoninto assessment reasons, falling backimpact_descriptionto reason, and annotatingformat_security_messagewith rule id / matched paths (Fixes #408).Fixes #409).H/M/L-###, and clamp narrow-terminal wrap width.Change Type
Scope
User-visible Changes
Compatibility
Testing
cargo test -p aish-security assess_sandbox_result_exposes_rule_reason_when_description_missingcargo test -p aish-tools test_bash_tool_preflight_sandbox_hit_uses_rule_reason_without_descriptioncargo test -p aish-tools test_bash_tool_preflight_sandbox_hit_falls_back_to_rule_identitycargo test -p aish-shell security_panelrm /etc/...path and confirm Block text shows rule reason + H-001/paths instead of the generic HIGH sentence🤖 AI-assisted · lightly tested (unit/integration); needs local UI confirmation screenshots before merge
Summary by CodeRabbit