fix(memory): separate uploaded documents from facts - #532
Merged
Conversation
A spreadsheet upload was landing in shared memory as 29 "facts". The dashboard
showed `Awards ( 6 hours) NaN NaN 2.0 ... Total before Taxes NaN NaN NaN 34980`
under "Recent Shared Memory", and the same slices were injected into every
agent's system prompt via build_shared_context_block.
Three defects, one cause — a document chunk was being treated as a fact:
- `df.to_string()` is a console rendering. It emits "NaN" for blank cells, which
an LLM reads as a value, and truncates wide frames. Now `to_csv(na_rep="")`,
across every sheet rather than only the first.
- Chunking sliced on character count, so rows were cut mid-number and the header
appeared only in chunk 1 — every later chunk was unlabelled figures. Tabular
text now splits on line boundaries with the header repeated on each chunk.
- Chunks were stored with visibility="shared", so they were injected always and
crowded real facts off the dashboard. They now go to a `document_chunks`
collection, retrieved on demand through a new `search_documents` tool.
A fact asserts something ("Acme renewed in March"); a document chunk is raw
source text with no subject. They are different kinds and now live apart.
Existing uploads are not deleted: fact reads exclude the `file-upload` tag, and
POST /api/mongodb/documents/migrate-legacy moves them across, reusing their
embeddings so it costs no provider calls. Filtering happens after the vector
search rather than in the $vectorSearch filter, because Atlas only filters on
indexed fields and `tags` is not one — doing it there would error into the
recency fallback and lose ranking.
Also adds GET /documents, GET /documents/search, DELETE /documents/{filename}.
The new collection needs a vector index created by hand in Atlas; until it
exists, search falls back to recency so uploads stay reachable.
Frontend: @floating-ui/utils pinned and lockfile updated (an interrupted npm
install had left it with an empty dist, so @base-ui/react could not resolve it).
Verified: 250 tests pass (7 new). Removing the legacy filter fails exactly the
two injection tests, so they pin this regression rather than passing vacuously.
Extraction checked against a sheet with blanks and a totals row — no NaN in the
output; 120-row table chunks into 8 parts, every chunk headed, every row intact
and in order.
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
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
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.
A spreadsheet upload was landing in shared memory as 29 "facts". The dashboard showed
Awards ( 6 hours) NaN NaN 2.0 ... Total before Taxes NaN NaN NaN 34980under "Recent Shared Memory", and the same slices were injected into every agent's system prompt via build_shared_context_block.Three defects, one cause — a document chunk was being treated as a fact:
df.to_string()is a console rendering. It emits "NaN" for blank cells, which an LLM reads as a value, and truncates wide frames. Nowto_csv(na_rep=""), across every sheet rather than only the first.document_chunkscollection, retrieved on demand through a newsearch_documentstool.A fact asserts something ("Acme renewed in March"); a document chunk is raw source text with no subject. They are different kinds and now live apart.
Existing uploads are not deleted: fact reads exclude the
file-uploadtag, and POST /api/mongodb/documents/migrate-legacy moves them across, reusing their embeddings so it costs no provider calls. Filtering happens after the vector search rather than in the $vectorSearch filter, because Atlas only filters on indexed fields andtagsis not one — doing it there would error into the recency fallback and lose ranking.Also adds GET /documents, GET /documents/search, DELETE /documents/{filename}. The new collection needs a vector index created by hand in Atlas; until it exists, search falls back to recency so uploads stay reachable.
Frontend: @floating-ui/utils pinned and lockfile updated (an interrupted npm install had left it with an empty dist, so @base-ui/react could not resolve it).
Verified: 250 tests pass (7 new). Removing the legacy filter fails exactly the two injection tests, so they pin this regression rather than passing vacuously. Extraction checked against a sheet with blanks and a totals row — no NaN in the output; 120-row table chunks into 8 parts, every chunk headed, every row intact and in order.