Commit Graph

2 Commits

Author SHA1 Message Date
Vineeth Voruganti c9f836c7f6
fix(filter): make the filter DSL reject bad input instead of 500ing, and fix negation over unset fields (#947)
* fix(filter): reject unknown operator dicts on scalar columns

An unrecognized operator dict on a non-JSONB column (e.g.
{"session_id": {"operator": "null"}}) fell through to `column == value`,
binding a dict to a VARCHAR parameter. That compiles, then fails in the
driver at execute time with "cannot adapt type 'dict'" — an unhandled
500 for what is invalid input.

Raise FilterError (422) instead. The guard lives in the shared
_build_field_condition, so every route through apply_filter is covered.
It keys on the actual column type rather than the JSONB_COLUMNS name
list, so dict equality still works on JSONB columns reachable through
Document's raw-key fallback (e.g. source_ids), where the driver adapts
dicts fine.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore(filter): name psycopg explicitly in comment

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(filter): handle null operands and non-numeric columns in comparisons

Two defects in _build_comparison_conditions, both reachable from any
route that accepts filters:

1. A null operand hit float(None), raising TypeError where only
   ValueError was caught — an unhandled 500. A null operand is a null
   check, not a value comparison, so {"ne": null} now compiles to
   IS NOT NULL and the other operators reject null with a 422. Equality
   against null already produced IS NULL via _build_field_condition.

2. Numeric operators float()-cast on every column type, so a string
   inequality on a text column ({"session_id": {"ne": "abc"}}) was
   rejected as an invalid number. Coercion is now gated on the column
   actually being numeric; text columns compare as text. Numeric columns
   still validate, and TypeError is caught alongside ValueError.

Existing ne coverage only exercised the JSONB metadata path, which uses
_safe_numeric_cast and handles strings — the scalar column path was
untested. Adds cases for both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(filter): fail closed on unrecognized filter shapes

The filter body is arbitrary client JSON with no schema, so validation
was emergent: any shape the DSL didn't recognize surfaced as an
unhandled 500 from somewhere in SQLAlchemy or psycopg. Fixing individual
shapes doesn't converge — a fuzz over the DSL found five more families
beyond the three already fixed here:

  {"AND": [None]}                TypeError, non-dict in a logical list
  {"AND": [[]]}                  AttributeError on .items()
  {"session_id": {"gte": true}}  SQLAlchemy ArgumentError
  {"embedding": []}              NotImplementedError, no python_type
  {"session_id": {"ne": {...}}}  execute-time "cannot adapt type 'dict'"

Two generic guards instead:

1. Any operand bound to a non-JSONB column must be a scalar, checked
   element-wise for `in`. A dict or list bound to a scalar column
   compiles cleanly and only fails in the driver at execute time, so it
   has to be rejected during construction. JSONB columns are exempt —
   a dict there is a containment match.

2. apply_filter fails closed: FilterError propagates, anything else is
   logged with logger.exception (filter shape included) and re-raised as
   FilterError. Unknown filter failures become 422s while staying fully
   visible as errors rather than being swallowed.

Adds two invariant tests over a generated matrix of filter shapes: every
shape either compiles or raises FilterError, and no non-scalar is ever
bound to a scalar column. Both fail without the guards above. They cover
shapes nobody enumerated, so the next unimagined body fails in CI rather
than in production.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore: clear the two remaining basedpyright warnings

`uv run basedpyright tests/ src/` reported two warnings in files that
predate this change. The pre-commit hook is file-scoped, so neither was
visible unless the owning file was touched.

- src/vector_store/__init__.py: join the lancedb error message with
  explicit `+` instead of adjacent literals (reportImplicitStringConcatenation).
- tests/test_cache_redaction.py: the test covers a private helper
  deliberately, so annotate the import (reportPrivateUsage).

No behavior change; whole-tree check is now clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(filter): keep numeric operands exact instead of coercing to float

float() rounds any integer past 2**53 and flattens a Decimal, so
{"token_count": {"gt": 9007199254740993}} silently compared against
9007199254740992 — a different row set than the client asked for.

_coerce_numeric passes already-numeric operands through untouched and
only parses strings, trying int() before float() so "5" stays exact
while "5.5" still parses. bool narrows to int: it is an int subclass,
but binding it as a boolean against a numeric column produces SQL
Postgres has no operator for.

Not coerced to the column's own type: int(5.5) would turn
{"token_count": {"lt": 5.5}} into `lt 5`, changing which rows match.

Also fixes a vacuous assertion in test_dict_on_jsonb_column_still_works.
It checked for "internal_metadata" in the whole statement, but that name
is in the SELECT projection either way, so the test passed even when no
WHERE clause was applied. Now asserts on stmt.whereclause and that the
filter payload is actually bound.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(filter): bind boolean columns as booleans, numerics without a cast

Boolean columns were treated as numeric because bool subclasses int, so
`{"is_active": {"ne": true}}` coerced true to 1 and Postgres rejected
`boolean <> integer` at execute time. Confirmed against a live database:
every form except bare equality with a native boolean was a 500.

Boolean columns now get their own branch: native true/false bind as
booleans, and any other operand is a FilterError. SQLAlchemy types the
bind from the operand rather than the column, so "true" renders
`is_active = %(param)s::VARCHAR` and Postgres has no such operator — a
422 is the honest answer. String booleans have never worked, are absent
from the docs (every documented boolean is inside metadata, which is
JSONB containment and unaffected), and produced no Sentry events in 90
days, so nothing can depend on the current behavior.

Also corrects the previous commit. Coercing operands to exact ints made
SQLAlchemy render an ::INTEGER cast, so any value past int4 — not 2**53
— started failing with "integer out of range" where float() had silently
compared as a double. Decimal keeps the value exact and renders no cast,
matching what float() did. The `in` branch never went through coercion
at all, so {"token_count": {"in": [1, 2147483648]}} was a 500 before
this PR too; it now takes the same path.

Verified end to end against the live database, not just at compile time.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(filter): coerce every operand against its column's type in one place

The DSL had two operand paths with different rules. Comparison operators
parsed datetimes and coerced numbers; bare equality bound whatever it was
handed. SQLAlchemy types a bind from the operand rather than the column
and the psycopg dialect renders that type as an explicit cast, so a
mismatch compiled into valid-looking SQL and failed at execute time:
"operator does not exist: timestamp with time zone = character varying".

A matrix of column type x operand type x operator against a live
database found 462 combinations, of which 55 built cleanly and then
failed. The most plausible was a filter someone would write first try:

    {"created_at": "2026-01-01"}          500
    {"created_at": {"gte": "2026-01-01"}} worked

_coerce_operand now handles every operand, whatever the operator, keyed
on the column's real type: JSONB takes an object, boolean takes only
true/false, datetime parses strings, numeric goes through _coerce_numeric,
text requires a string, and a column with no python_type (pgvector) is
not filterable. eq/ne/gt/in cannot drift apart because they share the
one call; `in` coerces element-wise, since a single element's type
decides the cast rendered for that parameter. The matrix is now clean.

This is a net deletion: the separate datetime, numeric, in-datetime and
boolean branches, plus _require_bindable_operand, all collapse into it.

Two more execute-time failures fixed on the way. `contains` was keyed on
column_name == "h_metadata", so Document's equally-JSONB
internal_metadata fell through to ILIKE and produced `jsonb ~~* text`;
it now keys on the column type. And {"source_ids": "abc"} was
`jsonb = character varying`.

Closed-set columns are validated against the Literal that defines them,
so declaring a new level or sync state updates filter validation with no
change here. {"level": "banana"} was silently matching nothing.

Empty IN is now always applied rather than skipped. Unifying the branches
inherited a guard that had only ever wrapped the datetime path, which
dropped the condition entirely and widened the query to every row —
fail-open on an empty allowlist, which session scoping relies on to fail
closed (see extract_session_allowlist). Caught by an existing test that
asserts returned rows; the fuzz and the type matrix only check for
errors, so neither would have seen it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(filter): make NOT and ne include rows where the field is unset

NOT (col = v) and col <> v are NULL when col is NULL, so negation
dropped rows whose column is unset — a conclusion with no session is
not "some other session", but was excluded anyway. IS NOT TRUE and
IS DISTINCT FROM behave identically when no NULL is involved.

Adds a row-count test, since this class of bug builds and executes
cleanly, and documents {"ne": null} for excluding unset fields.

* fix(filter): reject session ids that can't name a real session

extract_session_allowlist accepted any non-empty string, so "*" reached
the three consumers of the allowlist — direct IN, the filter DSL, and a
Python membership test — which disagree about it. The DSL reads "*" as
"drop the condition" and matches every session; the others treat it as a
literal name and match none. One /chat request could have some recall
sources unscoped and others scoped to nothing.

Entries are now validated against RESOURCE_NAME_PATTERN, the same pattern
the API requires of session ids, so no session could ever be named "*"
anyway. Wildcards were never part of this endpoint's documented contract
(an id, a list of ids, or {"in": [...]}), and a wildcard alongside a
top-level session_id already 422'd via must_include.

* fix(filter): treat a bare null operand as a null check

Routing every operand through _coerce_operand made bare `None` a type
error rather than a null check, so {"session_id": null} raised FilterError
where it previously built IS NULL: _build_field_condition used to end in
`column == value`, which SQLAlchemy renders as IS NULL. Confirmed 422 on
all five column families (text, numeric, boolean, datetime, JSONB).

Nothing caught it. The docs added in this branch promise
`{"session_id": None}` matches unset rows, the comment in
_build_comparison_conditions claimed the equality path already covered it,
and the DSL-wide invariant test accepts "compiles OR raises FilterError",
so a 422 passed. _coerce_operand's docstring already stated the contract
its caller wasn't honoring — "Callers handle None (a null check) and `*`
(a wildcard) before calling" — so the guard restores that rather than
adding a new rule.

The three null forms now agree: {"col": null} is IS NULL, {"col": {"ne":
null}} is IS NOT NULL, NOT [{"col": null}] is (IS NULL) IS NOT true.

Also from review of #947:

- Log filter keys, not the body. That log line is new in this branch and
  operands carry peer/session ids and free-text `contains` values; the
  traceback plus the entry shape is what locates a builder bug.
- Assert whereclause in the _where test helper, so a dropped condition
  fails instead of returning the whole statement to substring-match.
- Cover the raw-key JSONB path via source_ids, a JSONB column outside
  JSONB_COLUMNS reachable through Document's raw-key fallback.
- Drop the orphaned comment left above ENUM_COLUMN_VALUES when
  _coerce_operand replaced SCALAR_OPERAND_TYPES.
- Document that a JSONB column takes an object bare or under `contains`
  and nothing else. Bare {"metadata": X} is containment, so the `ne` this
  branch removed was never its inverse: a row with {"status":"done","x":1}
  satisfied both it and {"metadata": {"status":"done"}}. Per-key operators
  and NOT cover the real intents.
- Rewrite "Negation and Unset Fields" to lead with the operator rule and a
  truth table, forward-linking to Filtering Conclusions instead of using
  conclusions ~525 lines before they are introduced. A conclusion's
  session_id is the only nullable documented filterable field, verified
  across Message/Document/Session/Peer/Workspace.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-13 10:39:19 -04:00
Vineeth Voruganti e7cbcc8432
feat: session allowlist on dialectic and representation via filters (#882)
* fix: apply session scoping to all working-representation query paths

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>

* feat: bare-list membership sugar in the filter DSL

{"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>

* feat: session allowlist on dialectic and representation via filters

Adds a constrained 'filters' body to peer.chat and /representation —
the same DSL search and conclusions already accept, supporting only the
session_id key (a session id, a bare list, or {"in": [...]}).
Unsupported keys and shapes are rejected with 422, never silently
ignored. Composes with session_id (must be included in the allowlist
when both are given). Capped at 1,000 sessions per request.

Enforcement is uniform at every recall chokepoint, fail-closed:
- dialectic prefetch + search_memory: conclusion recall restricted to
  the allowlist; dream docs (session_name IS NULL) excluded
- message tools (search/grep/date-range/temporal/context/history):
  strict intersection of allowlist and observer session membership
- get_reasoning_chain: unavailable under an allowlist (chains traverse
  provenance across sessions and cannot be scoped without leaking)
- empty allowlist short-circuits to empty results everywhere

Auth: workspace keys pass the allowlist as-given; peer-scoped JWTs must
be a member of every allowlisted session (403 otherwise), mirroring the
existing single-session check.

Fixes DEV-1995

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix: fail closed on empty session allowlist across all filter builders

Empty session allowlists relied solely on the early-return guard in
_get_working_representation_internal. The layers below it were
inconsistent, so a future direct caller (the DEV-1995 allowlist API)
would silently widen scope instead of failing closed:

- _build_filter_conditions used a truthiness check; an empty list was
  treated like None and dropped the filter. Now uses `is not None`,
  matching the recent/most-derived SQL paths.
- turbopuffer emitted a bare `In []` with undocumented (possibly
  fail-open) semantics. Now emits an explicit always-false predicate,
  mirroring lancedb's `1 = 0`.

Also extract the duplicated JSONB column tuple in filter.py to a
JSONB_COLUMNS constant.

Tests exercise each fail-closed guarantee at the layer it lives, rather
than masking it behind the early-return guard.

* fix: address tests

* fix(crud): fail closed when session_name is outside the allowlist

search/grep/history helpers scoped to a single session_name ignored the
session_names allowlist entirely — a caller could read a session the
allowlist forbids. The API routes guarded this with a 422, but the
dialectic tools call these CRUD functions directly and bypassed it.

Enforce it at the boundary: return [] when session_name is set and not in
the allowlist, across _semantic_search_messages (covers search_messages +
search_messages_temporal), grep_messages, get_messages_by_date_range,
get_recent_history, and get_observation_context.

Also rename the public param allowed_sessions -> session_names for
consistency with representation.py / chat.py / peers.py; the resolved
intersection keeps its distinct name allowed_session_names.

* fix: test

* fix(scopes): tighten and consolidate session allowlist per review

Addresses review feedback on the session allowlist (DEV-1995).

Behavior changes:

- Auth gate on peers.chat now uses active membership (left_at IS NULL)
  via get_peer_session_names(active_only=True), matching the adjacent
  is_peer_in_session check on options.session_id. Previously a peer that
  had left a session was denied when naming it directly but permitted
  when naming it in filters.session_id.
- Scoped conclusion recall is restricted to level == "explicit"
  (ALLOWLIST_SAFE_LEVELS). Dream-derived conclusions are stamped with a
  single session_name but synthesized across all sessions, so that stamp
  can't be scoped on. Applied at all four recall paths. Unscoped recall
  is unchanged. Follow-up to give conclusions an authoritative
  source-session set is tracked in DEV-2201.
- The allowlist gate checks `is not None` rather than truthiness, so
  filters={"session_id": []} reaches it instead of being skipped.

Refactors:

- New crud.message.resolve_session_scope replaces four near-identical
  copies of the allowlist-membership intersection. Returns
  (allowlist, deny) and never returns an empty list, so the None vs []
  distinction that external stores fail open on lives in one tested
  place. Takes db=None and opens its own short-lived session only when
  distinction that external stores fail open on lives in one tested
  place. Takes db=None and opens its own short-lived session only when
  an observer lookup is needed, preserving external-lookup-first
  ordering on the vector-store path.
- extract_session_allowlist takes must_include, collapsing the
  session_id-in-allowlist check duplicated across both peer routes.
- DialecticAgent._select_tools dedupes the two toolset-selection blocks
  and drops get_reasoning_chain under an allowlist, rather than paying
  for the schema plus a wasted turn to return a refusal.
- Rename session_names -> session_allowlist across crud, agent tools,
  dialectic and routes, to remove the one-character ambiguity with
  session_name. Internal only; the public filters.session_id surface is
  unchanged.

Docs:

- session_allowlist documented across all message and recall entry
  points, including the None / [] / populated contract.
- session_name marked deprecated for scoping. Not removed and not
  aliased: it also pins the query to one session, bypasses observer
  scoping, and drives session-history injection into the dialectic
  prompt, so it has no drop-in replacement.
- Note at the Document branch in utils/filter.py that the raw-key
  fallback is load-bearing for session scoping.

Tests: 20 -> 39 in tests/test_session_allowlist.py, covering the
peer-scoped JWT gate (member, non-member, left-session, workspace-key
bypass, empty allowlist), the resolve_session_scope tri-state including
the no-DB-checkout path, must_include, and the level narrowing.

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-28 10:55:02 -04:00