fix: apply session scoping to all working-representation query paths#881
fix: apply session scoping to all working-representation query paths#881VVoruganti wants to merge 2 commits into
Conversation
session_name was only applied to the recent-documents query in RepresentationManager; the semantic and most-derived paths ignored it, so limit_to_session leaked cross-session conclusions into perspectives. - Thread a session allowlist (session_names) uniformly through all three query paths; pushed down to pgvector and external vector stores - Accept a list so the upcoming session-allowlist API reuses this path - Fail closed on an empty allowlist (downstream stores drop empty IN clauses, which would silently widen scope) Fixes DEV-1994 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
{"session_id": ["s1", "s2"]} is now shorthand for
{"session_id": {"in": [...]}} on regular columns, generically
(peer_id, etc.). JSONB metadata columns are excluded — a bare list
there keeps JSONB containment semantics, unchanged.
Previously a bare list on a regular column compiled to a type-mismatched
equality that matched nothing, so this is strictly additive.
Also translates the same shape in the turbopuffer/lancedb filter
builders, and fixes lancedb dropping empty IN clauses (fail-open) —
an empty membership list now emits an always-false condition.
Groundwork for DEV-1995 (session allowlist via the existing filters
DSL, no new API params)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThis PR replaces the single-session ChangesSession Allowlist Scoping and Filter Membership Sugar
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Router
participant RepresentationManager
participant QueryHelpers
participant VectorStoreOrDB
Router->>RepresentationManager: get_working_representation(session_names)
alt session_names == []
RepresentationManager-->>Router: empty Representation (fail closed)
else session_names is None or non-empty list
RepresentationManager->>QueryHelpers: query with session_names
QueryHelpers->>VectorStoreOrDB: filter session_name IN (session_names)
alt IN list empty
VectorStoreOrDB-->>QueryHelpers: no rows (1=0)
else
VectorStoreOrDB-->>QueryHelpers: matching documents
end
QueryHelpers-->>RepresentationManager: documents
RepresentationManager-->>Router: Representation
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/filter.py (1)
242-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the JSONB column tuple to a shared constant.
The
("h_metadata", "configuration", "internal_metadata")tuple is now duplicated a third time in this function (also at lines 262 and 267). Extracting it into a module-level constant would reduce drift risk if a JSONB column is renamed or added.♻️ Proposed refactor
+JSONB_COLUMN_NAMES = ("h_metadata", "configuration", "internal_metadata") + def _build_field_condition( key: str, value: Any, model_class: type[Any] ) -> ColumnElement[bool] | None: ... - if isinstance(value, list | tuple | set) and column_name not in ( - "h_metadata", - "configuration", - "internal_metadata", - ): + if isinstance(value, list | tuple | set) and column_name not in JSONB_COLUMN_NAMES: value = {"in": list(typing_cast(Sequence[Any], value))}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/filter.py` around lines 242 - 251, The JSONB column names tuple is duplicated in the filter logic, including this bare-list check in src/utils/filter.py. Extract the repeated ("h_metadata", "configuration", "internal_metadata") values into a module-level constant and update this branch plus the other uses in the same function to reference that shared constant, keeping the behavior in the relevant filter helpers unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/crud/representation.py`:
- Around line 522-543: _build_filter_conditions currently treats an empty
session_names list like no filter at all because it uses a truthiness check,
which breaks the fail-closed behavior. Update the session_names branch in
_build_filter_conditions to distinguish None from an explicit empty list,
matching the behavior used by _query_documents_recent and
_query_documents_most_derived so empty allowlists still produce a closed filter
instead of unscoped results. Keep the existing caller invariant comment aligned
with this behavior and ensure the session_name filter is always emitted whenever
session_names is provided, even if it is empty.
---
Nitpick comments:
In `@src/utils/filter.py`:
- Around line 242-251: The JSONB column names tuple is duplicated in the filter
logic, including this bare-list check in src/utils/filter.py. Extract the
repeated ("h_metadata", "configuration", "internal_metadata") values into a
module-level constant and update this branch plus the other uses in the same
function to reference that shared constant, keeping the behavior in the relevant
filter helpers unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43040f2b-446b-40c0-b12f-902cecdd88b1
📒 Files selected for processing (8)
src/crud/representation.pysrc/routers/peers.pysrc/routers/sessions.pysrc/utils/filter.pysrc/vector_store/lancedb.pysrc/vector_store/turbopuffer.pytests/crud/test_representation_manager.pytests/test_advanced_filters.py
| def _build_filter_conditions( | ||
| self, | ||
| level: str | None = None, | ||
| session_names: list[str] | None = None, | ||
| ) -> dict[str, Any]: | ||
| """ | ||
| Build filter conditions for document queries. | ||
|
|
||
| Returns a flat dict of key-value pairs for vector store filtering. | ||
| Callers must not pass an empty session_names list — empty allowlists | ||
| fail closed before any query is issued (see | ||
| _get_working_representation_internal). | ||
| """ | ||
| filters: dict[str, Any] = {} | ||
|
|
||
| if level: | ||
| filters["level"] = level | ||
|
|
||
| if session_names: | ||
| filters["session_name"] = {"in": session_names} | ||
|
|
||
| return filters |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
_build_filter_conditions silently drops the fail-closed guarantee for empty allowlists.
_query_documents_recent/_query_documents_most_derived use session_names is not None, so an explicit empty list correctly produces .in_([]) (SQLAlchemy renders this as an always-false predicate). But _build_filter_conditions uses a truthiness check (if session_names:), so an empty list is treated identically to None — it silently omits the session_name filter and returns unscoped results instead of failing closed. The comment at line 531-534 documents this as a caller invariant ("Callers must not pass an empty session_names list"), but that invariant is only enforced by the guard in _get_working_representation_internal, one layer up. Any future direct caller of _query_documents_semantic/_query_documents_for_level/_build_filter_conditions (e.g. the "upcoming session-allowlist API" mentioned in the PR objectives) that bypasses that guard would silently widen scope rather than fail closed — the opposite of the other two query paths' behavior.
Making the check is not None here would be consistent with the other two paths and let the already-planned fail-closed handling in the vector-store filter builders (per PR objectives) take over.
🔒 Proposed fix for consistent fail-closed semantics
- if session_names:
+ if session_names is not None:
filters["session_name"] = {"in": session_names}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _build_filter_conditions( | |
| self, | |
| level: str | None = None, | |
| session_names: list[str] | None = None, | |
| ) -> dict[str, Any]: | |
| """ | |
| Build filter conditions for document queries. | |
| Returns a flat dict of key-value pairs for vector store filtering. | |
| Callers must not pass an empty session_names list — empty allowlists | |
| fail closed before any query is issued (see | |
| _get_working_representation_internal). | |
| """ | |
| filters: dict[str, Any] = {} | |
| if level: | |
| filters["level"] = level | |
| if session_names: | |
| filters["session_name"] = {"in": session_names} | |
| return filters | |
| def _build_filter_conditions( | |
| self, | |
| level: str | None = None, | |
| session_names: list[str] | None = None, | |
| ) -> dict[str, Any]: | |
| """ | |
| Build filter conditions for document queries. | |
| Returns a flat dict of key-value pairs for vector store filtering. | |
| Callers must not pass an empty session_names list — empty allowlists | |
| fail closed before any query is issued (see | |
| _get_working_representation_internal). | |
| """ | |
| filters: dict[str, Any] = {} | |
| if level: | |
| filters["level"] = level | |
| if session_names is not None: | |
| filters["session_name"] = {"in": session_names} | |
| return filters |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/crud/representation.py` around lines 522 - 543, _build_filter_conditions
currently treats an empty session_names list like no filter at all because it
uses a truthiness check, which breaks the fail-closed behavior. Update the
session_names branch in _build_filter_conditions to distinguish None from an
explicit empty list, matching the behavior used by _query_documents_recent and
_query_documents_most_derived so empty allowlists still produce a closed filter
instead of unscoped results. Keep the existing caller invariant comment aligned
with this behavior and ensure the session_name filter is always emitted whenever
session_names is provided, even if it is empty.
There was a problem hiding this comment.
@adavyas, glad the fix makes sense. Whenever you apply the is not None check in _build_filter_conditions, that'll close the gap and keep it consistent with the fail-closed behavior already used in _query_documents_recent and _query_documents_most_derived. Let me know if you'd like me to draft the full diff or open a follow-up issue to track it.
adavyas
left a comment
adavyas
left a comment
There was a problem hiding this comment.
looks pretty solid. just a few small wrinkles
| in_values = cast(Sequence[Any], value["in"]) | ||
| filter_list.append((key, "In", in_values)) | ||
| elif isinstance(value, list | tuple | set): | ||
| # Bare-list sugar: same membership semantics as {"in": [...]} |
There was a problem hiding this comment.
doesn't seem to handle the empty-list scenario as done in LanceDB with conditions.append("1 = 0"). this remains fail-open
| def _build_filter_conditions( | ||
| self, | ||
| level: str | None = None, | ||
| session_names: list[str] | None = None, | ||
| ) -> dict[str, Any]: | ||
| """ | ||
| Build filter conditions for document queries. | ||
|
|
||
| Returns a flat dict of key-value pairs for vector store filtering. | ||
| Callers must not pass an empty session_names list — empty allowlists | ||
| fail closed before any query is issued (see | ||
| _get_working_representation_internal). | ||
| """ | ||
| filters: dict[str, Any] = {} | ||
|
|
||
| if level: | ||
| filters["level"] = level | ||
|
|
||
| if session_names: | ||
| filters["session_name"] = {"in": session_names} | ||
|
|
||
| return filters |
| test_workspace, test_peer = sample_data | ||
| _, _, manager = await self._setup(db_session, test_workspace, test_peer) | ||
|
|
||
| representation = await manager.get_working_representation( |
There was a problem hiding this comment.
this test seems to be falsely passing because in src/crud/representation.py:
if session_names is not None and not session_names: return Representation()
seems to be returning an empty representation. we should probably call _build_filter_conditions directly
2 participants
Summary
session_namewas only applied to the recent-documents query inRepresentationManager; the semantic and most-derived paths ignored it, sosession.contextwithlimit_to_session=trueleaked cross-session conclusions into the perspective. The session restriction is now applied uniformly to all three query paths and pushed down to pgvector and external vector stores.session_names: list[str]) so the upcoming session-allowlist API (DEV-1995) reuses the same enforcement path. An empty allowlist fails closed — downstream stores drop emptyINclauses, which would otherwise silently widen scope.{"session_id": ["s1","s2"]}≡{"session_id": {"in": [...]}}on regular columns, generically (peer_id, etc.). JSONB metadata columns keep containment semantics unchanged. Previously a bare list compiled to a type-mismatched equality that matched nothing, so this is strictly additive. Same shape translated in the turbopuffer/lancedb filter builders, plus a fix for lancedb dropping empty IN clauses (fail-open → always-false).Behavior change
Results for
limit_to_session=trueget narrower (correct): cross-session and session-less (dream-produced) conclusions no longer appear. Needs a release-note entry framed as a bugfix.Tests
TestRepresentationManagerSessionScoping: per-path allowlist enforcement, NULL-session exclusion, end-to-end blend, empty-allowlist fail-closedtest_bare_list_membership_sugar: list≡in equivalence, empty-list fail-closed, JSONB containment semantics preservedtests/test_advanced_filters.py,tests/crud/test_representation_manager.py,tests/routes/test_sessions.py,tests/routes/test_peers.py(144 passed)Linear: DEV-1994 (part of DEV-1970 / Scopes RFC Phase 0)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes