test(search): pin that /search never answers with a blank content (pm-0918-c-02) - #1705
Merged
Merged
Conversation
arkash20
force-pushed
the
fix/pm-0918-c-02-search-content
branch
from
September 23, 2026 11:41
0e52a83 to
1666f0f
Compare
Contributor
|
Claude Code Review — skipped: PR author 'arkash20' is not a public member of the 'caura-ai' org |
1 similar comment
Contributor
|
Claude Code Review — skipped: PR author 'arkash20' is not a public member of the 'caura-ai' org |
Contributor
Author
|
@Eldad-Caura please approve |
Eldad-Caura
approved these changes
Sep 23, 2026
arkash20
force-pushed
the
fix/pm-0918-c-02-search-content
branch
from
September 23, 2026 20:35
1666f0f to
4cc5d4c
Compare
Contributor
|
Claude Code Review — skipped: PR author 'arkash20' is not a public member of the 'caura-ai' org |
arkash20
force-pushed
the
fix/pm-0918-c-02-search-content
branch
from
September 23, 2026 20:47
4cc5d4c to
eefc904
Compare
Contributor
|
Claude Code Review — skipped: PR author 'arkash20' is not a public member of the 'caura-ai' org |
pm-0918-c-02 reported that for ~75 minutes after a 3,500-chunk bulk write, POST /api/v1/search returned rows whose `content` was empty — 17% of the top-50 — while the stored rows were fine. The previous investigation cleared the recall path and flagged one thing it had not traced: a serialiser on the REST /search route itself. Traced, and the route is clear. /search and /recall build their rows with the same `_memory_to_out` into the same `MemoryOut`, and the two responses carry a byte-identical 35-key field set (verified against a live route, not by reading). `MemoryOut.content` is `str` with no default, so a row that reaches the serialiser without content raises rather than degrading to "". There is no projection, no embedding-state branch and no truncation knob on this route that can blank the field. What there was no coverage for is the state the report describes. Three serialisation paths can produce a search row — scored search with a vector, scored search WITHOUT one (the post-bulk-write window, where `passes_relevance_filter` short-circuits on `has_embedding is False` and skips the similarity floor entirely), and successor injection, whose rows never pass through the scored-search projection at all. Only the first had a route-level test. These four cases drive the real route in all three, plus a guard that feeds injection a row with `content` removed — the shape a narrowed storage projection would produce. The guard is mutation-verified in both directions: giving `MemoryOut.content` a `""` default does NOT make it pass (`_memory_to_out` passes the key explicitly, so `None` still fails validation), while `content=_mem_attr(memory, "content") or ""` does — the `or ""` idiom used on eight other content reads in this codebase, just not on this one. That single-token edit is what the case exists to catch. Two harness facts the tests compensate for, both of which are why path 2 had never been covered: the test database is built from the ORM models, so migration 001's `search_vector` trigger does not exist and full-text match is dead suite-wide; and `track_task` runs the deferred re-embed in-process, so a "deferred" bulk write is embedded again before the next await, which makes `embedding_pending` useless as evidence of the state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Arkady Mankovsky <arkash20@gmail.com>
arkash20
force-pushed
the
fix/pm-0918-c-02-search-content
branch
from
September 23, 2026 20:58
eefc904 to
d99e505
Compare
Contributor
|
Claude Code Review — skipped: PR author 'arkash20' is not a public member of the 'caura-ai' org |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
pm-0918-c-02reported that for ~75 minutes after a 3,500-chunk bulk write,POST /api/v1/searchreturned rows whosecontentwas empty — 17% of the top-50 — on a fresh store written and queried immediately. The stored rows were fine (0 empty-content rows across 104,437 local memories), so the claim was that the emptiness was in the response.The previous investigation cleared the recall path and explicitly flagged one thing it had not traced: "a serialiser on the REST /search route rather than the recall route I traced". This PR is that trace, and its outcome: the OSS
/searchroute is clear, plus the coverage that was missing for the state the report describes.What was traced, and how it was cleared
POST /api/v1/search→SearchResponse(items: list[MemoryOut])MemoryOut.contentisstrwith no default — a row without content raises, it does not degrade to""/searchvs/recallfield set_memory_to_out; a live run of both routes returns the same 35 keys and the same content. Recall's only difference is stripping platform keys frommetadata(C25), which never touchescontentMEMORY_LIST_FIELDS(MEMORY_FIELDSminusembedding/search_vector); the outer query selects the wholeMemoryentitypasses_relevance_filtershort-circuits onhas_embedding is False— an un-embedded row bypasses the similarity floor and is admitted on its full-text match alone. That changes admission, not serialisation. It is the mechanism that links this row toax-0917-h-06LoadAndSerializeappendsfind_successorsrows that never pass through the scored-search projection; storage returns them withMEMORY_FIELDS, socontentis present/load-by-ids)MEMORY_FIELDSCAURA_SOURCE_ONLY/CAURA_SHOW_TITLEdo not exist anywhere in this repo — they are the caller's own adapter/documents/search, whose rows carrydata, notcontentat allThe tests
Three serialisation paths can produce a
/searchrow, and only the first had a route-level test:Four cases drive the real route (httpx against the app, real Postgres) in all three, and assert
contentis non-blank in every returned row. The fourth is the guard: it feeds injection a row withcontentremoved — the shape a narrowed storage projection produces, sinceorm_to_dictreads every field withgetattr(obj, f, None).The guard is mutation-verified in both directions. Giving
MemoryOut.contenta""default does not make it pass (_memory_to_outpasses the key explicitly, soNonestill fails validation). What does make it pass iscontent=_mem_attr(memory, "content") or ""— theor ""idiom used on eight other content reads in this codebase, just not on this one. That single-token edit is the realistic defect, and it is what this case catches.Two harness facts the tests compensate for
Both are why path 2 had never been covered, and both are worth knowing beyond this PR:
search_vectortrigger does not exist and the column is NULL on every row. Since FTS is the only admission route an un-embedded row has, nothing could reach that path._index_for_ftsrebuilds the vector for one tenant with migration 034's own expression.embedding_pendingis not evidence of the state.track_taskruns the deferred re-embed in-process, so rows written by adeployment_mode="deferred"bulk are embedded again before the nextawait— measured: vectors NULL in the database, present by the time the search ran one statement later. The flag staysTrueregardless (only core-worker clears it), so the cases assert onhas_embeddingfrom the diagnostic instead.What this does NOT settle
No defect was found, and that is a real result rather than a stalled one — but it is scoped to this repo. The residual gap is unchanged from the original investigation and is the same one cheap ask:
/searchresponse from runcaura-bulk-2k-top50-sessshowing one empty-content row WITH ITS MEMORY ID. With an id we can say whether the row is a fan-out child, a source chunk, or something else, and whether its stored content was ever empty./searchthrough the enterprise gateway, which is not traced here.🤖 Generated with Claude Code