Skip to content

fix(rag): refuse to index a document whose bytes cannot be read for the cache key - #6978

Open
tharunsridhar wants to merge 7 commits into
amd:mainfrom
tharunsridhar:fix/rag-content-key-unreadable
Open

tharunsridhar wants to merge 7 commits into
amd:mainfrom
tharunsridhar:fix/rag-content-key-unreadable

Conversation

@tharunsridhar

@tharunsridhar tharunsridhar commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

When RAG cannot read a file to hash it, _content_key falls back to a path-only <hash>_notfound key. index_document carried on with that key, so a PDF, DOCX, XLSX or PPTX (which their libraries open themselves) was extracted and cached under a key that never changes with the file's content. This makes index_document stop at that point instead.

Because index_document now refuses a file the SDK is not allowed to read, the callers that index a file the user names have to grant that file first. This PR also does that for quick_rag, the chat's /index command and /resume, and corrects the RAG SDK guide.

Why

RAGSDK._content_key returns sha256(path)_notfound on any OSError while hashing. The docstring says indexing "then fails at extraction", which holds for text formats but not for PDF, DOCX, XLSX and PPTX: _extract_text_from_pdf, _extract_text_from_pptx, _extract_text_from_xlsx and _extract_text_from_docx open the path directly, so extraction can succeed on a file hashing could not read. The chunks were then cached under a key that is the same before and after every edit, and every later session served the first version's text.

Linked issue

Closes #4800

Changes

