feat(search): let a caller or a tenant exclude fan-out children (09/18 c-03) - #1708
Merged
Merged
Conversation
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
feat/pm-0918-c-03-include-derived
branch
from
September 23, 2026 17:23
fe2ddc4 to
0171610
Compare
Contributor
|
Claude Code Review — skipped: PR author 'arkash20' is not a public member of the 'caura-ai' org |
arkash20
force-pushed
the
feat/pm-0918-c-03-include-derived
branch
from
September 23, 2026 20:28
0171610 to
ffcb338
Compare
Contributor
|
Claude Code Review — skipped: PR author 'arkash20' is not a public member of the 'caura-ai' org |
…8 c-03) A70's fan-out writes a short single-claim child per extracted fact, and those children stay retrievable alongside the parent they came from. On the store that prompted this they were ~28% of returned rows — a caller asking for 50 memories got 36 memories and 14 fragments, worth 79.8% vs 82.2% on that harness. Filtering them client-side does not work, and that is the design constraint: dropping rows after the response distorts top_k. Ask for 50, drop 14, get 36, and now over-fetch and guess. So the filter runs server-side and BEFORE the trim, where the existing 2x SEARCH_OVERFETCH_FACTOR absorbs it — at the measured ~28%, 100 candidates become ~72 survivors and all 50 slots still fill. Three layers, request beats tenant beats global: include_derived on SearchRequest per query, tri-state search.include_derived per tenant INCLUDE_DERIVED_DEFAULT = True today's behaviour, unchanged The global default deliberately does NOT move. POST /search is in the frozen broker subset and a default flip is a semantic change that oasdiff cannot see, so it needs a BREAKING CHANGE trailer and a second store's evidence; docs/atomic-fact-fanout/pm-c03-include-derived-blast-radius.md sets out what that would take. A tenant that wants that behaviour now sets search.include_derived=false and gets it for its own store. The predicate is a conjunction and both halves are load-bearing: parent_memory_id IS NOT NULL AND source = 'atomic_fact_fanout' parent_memory_id alone also catches auto_chunk children, which are NOT redundant — the parent holds the whole document but carries one embedding over all of it, so those children are the only vectors that can match a specific passage. Excluding them would surface days later as "long documents stopped being findable" with nothing pointing here. And source alone is forgeable: it is deliberately absent from PLATFORM_ONLY_KEYS because ingest writes it too. The argument lives in a comment at the predicate, not only in the doc. Applied in PostFilterResults and in the legacy search path, from one shared helper in search_trim.py, so the _USE_PIPELINE_SEARCH rollback lever cannot silently revert it — two ranking features already shipped pipeline-only for exactly that reason. The settings knob is registered in DEFAULT_SETTINGS and in _LEAF_TYPES as bool, not only as a ResolvedConfig property. pm-0918-c-04 shipped that way and the knob was unsettable: _check_keys rejected every write, PUT /settings answered 422, and the resolver served the default anyway, so it read as working. test_the_tenant_default_survives_a_real_settings_put goes through the HTTP route rather than constructing a ResolvedConfig, because constructing it directly is what hid that. Verified by removing the DEFAULT_SETTINGS entry and watching the test fail with the exact c-04 error: 422 "Unknown settings key(s): ['search.include_derived']". The bool leaf type matters on its own: "false" is a truthy string, so without it a tenant that switched derived rows off would resolve to ON while the dashboard rendered their "off" back to them. Also names the exclusion in the D12 diagnostic rather than letting it read as trimmed_by_top_k — the one label that would send someone raising top_k to get the row back, which cannot work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Arkady Mankovsky <arkash20@gmail.com>
Additive: one new optional, nullable request property on SearchRequest. No path, method, required field or type is removed or narrowed. In its own commit because POST /search is in the frozen broker subset, so a reviewer should see the contract move even when the move is benign. Verified rather than asserted — the pinned oasdiff image reports "No breaking changes to report, but the specs are different" and exits 0 against origin/main's baseline, and --check passes. Worth knowing while looking at this file: that gate would ALSO have passed silently had this PR flipped the default instead of adding the field. A default value moving is a semantic change with no schema movement, and oasdiff has nothing to compare. See docs/atomic-fact-fanout/pm-c03-include-derived-blast-radius.md §4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Arkady Mankovsky <arkash20@gmail.com>
Review asked for the asymmetry to be explicit rather than left in a
handover, and for the MCP passthrough behaviour to be verified rather
than reasoned about. Both, plus the near-miss that fixed the settings
test's assertion order.
THE VERIFICATION CHANGES THE STORY. The argument for leaving the MCP
tool surface out of this PR was that /recall INHERITS include_derived
and would silently ignore it (divergence), whereas MCP does not expose
it at all, so nothing is silently dropped (a limitation). The first half
stands and is why /recall is wired. The second half is false as
measured: probing mcp.call_tool("caura_recall", ...) through the real
dispatch shows an unknown argument is swallowed during argument binding,
before the handler, with a normal success envelope and no warning —
search_memories never sees it. The same probe shows `limit: 2` returning
effective_top_k: 5, which is ax-0917-h-05's original defect, unfixed, on
the MCP surface.
That is broader than this row and is filed separately as oss-0923-h-01
rather than widening this PR: it is the MCP argument-binding path, so it
applies to all 12 tools, and the fix belongs at the call_tool chokepoint
that already inspects the raw arguments dict.
So the field description now says what a caller needs at the point of
use: REST only, use the tenant setting from MCP, and an unknown argument
to an MCP tool is dropped silently rather than reported.
Also moves the assertion-order rationale into the settings test's
docstring. The first draft of that test asserted the unset read shape
before the PUT, and under the reintroduced c-04 defect it failed on a
KeyError from the GET — a true report of a different thing, one step
removed from the defect being guarded. A test for an unsettable knob has
to fail on the write.
Broker baseline regenerated for the description text; --check passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Arkady Mankovsky <arkash20@gmail.com>
arkash20
force-pushed
the
feat/pm-0918-c-03-include-derived
branch
from
September 23, 2026 20:42
ffcb338 to
6238a82
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.
Implements the decision taken on pm-0918-c-03: option (a) — a request flag defaulting to today's behaviour, plus a per-tenant default. The evidence behind the choice is in #1703 (docs-only, merges independently).
A70's fan-out writes a short single-claim child per extracted fact, and those children stay retrievable alongside their parent. On the store that prompted this they were ~28% of returned rows — a caller asking for 50 memories got 36 memories and 14 fragments, worth 79.8% vs 82.2% on that harness.
Filtering them client-side does not work, and that is the design constraint: dropping rows after the response distorts
top_k. Ask for 50, drop 14, get 36, then over-fetch and guess. So the filter runs server-side and before the trim, where the existing 2×SEARCH_OVERFETCH_FACTORabsorbs it — at ~28%, 100 candidates become ~72 survivors and all 50 slots still fill.Three layers, request beats tenant beats global
include_derivedon the requestPOST /search,POST /recallsearch.include_derivedPUT /settingsINCLUDE_DERIVED_DEFAULTconstants.pyTrue— unchangedThe global default deliberately does not move.
/searchis in the frozen broker subset and a default flip is a semantic changeoasdiffcannot see, so it needs aBREAKING CHANGE:trailer and a second store's evidence. A tenant that wants that behaviour today setssearch.include_derived=falseand gets it for its own store.The predicate is a conjunction, and both halves are load-bearing
parent_memory_idalone also catches auto-chunk children, which are not redundant — the parent holds the whole document but carries one embedding over all of it, so those children are the only vectors that can match a specific passage. Excluding them would surface days later as "long documents stopped being findable" with nothing pointing at this filter. Andsourcealone is forgeable: it is deliberately absent fromPLATFORM_ONLY_KEYSbecause ingest writes it too. The argument sits in a comment at the predicate, not only in the doc.Applied in
PostFilterResultsand in the legacy search path from one shared helper insearch_trim.py, so the_USE_PIPELINE_SEARCHrollback lever cannot silently revert it — two ranking features already shipped pipeline-only for exactly that reason.Not repeating the c-04 defect
search.include_derivedis registered inDEFAULT_SETTINGS, in_LEAF_TYPESasbool, and on the resolver. On pm-0918-c-04 the switch existed only as aResolvedConfigproperty:_check_keysrejected every write,PUT /settingsanswered 422, and the resolver served the default anyway — so it read as shipped.test_the_tenant_default_survives_a_real_settings_putgoes through the HTTP route, not a constructedResolvedConfig, because constructing it directly is what hid that. Verified by removing theDEFAULT_SETTINGSentry and re-running, which fails with the exact c-04 error:The
boolleaf type earns its place separately:"false"is a truthy string, so without it a tenant that switched derived rows off would resolve to ON while the dashboard rendered their "off" back to them.Verification
origin/main.ruff check+ruff format --checkclean.mypyreports zero errors in the changed files (the 142 in the transitive graph are pre-existing and identical at baseline).--checkpasses and the pinnedoasdiffimage reports "No breaking changes to report, but the specs are different", exit 0.MCP: a limitation, and a silent drop that is now its own row
This PR does not add
include_derivedto the MCP tools — that pulls in the 40+ location tool-surface checklist, and the tenant setting covers MCP callers./recallwas wired, and the line between the two is worth stating so it does not read as inconsistent:RecallRequestsubclassesSearchRequest, so a caller can send the field to/recallwhether or not the route reads it — accepting and ignoring it is silent divergence, the class of ax-0917-m-19 / m-16 / oss-0922-m-03. MCP does not expose the field at all, which is a limitation rather than a divergence.That last claim was checked rather than assumed, and it did not survive. Probing
mcp.call_tool("caura_recall", ...)through the real dispatch:include_derived: falsesearch_memoriesreceivedinclude_derived=<ABSENT>bogus_param_xyz: 2limit: 2effective_top_k: 5Every one returns a normal success envelope with
isErrorabsent. The argument is dropped during the SDK's argument binding, before the handler. So MCP is a silent-drop surface too — andlimitsilently returning the default 5 is ax-0917-h-05's original defect, still unfixed on the surface agents actually use.That is broader than this row (it is the argument-binding path, so all 12 tools) and is filed as
oss-0923-h-01rather than widening this PR. The field description now says so at the point of use: REST only, use the tenant setting from MCP, and an unknown MCP argument is dropped silently rather than reported.For Arkady — flipping PersonaMem's tenant
Not run here: this is a live-data action on a real tenant. Idempotent, replace the placeholder:
Read it back with
curl -sS "https://memclaw.dev/api/v1/settings?tenant_id=<PERSONAMEM_TENANT_ID>" -H "X-API-Key: $CAURA_API_KEY" | jq .search.include_derived— expectfalse. Settings are cached per tenant, so allow for the cache TTL before re-measuring.🤖 Generated with Claude Code