Skip to content
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

feat(chat): Adding buffer watcher #610

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

bassamsdata
Copy link
Contributor

@bassamsdata bassamsdata commented Jan 7, 2025

Description

This PR introduces a buffer watching mechanism to improve how CodeCompanion handles code context in conversations. Instead of resending entire pinned buffers with each message (which is token-intensive), the watcher tracks and reports only the changes made to referenced buffers.

This is a work in progress PR that aims to optimize token usage and provide better context management in the chat stack.

If the idea of this PR is good and can be merged then I can continue working on it.

TODO

  • Add pre-PR changes to include line numbers in buffer slash commands for LLM context
  • Refactor the buffer watcher mechanism for more robust change tracking
  • Add test coverage
  • Update documentation to reflect new features
  • Modify the prompts to prepare the llm for watcher. (smaller llm needs example of what to receive). it needs more thinking and suitable separate PR.

Known Issues

  • Change detection becomes inconsistent (just sometimes) after multiple message exchanges (3rd/4th message). Need to verify if the change tracking logic in chat/init.lua is properly placed and triggered.
  • issue that the reference gets removed from the message after like the third response.
  • watcher now is better, but still register send the last changes only every time, leaving some changes unreported.
  • When adding a buffer watcher for the first time or stopping the watcher for a buffer, the buffer is printed in the chat as a duplicate. I’m unsure why this happens. Need help here (see video).
  • Occasionally, after a long conversation, I receive a warning message stating, “No messages to submit.” I’m not certain why this occurs or if it’s even related to this PR. IT LOOKS like it doesn't happen any more.

Related Issue(s)

#575
Currently, pinned buffers, while useful for maintaining context, are inefficient in terms of API token usage as they resend the same code repeatedly. This PR addresses this by implementing a change-tracking system that only sends buffer modifications to the LLM, significantly reducing token consumption while maintaining context awareness.

Technical Details

  • Implements a buffer watcher system in watcher.lua
  • Integrates with existing reference management
  • Tracks and reports buffer changes between chat interactions
  • Optimizes token usage by sending only modified content

Screenshots

Screen.Recording.2025-01-07.at.12.00.52.PM.mov

Checklist

  • I've read the CONTRIBUTING guidelines and have adhered to them in this PR
  • I've updated the README
  • I've ran the make docs command

@olimorris
Copy link
Owner

olimorris commented Jan 7, 2025

I am very excited about this. No pressure at all though. Appreciate this will require a tonne of work.

I would very much like this to replace pins in the future...watcher feels more intuitive as well.

One thing to think about as you're testing this is a prompt playbook that we can use to test against various models. I expect this will be fine with an gpt-4 or claude-sonnet but could be more challenging for self-hosted LLMs. I'm thinking out loud here but maybe for some models, you can specify to just continue to send the whole buffer with every response (after all, token consumption doesn't matter too much for those users).

Awesome job so far and I'll enjoy checking it out later in the week.

@olimorris
Copy link
Owner

olimorris commented Jan 7, 2025

[x] Add pre-PR changes to include line numbers in buffer slash commands for LLM context

I added this at the weekend btw.

@bassamsdata
Copy link
Contributor Author

I am very excited about this. No pressure at all though. Appreciate this will require a tonne of work.

Thank you for the nice words! indeed, especially for the tracking mechanism.

I expect this will be fine with a GPT-4 or Claude-Sonnet.

Yes, with Sonnet 3.5 I've had no issues at all. It's smart enough to keep track of line numbers.

I'm thinking out loud here but maybe for some models, you can specify to just continue to send the whole buffer with every response (after all, token consumption doesn't matter too much for those users).

I've tested with some 32B models on huggingface. It worked when I tell the LLM to track the line numbers with an exact example. I think some prompts after this will be changed a bit to include some examples when we finalize the tracking mechanism so the LLM can know exactly what to expect.

I'm building a test to send the same prompts automatically to multiple LLMs since I usually work with Hugging Face models, and they have all kinds of LLMs that can work locally.

If we succeed without sending the whole buffer again and again, we can benefit from not eating up the context window of the LLM, especially when smaller local LLMs have smaller context windows.

@bassamsdata
Copy link
Contributor Author

