Skip to content

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

Merged
merged 10 commits into from
Jul 2, 2025

Conversation

ravitemer
Copy link
Contributor

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

  1. User opens a chat → Chat creates an Agent with tools_config = ToolFilter.filter_enabled_tools(config.strategies.chat.tools)
  2. Some plugin adds a tool dynamicallyrequire('codecompanion.config').strategies.chat.tools.my_new_tool = { ... }
  3. User types @my_new_tool → Tool appears in completion menu (because completion.tools() calls ToolFilter.filter_enabled_tools() fresh each time)
  4. User submits message with @my_new_tool → Tool is highlighted in chat buffer as expected
  5. Agent tries to process the toolagents:find() uses the cached self.tools_config which doesn't include the new tool
  6. Result: Tool appears available everywhere except where it matters - the agent can't actually find/execute it

The issue seems to stem from two different data sources:

  • Completion menu: Uses fresh config.strategies.chat.tools via ToolFilter.filter_enabled_tools()
  • Chat agents: Use a cached snapshot self.tools_config from agent creation time

Additionally, ToolFilter's cache doesn't appear to detect config changes, so even calling filter_enabled_tools() again returns stale data within the cache TTL.

Changes Made

I tried to address this by:

  1. Config hash-based cache invalidation in ToolFilter - attempts to detect when tools are added/removed and invalidate cache
  2. Agent tools refresh - adds refresh_tools() method to update agent's tools config
  3. Pre-submission refresh - calls refresh before chat submission to ensure agents have latest tools

Instead of adding refresh_tools in the chat:submit() another way could be to change every self.tools_config in the agents/init.lua with ToolFilter.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. So refresh_tools() just before submitting a chat seemed the right way to go for me.

Related Issue(s)

Screenshots

Checklist

  • I've read the contributing guidelines and have adhered to them in this PR
  • I've updated CodeCompanion.has in the init.lua file for my new feature
  • I've added test coverage for this fix/feature
  • I've updated the README and/or relevant docs pages
  • I've run make all to ensure docs are generated, tests pass and my formatting is applied

- 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.
@bn-twitek
Copy link

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)
Copy link
Owner

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

Copy link
Contributor Author

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")
Copy link
Owner

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?

Copy link
Contributor Author

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!

Copy link
Contributor Author

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.

Copy link
Owner

@olimorris olimorris Jun 30, 2025

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.

Copy link
Contributor Author

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:

  1. The test modifies require("tests.config") directly to add the dynamic tool
  2. But setup_chat_buffer() passes a copied version of that config to the agent
  3. 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:

  1. Remove vim.deepcopy() from the test helpers (might affect test isolation)
  2. Keep using require("codecompanion.config") (simulates real-world dynamic tool addition)
  3. 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.

Copy link
Owner

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@olimorris
Copy link
Owner

Thanks @ravitemer sorry it took me so long to get to this. Couple of comments but I can make this a priority

@olimorris olimorris merged commit 3a5f895 into olimorris:main Jul 2, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants