Skip to content

feat: add secret detection for AI input with confirmation gate#196

Open
jexShain wants to merge 1 commit into
AI-Shell-Team:rustfrom
jexShain:rust-secinput
Open

feat: add secret detection for AI input with confirmation gate#196
jexShain wants to merge 1 commit into
AI-Shell-Team:rustfrom
jexShain:rust-secinput

Conversation

@jexShain
Copy link
Copy Markdown
Collaborator

@jexShain jexShain commented May 21, 2026

Summary

  • Add aish-security::secret module with built-in patterns for detecting API keys (OpenAI, Anthropic, AWS, Google, GitHub, Stripe, Slack, Fireworks), JWT tokens, and URL-embedded passwords in AI input text
  • Users can define custom patterns via secret_patterns in config/security_policy.yaml
  • When secrets are detected, a confirmation dialog prompts the user before sending input to the LLM — works in both local TUI and SSH sessions
  • Fix SecurityManager in app.rs to load policy from config (via load_policy()) instead of using SecurityPolicy::default(), so user-defined secret_patterns are applied
  • Harden scanner RegexSet/compiled Vec index alignment with empty() fallback instead of mismatched ["^$"] pattern

Test plan

  • cargo check -p aish-shell passes
  • cargo test -p aish-security passes (4/4 tests)
  • Manual test: type an OpenAI API key in aish prompt, verify confirmation dialog appears
  • Manual test: SSH into remote host, type a secret in AI input, verify SSH confirmation dialog
  • Manual test: add custom pattern in security_policy.yaml, restart aish, verify it's detected

Summary by CodeRabbit

  • New Features

    • Interactive secret detection for AI inputs with options to Redact, Allow, or Abort before sending.
    • Secrets are replaced with stable placeholders, restored automatically when commands execute, and redacted in command output.
    • Secret scanning integrated across AI prompts, shell interactions, and tooling; vault-based placeholder management persists during execution.
    • Localized user prompts added for English and Chinese; confirmation dialog labels are customizable.
  • Documentation

    • Added commented guidance for optional secret-pattern configuration with examples.

Review Change Stack

@github-actions
Copy link
Copy Markdown
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

@github-actions
Copy link
Copy Markdown
Contributor

This pull request description looks incomplete. Please update the missing sections below before review.

Missing items:

  • User-visible Changes
  • Compatibility
  • Testing
  • Change Type
  • Scope

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds regex-based secret detection (builtin + custom patterns), integrates a SecretScanner into SecurityManager and policy loading, gates AI inputs with an interactive terminal confirmation prompt, wires the check into PTY and REPL flows (with redaction/restore via an in-memory vault), and supplies localization and policy documentation.

Changes

Secret Detection and Interactive Confirmation

Layer / File(s) Summary
Secret pattern definitions and scanning engine
crates/aish-security/src/secret/patterns.rs, crates/aish-security/src/secret/scanner.rs, crates/aish-security/src/secret/mod.rs, crates/aish-security/src/secret/vault.rs
Defines SecretType, SecretMatch, SecretPattern, CustomPattern, builtin_patterns(), SecretVault (redact/restore/redact_output), and SecretScanner that compiles a RegexSet and per-pattern Regexes, extracts matches, sorts and deduplicates overlapping spans, with unit tests.
Security manager and policy integration
crates/aish-security/src/lib.rs, crates/aish-security/Cargo.toml, crates/aish-security/src/policy.rs, crates/aish-security/src/manager.rs, crates/aish-security/src/decision.rs, crates/aish-security/src/sandbox/degraded.rs
Adds pub mod secret, extends SecurityPolicy with secret_patterns loaded from YAML, embeds SecretScanner in SecurityManager, and adds check_ai_input returning allow/confirm with detected_secrets; updates SecurityAnalysis defaults and tests.
Shell REPL and PTY confirmation UI
crates/aish-shell/src/app.rs, crates/aish-pty/src/lib.rs, crates/aish-pty/src/persistent.rs, crates/aish-shell/src/tui.rs, crates/aish-pty/Cargo.toml
Builds a secret_check_closure from the scanner, calls check_ai_input before LLM invocation and shows a secret dialog when required, passes an optional secret_check and SecretVault into PersistentPty::send_command_interactive, restores placeholders before executing AI-generated commands, and implements interactive rendering and raw-stdin input handling for Redact/Allow/Abort.
Tool preflight and output redaction
crates/aish-tools/src/bash.rs
Adds VaultSlot and secret_vault to BashTool, restores placeholders before security preflight and execution, and redacts secret values in captured tool output before returning it to the AI.
Documentation and localization
config/security_policy.yaml, crates/aish-i18n/locales/en-US.yaml, crates/aish-i18n/locales/zh-CN.yaml
Policy YAML documents optional secret_patterns configuration with an example; English and Chinese localization strings added for the secret-detection confirmation UI, option labels, and status messages.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • AI-Shell-Team/aish#157: Modifies PersistentPty::send_command_interactive and dossier/AI trigger handling; overlaps integration points with this secret-check gating.
  • AI-Shell-Team/aish#152: Related PTY/AI session handling and callback threading touching persistent.rs and shell integration.
  • AI-Shell-Team/aish#139: Also edits PersistentPty::send_command_interactive and interactive behavior; likely to conflict at the same code path.

