memory: seed the categories memU actually advertises - #259
Closed
oranjeai wants to merge 5 commits into
Closed
Conversation
nerve forwards its own memory categories to memU only when config.memory.categories is non-empty; otherwise memU falls back to its 10 built-in defaults. Those defaults are formatted into the memorize prompt, so the LLM is told they exist. But _ensure_categories keyed its work on nerve's config, early-returning when it was empty, and nerve also sets categories_ready=True, which suppresses memU's own category initializer. On a default-config install nothing was ever created: 10 categories advertised, 0 rows, an empty name-to-id map, and every category name the LLM emits silently dropped by _map_category_names_to_ids. Category-scoped recall and every category summary stay permanently empty with no error anywhere. config.example.yaml ships a memory: block with no categories key and MemoryConfig.categories defaults to an empty list, so only wizard-created installs avoid this. Seed from self._service.category_configs, the effective set memU advertises, and load the persisted rows unconditionally so the name-to-id rebuild sees a warm cache even when nothing needs creating. Seeding writes through the category repository rather than _create_category_impl. That method appends a CategoryConfig for a name memU already advertises, so using it here doubles the advertised set. This is also live today on the configured path: a cold start with 3 configured categories left 6 advertised entries and listed each one twice in the memorize prompt. Runtime creation keeps using _create_category_impl, which is correct for a category memU does not yet advertise. Embeddings are computed in one batched call before any write, and a failure propagates. A row persisted with a null embedding is never repaired, because get_or_create_category returns an existing row untouched and a later boot skips the name, while both category rankers drop null vectors. Falling back to None would trade one silent failure for a permanent one. embedding=None remains correct when no provider is configured, where retrieval does not rank by vector. Availability moves to the end of _initialize_impl. It was published ~290 lines early, before category seeding, and engine.py discards initialize()'s return value, so _available is the only failure signal the running agent sees. A category created at runtime is resolvable again after a restart, but memU rebuilds the advertised set from config at construction, so the LLM is still not told it exists. That half needs the category_configs lifecycle and is not addressed here.
zip(..., strict=True) raises as the loop advances, so a provider returning fewer or more vectors than categories left rows already written before the error. Pair up front instead: nothing is written unless every category has a vector.
The three end-to-end arms in TestInitializeCategoryInvariant run a real MemUBridge.initialize(), whose warmup loop opens three LLM clients. Nothing pointed those at a stub, so each arm dialled api.anthropic.com: measured with strace -f -e trace=connect, tests/test_memu_bridge.py made 12 connections to port 443 and took 20.4s, against 0 and 3.7s at the base commit. A unit suite must not depend on a third-party endpoint, and this one passed quickly only because that endpoint rejects a fake key promptly; behind a blackholing proxy each arm would instead burn the warmup's 15s-per-profile timeout. Neutralize MemoryService._get_llm_base_client for the duration of _initialize_impl. It is the only client factory the warmup calls and the loop already swallows its exceptions, so no observable behaviour changes: every existing assertion in all three arms is unchanged and still passes. Make the offline property an assertion rather than a comment. The probe installs a socket.socket.connect that records and rejects, and reports offline = (nothing dialled) and (the blocker is installed). Both conjuncts are load-bearing: httpx reaches the network through socket.socket.connect and never through socket.create_connection (measured), the warmup's except Exception swallows a raise so only the record can fail a test, and without the second conjunct an empty record would also be satisfied by a blocker that was never armed. Mutation-checked in both directions: removing the warmup stub or the blocker fails the arms, a semantically identical no-op edit does not. tests/test_memu_bridge.py: 6 failed / 87 passed, 0 connections to :443, 5.6s. Whole suite: 2957 collected, 7 failed / 2950 passed, with the failing-name set unchanged (all pre-existing timezone artifacts). ## Coordination The sibling branch oranjeai/memu-category-name-normalization rewrites the same function and conflicts with this one: git merge-tree --write-tree reports CONFLICT on both nerve/memory/memu_bridge.py and tests/test_memu_bridge.py. The two changes are complementary in intent but incompatible in code -- that branch keeps the per-category create loop and normalizes names, while this one replaces the loop with a repo-level seed, and it still iterates the configured categories so it does not fix the empty-config case. Whichever lands second must rebase rather than auto-resolve, since a careless resolution can reinstate the double-advertising or drop one of the two fixes.
_seed_categories persisted, embedded and mapped the configured name verbatim,
and _ensure_categories compared raw names for its already-exists skip. memU
does not: _format_categories_for_prompt advertises name.strip() or "Untitled"
(memu/app/memorize.py:930-938), its own initializer persists that same form
(:663-665), and all three copies of the reverse lookup key on
name.strip().lower() (crud.py, patch.py, memorize.py). So a configured name
that is not already normalized produced a row under a key no consumer computes.
Measured with a full initialize() against an empty store and
categories = [{name: " alpha "}, {name: " "}]: the prompt advertises alpha
and Untitled, the rows and the name-to-id map hold ' alpha ' and ' ', and
mapping the advertised names returns [] while initialize() reports success.
That is the failure mode this branch exists to remove -- 0 of the advertised
categories can receive anything -- reached by a padded name instead of an empty
config, so the invariant the branch states does not hold on its own tree.
Route the four sites through one _memu_cat_name helper matching memU exactly:
the existing-row comparison, the missing-config test, the embedding text, and
the persisted name and description. The embedding text now matches
_category_embedding_text byte for byte, including choosing the desc-less form on
the stripped description, so seeded vectors stay in the space cosine_topk ranks
in. The log line and the audit record name the stored row rather than the raw
config text, so the audit target_id identifies what was written.
The name-to-id rebuild in _initialize_impl and _create_category_impl are
deliberately untouched: once the row is stored normalized, the rebuild's
cat.name.lower() already yields memU's lookup key, and both sites are contested
with the sibling branch below.
A row already stored under a raw name is RENAMED to memU's form, unless another
row already owns the name memU would look it up by, so one row ends up carrying
the key every consumer computes. Merely recognising such a row is not enough:
normalizing only the comparison suppresses the seed AND leaves the raw row
unresolvable, which is worse than not normalizing at all. nerve's own
update_category wrapper forwards summary and description only, but the repo
layer does rename in all three backends, and the row id survives it, so existing
category_items stay linked.
Two key domains are in play, and they are not the same. memU's own
resolver computes name.strip().lower() (memu/app/memorize.py:682), while the
name-to-id rebuild in _initialize_impl keys on raw cat.name.lower(), without
stripping. The repair guard compares in the resolver's domain, because that is
what decides whether two rows are one category: a padded ' Alpha ' normalizes
to a display name no row holds while sharing the resolver key of a stored
'alpha', so renaming it would leave two live rows behind ONE
category_name_to_id entry, with the winner decided by repo.categories order
(list_categories applies no ORDER BY). A key owned by more than one row is
therefore left entirely alone: base's rows survive, and no items are discarded.
Occupancy, though, is judged in the REBUILD's domain, and over the names the
rows carry AFTER those repairs. A key that exists only as some padded row's
stripped form is not one any consumer computes, so treating it as taken
suppresses the seed that would supply it and leaves the advertised category
unresolvable. That is reachable whenever two or more stored rows share one
resolver key and none of them is already normalized, for instance an upgrade
from a config that once carried a trailing-space and a leading-space spelling
of the same name. base seeds a third row in that shape and does resolve, so
judging occupancy in the wrong domain is a regression against base rather than
an unfixed gap. The same post-repair comparison drives the already-exists
test, so a padded configured name still does not seed a second row beside a
case-differing one.
Repairs and seeds are planned first and written together, after ONE batched
embedding call. A renamed row is re-embedded from its normalized text for the
same reason its name is normalized: both rankers read the stored vector, so a
name-only rename would leave that row ranked in the space its raw text occupied.
Writing the rename before the batch also made an embedding outage leave a
half-migrated store -- renamed row, unseeded category -- so the write now
happens only once every vector is in hand. The rename is audited as
category_updated against the row id, matching _update_category_impl. A
repaired row keeps its own description: it may have been edited through the
API, and resolution does not depend on it.
tests/test_memu_bridge.py: 118 collected, 6 failed / 112 passed; the same 6
fail on unmodified origin/main. Against origin/main source with these tests, 39
fail at file scope -- 33 of the 42 tests this branch adds, plus those 6.
Against the previous commit's source, 11 fail: the same 6, plus the 5 arms this
round adds for the multi-owner unaddressable-key defect. Those 5 also fail at
origin/main, but for a different reason: base seeds the third row and does
resolve, so there they fail on the row set rather than on resolution. Whole
suite: 7 failed / 2975 passed, against 7 failed / 2933 passed on origin/main --
an identical 7-name failure set (pre-existing timezone artifacts), and +42
passing, matching the 42 collected names added exactly (0 removed). 17 of 18
mutants killed, the eighteenth a no-op control that passes; the mutant
restoring the previous commit's occupancy set is killed by the new arms in both
insertion orders.
## Coordination
The sibling branch oranjeai/memu-category-name-normalization (PR ClickHouse#251, now
de6932f) rewrites the same function and still conflicts with this one:
git merge-tree --write-tree reports CONFLICT (content) on both
nerve/memory/memu_bridge.py and tests/test_memu_bridge.py. That branch fixes
normalization too, by a different and incompatible mechanism: a
_norm_category_name that strips and lowercases, applied to the rebuild, to
_create_category_impl and to the runtime lookup, while keeping the per-category
create loop this branch replaces -- at de6932f it drops the pre-filter this
branch normalizes. Whoever lands second must keep exactly one of the two rather
than auto-resolve, because a careless resolution can reinstate the
double-advertising or drop one of the fixes.
Re-derived at handoff time against origin/main 94406ea: nine open PRs touch
these two files, and six of them conflict. ClickHouse#251 conflicts semantically -- it
rewrites the same function. ClickHouse#249 edits _ensure_categories and the runtime map
write, so read its resolution rather than auto-merging. ClickHouse#254, ClickHouse#255, ClickHouse#248 and
ClickHouse#70 conflict mechanically only, on test-file layout or nerve/bootstrap.py,
without touching the seeding path; ClickHouse#254 and ClickHouse#255 conflict in
tests/test_memu_bridge.py alone. ClickHouse#256, ClickHouse#252 and ClickHouse#247 merge cleanly. This
table moves hourly; re-derive it before resolving anything.
…lize()
Two properties the branch publishes had no arm that could observe them.
_available moves to the end of _initialize_impl, 273 lines after seeding,
because engine.py discards initialize()'s return value so _available is the
only failure signal the running agent sees. The one failure arm injected at
_ensure_categories, which raises before either the old or the new publication
point, so it passed identically at both positions. A new "fail-late" probe mode
raises at the interceptor registration instead: that site runs after seeding and
after _instrument_llm_timeouts, and is not inside the swallowing try that opens
below it, so the raise reaches the outer except and initialize() returns False.
The new arm asserts initialize/available/service_available are all False AND
that the 10 advertised defaults were already seeded, which is what distinguishes
it from the pre-existing arm rather than duplicating it. Verified by moving
_available/service_available/initialized_at back to immediately after the
_ensure_categories call: the new arm fails while the pre-existing one still
passes, and that asymmetry is the proof.
The restart property -- a row created at runtime is resolvable again after a
restart -- was covered only through a test-local helper that re-implements the
name-to-ID rebuild instead of executing it, so no arm observed the production
rebuild that actually makes the row addressable. Every full-initialize arm that
pre-stored a row also configured the same name, so none exercised a persisted
row the config does not advertise. A new full-initialize arm pre-stores "work"
with no configured categories and asserts through the real
ctx.category_name_to_id: "work" is mapped, keeps its pre-store id so its
category_items stay linked, is still absent from the advertised set and the
prompt, and does not disturb the seeding of the 10 defaults. On a clean
git archive export of origin/main it fails on exactly that map assertion
(assert 'work' in {}), because the list_categories() warming the rebuild's cache
sits behind the empty-config early return. Two further mutants pin it: deleting
the rebuild loop body, and restoring the base shape (the early return plus the
preload list_categories the branch removed), which is what shows the arm pins
the unconditional load and not merely the rebuild.
Test-only. nerve/memory/memu_bridge.py is byte-identical to the previous
commit. The inherited 18-mutant matrix was re-run against the amended tree
alongside the three new mutants; controls green at both ends. The file-scope
FAILED-name set is unchanged (the same 6 pre-existing timezone artifacts), and
ruff reports the same two pre-existing findings in the test file.
|
|
Contributor
Author
Internal second-model review -- 7 rounds, 37 findings (27 agreed and fixed, 6 disagreed with evidence, 4 dropped)Before opening this PR I ran it through an independent review harness: each round a second Six rounds each found a real defect in the previous round's fix. That is the honest
Where I was wrong, in public:
Disagreed, with evidence (recorded rather than silently dropped):
Coordination. This branch conflicts in both files with open PRs #251 and #249, and #251 is Review cost for this PR: $90.28 across 13 gated runs (6 approach rounds, 7 diff rounds). |
Contributor
Author
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-impl-slot-4:20260803-221500 |
Contributor
Author
|
@pufit could you take a look? You own most of The two things worth your judgment rather than my measurement:
Note it conflicts with #251 in both files, and that conflict is semantic, not textual: #251 keeps the |
Contributor
Author
|
Closing per @pufit's directive on #247: memU is being rewritten and sunset, and Nerve fixes |
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.
Symptom
With no
memory.categoriesin config, the memorize LLM is told 10 categories exist while thestore holds none, so every name it emits is silently dropped and category recall stays
permanently empty. Measured on an empty DB and config: advertised 10, rows 0, map 0, resolvable
0 of 10. That is the default:
config.example.yamlships nocategorieskey.Root cause
nerve seeded from
config.memory.categories, memU advertised fromservice.category_configs;they coincide only when nerve configures its own. Otherwise memU's 10 defaults reach the prompt,
_ensure_categoriesearly-returned, andcategories_ready=Truesuppressed memU's initializer.The
list_categories()warming the rebuild's cache sat behind that return too, so aruntime-created category was unresolvable after restart.
The fix
Seed from
service.category_configsand load persisted rows unconditionally. Seeding writesthrough the category repository, not
_create_category_impl: that also appends aCategoryConfigfor an already-advertised name, doubling the advertised set -- live today onthe configured path too.
Embeddings are computed in one batched call before any write, and a failure propagates: a
null-embedding row is never repaired, so a
Nonefallback would trade one silent failure for apermanent one.
_availablemoves to the end of_initialize_implfrom ~290 lines early, before seeding:engine.pydiscardsinitialize()'s return, so it is the agent's only failure signal.Behaviour changes: a default install creates 10 rows on first boot (idempotent by name); a row
whose stored name is not memU's form is renamed in place, keeping id and items, unless another
row already owns its lookup key; and an embedding outage or a category read error now fails
memory init loudly. Out of scope: null-embedding rows (#70); runtime-created categories stay
unadvertised.
Validation
44 new tests, 35 failing against unmodified source (41 at file scope, six pre-existing).
Suite 2933 -> 2977 passed, identical 7-name pre-existing failure set.