Skip to content

fix: resolve 8+ char id prefixes in memory_forget and memory_update - #955

Merged
rwmjhb merged 1 commit into
CortexReach:masterfrom
gorkem2020:fix/memory-id-prefix-resolution
Aug 8, 2026
Merged

fix: resolve 8+ char id prefixes in memory_forget and memory_update#955
rwmjhb merged 1 commit into
CortexReach:masterfrom
gorkem2020:fix/memory-id-prefix-resolution

Conversation

@gorkem2020

Copy link
Copy Markdown
Contributor

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

  • New scope-filtered 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.
  • The shared resolveMemoryId resolver 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 and memory_update's inline copy of the resolution logic both route through the shared resolver.

Tests

test/memory-id-prefix-resolution.test.mjs drives 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

@gorkem2020
gorkem2020 force-pushed the fix/memory-id-prefix-resolution branch from f1055d8 to d6b01a3 Compare July 18, 2026 15:53
@gorkem2020
gorkem2020 force-pushed the fix/memory-id-prefix-resolution branch 2 times, most recently from 6504247 to 0e3288a Compare July 28, 2026 11:58
@gorkem2020
gorkem2020 marked this pull request as ready for review July 28, 2026 11:58

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

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 rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

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.

@rwmjhb

rwmjhb commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Thanks for addressing the memory_update exact-reference blocker in 6dee0b8. The current head now has merge conflicts with the latest master, so I cannot reliably verify the fix against the code that will be merged. Please rebase onto the latest master, resolve the conflicts, rerun the focused ID-prefix tests and full CI suite, and push the resolved head for re-review.

@gorkem2020
gorkem2020 force-pushed the fix/memory-id-prefix-resolution branch from 6dee0b8 to 78f8201 Compare July 30, 2026 03:14
@gorkem2020

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (post #927/#943/#974 merges). No content changes: the only conflicts were the mechanical test-registration unions (package.json test chain and the CI manifest). All gates green on the new head (typecheck, full suite, manifest verifier, dist rebuilt fresh).

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rwmjhb

rwmjhb commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

The current approved head 78f8201 now has merge conflicts after #952 merged, so it can no longer be merged as reviewed. Please rebase onto the latest master, resolve the conflicts while preserving the exact-reference behavior and the newly merged category changes, rerun the focused ID-prefix tests plus the full CI suite, and push the resolved head for a quick re-check.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

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).

@gorkem2020
gorkem2020 force-pushed the fix/memory-id-prefix-resolution branch from 78f8201 to 5dd173f Compare July 30, 2026 04:35

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rwmjhb

rwmjhb commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the update. After #934 merged, current head 5dd173f now has merge conflicts with the latest master, so the approved revision cannot be merged as reviewed. Please rebase onto the latest master, resolve the conflicts while preserving the reviewed exact-reference ID resolution behavior, rerun the relevant targeted tests and full CI suite, and push the resolved head for a quick re-check.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

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.

@gorkem2020
gorkem2020 force-pushed the fix/memory-id-prefix-resolution branch 3 times, most recently from 279c457 to 2a9df18 Compare August 2, 2026 09:28

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rwmjhb

rwmjhb commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (post #941); registration files re-unioned, typecheck and the full suite pass, dist rebuilt in-commit.

@gorkem2020
gorkem2020 force-pushed the fix/memory-id-prefix-resolution branch from 2a9df18 to db75164 Compare August 3, 2026 13:41

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rwmjhb

rwmjhb commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (post #944 and #986), conflicts resolved (test registration union), full gates green. Mergeable again.

@gorkem2020
gorkem2020 force-pushed the fix/memory-id-prefix-resolution branch from db75164 to 77aaae6 Compare August 4, 2026 08:46

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gorkem2020

Copy link
Copy Markdown
Contributor Author

Rebased onto master after #942's merge (registration-chain union only, no source conflicts). Full gates green: typecheck, fresh dist, manifest verifier, full suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants