fix(cat): the two prune deletes named the row but not its owner - #834
Merged
Conversation
Both prune paths in the Cat memory service deleted purely by `id`:
.from(CAT_MEMORIES).delete().in('id', ids)
Safe today, and only conditionally so — the ids come from a query already
filtered to this user, run through an RLS-scoped client. Neither condition is
stated by the code doing the deleting. Three other paths in this service
already use getAdminClient(), where RLS is not a backstop at all; hand one of
those to a prune and it deletes across users, silently, from a function whose
job is routine cleanup nobody watches.
Every other delete in these files already filters on user_id — including the
suppression-lifting one twenty lines above pruneIfNeeded. Both prunes now do
too.
Finding 13 named one of them. The second, in recordForgottenFacts, says "same
pattern as memory pruning" in its own comment and had inherited the gap along
with the pattern — which is why this adds check:user-scoped-deletes to verify
rather than making the same edit a third time later. The gate is deliberately
narrow: the Cat memory service, where every table is strictly per-user. Widening
it needs a per-table notion of ownership that does not exist yet, and a gate
that has to guess is a gate that gets muted.
Proven to fail before shipping: reverting either prune exits 1 naming the file
and line; the clean tree exits 0 over 6 deletes.
#563 finding 13. Verified latent, not live — the only callers
of both prune paths pass the request-scoped client.
73 Cat suites (1031 tests) and type-check green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018waGt1ieA9TjpscqrbrnGb
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.
Closes finding 13 of #563.
Both prune paths in the Cat memory service deleted purely by
id:Safe today — and only conditionally so. The ids come from a query already filtered to this user, run through an RLS-scoped client. Neither condition is stated by the code doing the deleting. Three other paths in this service already use
getAdminClient(), where RLS is not a backstop at all; hand one of those to a prune and it deletes across users, silently, from a function whose job is routine cleanup nobody watches.Every other delete in these files already filters on
user_id— including the suppression-lifting one twenty lines abovepruneIfNeeded. Both prunes now do too.A gate, because the finding named one and there were two
recordForgottenFactssays "same pattern as memory pruning" in its own comment, and had inherited the gap along with the pattern. That's why this addscheck:user-scoped-deletestoverifyrather than making the same edit a third time later.Deliberately narrow: the Cat memory service, where every table is strictly per-user. Widening it needs a per-table notion of ownership that doesn't exist yet, and a gate that has to guess is a gate that gets muted.
Proven to fail before shipping: reverting either prune exits 1 naming the file and line; the clean tree exits 0 over 6 deletes.
Severity, stated accurately
Latent, not live. I checked the callers:
extractAndStoreMemoriesandrememberFactsare both handed the request-scoped client. The admin client appears inchat-orchestrator.tsbut only formeterCreditUsage. So nothing crosses users today — this closes the door before someone changes the client, which is exactly when it would stop being noticeable.73 Cat suites (1031 tests) and
type-checkgreen.