Preference insights describe the adapters their own subject can see - #182
Merged
Conversation
Six witnesses against /v1/preferences/insights, two of them failing. The route always summarizes the authenticated principal, and inside summarize_preferences the events and the clusters are both read for that user. The adapter list alone is read with an unscoped list_artifacts. The store reads an unscoped artifact listing as a question about public visibility, deliberately: caller identity is what adds that caller's private rows, tenant identity is what adds their tenant's shared ones. So nothing leaked. The list collapsed to visibility='global' and never showed a user the per-user adapter their own feedback had just created. Five adapters around one subject rather than two, because owner_user_id alone turns the obvious red green while leaving this call one argument short of _select_adapters, which serves adapters at turn time. Measured: with the listing unscoped, only the global adapter is returned; the subject's own private row and their tenant's shared row are both absent. Cases assert specific ids. "The list is non-empty" would pass against a fix that returned every adapter on the instance, which is the one outcome worse than showing none, so the two negatives are paired with the two positives. The subject's own adapter is created by ensure_user_adapter rather than by hand, and the other four copy that artifact's schema, so a change to the adapter shape cannot leave this file asserting against a shape the product no longer creates.
summarize_preferences reads events and clusters for its subject; the adapter list alone was read with no identity at all. An unscoped artifact listing is a question about public visibility, so the clause collapsed to visibility='global' and the panel never showed a user the per-user adapter their own feedback had just created. It leaked nothing — it showed nobody their own. Both arguments, not one. owner_user_id adds the caller's private rows and tenant_id adds the ones their tenant shares, which is exactly what _select_adapters passes when a turn actually picks an adapter. Adding only the first would turn the reported symptom green and leave this call one argument short of the one that answers the same question at turn time. summarize_preferences(None) keeps the meaning it already had. The signature allows it, nothing in the product passes it, and with no identity to scope by the store's answer is the public set, so that is pinned rather than changed. Two mutations, each killed by exactly one witness: dropping owner_user_id kills the subject's-own-adapter case alone, dropping tenant_id kills the tenant-shared case alone. ensure_user_adapter already carried a comment describing this exact hazard, from an earlier fix at that call site. Grepping the shape rather than stopping at the reported line also finds it in clustering.promote_clusters, where an unscoped listing can leave a bound cluster looking unbound, and in the /metrics adapter gauge, which undercounts. Both run without a principal, so what scope they should have is a decision rather than an argument to add; they are recorded in ISSUES and left alone here.
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.
Found while qualifying the learning loop against a running instance. Feedback was recorded, a per-user adapter was created, a training job was opened — and
GET /v1/preferences/insightsreportedadapters: []. The adapter existed; the same run had just asserted one row inartifact.What was wrong
/v1/preferences/insightsis a user-scoped surface. The route always callssummarize_preferences(principal.user_id), and inside it the events and the clusters are both read for that user. The adapter list alone was read with an unscopedlist_artifacts(type_filter="adapter").The store treats an unscoped artifact listing as a question about public visibility, deliberately: caller identity is what adds that caller's private rows, and tenant identity is what adds the ones their tenant shares. With neither, the visibility clause collapses to
visibility = 'global'.So the panel never showed one user another user's adapter. It showed nobody their own:
The fail-safe direction is why it survived — an empty list looks like an account with no adapters yet.
The same hazard, already known one function away
ensure_user_adaptercarries this comment, from an earlier fix:The shape was understood and repaired at that call site.
summarize_preferencesis the sibling that was not searched for at the time.Two arguments, not one
Adding
owner_user_idalone turns the reported symptom green and leaves this call one argument short of_select_adapters, which answers the same question when a turn actually picks an adapter. A user whose tenant shares an adapter with them would still not see it in the panel that claims to describe their adapters.The invariant is therefore about agreement: preference insights describe the adapters visible to the same user whose preferences they summarize.
The two negatives matter as much as the positives. "The list is non-empty" would pass against a listing that returned every adapter on the instance, which is the one outcome worse than showing none — so every case asserts a specific adapter id.
summarize_preferences(None)keeps the meaning it already had. The signature allows it, nothing in the product passes it, and with no identity to scope by the store's answer is the public set — pinned by its own witness rather than changed.Verification
Six witnesses, two failing before the fix — the subject's own private row and their tenant's shared row. Measured: only the global adapter came back.
Two mutations, each killed by exactly one witness. Dropping
owner_user_idkills the subject's-own-adapter case and nothing else; droppingtenant_idkills the tenant-shared case and nothing else. That separation is what distinguishes this fix from the one-argument version of it.Lanes:
make test-xdist— 2875 passed, 27 skipped, 0 failed. CI's lint selection clean.The subject's adapter is created by
ensure_user_adapterrather than by hand, and the other four copy that artifact's schema, so a change to the adapter shape cannot leave the witness asserting against a shape the product no longer creates. The first attempt at this file did hand-write the schema and was rejected by artifact validation for three missing properties.Recorded, not fixed
Grepping for the shape rather than stopping at the reported line found two more unscoped adapter listings. Neither is this defect, and both raise a question this change does not answer — what scope does a caller with no user have?
clustering.promote_clustersbuilds its "already bound" set from an unscoped listing, so a cluster whose adapter is private or tenant-shared reads as unbound and the sweep can bind it again. It runs with no principal./metricsderivesliminallm_adapters_totalfrom the same listing, so the gauge counts global adapters only and reads zero on an instance whose adapters are all per-user personas.Both are in
docs/ISSUES.md. The common root is worth stating plainly, because it is what made all three easy to write and hard to see: an artifact listing with no identity is a question about the public set. It is a reasonable default and it fails quietly.🤖 Generated with Claude Code
https://claude.ai/code/session_01DQtPsg9YSUXaStGXyUjozA
Generated by Claude Code
Note
Overview
Preference insights (
GET /v1/preferences/insights) was user-scoped for events and clusters but listed adapters with an unscopedlist_artifacts(type_filter="adapter"), which the store treats as global-only visibility — so users sawadapters: []even after feedback created their private persona adapter (empty list, not cross-user leakage).The fix loads the subject user when
user_idis set and callslist_artifactswithowner_user_idandtenant_id, matching_select_adaptersat turn time: own private adapters, tenant-shared adapters, and globals; not neighbours' private or other tenants' shared rows.summarize_preferences(None)still returns the public adapter set only.SPEC.md now states this invariant normatively; docs/ISSUES.md records the defect, mutation witnesses, and related unscoped listings left unfixed elsewhere.
New integration tests in
tests/test_insights_adapter_visibility.pyassert per-adapter visibility (including negatives so “return everything” cannot pass).Reviewed by Cursor Bugbot for commit 7173487. Bugbot is set up for automated code reviews on this repo. Configure here.