feat(hooks): add on_request_permission hook for external notifications#246
Open
ro0gr wants to merge 2 commits into
Open
feat(hooks): add on_request_permission hook for external notifications#246ro0gr wants to merge 2 commits into
on_request_permission hook for external notifications#246ro0gr wants to merge 2 commits into
Conversation
Add new hook that fires when the agent requests user permission (e.g., for file edits). This enables external notification systems (like Ghostty OSC 9) to alert users when their attention is needed, even when they're not looking at the editor. Changes: - Add RequestPermissionData type to config_default.lua - Add on_request_permission to Hooks type and default config - Invoke hook in SessionManager:_build_handlers() with request, session_id, tab_page_id - Add unit tests in session_manager.test.lua Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
📝 WalkthroughWalkthroughThis PR introduces a new Sequence DiagramsequenceDiagram
participant Agent
participant SessionManager
participant InvokeHook as P.invoke_hook
participant Config as UserConfig
participant PermissionHandler as Handler
Agent->>SessionManager: Request tool execution
SessionManager->>InvokeHook: invoke_hook("on_request_permission", data)
InvokeHook->>Config: Lookup hooks.on_request_permission
alt Hook configured
Config->>PermissionHandler: Execute on_request_permission(data)
PermissionHandler->>PermissionHandler: Custom auth logic<br/>(derive label, notify user)
else Hook not set
InvokeHook->>InvokeHook: Return nil (no-op)
end
InvokeHook->>SessionManager: Return control
SessionManager->>SessionManager: Apply standard<br/>permission handling
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@lua/agentic/session_manager.lua`:
- Around line 980-984: The hook payload uses self.session_id which can be stale;
update the P.invoke_hook call in the on_request_permission flow so the
session_id field comes from the incoming request (use request.sessionId) instead
of self.session_id, keeping tab_page_id as is; ensure you reference
request.sessionId in the payload when calling
P.invoke_hook("on_request_permission", {...}) to avoid misreporting after
session switches.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8e82fb1-0745-4a10-942b-858dcf92a9fa
📒 Files selected for processing (5)
README.mddoc/agentic.txtlua/agentic/config_default.lualua/agentic/session_manager.lualua/agentic/session_manager.test.lua
Comment on lines
+980
to
+984
| P.invoke_hook("on_request_permission", { | ||
| request = request, | ||
| session_id = self.session_id, | ||
| tab_page_id = self.tab_page_id, | ||
| }) |
There was a problem hiding this comment.
Use the request’s session ID in hook payload to avoid stale-session misreporting.
session_id = self.session_id can be incorrect if an older permission event arrives after a session switch. Use request.sessionId for payload accuracy.
Proposed fix
P.invoke_hook("on_request_permission", {
request = request,
- session_id = self.session_id,
+ session_id = request.sessionId,
tab_page_id = self.tab_page_id,
})🤖 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 `@lua/agentic/session_manager.lua` around lines 980 - 984, The hook payload
uses self.session_id which can be stale; update the P.invoke_hook call in the
on_request_permission flow so the session_id field comes from the incoming
request (use request.sessionId) instead of self.session_id, keeping tab_page_id
as is; ensure you reference request.sessionId in the payload when calling
P.invoke_hook("on_request_permission", {...}) to avoid misreporting after
session switches.
Contributor
Author
There was a problem hiding this comment.
I guess if the session is stale, there is no point to notify? And maybe the whole callback should be ignored(or even not invoked at all)?
There was a problem hiding this comment.
@ro0gr — You're right, and that's a better approach. Just swapping the session ID in the payload would still show diffs, render inline permission buttons, and stop the status animation for an event that belongs to a dead session. All of that should be skipped.
The pattern the rest of this file uses (e.g., in _handle_model_change, _handle_thought_level_change) is to capture the session ID at closure-creation time and bail out early if it no longer matches. For _build_handlers that would look roughly like:
function SessionManager:_build_handlers()
local session_id_at_build = self.session_id -- capture now
local handlers = {
-- ...
on_request_permission = function(request, callback)
-- Drop stale permission events from a superseded session.
-- Still call callback with a rejection so the agent doesn't stall.
if self.session_id ~= session_id_at_build then
callback("reject_once")
return
end
P.invoke_hook("on_request_permission", {
request = request,
session_id = self.session_id,
tab_page_id = self.tab_page_id,
})
-- ... rest of handler unchanged
end,
}
return handlers
endThe key nuance is that silently dropping callback (not calling it at all) would leave the agent side waiting indefinitely for a response, so calling it with "reject_once" before returning ensures the protocol stays consistent. Would you like me to open a follow-up issue for this, or plan the full code change?
🧠 Learnings used
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 74
File: tests/helpers/spy.lua:21-61
Timestamp: 2026-01-11T12:49:03.529Z
Learning: Neovim uses Lua 5.1 (as of version 0.11.0). In this repository, avoid recommending Lua 5.2+ features (e.g., table.pack, table.unpack, goto, \z in string literals) when reviewing or writing Lua code. For tests/helpers/spy.lua and other Lua files, prefer Lua 5.1-compatible constructs; if newer features are necessary, ensure compatibility or document rationale and provide a shim.
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 98
File: lua/agentic/ui/window_decoration.lua:77-85
Timestamp: 2026-01-29T20:36:14.589Z
Learning: In lua/agentic/**/*.lua, when initializing per-tab at vim.t[tab_page_id], per-buffer at vim.b[bufnr], or per-window at vim.w[winid] from module constants, do not use vim.deepcopy(). Assigning the table directly creates an independent copy due to the serialized storage in vim.* tables, which is typically sufficient for per-instance state. Only use deepcopy if you truly need a shared, non-serializable reference behavior.
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 104
File: lua/agentic/session_manager.lua:245-254
Timestamp: 2026-02-03T21:12:22.183Z
Learning: In lua/agentic/session_manager.lua, within _handle_input_submit, ensure chat_history.title is set from input_text in these two scenarios: 1) when restoring a session on the first submit and _needs_history_send is true, update the title; 2) for a new session when title is empty, set the title. Use an elseif branch so the title is assigned only once per submit.
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 124
File: lua/agentic/acp/acp_client.lua:342-359
Timestamp: 2026-02-20T14:38:23.848Z
Learning: For Lua files (e.g., lua/agentic/acp/acp_client.lua and similar), prefer Selene as the static analysis/linting tool over Luacheck. Selene is better maintained. Ensure repository CI and editor tooling invoke Selene for linting/analysis and disable Luacheck where configured.
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 162
File: lua/agentic/ui/message_writer.lua:432-444
Timestamp: 2026-03-20T07:09:34.381Z
Learning: In this repository’s Lua code (e.g., files like lua/agentic/ui/message_writer.lua), follow the existing author style: prefer defining behavior as “private” methods on the module/table (e.g., MessageWriter:_build_header_line) rather than as module-level local functions, even if the method receiver (`self`) is unused. Do not recommend converting unused-`self` methods into module-level local functions in review feedback, since module-level locals require being defined before use (hoisting concern), while methods can appear anywhere in the file.
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 175
File: lua/agentic/utils/buf_helpers.lua:70-74
Timestamp: 2026-04-01T17:38:48.173Z
Learning: When reviewing Neovim Lua code that uses `vim.keymap.set`, ensure buffer-local mappings use the correct option key for the target Neovim version. For Neovim 0.12+, `opts.buf` is the correct buffer-local key; `opts.buffer` is deprecated and removed in Neovim 0.15. Use the conditional branching pattern instead of `opts.buffer` unconditionally:
```lua
if vim.fn.has("nvim-0.12") == 1 then
opts.buf = bufnr
else
opts.buffer = bufnr
end
```
Avoid suggesting or using `opts.buffer` by itself.
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 182
File: lua/agentic/utils/file_system.lua:123-124
Timestamp: 2026-04-06T12:02:59.060Z
Learning: When writing Neovim Lua (LuaLS-checked), prefer creating a new local variable to achieve type narrowing instead of narrowing via reassignment. LuaLS often will not narrow a variable’s type through reassignments (e.g., `local lines, err = fn(); lines = lines or {}` may keep `lines` as `string[]|nil`). Use a new local on the narrowed expression (e.g., `local result, err = fn(); local lines = result or {}`) so LuaLS infers the non-nil type (e.g., `string[]`). Apply the “new-local” pattern whenever you need LuaLS to infer a narrowed/non-nil type.
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 193
File: lua/agentic/ui/chat_widget.lua:289-295
Timestamp: 2026-04-14T18:10:13.081Z
Learning: In this Neovim Lua codebase (agentic.nvim), it’s expected that buffers created or looked up via `vim.fn.bufadd(filepath)` or `vim.fn.bufnr(filepath)` won’t incur disk I/O or read file contents until that buffer is actually displayed in a window. Therefore, during code review you should not flag or warn about intermediate buffer assignments within the same synchronous event-loop tick as a “redundant load,” I/O/performance issue, memory leak, or visual artifact. In particular, don’t suggest forwarding/altering the buffer just to avoid “redundant” intermediate buffer-loading—this is a non-issue here.
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 220
File: lua/agentic/ui/tool_call_fold.lua:37-43
Timestamp: 2026-05-03T16:04:10.818Z
Learning: When reviewing Lua code that calls `vim.api.nvim_win_text_height(winid, opts)`, expect the return value to be a table/dict (`vim.api.keyset.win_text_height_ret`), not a number. The table includes fields such as `all` (total screen lines occupied), `fill`, `end_row`, and `end_vcol`. Do not treat `type(result) == "table"` as incorrect if the code uses `result.all`. If you need to guard for unexpected return types, the correct check is `type(result) ~= "table"` (i.e., only treat non-table results as errors). Access the screen-line count via `result.all`.
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 231
File: lua/agentic/ui/permission_manager.lua:476-488
Timestamp: 2026-05-13T17:17:44.714Z
Learning: In this codebase, all keymap deletions must use `BufHelpers.keymap_del` instead of calling `vim.keymap.del` directly. Review `lua/agentic/**/*.lua` for any direct `vim.keymap.del(...)` usage and replace it with `require('...buf_helpers').keymap_del` (from `lua/agentic/utils/buf_helpers.lua`). `BufHelpers.keymap_del` must apply Neovim version-aware option handling: pass `{ buf = bufnr }` on Neovim 0.12.1+, and `{ buffer = bufnr }` on earlier versions. Treat any direct `vim.keymap.del` calls that include `{ buffer = ... }` as a bug.
Learnt from: carlos-algms
Repo: carlos-algms/agentic.nvim PR: 231
File: lua/agentic/ui/permission_manager.lua:129-135
Timestamp: 2026-05-13T19:45:47.457Z
Learning: In agentic.nvim, for MessageWriter’s `_capture_scroll`/`_apply_scroll` scroll-guard pair, only require the guard around buffer mutations that change the buffer’s total line count. Insertions/removals (which change `total_lines`) should trigger the gating condition; row-level text rewrites that keep line count constant (e.g., repainting a status row/button label width on focus toggle) should not. During reviews, do not flag missing `_capture_scroll`/`_apply_scroll` for row writes whose edits do not grow/shrink the buffer (and therefore do not affect `total_lines - cursor_line`).
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.
Add a new
on_request_permissionhook that fires when the agent requests user permission (e.g., for file edits).This complements the
on_response_completeto enable external notification systems (like Ghostty OSC 9) to alert users when their attention is needed, even when they're not looking at the editor.