-
-
Notifications
You must be signed in to change notification settings - Fork 266
fix: Dynamically Added Tools Not Available to Chat Agents #1693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add config hash-based cache invalidation to ToolFilter - Refresh agent tools before chat submission - Enables plugins to add tools at runtime This addresses an issue where dynamically added tools would appear in completion menus but fail during agent execution due to cached tool configurations.
Having that merged would be great, this IMHO is blocking MCP adoption in codecompanion (the subjectively best AI companion for neovim) |
---Generate a hash of the current tools config to detect changes | ||
---@param tools_config table | ||
---@return string | ||
local function generate_config_hash(tools_config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the existing hash algorithm from codecompanion.utils.hash
here instead? Would be in keeping with the rest of the plugin and clean this up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was aiming to only invalidate when tool names or groups change. I was trying to avoid unnecessary cache invalidations from other field changes and being extra cautious which was probably unnecessary.
Updated it now to use hash utils.
@@ -83,6 +83,32 @@ T["Agent"][":find"]["should not find a group when tool name starts with group na | |||
h.eq({}, result.groups) | |||
end | |||
|
|||
T["Agent"][":find"]["should find tools added after a chat is initialized"] = function() | |||
child.lua([[ | |||
local config = require("codecompanion.config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we're not using tests.config
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it was a mistake. Fixed it now. Sorry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is failing actually. Given the agents.lua
file uses require(codecompanion.config)
, adding a new tool to tests.config
will indeed fail as it tries to look in the codecompanion.config
. I think, that's why you are calling mock_config()
inside the setup_chat_buffer()
.
There's no other way to simulate adding a dynamic tool. Reverting it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this just takes the setup
function from the regular config and uses it in the test config. Was a way of ensuring we're never corrupting the setup process for users.
If you comment out the files
tool in tests.config
and run the T["Agent"][":find"]["should only find tools that end with space or are eol"]
test, you'll see that it fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right that mock_config()
ensures we don't corrupt the setup process. The issue I discovered is specifically with vim.deepcopy()
in the test helpers.
When we use require("tests.config")
in the test, it works perfectly if we remove the vim.deepcopy()
from this line in helpers.lua
:
local test_config = vim.deepcopy(require("tests.config")) -- Creates a separate copy
The problem is:
- The test modifies
require("tests.config")
directly to add the dynamic tool - But
setup_chat_buffer()
passes a copied version of that config to the agent - The agent's
refresh_tools()
method uses the copied config, which doesn't have the dynamic tool
When I remove vim.deepcopy()
, both the test and agent reference the same config object, so the dynamic tool addition is visible to refresh_tools()
.
However, I suspect the vim.deepcopy()
is there for test isolation, so removing it might affect other tests. That's why using require("codecompanion.config")
works - it bypasses this copy issue entirely by modifying the runtime config that the agent actually uses.
Would you prefer I:
- Remove
vim.deepcopy()
from the test helpers (might affect test isolation) - Keep using
require("codecompanion.config")
(simulates real-world dynamic tool addition) - Or find another approach?
The test is specifically validating that dynamically added tools are detected, which is a real-world scenario where tools would be added to the runtime config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Yes this is a super unique use case. Okay, let's stick with require("codecompanion.config")
but put a ton of comments to make sure no one deletes it 😆 . I'm talking:
--- DO NOT DELETE THIS ---
--- WE ARE USING THE REAL CONFIG AS PER #1693 ---
Or something like that. As far as the test goes, it's well out of the way of the blast radius of most changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Thanks @ravitemer sorry it took me so long to get to this. Couple of comments but I can make this a priority |
This PR primarily aims to highlight a specific issue I encountered with dynamic tool availability in chat agents. There is a disconnect between dynamically added tools and chat agents that causes tools to appear available but fail during execution.
Scenario Flow
tools_config = ToolFilter.filter_enabled_tools(config.strategies.chat.tools)
require('codecompanion.config').strategies.chat.tools.my_new_tool = { ... }
@my_new_tool
→ Tool appears in completion menu (becausecompletion.tools()
callsToolFilter.filter_enabled_tools()
fresh each time)@my_new_tool
→ Tool is highlighted in chat buffer as expectedagents:find()
uses the cachedself.tools_config
which doesn't include the new toolThe issue seems to stem from two different data sources:
config.strategies.chat.tools
viaToolFilter.filter_enabled_tools()
self.tools_config
from agent creation timeAdditionally,
ToolFilter
's cache doesn't appear to detect config changes, so even callingfilter_enabled_tools()
again returns stale data within the cache TTL.Changes Made
I tried to address this by:
ToolFilter
- attempts to detect when tools are added/removed and invalidate cacherefresh_tools()
method to update agent's tools configInstead of adding
refresh_tools
in the chat:submit() another way could be to change everyself.tools_config
in theagents/init.lua
withToolFilter.filter_enabled_tools
or such equivalent.self.tools_config
is in so many places in the flow like parsing, finding, replacing each have to go though the cache check. Sorefresh_tools()
just before submitting a chat seemed the right way to go for me.Related Issue(s)
Screenshots
Checklist
CodeCompanion.has
in the init.lua file for my new featuremake all
to ensure docs are generated, tests pass and my formatting is applied