feat(tui): arrow-key nav for tool-permission prompt + close P0 audit gaps#186
Merged
Conversation
…dit gaps
Bug fix:
- The tool-permission prompt (the y/n dialog shown e.g. when the LLM calls
EnterPlanMode) only accepted typed y/n. Users expected arrow-key selection,
so the UX felt like a typing-only modal.
Fix:
- Extend `PendingPermission` with a `cursor: PermissionChoice` (Allow/Deny,
default Allow).
- Modal handler: Left/Right/Up/Down/Tab → `ToolPermissionToggle`; Enter →
`ToolPermissionConfirm` (commits whichever choice the cursor points at).
y/n shortcuts and Esc-cancels preserved for backward compat.
- Render: highlight the focused choice with inverted bg, hint shows
"←/→ select Enter confirm Esc cancel".
Test coverage for the fix:
- `permission_arrow_nav_test` (8 tests): each arrow direction toggles,
Tab toggles, Enter dispatches Confirm, y/n still work, Esc still denies,
cursor field actually flips on toggle.
Audit-driven coverage additions:
`memory_consolidation_test` (3 tests) — agent-server's
`trigger_consolidation` was the only call site of `loopal-memory`'s
lock primitives and had zero integration coverage. New tests verify:
- Fresh-lock pre-exists → trigger short-circuits, lock content untouched,
`.last_consolidation` not written
- No-lock → trigger acquires synchronously before tokio::spawn returns
- After spawn-failure path releases the lock, a second trigger re-acquires
`hub_secret_client_test` (5 tests) — `HubSecretClient` had inline unit
tests for expand/health/placeholder/retry helpers but no end-to-end
coverage of the integration. New tests verify:
- `IpcBudget::Forbidden` rejects `get` and `list_names` synchronously
with a clear error (no IPC issued)
- `health()` starts in healthy state
- Health transitions to degraded after consecutive failures (3+ failures
cross the default threshold)
- `health()` returns the same Arc across calls (shared state contract)
Required schema/API additions:
- `PermissionChoice` enum (Allow/Deny) with `toggle()`, exported from
loopal-view-state alongside PendingPermission.
- `InputAction::{ToolPermissionToggle, ToolPermissionConfirm}` for
the new arrow-key flow.
- `loopal-agent-server::testing::trigger_consolidation` re-export so
the agent-server tests can drive the trigger.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
y/n.memory_consolidation::trigger_consolidation(zero coverage before) andHubSecretClientbudget/health gating (had inline unit tests but no integration coverage).Bug detail
When the LLM calls
EnterPlanMode,handle_enter_plan→request_permission→ManualPermissionHandleremits aToolPermissionRequest. The TUI shows a modal whose key handler only matchedy/n/Esc. Users expected arrow-key selection between Allow/Deny.Fix
PendingPermissiongains acursor: PermissionChoicefield (defaultAllow).ToolPermissionToggle; Enter →ToolPermissionConfirm.y/nshortcuts and Esc-cancel preserved.←/→ select Enter confirm Esc cancel.Coverage additions
permission_arrow_nav_test.rsmemory_consolidation_test.rshub_secret_client_test.rsIpcBudget::Forbiddenrejects synchronously; health starts healthy; health degrades after ≥3 consecutive failures;health()returns sameArcTotal: 16 new tests.
Required schema/API additions
PermissionChoiceenum (Allow/Deny) withtoggle(), exported fromloopal-view-stateInputAction::{ToolPermissionToggle, ToolPermissionConfirm}loopal-agent-server::testing::trigger_consolidationre-exportTest plan
bazel build / test / clippy / rustfmt)Manual repro of the bug fix
EnterPlanModetool cally/nwork; pressing←/→does nothing←/→/Tab toggle between Allow/Deny; Enter confirms; y/n still work as shortcuts