Suggested labels

security, config, tests

"I nibble on the regex thread by night,
Nose twitch bright at patterns out of sight.
I hide your keys with gentle vault and art,
A redact, allow, or abort — I play my part.
Hop safe, dear user, shell guard at heart."

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add secret detection for AI input with confirmation gate' clearly and accurately summarizes the main change: adding secret detection with a confirmation dialog gate for AI input.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 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 2374-2378: In the match arm that handles 0x03 (the unsafe block
calling libc::write), fix the off-by-one by passing the correct byte count for
the buffer (use the actual length of b"^C\r\n" instead of 5); locate the unsafe
block in persistent.rs where libc::write is called and replace the hardcoded 5
with the correct length (e.g., use b"^C\r\n".len() or the literal 4) so the
write does not read past the buffer.
- Around line 1248-1254: The code block around the secret detection gate
(references: secret_check, checker, question, show_secret_confirmation,
libc::STDIN_FILENO) is misformatted and failing `cargo fmt --check`; run `cargo
fmt` (or apply rustfmt) to reformat the block so spacing/indentation matches
project style—specifically ensure the nested if-let and function call arguments
are properly aligned and indented as rustfmt would produce (preserve the logic:
if let Some(ref checker) = secret_check { if let Some(warning) =
checker(&question) { let confirmed = show_secret_confirmation(&warning,
libc::STDIN_FILENO); if !confirmed { ... } } }).

In `@crates/aish-security/src/policy.rs`:
- Around line 454-460: The current construction of secret_patterns silently
drops parse errors because seq.iter().filter_map(|v|
serde_yaml::from_value(v.clone()).ok()) ignores errors; update this to collect
both successful CustomPattern values and parse failures and emit diagnostics
instead of discarding them: replace the filter_map(.ok()) usage in the
secret_patterns assembly (the let secret_patterns:
Vec<crate::secret::CustomPattern> block that uses mapping_get and
Value::as_sequence) with logic that attempts serde_yaml::from_value for each
entry, pushes successful parses into secret_patterns, and for failures pushes a
warning/entry into the module’s validation_issues (or calls the existing
diagnostics emitter) including the original value and the parse error message so
malformed patterns are reported.

In `@crates/aish-security/src/secret/patterns.rs`:
- Around line 37-39: The file fails rustfmt causing CI errors; run rustfmt/cargo
fmt on the crate to fix formatting mismatches around the Pattern struct methods
(e.g., format_reason) and other affected blocks (lines around the methods
starting at format_reason, the block at 58-74, and the block at 104-105).
Reformat the file by running cargo fmt in the workspace/crate so the code
conforms to rustfmt and the CI will pass.

In `@crates/aish-security/src/secret/scanner.rs`:
- Line 163: The test currently hardcodes a JWT-looking literal in the string
assigned to input, which trips secret scanners; change the test to build the
token at runtime instead: construct the token by joining safe parts (e.g.,
base64-encoding small header and payload strings and a benign signature or
concatenating static safe substrings) and then assemble the overall input string
(the variable named input) so the value still matches expected format but no
secret-like literal appears in the source; update the test that uses input (look
for the test function referencing input in secret::scanner.rs) to use this
dynamically built token instead.

