Skip to content

Commit 3281318

Browse files
phernandezclaude
andcommitted
fix(core): key observation identity and note type to the owning note
Two ways a valid-time query could silently return nothing, both from derived state answering a question about the note it came from. **An authored assertion could become permanently unqueryable.** A temporal qualifier is peeled off an observation before the content is stored, so two lines that differ only in their qualifier persist identical content and derive identical synthetic permalinks. The search index is unique on (permalink, project_id), so `index_entity_markdown` skipped the second as a duplicate -- while its temporal row went on addressing an observation with no search projection. Querying the second window returned nothing, and every reindex reproduced the omission from the same markdown: - [decision] @effective[2026-06-10,2026-07-27) The cache layer will use Redis. - [decision] @effective[2027-06-10,2027-07-27) The cache layer will use Redis. Reproduced before the fix: two observation rows, one search row, two memory_time_index rows, and the 2027 query returning zero results. The duplicate check is not what is wrong -- it guards a real unique index. Its input is. Identity is derived after the peel, so it is derived from a string the note does not consider distinguishing. Nothing on the observation row separates these two, so no row-local rule can: category, content, context and tags are all equal, and the qualifier lives in its own projection by an explicit design decision. A relationship to that projection is not usable either -- all three readers of `Observation.permalink` read it on *detached* instances after their session has closed, so a lazy load would raise rather than resolve. So the ordinal is stored, exactly as `note_section.duplicate_index` already does for duplicate headings. `replace_observations_for_generation` is the one place that sees a note's whole observation set in document order, and it counts the ordinal over `observation_permalink_tail` -- shared with `Observation.permalink` so the count is taken over exactly the identity the address is built from, rather than rebuilt inline and drifting (#929). Keying on the generated tail rather than raw values also closes slug aliasing, where `Foo Bar` and `foo-bar` are different content that generate one permalink. The ordinal is 0 for the first observation of any identity, so every permalink that resolves today is byte-identical afterwards; only later twins gain a suffix. It fixes the same collision for two observations differing only in `(context)`, which had the same defect for the same reason. **A valid-time query combined with `note_types` could not match anything.** A note's type lives in its frontmatter, so only its entity row carries `metadata.note_type`; observation rows carry tags and relation rows carry nothing. Both backends read the type off each row, which asks "is this row an entity of type X?" when the question was "does this row belong to a note of type X?". Valid time selects observation rows, so the two predicates were never true of the same row and the conjunction was unsatisfiable. Resolved through the owning note instead, in one shared builder both backends call -- only the JSON accessor differs, and that is all each supplies. Every search row already carries `entity_id` and an entity row's own `id` equals it, so one non-correlated membership test covers all three row kinds. Non-correlated for the reason `temporal_filters` documents: SQLite's `search_index` is an FTS5 virtual table and a correlated EXISTS beside a MATCH is refused outright. This makes `note_types` return observation and relation rows of matching notes, where it previously collapsed to entity rows. That is the fix, not a side effect: restricting which *kind* of row may match is `entity_types`' job, the two axes are independent, and a query setting neither returns all three kinds. `test_search_type` asserted the old entity-only shape and is updated -- it recorded what the defect allowed, not an intention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pmKq6bqCi6Zp6BTHuZjrp Signed-off-by: phernandez <paul@basicmachines.co>
1 parent 5782845 commit 3281318

9 files changed

Lines changed: 344 additions & 54 deletions
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
"""Add the duplicate ordinal that keeps same-identity observations addressable.
2+
3+
An observation's synthetic permalink is built from its category and content, both of
4+
which survive the peel that strips a temporal qualifier and a (context) off the authored
5+
line. Two observations differing only in one of those therefore shared one address, and
6+
the permalink-keyed search index kept only the first -- so the second note's authored
7+
valid time addressed an observation with no search row, and queries for its interval
8+
found nothing (SPEC-82).
9+
10+
This ordinal separates such twins. It defaults to 0, which is the value every existing
11+
row takes and the value the first observation of any identity keeps, so no permalink that
12+
resolves today changes. Search rows are derived state and are rebuilt from markdown, so
13+
the second twin becomes addressable on the next index pass for that note.
14+
15+
Revision ID: v5o6b7s8d9e0
16+
Revises: u4t5e6m7p8o9
17+
Create Date: 2026-09-02 10:00:00.000000
18+
19+
"""
20+
21+
from typing import Sequence, Union
22+
23+
from alembic import op
24+
import sqlalchemy as sa
25+
26+
27+
revision: str = "v5o6b7s8d9e0"
28+
down_revision: Union[str, None] = "u4t5e6m7p8o9"
29+
branch_labels: Union[str, Sequence[str], None] = None
30+
depends_on: Union[str, Sequence[str], None] = None
31+
32+
33+
def upgrade() -> None:
34+
"""Add observation.duplicate_index, defaulting every existing row to 0."""
35+
with op.batch_alter_table("observation", schema=None) as batch_op:
36+
batch_op.add_column(
37+
sa.Column(
38+
"duplicate_index",
39+
sa.Integer(),
40+
server_default=sa.text("0"),
41+
nullable=False,
42+
)
43+
)
44+
45+
46+
def downgrade() -> None:
47+
"""Remove the duplicate ordinal."""
48+
with op.batch_alter_table("observation", schema=None) as batch_op:
49+
batch_op.drop_column("duplicate_index")

src/basic_memory/models/knowledge.py

Lines changed: 53 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,45 @@ def __repr__(self) -> str: # pragma: no cover
339339
)
340340

341341

