feat: add on_file_edit hook for post-write callbacks#163
feat: add on_file_edit hook for post-write callbacks#163qianchongyang wants to merge 1 commit intocarlos-algms:mainfrom
Conversation
Added on_file_edit hook that fires after Agentic writes/edits files. The hook receives: - filepath: absolute path to the edited file - session_id: ACP session ID - tab_page_id: Neovim tabpage ID - bufnr: buffer number if file is loaded Use case: auto-format, lint, or post-process edited files. Fixes carlos-algms#110
📝 WalkthroughWalkthroughThis pull request introduces a new optional user hook called 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)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lua/agentic/config_default.lua`:
- Line 40: The doc comment for the buffer field uses union-style nullability;
change the `@field` annotation from using "number|nil" to the LuaCATS
optional-field syntax "bufnr? number" so update the comment that declares bufnr
(the `@field` for bufnr) to use "@field bufnr? number" ensuring consistency with
other `@field` annotations like "_state? string".
In `@lua/agentic/session_manager.lua`:
- Around line 313-314: The hook payload is using the widget field
`self.widget.tabpage` instead of the session instance's tabpage getter; update
`on_file_edit` to set `tab_page_id = self.tab_page_id` (ensure
`self.tab_page_id` computes the tabpage id correctly by using
`vim.api.nvim_win_get_tabpage(vim.fn.bufwinid(bufnr))` when a buffer window
exists or fallback to `vim.api.nvim_get_current_tabpage()`), replace references
to `self.widget.tabpage` with `self.tab_page_id` in the payload where `bufnr` is
available.
- Around line 304-312: The code currently assumes tracker.argument is a string;
add a strict type guard to ensure it's a non-empty string before calling
FileSystem.to_absolute_path and using it to compute bufnr and invoke the hook.
Concretely, in the block that reads tracker.argument (the local filepath), check
that type(filepath) == "string" and filepath ~= "" (or equivalent) before
calling FileSystem.to_absolute_path, computing bufnr via vim.fn.bufnr, and
calling P.invoke_hook("on_file_edit", { filepath = ..., session_id =
self.session_id }); if the check fails, skip the file-edit logic to avoid
passing non-strings into FileSystem.to_absolute_path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0b685fc-3484-4c46-a5f3-d8dbbdb4addc
📒 Files selected for processing (2)
lua/agentic/config_default.lualua/agentic/session_manager.lua
| --- @field filepath string Absolute path to the edited file | ||
| --- @field session_id string The ACP session ID | ||
| --- @field tab_page_id number The tabpage ID | ||
| --- @field bufnr number|nil Buffer number if the file is loaded in a buffer |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use @field bufnr? number for LuaCATS consistency.
This field currently uses union style (number|nil) in an @field annotation; this repo’s LuaCATS convention expects optional-field syntax.
♻️ Proposed fix
---- `@field` bufnr number|nil Buffer number if the file is loaded in a buffer
+--- `@field` bufnr? number Buffer number if the file is loaded in a bufferAs per coding guidelines: “For @field annotations, use variable? type format instead of union (e.g., @field _state? string).”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --- @field bufnr number|nil Buffer number if the file is loaded in a buffer | |
| --- `@field` bufnr? number Buffer number if the file is loaded in a buffer |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/agentic/config_default.lua` at line 40, The doc comment for the buffer
field uses union-style nullability; change the `@field` annotation from using
"number|nil" to the LuaCATS optional-field syntax "bufnr? number" so update the
comment that declares bufnr (the `@field` for bufnr) to use "@field bufnr? number"
ensuring consistency with other `@field` annotations like "_state? string".
| local filepath = tracker.argument | ||
| if filepath then | ||
| local bufnr = vim.fn.bufnr(FileSystem.to_absolute_path(filepath)) | ||
| if bufnr == -1 then | ||
| bufnr = nil | ||
| end | ||
| P.invoke_hook("on_file_edit", { | ||
| filepath = FileSystem.to_absolute_path(filepath), | ||
| session_id = self.session_id, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect tool-call payload shaping and updates (kind/argument/status) across ACP + message writer
rg -n -C3 'tool_call|toolCall|kind|argument|status' lua/agentic/acp lua/agentic/ui/message_writer.lua
# Check SessionManager usage and ChatWidget tabpage naming for consistency
CHAT_WIDGET_FILE="$(fd 'chat_widget\.lua$' lua | head -n1 || true)"
if [ -n "${CHAT_WIDGET_FILE}" ]; then
rg -n -C3 'tabpage|tab_page_id' "$CHAT_WIDGET_FILE" lua/agentic/session_manager.lua
else
rg -n -C3 'tabpage|tab_page_id' lua/agentic/session_manager.lua
fiRepository: carlos-algms/agentic.nvim
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# View the exact code in session_manager.lua around lines 304-312
echo "=== session_manager.lua lines 300-320 ==="
sed -n '300,320p' lua/agentic/session_manager.lua
echo ""
echo "=== FileSystem.to_absolute_path signature ==="
sed -n '150,156p' lua/agentic/utils/file_system.lua
echo ""
echo "=== Check argument handling in session_manager for file-mutating kinds ==="
rg -n -B5 -A5 'on_file_edit|tracker.argument|on_tool_call' lua/agentic/session_manager.lua | head -60Repository: carlos-algms/agentic.nvim
Length of output: 3363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get full FileSystem.to_absolute_path definition
echo "=== FileSystem.to_absolute_path function ==="
rg -n -A15 'function FileSystem.to_absolute_path' lua/agentic/utils/file_system.lua
# Check MessageWriter type annotations for tracker after merge
echo ""
echo "=== MessageWriter ToolCallBase and ToolCallBlock definitions ==="
rg -n -B2 -A8 '@class.*ToolCall' lua/agentic/ui/message_writer.lua | head -40
# Check how tracker is updated in message_writer
echo ""
echo "=== How tracker is merged in message_writer ==="
sed -n '287,320p' lua/agentic/ui/message_writer.lua
# Check if file-mutating kinds always have argument set
echo ""
echo "=== File-mutating kinds and how argument is set in adapters ==="
rg -n 'edit|create|write|delete|move' lua/agentic/acp/adapters/opencode_acp_adapter.lua | grep -i argument | head -20Repository: carlos-algms/agentic.nvim
Length of output: 2962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if argument can become nil during updates
echo "=== Check ACP adapters: file-mutating kinds and their argument handling ==="
rg -n -B3 -A3 'kind.*=.*"(edit|create|write|delete|move)"' lua/agentic/acp/adapters/ | head -40
# Check what happens when tool_call_update doesn't include argument for file-mutating kinds
echo ""
echo "=== Tool call update handling for file-mutating kinds ==="
rg -n -B5 -A5 '__handle_tool_call_update' lua/agentic/acp/adapters/opencode_acp_adapter.lua | grep -A10 'kind.*edit\|kind.*create\|kind.*write'
# Verify tracker behavior after merge
echo ""
echo "=== Test vim.tbl_deep_extend behavior when optional field is nil ==="
python3 << 'EOF'
# Simulate Lua's vim.tbl_deep_extend behavior
tracker = {
"tool_call_id": "id1",
"kind": "edit",
"argument": "/path/to/file",
"status": "pending"
}
update = {
"tool_call_id": "id1",
"status": "completed"
# Note: argument is missing
}
# vim.tbl_deep_extend("force", tracker, update) keeps tracker's fields
# and overwrites with update's fields
merged = {**tracker, **update}
print("Original tracker.argument:", tracker.get("argument"))
print("Update has argument:", "argument" in update)
print("After merge, argument exists:", "argument" in merged)
print("After merge, argument value:", merged.get("argument"))
print("")
print("BUT: if update explicitly sets argument to nil (or empty):")
update_with_nil = {
"tool_call_id": "id1",
"status": "completed",
"argument": None # explicit nil
}
merged_with_nil = {**tracker, **update_with_nil}
print("Merged with nil argument:", merged_with_nil.get("argument"))
EOFRepository: carlos-algms/agentic.nvim
Length of output: 3161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if any tool_call_update explicitly sends empty/nil argument for file-mutating kinds
echo "=== Check if tool_call_update can send nil/empty argument for file-mutating kinds ==="
rg -n -B10 -A10 '__build_tool_call_update|on_tool_call_update' lua/agentic/acp/adapters/ | grep -A10 -B10 'argument' | head -80
# Look at the learning: what does it say about tool_call_blocks tracking?
echo ""
echo "=== Direct check: can argument become nil after deep_extend? ==="
cat > /tmp/test_merge.lua << 'LUAEOF'
-- Simulate the merge scenario
local tracker = {
tool_call_id = "id1",
kind = "edit",
argument = "/path/to/file", -- required initially
status = "pending"
}
-- Scenario 1: Update without argument field (most common)
local update1 = {
tool_call_id = "id1",
status = "completed"
}
-- In Lua, vim.tbl_deep_extend("force", a, b) merges b into a, with b's values winning
-- If b doesn't have a key, a's value is preserved
local merged1 = vim.tbl_deep_extend("force", tracker, update1)
print("After merge (update without argument field):")
print(" argument type:", type(merged1.argument))
print(" argument value:", merged1.argument)
-- Scenario 2: If update explicitly has argument = nil (rare but possible)
local update2 = {
tool_call_id = "id1",
status = "completed",
argument = nil -- explicitly nil
}
local merged2 = vim.tbl_deep_extend("force", tracker, update2)
print("")
print("After merge (update with argument = nil):")
print(" argument in table:", merged2.argument ~= nil)
print(" argument value:", merged2.argument)
LUAEOF
# Check if we can identify actual patterns where file-mutating tools lose argument
echo ""
echo "=== Check OpenCode adapter for file-mutating tool argument handling ==="
sed -n '80,160p' lua/agentic/acp/adapters/opencode_acp_adapter.luaRepository: carlos-algms/agentic.nvim
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at the exact flow: initial tool_call, then tool_call_update
echo "=== How argument flows through initial tool_call vs updates ==="
sed -n '380,425p' lua/agentic/acp/acp_client.lua
echo ""
echo "=== Check OpenCode adapter's tool_call_update handling for file-mutating kinds ==="
sed -n '80,165p' lua/agentic/acp/adapters/opencode_acp_adapter.lua
echo ""
echo "=== Check if tracker.argument can be nil after update in session_manager context ==="
# Look at what happens in _on_tool_call_update
sed -n '268,320p' lua/agentic/session_manager.luaRepository: carlos-algms/agentic.nvim
Length of output: 7361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the learning about tool_call_blocks persistence across phases
echo "=== Verify the three phases of tool_call tracking ==="
echo "Phase 1: write_tool_call_block (initial)"
sed -n '237,285p' lua/agentic/ui/message_writer.lua | grep -A50 'function MessageWriter:write_tool_call_block'
echo ""
echo "Phase 2: update_tool_call_block (updates)"
sed -n '287,320p' lua/agentic/ui/message_writer.lua | head -40
# Check for any cases where argument could be set to empty or nil
echo ""
echo "=== Verify: can FileSystem.to_absolute_path handle non-string safely? ==="
cat lua/agentic/utils/file_system.lua | sed -n '164,168p'
# Check if there are any runtime type checks in place before calling FileSystem functions
echo ""
echo "=== Check current guards before FileSystem calls in session_manager ==="
rg -n -B5 'FileSystem.to_absolute_path' lua/agentic/session_manager.luaRepository: carlos-algms/agentic.nvim
Length of output: 3985
Guard tracker.argument type before treating it as a filepath.
FileSystem.to_absolute_path is defined with path: string and calls vim.fn.fnamemodify(path, ":p"), which will fail at runtime if passed a non-string value. While tracker.argument is initialized as a required string field from the initial tool_call, subsequent tool_call_update messages define argument as optional (@field argument? string). If an ACP provider sends a malformed update with a non-string or empty argument for file-mutating kinds, the truthy check alone (if filepath then) is insufficient to prevent runtime errors. Add a string type guard before normalization.
🛡️ Proposed hardening
local filepath = tracker.argument
- if filepath then
- local bufnr = vim.fn.bufnr(FileSystem.to_absolute_path(filepath))
+ if type(filepath) == "string" and filepath ~= "" then
+ local abs_filepath = FileSystem.to_absolute_path(filepath)
+ local bufnr = vim.fn.bufnr(abs_filepath)
if bufnr == -1 then
bufnr = nil
end
P.invoke_hook("on_file_edit", {
- filepath = FileSystem.to_absolute_path(filepath),
+ filepath = abs_filepath,
session_id = self.session_id,
tab_page_id = self.widget.tabpage,
bufnr = bufnr,
})
+ else
+ Logger.debug("Skipping on_file_edit: tool call argument is not a filepath string")
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local filepath = tracker.argument | |
| if filepath then | |
| local bufnr = vim.fn.bufnr(FileSystem.to_absolute_path(filepath)) | |
| if bufnr == -1 then | |
| bufnr = nil | |
| end | |
| P.invoke_hook("on_file_edit", { | |
| filepath = FileSystem.to_absolute_path(filepath), | |
| session_id = self.session_id, | |
| local filepath = tracker.argument | |
| if type(filepath) == "string" and filepath ~= "" then | |
| local abs_filepath = FileSystem.to_absolute_path(filepath) | |
| local bufnr = vim.fn.bufnr(abs_filepath) | |
| if bufnr == -1 then | |
| bufnr = nil | |
| end | |
| P.invoke_hook("on_file_edit", { | |
| filepath = abs_filepath, | |
| session_id = self.session_id, | |
| tab_page_id = self.widget.tabpage, | |
| bufnr = bufnr, | |
| }) | |
| else | |
| Logger.debug("Skipping on_file_edit: tool call argument is not a filepath string") | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/agentic/session_manager.lua` around lines 304 - 312, The code currently
assumes tracker.argument is a string; add a strict type guard to ensure it's a
non-empty string before calling FileSystem.to_absolute_path and using it to
compute bufnr and invoke the hook. Concretely, in the block that reads
tracker.argument (the local filepath), check that type(filepath) == "string" and
filepath ~= "" (or equivalent) before calling FileSystem.to_absolute_path,
computing bufnr via vim.fn.bufnr, and calling P.invoke_hook("on_file_edit", {
filepath = ..., session_id = self.session_id }); if the check fails, skip the
file-edit logic to avoid passing non-strings into FileSystem.to_absolute_path.
| tab_page_id = self.widget.tabpage, | ||
| bufnr = bufnr, |
There was a problem hiding this comment.
Use self.tab_page_id in the hook payload.
on_file_edit should emit the session’s tabpage ID from the session manager instance, not a widget field.
✅ Proposed fix
- tab_page_id = self.widget.tabpage,
+ tab_page_id = self.tab_page_id,As per coding guidelines: “Get tabpage ID correctly using: instance method self.tab_page_id, from buffer via vim.api.nvim_win_get_tabpage(vim.fn.bufwinid(bufnr)), or current tabpage via vim.api.nvim_get_current_tabpage().”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tab_page_id = self.widget.tabpage, | |
| bufnr = bufnr, | |
| tab_page_id = self.tab_page_id, | |
| bufnr = bufnr, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/agentic/session_manager.lua` around lines 313 - 314, The hook payload is
using the widget field `self.widget.tabpage` instead of the session instance's
tabpage getter; update `on_file_edit` to set `tab_page_id = self.tab_page_id`
(ensure `self.tab_page_id` computes the tabpage id correctly by using
`vim.api.nvim_win_get_tabpage(vim.fn.bufwinid(bufnr))` when a buffer window
exists or fallback to `vim.api.nvim_get_current_tabpage()`), replace references
to `self.widget.tabpage` with `self.tab_page_id` in the payload where `bufnr` is
available.
carlos-algms
left a comment
carlos-algms
left a comment
There was a problem hiding this comment.
It seems nice and simple, thanks for the contribution.
I won't merge it right now, as I'm in the middle of a huge refactor to support even more providers. and argument will become file_path for the sake of consistency, and if it's not nil, then the file definitely exists.
I'll approve as soon as the refactor is done, so you can rebase and fix conflicts, if any.
|
@qianchongyang I just merged the Feel free to rebase your branch and fix the file_path, and address the Bot comments 🫡 |
…lgms#202) ## Summary - Adds `on_file_edit` hook fired after file-mutating tool calls complete, with `filepath`, `session_id`, `tab_page_id`, and `bufnr` (nil when file not loaded). - Rebased and adapted from @qianchongyang's stale PR carlos-algms#163: - `tracker.argument` renamed to `tracker.file_path` (post-refactor PR carlos-algms#162). - Uses `self.tab_page_id` instead of `self.widget.tabpage` (per CodeRabbit). - Guards on `type(tracker.file_path) == \"string\"` and empty string (per CodeRabbit). - `bufnr` field uses LuaCATS `? number` optional syntax (per AGENTS.md style). - Adds doc/README warning: mutating the file in this hook can cause the agent to re-read or retry edits. - Fix carlos-algms#110 - Close carlos-algms#163 (credit to @qianchongyang) ## Test plan - [x] `make validate` (format, luals, selene, test all pass) - [x] 4 new `it()` blocks in `session_manager.test.lua`: - fires with absolute path + bufnr when file loaded - fires with nil bufnr when file not loaded - does not fire when tracker lacks `file_path` - does not fire for non-file-mutating tool calls
3 participants
Problem
The existing
on_response_completehook doesn't provide information about which files were modified. Users who want to auto-format, lint, or post-process edited files have no clean way to do so.Current workaround requires tracking all buffer writes with BufWritePost autocmd, which captures non-Agentic writes too.
Solution
Added new
on_file_edithook that fires after Agentic writes to a file:Validation
Fixes #110