From c9f836c7f65257aa05f1690d3508c7255c6b9bf7 Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:39:19 -0400 Subject: [PATCH 1/2] fix(filter): make the filter DSL reject bad input instead of 500ing, and fix negation over unset fields (#947) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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) * chore(filter): name psycopg explicitly in comment Co-Authored-By: Claude Opus 5 (1M context) * 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) * 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) * 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) * 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) * 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) * 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) * 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) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../features/advanced/using-filters.mdx | 136 ++++++ src/utils/filter.py | 314 ++++++++++--- tests/routes/test_conclusions.py | 78 ++++ tests/test_session_allowlist.py | 32 ++ tests/utils/test_filter.py | 412 ++++++++++++++++++ 5 files changed, 917 insertions(+), 55 deletions(-) create mode 100644 tests/utils/test_filter.py diff --git a/docs/v3/documentation/features/advanced/using-filters.mdx b/docs/v3/documentation/features/advanced/using-filters.mdx index 7d6caacd..4a1eefec 100644 --- a/docs/v3/documentation/features/advanced/using-filters.mdx +++ b/docs/v3/documentation/features/advanced/using-filters.mdx @@ -192,6 +192,90 @@ sessions = honcho.sessions(filters={ ``` +### Negation and Unset Fields + +A field can be unset, and negation has to account for it. `NOT` and the `ne` +comparison operator both **include** rows where the field has no value at all: a +field with no value is not the value you are excluding, so excluding that value +keeps the row. + +Positive conditions work the other way around. An unset field matches nothing, +so equality and `contains` never return those rows. To select them, filter on +`null` directly: + +| Filter | Rows where the field is unset | +| --- | --- | +| `{"field": "x"}`, `{"field": {"contains": "x"}}` | Excluded | +| `{"NOT": [{"field": "x"}]}`, `{"field": {"ne": "x"}}` | Included | +| `{"field": null}` | Only these | +| `{"field": {"ne": null}}` | Excluded — the field must have some value | + +Of the filterable fields, only a conclusion's `session_id` can be unset: a +conclusion drawn across a whole workspace belongs to no single session. Every +other field is always populated, so none of this affects filters on them. See +[Filtering Conclusions](#filtering-conclusions) for what conclusions are. + + +```python Python +# Every conclusion except the ones in this session — including +# workspace-level conclusions, which belong to no session at all +conclusions = peer.conclusions.list(filters={ + "NOT": [ + {"session_id": "session-123"} + ] +}) + +# Equivalent +conclusions = peer.conclusions.list(filters={ + "session_id": {"ne": "session-123"} +}) +``` + +```typescript TypeScript +(async () => { + // Every conclusion except the ones in this session — including + // workspace-level conclusions, which belong to no session at all + const conclusions = await peer.conclusions.list({ + filters: { NOT: [{ session_id: "session-123" }] } + }); + + // Equivalent + const same = await peer.conclusions.list({ + filters: { session_id: { ne: "session-123" } } + }); +})(); +``` + + +To exclude a value **and** require the field to be set, combine the two with +`AND`: + + +```python Python +# Excludes workspace-level conclusions and `session-123` +conclusions = peer.conclusions.list(filters={ + "AND": [ + {"session_id": {"ne": "session-123"}}, + {"session_id": {"ne": None}} + ] +}) +``` + +```typescript TypeScript +(async () => { + // Excludes workspace-level conclusions and `session-123` + const conclusions = await peer.conclusions.list({ + filters: { + AND: [ + { session_id: { ne: "session-123" } }, + { session_id: { ne: null } } + ] + } + }); +})(); +``` + + ### Combining Logical Operators Create sophisticated queries by combining different logical operators: @@ -700,6 +784,57 @@ bob_explicit = peer.conclusions_of("bob").list(filters={"level": "explicit"}) ``` +## Value Types + +A filter value has to be usable against the field it targets. Honcho validates +this before running the query and returns a `422` with an explanation when it +doesn't hold, rather than failing mid-query or quietly returning nothing. + +| Field | Accepts | +| --- | --- | +| Text — `peer_id`, `session_id`, `id`, `content` | Strings | +| Numeric — `token_count` | Numbers, or numeric strings like `"5"`. Exact for integers of any size | +| Timestamps — `created_at` | ISO 8601 strings such as `"2026-01-01"` or `"2026-01-01T12:00:00Z"` | +| Boolean — `is_active` | `true` / `false` | +| `metadata` | An object, matched by containment — bare or under `contains` | +| Fields with fixed values — `level` | One of the documented values | +| Any field | `null`, which matches rows where the field is unset | + +Three consequences worth knowing: + +- **Booleans must be real booleans.** `{"is_active": True}` filters; the string + `{"is_active": "true"}` is rejected. +- **Fixed-value fields are checked.** `{"level": "explicit"}` filters; + `{"level": "typo"}` is rejected instead of returning an empty list, so a + misspelling doesn't look like "no results". +- **`metadata` takes only the two shapes above** — bare, or under `contains`. + Comparison operators don't apply to the object as a whole, so + `{"metadata": {"ne": {...}}}` is rejected. To compare *within* metadata, put + the operator on the key — `{"metadata": {"status": {"ne": "done"}}}`. To negate + a match, wrap the whole condition in `NOT`. See + [Metadata Filtering](#metadata-filtering). + +For every field other than `metadata`, the same rules apply however the value is +wrapped — bare, under an operator, or inside an `in` list — so +`{"level": "explicit"}`, `{"level": {"ne": "explicit"}}` and +`{"level": {"in": ["explicit"]}}` all validate identically. + +An empty `in` list matches nothing: + + +```python Python +# Returns no results — an empty allowlist excludes everything +messages = session.messages(filters={"peer_id": {"in": []}}) +``` + +```typescript TypeScript +(async () => { + // Returns no results — an empty allowlist excludes everything + const messages = await session.messages({ filters: { peer_id: { in: [] } } }); +})(); +``` + + ## Scoping Recall to Sessions The [chat endpoint](/v3/documentation/features/chat) and the representation @@ -756,6 +891,7 @@ dropped filter here would widen recall instead of narrowing it. |------|----------| | Any key other than `session_id` | `422` | | A shape other than a string, a list of strings, or `{"in": [...]}` | `422` | +| An entry that isn't a well-formed session id — wildcards included | `422` | | More than 1,000 sessions | `422` | | `session_id` set alongside `filters` | The `session_id` must appear in the allowlist, else `422` | | An empty allowlist (`[]`) | Valid, and recalls nothing | diff --git a/src/utils/filter.py b/src/utils/filter.py index 8df3590c..1155d408 100644 --- a/src/utils/filter.py +++ b/src/utils/filter.py @@ -1,14 +1,19 @@ import datetime +import re from collections.abc import Callable, Sequence +from decimal import Decimal from logging import getLogger -from typing import Any, TypeVar +from typing import Any, TypeVar, get_args from typing import cast as typing_cast -from sqlalchemy import ColumnElement, Select, and_, case, cast, literal, not_, or_ +from sqlalchemy import ColumnElement, Select, and_, case, cast, literal, or_ +from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.types import Numeric from ..exceptions import FilterError +from ..schemas.api import RESOURCE_NAME_PATTERN from .formatting import ILIKE_ESCAPE_CHAR, escape_ilike_pattern, parse_datetime_iso +from .types import DocumentLevel, VectorSyncState logger = getLogger(__name__) @@ -63,6 +68,157 @@ ALLOWED_EXTERNAL_TO_INTERNAL_COLUMN_MAPPING_DOCUMENTS = { MAX_SESSION_ALLOWLIST_ENTRIES = 1000 +# Columns whose values come from a closed set. Derived from the Literal types +# themselves, so adding a level (e.g. "abduction") or a sync state updates +# filter validation with no change here — an unlisted value is a 422 rather +# than a filter that silently matches nothing. +ENUM_COLUMN_VALUES: dict[str, frozenset[str]] = { + "level": frozenset(get_args(DocumentLevel)), + "sync_state": frozenset(get_args(VectorSyncState)), +} + + +def _coerce_numeric(op_value: Any) -> float | Decimal: + """Validate a numeric operand without losing precision or overflowing. + + Integers become Decimal rather than staying int. SQLAlchemy types the bind + from the operand, so a plain int renders an ``::INTEGER`` cast and anything + past int4 fails at execute time with "integer out of range" — even when the + comparison itself is meaningful. Decimal renders no cast, matching what + float() used to do, but keeps the value exact: float() rounds any int past + 2**53 and would silently compare against a different number. + + bool is narrowed first because it subclasses int; binding it as a boolean + against a numeric column produces SQL Postgres has no operator for. + + Args: + op_value: The operand to validate. + + Returns: + The operand as an exact numeric value that binds without a cast. + + Raises: + ValueError, TypeError: If the operand is not numeric. Callers convert + these to FilterError. + """ + if isinstance(op_value, bool): + return Decimal(int(op_value)) + if isinstance(op_value, float): + return op_value + if isinstance(op_value, int | Decimal): + return Decimal(op_value) + try: + return Decimal(str(op_value)) + except ArithmeticError: + raise ValueError(f"not a number: {op_value!r}") from None + + +def _column_python_type(column: Any) -> type | None: + """Return a column's Python type, or None when it doesn't declare one. + + pgvector's Vector raises NotImplementedError rather than returning a type, + so this must not be called bare. + """ + try: + return typing_cast("type | None", column.type.python_type) + except (AttributeError, NotImplementedError): + return None + + +def _coerce_operand( + column: Any, column_name: str, value: Any, operator: str = "" +) -> Any: + """Return ``value`` ready to bind against ``column``, or raise FilterError. + + SQLAlchemy types a bind from the *operand*, not the column, and the psycopg + dialect renders that type as an explicit cast. So an operand whose type + doesn't match its column compiles into valid-looking SQL and then fails at + execute time — ``operator does not exist: text = integer``. Postgres will + not implicitly bridge these, so the mismatch has to be caught here. + + Every operand passes through this one function, whatever the operator, so + ``eq``/``ne``/``gt``/``in`` cannot drift apart by construction. Callers + handle None (a null check) and ``*`` (a wildcard) before calling. + + Args: + column: SQLAlchemy column object. + column_name: Internal column name, for error messages. + value: The operand to coerce. + operator: The comparison operator, or "" for bare equality. + + Returns: + The operand, coerced where a lossless coercion exists. + + Raises: + FilterError: If the operand cannot be bound to this column. + """ + # JSONB keeps containment semantics: the operand is a JSON document, not a + # scalar to compare. `jsonb >= 5` and `jsonb @> 'text'` have no operator. + if isinstance(column.type, JSONB): + if operator in ("", "contains") and isinstance(value, dict | list): + return typing_cast("Any", value) + raise FilterError( + f"Invalid filter for column '{column_name}': a JSONB column takes an object, optionally under 'contains'" + ) + + python_type = _column_python_type(column) + if python_type is None: + raise FilterError(f"Column '{column_name}' cannot be filtered on") + + # contains/icontains build an ILIKE pattern, so the operand is stringified + # and its own type doesn't matter — but the column must be text, or + # Postgres has no `~~` operator for it. + if operator in ("contains", "icontains"): + if python_type is not str: + raise FilterError( + f"Operator '{operator}' requires a text column, but '{column_name}' is {python_type.__name__}" + ) + return value + + # bool is checked before the numeric branch: it subclasses int, so a boolean + # column would otherwise have `true` coerced to 1, which Postgres rejects + # against a boolean column ("operator does not exist: boolean <> integer"). + if python_type is bool: + if isinstance(value, bool): + return value + raise FilterError( + f"Invalid value for column '{column_name}': expected true or false, got {type(value).__name__}" + ) + + if issubclass(python_type, datetime.datetime | datetime.date): + if isinstance(value, datetime.datetime | datetime.date): + return value + if isinstance(value, str): + validated = _validate_datetime_string(value) + if validated is None: + raise FilterError(f"Invalid datetime value: {value}") + return validated + raise FilterError( + f"Invalid value for column '{column_name}': expected a datetime, got {type(value).__name__}" + ) + + if issubclass(python_type, int | float | Decimal): + try: + return _coerce_numeric(value) + except (TypeError, ValueError): + raise FilterError( + f"Invalid numeric value: {value}. Expected a number, got {type(value).__name__}" + ) from None + + if python_type is str: + if not isinstance(value, str): + raise FilterError( + f"Invalid value for column '{column_name}': expected a string, got {type(value).__name__}" + ) + allowed = ENUM_COLUMN_VALUES.get(column_name) + if allowed is not None and value not in allowed: + raise FilterError( + f"Invalid value for column '{column_name}': {value!r}. Expected one of {sorted(allowed)}" + ) + return value + + raise FilterError(f"Column '{column_name}' cannot be filtered on") + def extract_session_allowlist( filters: dict[str, Any] | None, @@ -76,6 +232,11 @@ def extract_session_allowlist( FilterError (422) rather than being silently ignored — a dropped filter on these endpoints would widen recall scope. + Entries must be well-formed session ids. Wildcards are not part of this + subset: the DSL treats ``*`` as "match everything" while the non-DSL + consumers of the allowlist treat it as a literal name, so it is rejected + rather than meaning two things at once. + Args: filters: The raw ``filters`` body, or None. must_include: A session id that must appear in the parsed allowlist — @@ -128,6 +289,17 @@ def extract_session_allowlist( for entry in entries: if not isinstance(entry, str) or not entry: raise FilterError("filters.session_id entries must be non-empty strings") + # Only names a session could actually have. The allowlist reaches + # queries three ways — direct `IN`, the filter DSL, and a Python + # membership test — and they don't agree on a value like "*", which the + # DSL reads as "drop the condition" while the others treat as a literal. + # Rejecting it here keeps the divergent value away from all three, and + # matches this endpoint's documented contract (an id, a list of ids, or + # {"in": [...]}) which never included wildcards. + if not re.fullmatch(RESOURCE_NAME_PATTERN, entry): + raise FilterError( + f"Invalid session id in filters.session_id: {entry!r}. Session ids match {RESOURCE_NAME_PATTERN}" + ) if entry not in seen: seen.add(entry) allowlist.append(entry) @@ -181,9 +353,26 @@ def apply_filter( if filters is None: return stmt - conditions = _build_filter_conditions(filters, model_class) - if conditions is not None: - stmt = stmt.where(conditions) + # Fail closed. The filter body is arbitrary client JSON, so any shape the + # DSL doesn't recognize must become a 422, not an unhandled 500 from + # somewhere deep in SQLAlchemy. The exception is still logged in full so a + # genuine bug in the builder stays visible rather than being swallowed. + try: + conditions = _build_filter_conditions(filters, model_class) + if conditions is not None: + stmt = stmt.where(conditions) + except FilterError: + raise + except Exception: + # Keys only, not the body: filter operands are client-supplied and carry + # peer/session ids and free-text `contains` values. The traceback plus + # the entry shape is what actually locates a builder bug. + logger.exception( + "Unexpected error building filter for %s; filter keys: %s", + model_class.__name__, + sorted(filters), + ) + raise FilterError("Invalid filter configuration") from None return stmt @@ -256,8 +445,15 @@ def _build_filter_conditions( _depth=_depth + 1, ) if sub_condition is not None: + # `IS NOT TRUE` rather than `NOT`: under SQL's three-valued + # logic a comparison against a NULL column is NULL, and `NOT + # NULL` is NULL, so plain negation drops rows whose column is + # unset — even though an unset column does not match what is + # being excluded. NOT [{"session_id": "abc"}] must include + # documents that have no session, since those are not "abc". + # Composes over compound sub-conditions: (a AND b) IS NOT TRUE. not_conditions.append( - not_(sub_condition) + sub_condition.is_not(True) ) # Apply NOT to each condition individually if not_conditions: conditions.append(and_(*not_conditions)) # Then AND them together @@ -327,6 +523,14 @@ def _build_field_condition( if value == "*": return None + # A null operand is a null check, not a value to compare. Every branch below + # binds the operand against the column's type, and no type accepts None, so + # this has to short-circuit or `{"col": null}` raises instead of matching the + # rows it names. Keeps bare null agreeing with `{"ne": null}` (IS NOT NULL) + # and with `NOT [{"col": null}]`. + if value is None: + return column.is_(None) + # Bare-list sugar on regular columns: {"session_id": ["a", "b"]} is # shorthand for {"session_id": {"in": ["a", "b"]}}. JSONB columns are # excluded — a bare list there keeps JSONB containment semantics. @@ -345,13 +549,21 @@ def _build_field_condition( # For JSONB fields (metadata, configuration), check if it contains nested comparison operators if column_name in JSONB_COLUMNS: return _build_nested_metadata_conditions(column, value) # pyright: ignore + elif not isinstance(column.type, JSONB): + # A dict against a scalar column compiles fine but fails in + # psycopg at execute time ("cannot adapt type 'dict'") as a 500. + # Reject unknown operator dicts here as a 422 instead. + keys = sorted(typing_cast("dict[str, Any]", value)) + raise FilterError( + f"Invalid filter for column '{key}': unsupported operator(s) {keys}. Expected one of {sorted(COMPARISON_OPERATORS)} or a scalar value." + ) else: return column == value else: if column_name in JSONB_COLUMNS: - return column.contains(value) + return column.contains(_coerce_operand(column, column_name, value)) else: - return column == value + return column == _coerce_operand(column, column_name, value) def _safe_numeric_cast( @@ -552,11 +764,6 @@ def _build_comparison_conditions( """ conditions: list[ColumnElement[bool]] = [] - # Check if this is a datetime column - is_datetime_column = hasattr(column.type, "python_type") and issubclass( - column.type.python_type, datetime.datetime - ) - for operator, op_value in comparisons.items(): # Validate that the operator is supported if operator not in COMPARISON_OPERATORS: @@ -566,29 +773,25 @@ def _build_comparison_conditions( if op_value == "*": continue + # A null operand is a null check, not a value comparison, on every + # column type. Only `ne` is meaningful: {"col": None} covers IS NULL + # via the null guard in _build_field_condition. + if op_value is None: + if operator != "ne": + raise FilterError( + f"Operator '{operator}' does not accept null. Use {{\"ne\": null}} for a not-null check, or null on its own for a null check." + ) + conditions.append(column.is_not(None)) + continue + condition = None - # For datetime columns, cast string values to timestamp - if is_datetime_column and isinstance(op_value, str): - # Validate datetime string to prevent SQL injection - validated_datetime = _validate_datetime_string(op_value) - if validated_datetime is None: - # Raise error if datetime validation fails - raise FilterError(f"Invalid datetime value: {op_value}") - - # Use the validated datetime object directly instead of string interpolation - casted_value = validated_datetime - else: - # if the operator is a numeric operator, the value must cast to a number - if operator in NUMERIC_OPERATORS: - try: - casted_value = float(op_value) - except ValueError: - raise FilterError( - f"Invalid numeric value: {op_value}. Expected a number, got {type(op_value).__name__}" - ) from None - else: - casted_value = op_value + # `in` coerces element-wise below; every other operator has one operand. + casted_value = ( + op_value + if operator == "in" + else _coerce_operand(column, column_name, op_value, operator) + ) if operator == "gte": condition = column >= casted_value @@ -599,38 +802,39 @@ def _build_comparison_conditions( elif operator == "lt": condition = column < casted_value elif operator == "ne": - condition = column != casted_value + # IS DISTINCT FROM, not <>: `NULL <> 'abc'` is NULL, so plain + # inequality drops rows whose column is unset. Identical to <> + # whenever no NULL is involved, and keeps `ne` agreeing with the + # NOT operator instead of quietly returning a different row set. + condition = column.is_distinct_from(casted_value) elif operator == "in": if hasattr(op_value, "__iter__") and not isinstance(op_value, str | bytes): # Handle wildcard in iterable - if present, matches everything, so no condition needed if "*" in op_value: continue else: - if is_datetime_column: - # Validate and cast each datetime string value - casted_values: list[str | datetime.datetime] = [] - for val in op_value: - if isinstance(val, str): - validated_datetime = _validate_datetime_string(val) - if validated_datetime is None: - raise FilterError( - f"Invalid datetime value in list: {val}" - ) - casted_values.append(validated_datetime) - else: - casted_values.append(val) - if casted_values: - condition = column.in_(casted_values) - else: - condition = column.in_(list(op_value)) + # Element-wise: one bad element poisons the whole IN, since + # its type decides the cast rendered for that parameter. + # An empty list is applied, not skipped: `in: []` must match + # nothing. Dropping the condition would widen the query to + # every row, and session scoping relies on an empty + # allowlist failing closed (see extract_session_allowlist). + condition = column.in_( + [ + _coerce_operand(column, column_name, val, operator) + for val in op_value + ] + ) else: raise FilterError( f"Invalid value for 'in' operator: {op_value}. Expected an iterable (list, tuple, set), got {type(op_value).__name__}" ) elif operator == "contains": - if column_name == "h_metadata": - # For JSONB columns, use JSONB contains - condition = column.contains(op_value) + if isinstance(column.type, JSONB): + # Keyed on the column type, not the name: internal_metadata is + # equally JSONB and was falling through to ILIKE, which + # Postgres rejects as `jsonb ~~* text`. + condition = column.contains(casted_value) else: # For text columns, use ILIKE with escaped pattern escaped_value = escape_ilike_pattern(str(op_value)) diff --git a/tests/routes/test_conclusions.py b/tests/routes/test_conclusions.py index 77cb23fb..ccd3b337 100644 --- a/tests/routes/test_conclusions.py +++ b/tests/routes/test_conclusions.py @@ -1381,3 +1381,81 @@ class TestConclusionRoutes: # Verify the conclusion has null session_id conclusion = next(c for c in data["items"] if c["id"] == created_id) assert conclusion["session_id"] is None + + @pytest.mark.asyncio + async def test_negation_includes_conclusions_with_no_session( + self, + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], + ): + """Negation must not silently drop workspace-level conclusions. + + A conclusion with no session is not "some other session", so excluding + that session has to leave it in the result. Under SQL's three-valued + logic a comparison against NULL is NULL, which would drop the row. + + This is only visible by counting returned rows — the filter builds and + executes cleanly either way. + """ + test_workspace, test_peer = sample_data + + test_peer2 = models.Peer( + name=str(generate_nanoid()), workspace_name=test_workspace.name + ) + db_session.add(test_peer2) + await db_session.flush() + + test_session = models.Session( + name=str(generate_nanoid()), workspace_name=test_workspace.name + ) + other_session = models.Session( + name=str(generate_nanoid()), workspace_name=test_workspace.name + ) + db_session.add_all([test_session, other_session]) + await db_session.commit() + + await self._create_collection( + db_session, test_workspace.name, test_peer.name, test_peer2.name + ) + + scoped = models.Document( + workspace_name=test_workspace.name, + observer=test_peer.name, + observed=test_peer2.name, + content="Scoped to a session", + session_name=test_session.name, + ) + workspace_level = models.Document( + workspace_name=test_workspace.name, + observer=test_peer.name, + observed=test_peer2.name, + content="Not scoped to any session", + session_name=None, + ) + db_session.add_all([scoped, workspace_level]) + await db_session.commit() + + def contents(filters: dict[str, object]) -> set[str]: + response = client.post( + f"/v3/workspaces/{test_workspace.name}/conclusions/list", + json={"filters": filters}, + ) + assert response.status_code == 200, response.text + return {item["content"] for item in response.json()["items"]} + + both = {"Scoped to a session", "Not scoped to any session"} + + # NOT and ne agree, and both keep the session-less conclusion. + assert contents({"NOT": [{"session_id": other_session.name}]}) == both + assert contents({"session_id": {"ne": other_session.name}}) == both + + # Requiring the field to be set is how you narrow to sessioned rows. + assert contents( + { + "AND": [ + {"session_id": {"ne": other_session.name}}, + {"session_id": {"ne": None}}, + ] + } + ) == {"Scoped to a session"} diff --git a/tests/test_session_allowlist.py b/tests/test_session_allowlist.py index aa6077cb..86c9280b 100644 --- a/tests/test_session_allowlist.py +++ b/tests/test_session_allowlist.py @@ -62,6 +62,38 @@ class TestExtractSessionAllowlist: with pytest.raises(FilterError): extract_session_allowlist({"session_id": bad}) + @pytest.mark.parametrize( + "filters", + [ + {"session_id": "*"}, + {"session_id": ["s1", "*"]}, + {"session_id": {"in": ["*"]}}, + ], + ) + def test_wildcard_rejected(self, filters: dict[str, Any]): + """A wildcard means two different things depending on which consumer + receives the allowlist: the filter DSL drops the condition entirely + (matching every session), while the direct `IN` and Python membership + paths treat "*" as a literal session name (matching none). It is not + part of this endpoint's contract, so it is rejected outright. + + The mixed list is the case that matters most — it looks narrowed. + """ + with pytest.raises(FilterError, match="Invalid session id"): + extract_session_allowlist(filters) + + @pytest.mark.parametrize("name", ["a b", "a/b", "a.b", "a%b", "s1;drop"]) + def test_malformed_session_ids_rejected(self, name: str): + with pytest.raises(FilterError, match="Invalid session id"): + extract_session_allowlist({"session_id": name}) + + def test_valid_id_characters_still_accepted(self): + """The pattern must not be stricter than the ids the API actually + issues, which include underscores and hyphens.""" + assert extract_session_allowlist({"session_id": "Valid_name-123"}) == [ + "Valid_name-123" + ] + def test_cap_enforced(self): too_many = [f"s{i}" for i in range(MAX_SESSION_ALLOWLIST_ENTRIES + 1)] with pytest.raises(FilterError, match="at most"): diff --git a/tests/utils/test_filter.py b/tests/utils/test_filter.py new file mode 100644 index 00000000..c90c8442 --- /dev/null +++ b/tests/utils/test_filter.py @@ -0,0 +1,412 @@ +"""Unit tests for filter condition building.""" + +from collections.abc import Sequence +from datetime import datetime +from decimal import Decimal +from typing import Any, cast, get_args + +import pytest +from sqlalchemy import select +from sqlalchemy.dialects import postgresql +from sqlalchemy.dialects.postgresql import JSONB +from sqlalchemy.dialects.postgresql import psycopg as psycopg_dialect + +from src.exceptions import FilterError +from src.models import Document, Message, Peer, Session +from src.utils.filter import apply_filter +from src.utils.types import DocumentLevel + + +def test_unknown_operator_dict_on_scalar_column_raises(): + """An unrecognized operator dict must 422, not reach the driver as a 500. + + Regression: {"session_id": {"operator": "null"}} compiled to + `session_name = %(param)s` with a dict bind, which psycopg rejected with + "cannot adapt type 'dict'" -> unhandled 500. + """ + with pytest.raises(FilterError): + apply_filter(select(Document), Document, {"session_id": {"operator": "null"}}) + + +def test_known_operator_dict_on_scalar_column_still_works(): + stmt = apply_filter( + select(Document), Document, {"session_id": {"in": ["s1", "s2"]}} + ) + assert "session_name IN" in str(stmt).replace("documents.", "") + + +def test_dict_on_jsonb_column_still_works(): + stmt = apply_filter(select(Document), Document, {"metadata": {"kind": "note"}}) + # Assert on the WHERE clause specifically: internal_metadata is in the + # SELECT projection either way, so checking the whole statement passes even + # when no condition was applied at all. + assert stmt.whereclause is not None + assert "internal_metadata" in str(stmt.whereclause) + compiled = stmt.compile(dialect=postgresql.dialect()) + assert {"kind": "note"} in [bind.value for bind in compiled.binds.values()] + + +def test_numeric_operand_keeps_integer_precision(): + """float() rounds anything past 2**53, silently shifting the comparison.""" + big = 2**53 + 1 + stmt = apply_filter(select(Message), Message, {"token_count": {"gt": big}}) + compiled = stmt.compile(dialect=postgresql.dialect()) + assert big in [bind.value for bind in compiled.binds.values()] + + +def test_fractional_operand_on_integer_column_is_not_truncated(): + """Coercing to the column's int type would turn `lt 5.5` into `lt 5`.""" + stmt = apply_filter(select(Message), Message, {"token_count": {"lt": 5.5}}) + compiled = stmt.compile(dialect=postgresql.dialect()) + assert 5.5 in [bind.value for bind in compiled.binds.values()] + + +def test_integer_operand_binds_without_an_integer_cast(): + """A plain int bind renders `::INTEGER`, so any value past int4 fails at + execute time with "integer out of range" even when the comparison is + meaningful. Decimal renders no cast, which is what float() used to do.""" + stmt = apply_filter(select(Message), Message, {"token_count": {"gt": 2**31}}) + assert "::INTEGER" not in str(stmt.compile(dialect=psycopg_dialect.dialect())) + + +def test_in_list_on_numeric_column_handles_out_of_range_values(): + stmt = apply_filter(select(Message), Message, {"token_count": {"in": [1, 2**31]}}) + assert "::INTEGER" not in str(stmt.compile(dialect=psycopg_dialect.dialect())) + + +def test_in_list_on_numeric_column_rejects_garbage(): + with pytest.raises(FilterError): + apply_filter(select(Message), Message, {"token_count": {"in": [1, "nope"]}}) + + +@pytest.mark.parametrize( + "filters", + [ + {"is_active": True}, + {"is_active": False}, + {"is_active": {"ne": True}}, + {"is_active": {"in": [True, False]}}, + ], +) +def test_bool_column_accepts_native_booleans(filters: dict[str, Any]): + """bool subclasses int, so a boolean column would otherwise be coerced to 1 + and rejected by Postgres as `boolean <> integer`.""" + stmt = apply_filter(select(Session), Session, filters) + assert stmt.whereclause is not None + assert "is_active" in str(stmt.whereclause) + + +@pytest.mark.parametrize( + "filters", + [ + {"is_active": "true"}, + {"is_active": "false"}, + {"is_active": 1}, + {"is_active": {"ne": "true"}}, + ], +) +def test_bool_column_rejects_non_boolean_operands(filters: dict[str, Any]): + """These bind as VARCHAR/INTEGER against a boolean column, which Postgres + rejects at execute time — a 422 is the honest answer, not a 500.""" + with pytest.raises(FilterError): + apply_filter(select(Session), Session, filters) + + +def test_numeric_string_operand_stays_exact(): + stmt = apply_filter(select(Message), Message, {"token_count": {"gt": "5"}}) + compiled = stmt.compile(dialect=postgresql.dialect()) + assert 5 in [bind.value for bind in compiled.binds.values()] + + +def test_ne_none_on_scalar_column_is_not_null(): + """Regression: float(None) raised TypeError, which the ValueError handler + missed -> unhandled 500.""" + stmt = apply_filter(select(Document), Document, {"session_id": {"ne": None}}) + assert "session_name IS NOT NULL" in str(stmt) + + +def test_ne_string_on_text_column_compares_as_string(): + """Regression: numeric operators float()-cast on every column type, so a + string inequality on a text column was rejected as a bad number.""" + stmt = apply_filter(select(Document), Document, {"session_id": {"ne": "abc"}}) + assert "session_name IS DISTINCT FROM" in str(stmt) + + +def test_numeric_operator_still_validates_on_numeric_column(): + with pytest.raises(FilterError): + apply_filter(select(Message), Message, {"token_count": {"gt": "nope"}}) + + +def test_ne_none_on_numeric_column_is_not_null(): + stmt = apply_filter(select(Message), Message, {"token_count": {"ne": None}}) + assert "token_count IS NOT NULL" in str(stmt) + + +def test_null_operand_on_non_ne_operator_raises(): + with pytest.raises(FilterError): + apply_filter(select(Message), Message, {"token_count": {"gt": None}}) + + +def test_enum_column_rejects_an_unknown_value(): + """An invalid level silently matched nothing, which reads as "no results" + rather than "you sent a value that cannot exist".""" + with pytest.raises(FilterError): + apply_filter(select(Document), Document, {"level": "banana"}) + + +@pytest.mark.parametrize("level", get_args(DocumentLevel)) +def test_enum_column_accepts_every_declared_level(level: str): + """Derived from the Literal, so a new level (e.g. "abduction") is covered + here the moment it is declared — no second list to keep in sync.""" + stmt = apply_filter(select(Document), Document, {"level": level}) + assert stmt.whereclause is not None + + +def test_empty_in_list_matches_nothing_rather_than_everything(): + """Dropping an empty IN would widen the query to every row. Session scoping + relies on an empty allowlist failing closed.""" + stmt = apply_filter(select(Document), Document, {"session_id": {"in": []}}) + assert stmt.whereclause is not None + assert "IN" in str(stmt.whereclause) + + +@pytest.mark.parametrize( + "filters", + [ + {"created_at": "2026-01-01"}, + {"created_at": {"gte": "2026-01-01"}}, + {"token_count": "5"}, + {"token_count": {"gt": "5"}}, + ], +) +def test_equality_and_comparison_paths_coerce_alike(filters: dict[str, Any]): + """The two paths had different rules: comparison operators parsed datetimes + and coerced numbers, bare equality bound the raw string and 500'd on + `timestamp with time zone = character varying`.""" + stmt = apply_filter(select(Message), Message, filters) + assert stmt.whereclause is not None + + +@pytest.mark.parametrize( + ("model", "filters"), + [ + (Message, {"token_count": {"contains": 5}}), # integer ~~* text + (Message, {"created_at": {"contains": "x"}}), # timestamptz ~~* text + (Document, {"metadata": {"gte": 5}}), # jsonb >= integer + (Document, {"metadata": {"contains": "x"}}), # jsonb ~~* text + (Document, {"source_ids": "abc"}), # jsonb = character varying + (Document, {"session_id": 5}), # text = integer + (Document, {"session_id": True}), # text = boolean + (Message, {"created_at": 5}), # timestamptz = integer + (Document, {"embedding": 5}), # no python_type at all + ], +) +def test_incompatible_operand_types_are_rejected(model: Any, filters: dict[str, Any]): + """Each of these compiled cleanly and failed in Postgres as + `operator does not exist: `.""" + with pytest.raises(FilterError): + apply_filter(select(model), model, filters) + + +def _where(model: Any, filters: dict[str, Any]) -> str: + stmt = apply_filter(select(model), model, filters) + # Without this, a dropped condition makes split("WHERE") return the whole + # statement, so a filter that silently widened could still pass the asserts. + assert stmt.whereclause is not None + return str(stmt.compile(dialect=psycopg_dialect.dialect())).split("WHERE")[-1] + + +def test_not_is_null_safe(): + """`NOT (col = v)` is NULL when col is NULL, so plain negation drops rows + whose column is unset — even though an unset column is not `v`.""" + where = _where(Document, {"NOT": [{"session_id": "abc"}]}) + assert "IS NOT true" in where + + +def test_ne_is_null_safe(): + where = _where(Document, {"session_id": {"ne": "abc"}}) + assert "IS DISTINCT FROM" in where + + +def test_not_is_null_safe_over_a_compound_condition(): + """Negation has to survive nesting, not just single comparisons.""" + where = _where( + Document, {"NOT": [{"AND": [{"session_id": "a"}, {"level": "explicit"}]}]} + ) + assert "IS NOT true" in where + assert "AND" in where + + +def test_not_is_null_safe_over_contains(): + where = _where(Document, {"NOT": [{"session_id": {"contains": "x"}}]}) + assert "ILIKE" in where + assert "IS NOT true" in where + + +def test_ne_null_still_renders_is_not_null(): + """The null operand is intercepted before the operator dispatch, so this + path is unchanged by null-safe `ne`.""" + assert "IS NOT NULL" in _where(Document, {"session_id": {"ne": None}}) + + +@pytest.mark.parametrize( + ("model", "filters"), + [ + (Document, {"session_id": None}), # text + (Message, {"token_count": None}), # numeric + (Session, {"is_active": None}), # boolean + (Message, {"created_at": None}), # datetime + (Document, {"metadata": None}), # JSONB + (Document, {"source_ids": None}), # JSONB via the raw-key fallback + ], +) +def test_bare_null_is_a_null_check(model: Any, filters: dict[str, Any]): + """Regression: every operand routes through _coerce_operand, which accepts + no None on any column type, so bare null raised FilterError instead of + matching the unset rows it names.""" + assert "IS NULL" in _where(model, filters) + + +def test_bare_null_agrees_with_negation(): + """`NOT [{col: null}]` and `{col: {ne: null}}` must select the same rows. + `x IS NULL` never evaluates to NULL, so `(x IS NULL) IS NOT true` is exactly + `x IS NOT NULL` — the three forms have to stay in agreement.""" + assert "IS NOT NULL" in _where(Document, {"session_id": {"ne": None}}) + negated = _where(Document, {"NOT": [{"session_id": None}]}) + assert "IS NULL" in negated + assert "IS NOT true" in negated + + +def test_dict_on_raw_key_jsonb_column_is_equality(): + """Document falls back to raw column names, so a JSONB column outside + JSONB_COLUMNS reaches _build_field_condition's dict branch. It compares as + equality rather than containment — unlike `metadata`.""" + where = _where(Document, {"source_ids": {"kind": "note"}}) + assert "source_ids =" in where + + +@pytest.mark.parametrize( + ("filters", "expected"), + [ + ({"session_id": "abc"}, "session_name ="), + ({"session_id": {"contains": "x"}}, "ILIKE"), + ], +) +def test_positive_predicates_are_unchanged(filters: dict[str, Any], expected: str): + """A NULL column does not equal or contain anything, so positive predicates + correctly exclude those rows and must keep their plain operators.""" + where = _where(Document, filters) + assert expected in where + assert "IS NOT true" not in where + assert "IS DISTINCT FROM" not in where + + +# --- Invariants over the whole DSL ------------------------------------------- +# +# The filter body is arbitrary client JSON. Enumerating bad shapes one at a time +# is endless, so these two tests assert the properties that make any unhandled +# shape a 422 instead of a 500, and fail on the next shape nobody thought of. + +_OPERANDS: list[Any] = [ + None, + True, + False, + 0, + -1, + 1.5, + "", + "abc", + "*", + [], + [None], + [[1]], + [{"a": 1}], + {}, + {"operator": "null"}, + {"ne": None}, + {"ne": {"a": 1}}, + {"ne": [1]}, + {"in": None}, + {"in": "abc"}, + {"in": [{"a": 1}]}, + {"in": [[1]]}, + {"gt": {}}, + {"gt": []}, + {"gt": True}, + {"contains": None}, + {"contains": {"a": 1}}, + {"lt": [1, 2]}, +] + +_COLUMNS: dict[Any, list[str]] = { + Document: ["session_id", "workspace_id", "metadata", "level", "source_ids", "id"], + Message: ["session_id", "peer_id", "token_count", "created_at", "metadata"], + Session: ["id", "is_active", "created_at", "configuration"], + Peer: ["id", "created_at", "metadata"], +} + +_MALFORMED: list[dict[str, Any]] = [ + {"AND": "notalist"}, + {"AND": [None]}, + {"AND": [[]]}, + {"AND": [1]}, + {"OR": [None]}, + {"OR": [1]}, + {"NOT": None}, + {"NOT": [None]}, + {"unknown_column": 1}, +] + + +def _filter_shapes() -> list[tuple[Any, dict[str, Any]]]: + shapes: list[tuple[Any, dict[str, Any]]] = [] + for model, columns in _COLUMNS.items(): + for column in columns: + for operand in _OPERANDS: + leaf = {column: operand} + shapes.append((model, leaf)) + shapes.append((model, {"AND": [leaf]})) + shapes.append((model, {"NOT": [leaf]})) + shapes.extend((model, bad) for bad in _MALFORMED) + return shapes + + +def test_every_filter_shape_either_compiles_or_raises_filter_error(): + """No filter body may escape as anything other than a compiled statement or + a FilterError. Anything else reaches the client as an unhandled 500.""" + escaped: list[tuple[str, dict[str, Any], str]] = [] + for model, filters in _filter_shapes(): + try: + str(apply_filter(select(model), model, filters)) + except FilterError: + pass + except Exception as exc: # pragma: no cover - failure path + escaped.append((model.__name__, filters, type(exc).__name__)) + assert not escaped, f"non-FilterError escapes: {escaped[:10]}" + + +def test_no_non_scalar_value_is_bound_to_a_scalar_column(): + """A dict or list bound to a non-JSONB parameter compiles cleanly and then + fails in psycopg at execute time — the original 500. Nothing may reach that + state, including non-scalars nested inside an `in` list.""" + offenders: list[tuple[str, dict[str, Any], str]] = [] + for model, filters in _filter_shapes(): + try: + stmt = apply_filter(select(model), model, filters) + except FilterError: + continue + compiled = stmt.compile(dialect=postgresql.dialect()) + for bind in compiled.binds.values(): + if isinstance(bind.type, JSONB): + continue + value: Any = bind.value + # An expanding IN bind holds the list itself; check its elements. + elements = cast( + "Sequence[Any]", value if isinstance(value, list | tuple) else [value] + ) + for element in elements: + if element is not None and not isinstance( + element, str | bool | int | float | Decimal | datetime + ): + offenders.append((model.__name__, filters, repr(element)[:40])) + assert not offenders, f"non-scalar bound to scalar column: {offenders[:10]}" From b7d4a2a3ad0d83ab230ed2358f0a4df8004ae26d Mon Sep 17 00:00:00 2001 From: ajspig <46900795+ajspig@users.noreply.github.com> Date: Thu, 13 Aug 2026 16:40:00 -0400 Subject: [PATCH 2/2] feat(mcp): search conclusions via search tool (#974) * feat(mcp): search conclusions via search tool Port of plastic-labs/claude-honcho#113 to the standalone MCP worker. The search tool only queried the message store, so saved conclusions were unreachable through search (findable only by paging list_conclusions or via query_conclusions with a known scope). search now also queries conclusions in parallel with the message search, returning {messages, conclusions}. Since the conclusions query API requires an explicit (observer, observed) pair, the conclusion leg runs only when peer_id is given (self-conclusions, matching list_conclusions/query_conclusions defaults) and degrades to [] on error so search never gets worse than before. Conclusion results include IDs usable with delete_conclusion. Also syncs mcp/bun.lock with package.json's @honcho-ai/sdk ^2.1.0 (the lock still recorded ^2.0.0) and points delete_conclusion's description at query_conclusions/list_conclusions for ID discovery. Co-Authored-By: Claude Fable 5 * fix: add support for filtering to mcp * chore: minor nits * fix: adding better error handling --------- Co-authored-by: Claude Fable 5 --- mcp/bun.lock | 8 +-- mcp/package.json | 2 +- mcp/src/tools/conclusions.ts | 12 +++- mcp/src/tools/workspace.ts | 103 ++++++++++++++++++++++++++++++----- 4 files changed, 102 insertions(+), 23 deletions(-) diff --git a/mcp/bun.lock b/mcp/bun.lock index 29d050c2..52b157a4 100644 --- a/mcp/bun.lock +++ b/mcp/bun.lock @@ -5,7 +5,7 @@ "": { "name": "honcho-mcp-proxy", "dependencies": { - "@honcho-ai/sdk": "^2.0.0", + "@honcho-ai/sdk": "^2.2.0", "@modelcontextprotocol/sdk": "^1.26.0", "agents": "^0.4.0", "nanoid": "^5.1.7", @@ -107,7 +107,7 @@ "@esbuild/win32-x64": ["@esbuild/win32-x64@0.25.4", "", { "os": "win32", "cpu": "x64" }, "sha512-nOT2vZNw6hJ+z43oP1SPea/G/6AbN6X+bGNhNuq8NtRHy4wsMhw765IKLNmnjek7GvjWBYQ8Q5VBoYTFg9y1UQ=="], - "@honcho-ai/sdk": ["@honcho-ai/sdk@2.0.1", "", { "dependencies": { "@types/node": "^24.0.1", "zod": "4.0.0" } }, "sha512-y/Wk49C0N1miI9BZTNWFIbzdUkMZfP4Do/EJ1q4lEIK+FAOKxQgces/zET3kPKV3zF9sOUl2pXrFb/XKYayeYw=="], + "@honcho-ai/sdk": ["@honcho-ai/sdk@2.2.0", "", { "dependencies": { "zod": "4.0.0" } }, "sha512-SyygN+BrpUB2fRjhwcYmT+tcEhHrKmbj9nOZLVUFY7M5YBswJ+mZb/CeLpNbRh+QQTU8F8JWY3lQat95c6nwmA=="], "@hono/node-server": ["@hono/node-server@1.19.9", "", { "peerDependencies": { "hono": "^4" } }, "sha512-vHL6w3ecZsky+8P5MD+eFfaGTyCeOHUIFYMGpQGbrBTSmNNoxv0if69rEZ5giu36weC5saFuznL411gRX7bJDw=="], @@ -177,8 +177,6 @@ "@types/lodash": ["@types/lodash@4.17.23", "", {}, "sha512-RDvF6wTulMPjrNdCoYRC8gNR880JNGT8uB+REUpC2Ns4pRqQJhGz90wh7rgdXDPpCczF3VGktDuFGVnz8zP7HA=="], - "@types/node": ["@types/node@24.1.0", "", { "dependencies": { "undici-types": "~7.8.0" } }, "sha512-ut5FthK5moxFKH2T1CUOC6ctR67rQRvvHdFLCD2Ql6KXmMuCrjsSsRI9UsLCm9M18BMwClv4pn327UvB7eeO1w=="], - "@vercel/oidc": ["@vercel/oidc@3.1.0", "", {}, "sha512-Fw28YZpRnA3cAHHDlkt7xQHiJ0fcL+NRcIqsocZQUSmbzeIKRpwttJjik5ZGanXP+vlA4SbTg+AbA3bP363l+w=="], "accepts": ["accepts@2.0.0", "", { "dependencies": { "mime-types": "^3.0.0", "negotiator": "^1.0.0" } }, "sha512-5cvg6CtKwfgdmVqY1WIiXKc3Q1bkRqGLi+2W/6ao+6Y7gu/RCwRuAhGEzh5B4KlszSuTLgZYuqFqo5bImjNKng=="], @@ -471,8 +469,6 @@ "undici": ["undici@7.12.0", "", {}, "sha512-GrKEsc3ughskmGA9jevVlIOPMiiAHJ4OFUtaAH+NhfTUSiZ1wMPIQqQvAJUrJspFXJt3EBWgpAeoHEDVT1IBug=="], - "undici-types": ["undici-types@7.8.0", "", {}, "sha512-9UJ2xGDvQ43tYyVMpuHlsgApydB8ZKfVYTsLDhXkFL/6gfkp+U8xTGdh8pMJv1SpZna0zxG1DwsKZsreLbXBxw=="], - "unenv": ["unenv@2.0.0-rc.17", "", { "dependencies": { "defu": "^6.1.4", "exsolve": "^1.0.4", "ohash": "^2.0.11", "pathe": "^2.0.3", "ufo": "^1.6.1" } }, "sha512-B06u0wXkEd+o5gOCMl/ZHl5cfpYbDZKAT+HWTL+Hws6jWu7dCiqBBXXXzMFcFVJb8D4ytAnYmxJA83uwOQRSsg=="], "unpipe": ["unpipe@1.0.0", "", {}, "sha512-pjy2bYhSsufwWlKwPc+l3cN7+wuJlK6uz0YdJEOlQDbl6jo/YlPi4mb8agUkVC8BF7V8NuzeyPNqRksA3hztKQ=="], diff --git a/mcp/package.json b/mcp/package.json index 2311020c..900df5cf 100644 --- a/mcp/package.json +++ b/mcp/package.json @@ -15,7 +15,7 @@ "deploy:staging": "wrangler deploy --env staging" }, "dependencies": { - "@honcho-ai/sdk": "^2.1.0", + "@honcho-ai/sdk": "^2.2.0", "@modelcontextprotocol/sdk": "^1.26.0", "agents": "^0.4.0", "nanoid": "^5.1.7", diff --git a/mcp/src/tools/conclusions.ts b/mcp/src/tools/conclusions.ts index e6fedf76..f6170fce 100644 --- a/mcp/src/tools/conclusions.ts +++ b/mcp/src/tools/conclusions.ts @@ -71,19 +71,26 @@ export function register(server: McpServer, ctx: ToolContext) { .number() .optional() .describe("Max results to return."), + filters: z + .record(z.string(), z.unknown()) + .optional() + .describe( + 'Optional: filter criteria, e.g. {"level": ["deductive", "inductive"]} to only return conclusions derived during dreaming. Levels: explicit (extracted directly from messages), deductive, inductive, contradiction. See https://honcho.dev/docs/v3/documentation/features/advanced/using-filters', + ), }, }, - async ({ peer_id, query, target_peer_id, top_k }) => { + async ({ peer_id, query, target_peer_id, top_k, filters }) => { try { const peer = await ctx.honcho.peer(peer_id); const scope = target_peer_id ? peer.conclusionsOf(target_peer_id) : peer.conclusions; - const conclusions = await scope.query(query, top_k); + const conclusions = await scope.query(query, top_k, undefined, filters); return textResult( conclusions.map((c) => ({ id: c.id, content: c.content, + level: c.level, observer_id: c.observerId, observed_id: c.observedId, session_id: c.sessionId, @@ -149,6 +156,7 @@ export function register(server: McpServer, ctx: ToolContext) { { description: [ "Delete a specific conclusion by ID.", + "Use query_conclusions or list_conclusions to find the ID first.", "Use this to remove incorrect or outdated knowledge.", ].join("\n"), inputSchema: { diff --git a/mcp/src/tools/workspace.ts b/mcp/src/tools/workspace.ts index 04524bbb..b3a871b3 100644 --- a/mcp/src/tools/workspace.ts +++ b/mcp/src/tools/workspace.ts @@ -1,4 +1,5 @@ import { z } from "zod"; +import { BadRequestError, UnprocessableEntityError } from "@honcho-ai/sdk"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import type { ToolContext } from "../types.js"; import { textResult, errorResult, formatMessages } from "../types.js"; @@ -74,11 +75,13 @@ export function register(server: McpServer, ctx: ToolContext) { "search", { description: [ - "Semantic search across messages. Scope is determined by which optional params are provided:", + "Semantic search across messages and, when peer_id is given, that peer's saved conclusions.", + "Message scope is determined by which optional params are provided:", "- No scope params: search all messages in the workspace.", "- peer_id only: search messages authored by that peer across all sessions.", "- session_id only: search messages within that session.", - "Returns an array of matching messages with their content, peer, and session info.", + "Conclusions require peer_id (self-conclusions are searched; conclusion IDs are usable with delete_conclusion).", + "Returns {messages, conclusions}.", ].join("\n"), inputSchema: { query: z.string().describe("Search query."), @@ -90,21 +93,93 @@ export function register(server: McpServer, ctx: ToolContext) { .string() .optional() .describe("Optional: scope search to messages in this session."), + message_limit: z + .number() + .optional() + .describe("Optional: max message results (1-100, default 10)."), + message_filters: z + .record(z.string(), z.unknown()) + .optional() + .describe( + 'Optional: filters for the message search, e.g. {"created_at": {"gte": "2026-01-01"}}. See https://honcho.dev/docs/v3/documentation/features/advanced/using-filters', + ), + conclusion_top_k: z + .number() + .optional() + .describe("Optional: max conclusion results (default 10)."), + conclusion_filters: z + .record(z.string(), z.unknown()) + .optional() + .describe( + 'Optional: filters for the conclusion search, e.g. {"level": ["deductive", "inductive"]} to only return conclusions derived during dreaming. Levels: explicit (extracted directly from messages), deductive, inductive, contradiction. The session_id param does not scope conclusions; use {"session_id": ...} here for that.', + ), }, }, - async ({ query, peer_id, session_id }) => { + async ({ + query, + peer_id, + session_id, + message_limit, + message_filters, + conclusion_top_k, + conclusion_filters, + }) => { try { - let messages; - if (session_id) { - const session = await ctx.honcho.session(session_id); - messages = await session.search(query); - } else if (peer_id) { - const peer = await ctx.honcho.peer(peer_id); - messages = await peer.search(query); - } else { - messages = await ctx.honcho.search(query); - } - return textResult(formatMessages(messages)); + const peer = peer_id ? await ctx.honcho.peer(peer_id) : null; + const messageOptions = { + filters: message_filters, + limit: message_limit, + }; + + const searchMessages = async () => { + if (session_id) { + const session = await ctx.honcho.session(session_id); + return session.search(query, messageOptions); + } + if (peer) { + return peer.search(query, messageOptions); + } + return ctx.honcho.search(query, messageOptions); + }; + + // Conclusion search needs an (observer, observed) pair, so it only + // runs when peer_id is given. + const searchConclusions = async () => { + if (!peer) { + return []; + } + try { + return await peer.conclusions.query( + query, + conclusion_top_k, + undefined, + conclusion_filters, + ); + } catch (e) { + if ( + conclusion_filters && + (e instanceof BadRequestError || + e instanceof UnprocessableEntityError) + ) { + throw e; + } + return []; + } + }; + + const [messages, conclusions] = await Promise.all([ + searchMessages(), + searchConclusions(), + ]); + return textResult({ + messages: formatMessages(messages), + conclusions: conclusions.map((c) => ({ + id: c.id, + content: c.content, + level: c.level, + created_at: c.createdAt, + })), + }); } catch (e) { return errorResult( `Search failed: ${e instanceof Error ? e.message : String(e)}`,