342+
def observation_permalink_tail(category: str | None, content: str) -> str:
343+
"""The part of an observation's permalink that distinguishes it within its note.
344+
345+
This is the single definition of what makes two observations of one note share an
346+
address, and it is deliberately shared with the writer that assigns
347+
`Observation.duplicate_index`: an ordinal only disambiguates if it is counted over
348+
exactly the identity the permalink is built from. Computing the two separately is the
349+
defect this function exists to prevent -- rebuilding the permalink format inline is
350+
what diverged from the search index for long observations (#929).
351+
352+
Note what is *not* here. A qualifier (`@effective[...]`) and a `(context)` are peeled
353+
off the line before the observation is stored, so neither reaches this string, and two
354+
observations that differ only in one of them arrive identical. That is not an oversight
355+
to correct by stuffing them back in: the peel is the feature, and valid time is its own
356+
projection rather than an observation column. The note still distinguishes such lines,
357+
so the *address* must too, which is what the ordinal counted over this tail supplies.
358+
359+
Slug aliasing is why the count keys on this generated text rather than on the raw
360+
values: `Foo Bar` and `foo-bar` are different content that generate one permalink, so
361+
an ordinal counted over raw content would leave them colliding.
362+
363+
Content is truncated to 200 chars to stay under PostgreSQL's btree index limit of
364+
2704 bytes.
365+
"""
366+
if len(content) > 200:
367+
# Trigger: content exceeds the 200-char budget imposed by PostgreSQL's
368+
# 2704-byte btree index row limit, so the permalink can only carry a prefix.
369+
# Why: two distinct observations with the same category and an identical
370+
# 200-char prefix would collide on the same synthetic permalink, and the
371+
# search index (permalink-keyed upsert) silently drops the second one.
372+
# Outcome: a short stable digest of the FULL content disambiguates
373+
# truncated permalinks while staying well under the index limit.
374+
digest = hashlib.sha256(content.encode("utf-8")).hexdigest()[:12]
375+
content_for_permalink = f"{content[:200]}-{digest}"
376+
else:
377+
content_for_permalink = content
378+
return generate_permalink(f"observations/{category}/{content_for_permalink}")
379+
380+
342381
class Observation(Base):
343382
"""An observation about an entity.
344383
@@ -360,6 +399,11 @@ class Observation(Base):
360399
tags: Mapped[Optional[list[str]]] = mapped_column(
361400
JSON, nullable=True, default=list, server_default="[]"
362401
)
402+
# Which of the note's same-identity observations this one is, in document order.
403+
# See `observation_permalink_tail` for why an ordinal is needed at all and why it is
404+
# stored rather than derived: `permalink` is read on *detached* instances, long after
405+
# the session that could have looked at this row's siblings has closed.
406+
duplicate_index: Mapped[int] = mapped_column(Integer, default=0, server_default=text("0"))
363407

364408
# Relationships
365409
entity = relationship("Entity", back_populates="observations")
@@ -371,24 +415,17 @@ def permalink(self) -> str:
371415
We can construct these because observations are always defined in
372416
and owned by a single entity.
373417
374-
Content is truncated to 200 chars to stay under PostgreSQL's
375-
btree index limit of 2704 bytes.
418+
`duplicate_index` is what keeps the address faithful when one note says the same
419+
thing twice. It is 0 for the first observation carrying a given identity, so the
420+
overwhelming majority of permalinks are byte-identical to what they have always
421+
been; only the second and later twins gain a trailing ordinal.
376422
"""
377-
if len(self.content) > 200:
378-
# Trigger: content exceeds the 200-char budget imposed by PostgreSQL's
379-
# 2704-byte btree index row limit, so the permalink can only carry a prefix.
380-
# Why: two distinct observations with the same category and an identical
381-
# 200-char prefix would collide on the same synthetic permalink, and the
382-
# search index (permalink-keyed upsert) silently drops the second one.
383-
# Outcome: a short stable digest of the FULL content disambiguates
384-
# truncated permalinks while staying well under the index limit.
385-
digest = hashlib.sha256(self.content.encode("utf-8")).hexdigest()[:12]
386-
content_for_permalink = f"{self.content[:200]}-{digest}"
387-
else:
388-
content_for_permalink = self.content
389-
return generate_permalink(
390-
f"{self.entity.permalink}/observations/{self.category}/{content_for_permalink}"
423+
base = generate_permalink(
424+
f"{self.entity.permalink}/{observation_permalink_tail(self.category, self.content)}"
391425
)
426+
if not self.duplicate_index:
427+
return base
428+
return f"{base}/{self.duplicate_index}"
392429

393430
@override
394431
def __repr__(self) -> str: # pragma: no cover
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
"""SQL for the note-type search predicate.
2+
3+
A note type is a property of the *note*, not of the individual rows projected from it.
4+
One markdown file becomes several search rows -- the entity itself, one per observation,
5+
one per outgoing relation -- and only the entity row carries `metadata.note_type`, because
6+
that is where the frontmatter lives. An observation row carries `metadata.tags`; a relation
7+
row carries no metadata at all.
8+
9+
Reading the type off each row therefore answers "is this row an entity of type X?" when the
10+
question asked was "does this row belong to a note of type X?". Those coincide for entity
11+
rows and for nothing else, which is invisible until a filter selects non-entity rows -- as a
12+
valid-time filter does, since authored time lives on observations (SPEC-82). The conjunction
13+
of the two was unsatisfiable: every row admitted by the temporal predicate was excluded by
14+
the note-type one.
15+
16+
Resolving through the owning note fixes that at the source. Every search row already carries
17+
`entity_id`, and an entity row's own `id` equals it, so one membership test covers all three
18+
row kinds without special-casing any of them and without copying the type onto rows that
19+
would then have to be kept in step with the note's frontmatter.
20+
21+
The predicate is a *non-correlated* subquery for the reason `temporal_filters` documents at
22+
length: SQLite's `search_index` is an FTS5 virtual table, and a correlated `EXISTS` beside a
23+
`MATCH` makes SQLite refuse the statement outright. A non-correlated `IN` is evaluated once,
24+
independently, and composes with every FTS shape in this repository while leaving bm25
25+
ranking intact.
26+
27+
One builder serves both dialects. Only the JSON accessor differs, so that is the single
28+
thing a backend supplies -- the rule itself lives here rather than being written out once
29+
per backend and drifting.
30+
"""
31+
32+
from __future__ import annotations
33+
34+
from typing import Any, Sequence
35+
36+
from basic_memory.schemas.search import SearchItemType
37+
38+
SEARCH_TABLE = "search_index"
39+
40+
# The alias the owning note's row carries inside the subquery.
41+
_OWNER = "note_type_owner"
42+
43+
# Each dialect's expression for the owning note's frontmatter type.
44+
SQLITE_NOTE_TYPE_VALUE = f"json_extract({_OWNER}.metadata, '$.note_type')"
45+
POSTGRES_NOTE_TYPE_VALUE = f"{_OWNER}.metadata->>'note_type'"
46+
47+
48+
def build_note_type_predicate(
49+
note_types: Sequence[str],
50+
params: dict[str, Any],
51+
*,
52+
note_type_value: str,
53+
) -> str:
54+
"""Build the WHERE-clause fragment restricting rows to notes of the given types.
55+
56+
The stored type keeps the frontmatter's own casing (`Chapter`), while the filter is
57+
documented case-insensitive, so both sides are folded to lowercase.
58+
59+
Binds are added to `params` in place, following the convention the surrounding FTS
60+
query builders already use. `project_id` is bound by the caller for the whole query.
61+
"""
62+
placeholders = []
63+
for index, note_type in enumerate(note_types):
64+
name = f"note_type_{index}"
65+
params[name] = note_type.lower()
66+
placeholders.append(f":{name}")
67+
68+
return (
69+
f"{SEARCH_TABLE}.entity_id IN (\n"
70+
f" SELECT {_OWNER}.id\n"
71+
f" FROM {SEARCH_TABLE} AS {_OWNER}\n"
72+
f" WHERE {_OWNER}.type = '{SearchItemType.ENTITY.value}'\n"
73+
f" AND {_OWNER}.project_id = :project_id\n"
74+
f" AND LOWER({note_type_value}) IN ({', '.join(placeholders)}))"
75+
)

src/basic_memory/repository/observation_repository.py

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from sqlalchemy.orm.interfaces import LoaderOption
1010

1111
from basic_memory.models import Observation
12+
from basic_memory.models.knowledge import observation_permalink_tail
1213
from basic_memory.repository.relation_repository import current_relation_generation_statement
1314
from basic_memory.repository.repository import Repository
1415
from basic_memory.temporal import TemporalAssertion
@@ -142,17 +143,33 @@ async def replace_observations_for_generation(
142143
return ObservationGenerationWriteResult(generation_is_current=False)
143144

144145
await self.delete_by_fields(session, entity_id=entity_id)
145-
rows = [
146-
Observation(
147-
project_id=self.project_id,
148-
entity_id=entity_id,
149-
content=obs.content,
150-
category=obs.category,
151-
context=obs.context,
152-
tags=obs.tags,
146+
# A note may say the same thing twice and mean two different things -- most
147+
# sharply when a temporal qualifier or a (context) is what separates them, since
148+
# both are peeled off before the content reaches this row. The permalink is built
149+
# from what survives that peel, so those twins would address one row, and the
150+
# permalink-keyed search index would keep only the first (SPEC-82).
151+
#
152+
# This is the one place that sees a note's whole observation set in document
153+
# order, so it is where the ordinal that separates them can be counted at all.
154+
# `observation_permalink_tail` is shared with `Observation.permalink` so the
155+
# count is taken over exactly the identity the address is built from.
156+
duplicates_seen: dict[str, int] = {}
157+
rows = []
158+
for obs in observations:
159+
identity = observation_permalink_tail(obs.category, obs.content)
160+
duplicate_index = duplicates_seen.get(identity, 0)
161+
duplicates_seen[identity] = duplicate_index + 1
162+
rows.append(
163+
Observation(
164+
project_id=self.project_id,
165+
entity_id=entity_id,
166+
content=obs.content,
167+
category=obs.category,
168+
context=obs.context,
169+
tags=obs.tags,
170+
duplicate_index=duplicate_index,
171+
)
153172
)
154-
for obs in observations
155-
]
156173
await self.add_all_no_return(session, rows)
157174
# add_all_no_return flushes, so every row now carries its database id.
158175
# Reading them here, inside the same transaction, is what lets the temporal

src/basic_memory/repository/postgres_search_repository.py

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,10 @@
3535
build_fts_page_stage,
3636
)
3737
from basic_memory.repository.metadata_filters import parse_metadata_filters
38+
from basic_memory.repository.note_type_filters import (
39+
POSTGRES_NOTE_TYPE_VALUE,
40+
build_note_type_predicate,
41+
)
3842
from basic_memory.repository.temporal_filters import build_temporal_predicate
3943
from basic_memory.repository.semantic_errors import SemanticDependenciesMissingError
4044
from basic_memory.repository.semantic_vector_index import SemanticVectorIndex
@@ -1169,19 +1173,18 @@ async def _build_fts_query_parts(
11691173

11701174
# Handle note type filter (frontmatter type field, parameterized).
11711175
# Trigger: caller passed `note_types` to scope by the frontmatter `type` field.
1172-
# Why: the stored note_type preserves the frontmatter casing (e.g. `Chapter`),
1173-
# but the filter is documented case-insensitive. JSONB `@>` containment is
1174-
# exact-match, so capitalized types were unfindable.
1175-
# Outcome: compare LOWER(metadata->>'note_type') against lowercased filter
1176-
# values so `note_types=["Chapter"]` matches a stored `Chapter`.
1176+
# Why: the type belongs to the note, but only its entity row carries the
1177+
# frontmatter; observation and relation rows do not. Reading it off each row
1178+
# silently excluded every non-entity row, which made `note_types` combined
1179+
# with a valid-time filter unsatisfiable.
1180+
# Outcome: resolved through the owning note in one shared builder, so both
1181+
# backends ask the same question and observation rows of a matching note
1182+
# are admitted.
11771183
if note_types:
1178-
type_placeholders = []
1179-
for idx, note_type in enumerate(note_types):
1180-
param_name = f"note_type_{idx}"
1181-
params[param_name] = note_type.lower()
1182-
type_placeholders.append(f":{param_name}")
11831184
conditions.append(
1184-
f"LOWER(search_index.metadata->>'note_type') IN ({', '.join(type_placeholders)})"
1185+
build_note_type_predicate(
1186+
note_types, params, note_type_value=POSTGRES_NOTE_TYPE_VALUE
1187+
)
11851188
)
11861189

11871190
# Handle date filter

src/basic_memory/repository/sqlite_search_repository.py

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,10 @@
4040
build_fts_page_stage,
4141
)
4242
from basic_memory.repository.metadata_filters import parse_metadata_filters, build_sqlite_json_path
43+
from basic_memory.repository.note_type_filters import (
44+
SQLITE_NOTE_TYPE_VALUE,
45+
build_note_type_predicate,
46+
)
4347
from basic_memory.repository.temporal_filters import build_temporal_predicate
4448
from basic_memory.repository.semantic_errors import SemanticDependenciesMissingError
4549
from basic_memory.repository.semantic_vector_index import SemanticVectorIndex
@@ -909,20 +913,18 @@ async def _build_fts_query_parts(
909913

910914
# Handle note type filter (frontmatter type field, parameterized).
911915
# Trigger: caller passed `note_types` to scope by the frontmatter `type` field.
912-
# Why: the stored note_type preserves the frontmatter casing (e.g. `Chapter`),
913-
# but the filter is documented case-insensitive; comparing raw values
914-
# would miss capitalized types.
915-
# Outcome: fold both sides to lowercase so `note_types=["Chapter"]` matches a
916-
# stored `Chapter`, `chapter`, etc.
916+
# Why: the type belongs to the note, but only its entity row carries the
917+
# frontmatter; observation and relation rows do not. Reading it off each row
918+
# silently excluded every non-entity row, which made `note_types` combined
919+
# with a valid-time filter unsatisfiable.
920+
# Outcome: resolved through the owning note in one shared builder, so both
921+
# backends ask the same question and observation rows of a matching note
922+
# are admitted.
917923
if note_types:
918-
type_placeholders = []
919-
for idx, t in enumerate(note_types):
920-
param_name = f"note_type_{idx}"
921-
params[param_name] = t.lower()
922-
type_placeholders.append(f":{param_name}")
923924
conditions.append(
924-
"LOWER(json_extract(search_index.metadata, '$.note_type')) "
925-
f"IN ({', '.join(type_placeholders)})"
925+
build_note_type_predicate(
926+
note_types, params, note_type_value=SQLITE_NOTE_TYPE_VALUE
927+
)
926928
)
927929

928930
# Handle date filter using datetime() for proper comparison

tests/repository/test_postgres_search_repository.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,10 @@ async def test_postgres_search_repository_index_and_search(session_maker, test_p
142142
permalink="docs/coffee-brewing",
143143
file_path="docs/coffee-brewing.md",
144144
type="entity",
145+
# An entity row addresses itself: every indexing path sets entity_id on all three
146+
# row kinds, and note_type is resolved through it, so a row built by hand here
147+
# must carry it too or it belongs to no note at all.
148+
entity_id=1,
145149
metadata={"note_type": "note"},
146150
created_at=now,
147151
updated_at=now,

0 commit comments

Comments
 (0)