Skip to content

Commit de4c8aa

Browse files
BunsDevclaude
andcommitted
fix(tui): replace permission_request double-unwrap with pattern match
Closes the concrete Phase 4 'permissions hardening' defect flagged by the audit at app.rs:5806-5812. `handle_permission_key` bound `pr = self.permission_request.as_mut()` once at the top of the function, then re-borrowed `.as_mut().unwrap()` twice more inside the Up and Down arms. The audit called this out as high-severity because: * the dialog can be cleared between key events (e.g. a tool result resolves the request from the model side), and * the inner unwrap would panic the TUI on what should be a no-op Up/Down keystroke against a dialog that just went away. Both arms now pattern-match via `if let Some(pr) = self.permission_request.as_mut()` per AGENTS.md's rule "No `.unwrap()` / `.expect()` on fallible operations in production paths — propagate via Result or pattern-match." The Up and Down handlers silently no-op when no dialog is active, which is the correct behaviour: there is nothing to navigate. Two regression tests added in claurst-tui: * arrow_keys_do_not_panic_without_permission_dialog — handles the audit scenario directly: a stray Up/Down with no active dialog. Before this fix that would have panicked the second unwrap. * arrow_keys_navigate_permission_dialog_selection — pins the bound-saturation behaviour with a real PermissionRequest, walking past index 0 and the last index in both directions. All four verification gates clean at v0.0.14: cargo fmt --all -- --check ✅ cargo check --workspace ✅ cargo clippy -- -D warnings ✅ cargo test --workspace ✅ 1549 passing, 0 failing (+2 net new tests) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 9b39623 commit de4c8aa

2 files changed

Lines changed: 62 additions & 6 deletions

File tree

src-rust/crates/tui/src/app.rs

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5880,15 +5880,21 @@ impl App {
58805880
self.permission_request = None;
58815881
}
58825882
KeyCode::Up => {
5883-
let pr = self.permission_request.as_mut().unwrap();
5884-
if pr.selected_option > 0 {
5885-
pr.selected_option -= 1;
5883+
// Pattern-match instead of `.as_mut().unwrap()`: the
5884+
// permission_request can be cleared between key events
5885+
// (e.g. by a model-side cancellation), so we must not
5886+
// panic when the dialog has gone away.
5887+
if let Some(pr) = self.permission_request.as_mut() {
5888+
if pr.selected_option > 0 {
5889+
pr.selected_option -= 1;
5890+
}
58865891
}
58875892
}
58885893
KeyCode::Down => {
5889-
let pr = self.permission_request.as_mut().unwrap();
5890-
if pr.selected_option + 1 < pr.options.len() {
5891-
pr.selected_option += 1;
5894+
if let Some(pr) = self.permission_request.as_mut() {
5895+
if pr.selected_option + 1 < pr.options.len() {
5896+
pr.selected_option += 1;
5897+
}
58925898
}
58935899
}
58945900
KeyCode::Esc => {

src-rust/crates/tui/src/lib.rs

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1575,4 +1575,54 @@ mod tests {
15751575
assert_eq!(pr.options[2].key, 'p');
15761576
assert_eq!(pr.options[3].key, 'n');
15771577
}
1578+
1579+
/// Regression: the audit flagged a double-unwrap in
1580+
/// `handle_permission_key`'s Up/Down arms. If the dialog was cleared
1581+
/// between the outer `as_mut()` bind and the inner `.as_mut().unwrap()`
1582+
/// (e.g. a tool result resolved the request), the second unwrap would
1583+
/// panic the TUI. Now those branches pattern-match and silently no-op.
1584+
/// This test pins the no-panic contract by handling Up/Down with no
1585+
/// active permission dialog.
1586+
#[test]
1587+
fn arrow_keys_do_not_panic_without_permission_dialog() {
1588+
let mut app = make_app();
1589+
assert!(app.permission_request.is_none());
1590+
// Either branch could have panicked before the fix.
1591+
app.handle_key_event(key(KeyCode::Up));
1592+
app.handle_key_event(key(KeyCode::Down));
1593+
// No assertion needed — surviving these calls is the assertion.
1594+
}
1595+
1596+
/// Up/Down with an active dialog adjusts `selected_option` correctly
1597+
/// and saturates at the bounds.
1598+
#[test]
1599+
fn arrow_keys_navigate_permission_dialog_selection() {
1600+
let mut app = make_app();
1601+
let mut pr = PermissionRequest::standard(
1602+
"tu_test".to_string(),
1603+
"Bash".to_string(),
1604+
"Run a command".to_string(),
1605+
);
1606+
pr.selected_option = 0;
1607+
app.permission_request = Some(pr);
1608+
1609+
app.handle_key_event(key(KeyCode::Down));
1610+
assert_eq!(app.permission_request.as_ref().unwrap().selected_option, 1);
1611+
app.handle_key_event(key(KeyCode::Down));
1612+
assert_eq!(app.permission_request.as_ref().unwrap().selected_option, 2);
1613+
app.handle_key_event(key(KeyCode::Up));
1614+
assert_eq!(app.permission_request.as_ref().unwrap().selected_option, 1);
1615+
1616+
// Saturation: walking past index 0 should stay at 0.
1617+
app.handle_key_event(key(KeyCode::Up));
1618+
app.handle_key_event(key(KeyCode::Up));
1619+
app.handle_key_event(key(KeyCode::Up));
1620+
assert_eq!(app.permission_request.as_ref().unwrap().selected_option, 0);
1621+
1622+
// Saturation at the end too — 4 options, max index 3.
1623+
for _ in 0..10 {
1624+
app.handle_key_event(key(KeyCode::Down));
1625+
}
1626+
assert_eq!(app.permission_request.as_ref().unwrap().selected_option, 3);
1627+
}
15781628
}

0 commit comments

Comments
 (0)