Conversation
mvanderlinde
left a comment
There was a problem hiding this comment.
Code change itself looks straightforward, but I did add a question about optional chaining. Approving with the caveat that tests are fixed (looks like a bunch are failing, I assume because the test key names don't start with gist.)
|
|
||
| function checkKeyForExpiry(key: string | null): unknown | null { | ||
| if (!key) return null; | ||
| if (!key || !key.startsWith('gist.')) return null; |
There was a problem hiding this comment.
Style: Could this be simplified to...
| if (!key || !key.startsWith('gist.')) return null; | |
| if (!key?.startsWith('gist.')) return null; |
There was a problem hiding this comment.
Yes! Or my preference if it's a single condition, key?.startsWith('gist.') !== true.
| if (now.getTime() > expiryTime.getTime()) { | ||
| clearKeyFromLocalStore(key); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Guard for non-gist keys doesn't prevent returning values
High Severity
The gist. prefix guard in checkKeyForExpiry only wraps the expiry-checking logic (lines 76–97), but return item.value on line 99 sits outside the guard. For any key that doesn't start with 'gist.' but exists in storage with an expiry (which is always the case since setKeyToLocalStore defaults to a 365-day expiry), the function skips the if block and falls through to return the stored value. The guard needs to return null early for non-gist. keys — e.g., if (!key.startsWith('gist.')) return null — instead of only wrapping the expiry logic.
Reviewed by Cursor Bugbot for commit eb7b7df. Configure here.
There was a problem hiding this comment.
This was an active choice to retain preexisting behavior. Will have a followup PR to explicitly restrict local storage functionality across various operations.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 766bb8e. Configure here.


Note
Medium Risk
Changes local/session storage cleanup behavior so expiry logic no longer runs for non-
gist.keys, which could leave stale entries if any callers rely on expiry for other prefixes. Scope is small but affects persistence/cleanup semantics.Overview
Adds a
gist.prefix guard to local/session storage expiry handling so onlygist.-namespaced keys are checked and auto-removed duringclearExpiredFromLocalStore()andgetKeyFromLocalStore().Updates tests to consistently use
gist.*keys and adds coverage ensuring non-gist.keys can still be read without being deleted.Reviewed by Cursor Bugbot for commit f257b03. Bugbot is set up for automated code reviews on this repo. Configure here.