gptel-rewrite: Fix directive hook evaluation location

[aagit]

The gptel-rewrite-directive-hook was being evaluated in the wrong buffer context, causing (buffer-name) to return "gptel-prompt" instead of the actual buffer name.

The gptel-rewrite menu correctly displayed the prompt generated by the hook, making the issue not apparent during normal usage.

This was discovered while testing gptel-rewrite-ripgrep-edit, where the generic hooks fail to handle the ripgrep-edit format, making the incorrect hook evaluation apparent.

Fixes: 89d1b47 ("gptel: Switch to prompt-buffer centric parsing")

[karthink]

@aagit I think you've sent me emails with a couple of patches about a similar issue that I haven't got to yet.

Do you want me to ignore the emails and look at your PRs, or do the opposite?

[aagit]

You can ignore the email and only look at the PR. I did the PR after a few days because I wasn't sure if the git-send-email went to the gmail spam folder.

[aagit]

Hello,

I probably didn't flag this properly but this is a regression fix so it's technically higher priority than other optimizations. Without fixing this bug ripgrep-edit.el doesn't work, but it's not just ripgrep-edit.el that doesn't work: all other per-file prompts also are currently non functional including the default "programming" one, it's just more apparent with ripgrep-edit.

[karthink]

@aagit Two notes:

1. The right way to do this would be to replace gptel--rewrite-directive with (gptel--parse-directive gptel--rewrite-directive 'raw) instead of checking if it's a function here. (The directive can be more complex than a single function, see gptel--parse-directive)

2. The directive is expanded in the prompt construction buffer intentionally. This way presets that modify the rewrite directive will be dealing with the object they expect. For example:

(gptel-make-preset 'rewrite-extra
  :system (concat (funcall gptel--system-message)
                  "\nExtra rewrite instructions"))

If you now use @rewrite-extra in the rewrite message (entered at the minibuffer), it will cause an error when the preset is applied in the *gptel-prompt* buffer because gptel--system-message is not a function, gptel--rewrite-directive has already been evaluated to a string.

This is a niche case, but it gets in the way of building a clean mental model of what gptel is doing. On balance, it might be worth the trade-off -- I'll think about it for a bit longer before deciding if I should merge this.

[aagit]

Sorry, I didn't notice I could select oneshot presets with @preset-name in the prompt.

Another way I thought of was to switch back to the orig buffer during the callback execution... no idea if it's feasible but I though I should mention it just in case.

[karthink]

After some more thinking, it makes sense for the system message to always be parsed (eval'd if it's a function) in the original buffer -- if it's a string or list, it doesn't matter where it is parsed. And if it's a function, it needs to have access to the local state (the current buffer, in your case).

Applying presets via gptel-with-preset or the transient menu will continue to work fine. The only thing this breaks is oneshot presets where the user assumes the original system message (before the preset is applied) will be a function because they set it to a function. This is a special case that I need to avoid or at least document.

If I make this change I'll do it in gptel-request, since this problem is not unique to gptel-rewrite.

[karthink]

I've had more time to ponder this behavior now, and I think the correct thing to do is to parse the directive at the time of request, in the context in which gptel-request was invoked. So I'll make this change to gptel-request soon.

[karthink]

It's just a little tricky to do correctly since the directive can take many forms -- a string, list, or a function that can return a string or a list. I'll get to it when I can.

[aagit]

Hello, what's the status of the fix? I think the gptel-rewrite-directives-hook working again is more important than the @preset feature I didn't know about. Perhaps you shall consider dropping the @preset feature if it made things too complicated that hard to maintain. Overall I don't think mixing metadata that has side effect with the prompt is particularly clean interface. Selecting the preset with ALT+RET+@ may actually be faster with fewer keypress and less prone to a typo and then it's also applicable per buffer or globally, not just oneshot.

[karthink]

The status of the fix is that I've tried twice to move the directive expansion to the prompt buffer in gptel-request, and failed both times because of subtle issues. As I expected, it's been surprisingly tricky to get it right without edge cases.

The @preset issue is not a concern for now.

[aagit]

Thanks for the update!

I may have misunderstood, I thought the cons of evaluating the gptel--rewrite-directive before switching buffer was purely that @rewrite-extra couldn't be used in the minibuffer anymore, which again looks like a "dubious" feature as it's slower and more error prone to type than the alternative.