In `@crates/aish-shell/src/app.rs`:
- Around line 999-1001: The file has formatting issues: reflow the
secret_check_closure assignment (the call to Self::build_secret_check_closure
and its argument security_manager.secret_scanner().clone()), the
detected_secrets mapping (and its reasons formatting) around the
detected_secrets variable, and the wrapping of the argument to
pty.send_command_interactive so they conform to rustfmt style; run cargo fmt and
commit the resulting changes to fix all formatting mismatches.

In `@crates/aish-shell/src/tui.rs`:
- Around line 79-83: The function show_confirmation_dialog has formatting issues
and redundant .to_string() calls; update the signature/spacing to match rustfmt
(run cargo fmt) and remove the unnecessary conversions by passing yes_label and
no_label directly into DialogOption::new (which accepts impl Into<String>)
instead of yes_label.to_string() and no_label.to_string(); ensure the Vec
options uses DialogOption::new("yes", yes_label) and DialogOption::new("no",
no_label) so formatting and types satisfy cargo fmt and the compiler.
🪄 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: e760bce0-2570-47d1-ace8-0221ea17ec48

📥 Commits

Reviewing files that changed from the base of the PR and between 5606ddd and 8d307c6.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (15)
  • config/security_policy.yaml
  • crates/aish-i18n/locales/en-US.yaml
  • crates/aish-i18n/locales/zh-CN.yaml
  • crates/aish-pty/src/persistent.rs
  • crates/aish-security/Cargo.toml
  • crates/aish-security/src/decision.rs
  • crates/aish-security/src/lib.rs
  • crates/aish-security/src/manager.rs
  • crates/aish-security/src/policy.rs
  • crates/aish-security/src/sandbox/degraded.rs
  • crates/aish-security/src/secret/mod.rs
  • crates/aish-security/src/secret/patterns.rs
  • crates/aish-security/src/secret/scanner.rs
  • crates/aish-shell/src/app.rs
  • crates/aish-shell/src/tui.rs

Comment thread crates/aish-pty/src/persistent.rs Outdated
Comment thread crates/aish-pty/src/persistent.rs Outdated
Comment thread crates/aish-security/src/policy.rs
Comment thread crates/aish-security/src/secret/patterns.rs
Comment thread crates/aish-security/src/secret/scanner.rs Outdated
Comment thread crates/aish-shell/src/app.rs Outdated
Comment thread crates/aish-shell/src/tui.rs Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
crates/aish-security/src/secret/scanner.rs (1)

157-157: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid committing JWT-looking literal in tests.

This test embeds a JWT-shaped string that can trigger secret scanners. Building the token dynamically keeps coverage without false-positive alerts.

