ai: honour response_cache for local reviews - #493
Open
shardulsdk-mpiric wants to merge 1 commit into
Open
shardulsdk-mpiric wants to merge 1 commit into
shardulsdk-mpiric wants to merge 1 commit into
Conversation
AiSettings carries response_cache, but only the daemon ever consulted it. sashiko review builds its provider with create_provider_from_ai, which is uncached, so setting response_cache did nothing on that path and gave no sign that it had not taken effect. create_provider_cached cannot be used there. It derives the cache location from settings.database.url, and a local review holds AiSettings alone with no database beside it. Add create_provider_cached_from_ai, which wraps the provider when response_cache is set and places the cache under the XDG data directory already used for the prompt bundle, overridable with SASHIKO_RESPONSE_CACHE. The helper that resolves that directory is shared rather than copied. response_cache still defaults to false, so nothing changes unless it is switched on. This matters most when a review is retried. A run that fails part way re-issues every earlier call, and without a cache each of those is paid for again. Assisted-by: Claude:claude-opus-5 Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
Contributor
|
I could use this! :) I think it should use a common method, though. This is kind of duplicating code from the daemon. What about consolidating it like attached? Also, I think the original PR suffered from creating two caches since the stdio workers under the daemon would end up creating the non-daemon cache location. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AiSettingscarriesresponse_cache, but only the daemon ever consultedit.
sashiko reviewbuilds its provider withcreate_provider_from_ai,which is uncached, so turning the setting on did nothing on that path and
there was nothing in the output to say so.
create_provider_cachedcannot be reused there, since it derives the cachelocation from
settings.database.urland a local review holdsAiSettingsalone with no database beside it. This adds
create_provider_cached_from_ai,which places the cache under the XDG data directory already used for the
prompt bundle and lets
SASHIKO_RESPONSE_CACHEoverride it. The helper thatresolves that directory is now shared rather than copied.
response_cachestill defaults to false, so nothing changes unless it is switched on.
It matters most on a retry. A run that fails part way re-issues every
earlier call, and with no cache each of those is paid for again. One such
retry after a late-stage failure cost me $3.33 against a hosted endpoint,
which is what sent me looking at this.
make check-allpasses, and there is a test for the path resolutioncovering both the environment override and the XDG default. I am relying on
#480 having made the cache key safe across provider configurations rather
than having exercised that myself.