The fix

  • src/gaia/rag/sdk.py: index_document returns an error (error_type: "file_unreadable", naming the file) when the content key is the _notfound fallback, before any cache is read or written. New helper _document_unhashable, shaped like _document_locked. The _content_key docstring now says what happens.
  • _content_key itself is unchanged. is_current() and freshness() read the _notfound key to report "unreadable" or "missing", and several existing tests assert it, so raising there (the issue's suggestion) would have changed all of them. Refusing at the one place that caches is the smaller change.

Callers that index a file the user names

Without allowed_paths the SDK only reads the working directory, and PDF, DOCX, XLSX and PPTX used to get past that because their parsers opened the path themselves. Each caller below now grants the file it was given, as AgentSDK._index_named_document and ChatAgent._index_documents already do.

  • quick_rag() (src/gaia/rag/sdk.py): grants the file it is given when the caller did not pass allowed_paths. An explicit allowed_paths is left alone.
  • /index <path> (hub/agents/gaia/python/gaia_agent/interactive.py): grants the named file before indexing it. For a folder it grants each file the folder holds, one at a time (not the folder), and skips symbolic links and protected files such as credentials.json. Granting resolves symlinks and does not run the protected-file check, so without the skip a link inside the folder could add its target to the session's file scope.
  • /resume <session> (hub/agents/gaia/python/gaia_agent/chat_agent.py, interactive.py): a file is added to indexed_files only if index_document succeeded, and the log says why a document was skipped. The chat now reports "N of M documents" loaded instead of counting what the session file recorded. /resume does not grant paths: a saved session file must not widen the read scope.

Docs

  • docs/sdk/sdks/rag.mdx: the security note no longer says that without allowed_paths RAG can index any file the process can read. The default scope is the working directory plus granted paths, for every format.

Tests

  • tests/unit/rag/test_cache_key.py: three tests. Hashing fails while extraction would succeed, so indexing is refused, nothing is cached under a _notfound name and the extractor is never called. The same across two sessions with an edit in between, so the first version's text can't come back. A readable document still indexes.
  • tests/unit/rag/test_quick_rag_scope.py (new): quick_rag on a file outside the working directory, and an explicit allowed_paths is left alone.
  • hub/agents/gaia/python/tests/test_index_grants_named_file.py (new): /index of a file outside the working folder, the grant covers that file and not its folder, /index of a folder, symlinks and protected files in a folder are skipped, /resume lists only documents that indexed, logs why one did not, does not grant a path from the session file, and reports "N of M".
  • tests/test_rag.py and tests/unit/rag/test_pdf_extraction_errors.py: these build an SDK with only a temp cache_dir and put their PDFs in a temp dir outside the default allowed paths (the current directory). They indexed only because of this bug, so their configs now set allowed_paths to the temp dir.

One behaviour change worth knowing: a PDF, DOCX, XLSX or PPTX outside the SDK's allowed paths is now refused like a text file already was, instead of being read by its parser. The callers above grant the file the user names, so they are unaffected.

Test plan

  • New tests fail on main and pass with the change (checked by reverting the source changes).
  • pytest tests/unit/rag tests/test_rag.py: 814 passed, 4 skipped.
  • pytest tests/unit/chat tests/unit/agents/test_read_file_documents.py tests/unit/agents/test_tool_scope.py (the other suites that use RAG): no failures.
  • pytest hub/agents/gaia/python/tests/test_index_grants_named_file.py (with PYTHONPATH=hub/agents/gaia/python): 10 passed.
  • black and isort clean on the touched files.
  • python util/lint.py --all: not run as a whole. Ran black, isort (8.0.1) and flake8 on the touched files, all clean.

Evidence

N/A: an internal change to RAG document indexing. It touches no Agent UI surface, MCP tool, CLI command or HTTP route. I reproduced the bug with a real PDF (hashing forced to fail, file edited, new session): upstream main still held the old text, and this branch refuses with file_unreadable and holds nothing.

Checklist

  • Linked issue above (Closes #4800).
  • Described why, not just what.
  • Linting/tests: see test plan.
  • Docs updated: docs/sdk/sdks/rag.mdx security note corrected (no other user-facing docs change).

…he cache key

When _content_key cannot read a file it falls back to a path-only
<hash>_notfound key. index_document carried on with that key, so a PDF,
DOCX, XLSX or PPTX (opened by their own libraries) could be extracted and
cached under a key that never changes with the file's content, and every
later session served the first version's text.

index_document now stops at that point with error_type "file_unreadable"
before any cache is read or written. _content_key is unchanged, since
is_current() and freshness() read the _notfound key.

Test configs that put documents outside the SDK's allowed paths only
indexed because of this bug; they now set allowed_paths to their temp dir.

Closes amd#4800
@github-actions github-actions Bot added rag RAG system changes tests Test changes performance Performance-critical changes labels Nov 8, 2026
@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Verdict: Request changes

This PR fixes a real bug: a PDF, Word, Excel or PowerPoint file that GAIA couldn't read for its fingerprint was still indexed, and every later session then served the first version's text. Indexing now stops with a clear "could not read" error. The fix is correct and well tested. It also closes a quiet gap: those four formats were read by their own parsers even outside the folders GAIA is allowed to read.

One caller wasn't updated to match:

  • quick_rag() now rejects any document outside the current folder. This one-call helper is in the public SDK docs. Until now a PDF anywhere on disk worked; after this change it raises "outside the folders GAIA is allowed to read", and a caller has no obvious fix. The chat agent, voice chat and the Agent UI already give read access to the file the user names. quick_rag should do the same (a two-line fix, below).
  • Follow-up: the RAG SDK guide still says that without allowed_paths, RAG can index any file the process can read. That was already wrong for text files. After this PR it's wrong for every format, because the default is the current folder.

Real-world evidence

N/A: there is no evidence bundle for this run. The PR says no user surface is touched and reports a manual reproduction with a real PDF. The verdict rests on static review. The tests couldn't run on this runner because pytest isn't installed.

🔍 Technical details

Issues

🟡 quick_rag regresses for documents outside the CWD (src/gaia/rag/sdk.py:4942). RAGConfig(**kwargs) with no allowed_paths builds a PathValidator(None), which is the CWD plus persisted grants. Before this PR, _safe_open refused the hash read, but pypdf opened the path itself, so quick_rag("/data/guide.pdf", ...) worked. Now index_document returns file_unreadable and quick_rag raises ValueError. Other callers already grant the named file: AgentSDK._index_named_document (src/gaia/chat/sdk.py:1258), ChatAgent._index_documents (chat_agent.py:3531) and the UI (_chat_helpers.py:5910). Mirror them:

    config = RAGConfig(**kwargs)
    rag = RAGSDK(config)
    # Naming the file is the grant to read it, as in AgentSDK._index_named_document.
    if "allowed_paths" not in kwargs and os.path.isfile(pdf_path):
        rag.path_validator.add_allowed_path(pdf_path)

Add a test with a PDF in tmp_path while the CWD is somewhere else. No existing test covers quick_rag outside the CWD.

🟡 Follow-up: doc drift (docs/sdk/sdks/rag.mdx:1063). "Without allowed_paths, RAG can index any file the process can read" contradicts PathValidator's default (CWD plus persisted grants). It's now false for PDF, DOCX, XLSX and PPTX as well. Suggested issue title: "rag.mdx: allowed_paths default is the working directory, not unrestricted".

Sibling paths checked

  • ChatAgent (gaia chat --index), AgentSDK.enable_rag (gaia talk --index) and the Agent UI upload path all grant the named file or its folder before indexing, so none of them regress.
  • messaging/ingest.py (Telegram) uses the default config with files under the system temp dir. That path was already refused for text files, and the downloaded name has no extension. This PR doesn't change it.
  • The eval retrieval harness passes explicit allowed_paths.

Strengths

  • The refusal sits at the single point that caches, so is_current() and freshness() can still read the _notfound key. That's a smaller change than raising inside _content_key.
  • The tests assert that the extractor is never called and nothing _notfound is written, and they cover the restart-then-edit case, which is the real bug.
  • The test configs now name allowed_paths, so they no longer pass only because of the bug.

index_document now refuses a document it cannot read for the cache key, and
without allowed_paths the SDK only reads the working directory. quick_rag()
would therefore reject a PDF, DOCX, XLSX or PPTX anywhere else, which worked
before only because the parser opened the path itself.

Grant the named file when the caller did not scope the SDK, as
AgentSDK._index_named_document does. An explicit allowed_paths is left alone.

Refs amd#4800
@kovtcharov-amd

Copy link
Copy Markdown
Collaborator

Thanks for closing the fingerprint gap. This now refuses readable PDFs in two gaia chat paths that index a file without granting it first, so they need a small follow-up before merge:

  • /index <path> refuses a readable PDF outside the working folder (its own help text shows an absolute path). Grant the named file first, the way _index_documents and quick_rag already do.
  • /resume <session> hits the same refusal, but ignores the result and still lists the file as indexed, so the chat says N documents are loaded when they aren't. Please check success before adding a file to indexed_files.
  • Docs and body: docs/sdk/sdks/rag.mdx still says that without allowed_paths RAG can index any readable file, which is no longer true, and the PR body doesn't mention the second commit (quick_rag granting its file).
🔍 Technical details
  • Reproduced: the same PDF indexes on main and returns file_unreadable on this branch through interactive.py's /index and ChatAgent.load_session (chat_agent.py around 3716–3720).
  • The locked-file, deleted-mid-index and empty-file cases all behave correctly, and the new tests fail without the fix.

index_document refuses a file outside the working folder unless it was
granted, so the chat's /index command rejected a readable PDF given by
absolute path. Grant the named file first, as _index_documents and
quick_rag do. A folder named to /index grants the files it holds.

/resume ignored the index result and still added the file to
indexed_files, so the chat reported documents as loaded that were not.
Check success before adding, log why a document was skipped, and report
how many of the session's documents are actually loaded. Resume does not
grant paths: a saved session file must not widen the read scope.

docs/sdk/sdks/rag.mdx no longer says RAG can index any readable file
without allowed_paths.

Refs amd#4800
@kovtcharov-amd

Copy link
Copy Markdown
Collaborator

Thanks — /index and /resume now behave correctly and the docs are fixed. One thing left before merge: the PR body becomes the commit message and still describes only the first commit. Please add the quick_rag grant, the /index grant, the /resume change ("N of M documents"), and the rag.mdx correction.

Optional hardening while you're there: /index <folder> grants each file through add_allowed_path, which resolves symlinks, so a link inside the folder could add its target to the session's file scope; and that path skips the protected-file check (e.g. credentials.json) that the agent's index_directory tool applies. Skipping symlinks and protected files there would close both.

Granting each file in a folder resolves symlinks and skips the protected-file
check that the agent's index_directory tool applies. A link inside the folder
could add its target to the session's file scope, and a file such as
credentials.json would be indexed.

Skip both in the folder loop of /index. Naming a single file is still the
user's explicit grant.

Refs amd#4800
@tharunsridhar

Copy link
Copy Markdown
Contributor Author

Thanks @kovtcharov-amd. The /index, /resume and docs items are addressed. The PR description now covers all commits (quick_rag and /index grants, /resume "N of M documents", the rag.mdx correction). I also took the optional hardening: /index <folder> now skips symlinks and protected files. The "Verify external URLs" failure is the same dead links that already fail on main, and #7001 fixes it.

SanketJadhav7d3 pushed a commit to SanketJadhav7d3/gaia that referenced this pull request Nov 9, 2026
…amd#7001)

The `Verify external URLs` job in Check Documentation Links fails on
every run, including `main`, because `docs/releases/v0.25.2.mdx` links
to three targets that now return HTTP 404:

- amd#5858
- amd#5859
- https://github.com/DineshKadiam

The PRs and the account no longer exist on GitHub, so the links cannot
be restored.

This removes the two PR links from the Docs bullet and keeps the
contributor credit as plain text without links or PR numbers.

Seen on the run for amd#6978 (fix/rag-content-key-unreadable) and the
earlier `main` run for amd#6990. The remaining `slack.com/signin` 403 is
only a warning and does not fail the job.

Verified locally with `python util/check_doc_links.py --internal-only`;
no other references to the removed URLs remain under `docs/`.
@kovtcharov-amd

Copy link
Copy Markdown
Collaborator

Thanks @tharunsridhar, and for taking the symlink and protected-file hardening too. The description now covers every commit, so nothing else is needed from you. I merged main into the branch; the RAG, quick_rag and /index grant tests pass on the merged result.

/index <file> now refuses credentials.json and other protected files the
same way /index <folder> skips them. The flagship changelog describes the
/index and /resume changes from this PR.
@kovtcharov-amd

Copy link
Copy Markdown
Collaborator

Thanks @tharunsridhar, careful work, especially the folder-index symlink and protected-file skip. I merged main, added the flagship changelog entry for the /index and /resume changes, and made /index <file> refuse a protected file such as credentials.json the same way the folder path already skips one, with a test.

This branch has not been deployed

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

Labels

documentation Documentation changes performance Performance-critical changes rag RAG system changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RAG caches some documents under a path-only key, so later edits are never picked up

2 participants