🔧 Suggested dynamic token construction
-        let input = "key=sk-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuv token=eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dozjgNryP4J3jVmNHl0w5N_XgL0n3I9PlFUP0THsR8U";
+        let jwt = format!("eyJ{}.eyJ{}.doz{}", "hbGciOiJIUzI1NiJ9", "zdWIiOiIxMjM0NTY3ODkwIn0", "jgNryP4J3jVmNHl0w5N_XgL0n3I9PlFUP0THsR8U");
+        let input = format!("key=sk-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuv token={}", jwt);
🤖 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/secret/scanner.rs` at line 157, The test currently
assigns a JWT-looking literal to the variable `input` which triggers secret
scanners; modify the test in `crates/aish-security/src/secret/scanner.rs` to
construct the token dynamically (e.g., build header, payload, and signature
parts at runtime using benign strings and base64/url-safe encoding or simple
concatenation) and then set `input` to "key=sk-... token=" + constructed_token
so the test coverage remains but no hardcoded JWT-like literal is committed;
locate the assignment to `input` in the test and replace it with the runtime
construction logic using the same variable name.
🤖 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.

Duplicate comments:
In `@crates/aish-security/src/secret/scanner.rs`:
- Line 157: The test currently assigns a JWT-looking literal to the variable
`input` which triggers secret scanners; modify the test in
`crates/aish-security/src/secret/scanner.rs` to construct the token dynamically
(e.g., build header, payload, and signature parts at runtime using benign
strings and base64/url-safe encoding or simple concatenation) and then set
`input` to "key=sk-... token=" + constructed_token so the test coverage remains
but no hardcoded JWT-like literal is committed; locate the assignment to `input`
in the test and replace it with the runtime construction logic using the same
variable name.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e97a252c-8e99-486c-b94a-65e511541d56

📥 Commits

Reviewing files that changed from the base of the PR and between 8d307c6 and 2e90479.

📒 Files selected for processing (4)
  • crates/aish-pty/src/persistent.rs
  • crates/aish-security/src/secret/scanner.rs
  • crates/aish-shell/src/app.rs
  • crates/aish-shell/src/tui.rs

@github-actions github-actions Bot added size: XL and removed size: L labels May 21, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/aish-security/src/secret/patterns.rs (1)

13-17: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Derive Default for SecretType to resolve clippy::derivable-impls.

crates/aish-security/src/secret/patterns.rs defines a manual impl Default for SecretType returning SecretType::ApiKey while the enum has no Default derive; this is the pattern clippy::derivable-impls flags (and the repo runs clippy with -D warnings). Replace the manual impl with #[derive(Default)] and mark the default variant.

Proposed fix
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)]
 #[serde(rename_all = "snake_case")]
 pub enum SecretType {
+    #[default]
     ApiKey,
     Token,
     Password,
     Credential,
 }
-
-impl Default for SecretType {
-    fn default() -> Self {
-        Self::ApiKey
-    }
-}
🤖 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/secret/patterns.rs` around lines 13 - 17, The manual
impl Default for SecretType (fn default() -> Self { Self::ApiKey }) should be
replaced by deriving Default on the enum: add #[derive(Default)] to the
SecretType declaration, annotate the ApiKey variant with #[default], and remove
the existing impl Default block so clippy's clippy::derivable-impls warning is
resolved; locate the SecretType enum and the impl Default in patterns.rs to make
these changes.
🤖 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 913-935: The restore logic for secret placeholders is only applied
to one AI-command execution path (it builds cmd_restored via
secret_vault.lock().restore and writes an info message) but the pending-response
branch still uses the original cmd and close_unclosed_heredoc(cmd), allowing
unresolved placeholders to be executed; fix by factoring the restoration into a
shared helper (or reuse the existing cmd_restored logic) so both execution
branches use the restored command variable (e.g., use cmd_restored instead of
cmd and call close_unclosed_heredoc(cmd_restored) where the pending-response
execution happens), referencing the secret_vault, restore(...) result,
cmd_restored variable and close_unclosed_heredoc to locate and update the code
paths.
- Around line 1306-1335: The current branch only redacts when secret_vault is
Some, so if the user chose Redact but secret_vault is None the code silently
sends the original question; change the logic around secret_vault so that when
the user selected Redact and secret_vault.is_none() you explicitly abort or
return an error (or refuse to send) and emit a clear message to the user instead
of proceeding. Locate the block that references secret_vault, vault.lock(),
redact(...), question and detected_secrets and add a guard that handles None by
reporting the failure (and not sending the original question) or by forcing safe
fallback behavior (e.g., canceling the send or prompting to configure a vault)
before any write/return occurs.

In `@crates/aish-security/src/secret/vault.rs`:
- Around line 7-11: The SecretEntry struct contains an unused field secret_type
which triggers clippy warnings; either remove the secret_type field from
SecretEntry or use it where entries are constructed/consumed. Locate the
SecretEntry definition and the places that create or match on SecretEntry
(references to SecretEntry and its fields placeholder and original) and either
delete the secret_type field from the struct and update all constructors/usages
to stop supplying or expecting it, or thread the SecretType through
creation/consumption sites so secret_type is actually read (e.g., when building,
matching, or serializing SecretEntry) to eliminate the dead-field warning.

In `@crates/aish-shell/src/app.rs`:
- Around line 1317-1329: The code in the SecretDialogChoice::Redact branch
unwraps decision.analysis.detected_secrets which can panic; change it to safely
handle None (e.g., if let Some(secrets) = &decision.analysis.detected_secrets {
... } else { log or print a warning and skip redaction/leave question unchanged
}). Ensure the logic still calls
self.secret_vault.lock().unwrap().redact(&secrets, &question) only when secrets
is Some, updates count via self.secret_vault.lock().unwrap().len(), prints the
redaction message, and assigns question = redacted; otherwise avoid calling
redact and don’t unwrap the Option.

