feat(api): expose memory_type on create and update#10
Closed
doctatortot wants to merge 1 commit into
Closed
Conversation
The POST /v1/memories handler hardcoded MemoryType::Pinned as the
default for direct content creation, and PUT /v1/memories/{id} had no
way to change the type at all. As a result every API-created memory
ended up pinned, and there was no recovery path short of delete-and-
recreate.
This commit:
- Adds memory_type: Option<String> to CreateMemoryBody. When provided,
the value is parsed via MemoryType::FromStr (returns
Validation/400 on unknown variants). When absent the existing
default of Pinned is preserved so this is not a breaking change.
- Adds memory_type: Option<String> to UpdateMemoryBody with the same
parse-and-validate flow.
Tests:
- test_create_memory_with_type: POST with memory_type=insight returns
insight; POST without it still returns pinned (default-preserved).
- test_update_memory_type: PUT memory_type=insight on an existing
pinned memory demotes it.
- test_update_memory_type_invalid: PUT with an unknown type returns
400 Bad Request rather than silently accepting.
Note for reviewers: the create-side default of Pinned is preserved
here intentionally to avoid a behavior change in a single PR. Whether
Insight would be a better default is worth a separate design
discussion — happy to open a follow-up issue if the maintainer wants
to push on it.
Co-Authored-By: Claude Opus 4.7 <[email protected]>
yhyyz
added a commit
that referenced
this pull request
May 19, 2026
- test(api): install rustls CryptoProvider in setup_app (PR #6) Fixes ~20 api test failures from missing TLS provider - fix(retrieve): replace min-max RRF normalization with scale-and-clamp (PR #9) Preserves absolute quality signal; weak matches no longer inflate to 1.0 RRF_SCALE=61 so ideal dual-leg rank-1 maps to ~1.0. Closes #7 - feat(api): expose memory_type on create and update (PR #10) POST /v1/memories accepts memory_type (pinned|insight|session) PUT /v1/memories/:id can change memory_type Default remains 'pinned' for backwards compat Co-authored-by: doctatortot <[email protected]>
Contributor
|
Applied in Clean addition — the Re: the design note about whether |
This was referenced May 19, 2026
yhyyz
pushed a commit
that referenced
this pull request
May 21, 2026
Closes #8. Lets a caller consolidate fragmented memories into one new memory in a single API call, marking the olds as superseded so search naturally hides them while history stays recoverable. ## API surface * `POST /v1/memories` accepts an optional `replaces: string[]`. When present, the new memory is created and each old `id` in the list is marked `state=superseded`, `superseded_by=<new.id>`, `invalidated_at=now()`. * `GET /v1/memories/search` and `/v1/memories` (list) accept `include_superseded=true` to opt back into seeing them. * `PUT /v1/memories/:id` already accepted `state` via #10; it now parses `state=superseded` since the variant exists. * MCP plugin: `memory_store` gains an optional `replaces` array; description nudges the agent toward consolidate-not-delete semantics. ## Decisions matching the design comment | Maintainer point | Implementation | |---|---| | A1 (`replaces` on POST), not A2 | Done. | | Reuse `invalidated_at`, no new timestamp | `supersede_batch` sets it. | | New `Superseded` state variant | Added (`MemoryState::Superseded`). | | Default search/list excludes superseded | Filter is now `state NOT IN ('deleted', 'superseded')`; `include_superseded=true` opts back in. | | `memory_get` returns superseded regardless | `get_by_id` was already unfiltered; unchanged. | | 400 with list of missing IDs | `supersede_batch` precheck collects missing + already-superseded IDs and returns `Validation` with both lists; handler maps to 400. | | No cross-tenant check | Per design, stores are physically isolated; unknown ID → missing. | | 409 on already-superseded | Currently 400 (`Validation`) with explicit `already superseded: [...]`. Happy to switch to 409 if you'd rather have a distinct status — let me know. | | Tag inheritance: none | Caller controls tags on the new memory. | | Plugin: `replaces` on `memory_store` | Done. | ## Atomicity caveat LanceDB has no multi-row transactions. `supersede_batch` does the precheck up front (no writes happen on bad input) then creates the new memory, then loops marking olds. If a per-old mark fails after the new exists, the call returns `Storage(...)` naming the IDs that failed. This matches the semantics of the existing `handle_supersede` in `reconciler.rs:231` (create new → mark old). ## Tests Store layer (4 new): - `test_supersede_batch_marks_old_as_superseded` - `test_supersede_batch_rejects_missing_id` (precheck, no writes) - `test_supersede_batch_rejects_already_superseded` - `test_default_state_filter_excludes_superseded` API layer (3 new): - `test_create_with_replaces_supersedes_old` - `test_create_with_missing_replaces_returns_400` - `test_search_excludes_superseded_by_default` (+ verifies `include_superseded=true` resurfaces them) Full lib suite: **386 passed; 0 failed** (was 379 before this PR; +7 new tests, zero regressions). Co-Authored-By: Claude Opus 4.7 <[email protected]>
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
2 participants
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.
Summary
POST /v1/memorieshardcodedMemoryType::Pinnedas the default for direct content creation, andPUT /v1/memories/{id}had no way to change the type at all. As a result every API-created memory ended up pinned, and there was no recovery path short of delete-and-recreate.This PR:
memory_type: Option<String>toCreateMemoryBody. When provided, the value is parsed viaMemoryType::FromStr(returnsValidation/ 400 on unknown variants). When absent, the existing default ofPinnedis preserved so this is not a breaking change.memory_type: Option<String>toUpdateMemoryBodywith the same parse-and-validate flow.Motivation
I ran into this myself. After migrating ~170 memories into my omem instance via the API, every single one ended up
memory_type: pinnedand got the 1.5×pinned_boostinstage_rrf_fusion— effectively a uniform inflation across the corpus that defeats the boost's purpose. There was no API path to demote them; the only fix was delete-and-recreate.Tests
test_create_memory_with_type:POSTwithmemory_type=insightreturns insight;POSTwithout it still returns pinned (default-preserved, no regression for existing callers).test_update_memory_type:PUT memory_type=insighton an existing pinned memory demotes it.test_update_memory_type_invalid:PUTwith an unknown variant returns400 Bad Requestrather than silently accepting.Note for reviewers
The create-side default of
Pinnedis preserved here intentionally to avoid a behavior change in a single PR. WhetherInsightwould be a better default is worth a separate design discussion — happy to open a follow-up issue if you'd like to push on it.CI on this PR will still surface the pre-existing
cargo fmt --checkdebt and the rustls 0.23CryptoProvidertest failures (the latter is what #6 addresses); my new tests rely on the same fix #6 applies, so once #6 lands they'll go green. I validated them locally on my fork with the #6 patch cherry-picked in: all four scoped test buckets pass (store 34/34, retrieve::pipeline 19/19, the new create test 1/1, the new update tests 2/2).