I added this at the weekend btw.

Ah, I hadn’t noticed it since I started working from the local branch the other two days. Thanks a bunch! One todo item is gone.

timestamp = vim.loop.now(),
reported = false,
})
log:debug("Recording change in buffer %d: lines %d-%d: %s", buf, start_row + 1, end_row + 1, vim.inspect(lines))
Copy link
Owner

Choose a reason for hiding this comment

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

Might be worth changing this line to log:trace as it will be writing to the log all the time. I ask users to turn logging to debug in their config when reporting an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, I’ve added extensive debug logging to monitor the process and changes. However, in the final version, it will be reduced to log:trace.

@olimorris
Copy link
Owner

Btw, great move to use vim.api.nvim_buf_get_changedtick(bufnr). I always forget that exists!

@olimorris
Copy link
Owner

Regarding the latter issue, is it a storage issue? I.e Neovim is only sharing the last change versus all of the changes since a point in time?

@bassamsdata
Copy link
Contributor Author

I don't think that's the case, The consolidation logic is too aggressive on purpose.
it's only keeping the last change for a line range. so basically sometimes multiple changes happen in one range, if line 2 changed and then line 3 changed , then the consolidate will send line 3 only.

I’m considering a different approach to keep watching lines only and then capture all modified lines that changed when FocusLost event on the buffer, or maybe a less aggressive consolidation mechanism.

@bassamsdata
Copy link
Contributor Author

Current State of the PR:
The watcher seems to be sending the correct data to the LLM. However, please ensure the watcher starts from the second message, not the first, where the buffer is added. (I haven’t tested this yet, so I’m unsure if it works correctly when the watcher starts immediately after adding the buffer.)

This PR is still incomplete, but the basic structure seems solid. Next steps include refining and modifying the prompts, cleaning up the code, and conducting thorough testing (both with written tests and LLM testing).

@olimorris
Copy link
Owner

Awesome. Plan on testing this properly at the weekend.

@olimorris
Copy link
Owner

Btw, wondered whether oil.nvim is a useful reference for keeping track of buffer changes?

@bassamsdata
Copy link
Contributor Author

When I first saw your comment, I was commuting and started wondering why oil.nvim would watch buffers (I don’t use it myself). Then it clicked, oil is a buffer, so of course it’s watching for changes!

Honestly, I’m glad I built the buffer watcher without knowing oil.nvim does something similar. It’s satisfying to create something independently without directly borrowing ideas from other plugins.

At first, I struggled with nvim_buf_attach() because handling multiline deletions was tricky, only a single line would register as deleted in Neovim. After some digging on Stack Exchange and Stack Overflow, I learned more about these quirks. My goal was to have the watcher gather detailed data and consolidate it for the LLM.

Last night, I decided to simplify things and ditched nvim_buf_attach(). Instead, I implemented a straightforward solution based on text differences. It turned out great, simple, robust, and capable of sending clear data to the LLM with distinct types like deletion, insertion, and modification. However, I didn’t have time to fully test it then but I was testing it with the llm.

Today, I wrote a lot of tests, including some edge cases I thought of, 13 tests in total spanning about 370 lines of code. Despite that, the core mechanism of the watcher remains simple, at around 130 lines. After all those tests, it’s proven to handle all those cases. I’m sure it’s not perfect, and users will likely uncover new edge cases, but that’s okay. I’m planning to add more edge cases to the tests as well.

@bassamsdata
Copy link
Contributor Author

I took a quick look at the oil.nvim implementation, the codebase is large and complex. From what I observed, it uses a different approach and incorporates several advanced features, such as:

  1. uv.new_fs_event() to monitor changes in the oil directory path.
  2. Handling events like file additions, deletions, or renames with multiple buffer checks.
  3. Using numerous Neovim events like TextChanged, CursorMoved, ModeChanged, InsertEnter, BufEnter, and BufUnload to track various changes.
  4. Frequent use of the buffer-local variable vim.bo.modified, which I found particularly interesting.
  5. Extensive use of timers. I experimented with timers in a more complex earlier version of the watcher, it worked well but didn’t seem strictly necessary.
  6. it doesn't seem to use changedtick buffer variable :(

