search

karthink / gptel

A simple LLM client for Emacs
GNU General Public License v3.0
1.03k stars 111 forks source link

Contexts persist in `gptel-context--alist` when removed in alternate fashion #332

Open daedsidog opened 1 week ago

daedsidog commented 1 week ago
  1. Add a context in a buffer
  2. Remove the context by invoking gptel-add at point
  3. gptel-context--alist has an empty context, causing bugs later on.

The problem lies in the fact that gptel-remove-context does no cleanup with the alist.

karthink commented 1 week ago

Talk about timing, I fixed this 10 minutes ago.

karthink commented 1 week ago

Also it doesn't cause any bugs in gptel's request, since this clean-up is done before the request is constructed. It's only the visual indicators (in the transient menu and now in the chat header too) that can be out of date as a result.

daedsidog commented 1 week ago

Talk about timing, I fixed this 10 minutes ago.

I'm on the latest master and I'm still getting it.

Also it doesn't cause any bugs in gptel's request, since this clean-up is done before the request is constructed. It's only the visual indicators (in the transient menu and now in the chat header too) that can be out of date as a result.

It does cause bugs when commandeering the context string generation functions, for example.

~I really think cleanup should be handled in the removal (unless there's a reason not to?)~ Actually, I see the issues with that. So, outside of commandeering the default string generation functions, is there any risk of the alist being polluted?

karthink commented 1 week ago

I'm on the latest master and I'm still getting it.

https://github.com/karthink/gptel/blob/master/gptel-context.el#L169

karthink commented 1 week ago

So, outside of commandeering the default string generation functions, is there any risk of the alist being polluted?

What do you mean by polluted?

karthink commented 1 week ago

I'm on the latest master and I'm still getting it.

https://github.com/karthink/gptel/blob/master/gptel-context.el#L169

This fix is incomplete, I guess I need this check in other places in this function too.

daedsidog commented 1 week ago

What do you mean by polluted?

Removing contexts programmatically leaves you with an alist of the form:

((#<buffer` main.c> #<overlay in no buffer>))

In such a case executing (gptel-context--string gptel-context--alist) results in an error.

karthink commented 1 week ago

In such a case executing (gptel-context--string gptel-context--alist) results in an error.

Ah, this is what you meant by "commandeering the string generation". I'll fix it, but it's a low priority for now since doing this the right way (via customizing gptel-context-wrap-function) should not cause any issues.