dgf1988/codewhale
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

fix(tui): revert v0.8.38 /model picker rework and restore approval grouping

Browse Source 
The v0.8.38 upgrade dramatically changed two user-visible behaviors that
were not intended as regressions:

- The /model picker was reworked (#1201/#1632) to make a blocking network
  fetch on open and replace the curated tier list with the raw provider
  catalog. Revert model_picker.rs and the OpenModelPicker handler to the
  v0.8.37 instant curated picker. The /models command still lists the live
  catalog.

- #1617 rekeyed the approval cache to an exact full-argument fingerprint,
  which also dropped the v0.8.37 arity-aware command-family grouping for
  "approve for session". Reintroduce build_approval_grouping_key (the lossy
  v0.8.37 logic) for approvals while keeping the exact key for denials, so
  denying one call no longer over-blocks later differing calls.

https://claude.ai/code/session_01NDuRxM56o17SE7SDLcTFYT
This commit is contained in:
Claude
2026-05-17 04:03:13 +00:00
committed by Hunter Bown
parent f0a7e15d30
commit 81bc2da069
10 changed files with 252 additions and 112 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+10
View File
@@ -12,6 +12,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **Feishu/Lark bridge startup order is guarded.** The bridge now keeps
`ThreadStore` initialized before startup opens persisted thread state, with a
regression test to prevent moving it below its first use.
- **`/model` picker opens instantly with the curated list again.** Reverted
the v0.8.38 live-catalog rework: the picker no longer makes a blocking
network call on open and once again shows the curated `auto` /
`deepseek-v4-pro` / `deepseek-v4-flash` rows. The `/models` command still
lists the live provider catalog.
- **"Approve for session" groups by command family again.** Session approvals
are keyed by a lossy, arity-aware fingerprint once more, so approving
`cargo build` also covers `cargo build --release`. Denials keep the exact
per-call fingerprint from #1617, so denying one call no longer over-blocks
later, different calls to the same tool.
## [0.8.38] - 2026-05-15
crates/tui/CHANGELOG.md
+10
View File
@@ -12,6 +12,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **Feishu/Lark bridge startup order is guarded.** The bridge now keeps
`ThreadStore` initialized before startup opens persisted thread state, with a
regression test to prevent moving it below its first use.
- **`/model` picker opens instantly with the curated list again.** Reverted
the v0.8.38 live-catalog rework: the picker no longer makes a blocking
network call on open and once again shows the curated `auto` /
`deepseek-v4-pro` / `deepseek-v4-flash` rows. The `/models` command still
lists the live provider catalog.
- **"Approve for session" groups by command family again.** Session approvals
are keyed by a lossy, arity-aware fingerprint once more, so approving
`cargo build` also covers `cargo build --release`. Denials keep the exact
per-call fingerprint from #1617, so denying one call no longer over-blocks
later, different calls to the same tool.
## [0.8.38] - 2026-05-15
crates/tui/src/core/engine/turn_loop.rs
+7
View File
@@ -1597,6 +1597,12 @@ impl Engine {
&tool_input,
)
.0;
let approval_grouping_key =
crate::tools::approval_cache::build_approval_grouping_key(
&tool_name,
&tool_input,
)
.0;
let _ = self
.tx_event
.send(Event::ApprovalRequired {
@@ -1604,6 +1610,7 @@ impl Engine {
tool_name: tool_name.clone(),
description: plan.approval_description.clone(),
approval_key,
approval_grouping_key,
})
.await;
crates/tui/src/core/events.rs
+4 -1
View File
@@ -226,8 +226,11 @@ pub enum Event {
id: String,
tool_name: String,
description: String,
/// Fingerprint key for per‑call approval caching (§5.A).
/// Exact-argument fingerprint, used to scope *denials* (#1617).
approval_key: String,
/// Lossy / arity-aware fingerprint, used to scope *approvals* so an
/// "approve for session" covers later flag variants (v0.8.37).
approval_grouping_key: String,
},
/// Request user input for a tool call
crates/tui/src/runtime_threads.rs
+4
View File
@@ -4154,6 +4154,7 @@ mod tests {
.tx_event
.send(EngineEvent::ApprovalRequired {
approval_key: "test_key".to_string(),
approval_grouping_key: "test_key".to_string(),
id: "tool_stale".to_string(),
tool_name: "exec_command".to_string(),
description: "stale approval".to_string(),
@@ -4226,6 +4227,7 @@ mod tests {
.tx_event
.send(EngineEvent::ApprovalRequired {
approval_key: "key1".to_string(),
approval_grouping_key: "key1".to_string(),
id: "tool_external_allow".to_string(),
tool_name: "exec_command".to_string(),
description: "external allow".to_string(),
@@ -4302,6 +4304,7 @@ mod tests {
.tx_event
.send(EngineEvent::ApprovalRequired {
approval_key: "key2".to_string(),
approval_grouping_key: "key2".to_string(),
id: "tool_external_deny".to_string(),
tool_name: "exec_command".to_string(),
description: "external deny".to_string(),
@@ -4487,6 +4490,7 @@ mod tests {
.tx_event
.send(EngineEvent::ApprovalRequired {
approval_key: "key3".to_string(),
approval_grouping_key: "key3".to_string(),
id: "tool_remember".to_string(),
tool_name: "exec_command".to_string(),
description: "remember=true".to_string(),
crates/tui/src/tools/approval_cache.rs
+152 -7
View File
@@ -6,14 +6,31 @@
//! cache keys off a **call fingerprint** — a digest of the tool name and
//! the semantically‑relevant portion of its arguments.
//!
//! ## Fingerprint shape
//! ## Two fingerprint shapes
//!
//! | Tool | Key |
//! |---------------|------------------------------------------|
//! | file writes | `file:<tool_name>:<hash of args>` |
//! | shell tools | `shell:<tool_name>:<hash of args>` |
//! | `fetch_url` | `net:<hostname>` |
//! | everything else| `tool:<tool_name>:<hash of input>` |
//! There are two key flavours, used for opposite sides of the decision:
//!
//! * [`build_approval_key`] — an **exact** digest of the full arguments.
//! Used to scope *denials* so that denying one call (e.g. `rm -rf /tmp/x`)
//! does not also suppress a later, different call to the same tool (#1617).
//!
//! | Tool | Exact key |
//! |---------------|------------------------------------------|
//! | file writes | `file:<tool_name>:<hash of args>` |
//! | shell tools | `shell:<tool_name>:<hash of args>` |
//! | `fetch_url` | `net:<hostname>` |
//! | everything else| `tool:<tool_name>:<hash of input>` |
//!
//! * [`build_approval_grouping_key`] — a **lossy / arity-aware** digest.
//! Used to scope *approvals* so that approving `cargo build` for the
//! session also covers `cargo build --release` (the v0.8.37 behaviour).
//!
//! | Tool | Grouping key |
//! |---------------|------------------------------------------|
//! | `apply_patch` | `patch:<hash of file paths>` |
//! | shell tools | `shell:<command prefix>` |
//! | `fetch_url` | `net:<hostname>` |
//! | everything else| `tool:<tool_name>:<hash of input>` |
//!
//! The cache is **session‑keyed**: entries carry an
//! `ApprovedForSession` flag. When true, the approval is reused for the
@@ -27,6 +44,8 @@ use std::time::Instant;
use serde_json::Value;
use sha2::{Digest, Sha256};
use crate::command_safety::classify_command;
/// The fingerprint of a tool call — stable enough to match repeated
/// calls but specific enough to avoid privilege confusion.
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -141,6 +160,86 @@ pub fn build_approval_key(tool_name: &str, input: &serde_json::Value) -> Approva
ApprovalKey(fingerprint)
}
/// Build the **grouping** approval key for a tool call.
///
/// Unlike [`build_approval_key`], this collapses argument variants of the
/// same command family onto one key (the v0.8.37 behaviour) so that an
/// "approve for session" decision covers later invocations that differ only
/// by flags. Denials must keep using the exact [`build_approval_key`].
#[must_use]
pub fn build_approval_grouping_key(tool_name: &str, input: &serde_json::Value) -> ApprovalKey {
let fingerprint = match tool_name {
"apply_patch" => {
let paths_hash = hash_patch_paths(input);
format!("patch:{paths_hash}")
}
"exec_shell"
| "task_shell_start"
| "exec_shell_wait"
| "exec_shell_interact"
| "exec_wait"
| "exec_interact" => {
let prefix = command_prefix(input);
format!("shell:{prefix}")
}
"fetch_url" | "web.fetch" | "web_fetch" => {
let host = parse_host(input);
format!("net:{host}")
}
_ => format!("tool:{tool_name}:{}", hash_json_value(input)),
};
ApprovalKey(fingerprint)
}
/// Return the canonical command prefix for the shell command in `input`.
///
/// Uses [`classify_command`] from the arity dictionary so that approving
/// `git status` also covers `git status -s` / `git status --porcelain`
/// without also covering `git push`.
fn command_prefix(input: &serde_json::Value) -> String {
let cmd = input.get("command").and_then(|v| v.as_str()).unwrap_or("");
let tokens: Vec<&str> = cmd.split_whitespace().collect();
if tokens.is_empty() {
return "<empty>".to_string();
}
classify_command(&tokens)
}
/// Hash the sorted set of file paths referenced by a patch input.
fn hash_patch_paths(input: &serde_json::Value) -> String {
use std::collections::hash_map::DefaultHasher;
use std::hash::{Hash, Hasher};
let mut paths: Vec<&str> = Vec::new();
if let Some(changes) = input.get("changes").and_then(|v| v.as_array()) {
for change in changes {
if let Some(path) = change.get("path").and_then(|v| v.as_str()) {
paths.push(path);
}
}
} else if let Some(patch_text) = input.get("patch").and_then(|v| v.as_str()) {
for line in patch_text.lines() {
if let Some(rest) = line.strip_prefix("+++ b/") {
paths.push(rest.trim());
}
}
}
paths.sort();
paths.dedup();
if paths.is_empty() {
return "no_files".to_string();
}
let mut hasher = DefaultHasher::new();
for path in &paths {
path.hash(&mut hasher);
}
format!("{:x}", hasher.finish())
}
/// Parse the host portion from a URL input.
fn parse_host(input: &serde_json::Value) -> String {
let url = input.get("url").and_then(|v| v.as_str()).unwrap_or("");
@@ -259,6 +358,52 @@ mod tests {
assert_ne!(key_a, key_b);
}
#[test]
fn grouping_key_collapses_shell_flag_variants() {
let key_a = build_approval_grouping_key("exec_shell", &json!({"command": "cargo build"}));
let key_b =
build_approval_grouping_key("exec_shell", &json!({"command": "cargo build --release"}));
assert_eq!(
key_a, key_b,
"approving a command family must cover later flag variants"
);
}
#[test]
fn grouping_key_still_separates_distinct_commands() {
let key_a = build_approval_grouping_key("exec_shell", &json!({"command": "git status"}));
let key_b = build_approval_grouping_key("exec_shell", &json!({"command": "git push"}));
assert_ne!(key_a, key_b);
}
#[test]
fn grouping_key_collapses_patch_body_for_same_path() {
let key_a = build_approval_grouping_key(
"apply_patch",
&json!({"changes": [{"path": "a.rs", "content": "x"}]}),
);
let key_b = build_approval_grouping_key(
"apply_patch",
&json!({"changes": [{"path": "a.rs", "content": "y"}]}),
);
assert_eq!(
key_a, key_b,
"approving a patch family must cover later edits to the same path"
);
}
#[test]
fn denial_key_stays_exact_while_grouping_key_collapses() {
let exact_a = build_approval_key("exec_shell", &json!({"command": "cargo build"}));
let exact_b = build_approval_key("exec_shell", &json!({"command": "cargo build --release"}));
assert_ne!(exact_a, exact_b, "denials must remain exact-call scoped");
let group_a = build_approval_grouping_key("exec_shell", &json!({"command": "cargo build"}));
let group_b =
build_approval_grouping_key("exec_shell", &json!({"command": "cargo build --release"}));
assert_eq!(group_a, group_b, "approvals must group by command family");
}
#[test]
fn patch_keys_differ_by_path() {
let key_a = build_approval_key(
crates/tui/src/tui/approval.rs
+8 -1
View File
@@ -130,8 +130,11 @@ pub struct ApprovalRequest {
pub impacts: Vec<String>,
/// Tool parameters (for display)
pub params: Value,
/// Fingerprint key for per‑call approval caching (§5.A).
/// Exact-argument fingerprint, used to scope *denials* (#1617).
pub approval_key: String,
/// Lossy / arity-aware fingerprint, used to scope *approvals* so an
/// "approve for session" covers later flag variants (v0.8.37).
pub approval_grouping_key: String,
}
impl ApprovalRequest {
@@ -144,6 +147,8 @@ impl ApprovalRequest {
) -> Self {
let category = get_tool_category(tool_name);
let risk = classify_risk(tool_name, category, params);
let approval_grouping_key =
crate::tools::approval_cache::build_approval_grouping_key(tool_name, params).0;
Self {
id: id.to_string(),
@@ -154,6 +159,7 @@ impl ApprovalRequest {
impacts: build_impact_summary(tool_name, category, params),
params: params.clone(),
approval_key: approval_key.to_string(),
approval_grouping_key,
}
}
@@ -597,6 +603,7 @@ impl ApprovalView {
decision,
timed_out,
approval_key: self.request.approval_key.clone(),
approval_grouping_key: self.request.approval_grouping_key.clone(),
})
}
crates/tui/src/tui/model_picker.rs
+42 -69
View File
@@ -25,40 +25,18 @@ use ratatui::{
text::{Line, Span},
widgets::{Block, Borders, Clear, Paragraph, Widget},
};
use std::collections::HashSet;
use crate::config::{
ApiProvider, model_completion_names_for_provider, provider_passes_model_through,
};
use crate::palette;
use crate::tui::app::{App, ReasoningEffort};
use crate::tui::views::{ModalKind, ModalView, ViewAction, ViewEvent};
fn picker_models_for_provider(provider: ApiProvider) -> Vec<(String, String)> {
let mut rows = vec![("auto".to_string(), "select per turn".to_string())];
if provider_passes_model_through(provider) {
return rows;
}
rows.extend(
model_completion_names_for_provider(provider)
.into_iter()
.map(|id| (id.to_string(), String::new())),
);
rows
}
fn picker_rows_from_model_ids(model_ids: Vec<String>) -> Vec<(String, String)> {
let mut rows = vec![("auto".to_string(), "select per turn".to_string())];
let mut seen = HashSet::from(["auto".to_string()]);
for model_id in model_ids {
let id = model_id.trim();
if id.is_empty() || !seen.insert(id.to_string()) {
continue;
}
rows.push((id.to_string(), String::new()));
}
rows
}
/// Models the picker exposes by default. Kept short on purpose — power
/// users can still type `/model <id>` for anything else.
const PICKER_MODELS: &[(&str, &str)] = &[
("auto", "select per turn"),
("deepseek-v4-pro", "flagship"),
("deepseek-v4-flash", "fast / cheap"),
];
/// Thinking-effort rows shown in the picker, in the order DeepSeek
/// behaviorally distinguishes them.
@@ -78,7 +56,6 @@ enum Pane {
pub struct ModelPickerView {
initial_model: String,
initial_effort: ReasoningEffort,
model_rows: Vec<(String, String)>,
/// Working selection (separate from the initial values so we can offer a
/// clean Esc-to-cancel without mutating App state).
selected_model_idx: usize,
@@ -87,29 +64,30 @@ pub struct ModelPickerView {
/// True when the active model is one we don't list — we still show it
/// so the picker doesn't quietly forget the user's chosen IDs.
show_custom_model_row: bool,
/// When true, hide DeepSeek-specific model rows (pass-through providers
/// like openai don't support them).
hide_deepseek_models: bool,
}
impl ModelPickerView {
#[must_use]
pub fn new(app: &App) -> Self {
Self::new_with_rows(app, picker_models_for_provider(app.api_provider))
}
#[must_use]
pub fn new_with_models(app: &App, model_ids: Vec<String>) -> Self {
Self::new_with_rows(app, picker_rows_from_model_ids(model_ids))
}
fn new_with_rows(app: &App, model_rows: Vec<(String, String)>) -> Self {
let hide_deepseek_models = crate::config::provider_passes_model_through(app.api_provider);
let initial_model = if app.auto_model {
"auto".to_string()
} else {
app.model.clone()
};
let mut selected_model_idx = model_rows.iter().position(|(id, _)| *id == initial_model);
// On pass-through providers, only show "auto" and the custom row.
let visible_models: Vec<&str> = if hide_deepseek_models {
vec!["auto"]
} else {
PICKER_MODELS.iter().map(|(id, _)| *id).collect()
};
let mut selected_model_idx = visible_models.iter().position(|id| *id == initial_model);
let show_custom_model_row = selected_model_idx.is_none();
if show_custom_model_row {
selected_model_idx = Some(model_rows.len());
selected_model_idx = Some(visible_models.len());
}
let selected_model_idx = selected_model_idx.unwrap_or(0);
@@ -127,26 +105,35 @@ impl ModelPickerView {
Self {
initial_model,
initial_effort,
model_rows,
selected_model_idx,
selected_effort_idx,
focus: Pane::Model,
show_custom_model_row,
hide_deepseek_models,
}
}
fn visible_model_ids(&self) -> Vec<&'static str> {
if self.hide_deepseek_models {
vec!["auto"]
} else {
PICKER_MODELS.iter().map(|(id, _)| *id).collect()
}
}
fn model_row_count(&self) -> usize {
self.model_rows.len() + if self.show_custom_model_row { 1 } else { 0 }
self.visible_model_ids().len() + if self.show_custom_model_row { 1 } else { 0 }
}
/// Resolve the currently highlighted model row to a model id. If the
/// custom row is selected we return the original model from the App so
/// "Apply" doesn't blow away an unrecognised id.
fn resolved_model(&self) -> String {
if self.show_custom_model_row && self.selected_model_idx == self.model_rows.len() {
let visible = self.visible_model_ids();
if self.show_custom_model_row && self.selected_model_idx == visible.len() {
self.initial_model.clone()
} else if let Some((model, _)) = self.model_rows.get(self.selected_model_idx) {
model.clone()
} else if self.selected_model_idx < visible.len() {
visible[self.selected_model_idx].to_string()
} else {
self.initial_model.clone()
}
@@ -337,7 +324,14 @@ impl ModalView for ModelPickerView {
.constraints([Constraint::Percentage(60), Constraint::Percentage(40)])
.split(inner);
let mut model_rows = self.model_rows.clone();
let mut model_rows: Vec<(String, String)> = if self.hide_deepseek_models {
vec![("auto".to_string(), "select per turn".to_string())]
} else {
PICKER_MODELS
.iter()
.map(|(id, hint)| ((*id).to_string(), (*hint).to_string()))
.collect()
};
if self.show_custom_model_row {
model_rows.push((self.initial_model.clone(), "current (custom)".to_string()));
}
@@ -478,8 +472,7 @@ mod tests {
#[test]
fn picker_exposes_auto_and_distinct_thinking_tiers() {
let model_rows = picker_models_for_provider(crate::config::ApiProvider::Deepseek);
let model_labels: Vec<_> = model_rows.iter().map(|(id, _)| id.as_str()).collect();
let model_labels: Vec<_> = PICKER_MODELS.iter().map(|(id, _)| *id).collect();
assert_eq!(
model_labels,
vec!["auto", "deepseek-v4-pro", "deepseek-v4-flash"]
@@ -502,26 +495,6 @@ mod tests {
assert_eq!(view.resolved_model(), "deepseek-v4-pro-2026-04-XX");
}
#[test]
fn picker_uses_live_provider_model_ids_when_supplied() {
let (mut app, _lock) = create_test_app();
app.api_provider = crate::config::ApiProvider::Openrouter;
app.model = "meta-llama/llama-3.1-405b-instruct".to_string();
app.auto_model = false;
let view = ModelPickerView::new_with_models(
&app,
vec![
"deepseek/deepseek-chat-v3.1".to_string(),
"meta-llama/llama-3.1-405b-instruct".to_string(),
"qwen/qwen3-coder".to_string(),
],
);
assert!(!view.show_custom_model_row);
assert_eq!(view.resolved_model(), "meta-llama/llama-3.1-405b-instruct");
}
#[test]
fn arrow_keys_move_within_focused_pane() {
let (app, _lock) = create_test_app();
crates/tui/src/tui/ui.rs
+12 -33
View File
@@ -150,8 +150,8 @@ const PERIODIC_FULL_REPAINT_EVERY_N: u64 = 50;
const TURN_META_PREFIX: &str = "<turn_meta>";
const SESSION_TITLE_MAX_CHARS: usize = 32;
fn is_session_approved_for_tool(app: &App, tool_name: &str, approval_key: &str) -> bool {
app.approval_session_approved.contains(approval_key)
fn is_session_approved_for_tool(app: &App, tool_name: &str, grouping_key: &str) -> bool {
app.approval_session_approved.contains(grouping_key)
|| app.approval_session_approved.contains(tool_name)
}
@@ -1694,9 +1694,10 @@ async fn run_event_loop(
tool_name,
description,
approval_key,
approval_grouping_key,
} => {
let session_approved =
is_session_approved_for_tool(app, &tool_name, &approval_key);
is_session_approved_for_tool(app, &tool_name, &approval_grouping_key);
let session_denied = is_session_denied_for_key(app, &approval_key);
if session_denied {
// The user already said no to this exact tool /
@@ -4518,33 +4519,8 @@ async fn apply_command_result(
}
AppAction::OpenModelPicker => {
if app.view_stack.top_kind() != Some(ModalKind::ModelPicker) {
app.status_message =
Some(format!("Fetching {} models...", app.api_provider.as_str()));
let picker = match fetch_available_models(config).await {
Ok(models) if !models.is_empty() => {
app.status_message = Some(format!("Found {} model(s)", models.len()));
crate::tui::model_picker::ModelPickerView::new_with_models(app, models)
}
Ok(_) => {
app.status_message = Some(format!(
"{} returned no models; showing defaults",
app.api_provider.as_str()
));
crate::tui::model_picker::ModelPickerView::new(app)
}
Err(error) => {
app.add_message(HistoryCell::System {
content: format!(
"Failed to fetch {} models: {error}. Showing built-in defaults.",
app.api_provider.as_str()
),
});
app.status_message =
Some("Model fetch failed; showing defaults".to_string());
crate::tui::model_picker::ModelPickerView::new(app)
}
};
app.view_stack.push(picker);
app.view_stack
.push(crate::tui::model_picker::ModelPickerView::new(app));
}
}
AppAction::OpenProviderPicker => {
@@ -5718,12 +5694,15 @@ async fn handle_view_events(
decision,
timed_out,
approval_key,
approval_grouping_key,
} => {
if decision == ReviewDecision::ApprovedForSession {
// Store both the tool name (backward compat) and the
// approval key (fingerprint-based).
// Store the tool name (backward compat) and the lossy
// grouping key so later flag variants of the same
// command family are also auto-approved (v0.8.37).
app.approval_session_approved.insert(tool_name.clone());
app.approval_session_approved.insert(approval_key.clone());
app.approval_session_approved
.insert(approval_grouping_key.clone());
}
match decision {
crates/tui/src/tui/views/mod.rs
+3 -1
View File
@@ -92,8 +92,10 @@ pub enum ViewEvent {
tool_name: String,
decision: ReviewDecision,
timed_out: bool,
/// Fingerprint key for per‑call approval caching (§5.A).
/// Exact-argument fingerprint, used to scope *denials* (#1617).
approval_key: String,
/// Lossy / arity-aware fingerprint, used to scope *approvals*.
approval_grouping_key: String,
},
ElevationDecision {
tool_id: String,