Overall, the oil.nvim implementation is significantly more complex, working closely with system events and serving a different purpose. Personally, I tend to favor simpler, easier-to-maintain solutions unless there’s a substantial benefit to a more intricate approach.

That said, if you think the oil.nvim implementation would better suit this purpose, I can revisit the oil.nvim code to fully understand the mechanism. It might take some time to go through, as it’s a bit complex.

For context, I’m a big fan of Steven’s work and regularly use some of his other plugins like quicker, overseer, and conform. If we encounter edge cases that this watcher doesn’t handle, we can consider integrating some of those ideas (e.g., involving vim.bo.modified or certain Neovim events).
what do you think?

@bassamsdata
Copy link
Contributor Author

bassamsdata commented Jan 11, 2025

  • Models tested with complex mdofications:
    • GPT4o - PASS
    • sonnet 3.5 - PASS
    • Qwen2.5-coder-32B - PASS
    • Gemini 2 flash - PASS
    • llama-3.2-90b-vision-preview - PASS
    • llama 3.2 - 11b - PASS
    • mixtral 8x7b - PASS
    • GPT4o mini - NOT tested yet
    • haiku 3.5 - NOT tested yet

@GitMurf
Copy link
Contributor

GitMurf commented Jan 12, 2025

I have not reviewed any of the code. On a trip with the family. But I reviewed the conversation above. Thought maybe my 2cents in the form of a potentially "dumb" question may help as it is the first thing that came to mind when trying to think of a "simple" solution.

What if when a buffer is added to a chat it stores the last modified timestamp of that file? I know this may be an issue with handling temp buffers that are not tied to a timestamp but let's forget about that case for now.

Then you simply check each time you send a message to LLM to see if the last modified timestamp of the file has changed. If so, grab the new content, compare / diff the changes and send those changes.

Thoughts?


local allowed_pins = {
"<buf>",
"<file>",
}

local allowed_watches = {
Copy link
Owner

Choose a reason for hiding this comment

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

Think we should rename this to allowed_watchers

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

@@ -8,12 +8,17 @@ local config = require("codecompanion.config")
local api = vim.api
local user_role = config.strategies.chat.roles.user
local pinned_icon = config.display.chat.icons.pinned_buffer
local watched_icon = config.display.chat.icons.watched_buffer
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add these icons into a table then we can iterate over them to remove them on line 253

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

Just played around with gpt-4o-mini and it works perfectly. My playbook conversation:

user: #buffer What does this file do?
llm: This Lua file defines three functions: ...

user: [watches buffer] I've just changed the file. Can you tell what I've done?
llm: You have deleted the `hello_oli` function

user: @editor can you add it back?
llm: [Runs editor tool correctly]

user: Now what have I done?
llm: You have added a new function

user: Can you share with me what you think the file looks like now?
llm: [Shares correct output]

user: @editor can we change hello_oliver to hello_oli?
llm: [Does so correctly]

@bassamsdata
Copy link
Contributor Author

Then you simply check each time you send a message to LLM to see if the last modified timestamp of the file has changed. If so, grab the new content, compare / diff the changes and send those changes.

@GitMurf, well basically this final solution is doing something similar in the idea, so It maintains
a state of the buffer, recording the current content and last sent content so basically comparing 2 states. also oil.nvim is doing something similar, yet very sophisticated but it check the changes, via b:modified variable, and save the initial buffer into cache mechanism, then when saving, it locks the buffer to prevent any additional changes.

@bassamsdata
Copy link
Contributor Author

@olimorris, if there are no further comments, I believe this is ready to merge. Regarding the documentation, I was working with an older version of the README, so I wasn’t fully aware of the updated structure and the docs site, congratulations on that, by the way; it’s fantastic!

As for potential future improvements that could be addressed in separate PRs:

  1. I've already wrote docs for this feature, do you want it on the website as PR in usage/chat buffer section?
  2. We should provide an example to explain how changes will be sent to the LLM when the watcher is activated.
  3. I’m planning to implement a change limit. If the user modifies more than 70% to 80% of the buffer, it’s more efficient to send the updated buffer content instead of changes. This PR is nearly ready, just needs some final cleanup.

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