---

Outside diff comments:
In `@crates/aish-security/src/secret/patterns.rs`:
- Around line 13-17: The manual impl Default for SecretType (fn default() ->
Self { Self::ApiKey }) should be replaced by deriving Default on the enum: add
#[derive(Default)] to the SecretType declaration, annotate the ApiKey variant
with #[default], and remove the existing impl Default block so clippy's
clippy::derivable-impls warning is resolved; locate the SecretType enum and the
impl Default in patterns.rs to make these changes.
🪄 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: a513f25a-1379-48d4-9995-7c5875bcbb21

📥 Commits

Reviewing files that changed from the base of the PR and between 2e90479 and 095bd94.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (11)
  • crates/aish-i18n/locales/en-US.yaml
  • crates/aish-i18n/locales/zh-CN.yaml
  • crates/aish-pty/Cargo.toml
  • crates/aish-pty/src/lib.rs
  • crates/aish-pty/src/persistent.rs
  • crates/aish-security/src/lib.rs
  • crates/aish-security/src/secret/mod.rs
  • crates/aish-security/src/secret/patterns.rs
  • crates/aish-security/src/secret/vault.rs
  • crates/aish-shell/src/app.rs
  • crates/aish-shell/src/tui.rs
✅ Files skipped from review due to trivial changes (1)
  • crates/aish-pty/Cargo.toml

Comment thread crates/aish-pty/src/persistent.rs
Comment thread crates/aish-pty/src/persistent.rs
Comment thread crates/aish-security/src/secret/vault.rs
Comment thread crates/aish-shell/src/app.rs Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
crates/aish-security/src/secret/vault.rs (2)

146-146: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Replace sort_by with sort_by_key to fix clippy warning.

Clippy flags unnecessary_sort_by when sorting by a key projection. Use sort_by_key with Reverse for clarity and to pass CI.

🐛 Proposed fix
-        replacements.sort_by(|a, b| b.1.cmp(&a.1));
+        replacements.sort_by_key(|r| std::cmp::Reverse(r.1));
🤖 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/secret/vault.rs` at line 146, Clippy warns about
using sort_by with a direct key comparison in replacements.sort_by(|a, b|
b.1.cmp(&a.1)); — replace that call with sort_by_key and Reverse to express the
descending key sort clearly: use replacements.sort_by_key(|t|
std::cmp::Reverse(t.1)); this removes the unnecessary_sort_by lint while keeping
the same sort order (reference the replacements.sort_by call and use
std::cmp::Reverse and sort_by_key).

168-168: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Replace sort_by with sort_by_key to fix clippy warning.

Clippy flags unnecessary_sort_by when sorting by a key projection. Use sort_by_key with Reverse for clarity and to pass CI.

🐛 Proposed fix
-        entries.sort_by(|a, b| b.placeholder.len().cmp(&a.placeholder.len()));
+        entries.sort_by_key(|e| std::cmp::Reverse(e.placeholder.len()));
🤖 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/secret/vault.rs` at line 168, Clippy warns about
using sort_by where a key projection suffices; change the call to use
sort_by_key with Reverse on the key so the sort remains descending: replace the
entries.sort_by(...) invocation that compares b.placeholder.len() to
a.placeholder.len() with entries.sort_by_key(|e| Reverse(e.placeholder.len()))
(reference symbols: entries.sort_by, placeholder.len(), sort_by_key, Reverse).
♻️ Duplicate comments (1)
crates/aish-security/src/secret/vault.rs (1)

7-11: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Remove unused secret_type field to fix build failure.

This field is stored (line 136) but never read, causing clippy dead_code errors that fail CI with -D warnings. Either remove the field entirely or use it where entries are consumed (e.g., in restore or for logging/debugging).

🐛 Proposed fix: remove the field
 struct SecretEntry {
     placeholder: String,
     original: String,
-    secret_type: SecretType,
 }
                 self.entries.insert(
                     value.to_string(),
                     SecretEntry {
                         placeholder: placeholder.clone(),
                         original: value.to_string(),
-                        secret_type: sm.secret_type,
                     },
                 );
🤖 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/secret/vault.rs` around lines 7 - 11, The
SecretEntry struct contains an unused field secret_type causing dead_code
warnings; remove the secret_type field from the SecretEntry definition and any
code that constructs or assigns to that field (e.g., the place where entries are
pushed/stored around the code that corresponds to line 136), and update any
pattern matches or constructors referencing SecretEntry::secret_type so they use
only placeholder and original (or add use of the type if you prefer to keep it);
ensure all places creating SecretEntry (constructors/vec pushes) are updated to
the new two-field shape to fix the build.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/aish-security/src/secret/vault.rs`:
- Line 146: Clippy warns about using sort_by with a direct key comparison in
replacements.sort_by(|a, b| b.1.cmp(&a.1)); — replace that call with sort_by_key
and Reverse to express the descending key sort clearly: use
replacements.sort_by_key(|t| std::cmp::Reverse(t.1)); this removes the
unnecessary_sort_by lint while keeping the same sort order (reference the
replacements.sort_by call and use std::cmp::Reverse and sort_by_key).
- Line 168: Clippy warns about using sort_by where a key projection suffices;
change the call to use sort_by_key with Reverse on the key so the sort remains
descending: replace the entries.sort_by(...) invocation that compares
b.placeholder.len() to a.placeholder.len() with entries.sort_by_key(|e|
Reverse(e.placeholder.len())) (reference symbols: entries.sort_by,
placeholder.len(), sort_by_key, Reverse).

---

Duplicate comments:
In `@crates/aish-security/src/secret/vault.rs`:
- Around line 7-11: The SecretEntry struct contains an unused field secret_type
causing dead_code warnings; remove the secret_type field from the SecretEntry
definition and any code that constructs or assigns to that field (e.g., the
place where entries are pushed/stored around the code that corresponds to line
136), and update any pattern matches or constructors referencing
SecretEntry::secret_type so they use only placeholder and original (or add use
of the type if you prefer to keep it); ensure all places creating SecretEntry
(constructors/vec pushes) are updated to the new two-field shape to fix the
build.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a595ca60-a3dd-46bc-b206-b10fccf6cac1

📥 Commits

Reviewing files that changed from the base of the PR and between 095bd94 and 8c9d819.

📒 Files selected for processing (2)
  • crates/aish-security/src/secret/vault.rs
  • crates/aish-shell/src/app.rs

@jexShain jexShain force-pushed the rust-secinput branch 2 times, most recently from 2554047 to 9c658d1 Compare May 21, 2026 09:25
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/aish-tools/src/bash.rs`:
- Line 12: The import line using aish_security (symbols load_policy,
secret::SecretVault, SecurityDecision, SecurityManager, SecurityRequest) is not
formatted according to rustfmt; run `cargo fmt --all` (or reformat the use
statement to match rustfmt style) so the use declaration is split/ordered as
rustfmt expects and CI passes.
🪄 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: 49e8e897-2e85-4436-a4b3-ce95fccabf7c

📥 Commits

Reviewing files that changed from the base of the PR and between 8c9d819 and 2554047.

📒 Files selected for processing (3)
  • crates/aish-security/src/secret/vault.rs
  • crates/aish-shell/src/app.rs
  • crates/aish-tools/src/bash.rs

Comment thread crates/aish-tools/src/bash.rs Outdated
@jexShain jexShain force-pushed the rust-secinput branch 2 times, most recently from 3f78f04 to ee7c869 Compare May 21, 2026 13:14
Add regex-based secret scanner (API keys, JWTs, passwords) and an
in-memory SecretVault that redacts detected secrets to semantic
placeholders ($SECRET_*) before sending to the LLM, then restores
them for command execution and re-redacts command output.

- SecretScanner with RegexSet for efficient multi-pattern matching
- SecretVault with redact/restore/redact_output round-trip
- Three-option dialog: redact to env vars, send plaintext, or abort
- Both TUI (inquire) and SSH (raw terminal I/O) dialog paths
- BashTool integration: restore before execution, redact output before
  returning to AI, restore in preflight for accurate security checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant