Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions crates/tui/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5525,12 +5525,6 @@ pub fn active_provider_has_config_api_key(config: &Config) -> bool {
// active_provider_has_env_api_key.
return crate::oauth::auth_file_path().exists();
}
if matches!(provider, ApiProvider::Huggingface)
&& std::env::var("HF_TOKEN").is_ok_and(|k| !k.trim().is_empty())
{
return true;
}

if config
.provider_config_string_with_runtime_fallback(provider, |entry| entry.api_key.clone())
.is_some_and(|k| !k.trim().is_empty() && k != API_KEYRING_SENTINEL)
Expand Down Expand Up @@ -5736,6 +5730,17 @@ fn provider_config_table_name(provider: ApiProvider) -> Result<String> {
}

fn provider_env_api_key(provider: ApiProvider) -> Option<String> {
if provider == ApiProvider::Huggingface {
return std::env::var("HUGGINGFACE_API_KEY")
.ok()
.filter(|value| !value.trim().is_empty())
.or_else(|| {
std::env::var("HF_TOKEN")
.ok()
.filter(|value| !value.trim().is_empty())
});
}

provider.env_vars().iter().find_map(|var| {
std::env::var(var)
.ok()
Expand Down
30 changes: 24 additions & 6 deletions crates/tui/src/tools/verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1122,8 +1122,28 @@ fn char_boundary_index(text: &str, max_chars: usize) -> usize {
mod tests {
use super::*;
use crate::tools::shell::ShellStatus;
use std::time::Duration;
use tempfile::tempdir;

const BACKGROUND_COMPLETION_WAIT_MS: u64 = 30_000;

fn wait_for_completed_shell(
manager: &mut crate::tools::shell::ShellManager,
task_id: &str,
) -> crate::tools::shell::ShellResult {
let deadline = Instant::now() + Duration::from_millis(BACKGROUND_COMPLETION_WAIT_MS);

loop {
let result = manager
.get_output(task_id, true, 1_000)
.expect("background output");
if result.status != ShellStatus::Running || Instant::now() >= deadline {
return result;
}
std::thread::sleep(Duration::from_millis(50));
}
}

#[test]
fn run_verifiers_requires_user_approval() {
let tool = RunVerifiersTool;
Expand Down Expand Up @@ -1316,12 +1336,10 @@ mod tests {
Some("nonblocking")
);

let output = ctx
.shell_manager
.lock()
.expect("shell manager")
.get_output(task_id, true, 10_000)
.expect("background output");
let output = wait_for_completed_shell(
&mut ctx.shell_manager.lock().expect("shell manager"),
task_id,
);
assert_eq!(output.status, ShellStatus::Completed);
assert!(
output.stdout.contains("rustc"),
Expand Down
114 changes: 114 additions & 0 deletions crates/tui/src/tui/approval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use crate::localization::Locale;
use crate::sandbox::SandboxPolicy;
use crate::tui::views::{ModalKind, ModalView, ViewAction, ViewEvent};
use crate::tui::widgets::{ApprovalWidget, ElevationWidget, Renderable};
use codewhale_config::ToolAskRule;
use crossterm::event::{KeyCode, KeyEvent};
use serde_json::Value;
use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -138,6 +139,8 @@ pub struct ApprovalRequest {
/// Displayed in the approval view so users understand *why* the change
/// is being made before reviewing *what* will change.
pub intent_summary: Option<String>,
/// Ask-only persistent rules that can be saved with the approval.
pub persistent_ask_rules: Vec<ToolAskRule>,
}

/// Key approval details rendered prominently in the approval card.
Expand Down Expand Up @@ -193,6 +196,7 @@ impl ApprovalRequest {
Some(summary.to_string())
}
}),
persistent_ask_rules: build_persistent_ask_rules(tool_name, params),
}
}

Expand All @@ -218,6 +222,22 @@ impl ApprovalRequest {
}
}

#[must_use]
pub fn can_save_ask_rule(&self) -> bool {
!self.persistent_ask_rules.is_empty()
}

#[must_use]
pub fn ask_rule_preview(&self) -> Option<String> {
if self.persistent_ask_rules.is_empty() {
return None;
}
let permissions = codewhale_config::PermissionsToml {
rules: self.persistent_ask_rules.clone(),
};
toml::to_string_pretty(&permissions).ok()
}

/// Extract the most important params for the approval card.
#[must_use]
pub fn prominent_detail_items(&self, locale: Locale) -> Vec<ApprovalDetail> {
Expand All @@ -231,6 +251,22 @@ impl ApprovalRequest {
}
}

#[must_use]
fn build_persistent_ask_rules(tool_name: &str, params: &Value) -> Vec<ToolAskRule> {
if tool_name != "exec_shell" {
return Vec::new();
}
let Some(command) = params
.get("command")
.and_then(Value::as_str)
.map(str::trim)
.filter(|command| !command.is_empty())
else {
return Vec::new();
};
vec![ToolAskRule::exec_shell(command)]
}

/// Get the category for a tool by name
pub fn get_tool_category(name: &str) -> ToolCategory {
if matches!(name, "write_file" | "edit_file" | "apply_patch") {
Expand Down Expand Up @@ -888,13 +924,23 @@ impl ApprovalView {
}

fn emit_decision(&self, decision: ReviewDecision, timed_out: bool) -> ViewAction {
self.emit_decision_with_rules(decision, timed_out, Vec::new())
}

fn emit_decision_with_rules(
&self,
decision: ReviewDecision,
timed_out: bool,
persistent_ask_rules: Vec<ToolAskRule>,
) -> ViewAction {
ViewAction::EmitAndClose(ViewEvent::ApprovalDecision {
tool_id: self.request.id.clone(),
tool_name: self.request.tool_name.clone(),
decision,
timed_out,
approval_key: self.request.approval_key.clone(),
approval_grouping_key: self.request.approval_grouping_key.clone(),
persistent_ask_rules,
})
}

Expand Down Expand Up @@ -947,6 +993,12 @@ impl ModalView for ApprovalView {
KeyCode::Char('a') | KeyCode::Char('A') | KeyCode::Char('2') => {
self.commit_option(ApprovalOption::ApproveAlways)
}
KeyCode::Char('s') | KeyCode::Char('S') if self.request.can_save_ask_rule() => self
.emit_decision_with_rules(
ReviewDecision::Approved,
false,
self.request.persistent_ask_rules.clone(),
),
KeyCode::Char('n')
| KeyCode::Char('N')
| KeyCode::Char('d')
Expand Down Expand Up @@ -1261,6 +1313,16 @@ mod tests {
)
}

fn shell_request() -> ApprovalRequest {
ApprovalRequest::new(
"test-id",
"exec_shell",
"Run a shell command",
&json!({"command": "cargo test --workspace"}),
"tool:exec_shell",
)
}

// ========================================================================
// Tool Category Tests
// ========================================================================
Expand Down Expand Up @@ -1549,6 +1611,28 @@ mod tests {
assert_eq!(view.risk(), RiskLevel::Benign);
}

#[test]
fn exec_shell_request_builds_ask_rule_preview() {
let request = shell_request();

assert_eq!(
request.persistent_ask_rules,
vec![ToolAskRule::exec_shell("cargo test --workspace")]
);
let preview = request.ask_rule_preview().expect("preview");
assert!(preview.contains("[[rules]]"));
assert!(preview.contains("tool = \"exec_shell\""));
assert!(preview.contains("command = \"cargo test --workspace\""));
}

#[test]
fn non_shell_request_has_no_persistent_ask_rules() {
let request = destructive_request();

assert!(request.persistent_ask_rules.is_empty());
assert_eq!(request.ask_rule_preview(), None);
}

#[test]
fn tab_toggles_collapsed_card_so_transcript_stays_visible() {
// Regression for PR #1455 / @tiger-dog: the approval modal
Expand Down Expand Up @@ -1609,6 +1693,36 @@ mod tests {
}
}

#[test]
fn save_ask_rule_shortcut_approves_once_with_rule() {
let mut view = ApprovalView::new(shell_request());

let action = view.handle_key(create_key_event(KeyCode::Char('s')));
let ViewAction::EmitAndClose(ViewEvent::ApprovalDecision {
decision,
persistent_ask_rules,
..
}) = action
else {
panic!("expected approval decision");
};

assert_eq!(decision, ReviewDecision::Approved);
assert_eq!(
persistent_ask_rules,
vec![ToolAskRule::exec_shell("cargo test --workspace")]
);
}

#[test]
fn save_ask_rule_shortcut_is_ignored_without_rule() {
let mut view = ApprovalView::new(benign_request());

let action = view.handle_key(create_key_event(KeyCode::Char('s')));

assert!(matches!(action, ViewAction::None));
}

#[test]
fn benign_one_key_approves_via_numeric_pad() {
let mut view = ApprovalView::new(benign_request());
Expand Down
43 changes: 43 additions & 0 deletions crates/tui/src/tui/ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8664,17 +8664,20 @@ async fn handle_view_events(
timed_out,
approval_key,
approval_grouping_key,
persistent_ask_rules,
} => {
apply_approval_decision(
app,
engine_handle,
config,
ApprovalDecisionEvent {
tool_id,
tool_name,
decision,
timed_out,
approval_key,
approval_grouping_key,
persistent_ask_rules,
},
)
.await;
Expand Down Expand Up @@ -9014,11 +9017,13 @@ struct ApprovalDecisionEvent {
timed_out: bool,
approval_key: String,
approval_grouping_key: String,
persistent_ask_rules: Vec<codewhale_config::ToolAskRule>,
}

async fn apply_approval_decision(
app: &mut App,
engine_handle: &mut EngineHandle,
config: &mut Config,
event: ApprovalDecisionEvent,
) {
if event.decision == ReviewDecision::ApprovedForSession {
Expand All @@ -9031,6 +9036,15 @@ async fn apply_approval_decision(
.insert(event.approval_grouping_key.clone());
}

if matches!(
event.decision,
ReviewDecision::Approved | ReviewDecision::ApprovedForSession
) && !event.persistent_ask_rules.is_empty()
&& !event.timed_out
{
persist_ask_rules_from_approval(app, config, &event.persistent_ask_rules);
}
Comment on lines +9039 to +9046

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The condition checks ReviewDecision::ApprovedForSession to decide whether to persist the ask rules. However, 'Approved for Session' is intended to be a temporary, session-only approval. Persisting rules to permissions.toml under this decision contradicts the user's choice of a session-only approval. We should only persist rules when the decision is permanently Approved.

    if event.decision == ReviewDecision::Approved
        && !event.persistent_ask_rules.is_empty()
        && !event.timed_out
    {
        persist_ask_rules_from_approval(app, config, &event.persistent_ask_rules);
    }


match event.decision {
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => {
let _ = engine_handle.approve_tool_call(event.tool_id).await;
Expand All @@ -9053,6 +9067,35 @@ async fn apply_approval_decision(
}
}

fn persist_ask_rules_from_approval(
app: &mut App,
config: &mut Config,
rules: &[codewhale_config::ToolAskRule],
) {
match codewhale_config::ConfigStore::load(app.config_path.clone()).and_then(|mut store| {
let added = store.append_ask_rules(rules)?;
let permissions_path = store.permissions_path();
config.exec_policy_engine = store.exec_policy_engine();
Ok((added, permissions_path))
}) {
Ok((added, path)) if added > 0 => {
app.status_message = Some(format!(
"Saved {added} ask permission rule(s) to {}",
path.display()
));
}
Ok((_added, path)) => {
app.status_message = Some(format!(
"Ask permission rule already saved in {}",
path.display()
));
}
Err(err) => {
app.status_message = Some(format!("Failed to save ask permission rule: {err:#}"));
}
Comment on lines +9081 to +9095

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The status messages set in persist_ask_rules_from_approval are hardcoded in English. Since the TUI supports multiple locales (such as Chinese/ZhHans), these user-facing status messages should be localized using the project's established internationalization/localization patterns rather than being hardcoded.

}
}

fn mark_active_turn_cancelled_locally(app: &mut App) {
// #2739: every local cancel surface (Esc, Ctrl+C, approval abort, paused
// command abort) must snapshot before it clears turn state. Otherwise
Expand Down
Loading
Loading