fix: resolve 8+ char id prefixes in memory_forget and memory_update - #955
Conversation
f1055d8 to
d6b01a3
Compare
6504247 to
0e3288a
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Reviewed head 0e3288a. The prefix happy paths and full suite pass, but there is one destructive-path blocker.
memory_forget now routes its direct memoryId argument through resolveMemoryId. Values that are neither a full UUID nor a validated prefix, including malformed input and supported legacy IDs such as mem-md-42, fall into semantic retrieval. That branch accepts a sole result without a minimum score; a probe resolved both not-an-id and mem-md-42 to an unrelated UUID at score 0.01, after which memory_forget deletes it. This also bypasses MemoryStore.delete's existing exact legacy-ID handling.
Please keep the direct-ID branch exact: accept full UUIDs, validated prefixes, and supported legacy IDs, reject malformed references, and reserve semantic lookup for the separate query flow with its confidence/confirmation safeguards. Add regression cases proving malformed and mem-md-N direct IDs cannot delete an unrelated row.
Non-blocking follow-ups: apply scope filtering before the prefix result cap, preserve fail-closed NULL-scope handling, and return distinguishable IDs for ambiguity responses.
|
The destructive-path blocker is fixed in 769a513. resolveMemoryId gains a requireExactRef mode, and the memory_forget direct memoryId branch uses it: full UUIDs and validated prefixes resolve as before, supported legacy mem-md-N ids pass through untouched to MemoryStore.delete's exact legacy handling, and anything else is rejected as an invalid reference instead of falling into semantic retrieval. Semantic resolution stays reserved for the query flow with its confidence and confirmation safeguards. Regressions added: a malformed direct id (not-an-id) and a legacy direct id (mem-md-42) each run against a store holding one unrelated row and assert the row survives and the response never claims a deletion. The malformed case is red on the previous head. Full chain and the core-regression group pass on this head. The three non-blocking notes (scope filtering before the prefix result cap, fail-closed NULL-scope handling, distinguishable ids in ambiguity responses) are tracked as follow-ups on our side. |
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head 769a513. The previous memory_forget blocker is fixed: malformed direct references now fail exact and legacy mem-md-N IDs retain exact handling. The targeted tests, full suite, and repository CI pass. One equivalent destructive-path issue remains in memory_update.
memory_update still calls resolveMemoryId without requireExactRef, even though its memoryId schema promises a UUID or prefix. With the new anchored classifiers, a malformed ID-shaped value such as 12345678-1234-1234-1234-123456789012, falls through to semantic retrieval, where a sole result is accepted without a minimum score. A focused probe selected an unrelated UUID at score 0.01; the update path can then update or supersede that unrelated row. Before this PR, the looser UUID-like branch kept this input on the exact path and failed safely.
Please make direct memory_update references exact as well, or introduce a separate explicit semantic-query selector with a confidence threshold and confirmation behavior. Add a regression proving malformed UUID-like update input cannot modify an unrelated row.
Follow-ups: preserve fail-closed NULL-scope handling in findByIdPrefix, apply scope filtering before the result cap, exclude superseded rows from prefix resolution, and return distinguishable candidate IDs for ambiguous prefixes.
|
Fixed in 6dee0b8. memory_update now resolves its direct memoryId with the same requireExactRef mode as memory_forget, so the probe input (a UUID-shaped value with a trailing comma) is rejected instead of falling through to no-minimum-score semantic retrieval. While closing this class I applied the same gate to the other two direct-id mutating paths, memory_promote and memory_archive: they carry an explicit dual selector (memoryId or query), so a supplied memoryId now resolves exactly while the explicit query selector keeps its documented semantic resolution. That matches the separate-selector shape you suggested, which those tools already had. Regressions added (all red-proofed against the pre-fix code): the exact review probe against memory_update proving a malformed UUID-shaped reference cannot modify or supersede an unrelated row, malformed promote/archive references leaving unrelated rows untouched, and promote-by-query still resolving semantically. Full suite 438/438, tsc clean, dist rebuilt in the same commit. On the four follow-ups (fail-closed NULL-scope in findByIdPrefix, scope filtering before the result cap, superseded-row exclusion from prefix resolution, distinguishable candidate ids for ambiguous prefixes): tracked on our side and queued as follow-up changes rather than folded into this PR, keeping this diff at the destructive-path contract. |
|
Thanks for addressing the |
6dee0b8 to
78f8201
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head 78f8201. The destructive semantic-fallback blockers are fixed across all direct-ID mutating paths: malformed memoryId values now fail exact resolution for forget, update, promote, and archive, while the explicit query selectors retain semantic behavior. The focused regressions, full suite, packaging checks, and repository CI pass on the clean rebased head. I found no remaining HIGH/CRITICAL issue. Approving.
Follow-ups worth addressing: apply scope authorization before the prefix result cap; keep NULL-scope rows fail-closed and out of ambiguity output; add real-store mixed-scope/multi-match tests; support uppercase imported UUID prefixes; and return distinguishable candidate IDs for ambiguous prefixes.
|
The current approved head |
|
Rebased onto master post #952. Only the test-registration union moved; no content changes. Gates green (typecheck, full suite, manifest verifier 102 entries, fresh dist). |
78f8201 to
5dd173f
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Reviewed head 5dd173f after the rebase. The exact-reference behavior remains fail-closed across forget, update, promote, and archive; focused tests, the full suite, and CI pass. The NULL-scope, scope-before-limit, uppercase imported-prefix, and distinguishable-ambiguity items remain the previously accepted follow-ups. No new merge blocker found.
|
Thanks for the update. After #934 merged, current head |
|
Rebased onto current master (post #934); the three review-round commits are squashed into one. Conflicts were the two test-registration files, resolved by union. Typecheck, build (dist recommitted), manifest verifier, and the full suite are green. Mergeable again. |
279c457 to
2a9df18
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head 2a9df18. The conflict resolution is limited to the test-registration union, and the exact-reference safeguards remain intact across forget, update, promote, and archive. The focused prefix tests, full suite, and GitHub CI are green.
The previously accepted follow-ups remain: apply scope authorization before the query cap, keep NULL-scoped rows fail-closed, support uppercase imported IDs, and return distinguishable IDs for ambiguous prefixes. These are not regressions introduced by this rebase and do not block the reviewed direct-ID safety fix.
Approving.
|
PR #941 has now merged, and this branch currently has merge conflicts with the latest master. Please rebase onto current master, resolve the conflicts, and push the updated branch. We will re-review the new head after CI completes. |
|
Rebased onto current master (post #941); registration files re-unioned, typecheck and the full suite pass, dist rebuilt in-commit. |
2a9df18 to
db75164
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head db75164. The conflict resolution preserves the previously approved prefix-resolution and fail-closed destructive-selector behavior; targeted tests, the full suite, and GitHub CI are green.
The scope-before-cap, NULL-scope exposure, imported-ID casing, legacy-ID compatibility, and ambiguity-response cases remain non-blocking follow-ups.
Approved.
|
PR #944 has now merged, and this branch currently has merge conflicts with the latest master. Please rebase onto current master, resolve the conflicts, and push the updated branch. We will re-review the new head after CI completes. |
db75164 to
77aaae6
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Reviewed head 77aaae6. The documented 8+ character prefix flow now resolves real scoped rows, direct mutating references fail closed, and the targeted tests, full local suite, and all GitHub CI checks pass.
Non-blocking follow-ups: apply scope authorization before the prefix-query result cap so mixed-scope collisions cannot appear falsely unique; keep raw NULL scopes fail closed and out of ambiguity text; expose distinguishable candidate IDs; support uppercase imported UUID prefixes; and restrict ellipsis stripping to ID references rather than semantic queries.
Approved.
…pdate, direct-id paths stay exact
|
Rebased onto master after #942's merge (registration-chain union only, no source conflicts). Full gates green: typecheck, fresh dist, manifest verifier, full suite. |
77aaae6 to
563e618
Compare
Problem
memory_update's parameter description has always promised "full UUID or 8+ char prefix", and agents naturally copy row ids out of injected context, where they are truncated (often with a trailing ellipsis, e.g.
407dec9c...). But the uuid detection regex accepted a bare 8-char prefix as a complete id, so a prefix-addressed forget or update went to the store as an exact-id lookup and always failed with "not found or access denied".This was caught live: an agent asked to delete a specific memory called memory_forget with the truncated ids it could see, got "not found" twice, and honestly reported that the deletion did not go through while the fact kept surfacing from its context.
Fix
store.findByIdPrefix(prefix, scopeFilter, limit): a LIKE lookup for hex-shaped 8-35 char prefixes, capped, with the same per-row scope accessibility check the other read paths use.resolveMemoryIdresolver now strips a trailing ellipsis, passes full UUIDs through unchanged, resolves hex-shaped refs as id prefixes (a unique match wins, multiple matches list candidates, zero matches is an honest not-found), and falls back to semantic search for everything else.memory_forget's direct-ID branch andmemory_update's inline copy of the resolution logic both route through the shared resolver.Tests
test/memory-id-prefix-resolution.test.mjsdrives the real registered tools against a real temp LanceDB store: prefix delete, ellipsis tolerance, full-UUID unchanged, ambiguity lists candidates without resolving, unmatched prefix deletes nothing, and prefix-addressed update supersedes with the new text. Registered in the CI chain and manifest.🤖 Generated with Claude Code