Skip to content

fix: preserve long-query retrieval and reject only credential literals - #262

Open
zuyu wants to merge 1 commit into
mainfrom
codex/preserve-long-query-retrieval
Open

zuyu wants to merge 1 commit into
mainfrom
codex/preserve-long-query-retrieval

Conversation

@zuyu

@zuyu zuyu commented Oct 3, 2026 •

Copy link
Copy Markdown

What type of PR is this?

  • fix (bug fix)

Which issue(s) this PR fixes

No linked issue.

What this PR does / why we need it

Long full-text queries could overwhelm a single database query, and punctuation-only diagnostic tokens could prevent otherwise useful terms from being searched. Batch all distinct terms into groups of 64, merge their lexical scores, and apply the result limit after merging while preserving user and optional retrieval scopes.

Credential detection now distinguishes ordinary prose and source-code references from literal credentials, reducing false-positive ingestion failures. Regression tests cover long-query term preservation, diagnostic underlines, benign references, and credential literals.

Validation

  • git diff --check e180dff..18106fd
  • At the stack tip (23725db, including the follow-up credential fix): cargo test --locked --offline -p memoria-core --lib sensitivity::tests — 11 passed.
  • At the same stack tip: cargo test --locked --offline -p memoria-storage --lib fulltext — 4 passed.

This PR contains only commit 18106fd. The dependent fix is #264, which remains a draft with do-not-merge until this PR merges into main. Afterward, rebase #264's single commit onto main, retarget it to main, and remove its block. Merge both PRs before deploying.

mergify Bot pushed a commit that referenced this pull request Oct 3, 2026
## What type of PR is this?

- [x] fix (bug fix)

## Which issue(s) this PR fixes

No linked issue.

## What this PR does / why we need it

The credential-reference exceptions could hide literal secrets in default arguments, multiline expressions, or nested assignments. Inspect complete right-hand-side expressions, constrain lookup exceptions to pure lookups, detect numeric literals, and inspect overlapping matches. Database string-column lengths remain exempt from literal-value detection.

Regression cases cover quoted and numeric defaults, multiline calls, fallback expressions, and nested assignments.

## Validation

- `git diff --check 18106fd..23725db`
- `cargo test --locked --offline -p memoria-core --lib sensitivity::tests` — 11 passed.
- `cargo test --locked --offline -p memoria-storage --lib fulltext` — 4 passed.

Depends on #262 and is stacked on its branch. This diff contains only the follow-up credential-detection change. Merge #262 first, then retarget this PR to `main`.


Approved by: @
@zuyu
zuyu marked this pull request as draft October 3, 2026 21:17
@zuyu
zuyu force-pushed the codex/preserve-long-query-retrieval branch from cfc2874 to 18106fd Compare October 3, 2026 21:17
@zuyu
zuyu marked this pull request as ready for review October 3, 2026 21:17
@loveRhythm1990

Copy link
Copy Markdown
Collaborator

Review

I checked this out at 18106fd (plus 23725db from #264). The sensitivity::tests (11) and fulltext (4) unit tests pass locally, and CI is green.

The PR combines two independent changes: full-text batching and credential heuristics. My comments on each are below.

1. Full-text batching (store.rs)

The underline filter is a good, targeted fix. Deduplicating terms and using a deterministic tie-break are also nice touches.

a. Each batch fetches full rows, so cost scales poorly. search_fulltext_batch selects full rows, including content and embedding AS emb_str. In the hybrid path, fetch_k = limit * 3, which is 300 for top_k=100. A 3,000-term query becomes 47 sequential queries, each returning up to 300 full rows with embeddings, and most of those rows are discarded after the merge. Two options:

  • Select only memory_id, ft_score per batch, then load the final limit rows by ID.
  • Cap the number of batches, for example by keeping the rarest or most informative terms. For a 32 KB query, preserving every term has sharply diminishing returns.

Running batches concurrently, with a bound, would also help latency.

b. Fusion is approximate. Each batch contributes only its own top-limit rows. A memory that matches moderately across many batches can be cut from every batch, even though its summed score would rank it highly. That's an acceptable trade-off, but the comment "no query terms are lost" may read as "no candidates are lost". Please document the approximation.

c. The main source of very long queries is probably on the caller side. The AML leaderboard adapter (memory_benchmarks/agent_memory_leaderboard, Memoria.verify_searchable) sends the entire stored content as a search query after every write to prove it is searchable. That is likely what produces 32 KB queries in practice. Fixing the probe there (use a short query, or probe once per Add) would avoid paying this multi-batch cost on every write. This PR is still useful for long natural queries, such as coding-track issue descriptions. Tracked in matrixorigin/memory_benchmarks#1, item 5.

d. The multi-batch path has no DB-level test. The new tests cover only the pure helpers. A DB test should exercise:

  • the merged path: score summation, limit applied after the merge, and session, subject and type scope filters;
  • the empty-owner short-circuit.

The DB Tests job is available in CI for this.

e. Lowercasing now also applies to the single-batch path. That looks harmless if MatrixOne's full-text matching is case-insensitive, but please confirm.

2. Credential detection (sensitivity.rs)

The direction is right: false-positive blocks are costly. However, I'm concerned about the approach of enumerating benign syntaxes and word lists. I ran some probes against 23725db:

Input Blocked Expected
password = None yes no
def login(username, password=None): yes no
password: str / password: string; yes no
password = self.password yes no
password = getpass.getpass("Enter password: ") yes no
password = request.form.get("password", "") yes no
Password: must be at least 8 characters yes no
My secret: I love pineapple pizza yes no
Bearer bonds were common in the 1920s. yes no
Use the standard Bearer eyJhbGciOiJIUzI1NiJ9.abc.def header no yes
a bearer of eyJhbGciOiJIUzI1NiJ9.abc.def no yes

Very common code patterns are still blocked. Meanwhile, the special cases (standard|flag|torch|pall, of, a|the secret:) look fitted to specific failing samples, and they open small bypasses for real tokens.

There is a bigger design issue as well. A HIGH-tier match blocks the entire memory with 403, which callers treat as non-retryable. For conversation or code ingestion, a single password: str line drops the whole chunk and fails the write. This affects benchmark ingestion directly. Suggestions, in rough priority:

  1. Make the policy configurable (block / redact / off) per deployment or per owner. Evaluation and coding workloads need this regardless of how good the heuristics get.
  2. For the HIGH tier, prefer redacting the matched value over blocking the whole memory, so the surrounding context survives.
  3. If we keep detection, consider detecting by value shape (length, character-class mix, entropy, known prefixes such as eyJ, ghp_, sk-) instead of enumerating benign syntaxes. That approach generalizes, while syntax allow-lists do not.

There is also a related issue outside this PR's scope. The MEDIUM-tier phone and credit_card regexes redact any 10–19-digit number in place, including IDs, order numbers and millisecond timestamps. That silently changes stored evidence.

Suggestion

I'd be happy to merge the full-text part once there is a DB test, and ideally the cheaper per-batch projection. For the credential part, I'd prefer to discuss the configurable/redact design before investing further in heuristics. Splitting the two would let the full-text fix land independently.

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