RomanNum3ral
/
honcho
mirror of https://github.com/plastic-labs/honcho.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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>
This commit is contained in:
Vineeth Voruganti 2026-08-12 23:37:19 -04:00
parent 1795032754
commit 2059215a4e
3 changed files with 91 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
docs/v3/documentation/features/advanced/using-filters.mdx
View File

@ -194,11 +194,26 @@ sessions = honcho.sessions(filters={
### Negation and Unset Fields
Some fields can be unset. A conclusion drawn across a whole workspace has no
session, so its `session_id` is null.
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.
`NOT` and `ne` include those rows. A conclusion with no session is not part of
some particular session, so excluding that session keeps it in the result:
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.
<CodeGroup>
```python Python
@ -232,12 +247,12 @@ conclusions = peer.conclusions.list(filters={
```
</CodeGroup>
To exclude a value **and** require the field to be set, add a null check.
`{"ne": null}` matches rows where the field has any value:
To exclude a value **and** require the field to be set, combine the two with
`AND`:
<CodeGroup>
```python Python
# In some session, just not this one — excludes conclusions with no session
# Excludes workspace-level conclusions and `session-123`
conclusions = peer.conclusions.list(filters={
"AND": [
{"session_id": {"ne": "session-123"}},
@ -248,7 +263,7 @@ conclusions = peer.conclusions.list(filters={
```typescript TypeScript
(async () => {
// In some session, just not this one — excludes conclusions with no session
// Excludes workspace-level conclusions and `session-123`
const conclusions = await peer.conclusions.list({
filters: {
AND: [
@ -261,14 +276,6 @@ conclusions = peer.conclusions.list(filters={
```
</CodeGroup>
Positive conditions work the other way around: an unset field matches nothing,
so `{"session_id": "session-123"}` and `{"session_id": {"contains": "abc"}}`
never return rows whose `session_id` is null. Use `{"session_id": None}` to
match those rows specifically.
Fields that are always populated — `metadata`, which defaults to `{}`, and
`created_at` — are unaffected by any of this.
### Combining Logical Operators
Create sophisticated queries by combining different logical operators:
@ -789,21 +796,28 @@ doesn't hold, rather than failing mid-query or quietly returning nothing.
| 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 |
| `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 |
Two consequences worth knowing:
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).
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.
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:

22
src/utils/filter.py
View File

@ -68,9 +68,6 @@ ALLOWED_EXTERNAL_TO_INTERNAL_COLUMN_MAPPING_DOCUMENTS = {
MAX_SESSION_ALLOWLIST_ENTRIES = 1000
# Values that can be bound to a non-JSONB column. Anything else (dict, list,
# bytes, arbitrary objects) compiles into a valid statement and then fails in
# psycopg at execute time as an unhandled 500, so it is rejected up front.
# 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
@ -367,8 +364,13 @@ def apply_filter(
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: %r", model_class.__name__, filters
"Unexpected error building filter for %s; filter keys: %s",
model_class.__name__,
sorted(filters),
)
raise FilterError("Invalid filter configuration") from None
@ -521,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.
@ -764,8 +774,8 @@ def _build_comparison_conditions(
continue
# A null operand is a null check, not a value comparison, on every
# column type. Only `ne` is meaningful: {"col": None} already covers
# IS NULL via the equality path in _build_field_condition.
# 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(

40
tests/utils/test_filter.py
View File

@ -210,6 +210,9 @@ def test_incompatible_operand_types_are_rejected(model: Any, filters: dict[str,
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]
@ -246,6 +249,42 @@ def test_ne_null_still_renders_is_not_null():
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"),
[
@ -316,7 +355,6 @@ _MALFORMED: list[dict[str, Any]] = [
{"NOT": None},
{"NOT": [None]},
{"unknown_column": 1},
{"metadata": None},
]