memory: key nerve's category name lookups the way memU reads them - #251
memory: key nerve's category name lookups the way memU reads them#251oranjeai wants to merge 3 commits into
Conversation
ctx.category_name_to_id is the only name-to-id channel between nerve and memU. memU's contract is strip at creation, strip+lowercase at lookup: it strips before writing a row, and all three copies of its reverse lookup key on name.strip().lower(). nerve bypasses memU's own category producer (it sets categories_ready=True) and substitutes its own rebuild and creation paths, neither of which strips. So nerve writes a key the consumers never compute, and the id is unreachable. Two shapes, both reproduced on a real SQLite store: - A row named ' procedures ' is completely unreachable, with no collision anywhere: the map key keeps the padding, every consumer asks for 'procedures', and gets nothing. A collision query cannot see this. - 'procedures' and 'PROCEDURES' become two rows sharing one map key (get_or_create_category filters on the exact name and the table has no unique index on it), so whichever row loses is orphaned: no name-based path can reach it or the items linked to it. _ensure_categories amplified this. Its exact-match skip missed a stored 'PROCEDURES' for a configured 'procedures', so it created a second row on the next start and the rebuild then orphaned the older one -- a one-off UI typo silently dropped a user's category on a restart they did not initiate. Route every nerve name-to-id site through one normalized key, and resolve creation against that key so a variant reuses the existing row instead of adding one. Resolution sits above the embedding call on purpose: seeding invokes this path once per configured category on every start, so resolving below it would cost an embedding API call per category per restart -- the cost categories_ready=True exists to avoid. It is re-checked after the embed's await because the memU loop runs each create as its own coroutine with no lock, and without that recheck two concurrent creates would reintroduce the collision through the fix. Also register the reused row under its stored name in both places memU reads by name: category_config_map, the name it looks a row up by, and category_configs, the set the LLM prompt offers. The second matters because a reuse returns before the create path's own registration, so a persisted row with no configured counterpart -- a category made in the web UI -- would be reachable by id and still never be offered, i.e. nothing could ever be filed under it. That append is idempotent on the normalized name, so a configured spelling and a stored one are never advertised as two categories. This restores only the repair the unconditional create already performed; rebuilding category_configs from the DB at init is the separately filed defect and stays out. Also reject a blank category name at its origin (config parse and the HTTP route) rather than coercing it, since coercing turns a typo into a permanent row. A non-string name is rejected there too: a bare 'name:' in YAML parses as None and str() would make that a category literally named 'None', while a non-string has never been reachable anyway (nerve's name.lower() and memU's cfg.name.strip() both raise AttributeError on one). Two adjacent behaviour changes worth calling out. The category cache is now hydrated before seeding, because both seeding and the rebuild read a cache that starts empty in a fresh process; the init path already loads it unconditionally later, so this only moves the first call earlier. And the availability flags move to just before the successful return, so a failure cannot leave a half-initialized bridge reporting itself available -- the primary caller ignores the return value. Two pre-existing defects found next to this are deliberately left alone: category_configs is never rebuilt from the DB, and a deployment with no configured categories never seeds memU's defaults. Both are broken independently of this change and are tracked separately. No migration: this makes new collisions unreachable and does not merge, rename or delete rows that already collide. On such a store the mapping is left alone too, which needs saying because it does not follow for free: the rebuild assigns the shared key row by row, so the last row wins there, while a first-match scan would answer the other one. Registering that answer would repoint the key without touching either row, orphaning everything linked to the row that just lost it. So resolution prefers the row the map already points at, and falls back to the scan only where there is no mapping yet -- a fresh process before the rebuild, and seeding. It is a no-op on a clean store, where the old and new key sets are identical.
Pre-PR validation gate (a-i)
Mutation-tested. Every mutant is killed, with an unmutated control green at both ends of the run (so the harness cannot have drifted mid-matrix). Each mutator asserts its anchor applies exactly once, that the export carries this round's edits, and that the file still parses; a harness-level failure is printed as
The Also measured: each new class passes alone (9+6+3+6+2 = 26); running the new classes first leaves the same pre-existing failures; whole-repo Two reporting corrections, stated rather than quietly fixed. An earlier round reported "3 randomized-order runs stable", but On the 4 non-ASCII characters in the diff: they are the |
Internal second-model review: 3 rounds, 11 findings (7 agreed and fixed, 4 disagreed with evidence)Before this PR was opened it went through three independent review rounds by a second model, adjudicated against the code. Every finding and its outcome:
The two round-3 blockers, and why they are not upheldBoth name real defects. Neither is introduced or widened by this change, and refuting each needed a measurement rather than an argument. 9. Persisted category absent from the prompt after restart. The scenario needs an init-time write to 10. Cross-process duplicate rows. Measured in the finding's own scenario, with two genuinely separate OS processes over one SQLite file creating
So in the shape production actually runs, this change is what fixes the scenario, and there is no shape in which it is worse than base. A truly simultaneous insert - both processes past their own lookup before either commits - does remain possible, but it is equally possible at base, needs no case variant to occur, and cannot be closed here: the The two disagreed nits8 and 11 both ask for less commentary. Measured before answering: of 67 docstrings in Reviewer's own correctionsThe review is not only a filter on the gate. Round 2 found three stale claims in the description that the gate did not, and round 3 found three more that this round's changes introduced - the test count, the missing prompt-registration effect, and a no-migration promise that now covers the mapping too. All were corrected before this PR was opened. Two reporting corrections are disclosed in the validation comment rather than quietly fixed: an earlier round's "randomized-order runs" claim was vacuous (that plugin is not installed, so the command exited without running), and replacing it surfaced a genuine test-isolation defect - the new fixture leaves an item-repo monkeypatch applied on exit, so one pre-existing test fails under a non-collection order. It affects no production behaviour and not the sequential order CI runs, and is left for a separate change rather than widened into this one. CoordinationThis branch conflicts semantically with #249 (both relocate the availability statements and rewrite the create path's registration) and trivially with #248 (both append at the tail of the same test file). It merges cleanly against Gate spend on this PR: $55.11 over 11 gated rounds (8 approach, 3 code). |
|
|
_CategoryFixture applied MemUBridge._patch_sqlite_bugs() and restored nothing,
so the wrapper it installs over SQLiteMemoryItemRepo.update_item outlived the
`with` block. TestIndexedUpdateItemForwarding reads that method's signature
(parameters["item_id"]), so any order running the category tests first turned it
red with KeyError: 'item_id' about 200 lines from the cause:
pytest tests/test_memu_bridge.py::TestCategoryNameNormalization \
tests/test_memu_bridge.py::TestIndexedUpdateItemForwarding -q
-> 1 failed, 10 passed
The snapshot has to be taken in __init__, not __enter__: __init__ calls
_category_models(), which patches first, so an __enter__-scoped snapshot would
already hold the patched methods and restore nothing. Measured both scopes.
Extends the save/restore idiom test_wrapper_forwards_item_id_as_keyword already
uses in this file, over the same seven names; six of them are what the patch
reassigns. schema_mod.get_sqlite_sqlalchemy_models stays replaced on purpose --
the models are cached and no test introspects it.
The new test observes the leak inside one body, so it needs no class ordering,
and it checks every name the patch reassigns rather than only the one the
neighbour reads.
Latent, not a live break: the victim class collects ahead of both polluters, so
CI's sequential `pytest tests/ -v` passes today; pytest-xdist is installed, so
sharding would expose it.
Internal second-model review (click to expand)An independent reviewer read this commit cold (without the author's evidence), then a second model
1. Scope was overstated (fixed where I own the text, disclosed where I do not). The code scope is right and I am not asking for it to change: no test under
2. The docstring is still too strong. 3. Mutant driver traceback. The "narrow the restore list" mutant's driver asserts a name tuple What I checked and found clean: the Rounds: 1 review round, 0 fix bounces. Gate spend: $4.26 (Gate A $1.37, Gate B $2.89). |
Pre-PR validation gate (click to expand)
Mutants, to show the new test is not vacuous (unmutated control run at three points, all green; each mutant asserts anchor hit count == 1 and that the file still parses): reverting only the Suites compared by name, never by count: Session id: cron:clickhouse-impl-slot-5:20260803-151900 |
d6e4c50 made the fixture restore the SQLiteMemoryItemRepo methods on exit. Three prose surfaces on this branch still claim the restore covers the whole of _patch_sqlite_bugs(), which overstates the isolation in the harmful direction: readers conclude the fixture leaves nothing applied. _patch_sqlite_bugs() has 18 source mutation sites that fire (27 concrete attribute mutations, because two loops iterate several model classes). d6e4c50 restores 6 of those sites, all on SQLiteMemoryItemRepo. The other 12 stay applied after __exit__, and two of them are irreversible (model_fields.pop('embedding'), del __annotations__['embedding']), so a full restore is not available. Nothing observes the residue today: no test that skips the fixture references any residual symbol. Corrected here, one qualifier each and no narrative growth: * the fixture docstring, which said nothing is left applied on exit; * the regression test's docstring, whose object was _patch_sqlite_bugs() rather than the SQLiteMemoryItemRepo patches it actually pins; * the in-test comment above the leak check, now naming the repo whose names the loop compares. Prose only. The executable AST with docstrings stripped is byte-identical to d6e4c50, the line count is unchanged, and the full suite has the same 7 pre-existing failures before and after (2960 passed). d6e4c50's own commit message carries the same overstatement in two paragraphs. It is deliberately left as is: our branch policy is to add new commits rather than rebase or amend, and the two surfaces a maintainer reads when deciding to merge -- the PR description and the validation comment -- were both corrected at review time.
Pre-PR validation gate (third commit) (click to expand)
Figures always with their partition, since a bare count here has an unstated denominator. Mutants re-run in full with the unmutated control at both ends, all green ( Suites compared by failure name, never by count: whole repo 7F/2960P before and after,
|
Internal second-model review (third commit: 3 rounds, 12 findings) (click to expand)An independent model (
On #9 (the only substantive disagreement). The mechanism is real and I reproduced it: in the
It is also already addressed by #249, whose On #12. That string is an Correction to an earlier comment on this PR. The validation comment for the previous commit Two things I got wrong and corrected. My earlier round left One pre-existing comment elsewhere in the file makes the same kind of overstatement about a Rounds: 3. Independent-review spend: $23.18. |
|
Closing per @pufit's directive on #247: memU is being rewritten and sunset, and Nerve fixes |
2 participants
Symptom
A name differing only in case or whitespace becomes unreachable, with every item linked to it:
' procedures ': unreachable with no collision anywhere -- the padded map key never matches the'procedures'readers ask for.'procedures'and'PROCEDURES': two rows sharing one map key, orphaning the loser._ensure_categoriesamplified it: its exact-match skip missed a stored'PROCEDURES'for a configured'procedures', so the next start added a second row the rebuild then orphaned. The survivor is stable, so it never self-heals.Root cause
memU's contract is strip at creation, strip+lowercase at lookup: all three copies of its reverse lookup key on
name.strip().lower().nerve bypasses that producer (
categories_ready=True) and substitutes its own rebuild and creation paths. Neither strips, so the key nerve writes intoctx.category_name_to_idis not the key readers compute.get_or_create_categoryfilters on the exact name over a table with no unique index, so it cannot deduplicate.The change
Every nerve name-to-id site now derives one normalized key; creation resolves against it, so a variant reuses the existing row, registered under its stored name (the one memU reads) and offered in the LLM's category prompt, so items can still be filed under it. A blank or non-string name is rejected at config parse (YAML
name:would otherwise become a category namedNone); the route rejects blanks.Resolution runs above the embedding call and again after its await. Also: the cache is hydrated before seeding, and the availability flags move to just before the successful return.
Validation
Reproduced on a real SQLite store through the bridge's own code, both directions: 23 of 26 new tests fail against unfixed code on real assertions, and the full-suite failure set is unchanged. A no-op on a clean store; an already-collided store keeps both rows and its existing mapping.
Second commit:
tests: restore the item-repo methods when _CategoryFixture exits_CategoryFixture(added above) appliedMemUBridge._patch_sqlite_bugs()and restored nothing, sothe wrapper it installs over
SQLiteMemoryItemRepo.update_itemoutlived thewithblock and turnedTestIndexedUpdateItemForwardingred under any order that ran the category tests first. Latent, nota live break: the victim collects ahead of both polluters, so CI's sequential run passes today.
The snapshot has to be taken in
__init__, not__enter__--__init__calls_category_models(),which patches first. Test-only,
+60/-0, plus one order-independent regression test. Scope: thisrestores the
SQLiteMemoryItemRepomethods, not all of what the patch touches: of the 18 sourcemutation sites that fire (27 concrete attribute mutations, since two loops iterate several model
classes) it restores 6. Widening it is possible but pointless: no test observes any of the
remaining 12, and restoring the two model-field mutations brings back the SQLAlchemy type
error they exist to remove. A third commit scopes the fixture's prose to match.